diff --git a/docs/streamable-http.md b/docs/streamable-http.md index 97d970c36c..a1e6d9890a 100644 --- a/docs/streamable-http.md +++ b/docs/streamable-http.md @@ -80,6 +80,19 @@ The OAuth protected resource metadata's `resource` attribute will be populated w This allows OAuth clients to discover authentication requirements and endpoint information automatically. +The HTTP server is the OAuth protected resource, not the authorization server. It +therefore serves `/.well-known/oauth-protected-resource` but does not serve +`/.well-known/oauth-authorization-server` unless a separately deployed authorization +server is explicitly hosted on the same origin. + +Clients discover authorization-server metadata from the issuer listed in +`authorization_servers`. For the default `https://github.com/login/oauth` issuer, +RFC 8414 path insertion produces +`https://github.com/.well-known/oauth-authorization-server/login/oauth`. Browser-based +clients require that authorization server and its discovery endpoints to support +their browser origin through CORS. If the selected authorization server does not, +configure `--authorization-server` to advertise a browser-compatible OAuth proxy. + ### Behind a Trusted Proxy (advanced) By default, the server ignores the `X-Forwarded-Host` and `X-Forwarded-Proto` headers when constructing OAuth resource metadata URLs, so an untrusted client cannot influence the URL advertised to MCP clients. For most deployments, setting `--base-url` to the externally visible URL is the right approach. diff --git a/pkg/http/middleware/cors.go b/pkg/http/middleware/cors.go index 2eaf4227b4..409d134127 100644 --- a/pkg/http/middleware/cors.go +++ b/pkg/http/middleware/cors.go @@ -31,7 +31,7 @@ func SetCorsHeaders(h http.Handler) http.Handler { w.Header().Set("Access-Control-Allow-Origin", "*") w.Header().Set("Access-Control-Allow-Methods", "GET, POST, DELETE, OPTIONS") w.Header().Set("Access-Control-Max-Age", "86400") - w.Header().Set("Access-Control-Expose-Headers", "Mcp-Session-Id, WWW-Authenticate") + w.Header().Add("Access-Control-Expose-Headers", "Mcp-Session-Id, WWW-Authenticate") w.Header().Set("Access-Control-Allow-Headers", allowHeaders) if r.Method == http.MethodOptions { diff --git a/pkg/http/middleware/cors_test.go b/pkg/http/middleware/cors_test.go index fbd7c40cf9..67305e0ac8 100644 --- a/pkg/http/middleware/cors_test.go +++ b/pkg/http/middleware/cors_test.go @@ -3,6 +3,7 @@ package middleware_test import ( "net/http" "net/http/httptest" + "strings" "testing" "github.com/github/github-mcp-server/pkg/http/middleware" @@ -10,19 +11,27 @@ import ( ) func TestSetCorsHeaders(t *testing.T) { + innerCalled := false inner := http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + innerCalled = true + w.Header().Add("Access-Control-Expose-Headers", "X-Existing-Response") w.WriteHeader(http.StatusOK) }) handler := middleware.SetCorsHeaders(inner) t.Run("OPTIONS preflight returns 200 with CORS headers", func(t *testing.T) { + innerCalled = false req := httptest.NewRequest(http.MethodOptions, "/", nil) - req.Header.Set("Origin", "http://localhost:6274") + req.Header.Set("Origin", "https://confer.to") + req.Header.Set("Access-Control-Request-Method", http.MethodPost) + req.Header.Set("Access-Control-Request-Headers", "content-type") rr := httptest.NewRecorder() handler.ServeHTTP(rr, req) assert.Equal(t, http.StatusOK, rr.Code) + assert.False(t, innerCalled) assert.Equal(t, "*", rr.Header().Get("Access-Control-Allow-Origin")) + assert.Empty(t, rr.Header().Get("Access-Control-Allow-Credentials")) assert.Contains(t, rr.Header().Get("Access-Control-Allow-Methods"), "POST") assert.Contains(t, rr.Header().Get("Access-Control-Allow-Headers"), "Authorization") assert.Contains(t, rr.Header().Get("Access-Control-Allow-Headers"), "Content-Type") @@ -33,13 +42,20 @@ func TestSetCorsHeaders(t *testing.T) { assert.Contains(t, rr.Header().Get("Access-Control-Expose-Headers"), "WWW-Authenticate") }) - t.Run("POST request includes CORS headers", func(t *testing.T) { + t.Run("POST request includes CORS headers without replacing existing exposed headers", func(t *testing.T) { + innerCalled = false req := httptest.NewRequest(http.MethodPost, "/", nil) - req.Header.Set("Origin", "http://localhost:6274") + req.Header.Set("Origin", "https://confer.to") rr := httptest.NewRecorder() handler.ServeHTTP(rr, req) assert.Equal(t, http.StatusOK, rr.Code) + assert.True(t, innerCalled) assert.Equal(t, "*", rr.Header().Get("Access-Control-Allow-Origin")) + assert.Empty(t, rr.Header().Get("Access-Control-Allow-Credentials")) + exposedHeaders := strings.Join(rr.Header().Values("Access-Control-Expose-Headers"), ", ") + assert.Contains(t, exposedHeaders, "Mcp-Session-Id") + assert.Contains(t, exposedHeaders, "WWW-Authenticate") + assert.Contains(t, exposedHeaders, "X-Existing-Response") }) } diff --git a/pkg/http/oauth/oauth.go b/pkg/http/oauth/oauth.go index e6a53ba80c..77cfe8fa10 100644 --- a/pkg/http/oauth/oauth.go +++ b/pkg/http/oauth/oauth.go @@ -82,11 +82,14 @@ func NewAuthHandler(cfg *Config, apiHost utils.APIHostResolver) (*AuthHandler, e // routePatterns defines the route patterns for OAuth protected resource metadata. var routePatterns = []string{ - "", // Root: /.well-known/oauth-protected-resource - "/readonly", // Read-only mode - "/insiders", // Insiders mode + "", // Root: /.well-known/oauth-protected-resource + "/readonly", + "/insiders", + "/readonly/insiders", "/x/{toolset}", "/x/{toolset}/readonly", + "/x/{toolset}/insiders", + "/x/{toolset}/readonly/insiders", } // RegisterRoutes registers the OAuth protected resource metadata routes. @@ -97,6 +100,7 @@ func (h *AuthHandler) RegisterRoutes(r chi.Router) { r.Handle(path, h.metadataHandler()) } } + r.Handle(OAuthProtectedResourcePrefix+"/*", http.NotFoundHandler()) } func (h *AuthHandler) metadataHandler() http.Handler { diff --git a/pkg/http/oauth/oauth_test.go b/pkg/http/oauth/oauth_test.go index bb75ea5d4d..52baae3b6c 100644 --- a/pkg/http/oauth/oauth_test.go +++ b/pkg/http/oauth/oauth_test.go @@ -542,37 +542,42 @@ func TestRegisterRoutes(t *testing.T) { router := chi.NewRouter() handler.RegisterRoutes(router) - // List of expected routes that should be registered - expectedRoutes := []string{ - OAuthProtectedResourcePrefix, - OAuthProtectedResourcePrefix + "/", - OAuthProtectedResourcePrefix + "/mcp", - OAuthProtectedResourcePrefix + "/mcp/", - OAuthProtectedResourcePrefix + "/readonly", - OAuthProtectedResourcePrefix + "/readonly/", - OAuthProtectedResourcePrefix + "/mcp/readonly", - OAuthProtectedResourcePrefix + "/mcp/readonly/", - OAuthProtectedResourcePrefix + "/x/repos", - OAuthProtectedResourcePrefix + "/mcp/x/repos", + resourcePaths := []string{ + "", + "/readonly", + "/insiders", + "/readonly/insiders", + "/x/repos", + "/x/repos/readonly", + "/x/repos/insiders", + "/x/repos/readonly/insiders", } - for _, route := range expectedRoutes { - t.Run("route:"+route, func(t *testing.T) { - // Test GET - req := httptest.NewRequest(http.MethodGet, route, nil) - req.Host = "api.example.com" - rec := httptest.NewRecorder() - router.ServeHTTP(rec, req) - assert.Equal(t, http.StatusOK, rec.Code, "GET %s should return 200", route) - - // Test OPTIONS (CORS preflight) - req = httptest.NewRequest(http.MethodOptions, route, nil) - req.Host = "api.example.com" - rec = httptest.NewRecorder() - router.ServeHTTP(rec, req) - assert.Equal(t, http.StatusNoContent, rec.Code, "OPTIONS %s should return 204", route) - }) + for _, basePath := range []string{"", "/mcp"} { + for _, resourcePath := range resourcePaths { + for _, trailingSlash := range []string{"", "/"} { + route := OAuthProtectedResourcePrefix + basePath + resourcePath + trailingSlash + t.Run("route:"+route, func(t *testing.T) { + req := httptest.NewRequest(http.MethodGet, route, nil) + req.Host = "api.example.com" + rec := httptest.NewRecorder() + router.ServeHTTP(rec, req) + assert.Equal(t, http.StatusOK, rec.Code, "GET %s should return 200", route) + + req = httptest.NewRequest(http.MethodOptions, route, nil) + req.Host = "api.example.com" + rec = httptest.NewRecorder() + router.ServeHTTP(rec, req) + assert.Equal(t, http.StatusNoContent, rec.Code, "OPTIONS %s should return 204", route) + }) + } + } } + + req := httptest.NewRequest(http.MethodGet, OAuthProtectedResourcePrefix+"/mcp/unknown", nil) + rec := httptest.NewRecorder() + router.ServeHTTP(rec, req) + assert.Equal(t, http.StatusNotFound, rec.Code) } func TestSupportedScopes(t *testing.T) { diff --git a/pkg/http/server.go b/pkg/http/server.go index e82be64bb7..cc2d23d3ac 100644 --- a/pkg/http/server.go +++ b/pkg/http/server.go @@ -204,28 +204,22 @@ func RunHTTPServer(cfg ServerConfig) error { WithScopeFetcher(scopeFetcher), } - r := chi.NewRouter() handler := NewHTTPMcpHandler(ctx, &cfg, deps, t, logger, apiHost, append(serverOptions, WithFeatureChecker(featureChecker), WithOAuthConfig(oauthCfg))...) oauthHandler, err := oauth.NewAuthHandler(oauthCfg, apiHost) if err != nil { return fmt.Errorf("failed to create OAuth handler: %w", err) } - r.Group(func(r chi.Router) { - r.Use(middleware.SetCorsHeaders) - - // Register Middleware First, needs to be before route registration - handler.RegisterMiddleware(r) - - // Register MCP server routes - handler.RegisterRoutes(r) - }) + r := newHTTPRouter( + func(r chi.Router) { + // Register Middleware First, needs to be before route registration + handler.RegisterMiddleware(r) + // Register MCP server routes + handler.RegisterRoutes(r) + }, + oauthHandler.RegisterRoutes, + ) logger.Info("MCP endpoints registered", "baseURL", cfg.BaseURL) - - r.Group(func(r chi.Router) { - // Register OAuth protected resource metadata endpoints - oauthHandler.RegisterRoutes(r) - }) logger.Info("OAuth protected resource endpoints registered", "baseURL", cfg.BaseURL) addr := resolveListenAddress(cfg.ListenHost, cfg.Port) @@ -259,6 +253,14 @@ func RunHTTPServer(cfg ServerConfig) error { return nil } +func newHTTPRouter(registerMCPRoutes, registerOAuthRoutes func(chi.Router)) chi.Router { + r := chi.NewRouter() + r.Use(middleware.SetCorsHeaders) + r.Group(registerMCPRoutes) + r.Group(registerOAuthRoutes) + return r +} + func newOAuthConfig(cfg ServerConfig) *oauth.Config { return &oauth.Config{ BaseURL: cfg.BaseURL, diff --git a/pkg/http/server_test.go b/pkg/http/server_test.go index d32354091c..a8c4e1a90b 100644 --- a/pkg/http/server_test.go +++ b/pkg/http/server_test.go @@ -3,15 +3,21 @@ package http import ( "context" "encoding/base64" + "encoding/json" "io" "log/slog" + "net/http" + "net/http/httptest" + "strings" "testing" ghcontext "github.com/github/github-mcp-server/pkg/context" "github.com/github/github-mcp-server/pkg/github" + "github.com/github/github-mcp-server/pkg/http/middleware" "github.com/github/github-mcp-server/pkg/http/oauth" "github.com/github/github-mcp-server/pkg/inventory" "github.com/github/github-mcp-server/pkg/utils" + "github.com/go-chi/chi/v5" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -78,6 +84,194 @@ func TestNewOAuthConfig(t *testing.T) { } } +func TestHTTPRouterCORSContract(t *testing.T) { + router := newHTTPRouter( + func(r chi.Router) { + r.Use(middleware.ExtractUserToken(&oauth.Config{ + BaseURL: "https://mcp.example.com", + ResourcePath: "/mcp", + })) + r.Post("/", func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusNoContent) + }) + }, + func(r chi.Router) { + r.Get("/metadata", func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusNoContent) + }) + r.Get("/metadata-error", func(w http.ResponseWriter, _ *http.Request) { + http.Error(w, "metadata unavailable", http.StatusInternalServerError) + }) + }, + ) + + tests := []struct { + name string + method string + path string + expectedStatus int + expectChallenge bool + expectAllowHeader bool + }{ + { + name: "MCP preflight", + method: http.MethodOptions, + path: "/", + expectedStatus: http.StatusOK, + expectAllowHeader: true, + }, + { + name: "metadata preflight", + method: http.MethodOptions, + path: "/metadata", + expectedStatus: http.StatusOK, + expectAllowHeader: true, + }, + { + name: "authentication challenge", + method: http.MethodPost, + path: "/", + expectedStatus: http.StatusUnauthorized, + expectChallenge: true, + }, + { + name: "metadata success", + method: http.MethodGet, + path: "/metadata", + expectedStatus: http.StatusNoContent, + }, + { + name: "metadata error", + method: http.MethodGet, + path: "/metadata-error", + expectedStatus: http.StatusInternalServerError, + }, + { + name: "method not allowed", + method: http.MethodPost, + path: "/metadata", + expectedStatus: http.StatusMethodNotAllowed, + }, + { + name: "not found", + method: http.MethodGet, + path: "/not-found", + expectedStatus: http.StatusNotFound, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + req := httptest.NewRequest(tt.method, tt.path, nil) + req.Header.Set("Origin", "https://confer.to") + if tt.method == http.MethodOptions { + req.Header.Set("Access-Control-Request-Method", http.MethodPost) + req.Header.Set("Access-Control-Request-Headers", "content-type") + } + + rec := httptest.NewRecorder() + router.ServeHTTP(rec, req) + + assert.Equal(t, tt.expectedStatus, rec.Code) + assert.Equal(t, "*", rec.Header().Get("Access-Control-Allow-Origin")) + assert.Empty(t, rec.Header().Get("Access-Control-Allow-Credentials")) + assert.Contains(t, rec.Header().Get("Access-Control-Expose-Headers"), "Mcp-Session-Id") + assert.Contains(t, rec.Header().Get("Access-Control-Expose-Headers"), "WWW-Authenticate") + if tt.expectAllowHeader { + assert.Contains(t, rec.Header().Get("Access-Control-Allow-Headers"), "Content-Type") + } + if tt.expectChallenge { + assert.Equal(t, + `Bearer resource_metadata="https://mcp.example.com/.well-known/oauth-protected-resource/mcp"`, + rec.Header().Get("WWW-Authenticate"), + ) + } + }) + } +} + +func TestOAuthChallengeMetadataRouteContracts(t *testing.T) { + const baseURL = "https://mcp.example.com" + oauthCfg := &oauth.Config{ + BaseURL: baseURL, + ResourcePath: "/mcp", + } + apiHost, err := utils.NewAPIHost("https://api.github.com") + require.NoError(t, err) + oauthHandler, err := oauth.NewAuthHandler(oauthCfg, apiHost) + require.NoError(t, err) + + resourcePaths := []string{ + "/", + "/readonly", + "/insiders", + "/readonly/insiders", + "/x/repos", + "/x/repos/readonly", + "/x/repos/insiders", + "/x/repos/readonly/insiders", + } + + router := newHTTPRouter( + func(r chi.Router) { + r.Use(middleware.ExtractUserToken(oauthCfg)) + for _, path := range resourcePaths { + r.Post(path, func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusNoContent) + }) + } + }, + oauthHandler.RegisterRoutes, + ) + + for _, path := range resourcePaths { + t.Run(path, func(t *testing.T) { + req := httptest.NewRequest(http.MethodPost, path, nil) + req.Header.Set("Origin", "https://confer.to") + rec := httptest.NewRecorder() + router.ServeHTTP(rec, req) + + require.Equal(t, http.StatusUnauthorized, rec.Code) + assert.Equal(t, "*", rec.Header().Get("Access-Control-Allow-Origin")) + challenge := rec.Header().Get("WWW-Authenticate") + require.True(t, strings.HasPrefix(challenge, `Bearer resource_metadata="`)) + metadataURL := strings.TrimSuffix( + strings.TrimPrefix(challenge, `Bearer resource_metadata="`), + `"`, + ) + metadataPath := strings.TrimPrefix(metadataURL, baseURL) + + req = httptest.NewRequest(http.MethodGet, metadataPath, nil) + req.Header.Set("Origin", "https://confer.to") + rec = httptest.NewRecorder() + router.ServeHTTP(rec, req) + + require.Equal(t, http.StatusOK, rec.Code) + assert.Equal(t, "*", rec.Header().Get("Access-Control-Allow-Origin")) + + var metadata map[string]any + require.NoError(t, json.Unmarshal(rec.Body.Bytes(), &metadata)) + expectedResourcePath := "/mcp" + if path != "/" { + expectedResourcePath += path + } + assert.Equal(t, baseURL+expectedResourcePath, metadata["resource"]) + }) + } + + req := httptest.NewRequest( + http.MethodGet, + oauth.OAuthProtectedResourcePrefix+"/mcp/unknown", + nil, + ) + req.Header.Set("Origin", "https://confer.to") + rec := httptest.NewRecorder() + router.ServeHTTP(rec, req) + assert.Equal(t, http.StatusNotFound, rec.Code) + assert.Equal(t, "*", rec.Header().Get("Access-Control-Allow-Origin")) + assert.Empty(t, rec.Header().Get("WWW-Authenticate")) +} + func TestInitGlobalToolScopeMapUsesHost(t *testing.T) { tests := []struct { name string