Skip to content

Commit c5e3f6f

Browse files
CAOShurongCAOShurong
committed
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.
1 parent febc329 commit c5e3f6f

7 files changed

Lines changed: 186 additions & 7 deletions

File tree

‎docs/feature-flags.md‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,9 +13,15 @@ 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 query parameter
23+
wins — the two channels are never combined.
24+
1925
Only flags listed in
2026
[`AllowedFeatureFlags`](../pkg/github/feature_flags.go) can be enabled by
2127
end users. Insiders-only flags are not user-toggleable.

‎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/http/middleware/request_config.go‎

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

12+
// queryParamFeatures is the URL query parameter that carries feature flags,
13+
// mirroring the X-MCP-Features header. It exists so clients that cannot set
14+
// custom headers on the MCP connection — hosted IDEs, agent platforms, or
15+
// harnesses that compose the server URL on the user's behalf (see #3145) —
16+
// can still opt into flagged tools.
17+
const queryParamFeatures = "features"
18+
1219
// WithRequestConfig is a middleware that extracts MCP-related headers and sets them in the request context.
1320
// This includes readonly mode, toolsets, tools, lockdown mode, insiders mode, and feature flags.
21+
// Feature flags may also arrive via the `features` URL query parameter; when
22+
// both are present the query parameter wins, matching how the toolset path
23+
// segments take precedence over their headers.
1424
func WithRequestConfig(next http.Handler) http.Handler {
1525
return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
1626
ctx := r.Context()
@@ -45,9 +55,20 @@ func WithRequestConfig(next http.Handler) http.Handler {
4555
ctx = ghcontext.WithInsidersMode(ctx, true)
4656
}
4757

48-
// Feature flags
49-
if features := headers.ParseCommaSeparated(r.Header.Get(headers.MCPFeaturesHeader)); len(features) > 0 {
50-
ctx = ghcontext.WithHeaderFeatures(ctx, features)
58+
// Feature flags: presence-based selection. The URL query parameter and
59+
// the X-MCP-Features header are separate channels — whichever is
60+
// present is used as-is, and they are never combined. When both are
61+
// present the query parameter wins, so a client composing the server
62+
// URL can always express its intent even when it cannot control
63+
// headers. Unknown flags are dropped later by ResolveFeatureFlags
64+
// against AllowedFeatureFlags, so neither channel is privileged.
65+
queryFeatures, hasQuery := r.URL.Query()[queryParamFeatures]
66+
headerFeatures := r.Header.Get(headers.MCPFeaturesHeader)
67+
switch {
68+
case hasQuery && strings.TrimSpace(queryFeatures[0]) != "":
69+
ctx = ghcontext.WithHeaderFeatures(ctx, headers.ParseCommaSeparated(queryFeatures[0]))
70+
case headerFeatures != "":
71+
ctx = ghcontext.WithHeaderFeatures(ctx, headers.ParseCommaSeparated(headerFeatures))
5172
}
5273

5374
next.ServeHTTP(w, r.WithContext(ctx))

‎pkg/http/middleware/request_config_test.go‎

Lines changed: 77 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,77 @@
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+
)
11+
12+
func TestWithRequestConfigFeatureSelection(t *testing.T) {
13+
tests := []struct {
14+
name string
15+
url string
16+
headerValue string
17+
wantFeatures []string
18+
}{
19+
{
20+
name: "query parameter only",
21+
url: "/?features=mcp_holdback_consolidated_projects",
22+
wantFeatures: []string{"mcp_holdback_consolidated_projects"},
23+
},
24+
{
25+
name: "header only",
26+
url: "/",
27+
headerValue: "mcp_holdback_consolidated_projects",
28+
wantFeatures: []string{"mcp_holdback_consolidated_projects"},
29+
},
30+
{
31+
name: "query parameter wins over header, never combined",
32+
url: "/?features=flag_from_query",
33+
headerValue: "flag_from_header",
34+
wantFeatures: []string{"flag_from_query"},
35+
},
36+
{
37+
name: "empty query value falls back to header",
38+
url: "/?features=",
39+
headerValue: "flag_from_header",
40+
wantFeatures: []string{"flag_from_header"},
41+
},
42+
{
43+
name: "no channel present stores nothing",
44+
url: "/",
45+
wantFeatures: nil,
46+
},
47+
}
48+
49+
for _, tc := range tests {
50+
t.Run(tc.name, func(t *testing.T) {
51+
var got []string
52+
handler := WithRequestConfig(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
53+
got = ghcontext.GetHeaderFeatures(r.Context())
54+
w.WriteHeader(http.StatusNoContent)
55+
}))
56+
57+
req := httptest.NewRequest(http.MethodPost, tc.url, nil)
58+
if tc.headerValue != "" {
59+
req.Header.Set(headers.MCPFeaturesHeader, tc.headerValue)
60+
}
61+
rec := httptest.NewRecorder()
62+
handler.ServeHTTP(rec, req)
63+
64+
if tc.wantFeatures == nil && len(got) == 0 {
65+
return
66+
}
67+
if len(got) != len(tc.wantFeatures) {
68+
t.Fatalf("got features %v, want %v", got, tc.wantFeatures)
69+
}
70+
for i := range tc.wantFeatures {
71+
if got[i] != tc.wantFeatures[i] {
72+
t.Fatalf("got features %v, want %v", got, tc.wantFeatures)
73+
}
74+
}
75+
})
76+
}
77+
}

‎pkg/http/oauth/oauth.go‎

Lines changed: 22 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -187,6 +187,10 @@ func ResolveResourcePath(r *http.Request, cfg *Config) string {
187187
}
188188

189189
// buildResourceURL constructs the full resource URL for OAuth metadata.
190+
// The request's query string is preserved: MCP clients that receive a server
191+
// URL such as /mcp/x/issues?features=... identify the protected resource by
192+
// exact string match against metadata.resource (RFC 9728), so dropping the
193+
// query would make standards-compliant clients reject the metadata.
190194
func (h *AuthHandler) buildResourceURL(r *http.Request, resourcePath string) string {
191195
host, scheme := GetEffectiveHostAndScheme(r, h.cfg)
192196
baseURL := fmt.Sprintf("%s://%s", scheme, host)
@@ -199,7 +203,17 @@ func (h *AuthHandler) buildResourceURL(r *http.Request, resourcePath string) str
199203
if !strings.HasPrefix(resourcePath, "/") {
200204
resourcePath = "/" + resourcePath
201205
}
202-
return baseURL + resourcePath
206+
return AppendQuery(baseURL+resourcePath, r.URL.RawQuery)
207+
}
208+
209+
// AppendQuery appends rawQuery to target when non-empty. It is shared by the
210+
// resource URL and the advertised metadata URL so both consistently carry the
211+
// same query string as the MCP server URL the client connects to.
212+
func AppendQuery(target, rawQuery string) string {
213+
if rawQuery == "" {
214+
return target
215+
}
216+
return target + "?" + rawQuery
203217
}
204218

205219
// GetEffectiveHostAndScheme returns the effective host and scheme for a request.
@@ -248,10 +262,15 @@ func BuildResourceMetadataURL(r *http.Request, cfg *Config, resourcePath string)
248262
suffix = resourcePath
249263
}
250264
}
265+
metadataURL := ""
251266
if cfg != nil && cfg.BaseURL != "" {
252-
return strings.TrimSuffix(cfg.BaseURL, "/") + OAuthProtectedResourcePrefix + suffix
267+
metadataURL = strings.TrimSuffix(cfg.BaseURL, "/") + OAuthProtectedResourcePrefix + suffix
268+
} else {
269+
metadataURL = fmt.Sprintf("%s://%s%s%s", scheme, host, OAuthProtectedResourcePrefix, suffix)
253270
}
254-
return fmt.Sprintf("%s://%s%s%s", scheme, host, OAuthProtectedResourcePrefix, suffix)
271+
// Preserve the request query so the advertised metadata endpoint matches
272+
// the full resource identifier, including feature-flag query parameters.
273+
return AppendQuery(metadataURL, r.URL.RawQuery)
255274
}
256275

257276
func normalizeBasePath(path string) string {

‎pkg/http/oauth/oauth_test.go‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -362,6 +362,28 @@ func TestBuildResourceMetadataURL(t *testing.T) {
362362
resourcePath: "",
363363
expectedURL: "http://api.example.com/.well-known/oauth-protected-resource",
364364
},
365+
{
366+
name: "query string is preserved on base URL config",
367+
cfg: &Config{
368+
BaseURL: "https://custom.example.com",
369+
},
370+
setupRequest: func() *http.Request {
371+
return httptest.NewRequest(http.MethodGet, "/mcp/x/issues?features=issue_dependencies", nil)
372+
},
373+
resourcePath: "/mcp/x/issues",
374+
expectedURL: "https://custom.example.com/.well-known/oauth-protected-resource/mcp/x/issues?features=issue_dependencies",
375+
},
376+
{
377+
name: "query string is preserved without base URL config",
378+
cfg: &Config{},
379+
setupRequest: func() *http.Request {
380+
req := httptest.NewRequest(http.MethodGet, "/mcp?features=a,b", nil)
381+
req.Host = "api.example.com"
382+
return req
383+
},
384+
resourcePath: "/mcp",
385+
expectedURL: "http://api.example.com/.well-known/oauth-protected-resource/mcp?features=a,b",
386+
},
365387
}
366388

367389
for _, tc := range tests {

‎pkg/http/server_test.go‎

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -259,6 +259,40 @@ func TestOAuthChallengeMetadataRouteContracts(t *testing.T) {
259259
})
260260
}
261261

262+
// Query-bearing MCP server URLs must round-trip: the challenge's
263+
// resource_metadata URL and the served metadata document's "resource"
264+
// must both carry the exact same query as the URL the client connects to,
265+
// because go-sdk validates metadata.resource with exact string equality.
266+
queryPath := "/x/repos?features=issue_dependencies"
267+
t.Run(queryPath, func(t *testing.T) {
268+
req := httptest.NewRequest(http.MethodPost, queryPath, nil)
269+
req.Header.Set("Origin", "https://confer.to")
270+
rec := httptest.NewRecorder()
271+
router.ServeHTTP(rec, req)
272+
273+
require.Equal(t, http.StatusUnauthorized, rec.Code)
274+
challenge := rec.Header().Get("WWW-Authenticate")
275+
require.True(t, strings.HasPrefix(challenge, `Bearer resource_metadata="`))
276+
metadataURL := strings.TrimSuffix(
277+
strings.TrimPrefix(challenge, `Bearer resource_metadata="`),
278+
`"`,
279+
)
280+
assert.Equal(t,
281+
baseURL+"/.well-known/oauth-protected-resource/mcp/x/repos?features=issue_dependencies",
282+
metadataURL,
283+
)
284+
285+
req = httptest.NewRequest(http.MethodGet, strings.TrimPrefix(metadataURL, baseURL), nil)
286+
req.Header.Set("Origin", "https://confer.to")
287+
rec = httptest.NewRecorder()
288+
router.ServeHTTP(rec, req)
289+
290+
require.Equal(t, http.StatusOK, rec.Code)
291+
var metadata map[string]any
292+
require.NoError(t, json.Unmarshal(rec.Body.Bytes(), &metadata))
293+
assert.Equal(t, baseURL+"/mcp"+queryPath, metadata["resource"])
294+
})
295+
262296
req := httptest.NewRequest(
263297
http.MethodGet,
264298
oauth.OAuthProtectedResourcePrefix+"/mcp/unknown",

0 commit comments

Comments
 (0)