[Z2O-1694] fix(mcp): resolve ${context:} headers caller-side, not on the MCP loop - #2
Conversation
…the MCP loop
${context:NAME} templated MCP headers (delegated principal, signed
assertions) were resolved inside the httpx request event hook — which
runs on the dedicated MCP background loop, in a task spawned by the
SDK's post_writer. That task's contextvar context is frozen at
server-connect time and never sees the per-turn session vars set on the
caller's thread, so resolution always returned "" and the header was
dropped. Net effect: delegated-principal propagation silently no-ops for
every streamable-HTTP MCP server (the default for remote servers).
Diagnosed end-to-end from Mercator: the plugin resolved the principal
(logs: status=resolved @verdigris.co) but Meridian saw email:null; a
direct curl to the gateway with vs without the header confirmed the
gateway honors it and Hermes was sending it empty.
Fix: resolve the templates on the CALLER's thread (the sync tool
handler, where the session vars are live), bridge the resolved values
onto the server instance, and have the request hook apply them. The
apply happens inside _rpc_lock so concurrent per-server calls can't
race, and is cleared after each call so non-tool requests (pings,
reconnect GETs) never carry a stale principal.
- _resolve_context_templates stays caller-side via the new
MCPServerTask._resolve_templated_headers().
- _apply_resolved_headers (module helper) stamps pre-resolved values,
preserving the cross-origin identity-header strip.
- The request hook reads self._outbound_resolved_headers instead of
resolving against its own (wrong) context.
SSE / legacy-HTTP transports already freeze templated headers at
connect (documented limitation) and are unchanged.
Tests: caller-side resolution, loop-side application, cross-origin
strip, and the crux regression — a value resolved while the var is set
still injects after that context is gone (simulating the MCP loop).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe PR moves ${context:NAME} MCP header template resolution from the MCP loop into the caller thread, stores templates at connect time, resolves per-call values before tool invocation, bridges them into MCPServerTask._outbound_resolved_headers for the duration of each RPC, and applies them in the httpx request hook with cross-origin identity stripping. ChangesCaller-thread header resolution with loop-side application
Sequence DiagramsequenceDiagram
actor Caller
participant MCPServerTask
participant httpxRequestHook
participant Remote
Caller->>MCPServerTask: _resolve_templated_headers()
MCPServerTask->>MCPServerTask: set _outbound_resolved_headers (under _rpc_lock)
Caller->>MCPServerTask: enter _rpc(...) context with bridged headers
httpxRequestHook->>MCPServerTask: read _outbound_resolved_headers
httpxRequestHook->>httpxRequestHook: _apply_resolved_headers(request, same_origin?, names, resolved)
httpxRequestHook->>Remote: send outbound HTTP request with applied headers
MCPServerTask->>MCPServerTask: clear _outbound_resolved_headers in finally
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
🔎 Lint report:
|
There was a problem hiding this comment.
Code Review
This pull request addresses issue Z2O-1694 by resolving templated headers on the caller's thread where the session context is active, and bridging these values to the MCP background loop via the MCPServerTask._outbound_resolved_headers instance attribute under _rpc_lock. Feedback on these changes highlights a potential security risk where background keepalive pings could leak stashed user headers, and notes that other handlers (such as resource and prompt handlers) currently omit this header resolution. Additionally, a minor code simplification was suggested to use pop instead of del when removing empty headers in _apply_resolved_headers.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
…alive principal leak Gemini review on PR #2 flagged two real issues: 1. (high) Other MCP handlers (list_resources, read_resource, list_prompts, get_prompt) also issue outbound HTTP but didn't resolve/stash templated headers — delegated principal silently dropped for resource/prompt ops. 2. (high) Background keepalive ping (list_tools in _wait_for_lifecycle_event) doesn't take _rpc_lock, so it could overlap a user tool call and pick up the stashed _outbound_resolved_headers — leaking the user's principal on a system keepalive. Fix: - Extract `MCPServerTask._rpc(resolved_headers=None)` async context manager: acquires _rpc_lock, exposes the caller-resolved headers for the call's duration, clears them after. System RPCs pass no headers and still hold the lock, so they can't overlap-and-read a user call's principal. - Use it in all five user-facing handlers (tool + 4 resource/prompt) with caller-side _resolve_templated_headers(), and in the keepalive ping (empty headers) — closing the leak. - (medium) Simplify _apply_resolved_headers drop path to headers.pop(name, None). Header partitioning (static vs templated, Gemini ref #3) was already in place via _split_static_and_templated_headers — only templated names are resolved. Tests: + _rpc scoping/clear contract (system RPC carries no headers). 94 passed across context-template + mcp + session-env suites. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…eader resolution test_mcp_structured_content fakes `server` as a SimpleNamespace; the tool handler now calls server._resolve_templated_headers() (caller-side) and server._rpc() under the lock, which the bare fake lacked → AttributeError. Give the fake an empty resolver and bind the real _rpc CM (it only needs _rpc_lock + _outbound_resolved_headers, both present). Pure test fixture fix; no production change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Why
${context:NAME}templated MCP headers — the mechanism delegated-principal propagation relies on — silently no-op for every streamable-HTTP MCP server (the default for remote servers).The request event hook (
_inject_templated_headers) resolves${context:}by readingget_session_env(...)— i.e. the ContextVar in whatever task drives the HTTP POST. But the POST runs on the dedicated MCP background loop, in a task the SDK'spost_writerspawns under a task group created at server-connect time. That task's contextvar context is frozen at startup and never sees the per-turn session vars set on the caller's thread. So resolution always returns""and the header is dropped.Diagnosis (end-to-end, from Mercator's delegated-principal feature)
delegated_principal_resolution status=resolved ... @verdigris.co.whoamireturnedemail: null.The hook's own docstring claimed it resolved "against the calling task's session context" — an invariant that does not hold for streamable HTTP, because
post_writerdecouples the request from the caller viawrite_stream(and it's a different event loop entirely).Fix
Resolve the templates where the context is correct — the caller's thread (the synchronous tool handler) — and bridge the resolved values to the request hook via an instance attribute:
MCPServerTask._resolve_templated_headers()— resolves${context:}against the current (caller) task's context, returning only non-empty values. Called from the sync tool handler.server._outbound_resolved_headersinside_rpc_lock(so concurrent per-server calls can't race) and clears it in afinally(so pings / reconnect GETs never carry a stale principal).self._outbound_resolved_headersvia the new_apply_resolved_headershelper instead of resolving against its own (wrong) context. The cross-origin identity-header strip is preserved.SSE / legacy-HTTP transports already freeze templated headers at connect (a documented limitation) and are unchanged.
Why caller-side is correct
The sync tool handler runs on the agent thread where plugin hooks (
pre_tool_call/pre_gateway_dispatch) set the session vars;_rpc_lockserializes calls per server, so a single "current outbound headers" slot on the instance is race-free.Tests
tests/tools/test_mcp_context_template.py— existing helper + httpx-mechanics tests retained (annotated); new coverage:TestResolveTemplatedHeadersMethod— caller-side resolution (set/unset/none).TestApplyResolvedHeaders— loop-side application: inject same-origin, drop when missing, strip cross-origin.test_resolved_headers_survive_lost_caller_context— the crux regression: a value resolved while the var is set still injects after that context is gone (simulating the MCP loop). This fails onmain.Run: 64 passed across the context-template + session-env suites; mcp circuit-breaker / error / url / cancellation suites green (29 passed, 1 skipped).
Rollout
After merge, Mercator bumps its Hermes pin to this SHA;
@Mercator whoamishould then return the caller's email with no Mercator-side change (Mercator PR NousResearch#30 already resolves the principal at the request root). Tracked: Z2O-1694; relates to MER-62, Z2O-1691.🤖 Generated with Claude Code
Summary by CodeRabbit
Tests
Improvements