fix(security): close FetchURL SSRF bypasses and DNS-rebinding window - #1791
Conversation
- resolve hostnames via DNS and reject any target resolving to loopback / RFC1918 / link-local / CGNAT / ULA ranges, including IPv4-mapped IPv6 forms (e.g. localtest.me, [::ffff:7f00:1]) — the static host denylist only matched literals and could be bypassed by crafted domains - follow redirects manually with full per-hop revalidation (10-hop cap) instead of auto-following, so a public URL cannot 302 the fetcher at internal services or cloud metadata endpoints - pin each connection to the DNS answers the check validated (per-hop undici Agent with a pinned lookup), closing the TOCTOU / DNS-rebinding window between the check and the connect; skipped when a proxy is configured or allowPrivateAddresses is set - apply to both agent-core and agent-core-v2 providers, with SSRF / redirect / pinning test coverage
🦋 Changeset detectedLatest commit: 4ecf1c3 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
commit: |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 86a1b7851e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // With an HTTP/SOCKS proxy configured, origin resolution happens on the | ||
| // proxy side; a direct-connect pinned Agent would bypass the proxy | ||
| // entirely, so pinning only applies to direct connections. | ||
| if (isProxyConfigured(process.env)) return undefined; |
There was a problem hiding this comment.
When any proxy variable is present, this skips the pinned dispatcher even for requests that will still connect directly, such as NO_PROXY=*, a destination matched by NO_PROXY, or an invalid proxy config that createProxyDispatcher later falls back from. In those cases undici performs a fresh connect-time DNS lookup after resolveSafeFetchTarget has approved a public address, so the DNS-rebinding window this change is meant to close remains open; the same guard exists in the v2 provider. Please only skip pinning when the specific request is actually going through the proxy, or keep the pinned lookup for direct/no-proxy paths.
Useful? React with 👍 / 👎.
| await Promise.all( | ||
| dispatchers.map((dispatcher) => | ||
| dispatcher.close().catch(() => { |
There was a problem hiding this comment.
For a public response whose Content-Length exceeds maxBytes, readResponse throws before the body is canceled, and this new finally then awaits dispatcher.close(). With a server that sends the headers and keeps the oversized body open, the graceful undici close can wait on that still-active response stream instead of returning the intended size error promptly; cancel the final response body on the content-length rejection path before closing the per-hop agent. The same pattern is present in the v2 provider.
Useful? React with 👍 / 👎.
| /** | ||
| * Pin the connection to the addresses the safety check just validated. | ||
| * undici resolves the origin again when it connects, so without pinning | ||
| * an attacker-controlled DNS could answer the check with a public IP and | ||
| * the connect with an internal one (TOCTOU / DNS rebinding). |
There was a problem hiding this comment.
packages/agent-core-v2/AGENTS.md says comments in v2 files must live solely in the top-of-file block and never beside functions, methods, or statements. This new method docblock, along with the added inline implementation comments in the same provider, violates that convention and should be moved into an allowed file header or removed.
Useful? React with 👍 / 👎.
…oonshotAI#1791) * fix(security): close FetchURL SSRF bypasses and DNS-rebinding window - resolve hostnames via DNS and reject any target resolving to loopback / RFC1918 / link-local / CGNAT / ULA ranges, including IPv4-mapped IPv6 forms (e.g. localtest.me, [::ffff:7f00:1]) — the static host denylist only matched literals and could be bypassed by crafted domains - follow redirects manually with full per-hop revalidation (10-hop cap) instead of auto-following, so a public URL cannot 302 the fetcher at internal services or cloud metadata endpoints - pin each connection to the DNS answers the check validated (per-hop undici Agent with a pinned lookup), closing the TOCTOU / DNS-rebinding window between the check and the connect; skipped when a proxy is configured or allowPrivateAddresses is set - apply to both agent-core and agent-core-v2 providers, with SSRF / redirect / pinning test coverage * chore: add changeset for FetchURL SSRF hardening * test: bridge undici type declarations in fetch pinning tests * fix(security): keep dispatcher option lib-agnostic for DOM typecheck consumers * fix(security): address review — pin NO_PROXY bypasses, drain oversized bodies, header-only comments
…oonshotAI#1791) * fix(security): close FetchURL SSRF bypasses and DNS-rebinding window - resolve hostnames via DNS and reject any target resolving to loopback / RFC1918 / link-local / CGNAT / ULA ranges, including IPv4-mapped IPv6 forms (e.g. localtest.me, [::ffff:7f00:1]) — the static host denylist only matched literals and could be bypassed by crafted domains - follow redirects manually with full per-hop revalidation (10-hop cap) instead of auto-following, so a public URL cannot 302 the fetcher at internal services or cloud metadata endpoints - pin each connection to the DNS answers the check validated (per-hop undici Agent with a pinned lookup), closing the TOCTOU / DNS-rebinding window between the check and the connect; skipped when a proxy is configured or allowPrivateAddresses is set - apply to both agent-core and agent-core-v2 providers, with SSRF / redirect / pinning test coverage * chore: add changeset for FetchURL SSRF hardening * test: bridge undici type declarations in fetch pinning tests * fix(security): keep dispatcher option lib-agnostic for DOM typecheck consumers * fix(security): address review — pin NO_PROXY bypasses, drain oversized bodies, header-only comments
…oonshotAI#1791) * fix(security): close FetchURL SSRF bypasses and DNS-rebinding window - resolve hostnames via DNS and reject any target resolving to loopback / RFC1918 / link-local / CGNAT / ULA ranges, including IPv4-mapped IPv6 forms (e.g. localtest.me, [::ffff:7f00:1]) — the static host denylist only matched literals and could be bypassed by crafted domains - follow redirects manually with full per-hop revalidation (10-hop cap) instead of auto-following, so a public URL cannot 302 the fetcher at internal services or cloud metadata endpoints - pin each connection to the DNS answers the check validated (per-hop undici Agent with a pinned lookup), closing the TOCTOU / DNS-rebinding window between the check and the connect; skipped when a proxy is configured or allowPrivateAddresses is set - apply to both agent-core and agent-core-v2 providers, with SSRF / redirect / pinning test coverage * chore: add changeset for FetchURL SSRF hardening * test: bridge undici type declarations in fetch pinning tests * fix(security): keep dispatcher option lib-agnostic for DOM typecheck consumers * fix(security): address review — pin NO_PROXY bypasses, drain oversized bodies, header-only comments
1 participant
Related Issue
No linked issue — this hardens the built-in URL fetch tool based on an external vulnerability disclosure (SSRF protection bypass).
Problem
The
FetchURLtool's SSRF guard was a static host denylist that only matched IP/host literals. It could be bypassed:localtest.me) or any internal address sailed through;169.254.169.254);http://[::ffff:7f00:1]/), which matched neither the IPv6 prefix checks nor the dotted-quad IPv4 regex.Since
FetchURLis in the default auto-approve tool set, a prompt-injected agent could use these to read loopback/internal services from the developer machine.What changed
net.BlockList, which also maps IPv4-mapped IPv6 onto the IPv4 subnets). DNS failures fail closed.redirect: 'manual', 10-hop cap), re-running the full check on every hop's target before connecting.Agentwhose connect-timelookupanswers from the addresses the check just validated, so an attacker-controlled DNS cannot return a different answer between check and connect. Pinning is skipped when an HTTP/SOCKS proxy is configured (origin resolution is proxy-side there) and for literal IPs /allowPrivateAddresses.agent-coreandagent-core-v2fetch providers.Checklist
gen-changesetsskill, or this PR needs no changeset.gen-docsskill, or this PR needs no doc update.