Harden ssrf fetch time resolve#210
Open
spartan8806 wants to merge 3 commits into
Open
Conversation
…only DNS bypass)
`guardFetchUrl` validates only the literal hostname (the code documents this as the
v0.3.0 contract, with a fetch-time re-check noted as a follow-up). Because the fetch
layer resolves DNS itself, a public hostname whose A/AAAA record points at a blocked
address — cloud metadata (169.254.169.254), an RFC-1918 host, or a loopback service in
serve mode — passes the input guard and is only caught once resolved. This defeats the
"always block metadata" and serve-mode loopback protections.
- Add `guardResolvedHost(hostname, field, { allowPrivate, lookup })`: resolves the host
and runs EVERY resolved address back through `guardFetchUrl` (as an `http://<ip>/`
literal), so the resolved-IP policy is identical to the literal-IP policy — no drift.
`lookup` is injectable for tests; defaults to `node:dns`.
- Wire it into the http-client redirect loop (runs before each hop's connect; skips
literal-IP hosts, already validated).
- Tests: metadata / private / IPv6-ULA resolutions are blocked, public + allowPrivate
cases pass, any-bad-address in a multi-record answer rejects, NXDOMAIN rejects.
Note: this closes the static-record bypass (the metadata-credential-theft case). Full
rebinding (TOCTOU) safety additionally wants the connection pinned to the validated IP
(a `lookup` hook / custom dispatcher) and the same check wired into the remaining fetch
seams (daemon dispatch, firecrawl shim, browser path) — happy to follow up on those.
…serve seam Phase-0 (c7168ab) added guardResolvedHost and the http-client input hop. This wires the resolved re-check into the remaining seams so a public hostname whose DNS record points at cloud metadata / RFC-1918 / (in serve mode) loopback cannot slip past the literal-only guard anywhere: - http-client: thread allowPrivate from the call site's opts (was re-reading config directly) so the resolved policy tracks the literal one - router (defaultPdfProbe) + tls-tier redirect loops: resolved check at hop 0 and every redirect Location - escape-hatch: shared resolvedGuardOk helper at all 5 guardFetchUrl sites, each threading that site's own allowPrivate (sidecar true, target config); lookup injectable for tests - dispatch + firecrawl-compat serve seams: guardResolvedServeTarget with bindIsLoopback, so a name resolving to loopback is refused under a non-loopback bind (guardResolvedHost still allows loopback for plain fetch / local dev) - browser-pool: pre-goto check-only guard; comment notes it does NOT close DNS rebinding (Chromium re-resolves) — pinning tracked in KnockOutEZ#207 Also fixes a latent bug this surfaced: guardFetchUrl's docstring promised the IPv6 loopback (::1) exemption mirroring 127.0.0.0/8, but it was never implemented, so a dual-stack localhost (127.0.0.1 + ::1) was wrongly blocked once the resolved re-check feeds each IP back through guardFetchUrl. Added the narrow ::1 exemption; serve-mode ::1 refusal still applies via the serve guard. New tests (all via injected lookup): guardResolvedServeTarget matrix (metadata / RFC-1918 ±allowPrivate / loopback under serve non-loopback bind / loopback allowed under loopback bind / WIGOLO_SERVE_ALLOW_LOCAL_TARGETS / multi-record / non-resolving) plus the guardResolvedHost loopback-allowed case.
…ral IP) The existing redirect tests refuse a 302 into a literal 169.254.169.254 — that's the literal guard. This adds the resolved counterpart: a 302 to an ordinary public-looking hostname that DNS-resolves to metadata, which only the fetch-time resolved re-check on the redirect hop can catch. Host-aware injected lookup so only the redirect target resolves to metadata; hop 0 stays public. Asserts the hop is refused before the second fetch fires.
📝 WalkthroughWalkthroughThe PR adds DNS-resolved SSRF rechecks for serve and fetch paths, applies them across dispatch, compatibility, browser, redirect, TLS, and escape-hatch flows, restores IPv6 loopback handling, and adds resolution, redirect, and deterministic DNS tests. ChangesDNS-Resolved SSRF Validation
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (4)
tests/watch/ssrf-resolved-serve-target.test.ts (1)
76-82: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd an
::1counterpart for thebindIsLoopback:trueallow-path.IPv4 loopback allowance under
bindIsLoopback:trueis tested here, but the IPv6::1exemption — which the PR objectives call out specifically ("implements the documented::1loopback exemption") — has no matching test. SinceisResolvedLoopbacklikely special-cases::1textually (distinct from the IPv4 CIDR-style check), this is the branch most likely to silently regress.Suggested additional test
it('allows a hostname that resolves to loopback when the server itself is bound to loopback', async () => { const r = await guardResolvedServeTarget('localhost.evil.example', 'url', { bindIsLoopback: true, lookup: mockLookup([{ address: '127.0.0.1', family: 4 }]), }); expect(r.ok).toBe(true); }); + + it('allows a hostname that resolves to IPv6 loopback (::1) when the server itself is bound to loopback', async () => { + const r = await guardResolvedServeTarget('localhost6.evil.example', 'url', { + bindIsLoopback: true, + lookup: mockLookup([{ address: '::1', family: 6 }]), + }); + expect(r.ok).toBe(true); + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/watch/ssrf-resolved-serve-target.test.ts` around lines 76 - 82, Add a matching test beside the existing bindIsLoopback IPv4 allow-path test, using guardResolvedServeTarget with bindIsLoopback: true and mockLookup returning the IPv6 loopback address ::1, then assert the result is allowed. Keep the hostname and other setup consistent with the existing test.tests/unit/fetch/browser-pool.challenge.test.ts (1)
75-89: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate
node:dnsstabilization mock across both browser-pool test files.Both files add the identical
vi.mock('node:dns', ...)block (same fixed address, same explanatory comment) to keepguardResolvedHost's pre-navigation check deterministic under fake timers. Extracting this into a shared test helper/fixture avoids the two copies drifting out of sync.
tests/unit/fetch/browser-pool.challenge.test.ts#L75-L89: replace with an import of a sharedmockNodeDnsLookup()helper (or a commonvi.mock('node:dns', ...)setup module).tests/unit/fetch/browser-pool.clearance.test.ts#L84-L98: same — replace with the same shared helper instead of a second copy of the mock.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/fetch/browser-pool.challenge.test.ts` around lines 75 - 89, The identical node:dns stabilization mock is duplicated across both browser-pool test files. Create one shared mock helper or setup module containing the lookup behavior and explanatory context, then replace the blocks at tests/unit/fetch/browser-pool.challenge.test.ts:75-89 and tests/unit/fetch/browser-pool.clearance.test.ts:84-98 with usage of that shared helper, preserving the synchronous benign-IP resolution and fake-timer behavior.src/watch/ssrf.ts (1)
393-408: 🩺 Stability & Availability | 🔵 TrivialConsider bounding and/or reusing the resolver lookup.
resolveAllissues a freshdns.lookupwith no timeout of its own; since it runs on the request path just before the real fetch (which resolves again), every guarded fetch/redirect hop pays a second, uncached getaddrinfo bounded only by the OS resolver — not by the caller's per-requestAbortSignal.timeout. A slow/hung resolver can therefore stall a request past its intended deadline. Consider racing the lookup against the caller's signal/timeout, and note the duplicate-resolution cost.This is inherent to the check-then-connect design (full rebinding safety is tracked separately in
#207); flagging only the unbounded-latency and double-resolution aspects.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/watch/ssrf.ts` around lines 393 - 408, Update resolveAll to honor the caller’s AbortSignal or request timeout by racing the DNS lookup against it and returning null when the deadline is reached; thread that signal through each guarded fetch/redirect call. Reuse the resolved addresses for the subsequent connection where the existing design permits, or otherwise avoid introducing additional resolver calls beyond the current check-then-connect flow.src/fetch/escape-hatch.ts (1)
57-59: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCentralize the duplicated IP-literal detection. The same
/^\d{1,3}(\.\d{1,3}){3}$/.test(host) || host.includes(':')skip-literal test is re-implemented at seven sites gating the resolved-host recheck. Export one helper (e.g. fromsrc/watch/ssrf.ts, or promote the existingisIpLiteralHost) so this security gate has a single source of truth.
src/fetch/escape-hatch.ts#L57-L59: exportisIpLiteralHost(or move it intossrf.ts) as the shared implementation.src/daemon/rest/dispatch.ts#L77-L77: replace the inlineisIpLiteralexpression with the shared helper.src/daemon/rest/firecrawl-compat.ts#L240-L240: replace the inlineisIpLiteralexpression with the shared helper.src/fetch/http-client.ts#L171-L171: replace the inlineisIpLiteralexpression with the shared helper.src/fetch/router.ts#L328-L328: replace the inlineisIpLiteralexpression with the shared helper.src/fetch/tls-tier.ts#L316-L316: replace the inlineisIpLiteralexpression with the shared helper.src/fetch/browser-pool.ts#L568-L568: replace the inlineisIpLiteralexpression with the shared helper.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/fetch/escape-hatch.ts` around lines 57 - 59, Centralize the IP-literal check by exporting the existing isIpLiteralHost helper from src/fetch/escape-hatch.ts (or moving it to the shared SSRF module), preserving its current detection behavior. Replace the duplicated inline checks with this helper in src/daemon/rest/dispatch.ts:77, src/daemon/rest/firecrawl-compat.ts:240, src/fetch/http-client.ts:171, src/fetch/router.ts:328, src/fetch/tls-tier.ts:316, and src/fetch/browser-pool.ts:568.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/fetch/escape-hatch.ts`:
- Around line 57-59: Centralize the IP-literal check by exporting the existing
isIpLiteralHost helper from src/fetch/escape-hatch.ts (or moving it to the
shared SSRF module), preserving its current detection behavior. Replace the
duplicated inline checks with this helper in src/daemon/rest/dispatch.ts:77,
src/daemon/rest/firecrawl-compat.ts:240, src/fetch/http-client.ts:171,
src/fetch/router.ts:328, src/fetch/tls-tier.ts:316, and
src/fetch/browser-pool.ts:568.
In `@src/watch/ssrf.ts`:
- Around line 393-408: Update resolveAll to honor the caller’s AbortSignal or
request timeout by racing the DNS lookup against it and returning null when the
deadline is reached; thread that signal through each guarded fetch/redirect
call. Reuse the resolved addresses for the subsequent connection where the
existing design permits, or otherwise avoid introducing additional resolver
calls beyond the current check-then-connect flow.
In `@tests/unit/fetch/browser-pool.challenge.test.ts`:
- Around line 75-89: The identical node:dns stabilization mock is duplicated
across both browser-pool test files. Create one shared mock helper or setup
module containing the lookup behavior and explanatory context, then replace the
blocks at tests/unit/fetch/browser-pool.challenge.test.ts:75-89 and
tests/unit/fetch/browser-pool.clearance.test.ts:84-98 with usage of that shared
helper, preserving the synchronous benign-IP resolution and fake-timer behavior.
In `@tests/watch/ssrf-resolved-serve-target.test.ts`:
- Around line 76-82: Add a matching test beside the existing bindIsLoopback IPv4
allow-path test, using guardResolvedServeTarget with bindIsLoopback: true and
mockLookup returning the IPv6 loopback address ::1, then assert the result is
allowed. Keep the hostname and other setup consistent with the existing test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 986b11a2-22bb-4adf-95a8-377b67a1bd95
📒 Files selected for processing (13)
src/daemon/rest/dispatch.tssrc/daemon/rest/firecrawl-compat.tssrc/fetch/browser-pool.tssrc/fetch/escape-hatch.tssrc/fetch/http-client.tssrc/fetch/router.tssrc/fetch/tls-tier.tssrc/watch/ssrf.tstests/unit/fetch/browser-pool.challenge.test.tstests/unit/fetch/browser-pool.clearance.test.tstests/unit/fetch/escape-hatch.test.tstests/watch/ssrf-resolved-host.test.tstests/watch/ssrf-resolved-serve-target.test.ts
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.
Description:
What & why
guardFetchUrlonly validated the literal hostname — it never resolved DNS. So apublic hostname whose A/AAAA record points at cloud metadata (
169.254.169.254), anRFC-1918 address, or (in serve mode) loopback passed the guard and was only ever stopped,
if at all, by the OS at connect time. This resolves the host at fetch time and runs every
resolved IP back through
guardFetchUrl, closing the static-record SSRF bypass. Phase-1guard (
guardResolvedHost). Unresolvable hosts fall through (no IP to reach — not a bypass).guardResolvedServeTarget): under a non-loopback bind, refuse a namethat resolves to loopback; plain fetch still allows it for local dev.
loops, escape-hatch (all sites incl. solver/reader sidecars), the dispatch and firecrawl-compat
serve seams, and the browser tier (pre-nav, check-only — see Scope; it does not close
rebinding). Per the Scope note, none of these close active rebinding — that's SSRF follow-up: pin the socket to the validated IP (DNS rebinding / TOCTOU) #207.
allowPrivatethreaded from each call site's own opts rather than re-defaulted.guardFetchUrl's docstring promised an::1exemption(mirroring
127.0.0.0/8) that was never implemented, so dual-stacklocalhostwouldbreak once resolved IPs are fed back through it. Narrow exemption added; serve-mode
::1refusal is unaffected. Called out so it's not a surprise in the diff.
lookup, no real DNS): metadata / RFC-1918 ±allowPrivate / loopbackserve-vs-fetch / bind-loopback / env override / multi-record / non-resolving, plus a *
target is a hostname that resolves to metadata → blocked** (the resolved counterpart of the
existing literal-IP 302 cases).
Testing
npm testpasses — for the surface this touches:tests/unit/fetch+tests/watch(43 files / 608 tests) and
rest-tools.test.ts(21/21, incl. "SSRF under non-loopback bind").Some unrelated suites fail only in my sandbox (symlink perms, missing Playwright, a live
external API) — none in this diff; please confirm in CI.
npm run lintpasses (tsc --noEmit, clean)Checklist
::1fix is called out aboveCONTRIBUTING.mdand agree to its contribution termsSummary by CodeRabbit
Security Enhancements
Bug Fixes
Tests