security: SSRF guardrails for server-side connector fetches - #223
Closed
amal66 wants to merge 2 commits into
Closed
Conversation
Port the fork's SSRF hardening for MCP connector egress into the upstream layout: - Extract private/reserved IP classification into lib/privateIp.ts and fix IPv6 gaps: fe80::/10 link-local matching (the /^fe[89ab]:/ regex only matched the hextet "fe8:" and let fe80::1 through), hex-form IPv4-mapped addresses (::ffff:a00:1), NAT64 (64:ff9b::/96) and 6to4 (2002::/16) embedded IPv4 ranges. - Strip brackets from IPv6 literals in validateRemoteMcpUrl so [::1] et al. are classified by the private-IP guard instead of falling through to DNS lookup. - Route all OAuth egress (metadata fetch, discovery probes, dynamic client registration, token refresh) through guardedFetch so every outbound MCP request gets the same HTTPS-only / blocked-host / private-IP / no-redirect checks; the discovery probes were previously raw, unvalidated fetches. - Add SSRF regression tests (run atop the test-harness PR) and exclude test files from the tsc production build. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CEguyEgXa9JjCciXCcVemC
Restore the fork's pinnedGuardAgent verbatim: guardedFetch now routes
through a per-request undici Agent whose connect-time DNS lookup runs
the private-IP guard and returns only validated addresses, so the
address we validate is the address we connect to — closing the
DNS-rebinding/TOCTOU window between the pre-fetch validation lookup and
the socket's own resolution. Adds the undici runtime dependency the
fork ships for exactly this purpose ("undici": "^6.27.0"), and restores
the dispatcher assertions in the SSRF test so it matches the fork's
byte for byte.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CEguyEgXa9JjCciXCcVemC
Collaborator
Author
|
Superseded by #227, which folds this into a single server-side security-hardening pack (with the vitest harness so its tests run standalone — 57 backend + 8 frontend tests green). |
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
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
When a user adds an MCP connector, the server makes outbound HTTP requests to whatever URL the connector points at — including the OAuth discovery, registration, and token-refresh calls. Without guardrails, a malicious connector URL can turn our own server into a proxy: it can make the backend fetch internal-network addresses or cloud metadata endpoints (e.g.
169.254.169.254,metadata.google.internal), which on most hosts hand out credentials that unlock the whole environment. This PR closes the remaining holes in that guard: the OAuth discovery probes previously went out with no validation at all, IPv6 literals likehttps://[::1]/slipped past the IP classifier, several IPv6 encodings of internal IPv4 addresses (link-localfe80::1, hex-form mapped addresses like::ffff:a00:1, NAT64, 6to4) were not recognized as private, and a DNS-rebinding attacker could pass the pre-fetch check with a public address and then serve a private one to the actual connection.Changes
backend/src/lib/privateIp.ts: shared private/reserved IP classification, extracted from the inline helpers inbackend/src/lib/mcp/client.ts, with hardened IPv6 handling:fe80::/10link-local check — the previous/^fe[89ab]:/regex only matched the unrelated hextetfe8:and letfe80::1through::ffff:a00:1), not just the dotted form (::ffff:10.0.0.1)64:ff9b::/96) and 6to4 (2002::/16) addresses whose embedded IPv4 is privatebackend/src/lib/mcp/client.ts:guardedFetchnow pins the connection via a per-request undici dispatcher (pinnedGuardAgent) whose connect-time DNS lookup runs the private-IP guard and returns only validated addresses — the address we validate is the address we connect to, so there is no second, unguarded resolution for a DNS-rebinding attacker to race (TOCTOU). Host header and TLS SNI are untouched, so HTTPS still verifies normallybackend/src/lib/mcp/client.ts:validateRemoteMcpUrlnow strips the brackets URL parsing adds around IPv6 literals ([::1]) so they are classified by the private-IP guard instead of falling through to a DNS lookup; inline IP helpers replaced by the shared modulebackend/src/lib/mcp/oauth.ts: all four outbound OAuth call sites (metadatafetchJson, both discovery probes, dynamic client registration, token refresh) now go throughguardedFetch, so every outbound MCP request gets the same HTTPS-only / blocked-host / private-IP / connect-time-pinned / no-auto-redirect checks — the discovery probes were previously raw, unvalidatedfetchcalls, and token refresh did not disable redirectsbackend/src/lib/mcp/__tests__/client.ssrf.test.ts: 9 regression tests covering HTTPS enforcement, metadata/localhost host blocking, private IPv4/IPv6 literals, DNS resolution to private addresses, mixed public+private record sets, credential/fragment stripping, dispatcher pinning, and redirect disablingbackend/package.json/package-lock.json: addsundici@^6.27.0(lockfile regenerated by npm)backend/tsconfig.json: excludes*.test.ts/__tests__/from the productiontscbuildWhy
Connector URLs are user-supplied and therefore attacker-influenced; every gap above is a way to aim the backend's own network position at things only the backend can reach. This shrinks that security surface with pure validation logic on the existing egress path — zero new services and zero hosting cost. It adds exactly one dependency: undici — the engine behind Node's own fetch — pinned for connect-time DNS validation; used by the fork in production for exactly this purpose.
Testing
npm install && npm run buildinbackend/on this branch as committed: passes (tsc, no errors)upstream-pr/test-harness), which this branch deliberately does not include; with that branch merged locally:npx vitest run src/lib/mcp/__tests__/client.ssrf.test.ts— 9/9 passednpx vitest run(full backend suite) — 2 files, 21/21 passedProvenance
All changes are mechanical ports of code in amal66/mike@origin/main (commit b3166dd):
pinnedGuardAgent,guardedFetch, the IPv6-bracket handling, the OAuth call-site changes, and the test file are verbatim from the fork (apps/api/src/lib/mcp/client.ts,oauth.ts,__tests__/client.ssrf.test.ts),privateIp.tsfromapps/api/src/lib/privateIp.ts, the tsconfig excludes and theundicidependency line from the fork'sapps/apiconfig. Exceptions:privateIp.tsdoc comment and the classifier-relocation comment inclient.tsno longer mention the fork's OpenAI base-URL validation, which does not exist upstream.🤖 Generated with Claude Code
https://claude.ai/code/session_01CEguyEgXa9JjCciXCcVemC