fix(mcp): block inbound internal header injection in reverse proxy - #665
Conversation
nikola0x0
left a comment
nikola0x0
left a comment
There was a problem hiding this comment.
Correct fix, no bypass found. All three handlers route through build_forwarded_headers(), the new check runs before the x-memwal- allowlist, apply_oauth_headers() runs after, and case is already normalized by HeaderName::from_bytes. The #659 repro no longer gets through.
Resolves #659 is fine. The issue lists two remediation items; this is the first. The second (sidecar verification) is now #685, so it stays owned once this closes the parent. Nothing below blocks the merge.
Why #685 matters. After this PR the relayer is the only enforcement point for OAuth scope, and it's load-bearing in both directions:
scripts/mcp/auth.ts:111trusts the header from any caller, and/mcp/*mounts beforesharedSecretAuthMiddleware(scripts/sidecar/app.ts:46,:62) so it has no shared-secret gate at all.scripts/mcp/tools/index.ts:19readsunrestricted = session.oauthScope === undefined, so a missing header grants full write rather than nothing.
Neither is reachable in a default 127.0.0.1 deployment. Context for the follow-up, not change requests.
Pre-existing / out of diff — not this PR (3)
- No detection signal. Dropping an injected header emits nothing — no log, no metric. A rate-limited
tracing::warn!is free on the normal path, since no legitimate client ever sendsx-memwal-internal-*. Relayer-side, so outside #685's scope and currently unticketed. apply_oauth_headersfails open on header-build errors (mcp_proxy.rs:170-190). Eachif let Ok(..)skips silently: a failed scope header means unrestricted write, and a failedx-memwal-account-idleaves the client's forged value in place — the exact merge the function's own doc comment at:160-165forbids. Latent only becausenormalize_scopesconstrains the input.sessionKeyomits the OAuth scope (scripts/mcp/auth.ts:134). Non-exploitable only because/oauth/authorizemints a fresh delegate keypair per grant (routes/oauth.rs:400) — an incidental property, not an enforced invariant.
4 inline. The two cheap ones worth taking: the rustfmt diff at line 591, and a positive-path assertion — nothing in the file asserts the scope header is ever set, only that it's dropped. The allowlist-narrowing note on line 42 is the highest-value but is a design change, fine to defer.
| /// | ||
| /// Internal headers (starting with `x-memwal-internal-`) are strictly reserved | ||
| /// for trusted relayer-to-sidecar communication and must NEVER be forwarded | ||
| /// from untrusted client requests. | ||
| const FORWARD_HEADER_PREFIXES: &[&str] = &["x-memwal-"]; | ||
| const INTERNAL_HEADER_PREFIXES: &[&str] = &["x-memwal-internal-"]; |
There was a problem hiding this comment.
Blocking the -internal- prefix on top of the x-memwal- wildcard works, but it leaves the protection resting on a naming convention rather than on the allowlist.
The sidecar only ever reads three x-memwal-* headers — x-memwal-account-id, x-memwal-namespace, and the internal scope one it should never accept from a client (scripts/mcp/auth.ts:103,110,111). Everything else matching the wildcard is forwarded verbatim for no reason.
So the next relayer→sidecar header that isn't spelled x-memwal-internal-* — x-memwal-oauth-scope, x-memwal-grant-id, x-memwal-admin — is client-injectable on the day it lands, and a typo at the injection site (line 189) has the same effect. Narrowing the allowlist closes the whole class and makes INTERNAL_HEADER_PREFIXES unnecessary:
-const FORWARD_HEADER_PREFIXES: &[&str] = &["x-memwal-"];
-const INTERNAL_HEADER_PREFIXES: &[&str] = &["x-memwal-internal-"];
const FORWARD_HEADER_EXACT: &[&str] = &[
+ "x-memwal-account-id",
+ "x-memwal-namespace",
"authorization",That's the pattern this file already uses, and the one main.rs:1478 uses for the CORS allow_headers list.
If you'd rather keep the blocklist: the new doc comment on lines 42-45 describes internal headers but is syntactically attached to FORWARD_HEADER_PREFIXES on line 46, so rustdoc associates it with the wrong const. Worth moving onto line 47.
| let s = name.as_str().to_ascii_lowercase(); | ||
| if INTERNAL_HEADER_PREFIXES.iter().any(|p| s.starts_with(p)) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
HeaderName::as_str() is always lowercase — http normalizes at construction — so to_ascii_lowercase() never actually folds anything. It does heap-allocate once per header per request on the /api/mcp/* path (SSE open, plus every JSON-RPC POST on the streamable transport), and this change adds a second scan over the result. Matching on name.as_str() directly is both correct and free.
Separately, this returns false silently. Since no legitimate client ever sends x-memwal-internal-*, a rate-limited tracing::warn! here costs nothing on the normal path and is the only thing that would make an injection attempt visible — to alerting, or to anyone reconstructing an incident later.
| for h in [ | ||
| "x-memwal-internal-oauth-scope", | ||
| "x-memwal-internal-auth", | ||
| "x-memwal-internal-test", | ||
| "X-MemWal-Internal-Oauth-Scope", | ||
| ] { | ||
| let name = AxumHeaderName::from_bytes(h.as_bytes()).unwrap(); | ||
| assert!(!should_forward(&name), "must not forward internal header {h}"); | ||
| } |
There was a problem hiding this comment.
Two things about this test.
The "X-MemWal-Internal-Oauth-Scope" case is vacuous. AxumHeaderName::from_bytes lowercases at construction, so by the time should_forward sees it it is byte-identical to case 1 — the test reads as coverage for the mixed-case spelling #659's repro actually uses, but exercises the same path twice. Genuine case-insensitivity coverage would need an HTTP round-trip where the raw casing survives to the boundary.
And line 591 isn't rustfmt-clean. rustfmt --edition 2021 --check on this file reports exactly one diff, here:
- assert!(!should_forward(&name), "must not forward internal header {h}");
+ assert!(
+ !should_forward(&name),
+ "must not forward internal header {h}"
+ );The rest of mcp_proxy.rs is clean and CI only runs clippy (soft-fail), so this won't get caught — it'll surface as an unrelated hunk in a security-sensitive file's blame the next time someone runs cargo fmt.
| assert!( | ||
| out.get("x-memwal-internal-oauth-scope").is_none(), | ||
| "inbound internal oauth scope header must be dropped" | ||
| ); |
There was a problem hiding this comment.
Good negative assertion. The matching positive one is missing next door, and that gap is worth closing in this PR specifically.
apply_oauth_headers_overwrites_forwarded_authorization_and_account_id (line 685) builds an identity with scope: "memwal:read" and asserts only on authorization and x-memwal-account-id. Across the whole file the only assertions on x-memwal-internal-oauth-scope are negative — nothing anywhere checks it actually gets injected.
The obvious next hardening step after this PR is running should_forward as a final sanitizer over the outbound map, or reusing build_forwarded_headers on the post-OAuth headers. Either would strip the legitimately-injected scope header, silently returning every OAuth session to unrestricted write via the fail-open at scripts/mcp/tools/index.ts:19 — with all 11 tests still green.
assert_eq!(
forwarded
.get("x-memwal-account-id")
.and_then(|v| v.to_str().ok()),
Some("0xrealaccount")
);
+ assert_eq!(
+ forwarded
+ .get("x-memwal-internal-oauth-scope")
+ .and_then(|v| v.to_str().ok()),
+ Some("memwal:read")
+ );
6e2be8f to
1b0e0ce
Compare
|
Added the #685 defense-in-depth fix on this PR:
Verified: 210/210 sidecar tests and 12/12 |
|
Deployment note: the sidecar endpoint is not publicly exposed in our current deployment; it is reachable only by the relayer over the internal/loopback network. This hardening is defense-in-depth for future split deployments, SSRF, or an internal network foothold. |
025c0ac to
334b876
Compare
2 participants
Resolves #659
Summary
Fixes inbound
X-MemWal-Internal-Oauth-Scopeheader injection vulnerability in the MCP reverse proxy (services/server/src/mcp_proxy.rs).Root Cause
FORWARD_HEADER_PREFIXESallowed any header starting withx-memwal-to be forwarded verbatim from client requests to the TypeScript sidecar. An external client could supplyX-MemWal-Internal-Oauth-Scope: memwal:writeto escalate privileges even with restricted scope.Changes
x-memwal-internal-inshould_forward().x-memwal-internal-*headers are stripped from inbound client requests while legitimate headers (authorization,x-memwal-account-id, etc.) are preserved.Verification
cargo test --manifest-path services/server/Cargo.toml mcp_proxy(11/11 tests pass)npm testinservices/server/scripts(195/195 tests pass)