Skip to content

fix(auth): map 401/403 challenges on the SSE GET stream - #1152

Merged
DaleSeo merged 1 commit into
modelcontextprotocol:mainfrom
shoemoney:fix/get-stream-auth-challenge
Aug 7, 2026
Merged

fix(auth): map 401/403 challenges on the SSE GET stream#1152
DaleSeo merged 1 commit into
modelcontextprotocol:mainfrom
shoemoney:fix/get-stream-auth-challenge

Conversation

@shoemoney

@shoemoney shoemoney commented Aug 7, 2026

Copy link
Copy Markdown

Summary

post_message_with_max_sse_event_size maps a 401 or 403 carrying a WWW-Authenticate header onto StreamableHttpError::AuthRequired / InsufficientScope. get_stream_with_max_sse_event_size, in the same file, handles only 405 and then falls straight through to error_for_status()?, so the identical challenge comes back as an opaque StreamableHttpError::Client.

crates/rmcp/src/transport/common/reqwest/streamable_http_client.rs:

401 + WWW-Authenticate 403 + WWW-Authenticate
post_message_… (L181–211) AuthRequired InsufficientScope
get_stream_… (L94–97, before this PR) Client(Status(401)) Client(Status(403))
unix_socket.rs get_stream_… (L441–472) AuthRequired InsufficientScope

Why it matters

AuthClient::get_stream routes through call_reacting_to_challenges, whose entire job is to catch AuthRequired, call try_refresh_or_reauth(), and retry once. It never receives that variant from this path, so an expired token on the standalone SSE stream is never refreshed — the stream just fails. The same expiry on post_message recovers silently, which makes this look like an intermittent SSE problem rather than an auth one.

The change

The two blocks copied verbatim from post_message in the same file, inserted before error_for_status()?. No new imports — WWW_AUTHENTICATE, Cow, extract_scope_from_header and the error types are already in scope for the sibling.

Testing

crates/rmcp/tests/test_streamable_http_get_stream_auth_challenge.rs, built on the harness in the existing test_streamable_http_4xx_error_body.rs (same axum mock-server shape).

Test Before After
401 + challenge → AuthRequired Err(Client(reqwest::Error { kind: Status(401, None) }))
403 + challenge → InsufficientScope, scope extracted Err(Client(… Status(403, None) …))
401 without challenge → not AuthRequired
405 → ServerDoesNotSupportSse

The bottom two pass either way on purpose: they exist to catch an over-broad fix, not to demonstrate the bug. Without a challenge header there is nothing for the caller to act on, so a bare 401 must keep falling through to the ordinary error path, and 405 must keep its dedicated meaning.

cargo test -p rmcp --test test_streamable_http_get_stream_auth_challenge \
  --features transport-streamable-http-client,transport-streamable-http-client-reqwest,client,auth

  with this change:  4 passed; 0 failed
  with only the source change reverted:  2 passed; 2 failed

cargo test -p rmcp --lib with the same features: 416 passed / 0 failed, identical to the baseline on main — no regression, and the delta is entirely in the new integration binary. cargo fmt -p rmcp -- --check clean and touching only these files; cargo clippy -p rmcp --lib exits 0 with no errors.

Notes

Toolchain. Built with stable 1.97.1; rust-toolchain.toml pins 1.96 and .githooks runs cargo +nightly fmt. There is no rustup on this machine, so formatting was applied with stable rustfmt and the repo's nightly-only options (imports_granularity, group_imports) were skipped with a warning. This change adds no imports and the formatting is unchanged from the copied sibling, but CI on 1.96 is the authoritative check.

Deliberately not fixed. get_stream in unix_socket.rs already handles both cases, so nothing to do there. I have not touched auth.rs#1102 is open against that file and this PR does not overlap it.


AI assistance disclosure

Per the modelcontextprotocol AI policy: this change was made in conjunction with my pair programmer, Claude Code.

Extent, so you know how much scrutiny to apply: the defect was surfaced by an automated sweep I run across MCP-ecosystem repos, and the patch was written with Claude Code working alongside me. I reviewed it before filing — the before/after test output, the baseline test counts, and the lint/format runs quoted above were executed on my machine, not pasted from a model. I understand what the change does and why, and replies on this PR are mine.

`post_message_with_max_sse_event_size` turns a 401 or 403 carrying a
`WWW-Authenticate` header into `StreamableHttpError::AuthRequired` /
`InsufficientScope`. `get_stream_with_max_sse_event_size`, in the same file,
handles only 405 and then falls through to `error_for_status()?`, so the same
challenge becomes an opaque `StreamableHttpError::Client`.

That matters because `AuthClient::get_stream` routes through
`call_reacting_to_challenges`, whose whole purpose is to catch `AuthRequired`,
run `try_refresh_or_reauth()`, and retry once. It never sees that variant from
this path, so an expired token on the standalone SSE stream is never refreshed —
the stream just fails, while the identical expiry on `post_message` recovers
silently.

Copies the two blocks verbatim from `post_message` in the same file.
`unix_socket.rs` already does the same thing in its own `get_stream`.

Adds four tests against an axum mock server: a 401 with a challenge maps to
`AuthRequired`, a 403 maps to `InsufficientScope` with the scope extracted, a
401 *without* a challenge is still not `AuthRequired`, and 405 keeps reporting
`ServerDoesNotSupportSse`. The first two fail before this change with
`Err(Client(reqwest::Error { kind: Status(401, None) }))`; the last two pass
either way and exist to catch an over-broad fix.
@github-actions github-actions Bot added T-test Testing related changes T-core Core library changes T-transport Transport layer changes labels Aug 7, 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, @shoemoney!

@DaleSeo
DaleSeo merged commit c345078 into modelcontextprotocol:main Aug 7, 2026
22 checks passed
@github-actions github-actions Bot mentioned this pull request Aug 7, 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.

2 participants