Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
fix(client/auth): finalize the discovery sub-generator when the flow …
…is closed mid-discovery

httpx2's _send_handling_auth acloses the auth flow on any transport error or
cancellation, throwing GeneratorExit at the relay's yield — caught by neither
except StopAsyncIteration nor except Exception — so both hand-relay sites (the
401 branch and the 403 step-up) abandoned the suspended
_discover_authorization_server_metadata sub-generator to GC (a ResourceWarning
on trio, which filterwarnings=["error"] turns into a test failure for any
future abort-mid-discovery test).

Both relays now drive the sub-generator under contextlib.aclosing, matching the
eager-refresh relay in #3263, so closing the outer flow finalizes it in the
same unwind. Covered by two regression tests that close the flow mid-discovery
and assert the captured sub-generator reports exhaustion instead of a live
suspended frame.
  • Loading branch information
claude committed Aug 10, 2026
commit aedc88c780e043e3dede817bcd2525cfd456e5dd
39 changes: 22 additions & 17 deletions src/mcp/client/auth/oauth2.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
import string
import time
from collections.abc import AsyncGenerator, Awaitable, Callable
from contextlib import aclosing
from dataclasses import dataclass, field
from typing import Any, Protocol, get_args
from urllib.parse import quote, urlencode, urljoin, urlparse
Expand Down Expand Up @@ -812,15 +813,17 @@ async def async_auth_flow(self, request: httpx2.Request) -> AsyncGenerator[httpx
# metadata, applying the SEP-2352 issuer checks along the way. The
# sequence lives in a sub-generator shared with the 403 step-up;
# its requests are relayed by hand (`yield from` cannot cross an
# async generator).
discovery = self._discover_authorization_server_metadata(response)
try:
discovery_request = await anext(discovery)
while True:
discovery_response = yield discovery_request
discovery_request = await discovery.asend(discovery_response)
except StopAsyncIteration:
pass
# async generator). `aclosing` finalizes the sub-generator when
# httpx2 closes this flow mid-discovery (transport error or
# cancellation throws `GeneratorExit` at the relay's `yield`).
async with aclosing(self._discover_authorization_server_metadata(response)) as discovery:
try:
discovery_request = await anext(discovery)
while True:
discovery_response = yield discovery_request
discovery_request = await discovery.asend(discovery_response)
except StopAsyncIteration:
pass

# Step 3: Apply scope selection strategy
self.context.client_metadata.scope = get_client_metadata_scopes(
Expand Down Expand Up @@ -884,14 +887,16 @@ async def async_auth_flow(self, request: httpx2.Request) -> AsyncGenerator[httpx
if self.context.oauth_metadata is None and (
self.context.registration_secret_expired() or self.context.client_info is None
):
Comment on lines +887 to +889

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟣 Pre-existing issue (the 403 branch had no discovery before this PR either): the new step-up discovery guard runs only when re-registration is coming (registration_secret_expired() or client_info is None), but the restart state that motivated it — _initialize() restores tokens and client info but never oauth_metadata, and a reloaded live access token keeps the 401 flow's discovery from running — applies equally when the stored registration is still valid. In that case the step-up authorizes blind against urljoin(resource_origin, "/authorize") / "/token"), burning an interactive consent per 403 in the separate-AS topology; widening the guard to run discovery whenever oauth_metadata is None (dropping the second conjunct) would close it, since the SEP-2352 issuer checks no-op for a matching issuer.

Extended reasoning...

What the bug is. The 403 insufficient_scope step-up added in this PR runs the shared discovery sub-generator only under the guard at src/mcp/client/auth/oauth2.py:884-886:

if self.context.oauth_metadata is None and (
    self.context.registration_secret_expired() or self.context.client_info is None
):

i.e. only when re-registration is about to happen (lapsed secret, or no stored record). But the restart state that motivated commit 43d9599 — _initialize() restores only current_tokens and client_info, never oauth_metadata/protected_resource_metadata, and token_expiry_time is None after reload so is_token_valid() is True and the 401 flow's discovery never runs — applies just as much when the stored registration is valid. For that population the guard is False, discovery is skipped, and the step-up proceeds to _perform_authorization() with oauth_metadata still None.

The code path. With oauth_metadata=None, _perform_authorization_code_grant() falls back to urljoin(resource_origin, "/authorize") (the else branch around oauth2.py:417-420) and the exchange targets urljoin(resource_origin, "/token") via _get_token_endpoint(). In the standard separate-AS topology those endpoints do not exist on the resource server. Note the 403's WWW-Authenticate resource_metadata pointer is available on this path — extract_resource_metadata_from_www_auth now accepts 403s per this PR — but only the (skipped) discovery path reads it.

Step-by-step proof. (1) Storage holds a valid, unexpired DCR record and a live access token; the process restarts. First request: _initialize() loads both, token_expiry_time is None, so the Bearer header is attached and no 401 occurs — discovery never runs. (2) The request needs a broader scope: 403 insufficient_scope. The guard is False (record valid, client_info present), so no discovery runs. (3) Step 2b calls _perform_authorization(): the user's browser is sent to a dead https://resource-origin/authorize URL — a burned interactive consent — and the flow fails (the callback never completes, or the exchange 404s and _handle_token_response raises OAuthTokenError). (4) The except re-raises without resetting any state, so every subsequent 403 repeats identically until the access token itself expires and the 401 flow finally discovers for real.

Why existing safeguards miss it. The 401 flow always discovers before authorizing; the 403 branch relies on cached oauth_metadata. The guard added in 43d9599 restores discovery only for the re-registration populations (that fix targeted the blind registration POST), deliberately leaving the valid-record population out — which never registers, but still authorizes and exchanges against the fallback endpoints. No test covers oauth_metadata=None + valid record + 403: all the new restart-shaped 403 tests use a lapsed secret.

Why pre_existing, not normal. Before this PR the 403 branch performed no discovery at all, so the valid-record population's behavior is byte-for-byte unchanged — the PR neither introduced nor worsened it. All three verifiers agreed on this grading. It is still worth flagging here because the PR rewrote this exact branch and added the machinery (_discover_authorization_server_metadata) that would fix it.

How to fix. Widen the guard to run discovery whenever self.context.oauth_metadata is None — i.e. drop the second conjunct. Running discovery with a valid record is safe: the SEP-2352 issuer checks either pass (no-op for a matching issuer) or correctly discard cross-issuer credentials, and the step-up then authorizes at the discovered authorization_endpoint. Same one-line shape as the existing fix, plus a regression test for oauth_metadata=None + valid record + 403.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Acknowledged — this is a real gap: after a restart with a still-valid registration, a 403 step-up authorizes against the {origin}/authorize fallback with no metadata cached. It's pre-existing behavior on main, though (the 403 branch never ran discovery before this PR), and this PR neither introduced nor worsens it — the guard added here deliberately covers only the re-registration path this PR touches. Widening it to any oauth_metadata is None case is better handled as a follow-up alongside the other eager-discovery work: #3263 covers the refresh path, and a follow-up can widen the 403/authorize path guard with its own regression test. Leaving code unchanged here.


Generated by Claude Code

discovery = self._discover_authorization_server_metadata(response)
try:
discovery_request = await anext(discovery)
while True:
discovery_response = yield discovery_request
discovery_request = await discovery.asend(discovery_response)
except StopAsyncIteration:
pass
# `aclosing` mirrors the 401 relay above: it finalizes the
# sub-generator when httpx2 closes this flow mid-discovery.
async with aclosing(self._discover_authorization_server_metadata(response)) as discovery:
try:
discovery_request = await anext(discovery)
while True:
discovery_response = yield discovery_request
discovery_request = await discovery.asend(discovery_response)
except StopAsyncIteration:
pass

# Step 2a: Union previously requested scopes with the newly challenged
# scopes (SEP-2350) so escalating one operation keeps the others' grants.
Expand Down
83 changes: 83 additions & 0 deletions tests/client/test_auth.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import base64
import json
import time
from collections.abc import AsyncGenerator
from unittest import mock
from urllib.parse import parse_qs, quote, unquote, urlparse

Expand Down Expand Up @@ -4031,3 +4032,85 @@ async def mock_callback() -> AuthorizationCodeResult:
await auth_flow.asend(httpx2.Response(200, request=final_request))
except StopAsyncIteration:
pass


@pytest.mark.anyio
async def test_closing_the_flow_mid_discovery_finalizes_the_401_discovery_sub_generator(
oauth_provider: OAuthClientProvider,
):
"""httpx2's `_send_handling_auth` acloses the auth flow when a discovery request
fails at the transport level (or the request is cancelled), throwing `GeneratorExit`
at the 401 relay's `yield`; the relay must finalize the discovery sub-generator with
the flow rather than abandon it suspended to GC (a `ResourceWarning` under trio).
The sub-generator is captured via a wrapper because its finalization is not
observable through the auth-flow protocol itself.
"""
captured: list[AsyncGenerator[httpx2.Request, httpx2.Response]] = []
original = oauth_provider._discover_authorization_server_metadata

def capturing(response: httpx2.Response) -> AsyncGenerator[httpx2.Request, httpx2.Response]:
discovery = original(response)
captured.append(discovery)
return discovery

oauth_provider._discover_authorization_server_metadata = capturing
oauth_provider._initialized = True

auth_flow = oauth_provider.async_auth_flow(httpx2.Request("GET", "https://api.example.com/v1/mcp"))
request = await auth_flow.__anext__()

# 401 → the flow yields the first discovery request, suspending both generators mid-relay.
prm_req = await auth_flow.asend(httpx2.Response(401, request=request))
assert "oauth-protected-resource" in str(prm_req.url)
assert len(captured) == 1

# Closing the flow here mirrors httpx2 aborting it on a transport error.
await auth_flow.aclose()

# The sub-generator was closed with the flow: probing it reports exhaustion instead
# of resuming a suspended frame.
with pytest.raises(StopAsyncIteration):
await captured[0].__anext__()


@pytest.mark.anyio
async def test_closing_the_flow_mid_discovery_finalizes_the_403_step_up_discovery_sub_generator(
oauth_provider: OAuthClientProvider,
):
"""The 403 step-up's discovery relay must finalize its sub-generator when httpx2
acloses the flow mid-discovery, exactly like the 401 relay (same abandonment
hazard, same fix); the sub-generator is captured via a wrapper because its
finalization is not observable through the auth-flow protocol itself.
"""
captured: list[AsyncGenerator[httpx2.Request, httpx2.Response]] = []
original = oauth_provider._discover_authorization_server_metadata

def capturing(response: httpx2.Response) -> AsyncGenerator[httpx2.Request, httpx2.Response]:
discovery = original(response)
captured.append(discovery)
return discovery

oauth_provider._discover_authorization_server_metadata = capturing
# Restart shape: a live token and no stored registration, so the 403 step-up must
# discover before registering (`oauth_metadata` is never restored by `_initialize`).
oauth_provider.context.current_tokens = OAuthToken(access_token="live-token")
oauth_provider._initialized = True

auth_flow = oauth_provider.async_auth_flow(httpx2.Request("GET", "https://api.example.com/v1/mcp"))
request = await auth_flow.__anext__()

response_403 = httpx2.Response(
403,
headers={"WWW-Authenticate": 'Bearer error="insufficient_scope", scope="write"'},
request=request,
)

# 403 → the step-up yields the first discovery request, suspending both generators mid-relay.
prm_req = await auth_flow.asend(response_403)
assert "oauth-protected-resource" in str(prm_req.url)
assert len(captured) == 1

await auth_flow.aclose()

with pytest.raises(StopAsyncIteration):
await captured[0].__anext__()
Loading