Skip to content

Commit ffa16d0

Browse files
CAOShurongSamMorrowDrumsCopilot
authored
Enable feature flags via URL query parameter (?features=) for headerless hosted connections (#3146)
* 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. * fix(http): give feature header precedence over query * 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> --------- Co-authored-by: Shurong Cao <170531907+CAOShurong@users.noreply.github.com> Co-authored-by: Sam Morrow <sammorrowdrums@github.com> Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
1 parent 198bc16 commit ffa16d0

13 files changed

Lines changed: 314 additions & 30 deletions

docs/feature-flags.md

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,9 +13,24 @@ section in the Insiders docs](./insiders-features.md#how-feature-flags-are-resol
1313
| Method | Remote Server | Local Server |
1414
|--------|---------------|--------------|
1515
| Header | `X-MCP-Features: <flag>,<flag>` | N/A |
16+
| URL query parameter | `?features=<flag>,<flag>` on the server URL | N/A |
1617
| CLI flag | N/A | `--features=<flag>,<flag>` |
1718
| Environment variable | N/A | `GITHUB_FEATURES=<flag>,<flag>` |
1819

20+
The URL query parameter exists for clients that compose the server URL on the
21+
user's behalf (hosted IDEs, agent platforms) and cannot set custom headers.
22+
When both the query parameter and the header are present, the header wins —
23+
even when its value is empty, whitespace-only, or contains only unknown flags.
24+
The two channels are never combined.
25+
26+
The complete query string is preserved during OAuth protected-resource metadata
27+
discovery because it is part of the canonical resource identifier. Query
28+
parameters also participate in HTTP cache keys, while MCP responses vary on
29+
`X-MCP-Features` so a header override cannot reuse a response selected for a
30+
different feature set. Feature names are configuration identifiers, not
31+
secrets; as with any URL query value, they may appear in client history, proxy
32+
logs, and server access logs.
33+
1934
Only flags listed in
2035
[`AllowedFeatureFlags`](../pkg/github/feature_flags.go) can be enabled by
2136
end users. Insiders-only flags are not user-toggleable.

docs/insiders-features.md

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -200,7 +200,9 @@ Insiders is a **meta feature flag** — the same shape as `default` or `all` for
200200

201201
1. **User input.** Users may opt into specific features:
202202
- Local server: `--features=<flag>,<flag>` CLI flag (or `GITHUB_FEATURES` env var).
203-
- Self-hosted HTTP server: `X-MCP-Features: <flag>,<flag>` request header.
203+
- HTTP server: `X-MCP-Features: <flag>,<flag>` request header or a
204+
`?features=<flag>,<flag>` server URL. Header presence takes precedence,
205+
and the two request channels are never combined.
204206
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.
205207
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.
206208
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
214216
### Adding a new feature flag
215217

216218
1. Add a constant in `pkg/github/feature_flags.go`.
217-
2. Add it to `AllowedFeatureFlags` if end users should be able to opt in via `--features` / `X-MCP-Features`.
219+
2. Add it to `AllowedFeatureFlags` if end users should be able to opt in via
220+
`--features`, `X-MCP-Features`, or the `features` URL query parameter.
218221
3. Add it to `InsidersFeatureFlags` if insiders mode should turn it on automatically.
219222
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.
220223
5. The MCP-diff CI workflow picks up new entries in `AllowedFeatureFlags` automatically — see `.github/workflows/mcp-diff.yml`.

docs/server-configuration.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@ We currently support the following ways in which the GitHub MCP Server can be co
1313
| Read-Only Mode | `X-MCP-Readonly` header or `/readonly` URL | `--read-only` flag or `GITHUB_READ_ONLY` env var |
1414
| Lockdown Mode | `X-MCP-Lockdown` header | `--lockdown-mode` flag or `GITHUB_LOCKDOWN_MODE` env var |
1515
| Insiders Mode | `X-MCP-Insiders` header or `/insiders` URL | `--insiders` flag or `GITHUB_INSIDERS` env var |
16-
| Feature Flags | `X-MCP-Features` header | `--features` flag |
16+
| Feature Flags | `X-MCP-Features` header or `?features=` URL query parameter | `--features` flag |
1717
| Scope Filtering | Always enabled | Always enabled |
1818
| Server Name/Title | Not available | `GITHUB_MCP_SERVER_NAME` / `GITHUB_MCP_SERVER_TITLE` env vars or `github-mcp-server-config.json` |
1919

pkg/context/request.go

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -98,15 +98,15 @@ func GetExcludeTools(ctx context.Context) []string {
9898
return nil
9999
}
100100

101-
// headerFeaturesCtxKey is a context key for raw header feature flags
101+
// headerFeaturesCtxKey is a context key for raw HTTP request feature flags.
102102
type headerFeaturesCtxKey struct{}
103103

104-
// WithHeaderFeatures stores the raw feature flags from the X-MCP-Features header into context
104+
// WithHeaderFeatures stores raw HTTP request feature flags in context.
105105
func WithHeaderFeatures(ctx context.Context, features []string) context.Context {
106106
return context.WithValue(ctx, headerFeaturesCtxKey{}, features)
107107
}
108108

109-
// GetHeaderFeatures retrieves the raw feature flags from context
109+
// GetHeaderFeatures retrieves raw HTTP request feature flags from context.
110110
func GetHeaderFeatures(ctx context.Context) []string {
111111
if features, ok := ctx.Value(headerFeaturesCtxKey{}).([]string); ok {
112112
return features

pkg/github/feature_flags.go

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,8 @@ const FeatureFlagDuplicateDetection = "duplicate_detection"
3838
const FeatureFlagThreadResolutionReason = "thread_resolution_reason"
3939

4040
// AllowedFeatureFlags is the allowlist of feature flags that can be enabled
41-
// by users via --features CLI flag or X-MCP-Features HTTP header.
41+
// by users via --features CLI flag, X-MCP-Features HTTP header, or the
42+
// features URL query parameter.
4243
// Only flags in this list are accepted; unknown flags are silently ignored.
4344
// This is the single source of truth for which flags are user-controllable.
4445
var AllowedFeatureFlags = []string{
@@ -71,7 +72,8 @@ type FeatureFlags struct {
7172
}
7273

7374
// ResolveFeatureFlags computes the effective set of enabled feature flags by:
74-
// 1. Taking the user-supplied flags (from --features or X-MCP-Features) and
75+
// 1. Taking the user-supplied flags (from --features or HTTP request
76+
// configuration) and
7577
// keeping only those present in AllowedFeatureFlags. Unknown or unsafe
7678
// flags from request input are silently dropped here.
7779
// 2. If insiders mode is on, unioning in every flag from InsidersFeatureFlags.

pkg/github/tools.go

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -159,9 +159,9 @@ var (
159159
FeatureFlagPullRequestsGranular = "pull_requests_granular"
160160
)
161161

162-
// HeaderAllowedFeatureFlags returns the feature flags that clients may enable via
163-
// the X-MCP-Features header. It delegates to AllowedFeatureFlags as the single
164-
// source of truth.
162+
// HeaderAllowedFeatureFlags returns the feature flags that clients may enable
163+
// through the X-MCP-Features header or features URL query parameter. It
164+
// delegates to AllowedFeatureFlags as the single source of truth.
165165
func HeaderAllowedFeatureFlags() []string {
166166
return slices.Clone(AllowedFeatureFlags)
167167
}

pkg/http/handler_test.go

Lines changed: 33 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -177,9 +177,9 @@ func testTools() []inventory.ServerTool {
177177
mockTool("create_issue", "issues", false),
178178
mockTool("list_pull_requests", "pull_requests", true),
179179
mockTool("create_pull_request", "pull_requests", false),
180-
// Feature-flagged tools for testing X-MCP-Features header
181-
mockToolWithFeatureFlag("needs_holdback", "repos", true, "mcp_holdback_consolidated_projects", ""),
182-
mockToolWithFeatureFlag("hidden_by_holdback", "repos", true, "", "mcp_holdback_consolidated_projects"),
180+
// Feature-flagged tools for testing per-request feature selection.
181+
mockToolWithFeatureFlag("needs_holdback", "repos", true, github.FeatureFlagIssueDependencies, ""),
182+
mockToolWithFeatureFlag("hidden_by_holdback", "repos", true, "", github.FeatureFlagIssueDependencies),
183183
}
184184
}
185185

@@ -293,7 +293,7 @@ func TestHTTPHandlerRoutes(t *testing.T) {
293293
name: "X-MCP-Features header enables flagged tool",
294294
path: "/",
295295
headers: map[string]string{
296-
headers.MCPFeaturesHeader: "mcp_holdback_consolidated_projects",
296+
headers.MCPFeaturesHeader: github.FeatureFlagIssueDependencies,
297297
},
298298
expectedTools: []string{"get_file_contents", "create_repository", "list_issues", "create_issue", "list_pull_requests", "create_pull_request", "needs_holdback"},
299299
},
@@ -305,6 +305,29 @@ func TestHTTPHandlerRoutes(t *testing.T) {
305305
},
306306
expectedTools: []string{"get_file_contents", "create_repository", "list_issues", "create_issue", "list_pull_requests", "create_pull_request", "hidden_by_holdback"},
307307
},
308+
{
309+
name: "features query parameter enables allowlisted feature",
310+
path: "/?features=" + github.FeatureFlagIssueDependencies,
311+
expectedTools: []string{"get_file_contents", "create_repository", "list_issues", "create_issue", "list_pull_requests", "create_pull_request", "needs_holdback"},
312+
},
313+
{
314+
name: "features query parameter works with toolset and readonly routes",
315+
path: "/x/repos/readonly?features=" + github.FeatureFlagIssueDependencies,
316+
expectedTools: []string{"get_file_contents", "needs_holdback"},
317+
},
318+
{
319+
name: "unknown feature in query parameter is ignored",
320+
path: "/?features=unknown_flag",
321+
expectedTools: []string{"get_file_contents", "create_repository", "list_issues", "create_issue", "list_pull_requests", "create_pull_request", "hidden_by_holdback"},
322+
},
323+
{
324+
name: "unknown header suppresses allowlisted query feature",
325+
path: "/?features=" + github.FeatureFlagIssueDependencies,
326+
headers: map[string]string{
327+
headers.MCPFeaturesHeader: "unknown_flag",
328+
},
329+
expectedTools: []string{"get_file_contents", "create_repository", "list_issues", "create_issue", "list_pull_requests", "create_pull_request", "hidden_by_holdback"},
330+
},
308331
{
309332
name: "X-MCP-Exclude-Tools header removes specific tools",
310333
path: "/",
@@ -346,10 +369,13 @@ func TestHTTPHandlerRoutes(t *testing.T) {
346369
var capturedInventory *inventory.Inventory
347370
var capturedCtx context.Context
348371

349-
// Create feature checker that reads from context without whitelist validation
350-
// (the whitelist is tested separately; here we test the filtering logic)
372+
// Match the production allowlist and insiders expansion behavior.
351373
featureChecker := func(ctx context.Context, flag string) (bool, error) {
352-
return slices.Contains(ghcontext.GetHeaderFeatures(ctx), flag), nil
374+
effective := github.ResolveFeatureFlags(
375+
ghcontext.GetHeaderFeatures(ctx),
376+
ghcontext.IsInsidersMode(ctx),
377+
)
378+
return effective[flag], nil
353379
}
354380

355381
apiHost, err := utils.NewAPIHost("https://api.github.com")

pkg/http/middleware/request_config.go

Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -9,10 +9,15 @@ import (
99
"github.com/github/github-mcp-server/pkg/http/headers"
1010
)
1111

12+
const queryParamFeatures = "features"
13+
1214
// WithRequestConfig is a middleware that extracts MCP-related headers and sets them in the request context.
1315
// This includes readonly mode, toolsets, tools, lockdown mode, insiders mode, and feature flags.
1416
func WithRequestConfig(next http.Handler) http.Handler {
1517
return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
18+
// Header-selected features can change the response for the same URL.
19+
w.Header().Add(headers.VaryHeader, headers.MCPFeaturesHeader)
20+
1621
ctx := r.Context()
1722

1823
// Readonly mode
@@ -45,9 +50,13 @@ func WithRequestConfig(next http.Handler) http.Handler {
4550
ctx = ghcontext.WithInsidersMode(ctx, true)
4651
}
4752

48-
// Feature flags
49-
if features := headers.ParseCommaSeparated(r.Header.Get(headers.MCPFeaturesHeader)); len(features) > 0 {
50-
ctx = ghcontext.WithHeaderFeatures(ctx, features)
53+
query := r.URL.Query()
54+
_, hasHeaderFeatures := r.Header[http.CanonicalHeaderKey(headers.MCPFeaturesHeader)]
55+
_, hasQueryFeatures := query[queryParamFeatures]
56+
if hasHeaderFeatures {
57+
ctx = ghcontext.WithHeaderFeatures(ctx, headers.ParseCommaSeparated(r.Header.Get(headers.MCPFeaturesHeader)))
58+
} else if hasQueryFeatures {
59+
ctx = ghcontext.WithHeaderFeatures(ctx, headers.ParseCommaSeparated(query.Get(queryParamFeatures)))
5160
}
5261

5362
next.ServeHTTP(w, r.WithContext(ctx))
Lines changed: 112 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,112 @@
1+
package middleware
2+
3+
import (
4+
"net/http"
5+
"net/http/httptest"
6+
"testing"
7+
8+
ghcontext "github.com/github/github-mcp-server/pkg/context"
9+
"github.com/github/github-mcp-server/pkg/http/headers"
10+
"github.com/stretchr/testify/assert"
11+
)
12+
13+
func TestWithRequestConfigFeatureSelection(t *testing.T) {
14+
tests := []struct {
15+
name string
16+
url string
17+
headerSet bool
18+
headerValue string
19+
wantFeatures []string
20+
wantPresent bool
21+
}{
22+
{
23+
name: "query parameter only",
24+
url: "/?features=mcp_holdback_consolidated_projects",
25+
wantFeatures: []string{"mcp_holdback_consolidated_projects"},
26+
wantPresent: true,
27+
},
28+
{
29+
name: "header only",
30+
url: "/",
31+
headerSet: true,
32+
headerValue: "mcp_holdback_consolidated_projects",
33+
wantFeatures: []string{"mcp_holdback_consolidated_projects"},
34+
wantPresent: true,
35+
},
36+
{
37+
name: "header wins over query parameter, never combined",
38+
url: "/?features=flag_from_query",
39+
headerSet: true,
40+
headerValue: "flag_from_header",
41+
wantFeatures: []string{"flag_from_header"},
42+
wantPresent: true,
43+
},
44+
{
45+
name: "empty header suppresses query parameter",
46+
url: "/?features=flag_from_query",
47+
headerSet: true,
48+
wantFeatures: []string{},
49+
wantPresent: true,
50+
},
51+
{
52+
name: "whitespace-only header suppresses query parameter",
53+
url: "/?features=flag_from_query",
54+
headerSet: true,
55+
headerValue: " , \t ",
56+
wantFeatures: []string{},
57+
wantPresent: true,
58+
},
59+
{
60+
name: "unknown header suppresses query parameter",
61+
url: "/?features=flag_from_query",
62+
headerSet: true,
63+
headerValue: "unknown_from_header",
64+
wantFeatures: []string{"unknown_from_header"},
65+
wantPresent: true,
66+
},
67+
{
68+
name: "empty query value with header",
69+
url: "/?features=",
70+
headerSet: true,
71+
headerValue: "flag_from_header",
72+
wantFeatures: []string{"flag_from_header"},
73+
wantPresent: true,
74+
},
75+
{
76+
name: "empty query value stores an explicit empty selection",
77+
url: "/?features=",
78+
wantFeatures: []string{},
79+
wantPresent: true,
80+
},
81+
{
82+
name: "no channel present stores nothing",
83+
url: "/",
84+
wantFeatures: nil,
85+
},
86+
}
87+
88+
for _, tc := range tests {
89+
t.Run(tc.name, func(t *testing.T) {
90+
var got []string
91+
handler := WithRequestConfig(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
92+
got = ghcontext.GetHeaderFeatures(r.Context())
93+
w.WriteHeader(http.StatusNoContent)
94+
}))
95+
96+
req := httptest.NewRequest(http.MethodPost, tc.url, nil)
97+
if tc.headerSet {
98+
req.Header.Set(headers.MCPFeaturesHeader, tc.headerValue)
99+
}
100+
rec := httptest.NewRecorder()
101+
handler.ServeHTTP(rec, req)
102+
103+
assert.Equal(t, tc.wantFeatures, got)
104+
if tc.wantPresent {
105+
assert.NotNil(t, got)
106+
} else {
107+
assert.Nil(t, got)
108+
}
109+
assert.Contains(t, rec.Header().Values(headers.VaryHeader), headers.MCPFeaturesHeader)
110+
})
111+
}
112+
}

pkg/http/oauth/oauth.go

Lines changed: 15 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -199,7 +199,16 @@ func (h *AuthHandler) buildResourceURL(r *http.Request, resourcePath string) str
199199
if !strings.HasPrefix(resourcePath, "/") {
200200
resourcePath = "/" + resourcePath
201201
}
202-
return baseURL + resourcePath
202+
return appendRawQuery(baseURL+resourcePath, r.URL.RawQuery)
203+
}
204+
205+
// appendRawQuery avoids re-encoding the resource identifier that RFC 9728
206+
// clients compare as an exact string.
207+
func appendRawQuery(target, rawQuery string) string {
208+
if rawQuery == "" {
209+
return target
210+
}
211+
return target + "?" + rawQuery
203212
}
204213

205214
// GetEffectiveHostAndScheme returns the effective host and scheme for a request.
@@ -248,10 +257,13 @@ func BuildResourceMetadataURL(r *http.Request, cfg *Config, resourcePath string)
248257
suffix = resourcePath
249258
}
250259
}
260+
metadataURL := ""
251261
if cfg != nil && cfg.BaseURL != "" {
252-
return strings.TrimSuffix(cfg.BaseURL, "/") + OAuthProtectedResourcePrefix + suffix
262+
metadataURL = strings.TrimSuffix(cfg.BaseURL, "/") + OAuthProtectedResourcePrefix + suffix
263+
} else {
264+
metadataURL = fmt.Sprintf("%s://%s%s%s", scheme, host, OAuthProtectedResourcePrefix, suffix)
253265
}
254-
return fmt.Sprintf("%s://%s%s%s", scheme, host, OAuthProtectedResourcePrefix, suffix)
266+
return appendRawQuery(metadataURL, r.URL.RawQuery)
255267
}
256268

257269
func normalizeBasePath(path string) string {

0 commit comments

Comments
 (0)