Skip to content

fix(transport): keep 401/403 status when the body is a JSON-RPC error - #1341

Merged
DaleSeo merged 1 commit into
modelcontextprotocol:mainfrom
aton-of-data:fix/auth-status-json-error-body
Oct 21, 2026
Merged

DaleSeo merged 1 commit into
modelcontextprotocol:mainfrom
aton-of-data:fix/auth-status-json-error-body

Conversation

@aton-of-data

Copy link
Copy Markdown
Contributor

Fixes #1339

What's wrong

When a Streamable HTTP server answers a POST with 401 or 403, no WWW-Authenticate header, and a JSON-RPC error body, the reqwest client returns Ok(StreamableHttpPostResponse::Json(..)). The caller sees an ordinary JSON-RPC error (ClientInitializeError::JsonRpcError during initialize, ServiceError::McpError afterwards) and has no way to tell that the server rejected its credentials. The same 401 with a plain-text body already comes back as UnexpectedServerResponse("HTTP 401 Unauthorized: …"), so the status only gets lost when the body is JSON-RPC.

Why it happens (line numbers at c1a8eea, crates/rmcp/src/transport/common/reqwest/streamable_http_client.rs):

  • The 401/403 checks at L212 and L227 only fire when WWW-Authenticate is present. A 401/403 without the header falls through.
  • It reaches the non-success branch at L278. After legacy_discover_response, the JSON branch at L289-301 parses the body with parse_json_rpc_error and returns it as Json without looking at the status.
  • Only non-JSON bodies reach the HTTP {status}: {body} error at L303.

The fix

The JSON-RPC branch is now skipped for 401 and 403. These responses fall through to the existing UnexpectedServerResponse("HTTP {status}: {body}"), the same result as a plain-text 401/403. Other 4xx/5xx JSON-RPC bodies are still returned as Json, as #748 intended.

Why this shape:

  • It matches the precedent in the same module. legacy_discover_response (crates/rmcp/src/transport/streamable_http_client.rs L374, L383) says "Keep authentication failures and server errors on their original paths" and excludes 401/403 for that reason.
  • It matches the unix-socket client. Its non-success branch (crates/rmcp/src/transport/common/unix_socket.rs L270-284) never parses a JSON-RPC body, so the same response already ends up as UnexpectedServerResponse there.
  • It matches existing behaviour on the GET path. get_stream_401_without_challenge_is_not_auth_required in tests/test_streamable_http_get_stream_auth_challenge.rs already says that a 401 without a challenge must be an error but not AuthRequired.
  • I did not return AuthRequired/InsufficientScope with an empty challenge. www_authenticate_header is a required String (L139, L155). StreamableHttpError::auth_challenge() (L261) and ClientInitializeError::auth_challenge() (service/client.rs) would then return Some("") as a challenge for AuthorizationRequest::with_challenge. AuthClient would also run its refresh-and-retry path (transport/common/auth/streamable_http_client.rs L47). Carrying the status alongside the JSON-RPC error (option (b) in the issue) would mean changing public types.
  • No public types change.

If you would rather have a dedicated variant for "401/403 without a challenge", that would be a follow-up with an API change. I kept this PR to the smallest fix that stops the status from being lost.

How it was reproduced

I added two tests to crates/rmcp/tests/test_streamable_http_4xx_error_body.rs, using the same local axum mock server the file already uses. They POST a ping to a server that answers 401 (and 403) with application/json, a JSON-RPC error body and no WWW-Authenticate. They assert UnexpectedServerResponse whose message starts with HTTP 401 Unauthorized (and HTTP 403 Forbidden).

On unmodified main, both tests fail:

expected UnexpectedServerResponse, got: Ok(Json(Error(JsonRpcError { jsonrpc: JsonRpcVersion2_0, id: Some(Number(1)), error: ErrorData { code: ErrorCode(-32001), message: "Unauthorized", data: None } }), None))
test result: FAILED. 4 passed; 2 failed

With the fix, test result: ok. 6 passed; 0 failed.

What was verified

All commands were run with stable 1.97, because the pinned 1.96 toolchain and nightly could not be downloaded in my environment.

  • cargo test -p rmcp --features client,transport-streamable-http-client,transport-streamable-http-client-reqwest --test test_streamable_http_4xx_error_body: 2 failed / 4 passed before the fix, 6 passed after.
  • cargo test -p rmcp --all-features: 885 passed, 0 failed, 15 ignored. Note that this file is gated on not(feature = "local"), so it does not run here.
  • cargo test -p rmcp --features "<all rmcp features except local and __*>" (the justfile's second test step): 1316 passed, 0 failed, 18 ignored. This run includes the new tests.
  • cargo test -p rmcp --lib --no-default-features: 139 passed.
  • cargo clippy --all-targets --all-features -- -D warnings and the CI's no-default-features "all except local" clippy: both clean apart from one clippy::question_mark hit in transport/auth.rs:2501. That hit is in a file this PR doesn't touch, and it also appears on unmodified main with clippy 1.97. I reran both with -A clippy::question_mark and they passed.
  • Formatting: I could not install nightly, so I ran stable rustfmt --edition 2024 --check on the two touched files only. It passes. The stable formatter skips the nightly-only import options, and this change touches no imports.

What was not verified

  • cargo +nightly fmt --all -- --check, as CI runs it.
  • Clippy and tests on the pinned 1.96 toolchain or the MSRV.
  • cargo semver-checks / cargo public-api. No public items change, but I did not run these tools.
  • No end-to-end test through ServiceExt::serve / initialize. The tests drive StreamableHttpClient::post_message directly.

A 401 or 403 without a WWW-Authenticate header falls through to the
non-success branch of the reqwest client's post_message, where a
JSON-RPC error body was returned as an ordinary JSON response. Callers
then saw a plain JSON-RPC error and could not tell that the server had
rejected their credentials.

Skip the JSON-RPC error branch for 401 and 403 so these responses end
as UnexpectedServerResponse("HTTP 401 Unauthorized: ..."), the same as
a plain-text body, and as legacy_discover_response already does.

Fixes modelcontextprotocol#1339
@aton-of-data
aton-of-data requested a review from a team as a code owner October 11, 2026 13:37
@github-actions github-actions Bot added T-test Testing related changes T-core Core library changes T-transport Transport layer changes labels Oct 11, 2026

@DaleSeo DaleSeo 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 for the fix, @aton-of-data!

@DaleSeo
DaleSeo merged commit e0e7e50 into modelcontextprotocol:main Oct 21, 2026
24 checks passed
@github-actions github-actions Bot mentioned this pull request Oct 21, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

T-core Core library changes T-test Testing related changes T-transport Transport layer changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Streamable HTTP client: a 401 or 403 with a JSON-RPC error body and no WWW-Authenticate loses its HTTP status

2 participants