diff --git a/docs/feature-flags.md b/docs/feature-flags.md index 0de5bdd722..45cb223bad 100644 --- a/docs/feature-flags.md +++ b/docs/feature-flags.md @@ -13,9 +13,15 @@ section in the Insiders docs](./insiders-features.md#how-feature-flags-are-resol | Method | Remote Server | Local Server | |--------|---------------|--------------| | Header | `X-MCP-Features: ,` | N/A | +| URL query parameter | `?features=,` on the server URL | N/A | | CLI flag | N/A | `--features=,` | | Environment variable | N/A | `GITHUB_FEATURES=,` | +The URL query parameter exists for clients that compose the server URL on the +user's behalf (hosted IDEs, agent platforms) and cannot set custom headers. +When both the query parameter and the header are present, the query parameter +wins — the two channels are never combined. + Only flags listed in [`AllowedFeatureFlags`](../pkg/github/feature_flags.go) can be enabled by end users. Insiders-only flags are not user-toggleable. diff --git a/docs/server-configuration.md b/docs/server-configuration.md index 5ec78c6ae4..42584362d9 100644 --- a/docs/server-configuration.md +++ b/docs/server-configuration.md @@ -13,7 +13,7 @@ We currently support the following ways in which the GitHub MCP Server can be co | Read-Only Mode | `X-MCP-Readonly` header or `/readonly` URL | `--read-only` flag or `GITHUB_READ_ONLY` env var | | Lockdown Mode | `X-MCP-Lockdown` header | `--lockdown-mode` flag or `GITHUB_LOCKDOWN_MODE` env var | | Insiders Mode | `X-MCP-Insiders` header or `/insiders` URL | `--insiders` flag or `GITHUB_INSIDERS` env var | -| Feature Flags | `X-MCP-Features` header | `--features` flag | +| Feature Flags | `X-MCP-Features` header or `?features=` URL query parameter | `--features` flag | | Scope Filtering | Always enabled | Always enabled | | Server Name/Title | Not available | `GITHUB_MCP_SERVER_NAME` / `GITHUB_MCP_SERVER_TITLE` env vars or `github-mcp-server-config.json` | diff --git a/pkg/http/middleware/request_config.go b/pkg/http/middleware/request_config.go index a7311334d3..fa01db3271 100644 --- a/pkg/http/middleware/request_config.go +++ b/pkg/http/middleware/request_config.go @@ -9,8 +9,18 @@ import ( "github.com/github/github-mcp-server/pkg/http/headers" ) +// queryParamFeatures is the URL query parameter that carries feature flags, +// mirroring the X-MCP-Features header. It exists so clients that cannot set +// custom headers on the MCP connection — hosted IDEs, agent platforms, or +// harnesses that compose the server URL on the user's behalf (see #3145) — +// can still opt into flagged tools. +const queryParamFeatures = "features" + // WithRequestConfig is a middleware that extracts MCP-related headers and sets them in the request context. // This includes readonly mode, toolsets, tools, lockdown mode, insiders mode, and feature flags. +// Feature flags may also arrive via the `features` URL query parameter; when +// both are present the query parameter wins, matching how the toolset path +// segments take precedence over their headers. func WithRequestConfig(next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { ctx := r.Context() @@ -45,9 +55,20 @@ func WithRequestConfig(next http.Handler) http.Handler { ctx = ghcontext.WithInsidersMode(ctx, true) } - // Feature flags - if features := headers.ParseCommaSeparated(r.Header.Get(headers.MCPFeaturesHeader)); len(features) > 0 { - ctx = ghcontext.WithHeaderFeatures(ctx, features) + // Feature flags: presence-based selection. The URL query parameter and + // the X-MCP-Features header are separate channels — whichever is + // present is used as-is, and they are never combined. When both are + // present the query parameter wins, so a client composing the server + // URL can always express its intent even when it cannot control + // headers. Unknown flags are dropped later by ResolveFeatureFlags + // against AllowedFeatureFlags, so neither channel is privileged. + queryFeatures, hasQuery := r.URL.Query()[queryParamFeatures] + headerFeatures := r.Header.Get(headers.MCPFeaturesHeader) + switch { + case hasQuery && strings.TrimSpace(queryFeatures[0]) != "": + ctx = ghcontext.WithHeaderFeatures(ctx, headers.ParseCommaSeparated(queryFeatures[0])) + case headerFeatures != "": + ctx = ghcontext.WithHeaderFeatures(ctx, headers.ParseCommaSeparated(headerFeatures)) } next.ServeHTTP(w, r.WithContext(ctx)) diff --git a/pkg/http/middleware/request_config_test.go b/pkg/http/middleware/request_config_test.go new file mode 100644 index 0000000000..b615b4f1f9 --- /dev/null +++ b/pkg/http/middleware/request_config_test.go @@ -0,0 +1,77 @@ +package middleware + +import ( + "net/http" + "net/http/httptest" + "testing" + + ghcontext "github.com/github/github-mcp-server/pkg/context" + "github.com/github/github-mcp-server/pkg/http/headers" +) + +func TestWithRequestConfigFeatureSelection(t *testing.T) { + tests := []struct { + name string + url string + headerValue string + wantFeatures []string + }{ + { + name: "query parameter only", + url: "/?features=mcp_holdback_consolidated_projects", + wantFeatures: []string{"mcp_holdback_consolidated_projects"}, + }, + { + name: "header only", + url: "/", + headerValue: "mcp_holdback_consolidated_projects", + wantFeatures: []string{"mcp_holdback_consolidated_projects"}, + }, + { + name: "query parameter wins over header, never combined", + url: "/?features=flag_from_query", + headerValue: "flag_from_header", + wantFeatures: []string{"flag_from_query"}, + }, + { + name: "empty query value falls back to header", + url: "/?features=", + headerValue: "flag_from_header", + wantFeatures: []string{"flag_from_header"}, + }, + { + name: "no channel present stores nothing", + url: "/", + wantFeatures: nil, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + var got []string + handler := WithRequestConfig(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + got = ghcontext.GetHeaderFeatures(r.Context()) + w.WriteHeader(http.StatusNoContent) + })) + + req := httptest.NewRequest(http.MethodPost, tc.url, nil) + if tc.headerValue != "" { + req.Header.Set(headers.MCPFeaturesHeader, tc.headerValue) + } + rec := httptest.NewRecorder() + handler.ServeHTTP(rec, req) + + if tc.wantFeatures == nil && len(got) == 0 { + return + } + if len(got) != len(tc.wantFeatures) { + t.Fatalf("got features %v, want %v", got, tc.wantFeatures) + } + for i := range tc.wantFeatures { + if got[i] != tc.wantFeatures[i] { + t.Fatalf("got features %v, want %v", got, tc.wantFeatures) + } + } + }) + } +} diff --git a/pkg/http/oauth/oauth.go b/pkg/http/oauth/oauth.go index 77cfe8fa10..2885898141 100644 --- a/pkg/http/oauth/oauth.go +++ b/pkg/http/oauth/oauth.go @@ -187,6 +187,10 @@ func ResolveResourcePath(r *http.Request, cfg *Config) string { } // buildResourceURL constructs the full resource URL for OAuth metadata. +// The request's query string is preserved: MCP clients that receive a server +// URL such as /mcp/x/issues?features=... identify the protected resource by +// exact string match against metadata.resource (RFC 9728), so dropping the +// query would make standards-compliant clients reject the metadata. func (h *AuthHandler) buildResourceURL(r *http.Request, resourcePath string) string { host, scheme := GetEffectiveHostAndScheme(r, h.cfg) baseURL := fmt.Sprintf("%s://%s", scheme, host) @@ -199,7 +203,17 @@ func (h *AuthHandler) buildResourceURL(r *http.Request, resourcePath string) str if !strings.HasPrefix(resourcePath, "/") { resourcePath = "/" + resourcePath } - return baseURL + resourcePath + return AppendQuery(baseURL+resourcePath, r.URL.RawQuery) +} + +// AppendQuery appends rawQuery to target when non-empty. It is shared by the +// resource URL and the advertised metadata URL so both consistently carry the +// same query string as the MCP server URL the client connects to. +func AppendQuery(target, rawQuery string) string { + if rawQuery == "" { + return target + } + return target + "?" + rawQuery } // GetEffectiveHostAndScheme returns the effective host and scheme for a request. @@ -248,10 +262,15 @@ func BuildResourceMetadataURL(r *http.Request, cfg *Config, resourcePath string) suffix = resourcePath } } + metadataURL := "" if cfg != nil && cfg.BaseURL != "" { - return strings.TrimSuffix(cfg.BaseURL, "/") + OAuthProtectedResourcePrefix + suffix + metadataURL = strings.TrimSuffix(cfg.BaseURL, "/") + OAuthProtectedResourcePrefix + suffix + } else { + metadataURL = fmt.Sprintf("%s://%s%s%s", scheme, host, OAuthProtectedResourcePrefix, suffix) } - return fmt.Sprintf("%s://%s%s%s", scheme, host, OAuthProtectedResourcePrefix, suffix) + // Preserve the request query so the advertised metadata endpoint matches + // the full resource identifier, including feature-flag query parameters. + return AppendQuery(metadataURL, r.URL.RawQuery) } func normalizeBasePath(path string) string { diff --git a/pkg/http/oauth/oauth_test.go b/pkg/http/oauth/oauth_test.go index 52baae3b6c..acff1689ee 100644 --- a/pkg/http/oauth/oauth_test.go +++ b/pkg/http/oauth/oauth_test.go @@ -362,6 +362,28 @@ func TestBuildResourceMetadataURL(t *testing.T) { resourcePath: "", expectedURL: "http://api.example.com/.well-known/oauth-protected-resource", }, + { + name: "query string is preserved on base URL config", + cfg: &Config{ + BaseURL: "https://custom.example.com", + }, + setupRequest: func() *http.Request { + return httptest.NewRequest(http.MethodGet, "/mcp/x/issues?features=issue_dependencies", nil) + }, + resourcePath: "/mcp/x/issues", + expectedURL: "https://custom.example.com/.well-known/oauth-protected-resource/mcp/x/issues?features=issue_dependencies", + }, + { + name: "query string is preserved without base URL config", + cfg: &Config{}, + setupRequest: func() *http.Request { + req := httptest.NewRequest(http.MethodGet, "/mcp?features=a,b", nil) + req.Host = "api.example.com" + return req + }, + resourcePath: "/mcp", + expectedURL: "http://api.example.com/.well-known/oauth-protected-resource/mcp?features=a,b", + }, } for _, tc := range tests { diff --git a/pkg/http/server_test.go b/pkg/http/server_test.go index a8c4e1a90b..fa019e3980 100644 --- a/pkg/http/server_test.go +++ b/pkg/http/server_test.go @@ -259,6 +259,40 @@ func TestOAuthChallengeMetadataRouteContracts(t *testing.T) { }) } + // Query-bearing MCP server URLs must round-trip: the challenge's + // resource_metadata URL and the served metadata document's "resource" + // must both carry the exact same query as the URL the client connects to, + // because go-sdk validates metadata.resource with exact string equality. + queryPath := "/x/repos?features=issue_dependencies" + t.Run(queryPath, func(t *testing.T) { + req := httptest.NewRequest(http.MethodPost, queryPath, nil) + req.Header.Set("Origin", "https://confer.to") + rec := httptest.NewRecorder() + router.ServeHTTP(rec, req) + + require.Equal(t, http.StatusUnauthorized, rec.Code) + challenge := rec.Header().Get("WWW-Authenticate") + require.True(t, strings.HasPrefix(challenge, `Bearer resource_metadata="`)) + metadataURL := strings.TrimSuffix( + strings.TrimPrefix(challenge, `Bearer resource_metadata="`), + `"`, + ) + assert.Equal(t, + baseURL+"/.well-known/oauth-protected-resource/mcp/x/repos?features=issue_dependencies", + metadataURL, + ) + + req = httptest.NewRequest(http.MethodGet, strings.TrimPrefix(metadataURL, baseURL), nil) + req.Header.Set("Origin", "https://confer.to") + rec = httptest.NewRecorder() + router.ServeHTTP(rec, req) + + require.Equal(t, http.StatusOK, rec.Code) + var metadata map[string]any + require.NoError(t, json.Unmarshal(rec.Body.Bytes(), &metadata)) + assert.Equal(t, baseURL+"/mcp"+queryPath, metadata["resource"]) + }) + req := httptest.NewRequest( http.MethodGet, oauth.OAuthProtectedResourcePrefix+"/mcp/unknown",