Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
7 changes: 7 additions & 0 deletions docs/error-handling.md
Original file line number Diff line number Diff line change
Expand Up @@ -20,11 +20,18 @@ Used for REST API errors from the GitHub API:
```go
type GitHubAPIError struct {
Message string `json:"message"`
Code string `json:"code"`
Response *github.Response `json:"-"`
Err error `json:"-"`
}
```

For HTTP-auth related failures, `Code` is populated with a machine-readable classifier so callers and middleware can distinguish:

- `missing_token`: no bearer token was provided
- `invalid_token`: the token was malformed or GitHub rejected it with `401`
- `insufficient_scope`: the token was valid but lacked the required permission or scope

### GitHubGraphQLError

Used for GraphQL API errors from the GitHub API:
Expand Down
22 changes: 22 additions & 0 deletions pkg/errors/error.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"context"
"fmt"
"net/http"
"strings"

"github.com/github/github-mcp-server/pkg/utils"
"github.com/google/go-github/v82/github"
Expand All @@ -12,6 +13,7 @@ import (

type GitHubAPIError struct {
Message string `json:"message"`
Code string `json:"code,omitempty"`
Response *github.Response `json:"-"`
Err error `json:"-"`
}
Expand All @@ -20,6 +22,7 @@ type GitHubAPIError struct {
func newGitHubAPIError(message string, resp *github.Response, err error) *GitHubAPIError {
return &GitHubAPIError{
Message: message,
Code: classifyHTTPErrorCode(resp.Response, message),
Response: resp,
Err: err,
}
Expand Down Expand Up @@ -47,18 +50,37 @@ func (e *GitHubGraphQLError) Error() string {

type GitHubRawAPIError struct {
Message string `json:"message"`
Code string `json:"code,omitempty"`
Response *http.Response `json:"-"`
Err error `json:"-"`
}

func newGitHubRawAPIError(message string, resp *http.Response, err error) *GitHubRawAPIError {
return &GitHubRawAPIError{
Message: message,
Code: classifyHTTPErrorCode(resp, message),
Response: resp,
Err: err,
}
}

func classifyHTTPErrorCode(resp *http.Response, message string) string {
if resp == nil {
return ""
}

switch resp.StatusCode {
case http.StatusUnauthorized:
return "invalid_token"
case http.StatusForbidden:
if strings.Contains(strings.ToLower(message), "scope") || strings.Contains(strings.ToLower(message), "permission") {
return "insufficient_scope"
}
}

return ""
}

func (e *GitHubRawAPIError) Error() string {
return fmt.Errorf("%s: %w", e.Message, e.Err).Error()
}
Expand Down
20 changes: 20 additions & 0 deletions pkg/errors/error_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ func TestGitHubErrorContext(t *testing.T) {

apiError := apiErrors[0]
assert.Equal(t, "failed to fetch resource", apiError.Message)
assert.Empty(t, apiError.Code)
assert.Equal(t, resp, apiError.Response)
assert.Equal(t, originalErr, apiError.Err)
assert.Equal(t, "failed to fetch resource: resource not found", apiError.Error())
Expand Down Expand Up @@ -86,6 +87,7 @@ func TestGitHubErrorContext(t *testing.T) {

rawError := rawErrors[0]
assert.Equal(t, "failed to fetch raw content", rawError.Message)
assert.Empty(t, rawError.Code)
assert.Equal(t, resp, rawError.Response)
assert.Equal(t, originalErr, rawError.Err)
})
Expand Down Expand Up @@ -361,6 +363,24 @@ func TestGitHubErrorContext(t *testing.T) {
assert.NoError(t, err, "NewGitHubAPIErrorToCtx should handle nil context gracefully")
assert.Nil(t, updatedCtx, "Context should remain nil when passed as nil")
})

t.Run("API errors classify invalid token and insufficient scope codes from HTTP status", func(t *testing.T) {
ctx := ContextWithGitHubErrors(context.Background())

unauthorized := &github.Response{Response: &http.Response{StatusCode: http.StatusUnauthorized}}
forbidden := &github.Response{Response: &http.Response{StatusCode: http.StatusForbidden}}

_, err := NewGitHubAPIErrorToCtx(ctx, "token rejected", unauthorized, fmt.Errorf("unauthorized"))
require.NoError(t, err)
_, err = NewGitHubAPIErrorToCtx(ctx, "insufficient permissions", forbidden, fmt.Errorf("forbidden"))
require.NoError(t, err)

apiErrors, err := GetGitHubAPIErrors(ctx)
require.NoError(t, err)
require.Len(t, apiErrors, 2)
assert.Equal(t, "invalid_token", apiErrors[0].Code)
assert.Equal(t, "insufficient_scope", apiErrors[1].Code)
})
}

func TestGitHubErrorTypes(t *testing.T) {
Expand Down
23 changes: 23 additions & 0 deletions pkg/http/middleware/auth_error.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
package middleware

import (
"encoding/json"
"net/http"
)

type authErrorBody struct {
Error string `json:"error"`
Code string `json:"code"`
}

func writeAuthError(w http.ResponseWriter, status int, code string, message string, wwwAuthenticate string) {
if wwwAuthenticate != "" {
w.Header().Set("WWW-Authenticate", wwwAuthenticate)
}
w.Header().Set("Content-Type", "application/json")
w.WriteHeader(status)
_ = json.NewEncoder(w).Encode(authErrorBody{
Error: message,
Code: code,
})
}
9 changes: 7 additions & 2 deletions pkg/http/middleware/scope_challenge.go
Original file line number Diff line number Diff line change
Expand Up @@ -137,8 +137,13 @@ func WithScopeChallenge(oauthCfg *oauth.Config, scopeFetcher scopes.FetcherInter
)

// 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)
writeAuthError(
w,
http.StatusForbidden,
"insufficient_scope",
"Forbidden: insufficient scopes",
wwwAuthenticateHeader,
)
}
return http.HandlerFunc(fn)
}
Expand Down
68 changes: 68 additions & 0 deletions pkg/http/middleware/scope_challenge_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
package middleware

import (
"context"
"encoding/json"
"net/http"
"net/http/httptest"
"testing"

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"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)

type scopeChallengeFetcher struct {
scopes []string
err error
}

func (m *scopeChallengeFetcher) FetchTokenScopes(_ context.Context, _ string) ([]string, error) {
return m.scopes, m.err
}

func TestWithScopeChallenge_ReturnsMachineReadableInsufficientScopeCode(t *testing.T) {
scopes.SetGlobalToolScopeMap(scopes.ToolScopeMap{
"create_or_update_file": {
RequiredScopes: []string{"repo"},
AcceptedScopes: []string{"repo"},
},
})
t.Cleanup(func() {
scopes.SetGlobalToolScopeMap(nil)
})

nextHandler := http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
w.WriteHeader(http.StatusOK)
})

handler := WithScopeChallenge(
&oauth.Config{BaseURL: "https://example.com"},
&scopeChallengeFetcher{scopes: []string{}},
)(nextHandler)

req := httptest.NewRequest(http.MethodPost, "/mcp", nil)
req = req.WithContext(ghcontext.WithTokenInfo(req.Context(), &ghcontext.TokenInfo{
Token: "gho_test",
TokenType: utils.TokenTypeOAuthAccessToken,
}))
req = req.WithContext(ghcontext.WithMCPMethodInfo(req.Context(), &ghcontext.MCPMethodInfo{
Method: "tools/call",
ItemName: "create_or_update_file",
}))
rr := httptest.NewRecorder()

handler.ServeHTTP(rr, req)

assert.Equal(t, http.StatusForbidden, rr.Code)
assert.Contains(t, rr.Header().Get("WWW-Authenticate"), `error="insufficient_scope"`)

var body struct {
Code string `json:"code"`
}
require.NoError(t, json.Unmarshal(rr.Body.Bytes(), &body))
assert.Equal(t, "insufficient_scope", body.Code)
}
14 changes: 10 additions & 4 deletions pkg/http/middleware/token.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,9 @@ func ExtractUserToken(oauthCfg *oauth.Config) func(next http.Handler) http.Handl
sendAuthChallenge(w, r, oauthCfg)
return
}
// For other auth errors (bad format, unsupported), return 400
http.Error(w, err.Error(), http.StatusBadRequest)
// For other auth errors (bad format, unsupported), keep the existing 400
// but expose a machine-readable invalid_token classification.
writeAuthError(w, http.StatusBadRequest, "invalid_token", err.Error(), "")
return
}

Expand All @@ -51,6 +52,11 @@ func ExtractUserToken(oauthCfg *oauth.Config) func(next http.Handler) http.Handl
func sendAuthChallenge(w http.ResponseWriter, r *http.Request, oauthCfg *oauth.Config) {
resourcePath := oauth.ResolveResourcePath(r, oauthCfg)
resourceMetadataURL := oauth.BuildResourceMetadataURL(r, oauthCfg, resourcePath)
w.Header().Set("WWW-Authenticate", fmt.Sprintf(`Bearer resource_metadata=%q`, resourceMetadataURL))
http.Error(w, "Unauthorized", http.StatusUnauthorized)
writeAuthError(
w,
http.StatusUnauthorized,
"missing_token",
"Unauthorized",
fmt.Sprintf(`Bearer resource_metadata=%q`, resourceMetadataURL),
)
}
18 changes: 18 additions & 0 deletions pkg/http/middleware/token_test.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package middleware

import (
"encoding/json"
"net/http"
"net/http/httptest"
"testing"
Expand All @@ -23,6 +24,7 @@ func TestExtractUserToken(t *testing.T) {
name string
authHeader string
expectedStatusCode int
expectedCode string
expectedTokenType utils.TokenType
expectedToken string
expectTokenInfo bool
Expand All @@ -33,6 +35,7 @@ func TestExtractUserToken(t *testing.T) {
name: "missing Authorization header returns 401 with WWW-Authenticate",
authHeader: "",
expectedStatusCode: http.StatusUnauthorized,
expectedCode: "missing_token",
expectTokenInfo: false,
expectWWWAuth: true,
},
Expand Down Expand Up @@ -151,18 +154,21 @@ func TestExtractUserToken(t *testing.T) {
name: "unsupported GitHub-Bearer header returns 400",
authHeader: "GitHub-Bearer some_encrypted_token",
expectedStatusCode: http.StatusBadRequest,
expectedCode: "invalid_token",
expectTokenInfo: false,
},
{
name: "invalid token format returns 400",
authHeader: "Bearer invalid_token_format",
expectedStatusCode: http.StatusBadRequest,
expectedCode: "invalid_token",
expectTokenInfo: false,
},
{
name: "unrecognized prefix returns 400",
authHeader: "Bearer xyz_notavalidprefix",
expectedStatusCode: http.StatusBadRequest,
expectedCode: "invalid_token",
expectTokenInfo: false,
},
}
Expand All @@ -189,6 +195,13 @@ func TestExtractUserToken(t *testing.T) {
handler.ServeHTTP(rr, req)

assert.Equal(t, tt.expectedStatusCode, rr.Code)
if tt.expectedCode != "" {
var body struct {
Code string `json:"code"`
}
require.NoError(t, json.Unmarshal(rr.Body.Bytes(), &body))
assert.Equal(t, tt.expectedCode, body.Code)
}

if tt.expectWWWAuth {
wwwAuth := rr.Header().Get("WWW-Authenticate")
Expand Down Expand Up @@ -253,6 +266,11 @@ func TestExtractUserToken_MissingAuthHeader_WWWAuthenticateFormat(t *testing.T)
handler.ServeHTTP(rr, req)

assert.Equal(t, http.StatusUnauthorized, rr.Code)
var body struct {
Code string `json:"code"`
}
require.NoError(t, json.Unmarshal(rr.Body.Bytes(), &body))
assert.Equal(t, "missing_token", body.Code)
wwwAuth := rr.Header().Get("WWW-Authenticate")
assert.NotEmpty(t, wwwAuth)
assert.Contains(t, wwwAuth, "Bearer")
Expand Down