fix(mcp): block cross-origin credential redirects#396
Conversation
There was a problem hiding this comment.
Pull request overview
This PR hardens MCP’s HTTP and SSE networking so requests that carry credentials (custom headers or OAuth bearer tokens) cannot be redirected or pointed to a different origin than the configured server, preventing accidental secret forwarding.
Changes:
- Added an HTTP redirect guard that enforces same-origin redirects (including default-port normalization) and caps redirect chains.
- Restricted SSE
endpointevent URLs to the configured server origin before any authenticated follow-up requests are made. - Updated and expanded tests to cover same-origin redirects, cross-origin redirect blocking, and cross-origin SSE endpoint blocking.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| internal/mcp/network_redirect.go | Adds same-origin logic + redirect guard used by MCP HTTP clients to prevent cross-origin credential redirects. |
| internal/mcp/network_client.go | Enforces same-origin SSE endpoints and ensures non-OAuth clients also use the redirect guard. |
| internal/mcp/network_client_test.go | Adds targeted unit tests for redirect blocking, origin normalization, and SSE endpoint origin restrictions. |
| internal/mcp/client_test.go | Adds integration-style tests validating same-origin redirects still work and cross-origin redirects/endpoints are blocked before headers leak. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
WalkthroughAdds MCP redirect and SSE origin enforcement, wires the guarded client into OAuth and non-OAuth paths, and expands tests for same-origin redirects, cross-origin rejection, and origin normalization. ChangesMCP Origin Enforcement
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant mcpHTTPClient
participant checkMCPRedirect
participant OriginServer
participant RedirectTarget
Client->>mcpHTTPClient: send request
mcpHTTPClient->>OriginServer: initial request
OriginServer-->>mcpHTTPClient: redirect response
mcpHTTPClient->>checkMCPRedirect: evaluate redirect
checkMCPRedirect->>checkMCPRedirect: compare origins
alt same origin
checkMCPRedirect-->>mcpHTTPClient: allow redirect
mcpHTTPClient->>RedirectTarget: follow redirect with headers
else cross-origin
checkMCPRedirect-->>mcpHTTPClient: return error
mcpHTTPClient-->>Client: error
end
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/mcp/network_client.go (1)
880-883: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFix garbled comment.
// not declare OAuth use the default transport with the same MCP redirect guard.reads as broken English — looks like leading words (e.g. "Servers that do") were dropped.✏️ Proposed fix
-// not declare OAuth use the default transport with the same MCP redirect guard. +// Servers that do not declare OAuth use the default transport with the same MCP redirect guard. func oauthHTTPClient(server Server) (*http.Client, error) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/mcp/network_client.go` around lines 880 - 883, The comment above oauthHTTPClient is garbled and appears to have lost its opening words; replace it with a clear sentence that accurately describes the non-OAuth case and the use of the default transport with the MCP redirect guard. Update the comment text only, keeping the behavior in oauthHTTPClient and the mcpHTTPClient call unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@internal/mcp/network_client.go`:
- Around line 880-883: The comment above oauthHTTPClient is garbled and appears
to have lost its opening words; replace it with a clear sentence that accurately
describes the non-OAuth case and the use of the default transport with the MCP
redirect guard. Update the comment text only, keeping the behavior in
oauthHTTPClient and the mcpHTTPClient call unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b6329b5a-1948-4b45-8836-c638d6765958
📒 Files selected for processing (4)
internal/mcp/client_test.gointernal/mcp/network_client.gointernal/mcp/network_client_test.gointernal/mcp/network_redirect.go
GautamBytes
force-pushed
the
fix/mcp-redirect-auth-leak
branch
from
fd0ae06 to
7ddee33
Compare
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Verdict: Approve ✅ — correct, complete for its scope, and it doesn't break same-origin flows
Reviewed this as a security change (checked out the branch, audited the whole client surface, ran the suite). The hardening is sound and the design choices are the right ones.
Security logic is correct
- Origin comparison (
sameMCPOrigin) is scheme + host + effective port, case-insensitive, with default-port normalization (80/443). Sohttps://h/x≡https://H:443/y, and anhttps→httpdowngrade is correctly treated as cross-origin. The table test covers all four cases. - Redirect target is compared to the original URL (
via[0].URL), not just the previous hop — so a multi-hop chain can't walk off-origin one same-origin step at a time. This is the subtle thing a naive implementation gets wrong; this one gets it right. - The bearer/custom header never reaches a cross-origin target:
CheckRedirectreturns the error before the client issues the next request, so the OAuth RoundTripper never runs for the target. All three no-leak tests asserttargetHits == 0, and confirm the header still reaches the configured origin. - SSE endpoint is origin-pinned (
resolveSSEEndpointURL) before any message POST, closing the "maliciousevent: endpointpoints cross-origin" vector — and the POST itself also rides the guarded client, so it's belt-and-suspenders. - Redirect chain is bounded (
maxMCPRedirects).
Coverage is complete for the data plane — I checked for bypasses
Both remote entry points build their client through oauthHTTPClient → mcpHTTPClient (HTTP at connectNetwork, SSE at connectRemoteSSE), and every client.client.Do(...) uses it. The non-OAuth path now gets the guard too (previously bare http.DefaultClient). I grepped the package for other client construction — no MCP request path bypasses the guard.
It doesn't break legitimate behavior
TestHTTPClientFollowsSameOriginRedirectproves same-origin redirects (incl. default-port equivalence) still work end to end with the header intact.- Non-OAuth servers switching from the shared
http.DefaultClientto a per-servermcpHTTPClient(server, nil)keepshttp.DefaultTransport(nil transport), so connection pooling and timeout behavior are unchanged. - Full
internal/mcpsuite passes locally;go build ./...,go vet,gofmtclean. (Race detector runs in CI on Linux — the new tests usesync/atomiccorrectly.)
One optional follow-up (non-blocking, out of this PR's scope)
The OAuth control-plane clients — token exchange / metadata discovery in oauth.go and the refresh storeTokenSource (network_client.go:892) — still use http.DefaultClient without this guard. A 307/308 redirect there preserves method+body, so in principle a hostile authorization-server-metadata could bounce a token request cross-origin. Different threat model (requires compromising AS discovery) and clearly outside "MCP data traffic stays on origin", but extending the same guard to those clients would be a tidy defense-in-depth follow-up.
Nice, careful work — approving. Thanks @GautamBytes, and good call reporting privately first.
Summary
Security report was submitted privately first.
Summary by CodeRabbit