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.
…t in HTTP handler
| // Only classic PATs (ghp_ prefix) return OAuth scopes via X-OAuth-Scopes header. | ||
| // Fine-grained PATs and other token types don't support this, so we skip filtering. | ||
| if tokenInfo.TokenType == utils.TokenTypePersonalAccessToken { | ||
| if tokenInfo.ScopesFetched { |
There was a problem hiding this comment.
just curious, was there a reason we needed this flag at all?
There was a problem hiding this comment.
I was trying to handle the scenario where we had a token info from copilot API, but it was a token with no scopes. But having it combined with the TokenInfo meant the ordering was a little awkward.
There was a problem hiding this comment.
It's also implied now, if ok is false from scopes, ok := ghcontext.GetTokenScopes(ctx)
2 participants
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