fix(security): route sitemap-mode fetches through SSRF guard#55
Merged
Conversation
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>
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>
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>
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>
arberx
force-pushed
the
arberx/sitemap-ssrf-fix
branch
from
70daab2 to
430a7b4
Compare
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.
Hardens
--sitemapmode against two ways a malicious or compromised target can abuse the auditing host.SSRF (the original fix). The runner fetched the sitemap,
robots.txt, and — critically — every sitemap-index child<loc>via a rawfetch()with auto-follow redirects and no SSRF guard, so a public/sitemap.xmlthat 302s to169.254.169.254, an index listing an internal<loc>, or an internal host passed via--sitemapall reached cloud-metadata/internal services. Every sitemap/robots/child-<loc>fetch now routes through the samefetchWithValidatedRedirectsguard as the page fetch — hostname blocklist + DNS→private-IP rejection re-checked on every redirect hop, redirects followed manually.--allow-localstill permits only the single host you named (host-only, per-hop).DoS (fan-out). A sitemap index was fetched with
Promise.allover every child<loc>with no concurrency limit (the--limitslice only applies afterward), so a tens-of-thousands-child index fired that many simultaneous fetches. Child sitemaps are now fetched with bounded concurrency (5) and capped at the first 1000 per index, with the skipped count surfaced via a CLI notice (no silent truncation).Adds
test/sitemap-ssrf.test.tsandtest/sitemap-fanout.test.ts, threadsallowPrivateHostthrough discovery, and adapts existing discovery/rewrite tests to opt their loopback server past the guard. Bumps to 4.1.2 — no CLI, output, or scoring change. (Note: a separate stacked follow-up PR addresses DNS-rebinding TOCTOU via IP pinning, which is cross-cutting to the whole fetch layer.)🤖 Generated with Claude Code