Skip to content

Validate redirect scheme and strip credentials on cross-origin redirects in MCP HTTP client#320347

Merged
connor4312 merged 3 commits into
microsoft:mainfrom
g0w6y:fix/mcp-redirect-scheme-and-credential-leak
Jun 10, 2026
Merged

Validate redirect scheme and strip credentials on cross-origin redirects in MCP HTTP client#320347
connor4312 merged 3 commits into
microsoft:mainfrom
g0w6y:fix/mcp-redirect-scheme-and-credential-leak

Conversation

@g0w6y

@g0w6y g0w6y commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

Summary

The MCP HTTP client in src/vs/workbench/api/common/extHostMcp.ts follows HTTP redirects (301/302/303/307/308) without validating the scheme of the redirect target and without stripping credential headers when the target is a different origin. Two issues result:

  1. Arbitrary scheme follow. The redirect target is resolved with new URL(location, currentUrl) and passed straight back into the fetch path. On the Node extension host, _fetchInternal dispatches unix:// and pipe:// URLs over a local socket (extHostMcpNode.ts). A Location: unix:///var/run/docker.sock#/... header therefore causes the client to issue requests to local sockets such as the Docker daemon socket. MCP server URLs are already restricted to http(s) at configuration time (mcpConfiguration.ts, pattern: ^https?:\/\/.+); the redirect path did not enforce the same restriction.

  2. Cross-origin credential replay. For 307/308 the method, body and all headers are preserved across the redirect. The configured Authorization bearer token (OAuth or a static key from the mcp.json headers), the Mcp-Session-Id, and the JSON-RPC body are re-sent to the redirect target even when it is a different origin. Browser fetch and curl strip credentials on a cross-origin redirect; the MCP client did not.

Fix

In the redirect loop:

  • Only follow redirects whose resolved protocol is http: or https:; otherwise stop following and log a warning. This blocks unix://, pipe://, file://, etc.
  • When the redirect target origin differs from the current origin, drop credential-bearing headers (Authorization, Cookie, Proxy-Authorization, Mcp-Session-Id) before the request is re-sent, matching fetch/curl behavior.

Same-origin redirects are unaffected and keep their headers. The change is confined to the redirect loop plus two module-level constants.

Testing

  • Manual: a redirect server returning 307 Location: <cross-origin> no longer receives the configured Authorization header or Mcp-Session-Id.
  • Manual: a redirect server returning 302 Location: unix:///.../docker.sock#/... is no longer followed; a warning is logged and the redirect response is returned to the caller.

Disclosure

This issue was reported to the Microsoft Security Response Center (MSRC) via the researcher portal — Case number 116632 (VULN-187202) — by Gouri Sankar A.

Copilot AI review requested due to automatic review settings June 8, 2026 06:08

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

This PR hardens MCP HTTP redirect handling to prevent redirects to non-HTTP(S) schemes and to avoid replaying sensitive headers across origins.

Changes:

  • Restricts followed redirects to http:/https: targets only.
  • Strips credential-bearing headers when redirects cross origin.
  • Adds constants documenting and enforcing these redirect policies.

Comment thread src/vs/workbench/api/common/extHostMcp.ts
Comment thread src/vs/workbench/api/common/extHostMcp.ts
@vs-code-engineering

vs-code-engineering Bot commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

📬 CODENOTIFY

The following users are being notified based on files changed in this PR:

@TylerLeonhardt

Matched files:

  • src/vs/workbench/api/common/extHostMcp.ts

@g0w6y

g0w6y commented Jun 8, 2026

Copy link
Copy Markdown
Contributor Author

@microsoft-github-policy-service agree

@g0w6y

g0w6y commented Jun 9, 2026

Copy link
Copy Markdown
Contributor Author

@TylerLeonhardt @dmitrivMS Any Update ?

Comment thread src/vs/workbench/api/common/extHostMcp.ts
Comment thread src/vs/workbench/api/common/extHostMcp.ts

@connor4312 connor4312 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks!

@connor4312
connor4312 enabled auto-merge June 10, 2026 17:30
@connor4312
connor4312 merged commit 29b16ad into microsoft:main Jun 10, 2026
25 checks passed
@vs-code-engineering vs-code-engineering Bot added this to the 1.125.0 milestone Jun 10, 2026
@g0w6y

g0w6y commented Jun 11, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for reviewing and merging this, @connor4312 / @vritant24.

For downstream tracking I'd like to flag that this is a security fix and ask about a formal advisory. The merged change addresses two issues in the MCP HTTP client:

  • Cross-origin credential disclosure: on a 307/308 redirect the client replayed the Authorization bearer token, any static mcp.json header (e.g. API keys), and the Mcp-Session-Id to the redirect target even on a different origin (browsers/curl strip these). This is attacker-readable token theft in the default configuration.
  • Redirect scheme confusion: redirects were followed without a scheme allow-list, so a Location: unix:///var/run/docker.sock#/... reached the unix:// / pipe:// socket dispatcher.

I reported this to MSRC (Case 116632 / VULN-187202). Would the team be willing to file a GitHub Security Advisory for the repository and request a CVE so users can track the fixed version? I'm happy to provide the full reproduction and write-up for the advisory. Thanks again.

@vs-code-engineering vs-code-engineering Bot locked and limited conversation to collaborators Jul 25, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants