fix: prevent DNS-rebinding TOCTOU in safeProxyFetch by pinning resolved IPs

[manjunathbhaskar]

NOTE

• Identified as useful in closing advisories in go-live 7/9: triage and bulk-close the v1 PR backlog #1819

Problem

safeProxyFetch contains a TOCTOU (time-of-check / time-of-use) race that lets a DNS-rebinding attack reach cloud instance-metadata services (169.254.169.254).

Attack flow (before this fix)

1. Attacker registers evil.example.com with a very short TTL.
2. Inspector user connects to an MCP server at evil.example.com.
3. The /fetch proxy calls assertSafeProxyTarget("evil.example.com") → DNS resolves to 1.2.3.4 → passes block-list check ✅
4. Attacker flips DNS to 169.254.169.254 before the TTL expires.
5. node-fetch resolves evil.example.com again (its own lookup) → gets 169.254.169.254 → connects to the AWS/GCP/Azure metadata endpoint.
6. The metadata response (IAM tokens, credentials) is forwarded back to the attacker.

The vulnerability exists because assertSafeProxyTarget and node-fetch each perform an independent DNS resolution. The gap between them is the attack window.

Solution

assertSafeProxyTarget now returns the list of validated IP addresses instead of returning void. On each hop of safeProxyFetch, the first validated IP is passed to createPinnedAgent(), which builds an http.Agent / https.Agent whose lookup hook unconditionally returns that IP. node-fetch calls the hook instead of the OS resolver, so there is never a second DNS lookup — the TOCTOU window is eliminated.

Before:  assertSafeProxyTarget(url)   ← DNS lookup #1, validated
         fetch(url)                   ← DNS lookup #2, unvalidated ← attack window

After:   validatedIps = await assertSafeProxyTarget(url)  ← DNS lookup #1
         agent = createPinnedAgent(protocol, validatedIps[0])
         fetch(url, { agent })        ← no DNS lookup, uses pinned IP

Changes

New file: server/src/proxy-security.ts

Security helpers extracted from index.ts so they can be unit-tested in isolation:

• isBlockedProxyAddress(ip) — unchanged logic, now exported
• assertSafeProxyTarget(url) — now returns string[] (validated IPs)
• createPinnedAgent(protocol, ip) — new: builds an http/https agent pinned to a specific IP

Modified: server/src/index.ts

• Imports the three helpers from proxy-security.ts
• Removes the duplicate inline implementations (~85 lines deleted)
• safeProxyFetch calls createPinnedAgent on every hop using the IPs returned by assertSafeProxyTarget

New file: server/src/__tests__/proxy-security.test.ts

First unit-test suite for the server package (previously had zero tests). 26 test cases across 3 suites:

Suite	Cases
isBlockedProxyAddress	IPv4 link-local, IPv6 link-local, AWS IPv6 IMDS, IPv4-mapped IPv6 (hex + dotted), safe IPs, non-IP strings
assertSafeProxyTarget	safe hostname, blocked hostname, mixed DNS results, DNS failure, literal IPv4/IPv6 hosts, multi-address return
createPinnedAgent	correct agent type per protocol, lookup always returns pinned IPv4, lookup always returns pinned IPv6, TOCTOU guarantee (OS resolver is never invoked)

New files: server/vitest.config.ts, server/package.json (test scripts + vitest devDep)

Testing

cd server
npm install
npm test

All 26 new unit tests pass. Existing integration tests in client/src/__tests__/proxyFetchEndpoint.test.ts continue to pass unchanged.

References

• DNS Rebinding (Wikipedia)
• OWASP SSRF Prevention Cheat Sheet
• CWE-918: Server-Side Request Forgery

[Copilot]

Pull request overview

Pins validated DNS addresses to prevent DNS-rebinding TOCTOU attacks in the /fetch proxy.

Changes:

• Extracts SSRF validation and adds IP-pinned agents.
• Updates redirect handling to pin each validated hop.
• Adds server security tests and Vitest configuration.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
server/src/proxy-security.ts	Adds DNS validation and pinned-agent helpers.
server/src/index.ts	Uses validated IPs for proxy requests.
server/src/__tests__/proxy-security.test.ts	Tests SSRF security helpers.
server/vitest.config.ts	Configures server unit tests.
server/package.json	Adds Vitest scripts and dependencies.

---

💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.

[cliffhall]

Copilot review — round 1 responses (pushed as 0e1bc1d4)

Five findings, all addressed. Inline replies go outdated once the fix is pushed, so mirroring here.

1. createPinnedAgent lookup must handle { all: true } — VALID, fixed. This was a functionality-breaking bug, not a nit: net.connect runs with autoSelectFamily on (Node >= 20), so it calls the lookup hook with {"hints":1024,"all":true} and expects an array. The scalar callback failed every hostname request with ERR_INVALID_IP_ADDRESS: Invalid IP address: undefined — i.e. /fetch could only reach literal-IP targets. Reproduced against a live local server before and after the fix. The hook now returns [{ address, family }] when options.all is set and keeps the scalar shape otherwise.

2. Tests missed the { all: true } shape — VALID, fixed. Added coverage for the array shape (IPv4 and IPv6) plus an end-to-end test over the real path (node-fetch → http.Agent → net.connect) against a local server using a deliberately unresolvable hostname, so the request can only succeed via the pinned IP. Verified these catch the regression: reverting the options.all branch fails exactly those three (3 failed | 31 passed). Also replaced the Function-typed casts with a typed pinnedLookup() helper.

3. Server suite not run by CI — VALID, fixed. Added a "Run server tests" step to .github/workflows/main.yml next to the client one, plus a root test-server script. Left root test scoped as it was; the CI step is the enforcement point.

4. package-lock.json out of sync — VALID, fixed. Regenerated with npm install --package-lock-only. Only the server workspace mirror changed, because vitest@4.1.10 and @types/node@22.20.1 are already hoisted at the lock root (from the client workspace) and satisfy the new ranges.

5. "Close this PR and file an issue instead" — DECLINED. v1/main is the deprecated line that takes security fixes only, and a v1 PR targets v1/main directly (v1 publishes straight from that branch to v1-latest; it never merges into main), so this is the documented flow. The rule being referenced is that contributors file issues while maintainers open and drive the PRs — which is what is happening here.

Additional fixes found while reviewing (not raised by Copilot)

• npm run prettier-check was failing on both new files, which would have failed CI at the very first step. Formatted.
• Removed the agent: agent as any cast and its eslint-disable. node-fetch's RequestInit already types agent; tsc --noEmit is clean without the suppression.

Verified locally

npx prettier --check . (whole repo) ✅ · cd server && npx tsc --noEmit ✅ · cd server && npx vitest run → 34 passed ✅ · end-to-end pinned-agent request against a live local HTTP server ✅.

Not verified: the full client test suite and npm run build (unchanged by this diff, left to CI).

[Copilot]

Pull request overview

Copilot reviewed 7 out of 8 changed files in this pull request and generated no new comments.

Suppressed comments (2)

server/src/index.ts:894

• Pinning only validatedAddresses[0] removes Node's normal address-family fallback. For a dual-stack hostname whose first DNS result is unreachable (for example, IPv6 on an IPv4-only network) but a later validated address is reachable, /fetch now fails even though the target is available. Preserve the TOCTOU protection while passing all validated addresses to the lookup hook so its { all: true } callback can let net.connect select among only those prevalidated results; add a dual-stack fallback test as well.

    const agent = createPinnedAgent(currentUrl.protocol, validatedAddresses[0]);

server/src/tests/proxy-security.test.ts:1

• This test directory is inside server/tsconfig.json's rootDir/include, and that config excludes only *.spec.ts, so npm run build emits the suite into server/build/__tests__; the server package then publishes it because server/package.json includes the entire build directory. Keep tests out of production artifacts by excluding src/**/__tests__/** (as client/tsconfig.app.json:31 does) or moving the suite outside src (as the CLI tests are kept outside cli/src).

/**

[cliffhall]

Copilot review — round 2 responses (pushed as 923c58ed)

No inline comments this round; both findings came from the Suppressed comments block, and both were valid. Responding here since suppressed comments have no thread to reply to.

1. Pinning only validatedAddresses[0] removes Node's address-family fallback — VALID, fixed. Real regression: a dual-stack host whose first DNS result is unreachable (IPv6 record on an IPv4-only network) would fail even though a later validated address answers. createPinnedAgent now takes the whole validated list and hands all of it to the lookup hook, so net.connect's Happy Eyeballs selection works normally — but only ever over the prevalidated set, so the TOCTOU guarantee is unchanged. An empty list now throws ProxyTargetError instead of pinning undefined.

Added the requested dual-stack coverage: the pass-through assertion, the empty-list guard, and an end-to-end request where the first pinned address is a black hole (2001:db8::1, documentation range) and the connection must complete via the second. That last one exercises the fallback through the real node-fetch → http.Agent → net.connect path, and it passes.

2. The test suite is emitted into server/build and published — VALID, fixed. Confirmed by building: server/build/__tests__/proxy-security.test.js was there, and the root files allowlist ships all of server/build, so the published package carried a test file importing vitest (a devDependency). server/tsconfig.json now excludes src/**/__tests__/**; after the change build/ contains only index.js, mcpProxy.js, proxy-security.js, and static.

Rather than lose type-checking on the suite as a side effect, I added a small server/tsconfig.test.json (noEmit, includes the tests) and made npm test run it before vitest — so the tests stay type-checked while staying out of the artifact.

Verified locally

npx prettier --check . (whole repo) ✅ · cd server && npm test (tsc + vitest) → 37 passed ✅ · cd server && npm run build → no __tests__ in output ✅ · dual-stack fallback exercised end-to-end against a live local server ✅.

Not verified: the client test suite and the full root npm run build (untouched by this diff, left to CI).

[Copilot]

Pull request overview

Copilot reviewed 9 out of 10 changed files in this pull request and generated no new comments.

[cliffhall]

Copilot review — round 3: clean ✅

Re-requested against 923c58ed: "Copilot reviewed 9 out of 10 changed files in this pull request and generated no new comments." No inline comments, no suppressed comments. Ending the review loop here.

Summary of the two rounds

Round 1 (5 findings, 4 fixed + 1 declined) — 0e1bc1d4

• The pinned lookup ignored Node's { all: true } callback shape, which broke every hostname request through /fetch with ERR_INVALID_IP_ADDRESS; only literal-IP targets worked. Fixed and covered by tests that were verified to fail without the fix.
• Test suite not run by CI → added a "Run server tests" workflow step and a root test-server script.
• package-lock.json server-workspace mirror out of sync → regenerated.
• "Close this PR and file an issue" → declined; a security fix targeting v1/main directly is the documented v1 flow, and a maintainer is driving this PR.
• Also found while reviewing, not raised by Copilot: npm run prettier-check was failing on both new files (would have failed CI at the first step), and the agent as any cast plus its eslint-disable were unnecessary — tsc is clean without them.

Round 2 (2 suppressed findings, both fixed) — 923c58ed

• Pinning only the first address dropped Node's address-family fallback → now pins the whole validated set, so dual-stack fallback works while the reachable set stays exactly what was validated.
• The test suite was compiled into server/build and shipped in the published package → excluded from the build config, with a tsconfig.test.json keeping the suite type-checked.

Net effect on the original vulnerability fix: the DNS-pinning approach is unchanged and still closes the TOCTOU window; what changed is that it now actually works for hostname targets, preserves dual-stack fallback, is enforced by CI, and does not ship tests to npm.