Skip to content

Commit c393e4a

Browse files
fix(http): preserve CORS across OAuth routes
Apply the browser CORS contract at the root router so authentication, metadata, error, and fallback responses retain the required headers. Register protected-resource metadata for every MCP route variant and verify each challenge round trip.\n\nRefs #3095\n\nCo-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
1 parent 64a49f3 commit c393e4a

7 files changed

Lines changed: 266 additions & 50 deletions

File tree

docs/streamable-http.md

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -80,6 +80,19 @@ The OAuth protected resource metadata's `resource` attribute will be populated w
8080

8181
This allows OAuth clients to discover authentication requirements and endpoint information automatically.
8282

83+
The HTTP server is the OAuth protected resource, not the authorization server. It
84+
therefore serves `/.well-known/oauth-protected-resource` but does not serve
85+
`/.well-known/oauth-authorization-server` unless a separately deployed authorization
86+
server is explicitly hosted on the same origin.
87+
88+
Clients discover authorization-server metadata from the issuer listed in
89+
`authorization_servers`. For the default `https://github.com/login/oauth` issuer,
90+
RFC 8414 path insertion produces
91+
`https://github.com/.well-known/oauth-authorization-server/login/oauth`. Browser-based
92+
clients require that authorization server and its discovery endpoints to support
93+
their browser origin through CORS. If the selected authorization server does not,
94+
configure `--authorization-server` to advertise a browser-compatible OAuth proxy.
95+
8396
### Behind a Trusted Proxy (advanced)
8497

8598
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.

pkg/http/middleware/cors.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,7 @@ func SetCorsHeaders(h http.Handler) http.Handler {
3131
w.Header().Set("Access-Control-Allow-Origin", "*")
3232
w.Header().Set("Access-Control-Allow-Methods", "GET, POST, DELETE, OPTIONS")
3333
w.Header().Set("Access-Control-Max-Age", "86400")
34-
w.Header().Set("Access-Control-Expose-Headers", "Mcp-Session-Id, WWW-Authenticate")
34+
w.Header().Add("Access-Control-Expose-Headers", "Mcp-Session-Id, WWW-Authenticate")
3535
w.Header().Set("Access-Control-Allow-Headers", allowHeaders)
3636

3737
if r.Method == http.MethodOptions {

pkg/http/middleware/cors_test.go

Lines changed: 19 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3,26 +3,35 @@ package middleware_test
33
import (
44
"net/http"
55
"net/http/httptest"
6+
"strings"
67
"testing"
78

89
"github.com/github/github-mcp-server/pkg/http/middleware"
910
"github.com/stretchr/testify/assert"
1011
)
1112

1213
func TestSetCorsHeaders(t *testing.T) {
14+
innerCalled := false
1315
inner := http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
16+
innerCalled = true
17+
w.Header().Add("Access-Control-Expose-Headers", "X-Existing-Response")
1418
w.WriteHeader(http.StatusOK)
1519
})
1620
handler := middleware.SetCorsHeaders(inner)
1721

1822
t.Run("OPTIONS preflight returns 200 with CORS headers", func(t *testing.T) {
23+
innerCalled = false
1924
req := httptest.NewRequest(http.MethodOptions, "/", nil)
20-
req.Header.Set("Origin", "http://localhost:6274")
25+
req.Header.Set("Origin", "https://confer.to")
26+
req.Header.Set("Access-Control-Request-Method", http.MethodPost)
27+
req.Header.Set("Access-Control-Request-Headers", "content-type")
2128
rr := httptest.NewRecorder()
2229
handler.ServeHTTP(rr, req)
2330

2431
assert.Equal(t, http.StatusOK, rr.Code)
32+
assert.False(t, innerCalled)
2533
assert.Equal(t, "*", rr.Header().Get("Access-Control-Allow-Origin"))
34+
assert.Empty(t, rr.Header().Get("Access-Control-Allow-Credentials"))
2635
assert.Contains(t, rr.Header().Get("Access-Control-Allow-Methods"), "POST")
2736
assert.Contains(t, rr.Header().Get("Access-Control-Allow-Headers"), "Authorization")
2837
assert.Contains(t, rr.Header().Get("Access-Control-Allow-Headers"), "Content-Type")
@@ -33,13 +42,20 @@ func TestSetCorsHeaders(t *testing.T) {
3342
assert.Contains(t, rr.Header().Get("Access-Control-Expose-Headers"), "WWW-Authenticate")
3443
})
3544

36-
t.Run("POST request includes CORS headers", func(t *testing.T) {
45+
t.Run("POST request includes CORS headers without replacing existing exposed headers", func(t *testing.T) {
46+
innerCalled = false
3747
req := httptest.NewRequest(http.MethodPost, "/", nil)
38-
req.Header.Set("Origin", "http://localhost:6274")
48+
req.Header.Set("Origin", "https://confer.to")
3949
rr := httptest.NewRecorder()
4050
handler.ServeHTTP(rr, req)
4151

4252
assert.Equal(t, http.StatusOK, rr.Code)
53+
assert.True(t, innerCalled)
4354
assert.Equal(t, "*", rr.Header().Get("Access-Control-Allow-Origin"))
55+
assert.Empty(t, rr.Header().Get("Access-Control-Allow-Credentials"))
56+
exposedHeaders := strings.Join(rr.Header().Values("Access-Control-Expose-Headers"), ", ")
57+
assert.Contains(t, exposedHeaders, "Mcp-Session-Id")
58+
assert.Contains(t, exposedHeaders, "WWW-Authenticate")
59+
assert.Contains(t, exposedHeaders, "X-Existing-Response")
4460
})
4561
}

pkg/http/oauth/oauth.go

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -82,11 +82,14 @@ func NewAuthHandler(cfg *Config, apiHost utils.APIHostResolver) (*AuthHandler, e
8282

8383
// routePatterns defines the route patterns for OAuth protected resource metadata.
8484
var routePatterns = []string{
85-
"", // Root: /.well-known/oauth-protected-resource
86-
"/readonly", // Read-only mode
87-
"/insiders", // Insiders mode
85+
"", // Root: /.well-known/oauth-protected-resource
86+
"/readonly",
87+
"/insiders",
88+
"/readonly/insiders",
8889
"/x/{toolset}",
8990
"/x/{toolset}/readonly",
91+
"/x/{toolset}/insiders",
92+
"/x/{toolset}/readonly/insiders",
9093
}
9194

9295
// RegisterRoutes registers the OAuth protected resource metadata routes.

pkg/http/oauth/oauth_test.go

Lines changed: 28 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -542,36 +542,36 @@ func TestRegisterRoutes(t *testing.T) {
542542
router := chi.NewRouter()
543543
handler.RegisterRoutes(router)
544544

545-
// List of expected routes that should be registered
546-
expectedRoutes := []string{
547-
OAuthProtectedResourcePrefix,
548-
OAuthProtectedResourcePrefix + "/",
549-
OAuthProtectedResourcePrefix + "/mcp",
550-
OAuthProtectedResourcePrefix + "/mcp/",
551-
OAuthProtectedResourcePrefix + "/readonly",
552-
OAuthProtectedResourcePrefix + "/readonly/",
553-
OAuthProtectedResourcePrefix + "/mcp/readonly",
554-
OAuthProtectedResourcePrefix + "/mcp/readonly/",
555-
OAuthProtectedResourcePrefix + "/x/repos",
556-
OAuthProtectedResourcePrefix + "/mcp/x/repos",
545+
resourcePaths := []string{
546+
"",
547+
"/readonly",
548+
"/insiders",
549+
"/readonly/insiders",
550+
"/x/repos",
551+
"/x/repos/readonly",
552+
"/x/repos/insiders",
553+
"/x/repos/readonly/insiders",
557554
}
558555

559-
for _, route := range expectedRoutes {
560-
t.Run("route:"+route, func(t *testing.T) {
561-
// Test GET
562-
req := httptest.NewRequest(http.MethodGet, route, nil)
563-
req.Host = "api.example.com"
564-
rec := httptest.NewRecorder()
565-
router.ServeHTTP(rec, req)
566-
assert.Equal(t, http.StatusOK, rec.Code, "GET %s should return 200", route)
567-
568-
// Test OPTIONS (CORS preflight)
569-
req = httptest.NewRequest(http.MethodOptions, route, nil)
570-
req.Host = "api.example.com"
571-
rec = httptest.NewRecorder()
572-
router.ServeHTTP(rec, req)
573-
assert.Equal(t, http.StatusNoContent, rec.Code, "OPTIONS %s should return 204", route)
574-
})
556+
for _, basePath := range []string{"", "/mcp"} {
557+
for _, resourcePath := range resourcePaths {
558+
for _, trailingSlash := range []string{"", "/"} {
559+
route := OAuthProtectedResourcePrefix + basePath + resourcePath + trailingSlash
560+
t.Run("route:"+route, func(t *testing.T) {
561+
req := httptest.NewRequest(http.MethodGet, route, nil)
562+
req.Host = "api.example.com"
563+
rec := httptest.NewRecorder()
564+
router.ServeHTTP(rec, req)
565+
assert.Equal(t, http.StatusOK, rec.Code, "GET %s should return 200", route)
566+
567+
req = httptest.NewRequest(http.MethodOptions, route, nil)
568+
req.Host = "api.example.com"
569+
rec = httptest.NewRecorder()
570+
router.ServeHTTP(rec, req)
571+
assert.Equal(t, http.StatusNoContent, rec.Code, "OPTIONS %s should return 204", route)
572+
})
573+
}
574+
}
575575
}
576576
}
577577

pkg/http/server.go

Lines changed: 17 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -204,28 +204,22 @@ func RunHTTPServer(cfg ServerConfig) error {
204204
WithScopeFetcher(scopeFetcher),
205205
}
206206

207-
r := chi.NewRouter()
208207
handler := NewHTTPMcpHandler(ctx, &cfg, deps, t, logger, apiHost, append(serverOptions, WithFeatureChecker(featureChecker), WithOAuthConfig(oauthCfg))...)
209208
oauthHandler, err := oauth.NewAuthHandler(oauthCfg, apiHost)
210209
if err != nil {
211210
return fmt.Errorf("failed to create OAuth handler: %w", err)
212211
}
213212

214-
r.Group(func(r chi.Router) {
215-
r.Use(middleware.SetCorsHeaders)
216-
217-
// Register Middleware First, needs to be before route registration
218-
handler.RegisterMiddleware(r)
219-
220-
// Register MCP server routes
221-
handler.RegisterRoutes(r)
222-
})
213+
r := newHTTPRouter(
214+
func(r chi.Router) {
215+
// Register Middleware First, needs to be before route registration
216+
handler.RegisterMiddleware(r)
217+
// Register MCP server routes
218+
handler.RegisterRoutes(r)
219+
},
220+
oauthHandler.RegisterRoutes,
221+
)
223222
logger.Info("MCP endpoints registered", "baseURL", cfg.BaseURL)
224-
225-
r.Group(func(r chi.Router) {
226-
// Register OAuth protected resource metadata endpoints
227-
oauthHandler.RegisterRoutes(r)
228-
})
229223
logger.Info("OAuth protected resource endpoints registered", "baseURL", cfg.BaseURL)
230224

231225
addr := resolveListenAddress(cfg.ListenHost, cfg.Port)
@@ -259,6 +253,14 @@ func RunHTTPServer(cfg ServerConfig) error {
259253
return nil
260254
}
261255

256+
func newHTTPRouter(registerMCPRoutes, registerOAuthRoutes func(chi.Router)) chi.Router {
257+
r := chi.NewRouter()
258+
r.Use(middleware.SetCorsHeaders)
259+
r.Group(registerMCPRoutes)
260+
r.Group(registerOAuthRoutes)
261+
return r
262+
}
263+
262264
func newOAuthConfig(cfg ServerConfig) *oauth.Config {
263265
return &oauth.Config{
264266
BaseURL: cfg.BaseURL,

0 commit comments

Comments
 (0)