From 4c07ce33e6a5b79ea1e9d77e6b08bed8c76b17d5 Mon Sep 17 00:00:00 2001 From: Shurong Cao Date: Mon, 24 Aug 2026 18:14:29 +0800 Subject: [PATCH] fix(http): apply CORS headers to all top-level responses MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The SetCorsHeaders middleware was only mounted on the MCP route group, so responses from the OAuth protected resource metadata endpoints and any router-level error fell through without Access-Control-* headers. Browser clients then masked the real failure (a 401 auth challenge or 404) behind an opaque "missing CORS headers" error, making the server appear unreachable from web-based MCP clients. Move the middleware to the top-level chi router so every response class — MCP auth challenges (401), OAuth metadata (200), and fall-through errors — carries CORS headers. The server authenticates via bearer tokens rather than cookies, so wildcard origins remain safe at this layer. --- pkg/http/server.go | 10 ++- pkg/http/server_cors_test.go | 127 +++++++++++++++++++++++++++++++++++ 2 files changed, 135 insertions(+), 2 deletions(-) create mode 100644 pkg/http/server_cors_test.go diff --git a/pkg/http/server.go b/pkg/http/server.go index e82be64bb7..bb9af4b43c 100644 --- a/pkg/http/server.go +++ b/pkg/http/server.go @@ -211,9 +211,15 @@ func RunHTTPServer(cfg ServerConfig) error { return fmt.Errorf("failed to create OAuth handler: %w", err) } - r.Group(func(r chi.Router) { - r.Use(middleware.SetCorsHeaders) + // CORS headers are applied at the top level so that every response — + // including the OAuth protected resource metadata endpoints and the + // router's 404 fallback — is readable by browser-based clients. Without + // this, browsers mask the real error (e.g. a 401 auth challenge or 404) + // behind an opaque "missing CORS headers" failure, which made hosted + // servers appear unreachable from web clients (see issue #3095). + r.Use(middleware.SetCorsHeaders) + r.Group(func(r chi.Router) { // Register Middleware First, needs to be before route registration handler.RegisterMiddleware(r) diff --git a/pkg/http/server_cors_test.go b/pkg/http/server_cors_test.go new file mode 100644 index 0000000000..deadaf97e4 --- /dev/null +++ b/pkg/http/server_cors_test.go @@ -0,0 +1,127 @@ +package http + +import ( + "context" + "log/slog" + "net/http" + "net/http/httptest" + "testing" + + "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/translations" + "github.com/github/github-mcp-server/pkg/utils" + "github.com/go-chi/chi/v5" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// newCORSTestServer builds the same top-level router layout as RunHTTPServer +// (MCP routes + OAuth metadata routes under a shared CORS middleware) without +// binding a real port, so tests can assert on CORS behavior for every response +// class: MCP auth challenges (401), OAuth metadata (200), and unmatched paths +// (404). +func newCORSTestServer(t *testing.T) http.Handler { + t.Helper() + + dotcomHost, err := utils.NewAPIHost("https://api.github.com") + require.NoError(t, err) + + handler := NewHTTPMcpHandler( + context.Background(), + &ServerConfig{Version: "test"}, + nil, // deps not exercised by these routes + translations.NullTranslationHelper, + slog.Default(), + dotcomHost, + ) + + oauthHandler, err := oauth.NewAuthHandler(&oauth.Config{ + BaseURL: "https://api.example.com", + }, dotcomHost) + require.NoError(t, err) + + r := chi.NewRouter() + r.Use(middleware.SetCorsHeaders) + + r.Group(func(r chi.Router) { + handler.RegisterMiddleware(r) + handler.RegisterRoutes(r) + }) + r.Group(func(r chi.Router) { + oauthHandler.RegisterRoutes(r) + }) + return r +} + +func TestTopLevelCORSHeadersOnAllResponses(t *testing.T) { + tests := []struct { + name string + method string + path string + wantStatus int + wantACAO string // Access-Control-Allow-Origin + wantExposeWW bool // WWW-Authenticate must be exposed via Access-Control-Expose-Headers + }{ + { + name: "MCP endpoint without token returns 401 challenge with CORS headers", + method: http.MethodPost, + path: "/mcp", + wantStatus: http.StatusUnauthorized, + wantACAO: "*", + wantExposeWW: true, + }, + { + name: "OAuth protected resource metadata returns 200 with CORS headers", + method: http.MethodGet, + path: "/.well-known/oauth-protected-resource", + wantStatus: http.StatusOK, + wantACAO: "*", + wantExposeWW: false, + }, + { + name: "unmatched path falls through to root-mounted MCP handler's 401 with CORS headers", + method: http.MethodGet, + path: "/definitely-not-a-route", + wantStatus: http.StatusUnauthorized, + wantACAO: "*", + wantExposeWW: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + srv := newCORSTestServer(t) + + req := httptest.NewRequest(tt.method, tt.path, nil) + rr := httptest.NewRecorder() + srv.ServeHTTP(rr, req) + + resp := rr.Result() + assert.Equal(t, tt.wantStatus, resp.StatusCode) + assert.Equal(t, tt.wantACAO, resp.Header.Get("Access-Control-Allow-Origin")) + if tt.wantExposeWW { + assert.NotEmpty(t, resp.Header.Get("WWW-Authenticate"), + "401 challenge should carry WWW-Authenticate per MCP spec") + assert.Contains(t, resp.Header.Get("Access-Control-Expose-Headers"), "WWW-Authenticate", + "WWW-Authenticate must be readable cross-origin") + } + }) + } +} + +func TestTopLevelCORSPreflightOnMetadataRoute(t *testing.T) { + srv := newCORSTestServer(t) + + req := httptest.NewRequest(http.MethodOptions, "/.well-known/oauth-protected-resource", nil) + req.Header.Set("Origin", "https://client.example.com") + req.Header.Set("Access-Control-Request-Method", http.MethodGet) + rr := httptest.NewRecorder() + srv.ServeHTTP(rr, req) + + resp := rr.Result() + assert.Equal(t, http.StatusOK, resp.StatusCode, + "preflight against the metadata route should short-circuit with 200, not fall through to 405/404") + assert.Equal(t, "*", resp.Header.Get("Access-Control-Allow-Origin")) + assert.Contains(t, resp.Header.Get("Access-Control-Allow-Methods"), "GET") +}