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>
SamMorrowDrums · Sep 1, 2026 · Aug 24, 2026 · Aug 27, 2026 · Sep 1, 2026 · Sep 1, 2026
commit e1a072890b89ac04f464e04f9fcf001921a4298a
@@ -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)

@@ -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=<flag>,<flag>` CLI flag (or `GITHUB_FEATURES` env var).
-   - Self-hosted HTTP server: `X-MCP-Features: <flag>,<flag>` request header.
+   - HTTP server: `X-MCP-Features: <flag>,<flag>` request header or a
+     `?features=<flag>,<flag>` 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`.
@@ -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

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

@@ -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)
 }

@@ -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")

@@ -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))

@@ -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,52 +17,66 @@ 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",
 			url:          "/",
 			headerSet:    true,
 			headerValue:  "mcp_holdback_consolidated_projects",
 			wantFeatures: []string{"mcp_holdback_consolidated_projects"},
+			wantPresent:  true,
 		},
 		{
 			name:         "header wins over query parameter, never combined",
 			url:          "/?features=flag_from_query",
 			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",
 			url:          "/?features=flag_from_query",
 			headerSet:    true,
 			headerValue:  "  , \t ",
 			wantFeatures: []string{},
+			wantPresent:  true,
 		},
 		{
 			name:         "unknown header suppresses query parameter",
 			url:          "/?features=flag_from_query",
 			headerSet:    true,
 			headerValue:  "unknown_from_header",
 			wantFeatures: []string{"unknown_from_header"},
+			wantPresent:  true,
 		},
 		{
 			name:         "empty query value with header",
 			url:          "/?features=",
 			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)
 		})
 	}
 }
@@ -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 {