Split the registration request model from the registered-client record

[maxisbey]

OAuthClientInformationFull — the client's parse of the authorization server's Dynamic Client Registration response — inherited from OAuthClientMetadata, the request the client sends. That typed the response as though it had to be a request this SDK would send, so any registered value the server substituted became a ValidationError on a 2xx. This splits the two into siblings so the record parses what a server may legitimately echo.

Motivation and Context

We received a report of an authorization server whose registration response returned an application_type value outside {"web", "native"}. The server answered 201 and provisioned the client; the SDK then raised Invalid registration response: … Input should be 'web' or 'native' [type=literal_error] out of the auth flow, discarding a registration whose client_id had already been minted (and orphaning it server-side on every retry). application_type is never read again anywhere in the SDK — it's a purely cosmetic field killing an otherwise-valid registration.

This isn't specific to that field. The response model is-a request model, so every tight type on the request re-arms the same failure on the response. The same closed-typing had already been widened piecemeal for *_supported metadata, grant_types, response_types, and empty-string URLs — this is the fifth instance of one class, and the class is the inheritance.

Spec. RFC 7591 §3.2.1: the server "MAY reject or replace any of the client's requested metadata values submitted during the registration and substitute them with suitable values", and "The client or developer can check the values in the response to determine if the registration is sufficient for use" — a substituted value is a judgment call for the client, not a parse failure. The MCP spec (SEP-837) constrains only what the client sends (MUST specify an appropriate application_type); it says nothing about the response. The server here is also off-spec — OIDC Registration §2 defines only native/web — but that doesn't oblige the client to abort a successful registration over a field it doesn't use. Both are true.

Cross-SDK. Every other MCP SDK constrains what it sends and parses application_type permissively — the TypeScript SDK types it z.string().optional() with a comment giving exactly this reasoning, go/rust/csharp/ruby likewise. This SDK was the only one that fails on it.

What changed

OAuthClientMetadata (request) and OAuthClientInformationFull (registered-client record) are now siblings over a shared OAuthClientMetadataBase holding only the fields that are genuinely identical between them:

class OAuthClientMetadataBase(BaseModel): ...          # verbatim-shared metadata
class OAuthClientMetadata(OAuthClientMetadataBase): ...          # request: strict (what we send)
class OAuthClientInformationFull(OAuthClientMetadataBase): ...   # record: tolerant (what a server echoes)

• The request keeps every strict type — you still cannot construct or send application_type="browser"; the MCP MUST on the wire is unchanged and enforced by the type.
• The record accepts what a server may echo: application_type/token_endpoint_auth_method are str | None (an echoed "" reads as absent, matching the existing URL-field coercion), grant_types is list[str], redirect_uris may be absent or empty. client_id is now required — RFC 7591 §3.2.1 makes it mandatory in the response, and a "registered client" record without one was never meaningful (a body without it should not parse as success).
• Usability is judged where it matters, not at parse. The one substitution that makes minted credentials unusable — a token-endpoint auth method this client cannot apply — is reported as an OAuthRegistrationError naming the method when the registration completes, before the record is persisted or any interactive authorization begins (per §3.2.1's "sufficient for use" model). prepare_token_auth reports the same for a stored/pre-registered record. The recognized set is derived from one TokenEndpointAuthMethod type via get_args, so send-side and recognize-side cannot drift (private_key_jwt remains recognized — PrivateKeyJWTOAuthProvider's refresh path is unaffected).
• Server side (bonus fix of the same mechanism): the bundled registration endpoint previously hand-copied ~20 fields into its 201 echo and dropped application_type, so the SDK's own AS reported the default in place of a client's "web". The echo is now built from the validated request's model_dump(), and a test pins that every request field exists on the record — a field can no longer be silently omitted.
• The except ValidationError in handle_registration_response carried # pragma: no cover — the invalid-response branch was asserted unreachable, which is why this shipped untested. It's now covered and chains the cause (raise … from e).

How Has This Been Tested?

• Model-level: the record parses off-set/null/empty application_type, an unimplemented token_endpoint_auth_method, extra grant_types, empty redirect_uris; the request still rejects an off-set application_type and requires redirect_uris.
• Real handle_registration_response path (the layer the failure occurred in) with a substituted body, and with a body that isn't client information at all.
• End-to-end interaction tests: a shimmed /register returning a substituted 201 completes the full flow (tool call succeeds); a 201 assigning an unusable auth method surfaces OAuthRegistrationError with no /authorize or /token request and nothing persisted.
• Server: the 201 echoes the client's application_type for both web and native.
• Driven manually against a live hostile authorization server over a socket: the production-shape body ("confidential", null redirect URIs, extra grants) now completes with HTTP 200 where main dies with the exact literal_error above; a "" auth method reads as absent; a client_secret_jwt assignment fails at registration before any browser round-trip.
• Full suite: 100% coverage, strict-no-cover clean.

Breaking Changes

Yes — documented in docs/migration.md alongside the existing SEP-837 entry.

• OAuthClientInformationFull is no longer a subclass of OAuthClientMetadata. Code relying on isinstance(info, OAuthClientMetadata), or passing a record where a request is annotated, must reference the record type directly. validate_scope()/validate_redirect_uri() moved with the record (they're what server code calls) and are no longer on OAuthClientMetadata.
• On the record: application_type/token_endpoint_auth_method → str | None, grant_types → list[str], redirect_uris optional, client_id required. Reads are unaffected.
• An unimplementable auth method now fails legibly (at registration / token exchange) instead of an opaque ValidationError during parsing.

Conformance: no scenario puts these values in a 201 body, and SEP-837 defines no response-side requirement, so a send-strict / parse-tolerant client passes every existing check unchanged.

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
• Documentation update

Checklist

• I have read the MCP Documentation
• My code follows the repository's style guidelines
• New and existing tests pass locally
• I have added appropriate error handling
• I have added or updated documentation as needed

Additional context

Two things I considered and want to name:

• A stored record with an unusable method now errors on refresh instead of falling through. The old fall-through wasn't a working fallback — it looped into a full re-auth against the same record and failed at the token exchange anyway. Naming the permanent condition once is the intent, and such a record could never have been produced by registration before this change, so it only affects hand-supplied pre-registered credentials, where a clear error beats an endless silent invalid_client.
• The record still serves two seats: the client's parse of an untrusted response and the SDK's own AS's stored-client type (OAuthAuthorizationServerProvider.get_client, authorize). The split stops one class short of separating those, which is why redirect_uris elements stay AnyUrl (the AS compares them) and why hand-built server records lose a couple of constructor invariants they previously borrowed from the request. Requiring client_id restores the one that matters; a fuller server-record / client-response separation is the next seam, deliberately left out of this change.

_{AI Disclaimer}

[github-actions]

📚 Documentation preview

Preview	https://pr-3181.mcp-python-docs.pages.dev
Deployment	https://9930f350.mcp-python-docs.pages.dev
Commit	884bf91
Triggered by	@maxisbey
Updated	2026-07-27 19:46:38 UTC

[cubic-dev-ai]

All reported issues were addressed across 11 files

<sub>Reply with feedback, questions, or to request a fix.

Re-trigger cubic</sub>

[claude]

Beyond the inline nits, two candidate concerns were examined and ruled out: check_registration_usable accepting private_key_jwt (correct — PrivateKeyJWTOAuthProvider supplies the assertion, and the recognized set is derived from the same TokenEndpointAuthMethod literal the SDK sends, so the refresh path a private-key-JWT client inherits keeps working), and an empty-string client_id bypassing the new client_id-REQUIRED invariant on the record (not a practical issue — "" still parses but downstream guards treat a falsy client_id as absent, and no observed server mints an empty id).

Extended reasoning...

Bugs were found (three nits, posted inline), so the review body is limited to recording what else was examined this run. The two refuted candidates both target the new tolerance/usability mechanism in src/mcp/client/auth/oauth2.py and src/mcp/shared/auth.py; verifiers concluded neither is a real defect. This note is informational only — it is not an approval, and the PR (a breaking change across OAuth client/server auth code) should still get human review.

[cubic-dev-ai]

All reported issues were addressed across 10 files (changes from recent commits).

<sub>Reply with feedback, questions, or to request a fix.

Re-trigger cubic</sub>

[maxisbey]

Follow-up in 29a3942 addressing the review round; the six inline threads are replied to and resolved individually. Two further items came in without a thread, so recording them here:

Reported: required client_id makes pyright fail with ~20 always-true is not None errors. This does not reproduce — uv run --frozen pyright over the whole repo on this branch reports 0 errors, 0 warnings, and the pre-commit pyright hook passes on both commits. The 22 assert … client_id is not None sites do exist and are now dead narrowing, but strict pyright here does not flag them, so no check fails. Cleaning them would touch roughly eight unrelated test/example files for no behaviour change, so they are left as-is; happy to trim them in a follow-up if wanted.

Reported: the SDK-internal issuer binding (SEP-2352) was populatable from the untrusted response body. Confirmed — handle_registration_response parsed the wire body straight into the record, so a server-echoed "issuer" member could populate the field, and on the fallback branch where the flow does not overwrite it, that value would be persisted as the trust binding (after which every 401 mismatches and re-registers). Pre-existing rather than introduced here, but this PR is the redesign of exactly that parse boundary, so it is now sealed: issuer is cleared right after the parse — the binding is stamped by the flow and never taken from the wire — with a test that an echoed "issuer" does not seed it. Verified against a server echoing a bogus issuer: the stored binding is the flow’s own.

Also folded in from the inline threads: private_key_jwt is now rejected at registration by the authorization-code flow (it holds no key to sign the assertion) while stored records still tolerate it; an explicit null in a registration response is read as an omitted key across the record, closing the grant_types/response_types gap; the bundled server emits client_secret_expires_at: 0 per RFC 7591 §3.2.1 instead of omitting it; and the migration entry now names both exception surfaces. One item was declined as out of scope (defaulting an omitted auth method to client_secret_basic — pre-existing and unchanged here, and a semantic change that wants its own discussion).

_{AI Disclaimer}

[claude]

Beyond this run's inline notes, two other candidates were examined and ruled out: (1) widening the record's token_endpoint_auth_method to str does not break client_auth.py — its unsupported-method branch raises a handled AuthenticationError (client_auth.py:94-97), not an asserted-unreachable, and SDK-registered records are still built from the strict request model; (2) the record's now-required client_id accepting "" is not a regression — pre-PR the field was optional and an absent client_id parsed as success, so requiring it strictly narrows what passes.

Extended reasoning...

Bugs were found and posted inline, so no approval or full deferral. This note records two candidate issues that were examined and refuted this run, so later review passes don't re-explore them from scratch: the widened auth-method type on the record versus the bundled AS middleware's unsupported-method branch (verified to be an explicit, handled error path), and the empty-string client_id parse (verified to be a strict improvement over the previous optional field). Informational only — not a guarantee of correctness.

[claude]

No new issues found in this pass over 884bf91 — all items from the previous review rounds are addressed — but this is a breaking change to the public auth models and a rewrite of the parse boundary for untrusted registration responses, so it warrants a maintainer's review rather than an auto-approval.

What was reviewed this pass:

• the pre-validation issuer drop in handle_registration_response — the except ValueError covers both from_json and ValidationError layers without over-catching
• the widened placeholder stripper against non-string members — client_secret_expires_at: 0 and other falsy-but-meaningful values survive (0 != "")
• removal of the client_id truthiness guards in prepare_token_auth — safe now that the record requires client_id and the placeholder rule rejects an empty one
• the new secret-not-issued gate — a "" client_secret reads as absent via the placeholder rule, so it correctly trips the gate before the browser round-trip

Extended reasoning...

Overview

PR #3181 splits OAuthClientInformationFull (the registered-client record parsed from an untrusted DCR response) from OAuthClientMetadata (the strict registration request), making them siblings over OAuthClientMetadataBase. It touches src/mcp/shared/auth.py, the client auth flow (oauth2.py, utils.py), the bundled server registration handler, docs (migration.md, oauth-clients.md), and adds substantial model/interaction test coverage. Head commit 884bf91 is the third revision, addressing two prior review rounds from me and one finding from cubic.

Security risks

This is squarely security-sensitive territory: it changes how the client parses an untrusted authorization-server response, how the SEP-2352 issuer trust binding is populated (now sealed — dropped from the wire body before validation rather than parsed-then-cleared), which token-endpoint auth methods the flow will act on, and the bundled AS's registration echo. The direction of each change is defensive — parse-tolerant on cosmetic metadata, but client_id is now required, an empty client_id is rejected, unusable credential assignments fail closed before persistence or any browser round-trip, and the server refuses private_key_jwt registrations it could never authenticate. I found no injection, bypass, or exposure vector in this pass; the earlier issuer-seeding concern is resolved at the correct layer.

Level of scrutiny

High — production client-auth code plus an intentional breaking API change (documented in docs/migration.md per repo policy). The design decisions here (sibling split, parse-tolerant record, registration-time usability gate, required client_id, the deliberately-deferred server-record/client-response separation named in the PR description) are exactly the kind of API-shape calls the guidelines say a human maintainer should own, regardless of correctness. That, not any outstanding defect, is why this defers rather than approves.

Other factors

All prior review threads are resolved with code changes rather than pushback (one item — defaulting an omitted auth method to client_secret_basic — was reasonably declined as an out-of-scope semantic change). Test coverage is strong: model-level parametrization over placeholder styles, real handle_registration_response paths including malformed/non-object/non-UTF-8 bodies, end-to-end interaction tests asserting no authorize/token traffic and nothing persisted on the unusable paths, and server-side 201-body assertions where the earlier omission actually lived. The repo requires 100% coverage in CI, which backstops the branch-level claims.

[maxisbey]

Second review round addressed in 884bf91; the four new inline threads are replied to and resolved individually. Three further reported items had no thread, so recording them here:

Reported (again): required client_id makes pyright fail with ~20 always-true is not None errors. Still does not reproduce, and this time there is external evidence: on this branch pyright reports 0 errors, 0 warnings locally, and CI is fully green — checks / pre-commit (which runs pyright), all-green, and the whole test matrix all pass on the pushed commit. The 22 assert ... client_id is not None sites are real but this configuration does not flag them, so no check fails and there is nothing to fix here.

Reported: stale prose about auth-method tolerance. Both bits valid and fixed. The comment, test docstring, and migration line no longer claim the private_key_jwt refresh path "keeps working" as such: PrivateKeyJWTOAuthProvider signs its assertion only in the client-credentials exchange, so the tolerance's real job is to keep prepare_token_auth from raising out of the refresh step — a refresh the server rejects then falls back to a fresh, signed exchange. The stale Raises: text in prepare_token_auth now says it fires only for stored/pre-registered records, the registration-time judgment being check_registration_usable.

Reported: bundled AS confirms private_key_jwt registrations it can never authenticate. Valid and fixed rather than deferred: the registration endpoint now answers token_endpoint_auth_method: private_key_jwt with 400 invalid_client_metadata. This server authenticates token requests with the secret it mints and holds no client key to verify an assertion, so confirming that method would register a client whose every request it then rejects — one this SDK's own client would immediately declare unusable, orphaning the record. Interaction test covers the refusal; a client_secret_basic control still registers.

Two more from the inline threads worth naming in one place: an SDK-internal issuer is now dropped from the untrusted body before validation (never sourced from the wire, and a malformed value can no longer fail the parse), and both server placeholder styles (null and "") now read as omitted keys across the record, which also makes an empty client_id an ordinary missing-client_id rejection.

One item remains deliberately out of scope — treating an omitted token_endpoint_auth_method as RFC 7591's client_secret_basic default. Two reviewers have now raised it; it is pre-existing and byte-identical to main, and inferring a method the server did not state is a real behavioural change with its own breakage surface, so it will get its own issue and discussion rather than riding on this change.

_{AI Disclaimer}