Token scopes context #1997
Token scopes context #1997
Conversation
This allows us to provide scopes seperately in the remote server, where we have scopes before we do the auth.
This is to avoid redundant token extraction in remote setup where token info may have already been extracted earlier in the request lifecycle.
…in scope challenge middleware
There was a problem hiding this comment.
Pull request overview
This PR separates OAuth/PAT scope storage from TokenInfo by introducing a dedicated “token scopes” context value, enabling upstream/remote deployments to pre-populate scopes and avoid redundant GitHub API calls.
Changes:
- Add
WithTokenScopes/GetTokenScopescontext helpers and remove scope fields fromTokenInfo. - Update HTTP middleware and inventory filtering to read/write scopes via the new context value.
- Update PAT scope middleware tests to assert scopes are stored in context.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/http/middleware/token.go | Skip token extraction when TokenInfo is already present in request context. |
| pkg/http/middleware/scope_challenge.go | Prefer scopes from context; fetch from GitHub only when absent; store scopes back into context. |
| pkg/http/middleware/pat_scope.go | Prefer scopes from context; otherwise fetch and store into context. |
| pkg/http/middleware/pat_scope_test.go | Update tests to validate scopes are stored/retrieved via the new context helpers. |
| pkg/http/handler.go | Update PAT tool filtering to read scopes from context (or fetch if missing). |
| pkg/context/token.go | Remove scopes from TokenInfo; add dedicated context storage for token scopes. |
| // Get OAuth scopes for Token. First check if scopes are already in context, then fetch from GitHub if not present. | ||
| // This allows Remote Server to pass scope info to avoid redundant GitHub API calls. | ||
| activeScopes, ok := ghcontext.GetTokenScopes(ctx) | ||
| if !ok { | ||
| activeScopes, err = scopeFetcher.FetchTokenScopes(ctx, tokenInfo.Token) | ||
| if err != nil { | ||
| next.ServeHTTP(w, r) | ||
| return | ||
| } | ||
| } | ||
|
|
||
| // Store active scopes in context for downstream use | ||
| tokenInfo.Scopes = activeScopes | ||
| tokenInfo.ScopesFetched = true | ||
| ctx = ghcontext.WithTokenInfo(ctx, tokenInfo) | ||
| ctx = ghcontext.WithTokenScopes(ctx, activeScopes) | ||
| r = r.WithContext(ctx) |
There was a problem hiding this comment.
New behavior reads scopes from context to avoid redundant GitHub API calls, but there’s no unit test coverage for (1) scopes already present in context skipping FetchTokenScopes, and (2) scopes being stored back into context for downstream use. Adding a table-driven test with a mock fetcher would prevent regressions here.
| // Check if token info already exists in context, if it does, skip extraction. | ||
| // In remote setup, we may have already extracted token info earlier. | ||
| if _, ok := ghcontext.GetTokenInfo(ctx); ok { | ||
| // Token info already exists in context, skip extraction | ||
| next.ServeHTTP(w, r) | ||
| return | ||
| } |
There was a problem hiding this comment.
This middleware now short-circuits when TokenInfo is already present in context, but token_test.go doesn’t cover the new branch. Add a test that pre-populates TokenInfo in the request context and asserts the middleware does not overwrite it and does not require an Authorization header.
| ScopesFetched bool | ||
| Scopes []string | ||
| Token string | ||
| TokenType utils.TokenType |
There was a problem hiding this comment.
TokenInfo is an exported struct in a public package (pkg/context). Removing the Scopes/ScopesFetched fields is a breaking API change for any external consumers importing this module. Consider keeping the fields (possibly deprecated) or introducing a new context value/type for scopes while maintaining backward compatibility.
| TokenType utils.TokenType | |
| TokenType utils.TokenType | |
| // Deprecated: Scopes are now stored separately in the context using WithTokenScopes/GetTokenScopes. | |
| Scopes []string | |
| // Deprecated: ScopesFetched is no longer used; scopes are managed via TokenScopesKey in the context. | |
| ScopesFetched bool |
| type TokenScopesKey tokenCtx | ||
|
|
||
| var tokenScopesKey TokenScopesKey = "tokenscopesctx" | ||
|
|
||
| // WithTokenScopes adds token scopes to the context | ||
| func WithTokenScopes(ctx context.Context, scopes []string) context.Context { | ||
| return context.WithValue(ctx, tokenScopesKey, scopes) | ||
| } |
There was a problem hiding this comment.
The new token-scopes context key uses an exported key type (TokenScopesKey) with a string underlying value. This allows other packages to create the same key type/value and accidentally collide, and it’s inconsistent with the repo’s common struct{}-typed unexported context keys (e.g. readonlyCtxKey{} in pkg/context/request.go). Prefer an unexported struct{} key type for scopes as well.
| if tokenInfo.TokenType == utils.TokenTypePersonalAccessToken { | ||
| existingScopes, ok := ghcontext.GetTokenScopes(ctx) | ||
| if ok { | ||
| logger.Debug("using existing scopes from context", "scopes", existingScopes) |
There was a problem hiding this comment.
This logs the full scope list. Token scopes can reveal granted permissions and may be sensitive in some environments even at debug level. Consider logging only the count, or gating the detailed list behind an explicit opt-in / trace-level log.
| logger.Debug("using existing scopes from context", "scopes", existingScopes) | |
| logger.Debug("using existing scopes from context", "scope_count", len(existingScopes)) |
1 participant
Summary
So that I can setup the scopes of a token before it is authed, separate the TokenScopes from the TokenInfo contexts. In the remote MCP server, by the time
ExtractUserTokenhappens, we already know thatWhy
Gives us greater flexibility for handling authentication in a gateway and pre-populating token scopes.
What changed
MCP impact
Prompts tested (tool changes only)
Security / limits
Tool renaming
deprecated_tool_aliases.goNote: if you're renaming tools, you must add the tool aliases. For more information on how to do so, please refer to the official docs.
Lint & tests
./script/lint./script/testDocs