chore: 2nd-pass audit catches — SSRF guard tests, theme_url scope, defense-in-depth#38
Merged
Merged
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.
Pinned each finding from the /code-review pass on this branch. Tests
grew from 164 → 183.
P0 (production-broken)
- Hashnode source disabled: gql.hashnode.com retired 2026-05 (verified
live: 301 to retirement announcement). source=hashnode now returns a
clean "use source=rss against your blog's /rss feed" error instead of
silently failing the fetch. Removed gql.hashnode.com from the host
allowlist. README and catalog updated; SECURITY.md notes the disablement.
- theme-url.js timer now covers the body read. clearTimeout had been
in an inner finally, leaving `await res.text()` unprotected — the
exact pattern posts.js's own comment warns about.
Hardening
- fetchCapped + theme-url.js fetcher: streaming byte-count cap via
res.body.getReader(). Cap is now enforced in bytes (not text.length
UTF-16 code units), and aborts mid-stream so chunked / no-content-
length responses cannot OOM the function before the post-read check.
- content-length sanitization: Number.isInteger + non-negative gate so
`Content-Length: -1` from a buggy/hostile upstream can't bypass the
pre-read cap by exploiting Number('-1') being finite.
- AbortError accuracy: catch now checks controller.signal.aborted
before translating to "timed out" — Undici can surface AbortError for
non-timer reasons (mid-stream server reset) and those should propagate
with their own message.
- fetchCapped tolerates `null` and `{ fetchImpl: null }` for the third
options arg (defaults only fire on `undefined`); falls back to
globalThis.fetch in both cases.
- theme-url.js gained MAX_BODY_BYTES (256 KB ceiling for theme JSON).
- Both fetch wrappers gained a `timeoutMs` test-override so unit tests
can drive the timer-induced abort path without waiting the real 5s.
Catalog / discovery
- catalog.js now injects UNIVERSAL_PARAMS (theme, theme_url, font,
*_color, hide_*, border_radius, card_width) into every card's
common_params at response time. The MCP wrapper and any other
discovery client now sees the same surface README claims.
- The catalog description for /api/posts notes the hashnode retirement.
/api/stack N+1 dedup
- resolveCardOptions gained a `prefetched` option. stack.js resolves
the top-level theme_url ONCE via the new prefetchExternalTheme helper
and passes the result to every child slot. A child overriding with
`<card>.theme_url=<different>` falls through to the normal fetch path.
Test coverage
- Switched test mocks from `new Map()` to real `new Headers()` so the
case-insensitive get path production uses is actually exercised.
- Added a text() spy on the content-length cap test — pins the pre-read
contract (the cap must reject BEFORE the body is materialized).
- Added signal non-overridability test (the source comment claimed it;
no test enforced it).
- Added byte-vs-char body cap test using multibyte emoji.
- Added timer-induced vs upstream-surfaced AbortError tests.
- Added null / { fetchImpl: null } options-bag tests.
- Added catalog.test.js — every card carries theme_url, no duplicates,
utility endpoints don't inherit card params, hashnode retirement
signaled in description.
- Added prefetched-palette tests for resolveCardOptions in theme-url.test.js.
Docs
- SECURITY.md: surfaces aligned with README ("every card endpoint" via
the shared resolver), wire-guard description updated to streaming
byte cap, hashnode added to out-of-scope.
- README: dropped 'hashnode' from /api/posts source list with a note
pointing users at source=rss against the Hashnode RSS feed.
heznpc
force-pushed
the
chore/2nd-pass-audit-2026-05-21
branch
from
d21e08a to
3366b53
Compare
heznpc
added a commit
that referenced
this pull request
May 28, 2026
Acted on the two ProfileKit-local items from /heznpc-market-pulse:
Major — Related projects comparison in README
- Added a "Related projects" section comparing ProfileKit against
anuraghazra/github-readme-stats (dominant, ~79.5k★, last commit
2026-01-18) and lowlighter/metrics (~16.7k★, push 2026-05-22, the
active 300+-option direct competitor). Differences framed as
stylistic, not better/worse — composition unit, update model,
configuration, runtime deps, card set, MCP integration. Includes
"when ProfileKit fits well" / "when alternatives fit better" so the
comparison stays honest.
Minor — response-header regression test
- New tests/endpoint-headers.test.js covers:
· /api/divider — Content-Type: image/svg+xml + Cache-Control max-age
(Camo cache depends on both)
· /api/health — application/json + Cache-Control: no-store (probes
must observe current pool state, not a snapshot)
· /api/catalog — application/json + max-age (MCP discovery hits this;
no caching = hammered function)
Also cross-checks the README/catalog contract that theme_url appears
in hero.common_params — closes the loop with the catalog test added
in #38.
Out of scope (verified clean, no fix needed):
- vercel.json + README scan for "edge runtime" mentions — none found.
Vercel Edge Functions deprecation (2026-04-08) does not affect this
repo because the functions are already Node serverless.
Out of scope (separately, different repo):
- MCP wrapper plugin-directory submission + "third-party, not Anthropic
Verified" disclaimer — lives in MCP/profilekit-mcp, not here.
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,
then a
/code-reviewadversarial sweep on this PR's own first commit.Together they caught 15 defects the first pass missed (including 2 P0).
Test count: 183/183 (was 158 before this branch, 164 after the first
commit on this branch, +19 in this revision).
Findings acted on
P0 (production-broken)
gql.hashnode.comreturns 301 to retirement page(verified live).
source=hashnodeon/api/postshas been silentlyfailing every request. Disabled with a clean "use source=rss" error.
clearTimeoutwasin an inner finally, leaving
await res.text()unprotected — theexact pattern posts.js's comment warns about. Fixed by restructuring
the try block to wrap both fetch + body read.
Major
fetchCapped+ theme-url fetcher: streaming byte-count cap viares.body.getReader(). Cap is now in real bytes (not UTF-16 chars)and aborts mid-stream so chunked / no-content-length responses
cannot OOM the function before the post-read check.
Content-Length: -1can no longer skip the pre-read cap.
controller.signal.abortedbeforetranslating to "timed out". Undici-side resets (mid-stream) now
propagate with their own message.
fetchCappedtoleratesnull/{ fetchImpl: null }for thethird options arg (defaults only fire on
undefined).theme-url.jsgainedMAX_BODY_BYTES(256 KB).timeoutMstest override.color, hide, border_radius, card_width) into every card.
MCP wrapper and external discovery clients now match README claim.
/api/stackresolves the top-leveltheme_urlonce via the newprefetchExternalThemehelper and passes it to every child slot.A child overriding with
<card>.theme_url=<different>stilltriggers a live fetch.
Test fidelity
Mapto realHeaders(case-insensitive.get).contract (body must NOT be materialized).
retirement signaled in description).
Docs
description, hashnode added to out-of-scope.
hashnodefrom posts source list with a notepointing users at
source=rssagainst the Hashnode RSS feed.Test plan
npm run check— cleannpm test— 183/183test (22)/test (24)green