Follow redirects only within the MCP endpoint's origin - #3397
Conversation
Sorry, something went wrong.
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
|
Sorry, something went wrong.
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.
Sorry, something went wrong.
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
Sorry, something went wrong.
…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_redirect helper and its call sites in src/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 one sse_within_origin site 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.py post_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.
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.
Sorry, something went wrong.
There was a problem hiding this comment.
2 issues found across 21 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="src/mcp/client/sse.py">
<violation number="1" location="src/mcp/client/sse.py:69">
P1: When an SSE endpoint upgrades from HTTP to HTTPS, this wrapper connects the stream over HTTPS but `sse_reader` still resolves the relative endpoint event against the original HTTP URL. The first MCP POST therefore targets HTTP and can fail on common 301/302 upgrades or send the message over an insecure connection; resolve and validate the endpoint against the final SSE response URL.</violation>
</file>
<file name="docs/migration.md">
<violation number="1" location="docs/migration.md:2105">
P3: This line says the transport follows any redirect that stays on the endpoint's origin, but stream_within_origin also requires the method to be unchanged. A same-origin 301/302/303 that httpx2 turns into a GET (common for a POST) is treated as unfollowed, not followed. Qualify the wording to mention that only method-preserving redirects (e.g. 307/308) are followed.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Sorry, something went wrong.
| headers=headers, auth=auth, timeout=httpx2.Timeout(timeout, read=sse_read_timeout) | ||
| ) as client: | ||
| async with client.sse(url) as event_source: | ||
| async with sse_within_origin(client, url) as event_source: |
There was a problem hiding this comment.
P1: When an SSE endpoint upgrades from HTTP to HTTPS, this wrapper connects the stream over HTTPS but sse_reader still resolves the relative endpoint event against the original HTTP URL. The first MCP POST therefore targets HTTP and can fail on common 301/302 upgrades or send the message over an insecure connection; resolve and validate the endpoint against the final SSE response URL.
Prompt for AI agentsCheck if this issue is valid — if so, understand the root cause and fix it. At src/mcp/client/sse.py, line 69:
<comment>When an SSE endpoint upgrades from HTTP to HTTPS, this wrapper connects the stream over HTTPS but `sse_reader` still resolves the relative endpoint event against the original HTTP URL. The first MCP POST therefore targets HTTP and can fail on common 301/302 upgrades or send the message over an insecure connection; resolve and validate the endpoint against the final SSE response URL.</comment>
<file context>
@@ -47,15 +52,21 @@ async def sse_client(
headers=headers, auth=auth, timeout=httpx2.Timeout(timeout, read=sse_read_timeout)
) as client:
- async with client.sse(url) as event_source:
+ async with sse_within_origin(client, url) as event_source:
event_source.response.raise_for_status()
logger.debug("SSE connection established")
</file context>
Sorry, something went wrong.
| ``` | ||
|
|
||
| v1's internal client set `follow_redirects=True`; set it explicitly when supplying your own `httpx2.AsyncClient` to preserve that behavior. | ||
| v1's internal client set `follow_redirects=True`. You don't need it on your own client: the transport follows a redirect within the endpoint's origin (a trailing-slash redirect, say) itself, and does not follow one anywhere else, whatever the client is configured to do. |
There was a problem hiding this comment.
P3: This line says the transport follows any redirect that stays on the endpoint's origin, but stream_within_origin also requires the method to be unchanged. A same-origin 301/302/303 that httpx2 turns into a GET (common for a POST) is treated as unfollowed, not followed. Qualify the wording to mention that only method-preserving redirects (e.g. 307/308) are followed.
Prompt for AI agentsCheck if this issue is valid — if so, understand the root cause and fix it. At docs/migration.md, line 2105:
<comment>This line says the transport follows any redirect that stays on the endpoint's origin, but stream_within_origin also requires the method to be unchanged. A same-origin 301/302/303 that httpx2 turns into a GET (common for a POST) is treated as unfollowed, not followed. Qualify the wording to mention that only method-preserving redirects (e.g. 307/308) are followed.</comment>
<file context>
@@ -2103,11 +2102,11 @@ async with http_client:
-v1's internal client set follow_redirects=True; set it explicitly when supplying your own httpx2.AsyncClient to preserve that behavior.
+v1's internal client set follow_redirects=True. You don't need it on your own client: the transport follows a redirect within the endpoint's origin (a trailing-slash redirect, say) itself, and does not follow one anywhere else, whatever the client is configured to do.
streamable_http_client itself keeps a small signature — streamable_http_client(url, *, http_client=None, terminate_on_close=True) — and now yields a 2-tuple (next section). The removed function's other parameters map onto the client you build:
</file context>
</details> ```suggestion v1's internal client set `follow_redirects=True`. You don't need it on your own client: the transport follows a method-preserving redirect within the endpoint's origin (a trailing-slash 307/308, say) itself, and does not follow one anywhere else, whatever the client is configured to do.
Sorry, something went wrong.
Reviewers
At least 1 approving review is required to merge this pull request.
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 added follow_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=True was 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 a Location header 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_client and sse_client now send each request with following disabled at the request level and handle a redirect themselves:
This holds whichever client is in use, so a caller-supplied client no longer needs follow_redirects=True for the trailing-slash case, and setting it doesn't widen what the transport follows. Requests an httpx2.Auth issues while a transport request is in flight (OAuth discovery, registration, token) inherit the per-request setting and don't follow redirects, so those URLs have to answer directly; the transports page and the OAuth page now say so, and authorization-server metadata discovery treats a redirecting well-known candidate like a missing one and tries the next. create_mcp_http_client drops follow_redirects=True and keeps only the timeouts.
How Has This Been Tested?
Breaking Changes
Types of changes
Checklist
Additional context
AI Disclaimer