extra header forwarding for mcp tools - #4572
Conversation
|
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughSummary by CodeRabbit
WalkthroughExtra MCP headers are shifted from per-call ChangesMCP Extra Headers via Transport Injection
Sequence Diagram(s)sequenceDiagram
participant Plugin as extraHeaderInjectPlugin
participant Pipeline as pluginpipeline
participant Transport as clientmanager (HeaderFunc)
participant ExtractFilteredExtras as utils.ExtractFilteredExtras
participant UpstreamMCP as Upstream MCP Server
Plugin->>Pipeline: PreMCPHook sets MCPExtraHeaders in gateCtx
Pipeline->>Transport: wire call with gateCtx (ping / tools/list / tools/call)
Transport->>ExtractFilteredExtras: gateCtx + AllowedExtraHeaders
ExtractFilteredExtras-->>Transport: filtered http.Header (allowlisted only)
Transport->>UpstreamMCP: HTTP request with allowlisted header injected
UpstreamMCP-->>Transport: response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Comment |
This stack of pull requests is managed by Graphite. Learn more about stacking. |
5f40c10 to
411f221
Compare
Merge activity
|
## Summary Per-request extra headers set via `BifrostContextKeyMCPExtraHeaders` in a `PreMCPHook` were not reaching the upstream MCP server for health-check probes (`ping` and `tools/list`). The `mcp-go` client drops `request.Header` for these internally-generated calls, so headers injected at the `CallToolRequest` level were silently lost. This PR centralizes all per-request extra header injection onto the transport layer via `WithHTTPHeaderFunc` / `WithHeaderFunc`, ensuring headers flow on every outgoing message — including `ping`, `tools/list`, and `tools/call` — while still being filtered by `AllowedExtraHeaders`. ## Changes - Registered a `headerFunc` on the `StreamableHTTP` and `SSE` transports (in `createHTTPConnection`, `createSSEConnection`, and `AcquireClientConn`) that reads `BifrostContextKeyMCPExtraHeaders` from the request context and injects only allowlisted headers per `MCPClientConfig.AllowedExtraHeaders`. This replaces the previous per-call `CallToolRequest.Header` approach. - Removed `credStore.RequestHeaders` calls and `CallToolRequest.Header` assignments from `executeToolInternal` (tool manager) and `callMCPTool` (Starlark code mode), since header injection is now handled uniformly by the transport. - Fixed `runListToolsWithHooks` and `runPingWithHooks` to pass `gateCtx` (the child context that carries `PreMCPHook` writes) instead of the outer `ctx` to the wire calls, so transport `headerFunc` can see values written during the plugin gate. - Relaxed `ExtractFilteredExtras` to accept a plain `context.Context` instead of `*schemas.BifrostContext`, enabling it to be called from the transport `headerFunc` closure. - Added end-to-end wire-level tests (`extraheaders_test.go`) using a real `httptest` streamable-HTTP server that records inbound headers per JSON-RPC method, covering: allowlisted headers reaching `ping`, `tools/list`, and `tools/call`; and non-allowlisted headers being filtered on all requests. ## Type of change - [x] Bug fix - [ ] Feature - [x] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [x] Core (Go) - [x] Transports (HTTP) - [ ] Providers/Integrations - [x] Plugins - [ ] UI (React) - [ ] Docs ## How to test ```sh go test ./core/internal/mcptests/... -run TestExtraHeaders -v go test ./... ``` The three new tests validate: - `TestExtraHeadersHealthCheckPingReachWire` — allowlisted header appears on `ping` probes; non-allowlisted header never appears on any request. - `TestExtraHeadersHealthCheckListToolsReachWire` — same guarantee for `tools/list` health-check probes when ping is unavailable. - `TestExtraHeadersToolCallReachWire` — allowlisted header appears on a normal `tools/call`; non-allowlisted header is filtered. ## Breaking changes - [x] No The `CallToolRequest.Header` field is no longer populated by Bifrost internals, but this is an internal implementation detail with no public API impact. Header forwarding behavior is preserved (and extended to health-check probes). ## Security considerations `AllowedExtraHeaders` filtering is now enforced at the transport layer for all outgoing MCP requests. Non-allowlisted headers set by plugins are dropped before reaching the wire on every request type, including health-check probes that previously bypassed the per-call header path entirely. ## Checklist - [ ] I read `docs/contributing/README.md` and followed the guidelines - [x] I added/updated tests where appropriate - [ ] I updated documentation where needed - [x] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable

Summary
Per-request extra headers set via
BifrostContextKeyMCPExtraHeadersin aPreMCPHookwere not reaching the upstream MCP server for health-check probes (pingandtools/list). Themcp-goclient dropsrequest.Headerfor these internally-generated calls, so headers injected at theCallToolRequestlevel were silently lost. This PR centralizes all per-request extra header injection onto the transport layer viaWithHTTPHeaderFunc/WithHeaderFunc, ensuring headers flow on every outgoing message — includingping,tools/list, andtools/call— while still being filtered byAllowedExtraHeaders.Changes
headerFuncon theStreamableHTTPandSSEtransports (increateHTTPConnection,createSSEConnection, andAcquireClientConn) that readsBifrostContextKeyMCPExtraHeadersfrom the request context and injects only allowlisted headers perMCPClientConfig.AllowedExtraHeaders. This replaces the previous per-callCallToolRequest.Headerapproach.credStore.RequestHeaderscalls andCallToolRequest.Headerassignments fromexecuteToolInternal(tool manager) andcallMCPTool(Starlark code mode), since header injection is now handled uniformly by the transport.runListToolsWithHooksandrunPingWithHooksto passgateCtx(the child context that carriesPreMCPHookwrites) instead of the outerctxto the wire calls, so transportheaderFunccan see values written during the plugin gate.ExtractFilteredExtrasto accept a plaincontext.Contextinstead of*schemas.BifrostContext, enabling it to be called from the transportheaderFuncclosure.extraheaders_test.go) using a realhttpteststreamable-HTTP server that records inbound headers per JSON-RPC method, covering: allowlisted headers reachingping,tools/list, andtools/call; and non-allowlisted headers being filtered on all requests.Type of change
Affected areas
How to test
The three new tests validate:
TestExtraHeadersHealthCheckPingReachWire— allowlisted header appears onpingprobes; non-allowlisted header never appears on any request.TestExtraHeadersHealthCheckListToolsReachWire— same guarantee fortools/listhealth-check probes when ping is unavailable.TestExtraHeadersToolCallReachWire— allowlisted header appears on a normaltools/call; non-allowlisted header is filtered.Breaking changes
The
CallToolRequest.Headerfield is no longer populated by Bifrost internals, but this is an internal implementation detail with no public API impact. Header forwarding behavior is preserved (and extended to health-check probes).Security considerations
AllowedExtraHeadersfiltering is now enforced at the transport layer for all outgoing MCP requests. Non-allowlisted headers set by plugins are dropped before reaching the wire on every request type, including health-check probes that previously bypassed the per-call header path entirely.Checklist
docs/contributing/README.mdand followed the guidelines