-
Notifications
You must be signed in to change notification settings - Fork 200
feat(mcp): 8245 header forwarding #2305
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
- Modified MCP server to capture and forward all HTTP headers to router - Router's existing header propagation rules apply transparently - Added test to validate header forwarding behavior
WalkthroughReplaces auth-centric MCP context with request-header-centric APIs, forwards all incoming HTTP headers to downstream GraphQL requests, adds Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related issues
Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Router image scan passed✅ No security vulnerabilities found in image: |
7faa9f5 to
1f58783
Compare
Add test proving custom headers like "foo: bar" are forwarded through MCP server to GraphQL subgraphs using stateless mode and direct HTTP requests.
1f58783 to
eec2900
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
router-tests/mcp_test.go (1)
572-573: Consider removing mutex for single-request test.The
sync.MutexprotectscapturedSubgraphRequest, but this test only makes a single HTTP request that will be processed serially. The GlobalMiddleware will be invoked once during that request, so concurrent access is unlikely. The mutex adds complexity without clear benefit.If you want to keep the mutex for defensive coding or future-proofing, this is fine. Otherwise, consider simplifying:
-var capturedSubgraphRequest *http.Request -var subgraphMutex sync.Mutex +var capturedSubgraphRequest *http.Request ... GlobalMiddleware: func(handler http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - subgraphMutex.Lock() capturedSubgraphRequest = r.Clone(r.Context()) - subgraphMutex.Unlock() handler.ServeHTTP(w, r) }) },And remove the mutex lock/unlock at lines 647-648.
Also applies to: 596-600
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
router-tests/mcp_test.go(2 hunks)router/pkg/mcpserver/server.go(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
router/pkg/mcpserver/server.go (1)
router/internal/expr/expr.go (2)
Context(35-39)Request(66-75)
router-tests/mcp_test.go (3)
router-tests/testenv/testenv.go (4)
Run(105-122)Config(284-341)SubgraphsConfig(366-379)Environment(1729-1765)router/pkg/config/config.go (7)
Config(1000-1074)MCPConfiguration(965-975)MCPSessionConfig(977-979)HeaderRules(253-258)GlobalHeaderRule(260-264)RequestHeaderRule(278-301)HeaderRuleOperationPropagate(269-269)router/core/router.go (2)
Option(172-172)WithHeaderRules(1720-1724)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: build-router
- GitHub Check: build_push_image
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: build_push_image (nonroot)
- GitHub Check: build_test
- GitHub Check: image_scan
- GitHub Check: Analyze (go)
🔇 Additional comments (4)
router-tests/mcp_test.go (1)
558-674: Excellent test coverage for end-to-end header propagation.This test effectively validates that headers flow through the complete chain (MCP Client → MCP Server → Router → Subgraphs). The comprehensive comments explaining why direct HTTP requests are used instead of the mcp-go client library are particularly valuable for future maintainers.
The test verifies multiple header types (custom, tracing, auth) and correctly uses stateless mode to avoid session complexity.
router/pkg/mcpserver/server.go (3)
25-47: Well-structured refactor from auth-only to all-headers context.The replacement of auth-centric context functions with request-header-centric APIs is clean and follows Go context best practices. Key improvements:
r.Header.Clone()at line 36 prevents mutation issues- Error message updated from "missing auth" to "missing request headers" appropriately reflects the new semantics
- Functions maintain consistent naming and patterns
227-230: LGTM - useful test helper.The public
SetHTTPClientmethod enables injecting custom HTTP clients for testing, which aligns with the test's use ofGlobalMiddlewareto capture subgraph requests.
681-697: Header forwarding implementation is correct.The approach of forwarding all headers from the MCP request context and then overriding GraphQL-specific headers (
Accept,Content-Type) is sound. Key aspects:
- Line 683: Headers retrieved from context
- Lines 688-692: All headers are forwarded, as intended per PR objectives
- Lines 696-697: GraphQL-required headers are enforced after copying
- Line 685: Debug-level logging for missing headers is appropriate
The security model correctly relies on the router's header rules (configured via
config.HeaderRules) to filter what ultimately reaches subgraphs, as validated by the test at lines 584-593 inmcp_test.go.
StarpTech
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Adds header forwarding from MCP client through MCP server to the GraphQL router, enabling authentication tokens, tracing headers, and custom metadata to be propagated through the request chain.
Summary by CodeRabbit
New Features
Tests