fix(mcp): verify internal headers at the sidecar and fail closed on missing scope - #688
Conversation
…issing scope Second remediation item from #659. #665 stops a client smuggling x-memwal-internal-* through the relayer, but the sidecar still trusted the header from any caller and treated its absence as unrestricted access. Relayer: apply_oauth_headers becomes apply_internal_headers, called on all three proxy handlers including the legacy Passthrough arm that previously set nothing. It states both internal headers explicitly — the sidecar shared secret, and the granted scope (the resolved grant for OAuth callers, full read+write for legacy delegate-key callers). Both are written with insert, so a client-supplied value is always overwritten. Header-build failures now return 500 instead of being silently skipped, which previously left an authenticated request with no scope. Sidecar: new verifyInternalOrigin() compares the token with timingSafeEqual and runs first in resolveAuth, so an unverified caller gets a 401 before any internal header is read. registerTools drops the `unrestricted` branch — an absent or empty scope now registers no tools rather than every tool. No new environment variables: SIDECAR_AUTH_TOKEN is already mandatory and already shared by both processes. Resolves #685
ducnmm
left a comment
There was a problem hiding this comment.
Thanks for taking this over — the shared-secret header and explicit legacy scope are the right direction. Two blockers before merge:
-
Bind the granted scope to the MCP session.
resolveAuth()still buildssessionKeyfrom only{accountId, delegatePubKey}. Existing SSE/streamable servers keep the tools registered at session creation, so a later request with the same account/delegate but a missing or narrower scope still passes the session-key check and can drive the original write-capable session. This means missing scope does not actually fail closed after initialization. Include the normalized scope insessionKey, and add a regression test that opens a full-scope session then reuses its session ID with no scope /memwal:readand expects 403. -
The stdio bridge integration test no longer exercises a successful upstream connection. Its
/apitest mount bypasses the Rust proxy, so the bridge cannot sendx-memwal-internal-sidecar-tokenor the explicit legacy scope. The test can still pass via the local/auth fallback because it only assertsmemwal_login. Add proxy-simulation middleware on the/apirouter to inject the internal token + full scope, then assert a remote tool such asmemwal_recallis present.
Everything else looks solid, and CI is green.
Addresses review on #688. The tool set is bound at session-open time, so a session opened with write scope keeps its write tools for its whole life. sessionKey was built from {accountId, delegatePubKey} only, so a later request with a narrower or absent scope passed the session-binding check and drove that write-capable transport — the fail-closed guarantee held only until initialization. Delegate keys are reused across grants for the same account (find_reusable_oauth_delegate keys on account_id alone), so {account, delegate} does not distinguish two grants of differing scope. The key now includes the normalized scope: deduplicated and sorted, so reordering a grant does not fork the session. Also fixes the stdio bridge test, which had stopped exercising a successful upstream connection. Its /api mount has no Rust proxy, so nothing supplied the internal headers and the bridge fell back to its local tool list. The fallback advertises the same tool names as the relayer, so no assertion on tool names could detect this. The mount now simulates the proxy's apply_internal_headers, and the test waits for the upstream SSE and asserts it returned 200 before requesting tools — the earlier list was answered by the bridge's coldstart path while the connection was still in flight.
|
Both fixed in 422d4c6. Verified each one first — the second turned out to be worse than described. 1. Scope bound to the session keyConfirmed and exploitable, not theoretical. I'd previously assumed each grant mints its own delegate keypair, which would have made the session keys differ anyway. That's wrong:
2. Stdio bridge testCorrect, and the assertion couldn't be repaired by swapping in a remote tool name. There was a second cause underneath. Even with the proxy simulation added, the test still passed with the scope injection removed — because So the fix is three parts:
Mutation-checked: removing either injected header now fails the test. Previously removing the scope changed nothing. VerificationTS sidecar 221/221, |
ducnmm
left a comment
There was a problem hiding this comment.
LGTM! The session key now properly includes the normalized scope to prevent post-init privilege escalation, and the integration tests properly simulate the Rust proxy headers. All CI checks pass.
Resolves #685 (WALM-372) — the second remediation item from #659, which #665 did not cover.
Problem
The relayer decides what an MCP caller may do and tells the sidecar with
x-memwal-internal-oauth-scope. Two defects made the relayer a single point of failure:/mcp/*is mounted beforesharedSecretAuthMiddleware(becauseAuthorizationalready carries the end user's delegate key), so it had no shared-secret gate at all. fix(mcp): block inbound internal header injection in reverse proxy #665 stops a client smuggling the header through the relayer; it does nothing about a caller reaching the sidecar directly.tools/index.tsreadconst unrestricted = session.oauthScope === undefined, so an absent header granted full write. The intent was legitimate — legacy delegate-key callers have no OAuth scope and should get everything — but the sidecar could not tell "legacy login" from "the header went missing."Changes
Relayer —
apply_oauth_headers→apply_internal_headers, now called on all three proxy handlers including the legacyPassthrougharm that previously set nothing. It states both internal headers explicitly:x-memwal-internal-sidecar-tokenSIDECAR_AUTH_TOKENx-memwal-internal-oauth-scopememwal:read memwal:writeBoth use
insert(overwrite), so a client-supplied value never survives. Header-build failures return 500 instead of being silently skipped — previously that left an authenticated request with no scope, which under the old fail-open meant full write.Sidecar — new
verifyInternalOrigin()(timingSafeEqual, length-guarded) runs first inresolveAuth, so an unverified caller gets 401 before any internal header is read.registerToolsdrops theunrestrictedbranch: absent or empty scope now registers no tools.No new environment variables
SIDECAR_AUTH_TOKENis already mandatory (the sidecarprocess.exit(1)s without it), already documented as required in three places, already in both.env.examplefiles and CI, and already inherited by the spawned sidecar from the relayer's environment. Nothing for operators to do.Verification
mcp_proxydev+ 4 new)devbaseline, no regressionsrustfmtmcp_proxy.rsclippyNotes for review
X-MemWal-Internal-Oauth-ScopeHeader Injection in MCP Proxy Permitting Scope Escalation #659's vector independently of fix(mcp): block inbound internal header injection in reverse proxy #665. Both headers are written withinsert, so a forged inbound value is always overwritten. Based ondevrather than stacked on fix(mcp): block inbound internal header injection in reverse proxy #665, so it can merge in either order — the two touch different functions inmcp_proxy.rs.toolscapability, not an empty list. The MCP SDK stops advertisingtools/listentirely and returns-32601. Correct fail-closed behavior, asserted intool-scope.test.ts.tool-annotations.test.tswas relying on the fail-open — it passed{} as MemWalSessionand got all six tools. It now passes an explicit full scope. That test was green because of the bug./mcp/*had no shared-secret gate are updated (mcp_proxy.rsmodule header,sidecar/app.tsmount block,auth.tsheader contract).