Repository navigation
Conversation
github-actions
Bot
added
T-core
|
@bolinfest is on triage rotation this week and is the reviewer for this one. Labels come from the PR labeler. Please review the change. Automated triage. Labels are a first pass and the assigned maintainer has the final say. |
…treamable HTTP client
The default reqwest client used `Policy::none()`, so any redirect from an
MCP server failed with `UnexpectedServerResponse("HTTP 307: ...")`.
It now follows 307 and 308 when the target has the same origin as the
original request, up to 10 hops. Those keep the method and body, so the
JSON-RPC message and the session, auth and custom headers arrive intact.
301, 302 and 303 are still returned to the caller, because following them
turns the POST into a bodyless GET. Cross-origin redirects are still
returned too, which keeps the header-leak guard.
Refs modelcontextprotocol#936
Reported-by: Mo (@trickyfalcon)
Document on `from_uri` and `from_config` which redirects the default client follows, that a 308 does not change the stored URI, and that a client passed to `with_client` keeps its own redirect policy.
c818296 to
a821a5e
Compare
|
Dale signed off on the approach in #1203 (#1203 (comment)). I've added the redirect note he asked for to the |
DaleSeo
left a comment
DaleSeo
left a comment
There was a problem hiding this comment.
Thanks for the fix, @gocamille! I left very minor feedback.
| /// Only same-origin 307 and 308 redirects are followed, up to | ||
| /// [`MAX_REDIRECTS`] hops. Those keep the method and body, so the | ||
| /// JSON-RPC message reaches the new location intact. A 301, 302 or 303 | ||
| /// would turn a POST into a bodyless GET and lose the message, and a | ||
| /// cross-origin redirect would replay the session id and caller-supplied | ||
| /// custom headers to another host, so both are returned to the caller as | ||
| /// the redirect response instead. A followed 308 does not update the | ||
| /// stored URI, so each request still starts at the configured one. A | ||
| /// client passed to `with_client` keeps its own redirect policy. |
There was a problem hiding this comment.
This paragraph repeats most of the # Redirects section on from_uri, and the rationale really applies to same_origin_redirect_policy. Could we move it next to that function, where it explains the code, and shorten this doc to a pointer?
| } | ||
|
|
||
| /// What the redirect target saw for a single request. | ||
| #[derive(Debug, Default)] |
There was a problem hiding this comment.
Default is never used.
| #[derive(Debug, Default)] | |
| #[derive(Debug)] |
This branch has not been deployed
2 participants
Fixes #1203. Reported by Mo (@trickyfalcon).
Motivation and Context
The default client built by
StreamableHttpClientTransport::default_http_clientnow uses a custom reqwest redirect policy (same_origin_redirect_policy) instead ofPolicy::none():MAX_REDIRECTS(10) hops. reqwest keeps the method, the.json()body, and the session, auth and custom headers on these.UnexpectedServerResponse("HTTP 30x ...").StreamableHttpError::Client,is_redirect()), so a redirect loop can't run forever.Only the default client changes. A caller-supplied
reqwest::Clientkeeps its own policy. Thefrom_uriandfrom_configdocs now describe this redirect handling. GET and DELETE use the same rule; following 301/302 for a GET would be safe, but one rule for every method seemed simpler. Happy to loosen that.How Has This Been Tested?
Tests in
streamable_http_client.rs:default_http_client_follows_same_origin_body_preserving_redirect(307, 308): the target gets a POST with the same JSON-RPC body,Mcp-Session-Id,Authorization: Bearerand the custom header. Fails onmain.default_http_client_does_not_follow_method_rewriting_redirect(301, 302, 303): returns an error and the target is never hit. Passes onmaintoo; it's there so a broader policy can't slip in.default_http_client_stops_redirect_loop: a self-redirecting 307 ends in a redirect error after 11 requests.default_http_client_does_not_leak_custom_headers_to_redirect_target(existing) still passes; it's a cross-origin 307.Validation:
cargo test -p rmcp --all-features: passed.cargo clippy --all-targets --all-features -- -D warningsandcargo +nightly fmt --all -- --check: clean.Breaking Changes
No API changes. The default client now follows same-origin 307/308 redirects instead of returning
UnexpectedServerResponse.Types of changes
Checklist
Additional context
A 308 is followed on every request; the stored URI doesn't change. Updating it is tracked in #1373.