fix(http): allow projected MCP headers in preflights - #3167
fix(http): allow projected MCP headers in preflights#3167SamMorrowDrums wants to merge 2 commits into
Conversation
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Auto-generated by license-check workflow
There was a problem hiding this comment.
Pull request overview
Extends browser CORS preflight support for MCP routing and projected parameter headers.
Changes:
- Adds fixed MCP method, name, owner, and repo headers.
- Validates, deduplicates, sorts, and reflects future
Mcp-Param-*headers. - Expands middleware and router-level CORS tests.
Show a summary per file
| File | Description |
|---|---|
pkg/http/server_test.go |
Tests projected headers through the full router. |
pkg/http/middleware/cors.go |
Implements projected-header preflight handling. |
pkg/http/middleware/cors_test.go |
Covers validation, ordering, deduplication, and preserved behavior. |
pkg/http/headers/headers.go |
Defines MCP projection header constants. |
go.mod |
Promotes x/net to a direct dependency. |
Review details
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 8/9 changed files
- Comments generated: 0
- Review effort level: Balanced
| for _, value := range requestHeaders.Values("Access-Control-Request-Headers") { | ||
| for header := range strings.SplitSeq(value, ",") { | ||
| header = strings.TrimSpace(header) | ||
| if !httpguts.ValidHeaderFieldName(header) { | ||
| continue | ||
| } | ||
|
|
||
| key := strings.ToLower(header) | ||
| if !strings.HasPrefix(key, prefix) || len(key) == len(prefix) { | ||
| continue | ||
| } | ||
| if _, ok := seen[key]; ok { | ||
| continue | ||
| } | ||
|
|
||
| seen[key] = struct{}{} | ||
| projected = append(projected, key) | ||
| } | ||
| } |
There was a problem hiding this comment.
There's no cap on how many projected names we reflect here. Within Go's 1MB request header limit, a client can send thousands of distinct Mcp-Param-x names on a preflight and get every one of them echoed back, growing both seen and the response header. That's a cheap request producing a large response — a small amplification vector.
Could we bound this? Stopping after something like 32–64 projected names would close it off at no practical cost, since real clients will only ever send a handful.
Summary
Mcp-MethodandMcp-Namerequest headers plus the currentMcp-Param-ownerandMcp-Param-repoprojectionsAccess-Control-Request-Headersvalue onOPTIONSand reflect only valid, non-emptyMcp-Param-*field namesOPTIONSbehaviorExact projected-header contract
CORS has no prefix wildcard, so preflights validate each requested name with
httpguts.ValidHeaderFieldName. Matching is case-insensitive against the exactMcp-Param-prefix and requires a non-empty suffix. Fixed owner/repo entries remain first; future names are case-insensitively deduplicated, sorted, and emitted withhttp.CanonicalHeaderKey. Arbitrary headers, bareMcp-Param-, malformed/control-character names, and lookalike prefixes are omitted.Mcp-Param-*headers are request-only and are not added toAccess-Control-Expose-Headers;Mcp-Session-IdandWWW-Authenticateremain exposed.Cross-repository dependencies
github/github-mcp-server-remote#955must consume this commit or a release containing it for direct remote traffic; its stable dependency blocker remains until this change is merged and released.github/copilot-api#37229separately owns matching hosted-edge preflight behavior.Validation
script/lintscript/test