Follow redirects only within the MCP endpoint's origin - #3397
Conversation
The HTTP client transports handled redirects differently depending on who built the httpx2 client: the SDK's default client followed every redirect, while a caller-supplied client followed none unless it set follow_redirects=True as the docs suggested. streamable_http_client and sse_client now handle redirects themselves, the same way for every client: a redirect that stays on the endpoint's origin (same scheme, host and port, or http to https on the same host with default ports) is followed, and a redirect anywhere else is not, so the message it answered fails with an error naming the location instead of the connection carrying on against a different host. - New private helpers in mcp.shared._httpx_utils send each request with follow_redirects=False and re-send httpx2's own next_request while it stays within the origin, bounded by the client's max_redirects. - create_mcp_http_client keeps only the timeouts; docs and examples no longer pass follow_redirects=True when building a client. - Requests an auth handler makes while an MCP request is in flight do not follow redirects either; the client transports and OAuth pages say so.
📚 Documentation preview
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline findings, I also checked whether requests yielded by an httpx2.Auth flow (e.g. OAuth metadata/token fetches) losing redirect following in stream_within_origin (src/mcp/shared/_httpx_utils.py:103) is an unintended regression — it is documented as deliberate in the helper's docstring and in both transports' public docstrings ("requests auth makes during an MCP request do not follow redirects"), so it was ruled out as intentional design rather than a bug.
Extended reasoning...
Findings were confirmed and posted inline (silent GET-stream redirect failure in streamable_http.py, SSE post_writer hang on refused redirects, and the example server's dropped timeouts), so this note only records the one additional concern investigated and ruled out: whether the new follow_redirects=False on client.send inside stream_within_origin silently changes behaviour for requests an httpx2.Auth flow makes mid-call. The behaviour is explicitly documented as intentional at src/mcp/shared/_httpx_utils.py:94-96 and in the sse_client/streamable_http_client docstrings, so it is a deliberate design decision, not a regression. The hunt exited on the bug cap with further verified findings unposted, so this is not an approval — the inline comments already indicate a human should look.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 19 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="examples/servers/simple-tool/mcp_simple_tool/server.py">
<violation number="1" location="examples/servers/simple-tool/mcp_simple_tool/server.py:12">
P2: This swap drops the read timeout for the fetch tool from 300s (create_mcp_http_client's Timeout(30, read=300)) to httpx2's flat 5-second default, because no timeout is passed. For a tool fetching arbitrary user-supplied websites, a 5s read timeout will fail on slow or large pages. Keep the previous timeouts and add follow_redirects explicitly.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
…very path Review follow-ups for the origin-scoped redirect handling: - A same-origin 301/302/303 answering a POST is no longer followed: httpx2's next-request rules turn those into a body-less GET, which would drop the JSON-RPC message. Only redirects that keep the method (307/308, or any status for a GET) are followed; the rest come back unfollowed like an off-origin redirect and the call fails naming the location. - The standalone GET stream and the resumption GET now handle an unfollowed redirect the way the message POST does: the GET stream logs it and stops instead of spending its reconnection attempts on the same answer, and a resumed request is resolved with the error instead of being left waiting. One helper builds the message for all three. - OAuth authorization-server metadata discovery treats a 3xx from a well-known candidate like a 4xx and tries the next candidate, matching the protected-resource metadata handler, now that these requests see redirect responses directly. - The simple-tool example keeps the 30s/300s timeouts it had before it stopped using the MCP client factory.
There was a problem hiding this comment.
Thanks — the follow-up commit resolves the points from my earlier review (unfollowed-redirect handling on the GET stream and resumption paths, method-preserving redirects only, OAuth metadata 3xx handling, restored example timeouts), and this run of the automated review found no new bugs. Since this changes redirect/origin security behaviour across both client transports, a human maintainer look is still worthwhile.
What was reviewed:
- The new
_unfollowed_redirecthelper and its call sites insrc/mcp/client/streamable_http.py; the resumption path's assert is safe given only requests carry resumption tokens. _handle_reconnection(streamable_http.py:537) is the onesse_within_originsite without the check — an unfollowed redirect there burns the reconnection attempts before resolving the request with the generic exhaustion error, so it degrades the message rather than hangs.- From my earlier comments, the
sse.pypost_writer redirect point is the one not reflected in code (the docstring was updated instead) — fine if deliberate, just flagging so it's a conscious call.
Extended reasoning...
Overview
This PR scopes HTTP redirect following for MCP client transports: src/mcp/shared/_httpx_utils.py gains origin-checking helpers (request_within_origin, sse_within_origin), and both src/mcp/client/streamable_http.py and src/mcp/client/sse.py route their requests through them so redirects are only followed same-origin (or http-to-https on the same host) and only when method-preserving. src/mcp/client/auth/utils.py, docs, docs_src tutorials, and examples are updated accordingly, with substantial new tests. The second commit (4a4b8fb) is an explicit review follow-up: it adds the _unfollowed_redirect helper and applies it to the standalone GET stream, the resumption GET, and the message POST; restricts followed redirects to method-preserving ones; makes OAuth metadata discovery treat 3xx like 4xx; and restores the simple-tool example's timeouts — addressing the findings from my prior review of the first push.
Security risks
The change is itself a security hardening: it prevents Authorization headers and JSON-RPC traffic from following cross-origin redirects (token exfiltration / request smuggling surface). The risk direction is therefore availability/correctness regressions rather than new exposure. I verified the three _unfollowed_redirect call sites resolve or log correctly, that the resumption path's assert original_request_id is not None is guarded by the request-only invariant on resumption tokens, and that _handle_reconnection — the one sse_within_origin site without the check — still terminates via its retry-exhaustion path (the request is resolved with an error after MAX_RECONNECTION_ATTEMPTS, so no hang, just a less specific error after retry delays). The bug hunt's two candidates this run were both that same _handle_reconnection gap, refuted as duplicates of earlier reporting.
Level of scrutiny
High. This alters observable redirect behaviour of a released 2.x public API (a compatibility contract per the repo's guidelines), touches auth-adjacent code paths, and encodes a deliberate policy decision (which redirects to follow, how unfollowed ones surface). Those are exactly the design calls a maintainer should sign off on, so approval by an automated review is not appropriate regardless of bug count.
Other factors
The hunt exited on dry_streak with zero findings on this version, and the new tests in tests/shared/test_httpx_utils.py, tests/client/test_streamable_http.py, and tests/shared/test_sse.py cover the redirect matrix well. One item from my prior review — the legacy sse.py post_writer dying on an unfollowed POST redirect — was answered with a docstring update rather than a code change; that may well be intentional for the legacy transport, but it is the one thread whose resolution I could not confirm from code, which further supports defer over approve.
There was a problem hiding this comment.
All reported issues were addressed across 21 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
…an https-to-http redirect Second round of review follow-ups for the origin-scoped redirect handling: - The rule "follow a redirect only within the request's origin, only when it keeps the method" now lives in one predicate, next_request_within_origin, which also declines a Location that carries userinfo (httpx2 would send it as Basic auth). - OAuthClientProvider and IdentityAssertionOAuthProvider apply that rule to the requests their flows make (metadata discovery, registration, token, refresh) through a small RedirectAwareAuth base, instead of those requests following nothing. A redirect elsewhere is still handed to the flow as a non-success, and the registration/token/refresh errors now name it. - When the redirect budget (the client's max_redirects) is spent, the last redirect is handed back unfollowed like any other, so a redirect loop fails the one call with the usual error instead of raising TooManyRedirects out of the transport. - The "not followed" error for an https endpoint redirected to plain http on the same host explains the likely cause (a TLS-terminating proxy the server does not trust, often plus a trailing slash) and suggests the https form of the location rather than the http one; locations are printed without query or userinfo. - docs: the OAuth and transports pages describe the shared rule; the ASGI mounting example points clients at /notes/ (the path that does not redirect).
# Conflicts: # src/mcp/client/auth/oauth2.py
There was a problem hiding this comment.
All reported issues were addressed across 13 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
This pull request has been reviewed before and this review found new issues. Where they share a root cause, one fix may close them together.
1 other optional suggestion (a nit or a note on pre-existing code) was found and not posted.
Findings marked 🟡 are optional suggestions and need no follow-up push.
…message covers any http downgrade - The OAuth registration/token/refresh messages named an unfollowed redirect with the raw location; they now share one helper with the transport message and print it without userinfo, query or fragment. - The "would downgrade to plain HTTP" explanation applies to any http location an https endpoint redirects to, not only one on the same host, so the error never suggests configuring an http:// URL. - Docstrings and the migration note say which redirects are followed more precisely (307/308 for a POST, any status for a GET).
There was a problem hiding this comment.
I re-reviewed this PR after the latest push and found no bugs; since it changes the SDK's redirect-following policy across both transports and the OAuth flows, a maintainer's look is still worthwhile.
What was reviewed:
- The new
redirect_locationhelper now strips userinfo, query and fragment before a location reaches any error message or log line, and all four OAuth call sites route through it — this resolves the sanitization concern from the previous review. - The https-to-http downgrade message now fires for any host, so the generic refuse-message no longer suggests a plain-HTTP URL for a cross-host downgrade.
- The updated docstrings in sse.py and streamable_http.py were checked against the implemented policy (307/308 for POSTs, any redirect status for the SSE/GET stream) — they match.
Extended reasoning...
Overview
Since my last review, one commit landed (plus a merge of main): it introduces redirect_location in src/mcp/shared/_httpx_utils.py, which returns the unfollowed redirect's target with userinfo, query and fragment stripped, and rewires both redirect_note (used by the OAuth token, refresh, registration and identity-assertion error paths) and _unfollowed_redirect in src/mcp/client/streamable_http.py through it. It also broadens the https-to-http downgrade message to any target host and tightens the transport docstrings to state the exact policy (method-preserving 307/308 for POSTs, any status for the SSE GET). Tests were extended in tests/client/test_streamable_http.py and tests/client/test_auth.py to cover the sanitized messages.
Security risks
This PR is itself a security hardening change: it stops httpx from silently following cross-origin or method-changing redirects on MCP and OAuth requests, which previously could replay tokens or POST bodies to another origin. The residual risks I previously flagged — leaking query/userinfo state from redirect locations into logs and error strings, and the SDK recommending a plain-HTTP URL after a cross-host downgrade — are both fixed by this push. This run's bug hunt over the full diff reported no findings, and I found no remaining path where an unsanitized location reaches a message.
Level of scrutiny
High. Redirect policy sits directly on the auth and transport trust boundary, the change spans both transports plus three OAuth code paths, and it intentionally alters observable v2 behavior (previously-followed redirects now fail with an error). That is exactly the category the approval guidelines say a human maintainer should sign off on, regardless of how clean this iteration looks — so defer, not approve.
Other factors
The conversation shows every earlier objection (mine and the other bots') answered by follow-up commits, and the fixes are faithful to what was asked. Test coverage for the new behavior is present and the repo enforces 100% coverage in CI. The reason to speak at all is that the author pushed a fix commit in response to a red finding; recording that the re-review of that push came back clean is new, useful information rather than noise.
…ders note to Deploy - Client transports: the redirect rule moves out of the intro paragraph and the httpx2 info box into a small "Redirects" section (what is followed, what is not, the error you see, the fix), so it is linkable and findable. - Deploy & scale: a short "Behind a TLS-terminating proxy" section with the uvicorn --proxy-headers / --forwarded-allow-ips fix for servers whose redirects point at http://, which is what the client's downgrade message links to.
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
…edirects section and recap
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="docs/client/transports.md">
<violation number="1" location="docs/client/transports.md:85">
P2: Same-origin GET streams also follow 301/302/303, but this bullet makes the following “anywhere else” rule say they are not followed. Document the method split (307/308 for POST, any redirect status for same-origin GET) and qualify the HTTP→HTTPS exception to default ports here and in the recap.
(Based on your team's feedback about redirect wording for streamable HTTP.)</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
1 participant
Redirect handling in the HTTP client transports depended on who built the
httpx2.AsyncClient: the SDK's default client followed every redirect, while a client you pass in (the way to set headers or auth on 2.x) followed none unless you addedfollow_redirects=True, which the docs told you to do. This moves redirect handling into the transports themselves and scopes it to the endpoint's origin, so it behaves the same for every client and a redirect can't quietly move a connection to a different host.Motivation and Context
follow_redirects=Truewas turned on in the client factory for trailing-slash 307s from Starlette-mounted servers (#105, #732) and hosted servers that redirect (#283) — same-origin redirects. Following any redirect also meant the transport would carry on against whatever host aLocationheader named, re-sending what was configured for the original endpoint (headers, auth, request body) there and treating the answer as the MCP server's. httpx2 leaves following off by default and keeps that kind of policy out of the library (encode/httpx#2533), so it belongs in the transport.streamable_http_clientandsse_clientnow send each request with following disabled at the request level and handle a redirect themselves:http→httpson the same host with default ports) and keeps the request method (307/308; any status for a GET) is followed, using httpx2's own next-request construction, up to the client'smax_redirects— a 301/302/303 answering a POST is not, because httpx2 would replay it as a body-less GET, and neither is aLocationcarrying userinfo;max_redirectsbudget, so a loop too) is not followed, and no request is made to its location. The transport gets the redirect response back unfollowed, exactly as httpx2 hands one back with following off, and treats it as the non-success it is: a message POST or a resumption GET resolves that call with anMCPErrornaming the location while the session stays usable (the per-request shape every other non-2xx already has on 2.x), the standalone GET stream logs it and stops rather than retrying, and the SSE connect fails withhttpx2.HTTPStatusErrorfor the redirect response. When anhttpsendpoint redirects to plainhttpon the same host — usually a TLS-terminating proxy the server doesn't trust plus a trailing slash — the error says so and suggests thehttpsform of the location, never thehttpone.This holds whichever client is in use, so a caller-supplied client no longer needs
follow_redirects=Truefor the trailing-slash case, and setting it doesn't widen what the transport follows. httpx2 applies the per-request setting to the requests anauth=flow makes as well, soOAuthClientProviderandIdentityAssertionOAuthProviderapply the same rule to their own requests (discovery, registration, token, refresh) through a small shared base: a within-origin, method-preserving redirect is followed, anything else is treated as that URL not answering (discovery moves on to the next well-known candidate; registration/token errors name the redirect).create_mcp_http_clientdropsfollow_redirects=Trueand keeps only the timeouts.How Has This Been Tested?
httpx2.MockTransport(tests/shared/test_httpx_utils.py): same-origin, relative and https-upgrade redirects followed with method and body intact and the intermediate response closed; other host, other port, subdomain, other scheme, https→http, 301/302/303-on-POST and userinfo locations handed back unfollowed with no request recorded for the location; the hop budget hands back the next redirect; a client built withfollow_redirects=Trueis still scoped; the SSE form sends the headersAsyncClient.sse()sends. OAuth flow tests: a flow request follows a same-origin redirect, hands back a cross-origin one, and stops after a few hops.Client/sse_clientwith in-process transports: Starlette's/mcp→/mcp/redirect is followed with a caller-supplied client left at httpx2's default; a redirect to another origin fails that one call withMCPErrorwhile the next call on the same session succeeds, and that origin is never contacted; the SSE connect equivalents. These fail onmainand pass here.Mount-induced 307 is followed and tool calls work with default, no-follow and follow-enabled clients; an endpoint that 307s to a different port fails the call with the error above and the second server logs no request; an OAuth flow whose first metadata candidate 307s to another port skips it (that port sees nothing) and whose registration endpoint 308s to/register/registers normally; a same-origin redirect loop fails the one call and the session carries on.Mounted, so each one rides a real 307). Full suite with coverage, pyright and ruff pass locally.Breaking Changes
MCPError("Redirect to … not followed; use that URL as the endpoint if it is the intended server") instead of being followed (SDK default client) or ending inUnexpected content type(caller-supplied client). Thehttp→httpsallowance applies only when neither side names a non-default port.httpx2.TooManyRedirectsout of the transport.follow_redirectson a caller-supplied client is no longer consulted for MCP requests; same-origin redirects are followed regardless of it.mcp.shared._httpx_utils.create_mcp_http_clientno longer enablesfollow_redirects.Types of changes
Checklist
help wanted, or I'm a maintainer)Additional context
follow_redirects=Truewhen building a client;docs/client/transports.mdanddocs/client/oauth-clients.mddescribe the redirect behaviour, thedocs/migration.mdpassages that told v1 users to set the flag are corrected, anddocs/run/asgi.mdpoints clients at the mounted path that doesn't redirect. Translated pages aren't regenerated in this PR.examples/servers/simple-toolfetched arbitrary web pages through the private MCP client factory; it now uses a plainhttpx2.AsyncClient(follow_redirects=True)of its own.AI Disclaimer