Conversation
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…hub/github-mcp-server into oauth-handler-implementation
There was a problem hiding this comment.
Pull request overview
This PR implements OAuth scope challenge functionality for the HTTP server, adding token type detection, scope validation, and tool filtering based on token scopes. The implementation includes new middleware for parsing MCP requests and validating OAuth scopes, along with supporting infrastructure for token type identification and scope management.
Changes:
- Adds OAuth scope challenge middleware that validates token scopes and returns WWW-Authenticate challenges for insufficient permissions
- Implements token type detection for PATs, fine-grained PATs, OAuth tokens, GitHub App tokens, and IDE tokens
- Adds MCP request parsing middleware to extract method and tool information early in the request lifecycle
- Introduces tool scope mapping infrastructure to track which tools require which OAuth scopes
- Adds CLI flag
--scope-challengeto enable the scope challenge feature
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/utils/token.go | New token parsing utility with type detection for various GitHub token formats |
| pkg/utils/api.go | Adds default API host resolver factory function |
| pkg/scopes/map.go | Tool scope mapping infrastructure for tracking tool scope requirements |
| pkg/scopes/map_test.go | Tests for tool scope mapping functionality |
| pkg/scopes/fetcher.go | Updates scope fetcher to use APIHostResolver interface instead of string |
| pkg/scopes/fetcher_test.go | Updates tests to use testAPIHostResolver |
| pkg/http/middleware/scope_challenge.go | New middleware for OAuth scope validation and challenge responses |
| pkg/http/middleware/mcp_parse.go | New middleware for early MCP JSON-RPC request parsing |
| pkg/http/middleware/token.go | Refactors token extraction to use centralized parsing utility |
| pkg/http/server.go | Adds ScopeChallenge config field and initializes scope fetcher |
| pkg/http/handler.go | Integrates scope challenge and fetcher into handler lifecycle |
| pkg/http/handler_test.go | Updates tests with scope fetcher mocks |
| pkg/context/token.go | Changes token context from string to TokenInfo struct with type information |
| pkg/context/mcp_info.go | New context type for storing parsed MCP method information |
| pkg/github/server.go | Uses MCP method info from context to optimize inventory filtering |
| pkg/github/dependencies.go | Updates to extract token from new TokenInfo structure |
| internal/ghmcp/server.go | Updates scope fetcher instantiation to use APIHostResolver |
| cmd/github-mcp-server/main.go | Adds --scope-challenge CLI flag |
Comments suppressed due to low confidence (3)
pkg/http/server.go:143
- Variable name typo: "severOptions" should be "serverOptions" to match the corrected variable name.
handler := NewHTTPMcpHandler(ctx, &cfg, deps, t, logger, apiHost, append(severOptions, WithFeatureChecker(featureChecker), WithOAuthConfig(oauthCfg))...)
pkg/http/server.go:139
- Variable name typo: "severOptions" should be "serverOptions" to match the corrected variable name.
severOptions = append(severOptions, WithScopeFetcher(scopeFetcher))
pkg/http/middleware/scope_challenge.go:178
- The new
scope_challenge.gomiddleware lacks test coverage. This middleware handles critical OAuth scope validation and authorization logic, including:
- Determining when to send scope challenges
- Fetching token scopes from GitHub API
- Checking if tools have required scopes
- Building WWW-Authenticate headers
Tests should verify the middleware behavior for different token types, scope combinations, error conditions, and ensure the fallback parsing logic works correctly.
package middleware
import (
"bytes"
"encoding/json"
"fmt"
"io"
"net/http"
"strings"
ghcontext "github.com/github/github-mcp-server/pkg/context"
"github.com/github/github-mcp-server/pkg/http/oauth"
"github.com/github/github-mcp-server/pkg/scopes"
"github.com/github/github-mcp-server/pkg/utils"
)
// WithScopeChallenge creates a new middleware that determines if an OAuth request contains sufficient scopes to
// complete the request and returns a scope challenge if not.
func WithScopeChallenge(oauthCfg *oauth.Config, scopeFetcher scopes.FetcherInterface) func(http.Handler) http.Handler {
return func(next http.Handler) http.Handler {
fn := func(w http.ResponseWriter, r *http.Request) {
ctx := r.Context()
// Skip health check endpoints
if r.URL.Path == "/_ping" {
next.ServeHTTP(w, r)
return
}
// Get user from context
tokenInfo, ok := ghcontext.GetTokenInfo(ctx)
if !ok {
next.ServeHTTP(w, r)
return
}
// Only check OAuth tokens - scope challenge allows OAuth apps to request additional scopes
if tokenInfo.TokenType != utils.TokenTypeOAuthAccessToken {
next.ServeHTTP(w, r)
return
}
// Try to use pre-parsed MCP method info first (performance optimization)
// This avoids re-parsing the JSON body if WithMCPParse middleware ran earlier
var toolName string
if methodInfo, ok := ghcontext.MCPMethod(ctx); ok && methodInfo != nil {
// Only check tools/call requests
if methodInfo.Method != "tools/call" {
next.ServeHTTP(w, r)
return
}
toolName = methodInfo.ItemName
} else {
// Fallback: parse the request body directly
body, err := io.ReadAll(r.Body)
if err != nil {
next.ServeHTTP(w, r)
return
}
r.Body = io.NopCloser(bytes.NewReader(body))
var mcpRequest struct {
JSONRPC string `json:"jsonrpc"`
Method string `json:"method"`
Params struct {
Name string `json:"name,omitempty"`
Arguments map[string]any `json:"arguments,omitempty"`
} `json:"params"`
}
err = json.Unmarshal(body, &mcpRequest)
if err != nil {
next.ServeHTTP(w, r)
return
}
// Only check tools/call requests
if mcpRequest.Method != "tools/call" {
next.ServeHTTP(w, r)
return
}
toolName = mcpRequest.Params.Name
}
toolScopeInfo, err := scopes.GetToolScopeInfo(toolName)
if err != nil {
next.ServeHTTP(w, r)
return
}
// If tool not found in scope map, allow the request
if toolScopeInfo == nil {
next.ServeHTTP(w, r)
return
}
// Get OAuth scopes from GitHub API
activeScopes, err := scopeFetcher.FetchTokenScopes(ctx, tokenInfo.Token)
if err != nil {
next.ServeHTTP(w, r)
return
}
// Store active scopes in context for downstream use
ghcontext.SetTokenScopes(ctx, activeScopes)
// Check if user has the required scopes
if toolScopeInfo.HasAcceptedScope(activeScopes...) {
next.ServeHTTP(w, r)
return
}
// User lacks required scopes - get the scopes they need
requiredScopes := toolScopeInfo.GetRequiredScopesSlice()
// Build the resource metadata URL using the shared utility
// GetEffectiveResourcePath returns the original path (e.g., /mcp or /mcp/x/all)
// which is used to construct the well-known OAuth protected resource URL
resourcePath := oauth.ResolveResourcePath(r, oauthCfg)
resourceMetadataURL := oauth.BuildResourceMetadataURL(r, oauthCfg, resourcePath)
// Build recommended scopes: existing scopes + required scopes
recommendedScopes := make([]string, 0, len(activeScopes)+len(requiredScopes))
recommendedScopes = append(recommendedScopes, activeScopes...)
recommendedScopes = append(recommendedScopes, requiredScopes...)
// Build the WWW-Authenticate header value
wwwAuthenticateHeader := fmt.Sprintf(`Bearer error="insufficient_scope", scope=%q, resource_metadata=%q, error_description=%q`,
strings.Join(recommendedScopes, " "),
resourceMetadataURL,
"Additional scopes required: "+strings.Join(requiredScopes, ", "),
)
// Send scope challenge response with the superset of existing and required scopes
w.Header().Set("WWW-Authenticate", wwwAuthenticateHeader)
http.Error(w, "Forbidden: insufficient scopes", http.StatusForbidden)
}
return http.HandlerFunc(fn)
}
}
| func WithScopeChallenge(oauthCfg *oauth.Config, scopeFetcher scopes.FetcherInterface) func(http.Handler) http.Handler { | ||
| return func(next http.Handler) http.Handler { | ||
| fn := func(w http.ResponseWriter, r *http.Request) { | ||
| ctx := r.Context() | ||
|
|
||
| // Skip health check endpoints | ||
| if r.URL.Path == "/_ping" { | ||
| next.ServeHTTP(w, r) | ||
| return | ||
| } | ||
|
|
||
| // Get user from context | ||
| tokenInfo, ok := ghcontext.GetTokenInfo(ctx) | ||
| if !ok { | ||
| next.ServeHTTP(w, r) | ||
| return | ||
| } | ||
|
|
||
| // Only check OAuth tokens - scope challenge allows OAuth apps to request additional scopes | ||
| if tokenInfo.TokenType != utils.TokenTypeOAuthAccessToken { | ||
| next.ServeHTTP(w, r) | ||
| return | ||
| } | ||
|
|
||
| // Try to use pre-parsed MCP method info first (performance optimization) | ||
| // This avoids re-parsing the JSON body if WithMCPParse middleware ran earlier | ||
| var toolName string | ||
| if methodInfo, ok := ghcontext.MCPMethod(ctx); ok && methodInfo != nil { | ||
| // Only check tools/call requests | ||
| if methodInfo.Method != "tools/call" { | ||
| next.ServeHTTP(w, r) | ||
| return | ||
| } | ||
| toolName = methodInfo.ItemName | ||
| } else { | ||
| // Fallback: parse the request body directly | ||
| body, err := io.ReadAll(r.Body) | ||
| if err != nil { |
There was a problem hiding this comment.
The exported function FetchScopesFromGitHubAPI is defined but never used anywhere in the codebase. It appears to be redundant with the scopes.FetcherInterface and scopes.Fetcher which are actually used in the middleware. Consider removing this unused function or documenting its intended use case if it's meant to be a public API.
| package utils //nolint:revive //TODO: figure out a better name for this package | ||
|
|
||
| import ( | ||
| "fmt" | ||
| "net/http" | ||
| "regexp" | ||
| "strings" | ||
|
|
||
| httpheaders "github.com/github/github-mcp-server/pkg/http/headers" | ||
| "github.com/github/github-mcp-server/pkg/http/mark" | ||
| ) | ||
|
|
||
| type TokenType int | ||
|
|
||
| const ( | ||
| TokenTypeUnknown TokenType = iota | ||
| TokenTypePersonalAccessToken | ||
| TokenTypeFineGrainedPersonalAccessToken | ||
| TokenTypeOAuthAccessToken | ||
| TokenTypeUserToServerGitHubAppToken | ||
| TokenTypeServerToServerGitHubAppToken | ||
| TokenTypeIDEToken | ||
| ) | ||
|
|
||
| var supportedThirdPartyTokenPrefixes = map[string]TokenType{ | ||
| "ghp_": TokenTypePersonalAccessToken, // Personal access token (classic) | ||
| "github_pat_": TokenTypeFineGrainedPersonalAccessToken, // Fine-grained personal access token | ||
| "gho_": TokenTypeOAuthAccessToken, // OAuth access token | ||
| "ghu_": TokenTypeUserToServerGitHubAppToken, // User access token for a GitHub App | ||
| "ghs_": TokenTypeServerToServerGitHubAppToken, // Installation access token for a GitHub App (a.k.a. server-to-server token) | ||
| } | ||
|
|
||
| var ( | ||
| ErrMissingAuthorizationHeader = fmt.Errorf("%w: missing required Authorization header", mark.ErrBadRequest) | ||
| ErrBadAuthorizationHeader = fmt.Errorf("%w: Authorization header is badly formatted", mark.ErrBadRequest) | ||
| ErrUnsupportedAuthorizationHeader = fmt.Errorf("%w: unsupported Authorization header", mark.ErrBadRequest) | ||
| ) | ||
|
|
||
| // oldPatternRegexp is the regular expression for the old pattern of the token. | ||
| // Until 2021, GitHub API tokens did not have an identifiable prefix. They | ||
| // were 40 characters long and only contained the characters a-f and 0-9. | ||
| var oldPatternRegexp = regexp.MustCompile(`\A[a-f0-9]{40}\z`) | ||
|
|
||
| // ParseAuthorizationHeader parses the Authorization header from the HTTP request | ||
| func ParseAuthorizationHeader(req *http.Request) (tokenType TokenType, token string, _ error) { | ||
| authHeader := req.Header.Get(httpheaders.AuthorizationHeader) | ||
| if authHeader == "" { | ||
| return 0, "", ErrMissingAuthorizationHeader | ||
| } | ||
|
|
||
| switch { | ||
| // decrypt dotcom token and set it as token | ||
| case strings.HasPrefix(authHeader, "GitHub-Bearer "): | ||
| return 0, "", ErrUnsupportedAuthorizationHeader | ||
| default: | ||
| // support both "Bearer" and "bearer" to conform to api.github.com | ||
| if len(authHeader) > 7 && strings.EqualFold(authHeader[:7], "Bearer ") { | ||
| token = authHeader[7:] | ||
| } else { | ||
| token = authHeader | ||
| } | ||
| } | ||
|
|
||
| // Do a naïve check for a colon in the token - currently, only the IDE token has a colon in it. | ||
| // ex: tid=1;exp=25145314523;chat=1:<hmac> | ||
| if strings.Contains(token, ":") { | ||
| return TokenTypeIDEToken, token, nil | ||
| } | ||
|
|
||
| for prefix, tokenType := range supportedThirdPartyTokenPrefixes { | ||
| if strings.HasPrefix(token, prefix) { | ||
| return tokenType, token, nil | ||
| } | ||
| } | ||
|
|
||
| matchesOldTokenPattern := oldPatternRegexp.MatchString(token) | ||
| if matchesOldTokenPattern { | ||
| return TokenTypePersonalAccessToken, token, nil | ||
| } | ||
|
|
||
| return 0, "", ErrBadAuthorizationHeader | ||
| } |
There was a problem hiding this comment.
The new token.go file with ParseAuthorizationHeader function and related token type detection logic lacks test coverage. Given that this is critical security-related functionality that parses authentication headers and determines token types, comprehensive tests should be added to verify:
- Correct parsing of different token formats
- Proper error handling for malformed headers
- Token type detection for all supported prefixes
- Edge cases like empty headers, missing Bearer prefix, etc.
SamMorrowDrums
left a comment
There was a problem hiding this comment.
I think this is looking good, didn't test it again, but I think it's a great and pretty substantial bit of work so I'm very happy to see it land!
| WithFeatureChecker(featureChecker) | ||
|
|
||
| b = InventoryFiltersForRequest(r, b) | ||
| b = PATScopeFilter(b, r, scopeFetcher) |
There was a problem hiding this comment.
Sanity check, this is still done in stdio server too right?
There was a problem hiding this comment.
Yeah, this runs slightly differently in STDIO, since you get the scopes at server startup: https://github.com/github/github-mcp-server/blob/scope-challenge-http/internal/ghmcp/server.go#L142-L144
e0db9b3 to
cb957d0
Compare
Summary
Adds Scope Challenge middleware for OAuth tokens, and Scope tool filtering for PATs
Why
Fixes #
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