1 issue found across 15 files

Reply with feedback, questions, or to request a fix.

Fix all with cubic | Re-trigger cubic

I didn't find any bugs in my review, but this PR adds a new OAuth grant type across the server token endpoint, provider protocol, AS metadata, and a new client auth extension — security-sensitive surface that warrants a human reviewer's sign-off.

Overview

This PR implements SEP-990 / RFC 8693 token exchange end to end: a new TokenExchangeRequest branch in the server token handler, a new exchange_token method on OAuthAuthorizationServerProvider, a token_exchange_enabled flag threaded through AuthSettings, create_auth_routes, and AS metadata, a new TokenExchangeToken response model, and a new client-side TokenExchangeOAuthProvider extension, plus docs, snippets, and substantial test coverage (~480 lines of server tests, ~220 of client tests).

Security risks

The change is squarely in the auth path. The design looks careful — the grant is opt-in and gated both at metadata advertisement and at the /token endpoint, the default exchange_token rejects with unsupported_grant_type, the handler still enforces the client's registered grant_types, requested scopes are passed to the provider rather than granted verbatim, and RFC 8693 actor-token pairing is validated. Subject-token validation is delegated entirely to provider implementations, and advertising the none token-endpoint auth method when the flag is on is a deliberate policy choice — both are decisions a human maintainer should explicitly endorse. There is also an outstanding bot comment about an assert in the example server snippet.

Level of scrutiny

High. New OAuth grant handling on the authorization server token endpoint is among the most security-sensitive code in the SDK, and the PR also introduces new public API surface (provider method, settings flag, client extension, shared model) that implies long-term maintenance commitments. This is well outside the simple/mechanical category eligible for shadow approval.

Other factors

The automated bug-hunting pass found no issues, tests are extensive (success, error, gating, disabled-flag, actor-token pairing, basic-auth, metadata advertisement cases), and the existing authorization-code/refresh-token paths are only touched by removing now-covered pragma: no cover markers. Those are positive signals, but they don't substitute for human judgment on the spec-conformance and API-design questions above.

I went through this fairly carefully against the SEP-990 normative text, RFC 8693, RFC 7523, and the sibling SDKs. There's one architectural issue that I think needs resolving before the rest is worth iterating on, plus a cluster of client-side discovery concerns and some smaller hardening points.

Reviewed at 55c2413.

Wrong grant type for the MCP-AS leg

The normative SEP-990 text — ext-auth/specification/stable/enterprise-managed-authorization.mdx §5 — specifies that the client presents the ID-JAG to the MCP authorization server using the RFC 7523 jwt-bearer grant:

The MCP Client presents the ID-JAG to the Resource Authorization Server's token endpoint as defined in [§4.4 of draft-ietf-oauth-identity-assertion-authz-grant-04], using grant_type urn:ietf:params:oauth:grant-type:jwt-bearer and the ID-JAG as the assertion.

RFC 8693 token-exchange is leg 1 only (client → enterprise IdP, ID-token → ID-JAG), which this PR correctly scopes out via subject_token_provider. But for leg 2 the PR sends and accepts grant_type=urn:ietf:params:oauth:grant-type:token-exchange with subject_token=<ID-JAG> instead.

The other SDKs all match the spec on this — typescript-sdk's crossAppAccess.ts uses token-exchange against the IdP and jwt-bearer against the MCP AS; go-sdk's auth/extauth/enterprise_handler.go and csharp-sdk's IdentityAssertionGrant.cs likewise. A python-sdk AS built from this PR rejects all of them with unsupported_grant_type (the discriminated union doesn't include jwt-bearer), and the python client is rejected by any spec-conforming AS.

Beyond interop, the grant-type choice has security weight. RFC 7523 §3 carries normative validation rules (the JWT MUST contain aud identifying the AS, iss/exp/signature MUST be checked) that map directly onto SEP-990 §5.1's processing rules — typ=oauth-id-jag+jwt, aud = own issuer, client_id claim ↔ authenticated client. RFC 8693 imposes nothing on subject_token, so those checks become provider-discretionary rather than something the SDK can structurally encourage. The §5.1 MUST that "the issued access token MUST be audience-restricted to the MCP Server identified by the resource claim in the ID-JAG" is similarly hard to surface when the SDK never decodes the assertion.

Discovery is also different from what's implemented: ext-auth §6 says clients detect support via authorization_grant_profiles_supported containing urn:ietf:params:oauth:grant-profile:id-jag (per draft-04 §7.2), not via grant_types_supported containing the token-exchange URN.

I think this means a reshape rather than a patch: a JwtBearerRequest (grant_type + assertion) on the server, a provider hook that's documented against RFC 7523 §3 + the §5.1 processing rules, and a client that posts assertion instead of subject_token. A lot of the smaller findings below (issued_token_type, token_type: N_A, actor-token pairing, multi-valued resource/audience) become moot under jwt-bearer since the response is plain RFC 6749.

Client: pre-registered credentials go through RS-controlled discovery

TokenExchangeOAuthProvider reuses the base OAuthClientProvider 401→PRM→ASM discovery flow, which was designed around DCR — the client_secret comes from the discovered AS, so sending it back there is harmless. Here the secret is pre-supplied for a specific known AS, but the constructor offers no way to pin that AS, and _fixed_client_info is built with issuer=None (token_exchange.py#L98-L105), so the existing credentials_match_issuer guard always passes.

Three concrete consequences I was able to demonstrate with httpx.MockTransport driving the real async_auth_flow:

• A hostile/compromised resource server that publishes authorization_servers: ["https://attacker.example"] in its PRM receives the confidential-client secret in the Basic header (or form body) on the first 401. The secret isn't audience-bound, so unlike the ID-JAG it's fully replayable against the real AS.
• On the legacy fallback (PRM 404s), AS metadata is fetched from the MCP server's own origin and validate_metadata_issuer is gated on auth_server_url is not None (oauth2.py#L616-L617) — which it isn't on this path. So the server can serve {issuer:"https://corp-as.example", token_endpoint:"https://evil/steal"}, subject_token_provider is called with the real AS issuer as audience, and a real-AS-bound ID-JAG plus the client secret go to the attacker.
• More generally, the audience handed to subject_token_provider is RS-influenced. go-sdk and csharp-sdk both require the integrator to explicitly configure the MCP AS URL; if the callback honours audience properly the aud-binding limits damage, but a callback that ignores it (or wraps a static-token helper) leaks a replayable credential.

I'd suggest an expected_issuer: str constructor parameter, set _fixed_client_info.issuer = expected_issuer so the existing guard fires, and assert str(oauth_metadata.issuer) == expected_issuer before invoking subject_token_provider or attaching the secret. The legacy-path issuer-check skip is arguably a pre-existing gap in the base class, but this PR is what makes it carry pre-registered credentials.

Server-side hardening

• Requested scopes bypass per-client registration. token.py#L274 passes scope.split(" ") straight to the provider with no client_info.validate_scope() check. The same client that gets error=invalid_scope at /authorize for scope=admin gets a 200 via this branch. For SEP-990 the provider should derive entitlement from the ID-JAG, so this may be intentional, but it's inconsistent with the other two grants and the test harness provider grants params.scopes verbatim.
• DCR + this flag = unauthenticated path to exchange_token. OAuthClientMetadata.grant_types is list[Literal[...] | str], register.py#L73 only requires authorization_code to be present, and token_endpoint_auth_method="none" is honoured. Anyone can self-register a public client that includes this grant and reach provider.exchange_token with an attacker-chosen client and arbitrary subject_token. The IETF draft's §8.1 says this flow "SHOULD only be supported for confidential clients," and ext-auth §5 says "the MCP Client authenticates with its credentials" — defaulting to none and advertising it in metadata sits uneasily with that. Stripping the grant from DCR-registered clients (require pre-registration) would be the simple fix.
• refresh_token passthrough. The wrap at token.py#L289 preserves a provider-set refresh_token. Returning one defeats the IdP lifetime control that SEP-990 exists to provide (off-boarded user keeps minting tokens). Probably worth dropping it in the wrap, or at least flagging in the docstring.
• Issued token bound to the request's resource, not the ID-JAG's resource claim. ext-auth §5.1: the issued token MUST be audience-restricted to the resource named in the ID-JAG. The handler surfaces only the form-body resource (token.py#L275), and the example writes it straight into AccessToken.resource. A client holding an ID-JAG for resource A can send resource=B and the example mints a B-audience token.
• client arg may be unauthenticated. For none-method clients, client.client_id is self-asserted; the exchange_token docstring doesn't warn that it must not drive authorization decisions. ext-auth §5.1 requires the ID-JAG's client_id claim to match the authenticated client — worth surfacing as a documented requirement on the hook.

subject_token_provider callback signature

The callback receives only audience (token_exchange.py#L59). ext-auth §4 has the leg-1 request to the IdP carrying both audience (the AS issuer — which is what's passed, correctly) and resource (the MCP server's RFC 9728 identifier); the resulting ID-JAG MUST carry that resource claim (§4.3) for §5.1 to bind against. The SDK has the PRM-derived value via self.context.get_resource_url() but never surfaces it to the callback. Widening to (audience, resource) would let callers send a correct leg-1 request without hardcoding the server URL.

Example server (examples/snippets/servers/token_exchange_server.py)

A few things here that I'd tighten regardless of how the grant-type question lands, since this is the file people will copy:

• _validate_id_jag accepts any non-empty string. The repo's existing oauth_server.py stub fails closed (returns None); this one fails open. raise NotImplementedError(...) would match the convention and force the replacement.
• Line 60/77 stores the raw subject_token string as AccessToken.subject — a bearer credential in a field that's designed to be logged and surfaced as identity (bearer_auth.py propagates it into the auth context).
• Line 59 writes client-supplied params.resource into AccessToken.resource with no invalid_target check (ties into the §5.1 point above).
• Line 48 returns invalid_grant for a rejected subject token; RFC 8693 §2.2.2 specifies invalid_request for that case.
• It never inspects subject_token_type, actor_token, requested_token_type, or audience — a request carrying actor_token (delegation) is silently treated as plain impersonation.

Smaller / informational

• OAuthToken.token_type is Literal["Bearer"], so TokenExchangeToken can't emit RFC 8693's N_A, and the client rejects a third-party AS that returns it. Moot if reshaped to jwt-bearer.
• The client never checks issued_token_type against requested_token_type; the sibling SDKs all do. Also moot post-reshape.
• Advertising none in token_endpoint_auth_methods_supported is endpoint-wide (RFC 8414 has no per-grant slot); enforcement is per-registered-client so there's no bypass, but it tells auth-code clients the AS accepts unauthenticated /token.
• TokenExchangeRequest is in the union unconditionally, so a server with the flag off still emits RFC 8693 field-level validation errors (e.g. the actor-token-pairing message) before the gate at line 260 runs.
• The # pragma: no cover on the oauth_metadata guard at token_exchange.py#L119-L120 is reachable when ASM discovery 404s.
• OAuthAuthorizationServerProvider is a Protocol, so the default exchange_token body is only inherited by explicit subclasses; a structural-only implementer hits AttributeError → 500 rather than unsupported_grant_type.
• scope.split(" ") forwards empty-string entries on multi-space input.

Happy to share the repro tests for any of the above if useful.

AI Disclaimer

3 issues found across 25 files (changes from recent commits).

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/server/auth/routes.py">

<violation number="1" location="src/mcp/server/auth/routes.py:180">
P2: Authorization-server metadata no longer advertises `none` even though the server still accepts `token_endpoint_auth_method="none"`, creating a protocol/metadata mismatch for public clients.</violation>
</file>

Tip: Review your code locally with the cubic CLI to iterate faster.

Fix all with cubic | Re-trigger cubic

Thanks for the quick reshape — went back through at 7783020 and everything you listed checks out. The grant-type, discovery field, confidential-only posture, DCR rejection, callback signature, AS-pinning constructor, fail-closed example, and the docstring duties are all in place, and a TS/Go/C#-shaped jwt-bearer request now reaches exchange_identity_assertion end-to-end.

On your two flagged items: I'm fine with scope bounding and resource-claim binding staying as documented provider responsibilities. The SDK can't enforce either without decoding the assertion, and the docstring + example now make the contract unambiguous.

There's one thing I think still needs closing before this is safe to ship, plus a smaller hardening point and some nits.

Legacy no-PRM path still leaks credentials

The expected_issuer pin at identity_assertion.py#L131-L135 checks the metadata document's self-reported issuer field — but not where the document was fetched from, nor where token_endpoint points. On the legacy fallback (PRM 404s → ASM fetched from the MCP server's own origin, utils.py#L164-L168), validate_metadata_issuer is still skipped because auth_server_url is None. So a hostile resource server that 404s PRM and serves {issuer: <expected_issuer>, token_endpoint: "https://evil/steal"} from its own well-known passes both the SEP-2352 guard and the new pin; assertion_provider is called with audience=<real-AS> (so an IdP audience allowlist is satisfied), and the ID-JAG plus client_secret go to the attacker. I have a MockTransport repro that completes the flow silently with an attacker-issued access token — happy to attach.

Since expected_issuer is now a required input, the simplest fix is to set self.context.auth_server_url = self._expected_issuer in _initialize(). ASM is then fetched directly from the pinned AS's well-known, validate_metadata_issuer runs, and the resource server can't forge the document at all — which is what go-sdk and csharp-sdk effectively do by taking the AS URL as config. That also closes a smaller side-effect on the PRM path: when PRM names an unexpected AS, the SEP-2352 guard currently clears client_info and the base flow attempts DCR against the attacker's /register (no secret leaked, but it discloses the client metadata and overwrites storage) before the pin at line 132 raises.

Confidential-client gate trusts the declared method

token.py#L255 rejects only token_endpoint_auth_method == "none", but ClientAuthenticator (unchanged) only compares secrets if client.client_secret: — a stored record with client_secret=None is returned without any credential check. So a pre-registered client with token_endpoint_auth_method="client_secret_post" and no secret passes both and reaches the hook unauthenticated, which makes the docstring's "the handler guarantees client is confidential" stronger than what's enforced. DCR-minted clients aren't affected (register.py always issues a secret for non-none); it only bites operator-provisioned records. Gating on client_info.client_secret being set (or having ClientAuthenticator reject a non-none client with no stored secret) would make the guarantee true.

Smaller

• token.py#L258: RFC 6749 §5.2 reserves invalid_client for failed authentication; "authenticated client may not use this grant" is unauthorized_client.
• Stale RFC 8693 citations: provider.py#L101 (invalid_target is now used for RFC 8707) and identity_assertion_server.py#L97-L98 (RFC 7523 §3.1 specifies invalid_grant for a rejected assertion).
• The §5.1 checklist in provider.py#L300-L311 and the example header omit RFC 7523 §3's sub MUST and the jti/replay guidance — worth adding since the docstring is the only thing steering implementers.
• The module docstring and migration.md say "SEP-990 §5.1 requires a confidential client, so client_secret is mandatory" — the spec requires client authentication, not specifically a shared secret (ext-auth §5 mentions private_key_jwt for the CIMD case). Probably worth wording as "this SDK currently requires a shared secret" so it doesn't read as a spec constraint.
• JWT_BEARER_GRANT_TYPE is duplicated as a literal in four files — minor.

The carry-over info-level items from the first round (request model parsed before the gate; Protocol default body only inherited by explicit subclasses; the # pragma: no cover at identity_assertion.py#L125 still being reachable; scope.split(" ") empty entries) all still apply but I don't think any of them block.

AI Disclaimer

2 issues found across 5 files (changes from recent commits).

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/auth/extensions/identity_assertion.py">

<violation number="1" location="src/mcp/client/auth/extensions/identity_assertion.py:37">
P2: Origin comparison does not normalize default ports, so valid same-origin endpoints can be rejected. This can fail exchanges against metadata that publishes an explicit default port.</violation>

<violation number="2" location="src/mcp/client/auth/extensions/identity_assertion.py:181">
P1: Configured scopes can be unintentionally expanded with server-advertised scopes. This may request broader privileges than the caller asked for during initial 401 authorization.</violation>
</file>

Tip: Review your code locally with the cubic CLI to iterate faster.

Fix all with cubic | Re-trigger cubic

1 issue found across 7 files (changes from recent commits).

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/server/auth/routes.py">

<violation number="1" location="src/mcp/server/auth/routes.py:180">
P2: Authorization-server metadata no longer advertises `none` even though the server still accepts `token_endpoint_auth_method="none"`, creating a protocol/metadata mismatch for public clients.</violation>
</file>

<file name="src/mcp/client/auth/extensions/identity_assertion.py">

<violation number="1" location="src/mcp/client/auth/extensions/identity_assertion.py:150">
P1: Issuer-pinning check is too permissive: it accepts PRMs where the pinned issuer is present but not selected, allowing attacker-first authorization_servers entries to bypass the new guard.</violation>
</file>

Tip: Review your code locally with the cubic CLI to iterate faster.

Fix all with cubic | Re-trigger cubic

1 issue found across 2 files (changes from recent commits).

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/server/auth/routes.py">

<violation number="1" location="src/mcp/server/auth/routes.py:180">
P2: Authorization-server metadata no longer advertises `none` even though the server still accepts `token_endpoint_auth_method="none"`, creating a protocol/metadata mismatch for public clients.</violation>
</file>

Tip: Review your code locally with the cubic CLI to iterate faster.

Fix all with cubic | Re-trigger cubic