feat(client,core): application_type client metadata with native/web inference (SEP-837)#2266
feat(client,core): application_type client metadata with native/web inference (SEP-837)#2266mattzcarey wants to merge 3 commits into
Conversation
🦋 Changeset detectedLatest commit: 4c6e107 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
@modelcontextprotocol/client
@modelcontextprotocol/codemod
@modelcontextprotocol/server
@modelcontextprotocol/server-legacy
@modelcontextprotocol/express
@modelcontextprotocol/fastify
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
There was a problem hiding this comment.
Needs a rebase first (conflicting with main), and the conflict is in test/conformance/expected-failures.yaml. Main is now on conformance 0.2.0-alpha.3; this branch still pins alpha.1. As part of the rebase, two conformance things:
- Bump the pin to
0.2.0-alpha.3and reconcile the baseline. - Drop
auth/scope-step-upfromexpected-failures.yaml.
Once #2265 is in main (scope union plus the harness fix) and application_type is here, auth/scope-step-up passes end to end.
I verified it goes 28/28 with both SEPs in, so it should come off the baseline with this PR once #2265 is fixed & landed and we rebased this one on top.
7f811c4 to
d7fcacd
Compare
|
Rebased onto current Verification run on the rebased branch:
|
|
Addressed the two new review threads in
Verification:
|
There was a problem hiding this comment.
Thanks — confirmed both points from my earlier review are addressed in e5fb185 (HTTPS-loopback redirect URIs now infer 'web' with a regression test, and inferApplicationType is no longer part of the public export surface); I found no further issues, but since this changes what the SDK sends in DCR request bodies on a security-sensitive auth path — and the inference-vs-docs-only design choice is explicitly left to maintainers — it should get a human look.
Extended reasoning...
Overview
This PR (SEP-837) adds an optional application_type field to OAuthClientMetadataSchema in @modelcontextprotocol/core, and makes registerClient() in @modelcontextprotocol/client infer 'native' vs 'web' from redirect_uris when the field is absent (loopback HTTP / custom-scheme URIs → native, everything else → web). It also updates docs/client.md, adds a changeset, extends auth.test.ts with inference and round-trip tests, and reconciles the conformance expected-failures baseline.
Status of prior review feedback
Both inline comments from my previous review were addressed in e5fb1852: isNativeRedirectUri() now treats https: loopback URIs as non-native (with a dedicated regression test), and inferApplicationType is module-internal rather than exported from the package index. I verified the current code reflects both fixes, and the bug-hunting pass on this revision found no new issues.
Security risks
The change is in the OAuth dynamic client registration path. The schema addition is benign (optional, additive, still .strip()-based). The behavioral change is that registration request bodies now include an SDK-injected application_type whenever the provider omits it — a deliberate, spec-motivated change, but one that alters what is sent to authorization servers and could interact with strict OIDC registries. There is no injection, credential-handling, or token-flow change.
Level of scrutiny
Auth-flow code in this SDK warrants human maintainer review by policy, and the PR description itself flags the inference-vs-docs-only choice as a design decision for maintainers ("Happy to drop the inference entirely"). That design call, plus the conformance-baseline edits, is not something I should approve on my own even though the implementation now looks correct.
Other factors
Test coverage for the new behavior is good (loopback, custom scheme, https, https-loopback, mixed, explicit-value, response retention), the changeset and docs match the implementation, and the author reports clean conformance, typecheck, lint, and build runs on the rebased branch.
…nference (SEP-837) - Add optional application_type to OAuthClientMetadataSchema (and thus OAuthClientInformationFullSchema) so user-supplied values survive DCR request bodies and response parsing. - registerClient() infers application_type when absent: 'native' if every redirect URI is loopback (localhost, 127.0.0.1, [::1]) or a custom non-http(s) scheme, otherwise 'web'. Explicit values are never overridden. - New inferApplicationType(redirectUris) export. - JSDoc on OAuthClientProvider.clientMetadata + docs/client.md note. Closes #2198
…ted-failures baseline The application_type implementation makes the 13 scenarios that failed only on the SEP-837 DCR checks pass, and the conformance runner fails on stale baseline entries. auth/scope-step-up stays: it still emits a SEP-2350 WARNING (scope union on re-authorization), which the evaluator counts as a failure.
e5fb185 to
4c6e107
Compare
There was a problem hiding this comment.
Thanks for addressing both earlier threads in e5fb185 — the HTTPS-loopback inference now correctly yields 'web' (with a regression test), and the helper is no longer part of the public export surface; I have no remaining correctness concerns, but since this changes the DCR request body the SDK sends by default and the inference-vs-docs-only design choice is explicitly left open in the description, a maintainer should make that call.
Extended reasoning...
Overview
This PR implements SEP-837: adds an optional application_type field to OAuthClientMetadataSchema in @modelcontextprotocol/core (previously stripped by .strip()), and makes registerClient() in @modelcontextprotocol/client infer 'native' vs 'web' from redirect_uris when the field is absent. It also updates docs/client.md, adds a changeset, adds vitest coverage for the inference cases, and reconciles the conformance expected-failures baseline.
Status of prior feedback
Both issues raised in my previous review have been addressed in the current revision: isNativeRedirectUri() now restricts loopback inference to the http: scheme (HTTPS loopback falls through to 'web', with a new regression test covering https://localhost/127.0.0.1/[::1]), and inferApplicationType is no longer exported from the package index — it is a module-internal detail of registerClient(). The bug-hunting system found no issues on the current revision.
Security risks
The change touches the OAuth dynamic client registration path, but the risk surface is small: it only adds an additive optional field to request/response schemas and injects an inferred application_type into the DCR request body when the provider omits it. No token handling, redirect validation, or credential storage logic is altered. The main residual risk is behavioral, not security: clients that previously omitted the field will now send an inferred value, which strict registries could treat differently than the server-side default.
Level of scrutiny
This is a feature PR changing default client behavior in auth code, not a mechanical change, so it warrants maintainer review rather than auto-approval. In particular, the PR description explicitly leaves a design decision open to maintainers — whether to keep the inference at all or make this docs-only (schema field + JSDoc) — and per this repo's conventions (burden of proof on addition, design justification first) that call belongs to a human. Test coverage for the inference rule and the explicit-value escape hatch is good, and the conformance baseline change matches the description.
Summary
Implements SEP-837 (clarify client type requirements) for dynamic client registration. The draft authorization spec,
basic/authorization/client-registration.mdx§ "Application Type and Redirect URI Constraints", is MUST-level:Previously the SDK made compliance impossible:
OAuthClientMetadataSchemauses.strip()and had noapplication_type, so any user-supplied value was silently dropped from DCR request bodies, andOAuthClientInformationFullSchemastripped it from registration responses.What changed
@modelcontextprotocol/core(packages/core/src/shared/auth.ts):OAuthClientMetadataSchemagains optionalapplication_type(typedz.string()because OIDC permits extension values beyond'web'/'native'; JSDoc documents the standard values and SEP-837 semantics). SinceOAuthClientInformationFullSchemamerges the metadata schema, the field now also survives DCR response parsing.@modelcontextprotocol/client(packages/client/src/client/auth.ts):registerClient()infersapplication_typewhen it is absent from the provided metadata:'native'if everyredirect_urisentry is loopback (localhost,127.0.0.1,[::1]) or a custom non-http(s) scheme; otherwise'web'. The rule lives in a new exported helperinferApplicationType(redirectUris). Empty/unparseable inputs conservatively yield'web'(the OIDC default).OAuthClientProvider.clientMetadataand a short note indocs/client.md.native, custom scheme →native, https →web, mixed →web,localhost.example.comis not loopback, explicit value never overridden).clientminor (new export + behavior),corepatch (additive optional schema field).Design choice for maintainers
Inference only applies when the field is absent — an explicitly provided
application_typeis never overridden. Happy to drop the inference entirely and make this docs-only (schema field + JSDoc) if you'd prefer registration bodies stay byte-for-byte what the provider supplies.Validation
pnpm build:all,pnpm typecheck:all,pnpm lint:all— clean@modelcontextprotocol/clienttests: 379/379 pass (11 files)@modelcontextprotocol/coretests: 463/463 pass (22 files)pnpm run test:conformance:client:all): the 13 scenarios that failed only on the SEP-837application_typeDCR check now pass, so they are removed fromtest/conformance/expected-failures.yaml(the runner fails on stale entries).auth/scope-step-upstays in the baseline for an unrelated SEP-2350 scope-union WARNING.Downstream impact: cloudflare/agents'
DurableObjectOAuthClientProvideruses remote https redirect URLs on Workers → inferred'web', which is correct; no changes needed downstream.Closes #2198