Enable feature flags via URL query parameter (?features=) for headerless hosted connections - #3146
Conversation
Sorry, something went wrong.
SamMorrowDrums
left a comment
There was a problem hiding this comment.
The query-string option makes sense for clients that cannot set headers, but I do not think this is release-ready yet because it breaks OAuth protected-resource metadata discovery for query-bearing MCP URLs.
For a server URL such as https://api.githubcopilot.com/mcp/x/issues?features=issue_dependencies, go-sdk v1.7.0 treats the full URL as the resource identifier and validates metadata.resource with exact string equality. Its path-based and root discovery candidates also preserve the query. Today BuildResourceMetadataURL and AuthHandler.buildResourceURL omit r.URL.RawQuery, so the challenge and returned metadata lose ?features=...; standards-compliant clients can reject the metadata as belonging to a different resource.
This should be fixable here: preserve the feature query consistently in both the advertised resource_metadata URL and the metadata document’s resource, then extend TestOAuthChallengeMetadataRouteContracts (or an equivalent client round-trip test) to cover a query-bearing MCP URL and both supported discovery routes. Please also make the header-selection rule below presence-based so routing/query and header features are never combined or used as fallbacks for one another.
Sorry, something went wrong.
| // when it cannot control headers. Unknown flags are dropped later by | ||
| // ResolveFeatureFlags against AllowedFeatureFlags, so this channel is | ||
| // no more privileged than the header — it is the same allowlist. | ||
| features := headers.ParseCommaSeparated(r.URL.Query().Get(queryParamFeatures)) |
There was a problem hiding this comment.
Headers need full precedence here. Please check whether X-MCP-Features is present before reading the query parameter; if it is present, parse only that header and never calculate or fall back to query features. Presence—not parsed length—is important, so an explicitly empty, whitespace-only, or unknown-valued header must still suppress ?features=. Please invert the precedence and add those cases to this test matrix.
Sorry, something went wrong.
Review on github#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.
|
Rebased onto current main (8898db9) and pushed both requested changes as 81d4230: 1. Query preservation in OAuth protected-resource metadata. BuildResourceMetadataURL and AuthHandler.buildResourceURL now append r.URL.RawQuery, via a shared AppendQuery helper, so the advertised resource_metadata URL and the metadata document's resource both carry the exact same query as the MCP server URL — satisfying go-sdk's exact string-equality check for query-bearing URLs like /mcp/x/issues?features=issue_dependencies. 2. Presence-based feature selection. The features query parameter and X-MCP-Features are now separate channels that are never combined or used as fallbacks for one another (except that an empty ?features= value falls back to the header). Query wins when both are present. Tests added:
Verification on this head: go build ./... exit 0, go vet ./pkg/http/... clean, full go test ./pkg/http/... -count=1 green (handler/middleware/oauth/transport all pass), gofmt clean. Docs updated in docs/feature-flags.md and docs/server-configuration.md. |
Sorry, something went wrong.
Reviewers
Requested changes must be addressed to merge this pull request.
Closes #3145
Problem
Feature flags can only be enabled per request through the X-MCP-Features header (or statically via --features / GITHUB_FEATURES). Clients that compose the server URL on the user's behalf — hosted IDEs, agent platforms, harnesses that provision the MCP connection — cannot set custom headers, so every flagged tool is unreachable on those connections.
Solution
Accept a features URL query parameter as an additional channel:
The gate itself stays intact — flagged tools remain out of the default surface, so fixed tool-schema cost doesn't change; this only widens how a user can opt in.
Changes
Testing