feat(client): SEP-837/2207 — application_type heuristic, grant_types default, https token-endpoint guard#2345
Conversation
🦋 Changeset detectedLatest commit: b445a38 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: |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| authServerMetadata?.scopes_supported?.includes('offline_access') && | ||
| !effectiveScope.split(' ').includes('offline_access') && | ||
| clientMetadata.grant_types?.includes('refresh_token') | ||
| (clientMetadata.grant_types ?? DEFAULT_DCR_GRANT_TYPES).includes('refresh_token') |
There was a problem hiding this comment.
it seems a bit strange to me to check it here, vs. filling in the defaults in clientMetadata
| authServerMetadata?.scopes_supported?.includes('offline_access') && | ||
| !effectiveScope.split(' ').includes('offline_access') && | ||
| clientMetadata.grant_types?.includes('refresh_token') | ||
| (clientMetadata.grant_types ?? DEFAULT_DCR_GRANT_TYPES).includes('refresh_token') |
There was a problem hiding this comment.
I think we should apply this when getting client metadata from the provider, and then pass it around from there. it feels strange to me to have a sneaky default check fairly removed from where we got the data
felixweinberger
force-pushed
the
fweinberger/auth-1-sep2468-client
branch
from
afb76ea to
c843d7e
Compare
felixweinberger
force-pushed
the
fweinberger/auth-2-sep837-2207
branch
from
99fbb49 to
5156ed7
Compare
felixweinberger
force-pushed
the
fweinberger/auth-1-sep2468-client
branch
from
c843d7e to
5dfc2be
Compare
felixweinberger
force-pushed
the
fweinberger/auth-2-sep837-2207
branch
from
5156ed7 to
409363a
Compare
felixweinberger
force-pushed
the
fweinberger/auth-2-sep837-2207
branch
from
409363a to
32d3af6
Compare
| OAuthServerInfo | ||
| } from './client/auth.js'; | ||
| export { | ||
| assertSecureTokenEndpoint, |
There was a problem hiding this comment.
🟡 New public export assertSecureTokenEndpoint (packages/client/src/index.ts) is the only new symbol in this PR not mentioned in the changeset or migration docs, and no in-PR consumer imports it from the package barrel (internal callers use ./auth.js). Either drop the barrel export to keep the helper internal, or add it to .changeset/auth-dcr-hygiene.md / the migration docs alongside the other new auth surface so the release notes match the shipped public API.
Extended reasoning...
What's inconsistent. packages/client/src/index.ts:21 adds assertSecureTokenEndpoint to the public barrel of @modelcontextprotocol/client. Every other new public symbol introduced by this PR — resolveClientMetadata, RegistrationRejectedError, InsecureTokenEndpointError, OAuthClientMetadata.application_type, OAuthErrorCode.InvalidRedirectUri — is explicitly named in .changeset/auth-dcr-hygiene.md, and the guard behavior is described in both docs/migration.md and docs/migration-SKILL.md. The exported helper itself, however, appears nowhere in the changeset or either migration doc.
No barrel consumer exists in the PR. A repo-wide search shows the only references are: the definition and internal use in packages/client/src/client/auth.ts (executeTokenRequest), the internal uses in packages/client/src/client/crossAppAccess.ts (requestJwtAuthorizationGrant / exchangeJwtAuthGrant) — both of which import it from ./auth.js, not the barrel — and the unit test, which imports from ../../src/client/auth.js directly. Nothing in the PR exercises or requires the index.ts export.
Why this matters under repo conventions. CLAUDE.md's Public API Exports section says adding a symbol to a package index.ts makes it public API and must be done intentionally, and the review checklist treats every new export as something that must be deliberate and documented. Once shipped in a minor release the export becomes surface that semver discourages removing, so the decision should be explicit now rather than discovered later.
Step-by-step view of the gap. (1) A consumer reads the v-next release notes generated from .changeset/auth-dcr-hygiene.md: they learn about InsecureTokenEndpointError and the guarded functions, but never that a reusable assertSecureTokenEndpoint(tokenEndpoint) helper is now exported. (2) Conversely, a consumer who discovers the export via autocomplete finds no changeset entry, no migration-doc mention, and no example showing intended use — so the symbol is shipped-but-unadvertised public API. (3) Meanwhile every internal caller resolves the helper through ./auth.js, so removing the barrel line would change nothing inside the SDK or its tests.
Impact. None at runtime — this is purely API-surface / release-notes hygiene. Exporting the guard is a defensible choice (consumers hand-rolling token requests could reuse it to get the same SEP-2207 behavior), which is why this is a nit rather than a bug.
How to fix. Pick one: (a) remove assertSecureTokenEndpoint from packages/client/src/index.ts and keep it internal — internal callers and tests are unaffected since they import from ./auth.js; or (b) keep the export and add one clause to .changeset/auth-dcr-hygiene.md (and optionally the migration docs' SEP-2207 section) noting the exported helper, so the release notes enumerate the full new auth surface.
felixweinberger
force-pushed
the
fweinberger/auth-1-sep2468-client
branch
from
5dfc2be to
e2cecde
Compare
felixweinberger
force-pushed
the
fweinberger/auth-2-sep837-2207
branch
from
32d3af6 to
b629c06
Compare
…default, https token-endpoint guard Claude-Session: https://claude.ai/code/session_01XBib5gRe8AMPPJhySCz3EJ
felixweinberger
force-pushed
the
fweinberger/auth-2-sep837-2207
branch
from
b629c06 to
b445a38
Compare
felixweinberger
force-pushed
the
fweinberger/auth-1-sep2468-client
branch
from
e2cecde to
7c38bad
Compare
2 participants
7ee1ad038registerClient()defaultsapplication_typefrom the redirect URI (loopback/localhost →'native', else'web'; consumer-set value always wins) and defaultsgrant_typesto['authorization_code', 'refresh_token']when omitted.auth()/exchangeAuthorization()/refreshAuthorization()reject a non-https:token endpoint (loopback exempt). New exportedRegistrationRejectedErrorcarrying the AS error and the submitted metadata.Motivation and Context
SEP-837 (modelcontextprotocol/modelcontextprotocol#837) and SEP-2207 (modelcontextprotocol/modelcontextprotocol#2207).
Normative sentences implemented:
How Has This Been Tested?
application_typeheuristic table (loopback v4/v6, localhost, custom-scheme, https web host, explicit override).client-auth:dcr:{application_type-heuristic, application_type-override, grant_types-default, registration-rejected-error},client-auth:https-token-endpoint-guard.auth/metadata-*,auth/scope-from-*,auth/token-endpoint-auth-*,auth/offline-access-not-supported) on the 2026 leg.Breaking Changes
Yes — DCR body and token-endpoint guard.
application_type(heuristic default) andgrant_types: ['authorization_code', 'refresh_token']when the consumer'sclientMetadataomits them. ASes that reject unknown fields or that constrain redirect URIs byapplication_typemay now reject registrations that previously succeeded. Consumers can override both fields explicitly;RegistrationRejectedErrorcarries the submitted metadata to support the spec's MAY-retry-with-adjusted-type.auth()and the low-level token helpers throw on anhttp:token endpoint (loopback hosts exempt). Hosts pointed at a plaintext token endpoint will fail closed; this enforces a spec MUST and there is no opt-out.Types of changes
Checklist
Additional context
The
application_typeheuristic is anOAuthClientMetadataSchemadefault, not a rewrite — a consumer-set value (includingundefinedset explicitly) is never overwritten. SEP-2207's RS-operatoroffline_accessSHOULD-NOTs and the in-storage confidentiality clause are documented as consumer obligations inmigration.md(PR 7); they are not SDK code.