feat(mcp): support custom HTTP headers for native agents - #1510
feat(mcp): support custom HTTP headers for native agents#1510fisherivco wants to merge 1 commit into
Conversation
|
/review |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Important
CHANGES REQUESTED
Consolidated review: #1510 (comment)
| None => { | ||
| let transport = StreamableHttpClientTransport::from_uri(url.as_str()); | ||
| let cfg = StreamableHttpClientTransportConfig::with_uri(url.as_str()) | ||
| .custom_headers(headers); |
There was a problem hiding this comment.
🔴 F1 - Prevent credential disclosure through redirects
This config is passed to rmcp 1.7/1.8, whose default reqwest client follows redirects. Reqwest strips standard auth/cookie headers across origins but not arbitrary credentials such as X-API-Key, so a redirect can replay this custom secret to another origin. The OAuth client constructed above has the same default redirect policy.
Requested change: use an MCP reqwest client with redirects disabled (or a rigorously tested same-origin-only policy) for both anonymous and OAuth paths, and add a two-server regression test proving the redirect target never receives the custom header.
| ServerConfig::Http { url, .. } => DialPlan::Dial(Dial::Http { url, client: None }), | ||
| } => DialPlan::OauthHttp { | ||
| url, | ||
| headers: parse_http_headers(name, headers, true)?, |
There was a problem hiding this comment.
🟡 F2 - Record deterministic header configuration failures in status
This ? returns before handle.status becomes Connecting and before the common dial-failure branch. Invalid header configuration can therefore leave mcp status reporting Disconnected rather than Failed.
Requested change: route these redacted configuration errors through ServerStatus::Failed without charging the transport circuit breaker, and assert status for each rejected-header test.
|
Important CHANGES REQUESTED What This PR DoesThis PR adds per-server custom HTTP headers to native Streamable HTTP MCP configuration. Header values use the existing How It Works
Findings
Finding Details🔴 F1: Prevent credential disclosure through redirectsThe workspace locks rmcp 1.8.0 and the standalone agent locks rmcp 1.7.0. Their default reqwest clients follow redirects. Reqwest removes a fixed set of standard sensitive headers when an origin changes, but it does not remove arbitrary credentials such as Requested change: disable redirects for credential-bearing MCP clients, or implement and test a strict same-origin policy that never forwards custom credentials across origins. Add a two-server regression test for anonymous and OAuth-backed client construction. 🟡 F2: Record deterministic configuration failures
Requested change: record a redacted 🟡 F3: Reject transport-owned headers locallyThe local validator only handles generic syntax, duplicates, and OAuth plus Requested change: align local validation with the pinned rmcp reserved-header contract, preserve server and header context, and add regression tests. Avoid a drifting hand-maintained denylist if upstream exposes a reusable validator. 🟡 F4: Validate literal header configuration before connection
Requested change: share header validation between boot/config validation and resolved connect-time validation. Literal names and values should fail early; unresolved value placeholders may be deferred until resolution. 🟡 F5: Document case-insensitive header namesJSON keys are case-sensitive, but HTTP header names are not. A user can write both Requested change: document case-insensitive names and case-variant duplicate rejection, and clarify that interpolation applies to values rather than header names. 🟡 F6: Restore required CIThe required Requested change: fix the lint failure and rerun required CI successfully. Inline Thread Disposition
Addressing All Review Feedback
Validation
What's Good (🟢)
5. Three Reasons We Might Not Need This PR
|
There was a problem hiding this comment.
Important
CHANGES REQUESTED
Consolidated review: #1510 (comment)
|
Let me take over. |
What problem does this solve?
Native
openab-agentbackends can discover remote Streamable HTTP MCP servers, but they cannot currently attach the custom HTTP headers required by authenticated servers. This forces native agents to use a separate credential-bearing CLI path even when the same MCP server is already available to other OpenAB agent backends.This change lets native agents declare secret-resolved, per-server HTTP headers in
mcp.jsonand sends them through the existingrmcptransport.Discord Discussion URL: https://discord.com/channels/1491295327620169908/1540933586973233264
Review Contract
Goal
Allow native
openab-agentbackends to connect to authenticated remote Streamable HTTP MCP servers by configuring per-server custom headers whose values can use OpenAB's existing${env:VAR}resolution.Non-goals
Accepted Residual Risks
StreamableHttpClientTransportConfig::custom_headersfield but does not add a second end-to-end OAuth-plus-custom-header fixture in this PR. Existing OAuth tests and the full workspace suite remain green; a dedicated OAuth wire fixture is listed as a follow-up.Authorizationheader combined withoauthis rejected instead of defining ambiguous precedence. Users can choose either OAuth or a customAuthorizationheader for that server.Acceptance Criteria
headerscontinue to deserialize unchanged.${env:VAR}resolution and fail closed when a referenced variable is absent.Authorizationconfiguration is rejected.Follow-ups
At a Glance
Prior Art & Industry Research
OpenClaw:
OpenClaw accepts per-server HTTP headers, resolves environment-backed values before connection, and attaches the resulting map to its remote MCP transport:
Hermes Agent:
Hermes Agent exposes a generic
headersmap for HTTP MCP servers and supports environment substitution in its MCP configuration:Other references (optional):
rmcpStreamable HTTP client transport already provides thecustom_headerstransport field used here.Proposed Solution
headersmap toServerConfig::Http.http::HeaderNameandhttp::HeaderValue, reject normalized duplicates, and mark values sensitive for debug output.Authorizationwhen OAuth is configured so authentication precedence is explicit.rmcp'scustom_headersfield for both anonymous and OAuth Streamable HTTP transports.Example:
{ "mcpServers": { "remote": { "type": "http", "url": "https://mcp.example.com/mcp", "headers": { "X-API-Key": "${env:REMOTE_MCP_API_KEY}" } } } }Why this approach?
The underlying
rmcpclient already supports custom headers, and OpenAB already owns environment interpolation and MCP configuration loading. Connecting those existing capabilities keeps authentication policy at the remote server, preserves the current native-agent architecture, and avoids introducing a second control plane.Typed validation makes malformed configuration fail before network activity. The OAuth conflict rule avoids silently sending two competing authorization mechanisms.
Alternatives Considered
rmcp.auth_headerfield: does not cover authenticated servers that use non-Authorizationheaders or require more than one header.Validation
cargo checkpasses — pending required upstream CI on the submitted commit.cargo test -p openab-mcppasses: 226 passed, 0 failed.cargo test --workspace -- --test-threads=1passes.cargo clippyclean — pending required upstream CI on the submitted commit.cargo testpasses with the nativeopenab-agentlock (rmcp1.7.0), including 62 default and 11 ignored tests.cargo build --releasepasses.X-API-Keyand accepted the native client configured with the environment-resolved header.git diff --checkpasses.Independent review completed with PASS on the exact submitted diff:
at3-20260826-openab-native-http-headers-chi-show5d89f9e133e25c27f7cea761e4758006e1717b29506bf94c2e076b83b8def87af91a0e85de7f259a960ce55eacf51c008aec298e