fix(security): pin validated IP to close DNS-rebinding TOCTOU#56
Merged
Conversation
validatePublicRequestTarget resolved the host and checked its IPs, then handed the hostname to fetch(), which re-resolved at connect time. A low-TTL DNS record could therefore answer public during validation and a private IP (127.0.0.1, 169.254.169.254) at connect — on the initial request or any redirect hop — defeating the guard. Per-hop re-validation did not help: the same validate-then-reconnect gap exists on every hop. Outbound requests now go through an undici dispatcher whose connect-time DNS lookup (validatingLookup) returns ONLY public IPs and connects to exactly that address, so the IP that was validated is the IP connected to. The hostname is still used for TLS SNI and the Host header, so HTTPS certificate validation is unchanged. --allow-local keeps the default resolver for the single host the user named. Node's global fetch rejects a userland-undici dispatcher, so production requests use undici's own fetch; tests that stub globalThis.fetch are deferred to (pinning is a socket-layer concern they never reach), and dedicated pin tests exercise the real lookup over real sockets. Adds undici dependency and test/fetch-page-dns-pin.test.ts. Bumps to 4.1.3. No CLI, output, or scoring change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
arberx
added a commit
that referenced
this pull request
Jun 25, 2026
* fix(security): route sitemap-mode fetches through SSRF guard In --sitemap mode the runner fetched the sitemap, robots.txt, and every sitemap-index child <loc> via raw fetch() with auto-follow redirects and no SSRF guard, letting a malicious target steer the auditing host onto internal endpoints (cloud metadata at 169.254.169.254, internal services). Every sitemap / robots / child-<loc> fetch now routes through fetchWithValidatedRedirects: hostname blocklist + DNS->private-IP rejection re-checked on every redirect hop, with redirects followed manually. --allow-local continues to permit only the single host you named (host-only, per-hop), so a redirect or <loc> to any other private host stays blocked. Adds test/sitemap-ssrf.test.ts covering internal child <loc> rejection, 302 redirect-to-metadata blocking, and the allow-host opt-in. Bumps to 4.1.2. No CLI, output, or scoring change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(security): bound sitemap-index child fan-out (DoS) resolveSitemapUrls fetched every child <loc> of a sitemap index via Promise.all with no concurrency limit; the --limit slice only applies after all children are fetched. A malicious or misconfigured index listing tens of thousands of children therefore fired that many simultaneous fetches on the shared runner and accumulated entries unboundedly in memory. Child sitemaps are now fetched with the existing mapWithConcurrency at DEFAULT_CONCURRENCY (5) and capped at the first MAX_CHILD_SITEMAPS (1000) per index. When the cap drops children, resolveSitemapUrls reports the skipped count via SitemapAuditPlan.childSitemapsSkipped and the CLI prints a notice (no silent truncation). 1000 children is far more than any audit consumes, so legitimate sites are unaffected. Adds test/sitemap-fanout.test.ts (concurrency stays <=5; >1000 children are capped with the skipped count surfaced). No CLI, output, or scoring change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(security): pin validated IP to close DNS-rebinding TOCTOU (#56) validatePublicRequestTarget resolved the host and checked its IPs, then handed the hostname to fetch(), which re-resolved at connect time. A low-TTL DNS record could therefore answer public during validation and a private IP (127.0.0.1, 169.254.169.254) at connect — on the initial request or any redirect hop — defeating the guard. Per-hop re-validation did not help: the same validate-then-reconnect gap exists on every hop. Outbound requests now go through an undici dispatcher whose connect-time DNS lookup (validatingLookup) returns ONLY public IPs and connects to exactly that address, so the IP that was validated is the IP connected to. The hostname is still used for TLS SNI and the Host header, so HTTPS certificate validation is unchanged. --allow-local keeps the default resolver for the single host the user named. Node's global fetch rejects a userland-undici dispatcher, so production requests use undici's own fetch; tests that stub globalThis.fetch are deferred to (pinning is a socket-layer concern they never reach), and dedicated pin tests exercise the real lookup over real sockets. Adds undici dependency and test/fetch-page-dns-pin.test.ts. Bumps to 4.1.3. No CLI, output, or scoring change. Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> * fix(ci): pin undici to v7 for Node 20 support undici 8.x requires Node >=22.19.0, but the package targets Node >=20 (engines) and CI tests on Node 20. On Node 20, undici 8.5.0 crashes at module load ("webidl.util.markAsUncloneable is not a function"), so every file importing fetch-page failed and the test job went red. A published 4.1.3 would crash for Node 20 users the same way. Downgrade to undici ^7.22.0 (engines: Node >=20.18.1), which exposes the same fetch / Agent / connect.lookup API the SSRF DNS-pinning dispatcher uses. Typecheck, the full vitest suite, and the real-socket pin tests pass. Also fix test/e2e/cli.test.ts, which the undici crash was masking: it imported the compiled CLI lazily inside the first test, AFTER installMockFetch ran. fetch-page.ts captures globalThis.fetch as `builtinFetch` at module load and only defers to a stub when the live global differs, so capturing the mock made the guard fall through to a real pinned undici fetch (offline: error; CI: real 1.1.1.1). Warm the module in a before() hook — before any mock — so builtinFetch is the real fetch and the per-test mocks are honored. Kept as a runtime dynamic import since dist/ is a gitignored artifact absent when typecheck runs (a static import broke typecheck, which runs before build). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
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.
Stacked on #55 (base is
arberx/sitemap-ssrf-fix, notmain) — review/merge that first; GitHub will retarget this tomainonce it lands.Closes the DNS-rebinding TOCTOU that survives #55. The guard resolved the host and checked its IPs, then handed the hostname to
fetch(), which re-resolved at connect time — so a low-TTL record could answer public during validation and a private IP (127.0.0.1,169.254.169.254) at connect, on the initial request or any redirect hop. Per-hop re-validation does not help; the validate-then-reconnect gap exists on every hop.Outbound requests now go through an
undicidispatcher whose connect-time DNS lookup returns only public IPs and connects to exactly that address, so the IP validated is the IP connected to. The hostname is still used for TLS SNI and theHostheader, so HTTPS cert validation is unchanged;--allow-localkeeps the default resolver for the single host you named. Because Node's globalfetchrejects an external-undici dispatcher, production uses undici's ownfetch; tests that stubglobalThis.fetchare deferred to (pinning is a socket-layer concern they never reach) and dedicated tests exercise the real lookup over real sockets.Adds
undicias a dependency andtest/fetch-page-dns-pin.test.ts. Verified end-to-end against live sites (HTTPS fetch, an http→https redirect chain, and a real sitemap audit). Bumps to 4.1.3 — no CLI, output, or scoring change.🤖 Generated with Claude Code