Project-wide code review: fix crash/date/XSS bugs, refactor for maintainability, add CONTRIBUTING.md - #23
Merged
Conversation
Four-digit years now pass through unchanged (1999 previously became
3999 via the unconditional 2000+offset normalisation) and two-digit
years follow the RFC 2822 §4.3 century rule (00-49 → 2000s, 50-99 →
1900s; "99" previously became 2099). The century rule is resolved at
parse time inside the year combinator, so the post-processing footgun
is gone entirely.
parse/1 now always returns {:ok, t} | {:error, reason} instead of
leaking NimbleParsec's six-element error tuple, and both functions
gained specs.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… into Req
Task.async_stream ran without on_timeout: :kill_task, so a single feed
exceeding feed_timeout exited the calling process and took the whole
build down — the {:exit, _} fallback branch was unreachable. Timed-out
feeds are now killed and dropped with a warning naming the feed URL.
feed_timeout is now also enforced at the HTTP layer as Req's
:receive_timeout (it previously only bounded the task; the config
argument to fetch_body/2 was ignored). The task-level timeout gets a
1s grace period so the HTTP timeout fires first and the parser can
still fall back to a cached body.
Also in Exoplanet.Parser:
- The app env key for extra Req options is now :req_options, with the
stale-named :planet_req_options kept as a deprecated fallback.
- Optional cache callbacks are probed with Code.ensure_loaded?/1 first,
so they are not silently skipped in dev/interactive mode.
- normalize_categories/1 renamed to clean_categories/1 to stop
colliding with the unrelated Exoplanet.Filters.normalize_categories/1.
Exoplanet.build/1 refactor: the per-source pipeline is now a named
build_source/3, the duplicated sort expression lives in
sort_by_published_desc/1, and Config.from_file/1 uses Filters.merge/2
so both entry points share one canonical defaults path.
New tests: feed-timeout drop, per-source filter overrides through
build/1, the global items cap, and the cache adapter on_success/2 and
on_error/3 notification callbacks.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…lt sanitizer With sanitize_html: true (the default), the sanitizer only dropped exact-name tags and attributes, so javascript: hrefs and onerror/onclick handlers passed straight through to consumers rendering post bodies with raw/1. The tree walk now also removes: - any attribute whose name starts with "on" (event handlers), and - href/src/srcset attributes whose URL scheme is not http, https, or mailto (relative URLs are kept). ASCII control characters and whitespace are stripped before the scheme check so "java\nscript:" obfuscation doesn't slip past, and each srcset candidate is checked individually. The src forwarded into the strip_images link replacement passes the same check, so image stripping can't smuggle an unsafe URL into a clickable <a href>. walk_node/2 now takes a single opts map instead of three positional flags, keeping the non-sanitizing strip_images path byte-identical for safe content. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- CONTRIBUTING.md: development setup, mix precommit, test conventions (Req.Test stubs + fixtures layout), the DateTimeParser regeneration workflow, and how to submit changes / report issues. - README: bump install snippet to ~> 0.5, document :req_options and the feed_timeout → :receive_timeout wiring, state the sanitizer's scope and the Code.eval_file/1 trust note, link CONTRIBUTING.md. - CLAUDE.md: fix two stale claims (unknown config keys are ignored, not raised; fixtures live in test/support/fixtures/feeds/, not inline) and document the new timeout/sanitizer/year semantics plus the exact parser regeneration command. - mix.exs: source_ref so HexDocs source links point at the release tag, CONTRIBUTING.md shipped as a docs extra and in the package files, skip_code_autolink_to for the hidden modules referenced from extras. - CHANGELOG: unreleased entries for all of the above. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- Disable Req's automatic retries by default in the feed fetch. With the
feed_timeout task backstop a retried request could never finish, and
the retry delay prevented the {:error, _} branch (and therefore the
cached-body fallback) from ever running for slow feeds. Consumers can
re-enable retries via :req_options.
- Log a one-time deprecation warning when the legacy :planet_req_options
key is used, and add a regression test covering the fallback path.
- Extend the sanitizer's URL-attribute scheme check to action,
formaction, poster, and xlink:href (verified bypass vectors for the
previous href/src/srcset set).
- Downcase user-supplied drop_attrs entries so drop_attrs: ["Style"]
matches the lowercased attribute names.
- Pin DateTimeParser.parse/1's {:ok, t} | {:error, reason} contract with
a direct test (previously only exercised through parse!/1).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
milmazz
commented
Jun 13, 2026
Replace the manual start()/stop() + on_exit teardown for the in-process cache adapters with ExUnit's start_supervised!/2. The adapters now expose child_spec/1 + start_link/1 (linked, so they can be supervised), and ExUnit owns teardown — terminating the process and freeing its registered name before the next test, removing the named-process race the manual pattern relied on async: false to avoid. Addresses review feedback on PR #23. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
11 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
A senior-review pass over the whole project surfaced two verified correctness bugs, an XSS-permissive default sanitizer, and several maintainability/contributor-friendliness gaps. This PR fixes all Critical and Important findings, applies the moderate refactors, and adds contributing guidelines.
Bug fixes
Task.async_streamran withouton_timeout: :kill_task, so a feed exceedingfeed_timeoutexited the calling process (the_ -> []fallback was unreachable). Slow feeds are now killed and dropped, with a warning naming the feed URL.2000 + offsetyear normalisation turned 1999 into 3999 and"99"into 2099 — corrupted dates then won the descending sort. Four-digit years now pass through; two-digit years follow the RFC 2822 century rule (00–49 → 2000s, 50–99 → 1900s), resolved inside the parsec year combinator.feed_timeoutis now enforced at the HTTP layer (Req:receive_timeout); previously it only bounded the task and theconfigargument tofetch_body/2was ignored. Req retries are off by default so the cached-body fallback can actually run (re-enable via:req_options).Code.ensure_loaded?/1, so they aren't silently skipped in dev/interactive mode.Security
With
sanitize_html: true(the default),javascript:hrefs andon*event handlers previously passed straight through. The sanitizer now also dropson*attributes and URL-bearing attributes (href,src,srcset,action,formaction,poster,xlink:href) whose scheme isn'thttp/https/mailto(relative URLs kept; control-char obfuscation likejava\nscript:handled; eachsrcsetcandidate checked;strip_imagescan't smuggle an unsafesrcinto the replacement link). Scope is documented as defense-in-depth in the module docs and README.Maintainability refactors
Exoplanet.build/1reads as named stages:build_source/3+sort_by_published_desc/1; timeout warnings name the feed via a zip with the source list.Config.from_file/1andbuild/1share one canonical defaults path (Filters.merge/2);nilfilter values now consistently keep library defaults.Parser's privatenormalize_categories/1renamedclean_categories/1(collided with the unrelatedFilters.normalize_categories/1).:planet_req_options→:req_options(deprecated fallback + one-time warning).DateTimeParser.parse/1returns a consistent{:ok, t} | {:error, reason}.Contributor-friendliness
CONTRIBUTING.md: setup,mix precommit, test conventions (Req.Test stubs + fixtures), the DateTimeParser regeneration workflow (mix nimble_parsec.compile), and submission guidelines. Shipped as an ExDoc extra and linked from the README.test/support/fixtures/feeds/).~> 0.5,:req_options+ sanitizer scope +Code.eval_file/1trust note documented.mix.exs:source_refso HexDocs source links target the release tag.Tests
20 new tests (100 → 120): feed-timeout drop, per-source filter overrides through
build/1, globalitemscap, cacheon_success/on_errorcallbacks, century-rule boundaries, sanitizer bypass vectors, the deprecated-key fallback.mix precommitis fully green.A follow-up reviewer pass on this branch confirmed the four agreed behaviors and probed the sanitizer adversarially (obfuscated schemes, casing) — both Important findings from that pass are folded into the last commit.
Suggested follow-ups (out of scope here)
Exoplanet.Parserinto fetcher (HTTP + cache) and pure parser modules — highest-value structural refactor, kept out to keep this PR reviewable.CODE_OF_CONDUCT.md, issue/PR templates,SECURITY.md, Credo/Dialyzer in CI.🤖 Generated with Claude Code