Skip to content

Token scopes context #1997

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
omgitsads wants to merge 4 commits into
base: main
Choose a base branch
from
+62 −27
Open

Token scopes context #1997

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
beb728a
Move scope storage into its own context key, separately from token info.
omgitsads Feb 10, 2026
2022dee
Skip token extraction if token info already exists in context.
omgitsads Feb 10, 2026
f1d372e
Check for existing scopes in context before fetching from GitHub API …
omgitsads Feb 10, 2026
48eb4cb
Merge branch 'main' into token-scopes-context
omgitsads Feb 11, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 19 additions & 4 deletions pkg/context/token.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,8 @@ type tokenCtx string
var tokenCtxKey tokenCtx = "tokenctx"

type TokenInfo struct {
Token string
TokenType utils.TokenType
ScopesFetched bool
Scopes []string
Token string
TokenType utils.TokenType
Copy link

Copilot AI Feb 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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

Copilot uses AI. Check for mistakes.
}

// WithTokenInfo adds TokenInfo to the context
Expand All @@ -30,3 +28,20 @@ func GetTokenInfo(ctx context.Context) (*TokenInfo, bool) {
}
return nil, false
}

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)
}
Comment on lines +32 to +39
Copy link

Copilot AI Feb 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.

// GetTokenScopes retrieves token scopes from the context
func GetTokenScopes(ctx context.Context) ([]string, bool) {
if scopes, ok := ctx.Value(tokenScopesKey).([]string); ok {
return scopes, true
}
return nil, false
}
6 changes: 4 additions & 2 deletions pkg/http/handler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -278,8 +278,10 @@ func PATScopeFilter(b *inventory.Builder, r *http.Request, fetcher scopes.Fetche
// 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 {
return b.WithFilter(github.CreateToolScopeFilter(tokenInfo.Scopes))
// Check if scopes are already in context (should be set by WithPATScopes). If not, fetch them.
existingScopes, ok := ghcontext.GetTokenScopes(ctx)
if ok {
return b.WithFilter(github.CreateToolScopeFilter(existingScopes))
}

scopesList, err := fetcher.FetchTokenScopes(ctx, tokenInfo.Token)
Expand Down
12 changes: 8 additions & 4 deletions pkg/http/middleware/pat_scope.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,18 +26,22 @@ func WithPATScopes(logger *slog.Logger, scopeFetcher scopes.FetcherInterface) fu
// 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 {
existingScopes, ok := ghcontext.GetTokenScopes(ctx)
if ok {
logger.Debug("using existing scopes from context", "scopes", existingScopes)
Copy link

Copilot AI Feb 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
logger.Debug("using existing scopes from context", "scopes", existingScopes)
logger.Debug("using existing scopes from context", "scope_count", len(existingScopes))

Copilot uses AI. Check for mistakes.
next.ServeHTTP(w, r)
return
}

scopesList, err := scopeFetcher.FetchTokenScopes(ctx, tokenInfo.Token)
if err != nil {
logger.Warn("failed to fetch PAT scopes", "error", err)
next.ServeHTTP(w, r)
return
}

tokenInfo.Scopes = scopesList
tokenInfo.ScopesFetched = true

// Store fetched scopes in context for downstream use
ctx := ghcontext.WithTokenInfo(ctx, tokenInfo)
ctx = ghcontext.WithTokenScopes(ctx, scopesList)

next.ServeHTTP(w, r.WithContext(ctx))
return
Expand Down
19 changes: 11 additions & 8 deletions pkg/http/middleware/pat_scope_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,12 +111,13 @@ func TestWithPATScopes(t *testing.T) {

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
var capturedTokenInfo *ghcontext.TokenInfo
var capturedScopes []string
var scopesFound bool
var nextHandlerCalled bool

nextHandler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
nextHandlerCalled = true
capturedTokenInfo, _ = ghcontext.GetTokenInfo(r.Context())
capturedScopes, scopesFound = ghcontext.GetTokenScopes(r.Context())
w.WriteHeader(http.StatusOK)
})

Expand All @@ -141,10 +142,9 @@ func TestWithPATScopes(t *testing.T) {

assert.Equal(t, tt.expectNextHandlerCalled, nextHandlerCalled, "next handler called mismatch")

if tt.expectNextHandlerCalled && tt.tokenInfo != nil {
require.NotNil(t, capturedTokenInfo, "expected token info in context")
assert.Equal(t, tt.expectScopesFetched, capturedTokenInfo.ScopesFetched)
assert.Equal(t, tt.expectedScopes, capturedTokenInfo.Scopes)
if tt.expectNextHandlerCalled {
assert.Equal(t, tt.expectScopesFetched, scopesFound, "scopes found mismatch")
assert.Equal(t, tt.expectedScopes, capturedScopes)
}
})
}
Expand All @@ -154,9 +154,12 @@ func TestWithPATScopes_PreservesExistingTokenInfo(t *testing.T) {
logger := slog.Default()

var capturedTokenInfo *ghcontext.TokenInfo
var capturedScopes []string
var scopesFound bool

nextHandler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
capturedTokenInfo, _ = ghcontext.GetTokenInfo(r.Context())
capturedScopes, scopesFound = ghcontext.GetTokenScopes(r.Context())
w.WriteHeader(http.StatusOK)
})

Expand All @@ -182,6 +185,6 @@ func TestWithPATScopes_PreservesExistingTokenInfo(t *testing.T) {
require.NotNil(t, capturedTokenInfo)
assert.Equal(t, originalTokenInfo.Token, capturedTokenInfo.Token)
assert.Equal(t, originalTokenInfo.TokenType, capturedTokenInfo.TokenType)
assert.True(t, capturedTokenInfo.ScopesFetched)
assert.Equal(t, []string{"repo", "user"}, capturedTokenInfo.Scopes)
assert.True(t, scopesFound)
assert.Equal(t, []string{"repo", "user"}, capturedScopes)
}
18 changes: 10 additions & 8 deletions pkg/http/middleware/scope_challenge.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,17 +94,19 @@ func WithScopeChallenge(oauthCfg *oauth.Config, scopeFetcher scopes.FetcherInter
return
}

// Get OAuth scopes from GitHub API
activeScopes, err := scopeFetcher.FetchTokenScopes(ctx, tokenInfo.Token)
if err != nil {
next.ServeHTTP(w, r)
return
// 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)
Comment on lines +97 to 110
Copy link

Copilot AI Feb 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot generated this review using guidance from repository custom instructions.

// Check if user has the required scopes
Expand Down
11 changes: 10 additions & 1 deletion pkg/http/middleware/token.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,16 @@ import (
func ExtractUserToken(oauthCfg *oauth.Config) func(next http.Handler) http.Handler {
return func(next http.Handler) http.Handler {
return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
ctx := r.Context()

// 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
}
Comment on lines +18 to +24
Copy link

Copilot AI Feb 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot generated this review using guidance from repository custom instructions.

tokenType, token, err := utils.ParseAuthorizationHeader(r)
if err != nil {
// For missing Authorization header, return 401 with WWW-Authenticate header per MCP spec
Expand All @@ -25,7 +35,6 @@ func ExtractUserToken(oauthCfg *oauth.Config) func(next http.Handler) http.Handl
return
}

ctx := r.Context()
ctx = ghcontext.WithTokenInfo(ctx, &ghcontext.TokenInfo{
Token: token,
TokenType: tokenType,
Expand Down