From c5e3f6fe668937f5a35095e5995225be1a6873ed Mon Sep 17 00:00:00 2001 From: Shurong Cao <170531907+CAOShurong@users.noreply.github.com> Date: Mon, 24 Aug 2026 23:06:54 +0800 Subject: [PATCH 1/3] fix(http): preserve feature query across OAuth resource metadata Review on #3146 identified that a query-bearing MCP server URL breaks OAuth protected-resource metadata discovery: go-sdk v1.7.0 validates metadata.resource with exact string equality against the full server URL, but BuildResourceMetadataURL and buildResourceURL dropped the request's RawQuery, so clients connecting to e.g. /mcp/x/issues?features=issue_dependencies received challenge and metadata URLs without the query and could reject the metadata as belonging to a different resource (RFC 9728). - Preserve r.URL.RawQuery in both the advertised resource_metadata URL and the metadata document's resource via a shared AppendQuery helper. - Make feature selection presence-based: the features query parameter and the X-MCP-Features header are separate channels that are never combined; query wins when both are present. - Extend TestOAuthChallengeMetadataRouteContracts with a query-bearing MCP URL round-trip (challenge URL + metadata.resource exact match). - Add TestWithRequestConfigFeatureSelection covering all four channel combinations, plus unit tests for query preservation in TestBuildResourceMetadataURL. --- docs/feature-flags.md | 6 ++ docs/server-configuration.md | 2 +- pkg/http/middleware/request_config.go | 27 +++++++- pkg/http/middleware/request_config_test.go | 77 ++++++++++++++++++++++ pkg/http/oauth/oauth.go | 25 ++++++- pkg/http/oauth/oauth_test.go | 22 +++++++ pkg/http/server_test.go | 34 ++++++++++ 7 files changed, 186 insertions(+), 7 deletions(-) create mode 100644 pkg/http/middleware/request_config_test.go diff --git a/docs/feature-flags.md b/docs/feature-flags.md index 0ed3f9dc0e..bf054b83f7 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", From 0c3dc680f33a19fae472e92f75efd67ea2f2bd3a Mon Sep 17 00:00:00 2001 From: Shurong Cao <170531907+CAOShurong@users.noreply.github.com> Date: Fri, 28 Aug 2026 03:33:51 +0800 Subject: [PATCH 2/3] fix(http): give feature header precedence over query --- docs/feature-flags.md | 5 ++-- pkg/http/middleware/request_config.go | 34 +++++++++++++--------- pkg/http/middleware/request_config_test.go | 32 +++++++++++++++++--- 3 files changed, 52 insertions(+), 19 deletions(-) diff --git a/docs/feature-flags.md b/docs/feature-flags.md index bf054b83f7..a965956c43 100644 --- a/docs/feature-flags.md +++ b/docs/feature-flags.md @@ -19,8 +19,9 @@ section in the Insiders docs](./insiders-features.md#how-feature-flags-are-resol 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. +When both the query parameter and the header are present, the header wins — +even when its value is empty, whitespace-only, or contains only unknown flags. +The two channels are never combined. Only flags listed in [`AllowedFeatureFlags`](../pkg/github/feature_flags.go) can be enabled by diff --git a/pkg/http/middleware/request_config.go b/pkg/http/middleware/request_config.go index fa01db3271..136cd7ee20 100644 --- a/pkg/http/middleware/request_config.go +++ b/pkg/http/middleware/request_config.go @@ -18,9 +18,9 @@ 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. +// Feature flags may also arrive via the `features` URL query parameter. When +// both are present, the X-MCP-Features header takes precedence and the two +// channels are never combined. func WithRequestConfig(next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { ctx := r.Context() @@ -57,18 +57,26 @@ func WithRequestConfig(next http.Handler) http.Handler { // 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. + // present is used as-is, and they are never combined. Header presence + // takes precedence even when its value is empty or contains only + // unknown flags, so a query parameter cannot override an explicit + // header. Unknown flags are dropped later by ResolveFeatureFlags + // against AllowedFeatureFlags. queryFeatures, hasQuery := r.URL.Query()[queryParamFeatures] - headerFeatures := r.Header.Get(headers.MCPFeaturesHeader) + headerFeatures, hasHeader := r.Header[http.CanonicalHeaderKey(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)) + case hasHeader: + headerValue := "" + if len(headerFeatures) > 0 { + headerValue = headerFeatures[0] + } + ctx = ghcontext.WithHeaderFeatures(ctx, headers.ParseCommaSeparated(headerValue)) + case hasQuery: + queryValue := "" + if len(queryFeatures) > 0 { + queryValue = queryFeatures[0] + } + ctx = ghcontext.WithHeaderFeatures(ctx, headers.ParseCommaSeparated(queryValue)) } next.ServeHTTP(w, r.WithContext(ctx)) diff --git a/pkg/http/middleware/request_config_test.go b/pkg/http/middleware/request_config_test.go index b615b4f1f9..520acaf8d1 100644 --- a/pkg/http/middleware/request_config_test.go +++ b/pkg/http/middleware/request_config_test.go @@ -13,6 +13,7 @@ func TestWithRequestConfigFeatureSelection(t *testing.T) { tests := []struct { name string url string + headerSet bool headerValue string wantFeatures []string }{ @@ -24,18 +25,41 @@ func TestWithRequestConfigFeatureSelection(t *testing.T) { { name: "header only", url: "/", + headerSet: true, headerValue: "mcp_holdback_consolidated_projects", wantFeatures: []string{"mcp_holdback_consolidated_projects"}, }, { - name: "query parameter wins over header, never combined", + name: "header wins over query parameter, never combined", url: "/?features=flag_from_query", + headerSet: true, headerValue: "flag_from_header", - wantFeatures: []string{"flag_from_query"}, + wantFeatures: []string{"flag_from_header"}, + }, + { + name: "empty header suppresses query parameter", + url: "/?features=flag_from_query", + headerSet: true, + wantFeatures: []string{}, + }, + { + name: "whitespace-only header suppresses query parameter", + url: "/?features=flag_from_query", + headerSet: true, + headerValue: " , \t ", + wantFeatures: []string{}, + }, + { + name: "unknown header suppresses query parameter", + url: "/?features=flag_from_query", + headerSet: true, + headerValue: "unknown_from_header", + wantFeatures: []string{"unknown_from_header"}, }, { - name: "empty query value falls back to header", + name: "empty query value with header", url: "/?features=", + headerSet: true, headerValue: "flag_from_header", wantFeatures: []string{"flag_from_header"}, }, @@ -55,7 +79,7 @@ func TestWithRequestConfigFeatureSelection(t *testing.T) { })) req := httptest.NewRequest(http.MethodPost, tc.url, nil) - if tc.headerValue != "" { + if tc.headerSet { req.Header.Set(headers.MCPFeaturesHeader, tc.headerValue) } rec := httptest.NewRecorder() From e1a072890b89ac04f464e04f9fcf001921a4298a Mon Sep 17 00:00:00 2001 From: Sam Morrow Date: Tue, 1 Sep 2026 12:46:17 +0200 Subject: [PATCH 3/3] fix(http): harden URL feature flag handling Preserve exact OAuth resource queries across route variants, retain presence-based header precedence, and mark feature-dependent responses with Vary. Expand request, allowlist, metadata, route, and cache behavior coverage. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- docs/feature-flags.md | 10 ++++- docs/insiders-features.md | 7 +++- pkg/context/request.go | 6 +-- pkg/github/feature_flags.go | 6 ++- pkg/github/tools.go | 6 +-- pkg/http/handler_test.go | 40 ++++++++++++++---- pkg/http/middleware/request_config.go | 40 +++++------------- pkg/http/middleware/request_config_test.go | 31 +++++++++----- pkg/http/oauth/oauth.go | 17 +++----- pkg/http/oauth/oauth_test.go | 47 +++++++++++++++++++++- pkg/http/server.go | 8 ++-- pkg/http/server_test.go | 22 ++++++---- 12 files changed, 156 insertions(+), 84 deletions(-) diff --git a/docs/feature-flags.md b/docs/feature-flags.md index a965956c43..fe72955b08 100644 --- a/docs/feature-flags.md +++ b/docs/feature-flags.md @@ -23,6 +23,14 @@ When both the query parameter and the header are present, the header wins — even when its value is empty, whitespace-only, or contains only unknown flags. The two channels are never combined. +The complete query string is preserved during OAuth protected-resource metadata +discovery because it is part of the canonical resource identifier. Query +parameters also participate in HTTP cache keys, while MCP responses vary on +`X-MCP-Features` so a header override cannot reuse a response selected for a +different feature set. Feature names are configuration identifiers, not +secrets; as with any URL query value, they may appear in client history, proxy +logs, and server access logs. + Only flags listed in [`AllowedFeatureFlags`](../pkg/github/feature_flags.go) can be enabled by end users. Insiders-only flags are not user-toggleable. @@ -364,7 +372,7 @@ runtime behavior (such as output formatting) won't appear here. ### `thread_resolution_reason` - **pull_request_review_write** - Write operations (create, submit, delete) on pull request reviews - - **Required OAuth Scopes**: `repo` + - **OAuth Challenge Scopes**: `repo` - `body`: Review comment text (string, optional) - `commitID`: SHA of commit to review (string, optional) - `event`: Review action to perform. (string, optional) diff --git a/docs/insiders-features.md b/docs/insiders-features.md index 6c941d97a1..6191857ac8 100644 --- a/docs/insiders-features.md +++ b/docs/insiders-features.md @@ -200,7 +200,9 @@ Insiders is a **meta feature flag** — the same shape as `default` or `all` for 1. **User input.** Users may opt into specific features: - Local server: `--features=,` CLI flag (or `GITHUB_FEATURES` env var). - - Self-hosted HTTP server: `X-MCP-Features: ,` request header. + - HTTP server: `X-MCP-Features: ,` request header or a + `?features=,` server URL. Header presence takes precedence, + and the two request channels are never combined. 2. **Allowlist filter.** User-supplied flags are filtered against [`AllowedFeatureFlags`](../pkg/github/feature_flags.go). Anything not on the allowlist is silently dropped — flags missing from the allowlist can only be turned on by remote-server feature management, not by end users. 3. **Insiders expansion.** If insiders mode is on (`--insiders`, `/insiders` route, or `X-MCP-Insiders: true`), every flag in [`InsidersFeatureFlags`](../pkg/github/feature_flags.go) is unioned in. The insiders expansion is **not** re-validated against the allowlist — insiders is a server-controlled switch that can reach internal-only flags. 4. **Server-side fallback (remote server only).** Any flag not yet decided falls back to the remote server's feature manager, which can roll a feature out independently of user input or insiders membership. @@ -214,7 +216,8 @@ Insiders is a **meta feature flag** — the same shape as `default` or `all` for ### Adding a new feature flag 1. Add a constant in `pkg/github/feature_flags.go`. -2. Add it to `AllowedFeatureFlags` if end users should be able to opt in via `--features` / `X-MCP-Features`. +2. Add it to `AllowedFeatureFlags` if end users should be able to opt in via + `--features`, `X-MCP-Features`, or the `features` URL query parameter. 3. Add it to `InsidersFeatureFlags` if insiders mode should turn it on automatically. 4. Gate the behavior on the concrete flag (`deps.IsFeatureEnabled(ctx, FeatureFlagX)`), never on `cfg.InsidersMode`. There is a `TestGitHubPackageDoesNotReadInsidersMode` guard test that fails if `pkg/github` reads `InsidersMode` directly. 5. The MCP-diff CI workflow picks up new entries in `AllowedFeatureFlags` automatically — see `.github/workflows/mcp-diff.yml`. diff --git a/pkg/context/request.go b/pkg/context/request.go index 6d8d8a1060..548dc7f575 100644 --- a/pkg/context/request.go +++ b/pkg/context/request.go @@ -98,15 +98,15 @@ func GetExcludeTools(ctx context.Context) []string { return nil } -// headerFeaturesCtxKey is a context key for raw header feature flags +// headerFeaturesCtxKey is a context key for raw HTTP request feature flags. type headerFeaturesCtxKey struct{} -// WithHeaderFeatures stores the raw feature flags from the X-MCP-Features header into context +// WithHeaderFeatures stores raw HTTP request feature flags in context. func WithHeaderFeatures(ctx context.Context, features []string) context.Context { return context.WithValue(ctx, headerFeaturesCtxKey{}, features) } -// GetHeaderFeatures retrieves the raw feature flags from context +// GetHeaderFeatures retrieves raw HTTP request feature flags from context. func GetHeaderFeatures(ctx context.Context) []string { if features, ok := ctx.Value(headerFeaturesCtxKey{}).([]string); ok { return features diff --git a/pkg/github/feature_flags.go b/pkg/github/feature_flags.go index 27202c5c83..a388f30d6b 100644 --- a/pkg/github/feature_flags.go +++ b/pkg/github/feature_flags.go @@ -38,7 +38,8 @@ const FeatureFlagDuplicateDetection = "duplicate_detection" const FeatureFlagThreadResolutionReason = "thread_resolution_reason" // AllowedFeatureFlags is the allowlist of feature flags that can be enabled -// by users via --features CLI flag or X-MCP-Features HTTP header. +// by users via --features CLI flag, X-MCP-Features HTTP header, or the +// features URL query parameter. // Only flags in this list are accepted; unknown flags are silently ignored. // This is the single source of truth for which flags are user-controllable. var AllowedFeatureFlags = []string{ @@ -71,7 +72,8 @@ type FeatureFlags struct { } // ResolveFeatureFlags computes the effective set of enabled feature flags by: -// 1. Taking the user-supplied flags (from --features or X-MCP-Features) and +// 1. Taking the user-supplied flags (from --features or HTTP request +// configuration) and // keeping only those present in AllowedFeatureFlags. Unknown or unsafe // flags from request input are silently dropped here. // 2. If insiders mode is on, unioning in every flag from InsidersFeatureFlags. diff --git a/pkg/github/tools.go b/pkg/github/tools.go index 8d568878db..ca46deadd2 100644 --- a/pkg/github/tools.go +++ b/pkg/github/tools.go @@ -159,9 +159,9 @@ var ( FeatureFlagPullRequestsGranular = "pull_requests_granular" ) -// HeaderAllowedFeatureFlags returns the feature flags that clients may enable via -// the X-MCP-Features header. It delegates to AllowedFeatureFlags as the single -// source of truth. +// HeaderAllowedFeatureFlags returns the feature flags that clients may enable +// through the X-MCP-Features header or features URL query parameter. It +// delegates to AllowedFeatureFlags as the single source of truth. func HeaderAllowedFeatureFlags() []string { return slices.Clone(AllowedFeatureFlags) } diff --git a/pkg/http/handler_test.go b/pkg/http/handler_test.go index f051084785..406f845897 100644 --- a/pkg/http/handler_test.go +++ b/pkg/http/handler_test.go @@ -177,9 +177,9 @@ func testTools() []inventory.ServerTool { mockTool("create_issue", "issues", false), mockTool("list_pull_requests", "pull_requests", true), mockTool("create_pull_request", "pull_requests", false), - // Feature-flagged tools for testing X-MCP-Features header - mockToolWithFeatureFlag("needs_holdback", "repos", true, "mcp_holdback_consolidated_projects", ""), - mockToolWithFeatureFlag("hidden_by_holdback", "repos", true, "", "mcp_holdback_consolidated_projects"), + // Feature-flagged tools for testing per-request feature selection. + mockToolWithFeatureFlag("needs_holdback", "repos", true, github.FeatureFlagIssueDependencies, ""), + mockToolWithFeatureFlag("hidden_by_holdback", "repos", true, "", github.FeatureFlagIssueDependencies), } } @@ -293,7 +293,7 @@ func TestHTTPHandlerRoutes(t *testing.T) { name: "X-MCP-Features header enables flagged tool", path: "/", headers: map[string]string{ - headers.MCPFeaturesHeader: "mcp_holdback_consolidated_projects", + headers.MCPFeaturesHeader: github.FeatureFlagIssueDependencies, }, expectedTools: []string{"get_file_contents", "create_repository", "list_issues", "create_issue", "list_pull_requests", "create_pull_request", "needs_holdback"}, }, @@ -305,6 +305,29 @@ func TestHTTPHandlerRoutes(t *testing.T) { }, expectedTools: []string{"get_file_contents", "create_repository", "list_issues", "create_issue", "list_pull_requests", "create_pull_request", "hidden_by_holdback"}, }, + { + name: "features query parameter enables allowlisted feature", + path: "/?features=" + github.FeatureFlagIssueDependencies, + expectedTools: []string{"get_file_contents", "create_repository", "list_issues", "create_issue", "list_pull_requests", "create_pull_request", "needs_holdback"}, + }, + { + name: "features query parameter works with toolset and readonly routes", + path: "/x/repos/readonly?features=" + github.FeatureFlagIssueDependencies, + expectedTools: []string{"get_file_contents", "needs_holdback"}, + }, + { + name: "unknown feature in query parameter is ignored", + path: "/?features=unknown_flag", + expectedTools: []string{"get_file_contents", "create_repository", "list_issues", "create_issue", "list_pull_requests", "create_pull_request", "hidden_by_holdback"}, + }, + { + name: "unknown header suppresses allowlisted query feature", + path: "/?features=" + github.FeatureFlagIssueDependencies, + headers: map[string]string{ + headers.MCPFeaturesHeader: "unknown_flag", + }, + expectedTools: []string{"get_file_contents", "create_repository", "list_issues", "create_issue", "list_pull_requests", "create_pull_request", "hidden_by_holdback"}, + }, { name: "X-MCP-Exclude-Tools header removes specific tools", path: "/", @@ -346,10 +369,13 @@ func TestHTTPHandlerRoutes(t *testing.T) { var capturedInventory *inventory.Inventory var capturedCtx context.Context - // Create feature checker that reads from context without whitelist validation - // (the whitelist is tested separately; here we test the filtering logic) + // Match the production allowlist and insiders expansion behavior. featureChecker := func(ctx context.Context, flag string) (bool, error) { - return slices.Contains(ghcontext.GetHeaderFeatures(ctx), flag), nil + effective := github.ResolveFeatureFlags( + ghcontext.GetHeaderFeatures(ctx), + ghcontext.IsInsidersMode(ctx), + ) + return effective[flag], nil } apiHost, err := utils.NewAPIHost("https://api.github.com") diff --git a/pkg/http/middleware/request_config.go b/pkg/http/middleware/request_config.go index 136cd7ee20..dee8a2c6f4 100644 --- a/pkg/http/middleware/request_config.go +++ b/pkg/http/middleware/request_config.go @@ -9,20 +9,15 @@ 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 X-MCP-Features header takes precedence and the two -// channels are never combined. func WithRequestConfig(next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + // Header-selected features can change the response for the same URL. + w.Header().Add(headers.VaryHeader, headers.MCPFeaturesHeader) + ctx := r.Context() // Readonly mode @@ -55,28 +50,13 @@ func WithRequestConfig(next http.Handler) http.Handler { ctx = ghcontext.WithInsidersMode(ctx, true) } - // 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. Header presence - // takes precedence even when its value is empty or contains only - // unknown flags, so a query parameter cannot override an explicit - // header. Unknown flags are dropped later by ResolveFeatureFlags - // against AllowedFeatureFlags. - queryFeatures, hasQuery := r.URL.Query()[queryParamFeatures] - headerFeatures, hasHeader := r.Header[http.CanonicalHeaderKey(headers.MCPFeaturesHeader)] - switch { - case hasHeader: - headerValue := "" - if len(headerFeatures) > 0 { - headerValue = headerFeatures[0] - } - ctx = ghcontext.WithHeaderFeatures(ctx, headers.ParseCommaSeparated(headerValue)) - case hasQuery: - queryValue := "" - if len(queryFeatures) > 0 { - queryValue = queryFeatures[0] - } - ctx = ghcontext.WithHeaderFeatures(ctx, headers.ParseCommaSeparated(queryValue)) + query := r.URL.Query() + _, hasHeaderFeatures := r.Header[http.CanonicalHeaderKey(headers.MCPFeaturesHeader)] + _, hasQueryFeatures := query[queryParamFeatures] + if hasHeaderFeatures { + ctx = ghcontext.WithHeaderFeatures(ctx, headers.ParseCommaSeparated(r.Header.Get(headers.MCPFeaturesHeader))) + } else if hasQueryFeatures { + ctx = ghcontext.WithHeaderFeatures(ctx, headers.ParseCommaSeparated(query.Get(queryParamFeatures))) } next.ServeHTTP(w, r.WithContext(ctx)) diff --git a/pkg/http/middleware/request_config_test.go b/pkg/http/middleware/request_config_test.go index 520acaf8d1..8aef0fa17a 100644 --- a/pkg/http/middleware/request_config_test.go +++ b/pkg/http/middleware/request_config_test.go @@ -7,6 +7,7 @@ import ( ghcontext "github.com/github/github-mcp-server/pkg/context" "github.com/github/github-mcp-server/pkg/http/headers" + "github.com/stretchr/testify/assert" ) func TestWithRequestConfigFeatureSelection(t *testing.T) { @@ -16,11 +17,13 @@ func TestWithRequestConfigFeatureSelection(t *testing.T) { headerSet bool headerValue string wantFeatures []string + wantPresent bool }{ { name: "query parameter only", url: "/?features=mcp_holdback_consolidated_projects", wantFeatures: []string{"mcp_holdback_consolidated_projects"}, + wantPresent: true, }, { name: "header only", @@ -28,6 +31,7 @@ func TestWithRequestConfigFeatureSelection(t *testing.T) { headerSet: true, headerValue: "mcp_holdback_consolidated_projects", wantFeatures: []string{"mcp_holdback_consolidated_projects"}, + wantPresent: true, }, { name: "header wins over query parameter, never combined", @@ -35,12 +39,14 @@ func TestWithRequestConfigFeatureSelection(t *testing.T) { headerSet: true, headerValue: "flag_from_header", wantFeatures: []string{"flag_from_header"}, + wantPresent: true, }, { name: "empty header suppresses query parameter", url: "/?features=flag_from_query", headerSet: true, wantFeatures: []string{}, + wantPresent: true, }, { name: "whitespace-only header suppresses query parameter", @@ -48,6 +54,7 @@ func TestWithRequestConfigFeatureSelection(t *testing.T) { headerSet: true, headerValue: " , \t ", wantFeatures: []string{}, + wantPresent: true, }, { name: "unknown header suppresses query parameter", @@ -55,6 +62,7 @@ func TestWithRequestConfigFeatureSelection(t *testing.T) { headerSet: true, headerValue: "unknown_from_header", wantFeatures: []string{"unknown_from_header"}, + wantPresent: true, }, { name: "empty query value with header", @@ -62,6 +70,13 @@ func TestWithRequestConfigFeatureSelection(t *testing.T) { headerSet: true, headerValue: "flag_from_header", wantFeatures: []string{"flag_from_header"}, + wantPresent: true, + }, + { + name: "empty query value stores an explicit empty selection", + url: "/?features=", + wantFeatures: []string{}, + wantPresent: true, }, { name: "no channel present stores nothing", @@ -85,17 +100,13 @@ func TestWithRequestConfigFeatureSelection(t *testing.T) { 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) - } + assert.Equal(t, tc.wantFeatures, got) + if tc.wantPresent { + assert.NotNil(t, got) + } else { + assert.Nil(t, got) } + assert.Contains(t, rec.Header().Values(headers.VaryHeader), headers.MCPFeaturesHeader) }) } } diff --git a/pkg/http/oauth/oauth.go b/pkg/http/oauth/oauth.go index 2885898141..1d8ba70afa 100644 --- a/pkg/http/oauth/oauth.go +++ b/pkg/http/oauth/oauth.go @@ -187,10 +187,6 @@ 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) @@ -203,13 +199,12 @@ func (h *AuthHandler) buildResourceURL(r *http.Request, resourcePath string) str if !strings.HasPrefix(resourcePath, "/") { resourcePath = "/" + resourcePath } - return AppendQuery(baseURL+resourcePath, r.URL.RawQuery) + return appendRawQuery(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 { +// appendRawQuery avoids re-encoding the resource identifier that RFC 9728 +// clients compare as an exact string. +func appendRawQuery(target, rawQuery string) string { if rawQuery == "" { return target } @@ -268,9 +263,7 @@ func BuildResourceMetadataURL(r *http.Request, cfg *Config, resourcePath string) } else { metadataURL = 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) + return appendRawQuery(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 acff1689ee..39c7e953b4 100644 --- a/pkg/http/oauth/oauth_test.go +++ b/pkg/http/oauth/oauth_test.go @@ -7,6 +7,7 @@ import ( "net/http" "net/http/httptest" "net/url" + "strings" "testing" "github.com/github/github-mcp-server/pkg/http/headers" @@ -384,6 +385,21 @@ func TestBuildResourceMetadataURL(t *testing.T) { resourcePath: "/mcp", expectedURL: "http://api.example.com/.well-known/oauth-protected-resource/mcp?features=a,b", }, + { + name: "raw query encoding and ordering are preserved", + cfg: &Config{ + BaseURL: "https://custom.example.com", + }, + setupRequest: func() *http.Request { + return httptest.NewRequest( + http.MethodGet, + "/mcp?features=issue_dependencies%2Cfile_blame&client=web%20ide", + nil, + ) + }, + resourcePath: "/mcp", + expectedURL: "https://custom.example.com/.well-known/oauth-protected-resource/mcp?features=issue_dependencies%2Cfile_blame&client=web%20ide", + }, } for _, tc := range tests { @@ -484,6 +500,20 @@ func TestHandleProtectedResource(t *testing.T) { assert.Equal(t, "https://api.example.com/mcp/", body["resource"]) }, }, + { + name: "path with feature query", + cfg: &Config{ + BaseURL: "https://api.example.com", + }, + path: OAuthProtectedResourcePrefix + "/mcp/x/repos?features=issue_dependencies", + host: "api.example.com", + method: http.MethodGet, + expectedStatusCode: http.StatusOK, + validateResponse: func(t *testing.T, body map[string]any) { + t.Helper() + assert.Equal(t, "https://api.example.com/mcp/x/repos?features=issue_dependencies", body["resource"]) + }, + }, { name: "custom authorization server in response", cfg: &Config{ @@ -580,11 +610,24 @@ func TestRegisterRoutes(t *testing.T) { for _, trailingSlash := range []string{"", "/"} { route := OAuthProtectedResourcePrefix + basePath + resourcePath + trailingSlash t.Run("route:"+route, func(t *testing.T) { - req := httptest.NewRequest(http.MethodGet, route, nil) + queryRoute := route + "?features=issue_dependencies" + req := httptest.NewRequest(http.MethodGet, queryRoute, 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) + require.Equal(t, http.StatusOK, rec.Code, "GET %s should return 200", queryRoute) + + var metadata map[string]any + require.NoError(t, json.Unmarshal(rec.Body.Bytes(), &metadata)) + resourcePath := resolveResourcePath( + strings.TrimPrefix(route, OAuthProtectedResourcePrefix), + "", + ) + assert.Equal( + t, + "https://api.example.com"+resourcePath+"?features=issue_dependencies", + metadata["resource"], + ) req = httptest.NewRequest(http.MethodOptions, route, nil) req.Host = "api.example.com" diff --git a/pkg/http/server.go b/pkg/http/server.go index cc2d23d3ac..8a3a305e49 100644 --- a/pkg/http/server.go +++ b/pkg/http/server.go @@ -310,13 +310,13 @@ func initGlobalToolScopeMap(t translations.TranslationHelperFunc, hostType utils } // createHTTPFeatureChecker creates a feature checker that resolves static CLI -// features plus per-request header features and insiders mode. +// features plus per-request features and insiders mode. func createHTTPFeatureChecker(enabledFeatures []string, insidersMode bool) inventory.FeatureFlagChecker { return func(ctx context.Context, flag string) (bool, error) { - headerFeatures := ghcontext.GetHeaderFeatures(ctx) - features := make([]string, 0, len(enabledFeatures)+len(headerFeatures)) + requestFeatures := ghcontext.GetHeaderFeatures(ctx) + features := make([]string, 0, len(enabledFeatures)+len(requestFeatures)) features = append(features, enabledFeatures...) - features = append(features, headerFeatures...) + features = append(features, requestFeatures...) effective := github.ResolveFeatureFlags(features, insidersMode || ghcontext.IsInsidersMode(ctx)) return effective[flag], nil diff --git a/pkg/http/server_test.go b/pkg/http/server_test.go index fa019e3980..8c62c01a40 100644 --- a/pkg/http/server_test.go +++ b/pkg/http/server_test.go @@ -282,15 +282,21 @@ func TestOAuthChallengeMetadataRouteContracts(t *testing.T) { 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) + metadataPaths := []string{ + strings.TrimPrefix(metadataURL, baseURL), + oauth.OAuthProtectedResourcePrefix + queryPath, + } + for _, metadataPath := range metadataPaths { + 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) - var metadata map[string]any - require.NoError(t, json.Unmarshal(rec.Body.Bytes(), &metadata)) - assert.Equal(t, baseURL+"/mcp"+queryPath, metadata["resource"]) + 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(