ext/auth: AcceptedScopes OR + includeGrantedScopes opt-in on tool-scope middleware - #811
Merged
Merged
Conversation
…n tool-scope middleware (#742, #743) Two ergonomic improvements to NewToolScopeMiddleware, bundled because they touch the same signature and share test scaffolding. #742 - AcceptedScopes OR-hierarchy: - core.ToolDef.AcceptedScopes []string: opt-in OR escape hatch on top of RequiredScopes' AND semantics. Non-empty AcceptedScopes flips the gate to "any of these is held," supporting hierarchies like a `repo` scope satisfying a tool nominally requiring `repo:read`. - core.WithToolAcceptedScopes typed-tool option mirrors WithToolRequiredScopes. - Gate-only contract: AcceptedScopes participates in the satisfaction check but NEVER appears in the 403 WWW-Authenticate challenge. Re-auth guidance stays least-privilege - the challenge advertises RequiredScopes alone so clients don't escalate by requesting tolerated alternatives. - Two-state: nil and explicit empty slice both fall back to the AND-on- RequiredScopes default. Allocating []string{} cannot silently disable enforcement. #743 - WithIncludeGrantedScopes opt-in: - NewToolScopeMiddleware gains variadic ToolScopeOption args. Existing callsites compile unchanged (no opts). - WithIncludeGrantedScopes(true): the 403 challenge advertises the union of caller's currently-granted scopes and the tool's required scopes. Server-side defense against non-mcpkit clients that overwrite scopes on every challenge instead of accumulating. Mirrors the upstream TypeScript SDK PR modelcontextprotocol/typescript-sdk#1657 fix client-side. - Default off - matches SEP-2350 per-operation semantics and keeps the challenge minimal for mcpkit-on-mcpkit deployments where clients accumulate correctly. Shared helpers: - core.GetScopes(ctx) - sibling of core.HasScope. Returns the caller's granted scope set or nil for absent/empty claims (consistent with HasScope semantics). - auth.UnionScopes(a, b) - set-union helper preserving first-seen order. Lives in ext/auth because the only in-tree caller is the middleware's challenge-construction path; not speculative core surface. Test plan: - 3 new core/scopes_test.go cases for GetScopes (absent/present/empty). - 5 new ext/auth/scopes_test.go cases for UnionScopes (nil-nil, disjoint, overlap dedup, nil-first allocates fresh, nil-second allocates fresh). - 6 new ext/auth/scope_middleware_test.go cases: - AcceptedScopesHierarchyPasses - AcceptedScopesEmptyFallsBackToAND - AcceptedScopesDeniedAdvertisesRequiredOnly (gate-only contract) - IncludeGrantedScopesOffByDefault (back-compat) - IncludeGrantedScopesUnionsInChallenge - IncludeGrantedScopesEmptyGrantedSameAsOff - All 5 existing middleware tests pass unchanged (no assertion weakening). - make test + make test-auth + make tidy-all all green. Closes #742. Closes #743.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changes
Two ergonomic improvements to
ext/auth.NewToolScopeMiddleware, bundled because they touch the same signature and share test scaffolding. Closes issues 742 and 743.ToolDef.AcceptedScopes(issue 742) — opt-in OR escape hatch on top ofRequiredScopes' AND semantics, supporting hierarchies like areposcope satisfying a tool nominally requiringrepo:read. Gate-only: never appears in the 403 challenge, so re-auth guidance stays least-privilege.WithIncludeGrantedScopes(bool)(issue 743) — server-side defense against non-mcpkit clients that overwrite scopes on every challenge. When true, the 403 advertises the union of (caller's granted scopes ∪ required scopes). Default off.Reviewer's guide
Read in this order:
AcceptedScopesfield onToolDef. Doc-block spells the gate-only contract and two-state nil-vs-empty semantics — both deliberate, both tested.NewToolScopeMiddlewarebecomes variadic-options;scopeGateSatisfiedfactors out the AND-vs-OR branching; the challenge-construction path branches oncfg.includeGrantedScopes.core.GetScopes(ctx)sibling tocore.HasScope. Three tests.auth.UnionScopes(a, b)helper. Lives in ext/auth (not core) because the only in-tree caller is the middleware's challenge path. Five tests.WithToolAcceptedScopesmirror ofWithToolRequiredScopes.Skim: the doc-comment additions on existing surface (no behavior change).
Decision log
Two helpers split across core and ext/auth rather than both in core.
GetScopesis a sibling ofHasScope(already in core) — handlers shouldn't need to import ext/auth just to inspect their own claims.UnionScopeshad no cross-cutting caller in mcpkit — it lives where it's actually used. Heads off the awkward "mcpkit/core duplicates oneauth/core.UnionScopes" footnote.AcceptedScopes is gate-only — it never appears in the 403 challenge. Re-auth guidance stays least-privilege so clients don't escalate by requesting tolerated alternatives. Aligns with the upstream TypeScript SDK PR feat(server): add request-time OAuth scope challenges modelcontextprotocol/typescript-sdk#1624 (same choice cross-SDK). Flipping this post-merge would be a wire-format change.
AcceptedScopes is two-state — nil and explicit
[]string{}both fall back to AND-on-RequiredScopes. Prevents the footgun where allocating an empty slice silently disables enforcement. Explicit test covers this.Variadic options on
NewToolScopeMiddlewarerather than a struct argument. Matches mcpkit precedent (server.WithMiddleware, everycore.WithTool*). All five existing callsite tests compile unchanged.RequireAnyScopehandler helper NOT added despite ext/auth: AcceptedScopes OR-hierarchy on ToolDef for tool-scope middleware #742's "optionally" mention. The static OR case is handled byAcceptedScopesdeclaratively (with correct WWW-Authenticate semantics for free). Dynamic OR in a handler body is twocore.HasScopecalls in three lines. File a follow-up if a real use surfaces.Risk / blast radius
auth.NewToolScopeMiddleware. Verified zero in-tree call sites outside the test file viagrep -rn NewToolScopeMiddleware.AcceptedScopes(Go footgun zone).AcceptedScopesvs union-in-challenge invariant ofIncludeGrantedScopes— these are orthogonal and tested together.Before / after — 403 challenge shape
RequiredScopes=["docs:write"]["docs:read"]scope="docs:write"(unchanged)+ AcceptedScopes=["docs:write","docs"]["unrelated"]scope="docs:write"(accepted set hidden)+ WithIncludeGrantedScopes(true)["docs:read"]scope="docs:read docs:write"Out of scope (deliberately deferred)
IntrospectionConfig.RequiredScopes/JWTValidator.RequiredScopesglobal gates — stay AND, per issue 742.WWWAuth401granted-scope union — unauthenticated path has no granted scopes to union with, per issue 743.RequireAnyScopehandler helper — file follow-up if a real dynamic-OR-in-handler case surfaces.