fix(security): pin outbound fetch connections to the validated IP (F-11) - #275
Merged
Conversation
assertPublicUrl resolved DNS once to decide a hostname was public, then handed the HOSTNAME (not the resolved address) to fetch, which resolves it AGAIN, independently, at connect time. An attacker controlling DNS for the target can answer publicly for the policy check and privately for the actual connection -- classic TOCTOU / DNS rebinding -- and nothing between those two lookups closed the gap. safeFetch now does a second, connection-time DNS resolution immediately before each fetch, re-validates ITS result for privacy too (in case the two lookups already disagree), and pins the actual socket to that exact address via an undici Agent with a custom connector -- so whatever fetch's own internal resolution would have produced is never consulted; the connection goes to the address this guard just checked, not to whatever DNS answers a moment later. Applies per hop, since safeFetch already re-validates every redirect the same way. undici was already an installed transitive dependency (multiple other packages pull it in); promoted to a direct dependency since the app now imports it directly for this. No pin is attempted for a literal-IP URL (nothing to resolve) or when the private-network guard itself was bypassed (allowlisted host, allowPrivate, or blocking disabled) -- those paths were not making an "is this the address I checked" promise to begin with. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KQq6TnVNHRNU6Wz1eQzPpD
…sses Three things the pre-merge review of this PR turned up. 1. SOCKET LEAK. `pinnedDispatcher()` built a new undici `Agent` per fetch (and per redirect hop) and never disposed of it. Undici's default keep-alive holds the socket open for a reuse that, for a per-request dispatcher, never comes — one leaked file descriptor per outbound call, which a crawl run reaches the process FD limit with. The agent now uses minimal keep-alive, and every dispatcher whose response nobody will read (each redirect hop, the error path, the redirect-cap throw) is destroyed explicitly; only the one whose response is handed back to the caller outlives the call. 2. LOST DUAL-STACK FAILOVER. The pin used `records[0]` only, so a host whose first DNS answer is an AAAA this container cannot route went from "works" (fetch tries the next address) to "always fails". All resolved addresses are now handed to the connector — and ALL of them are privacy-checked, not just the first, so a second private record cannot be the one it connects to. 3. UNVERIFIED CONNECTOR CONTRACT. Every existing test mocks `fetch`, so nothing exercised the custom `lookup` callback the pin depends on — if its shape were wrong for this Node/undici, every guarded outbound call in the product would fail at connect time and no test would notice. Adds outbound-fetch-pinning-live.test.ts: a real HTTP server on loopback, reached through the real dispatcher via a hostname that can never resolve (RFC 2606 .invalid), plus a failover case proving the multi-address list actually falls through to the second entry. Test plan: outbound-fetch.test.ts 22/22 unchanged and passing; 2 new live-connection tests; full suite 5053 passed, 5 skipped, 0 failed; tsc + eslint clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KQq6TnVNHRNU6Wz1eQzPpD
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Comment on lines
+416
to
+421
| response = await fetch(url, { | ||
| ...init, | ||
| signal: controller.signal, | ||
| redirect: 'manual', | ||
| ...(dispatcher ? { dispatcher } : {}), | ||
| } as RequestInit); |
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
2 participants
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
P1 finding from the 2026-09-05 finance-institution assessment.
assertPublicUrlresolved DNS once to decide a hostname was public, then handed the hostname (not the resolved address) tofetch, which resolves it again, independently, at connect time. An attacker controlling DNS for the target can answer publicly for the policy check and privately for the actual connection — classic TOCTOU / DNS rebinding — and nothing between those two lookups closed the gap.safeFetchnow:fetchcall (not reused from the 30s policy cache — freshness is the whole point).undici.Agentwith a custom connector, so whateverfetch's own internal resolution would have produced is never consulted — the connection goes to the address this guard just checked, not to whatever DNS answers a moment later.Applies per redirect hop, since
safeFetchalready re-validates every hop the same way.undiciwas already an installed transitive dependency (several other packages pull it in) — promoted to a direct dependency since the app now imports it explicitly for this.No pin is attempted for a literal-IP URL (nothing to resolve) or when the private-network guard itself was bypassed (allowlisted host,
allowPrivate, or blocking disabled globally) — those paths weren't making an "is this the address I checked" promise to begin with.Test plan
safeFetchnow rejects it (proven to fail against the pre-fix code: reverted the re-check, confirmed red, restored)outbound-fetch.test.ts+guardrail-fix2-outbound-redirect-ssrf.test.tstests still pass unmodifiednpx tsc --noEmitcleannpx eslintclean (0 errors, 0 warnings on changed files)vitest run: 5051 passed, 0 failed, 5 skipped🤖 Generated with Claude Code
https://claude.ai/code/session_01KQq6TnVNHRNU6Wz1eQzPpD