chore: 2nd-pass audit catches — SSRF guard tests, theme_url scope, defense-in-depth#38
Open
heznpc wants to merge 1 commit into
Open
chore: 2nd-pass audit catches — SSRF guard tests, theme_url scope, defense-in-depth#38heznpc wants to merge 1 commit into
heznpc wants to merge 1 commit into
Conversation
…fense-in-depth Previously declared done; adversarial 2nd-pass caught the following: M1 — SECURITY.md (added in #36) promises four guards on the user-controlled fetch path (redirect:error, 5s timeout, content-length cap, body-length cap). theme-url.js had integration tests via an injected fetchImpl; posts.js had none. A refactor that dropped redirect:error from src/fetchers/posts.js would have failed zero existing tests. - Thread fetchImpl through fetchCapped so its wire behavior is testable without monkey-patching globalThis.fetch. - Add 6 wire-level tests in tests/posts-security.test.js covering each guard plus non-ok status surfacing. - Tighten fetchCapped: spread caller init BEFORE the security defaults so redirect:error and signal are non-overridable. Caller code today doesn't trigger this, but the order made the guard pointlessly weak. M2 — README:268 "theme_url Currently supported by /api/stats, /api/stack (Other endpoints will adopt as a follow-up)" was stale. Every card endpoint already routes through resolveCardOptions. Updated the "Currently supported by" paragraph and dropped the redundant "theme_url adoption across the catalog" item from the Planned block. Minor (inline TODOs only, no behavior change): - src/common/card.js: data-cas-target="${titleTarget}" — all current callers pass the literal "username", so safe today, but unescaped attribute interpolation is a latent regression vector if anyone ever threads user input through. TODO marker. - src/fetchers/posts.js: FETCH_TIMEOUT_MS is duplicated in theme-url.js with the same value. TODO marker to lift to common/utils.js if a third caller arrives. Out-of-scope (verified clean, no fix needed): - SVG XSS surface: every <text>${x}</text> traced; all escaped or numeric/internal-static. - CRLF in X-Theme-Error: blocked at both URL parser and setHeader layers (empirically verified on Node 22). - Secret leakage: git history -S clean for ghp_/BEGIN PRIVATE KEY etc. - eval/Function/vm/child_process: 0 occurrences. - Telemetry/tracking pixels: 0 third-party calls (README claim verified). - cards.test.js "8 asserts / 244 LOC" was a misread — the wrapper runs ~3700 effective assertions across themes × cards.
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
Adversarial 2nd-pass audit on the 'done' state from PRs #35/#36/#37.
Treats the prior 'all 158 tests pass / declared shipped' state as
verification target, not signal.
Findings
Critical
None. SSRF/XSS/CRLF/secret-leak/eval/exec all clean. cards.test.js has
~3700 effective assertions (8 grep-matches is a wrapper-pattern false
positive).
Major (fixed in this PR — previously declared done; 2nd-pass caught)
M1 — SSRF wire-level guards untested. SECURITY.md (#36) promises
redirect:error + 5s timeout + 2MB cap on
?source=rss&url=. The codehad them; the tests did not. A refactor dropping
redirect:errorwouldhave failed 0 tests. Fixed by threading
fetchImplthroughfetchCappedand adding 6 wire-level tests modelled ontheme-url.test.js. Also tightened init-spread order soredirect:errorandsignalare non-overridable by caller init —defense in depth.
M2 — README:268 theme_url scope stale. Claimed
"Currently supported by /api/stats, /api/stack (others as follow-up)";
actually every card endpoint routes through
resolveCardOptions.Updated paragraph + dropped now-redundant Planned item.
Minor (inline TODOs)
src/common/card.js— unescapeddata-cas-target="${titleTarget}".Safe today (all callers pass literal
"username"), latent if afuture caller threads user input. TODO marker.
src/fetchers/posts.js—FETCH_TIMEOUT_MSis duplicated intheme-url.js. TODO marker for lifting tocommon/utils.jsif athird caller appears.
Out-of-scope
Verified clean: SVG XSS interpolations, CRLF at URL parser + setHeader,
git-history secret -S scan, no eval/Function/vm/child_process, 0
third-party tracking pixels, GitHub token pool isolation.
Test plan
npm run check— cleannpm test— 164/164 (was 158/158 + 6 new)test (22)/test (24)green