feat(server): add request-time OAuth scope challenges - #1624
feat(server): add request-time OAuth scope challenges#1624SamMorrowDrums wants to merge 4 commits into
Conversation
🦋 Changeset detectedLatest commit: c51e262 The changes in this PR will be included in the next version bump. This PR includes changesets to release 10 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/core
@modelcontextprotocol/server
@modelcontextprotocol/server-legacy
@modelcontextprotocol/express
@modelcontextprotocol/fastify
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
This comment was marked as abuse.
This comment was marked as abuse.
|
Cross-checking this against SEP-2350 which landed after this PR was opened - a few things shifted:
|
012707a to
e52a2cc
Compare
Apply review feedback from @localden on PR modelcontextprotocol#1624: - Flip the WWW-Authenticate `scope` value to advertise only the per-operation `required` scopes by default, per RFC 6750 Section 3.1 and SEP-2350. Add an opt-in `scopeChallenge.includeGrantedScopes` flag that restores the additive union behaviour for servers that need to defend against non-accumulating clients. - Change `ToolScopeConfig.required` to AND semantics (every scope must be present in the token). `accepted` is now the explicit OR/hierarchy escape hatch. - Escape `"` and `\` in all WWW-Authenticate quoted-string auth-param values per RFC 7235. - Replace the duck-typed transport check in `McpServer.connect` with a typed `ScopeAware` interface and `isScopeAware` guard. Export `ScopeAware`, `ScopeResolver`, `ScopeChallengeConfig`, `ToolScopeConfig`, and `isScopeAware` from `@modelcontextprotocol/server`. - Tests rewritten to focus on public-surface behaviour. 17 tests covering 403 emission, AND-required, OR-accepted, the `includeGrantedScopes` opt-in, header quoting, batch handling, setToolScopes override, custom error description, and auto-wiring. - Proposal doc updated to reflect SEP-2350 alignment and call out resources/prompts/completions step-up as follow-up work. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Thanks for the review @localden, all four points addressed and rebased on latest 1. Per-operation 2.
I will note that there are still some situations that come unstuck here where you have two required scopes, and one of them has a hierarchy. It is possible you could end up not issuing a challenge when needed in that OR case (because you don't know which scope(s) contain the other). I think perhaps there always needs to be an option for custom I deliberately kept 3. Quoted-string escaping. Added a 4. Resources / prompts. Noted in the proposal doc as follow-up. I can add a stacked PR for this (also covering completions, since those can require scopes too). One open question: re-auth I researched how every SDK handles the related 401 case (full re-auth, no refresh token) while a client has accumulated scopes from prior step-ups. The picture is:
SEP-2350 and the draft spec are silent on this. The Python PR #2676 authored by @dogacancolak pins the "drop on 401" invariant explicitly as the natural down-scoping moment. I think the spec should make this normative either way to stop the next round of cross-SDK drift. Does that need a PR or SEP clarification itself? Marking this PR ready for review on the back of the rebase and the fixes above. |
Apply review feedback from @localden on PR modelcontextprotocol#1624: - Flip the WWW-Authenticate `scope` value to advertise only the per-operation `required` scopes by default, per RFC 6750 Section 3.1 and SEP-2350. Add an opt-in `scopeChallenge.includeGrantedScopes` flag that restores the additive union behaviour for servers that need to defend against non-accumulating clients. - Change `ToolScopeConfig.required` to AND semantics (every scope must be present in the token). `accepted` is now the explicit OR/hierarchy escape hatch. - Escape `"` and `\` in all WWW-Authenticate quoted-string auth-param values per RFC 7235. - Replace the duck-typed transport check in `McpServer.connect` with a typed `ScopeAware` interface and `isScopeAware` guard. Export `ScopeAware`, `ScopeResolver`, `ScopeChallengeConfig`, `ToolScopeConfig`, and `isScopeAware` from `@modelcontextprotocol/server`. - Tests rewritten to focus on public-surface behaviour. 17 tests covering 403 emission, AND-required, OR-accepted, the `includeGrantedScopes` opt-in, header quoting, batch handling, setToolScopes override, custom error description, and auto-wiring. - Proposal doc updated to reflect SEP-2350 alignment and call out resources/prompts/completions step-up as follow-up work. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
e52a2cc to
5f20c37
Compare
8f86350 to
c898995
Compare
c898995 to
aa2a3fe
Compare
aa2a3fe to
f724f40
Compare
|
@maxisbey @felixweinberger how are we feeling about this latest design? I shipped effectively this to GitHub MCP server in production already, in Go to fix the final challenges of argument based OR scope challenges, and more complex ones like our workflow scope which can only be determined based on file paths. |
|
Hey @SamMorrowDrums, I ran the SEP-2350 conformance scenario against this branch. Good news - rewrite was mostly hassle free. I had shared runs against Keycloak and Okta before, for both TS and an experimental Go SDK. Same 9 checks and all four combos still pass. The scenarios I added check the wire and not hte dev API. So I found replacing the entire registration surface produced challenges that are identical byte by byte. Also watned to call out that the OR-hierarchy check also passed too. Now it is opt-in and behavioural (calling with a parent-scope token, gives 2xx), so expressing the hierarchy in a callback also ensure it exactly as a declarative accepted field did. The SUT is also provider-neutral - one binary pointed at an issuer, discovering endpoints from .well-known/openid-configuration and reading scopes from scp, scope or scopes. Reason I had to do that was becuase Keycloak emits scope as a string and Okta emits scp as an array, and a verifier handling only one sees an empty scope set and challenges every call. This was another example of provider speicfic things to watch out for. https://github.com/panyam/mcp-ts-sdk/tree/sut/verified-20260829/examples/scope-challenge Few things I wanted to call out:
Happy to add fixtures for more providers. Descope, Entra and WorkOS are the open columns and thankfully several folks have offered to help. |
Let tools return exact OAuth scope challenges from request-aware callbacks before invocation or SSE setup. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Route request-time OAuth challenges for tools, static and templated resources, and prompts across both modern and legacy HTTP serving. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
77846c2 to
f0f73ee
Compare
Expose portable low- and full-scope tokens over the existing tool, resource, template, and prompt fixtures for the official SEP-2350 scenario. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Reject scope and error description values that RFC 6749/6750 cannot represent in a Bearer challenge. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
pcarleton
left a comment
There was a problem hiding this comment.
Overall, I'm very onboard with this change. I left a comment on one piece I want to think through more carefully before approving. Open to ideas if you've already considered alternatives there.
| const node = toNodeHandler( | ||
| createMcpHandler(buildServer, { | ||
| scopeChallenge: { | ||
| resourceMetadataUrl: getOAuthProtectedResourceMetadataUrl(mcpServerUrl) |
There was a problem hiding this comment.
needing to specify this here rather than being able to get it from the requireBearerAuth seems quite unfortunate... I need to dig in more to see if there's a way we can avoid that or if this is the best tradeoff available.
There was a problem hiding this comment.
@SamMorrowDrums okay I've sketched this out here: c51e262...claude/pr1624-scope-challenge-authinfo
lmkwyt
There was a problem hiding this comment.
To be fair I also hated that part, will also check it out.
Summary
scopeChallengecallback for tools, static resources, resource templates, and prompts.AuthInfoso required scopes can depend on operation arguments or resource URIs.requireScopes(...scopes)for exact static all-of checks.403 insufficient_scoperesponses before handler invocation or SSE setup.createMcpHandlerrequests and legacy Streamable HTTP requests/batches.API
Register the callback as
scopeChallengeonregisterTool, eitherregisterResourceform, orregisterPrompt. Returningundefinedcontinues; returning a challenge sends its exact, complete scope set. Throwing, rejecting, or returning an invalid scope set fails closed. Scope values must satisfy OAuth'sscope-tokengrammar; an optional description must satisfy RFC 6750'serror-descriptiongrammar.The SDK does not infer scope hierarchies, alternatives, unions, or missing scopes. Dynamic callbacks receive JSON-parsed wire values before primitive input schema validation/transformation.
Cross-language conformance
The independent official scenario is in modelcontextprotocol/conformance#481, tracked by conformance#480.
mainatdcc01028:SUCCESS=5,WARNING=12, exit 1. All four upgraded-token retries succeeded; each primitive failed the expected HTTP 403,WWW-Authenticate, and single-challenge checks.c51e262f:SUCCESS=17, no failures or warnings, exit 0.The official scenario is language-neutral and covers tools, static resources, resource templates, and prompts using portable fixture tokens and wire assertions.
Validation
pnpm test:allpnpm build:allpnpm typecheck:allpnpm --filter @modelcontextprotocol/server checkpnpm docs:examplespnpm docs:buildpnpm sync:snippets --check