diff --git a/services/server/scripts/mcp/__tests__/auth.test.ts b/services/server/scripts/mcp/__tests__/auth.test.ts new file mode 100644 index 00000000..42a27195 --- /dev/null +++ b/services/server/scripts/mcp/__tests__/auth.test.ts @@ -0,0 +1,83 @@ +import assert from "node:assert/strict"; +import test from "node:test"; + +import { McpAuthError, normalizeScope, resolveAuth } from "../auth.js"; + +const TOKEN = "test-sidecar-token-0123456789"; +const DELEGATE_KEY = "a".repeat(64); +const ACCOUNT_ID = `0x${"b".repeat(64)}`; +const SERVER_URL = "http://relayer.invalid"; + +function mcpHeaders(overrides: Record = {}): Headers { + const base: Record = { + authorization: `Bearer ${DELEGATE_KEY}`, + "x-memwal-account-id": ACCOUNT_ID, + "x-memwal-internal-sidecar-token": TOKEN, + ...overrides, + }; + const h = new Headers(); + for (const [k, v] of Object.entries(base)) { + if (v !== undefined) h.set(k, v); + } + return h; +} + +test("resolveAuth rejects a caller that cannot prove it is the relayer", async () => { + process.env.SIDECAR_AUTH_TOKEN = TOKEN; + + await assert.rejects( + () => resolveAuth(mcpHeaders({ "x-memwal-internal-sidecar-token": undefined }), SERVER_URL), + (err: unknown) => { + assert.ok(err instanceof McpAuthError, `expected McpAuthError, got ${err}`); + assert.equal(err.status, 401); + return true; + } + ); +}); + +test("resolveAuth reads the OAuth scope once the origin is verified", async () => { + process.env.SIDECAR_AUTH_TOKEN = TOKEN; + + const { session } = await resolveAuth( + mcpHeaders({ "x-memwal-internal-oauth-scope": "memwal:read" }), + SERVER_URL + ); + + assert.equal(session.oauthScope, "memwal:read"); +}); + +test("normalizeScope is order- and duplicate-insensitive", () => { + // The session key embeds this, so two spellings of the same grant must not + // open two distinct sessions. + assert.equal( + normalizeScope("memwal:write memwal:read"), + normalizeScope("memwal:read memwal:write") + ); + assert.equal(normalizeScope("memwal:read memwal:read"), "memwal:read"); + assert.equal(normalizeScope(" memwal:read memwal:write "), "memwal:read memwal:write"); +}); + +test("normalizeScope collapses an absent or blank scope to the empty string", () => { + assert.equal(normalizeScope(undefined), ""); + assert.equal(normalizeScope(""), ""); + assert.equal(normalizeScope(" "), ""); +}); + +test("sessionKey is stable across scope orderings but differs by granted scope", async () => { + process.env.SIDECAR_AUTH_TOKEN = TOKEN; + + const keyFor = async (scope: string) => + (await resolveAuth(mcpHeaders({ "x-memwal-internal-oauth-scope": scope }), SERVER_URL)) + .sessionKey; + + assert.equal( + await keyFor("memwal:write memwal:read"), + await keyFor("memwal:read memwal:write"), + "reordering the same grant must not fork the session" + ); + assert.notEqual( + await keyFor("memwal:read memwal:write"), + await keyFor("memwal:read"), + "a narrower grant must not reuse a wider grant's session" + ); +}); diff --git a/services/server/scripts/mcp/__tests__/instructions.test.ts b/services/server/scripts/mcp/__tests__/instructions.test.ts index b99288c6..791e3547 100644 --- a/services/server/scripts/mcp/__tests__/instructions.test.ts +++ b/services/server/scripts/mcp/__tests__/instructions.test.ts @@ -18,6 +18,7 @@ * IMPORTANT: env vars MUST be set BEFORE importing `mountMcpRoutes` because * the rate limiter is constructed at module-load time. */ +process.env.SIDECAR_AUTH_TOKEN ??= "instructions-test-sidecar-token"; process.env.MCP_MAX_TOTAL_SESSIONS = "100"; process.env.MCP_MAX_SESSIONS_PER_IP = "100"; process.env.MCP_MAX_NEW_SESSIONS_PER_IP_PER_MIN = "100"; @@ -81,6 +82,9 @@ async function initialize(): Promise> { authorization: `Bearer ${randomBytes(32).toString("hex")}`, "x-memwal-account-id": "0x" + randomBytes(32).toString("hex"), "x-forwarded-for": "203.0.113.7", + // Stands in for the relayer, which proves its origin with the + // sidecar shared secret before any internal header is honoured. + "x-memwal-internal-sidecar-token": process.env.SIDECAR_AUTH_TOKEN!, }, body: JSON.stringify({ jsonrpc: "2.0", diff --git a/services/server/scripts/mcp/__tests__/integration.test.ts b/services/server/scripts/mcp/__tests__/integration.test.ts index e5079920..b427bc20 100644 --- a/services/server/scripts/mcp/__tests__/integration.test.ts +++ b/services/server/scripts/mcp/__tests__/integration.test.ts @@ -16,6 +16,7 @@ * IMPORTANT: env vars MUST be set BEFORE importing `mountMcpRoutes` because * the rate limiter is constructed at module-load time. */ +process.env.SIDECAR_AUTH_TOKEN ??= "integration-test-sidecar-token"; process.env.MCP_MAX_TOTAL_SESSIONS = "100"; process.env.MCP_MAX_SESSIONS_PER_IP = "100"; // Tight burst cap so the test can trip it in 3 calls. @@ -51,6 +52,23 @@ function fakeCreds(): { bearer: string; accountId: string } { let server: Server; let baseUrl: string; +const apiLog: Array<{ path: string; status: number }> = []; + +/** Wait for the bridge's own upstream request to land on the /api mount. */ +async function waitForApi( + pred: (entry: { path: string; status: number }) => boolean, + timeoutMs = 5000, +): Promise<{ path: string; status: number }> { + const deadline = Date.now() + timeoutMs; + for (;;) { + const hit = apiLog.find(pred); + if (hit) return hit; + if (Date.now() > deadline) { + throw new Error(`no matching /api request within ${timeoutMs}ms; saw ${JSON.stringify(apiLog)}`); + } + await new Promise((r) => setTimeout(r, 25)); + } +} before(async () => { const app = express(); @@ -66,6 +84,30 @@ before(async () => { // routes. Mount the real handlers under /api as well so the official // stdio bridge can exercise its production URLs without a proxy stub. const publicApi = express.Router(); + // Record what the bridge actually got back. A tools/list assertion cannot + // tell upstream from the bridge's local auth-required fallback, because + // that fallback advertises the same tool names — so the only honest signal + // that the upstream connection worked is the response status here. + publicApi.use((req, res, next) => { + // Capture the status as headers are written, not on "finish": an SSE + // response stays open, so "finish" never fires for /mcp/sse. + const writeHead = res.writeHead.bind(res); + res.writeHead = ((...args: Parameters) => { + apiLog.push({ path: req.path, status: args[0] as number }); + return writeHead(...args); + }) as typeof res.writeHead; + next(); + }); + // Stand in for the Rust relayer's `apply_internal_headers`. In production + // /api/mcp/* is served by the proxy, which states the sidecar token and the + // caller's granted scope on every forwarded request. Without this the + // official bridge cannot authenticate against the sidecar at all, and the + // test would silently fall back to the bridge's local tool list. + publicApi.use((req, _res, next) => { + req.headers["x-memwal-internal-sidecar-token"] = INTERNAL_TOKEN; + req.headers["x-memwal-internal-oauth-scope"] = "memwal:read memwal:write"; + next(); + }); mountMcpRoutes(publicApi, { relayerUrl: "http://localhost:1" }); app.use("/api", publicApi); await new Promise((resolve) => { @@ -92,11 +134,17 @@ function initializeBody(id: number) { }); } +// The relayer proves it is the relayer with the sidecar shared secret; every +// request that expects to get past `resolveAuth` has to carry it. +const INTERNAL_TOKEN_HEADER = "x-memwal-internal-sidecar-token"; +const INTERNAL_TOKEN = process.env.SIDECAR_AUTH_TOKEN!; + function mcpHeaders(opts: { bearer: string; accountId: string; xff: string; sessionId?: string; + scope?: string; }): Record { const h: Record = { "content-type": "application/json", @@ -104,8 +152,10 @@ function mcpHeaders(opts: { authorization: `Bearer ${opts.bearer}`, "x-memwal-account-id": opts.accountId, "x-forwarded-for": opts.xff, + [INTERNAL_TOKEN_HEADER]: INTERNAL_TOKEN, }; if (opts.sessionId) h["mcp-session-id"] = opts.sessionId; + if (opts.scope !== undefined) h["x-memwal-internal-oauth-scope"] = opts.scope; return h; } @@ -114,6 +164,7 @@ async function postInit(opts: { accountId: string; xff: string; sessionId?: string; + scope?: string; id?: number; }): Promise<{ status: number; sessionId: string | null; bodyText: string }> { const res = await fetch(`${baseUrl}/mcp`, { @@ -142,6 +193,7 @@ async function openSse(opts: { authorization: `Bearer ${opts.bearer}`, "x-memwal-account-id": opts.accountId, "x-forwarded-for": opts.xff, + [INTERNAL_TOKEN_HEADER]: INTERNAL_TOKEN, }, }); assert.equal(response.status, 200); @@ -225,6 +277,46 @@ test("reusing mcp-session-id under a different bearer returns 403", async () => assert.match((json as any).error.message, /does not match authenticated caller/); }); +test("reusing mcp-session-id under a narrower scope returns 403", async () => { + // The tool set is bound at session-open time, so a session opened with + // write scope keeps its write tools registered. If the session key ignores + // scope, a later read-only request routes into that write-capable + // transport and the fail-closed guarantee stops holding after init. + const xff = "192.0.2.80"; + const creds = fakeCreds(); + + const opened = await postInit({ ...creds, xff, scope: "memwal:read memwal:write" }); + assert.equal(opened.status, 200); + const sid = opened.sessionId; + assert.ok(sid); + + const narrowed = await fetch(`${baseUrl}/mcp`, { + method: "POST", + headers: mcpHeaders({ ...creds, xff, sessionId: sid!, scope: "memwal:read" }), + body: JSON.stringify({ jsonrpc: "2.0", id: 98, method: "ping" }), + }); + + assert.equal(narrowed.status, 403, `expected 403, got ${narrowed.status}`); +}); + +test("reusing mcp-session-id with the scope header dropped returns 403", async () => { + const xff = "192.0.2.81"; + const creds = fakeCreds(); + + const opened = await postInit({ ...creds, xff, scope: "memwal:read memwal:write" }); + assert.equal(opened.status, 200); + const sid = opened.sessionId; + assert.ok(sid); + + const dropped = await fetch(`${baseUrl}/mcp`, { + method: "POST", + headers: mcpHeaders({ ...creds, xff, sessionId: sid! }), + body: JSON.stringify({ jsonrpc: "2.0", id: 97, method: "ping" }), + }); + + assert.equal(dropped.status, 403, `expected 403, got ${dropped.status}`); +}); + test("reusing mcp-session-id under SAME bearer is accepted (sanity check)", async () => { const xff = "192.0.2.50"; const creds = fakeCreds(); @@ -258,6 +350,8 @@ test("malformed bearer returns 401, not 429 — auth still runs after rate-limit authorization: "Bearer not-a-hex-key", "x-memwal-account-id": "0x" + "a".repeat(64), "x-forwarded-for": "192.0.2.60", + // Origin is verified; the malformed bearer is what must be rejected. + [INTERNAL_TOKEN_HEADER]: INTERNAL_TOKEN, }, body: initializeBody(1), }); @@ -276,7 +370,11 @@ test("SSE message POST is bound to the principal that opened the session", async try { const noAuth = await fetch(url, { method: "POST", - headers: { "content-type": "application/json" }, + headers: { + "content-type": "application/json", + // Origin is verified; the absent bearer is what must be rejected. + [INTERNAL_TOKEN_HEADER]: INTERNAL_TOKEN, + }, body, }); assert.equal(noAuth.status, 401); @@ -407,12 +505,25 @@ test("official stdio bridge authenticates SSE message POSTs", async (t) => { assert(initialized.result, JSON.stringify(initialized)); send({ jsonrpc: "2.0", method: "notifications/initialized" }); + + // Wait for the upstream SSE BEFORE asking for tools. The bridge answers + // tools/list from its coldstart path while the connection is still in + // flight, and its local fallback advertises the same tool names as the + // relayer — so a list requested too early cannot distinguish the two. + const sse = await waitForApi((entry) => entry.path.includes("/mcp/sse")); + assert.equal(sse.status, 200, `upstream SSE was rejected: ${JSON.stringify(apiLog)}`); + send({ jsonrpc: "2.0", id: 902, method: "tools/list", params: {} }); const tools = await waitFor((message) => message.id === 902); assert(Array.isArray(tools.result?.tools), JSON.stringify(tools)); + + // Require the relayer-registered tools. These appear only when the proxy + // stated a scope wide enough for `registerTools` to register them, so this + // fails if the simulated proxy stops sending either internal header. + const names = new Set(tools.result.tools.map((tool: { name?: string }) => tool.name)); assert( - tools.result.tools.some((tool: { name?: string }) => tool.name === "memwal_login"), - "bridge should receive the authenticated tools/list response", + ["memwal_recall", "memwal_health", "memwal_remember"].every((n) => names.has(n)), + `bridge should receive the authenticated upstream tools/list response, got: ${[...names].join(", ")}`, ); child.stdin.end(); diff --git a/services/server/scripts/mcp/__tests__/internal-auth.test.ts b/services/server/scripts/mcp/__tests__/internal-auth.test.ts new file mode 100644 index 00000000..23e6b943 --- /dev/null +++ b/services/server/scripts/mcp/__tests__/internal-auth.test.ts @@ -0,0 +1,45 @@ +import assert from "node:assert/strict"; +import test from "node:test"; + +import { verifyInternalOrigin } from "../internal-auth.js"; + +const TOKEN = "test-sidecar-token-0123456789"; + +function headersWith(token?: string): Headers { + const h = new Headers(); + if (token !== undefined) h.set("x-memwal-internal-sidecar-token", token); + return h; +} + +test("verifyInternalOrigin accepts a request carrying the configured sidecar token", () => { + process.env.SIDECAR_AUTH_TOKEN = TOKEN; + + assert.equal(verifyInternalOrigin(headersWith(TOKEN)), true); +}); + +test("verifyInternalOrigin rejects a request with no sidecar token header", () => { + process.env.SIDECAR_AUTH_TOKEN = TOKEN; + + assert.equal(verifyInternalOrigin(headersWith(undefined)), false); +}); + +test("verifyInternalOrigin rejects a wrong token of the same length", () => { + process.env.SIDECAR_AUTH_TOKEN = TOKEN; + const wrong = "X".repeat(TOKEN.length); + + assert.equal(wrong.length, TOKEN.length); + assert.equal(verifyInternalOrigin(headersWith(wrong)), false); +}); + +test("verifyInternalOrigin rejects a token that is a prefix of the real one", () => { + process.env.SIDECAR_AUTH_TOKEN = TOKEN; + + assert.equal(verifyInternalOrigin(headersWith(TOKEN.slice(0, -1))), false); +}); + +test("verifyInternalOrigin rejects everything when SIDECAR_AUTH_TOKEN is unset", () => { + delete process.env.SIDECAR_AUTH_TOKEN; + + assert.equal(verifyInternalOrigin(headersWith(TOKEN)), false); + assert.equal(verifyInternalOrigin(headersWith("")), false); +}); diff --git a/services/server/scripts/mcp/__tests__/tool-annotations.test.ts b/services/server/scripts/mcp/__tests__/tool-annotations.test.ts index 17612cce..9bd83a1d 100644 --- a/services/server/scripts/mcp/__tests__/tool-annotations.test.ts +++ b/services/server/scripts/mcp/__tests__/tool-annotations.test.ts @@ -7,7 +7,11 @@ import { createMcpServer } from "../server.js"; test("tools/list publishes safe titles and behavior annotations for every remote tool", async (t) => { const [clientTransport, serverTransport] = InMemoryTransport.createLinkedPair(); - const server = createMcpServer({} as MemWalSession); + // Full scope so every remote tool is registered — tool registration is + // scope-gated, and this test is about the annotations each tool publishes. + const server = createMcpServer({ + oauthScope: "memwal:read memwal:write", + } as MemWalSession); const client = new Client({ name: "annotations-test", version: "1.0.0" }); t.after(async () => { diff --git a/services/server/scripts/mcp/__tests__/tool-scope.test.ts b/services/server/scripts/mcp/__tests__/tool-scope.test.ts new file mode 100644 index 00000000..e902001a --- /dev/null +++ b/services/server/scripts/mcp/__tests__/tool-scope.test.ts @@ -0,0 +1,58 @@ +import assert from "node:assert/strict"; +import test, { type TestContext } from "node:test"; +import { Client } from "@modelcontextprotocol/sdk/client/index.js"; +import { InMemoryTransport } from "@modelcontextprotocol/sdk/inMemory.js"; +import type { MemWalSession } from "../auth.js"; +import { createMcpServer } from "../server.js"; + +const WRITE_TOOLS = [ + "memwal_remember", + "memwal_remember_bulk", + "memwal_analyze", + "memwal_restore", +]; +const READ_TOOLS = ["memwal_recall", "memwal_health"]; + +async function toolNamesFor(oauthScope: string | undefined, t: TestContext): Promise { + const [clientTransport, serverTransport] = InMemoryTransport.createLinkedPair(); + const server = createMcpServer({ oauthScope } as MemWalSession); + const client = new Client({ name: "scope-test", version: "1.0.0" }); + + t.after(async () => { + await client.close(); + await server.close(); + }); + + await server.connect(serverTransport); + await client.connect(clientTransport); + + // With zero tools registered the SDK never declares the `tools` capability, + // so `tools/list` is an unknown method rather than an empty list. From the + // client's side both mean the same thing: no tools are available. + try { + const { tools } = await client.listTools(); + return tools.map((tool) => tool.name).sort(); + } catch (err) { + if (err instanceof Error && err.message.includes("Method not found")) return []; + throw err; + } +} + +test("registerTools grants nothing when the relayer sent no scope", async (t) => { + assert.deepEqual(await toolNamesFor(undefined, t), []); +}); + +test("registerTools grants nothing when the scope is empty or whitespace", async (t) => { + assert.deepEqual(await toolNamesFor(" ", t), []); +}); + +test("registerTools grants only read tools for memwal:read", async (t) => { + assert.deepEqual(await toolNamesFor("memwal:read", t), [...READ_TOOLS].sort()); +}); + +test("registerTools grants every tool for the full legacy scope", async (t) => { + assert.deepEqual( + await toolNamesFor("memwal:read memwal:write", t), + [...READ_TOOLS, ...WRITE_TOOLS].sort() + ); +}); diff --git a/services/server/scripts/mcp/auth.ts b/services/server/scripts/mcp/auth.ts index 74ce205d..cf14857d 100644 --- a/services/server/scripts/mcp/auth.ts +++ b/services/server/scripts/mcp/auth.ts @@ -16,6 +16,7 @@ * ============================================================================= */ import { MemWal } from "@mysten-incubation/memwal"; +import { verifyInternalOrigin } from "./internal-auth.js"; export interface MemWalSession { accountId: string; @@ -43,6 +44,16 @@ export class McpAuthError extends Error { const HEX64_RE = /^(0x)?[0-9a-fA-F]{64}$/; +/** + * Canonical form of a scope string for use in the session key: deduplicated, + * sorted, single-space separated. Order and repetition carry no meaning, so + * `"memwal:write memwal:read"` and `"memwal:read memwal:read memwal:write"` + * must not open two distinct sessions. Absent scope collapses to `""`. + */ +export function normalizeScope(scope: string | undefined): string { + return [...new Set(scope?.split(/\s+/).filter(Boolean) ?? [])].sort().join(" "); +} + /** * Derive the Ed25519 public-key hex from a private-key hex (32-byte seed). * Lazy import so we don't pull crypto into module init. @@ -75,10 +86,16 @@ function bytesToHex(b: Uint8Array): string { * Resolve auth from incoming HTTP headers. * * Required headers: + * X-MemWal-Internal-Sidecar-Token: * Authorization: Bearer (64 hex chars) * X-MemWal-Account-Id: 0x (66 chars) * Optional: * X-MemWal-Namespace: (default per-tool) + * X-MemWal-Internal-Oauth-Scope: (relayer-issued) + * + * The internal token is checked first: `x-memwal-internal-*` headers state + * decisions the relayer already made, so only the relayer may set them. An + * absent scope grants no tools — see tools/index.ts. * * Throws McpAuthError on missing / malformed credentials. */ @@ -86,6 +103,16 @@ export async function resolveAuth( headers: Headers, serverUrl: string ): Promise { + // Runs before anything else reads the request: `x-memwal-internal-*` + // headers carry decisions the relayer already made, so a caller that + // cannot prove it is the relayer must not be able to state them. + if (!verifyInternalOrigin(headers)) { + throw new McpAuthError( + "Request did not originate from the MemWal relayer", + 401 + ); + } + const authHeader = headers.get("authorization"); if (!authHeader || !authHeader.toLowerCase().startsWith("bearer ")) { throw new McpAuthError( @@ -128,10 +155,19 @@ export async function resolveAuth( oauthScope, }; - // Session key stable across reconnects from same {account, delegate}. We - // don't include namespace because the same client can call multiple - // namespaces in one session via per-tool overrides. - const sessionKey = `delegate:${accountId}:${delegatePubKeyHex}`; + // Session key stable across reconnects from same {account, delegate, + // scope}. We don't include namespace because the same client can call + // multiple namespaces in one session via per-tool overrides. + // + // The scope IS included: `registerTools` binds the tool set at session-open + // time, so a session opened with write scope keeps its write tools for its + // whole life. Without the scope in the key, a later read-only (or + // scope-less) request would pass the session-binding check and drive that + // write-capable transport — which would leave the fail-closed guarantee + // holding only until initialization. Delegate keys are reused across grants + // for the same account (`find_reusable_oauth_delegate`), so {account, + // delegate} alone does not distinguish two grants of differing scope. + const sessionKey = `delegate:${accountId}:${delegatePubKeyHex}:${normalizeScope(oauthScope)}`; return { session, sessionKey }; } diff --git a/services/server/scripts/mcp/internal-auth.ts b/services/server/scripts/mcp/internal-auth.ts new file mode 100644 index 00000000..91f22f39 --- /dev/null +++ b/services/server/scripts/mcp/internal-auth.ts @@ -0,0 +1,36 @@ +/** + * ============================================================================= + * MCP INTERNAL-ORIGIN VERIFICATION + * ============================================================================= + * `x-memwal-internal-*` headers carry decisions the relayer has already made + * (currently the resolved OAuth scope). They are trusted input, so the sidecar + * must confirm they actually came from the relayer before reading any of them. + * + * `/mcp/*` is mounted BEFORE `sharedSecretAuthMiddleware` (sidecar/app.ts) + * because the `Authorization` header already carries the end user's delegate + * key, leaving no room for the sidecar's shared secret. The relayer therefore + * presents the same secret in a dedicated internal header instead. + * + * `SIDECAR_AUTH_TOKEN` is read from the environment on every call rather than + * imported from sidecar/middleware.ts, whose module-level `process.exit(1)` + * guard would fire inside unit tests that never intend to boot the sidecar. + * ============================================================================= + */ +import { timingSafeEqual } from "node:crypto"; + +export const INTERNAL_TOKEN_HEADER = "x-memwal-internal-sidecar-token"; + +export function verifyInternalOrigin(headers: Headers): boolean { + const expected = process.env.SIDECAR_AUTH_TOKEN; + if (!expected) return false; + + const provided = headers.get(INTERNAL_TOKEN_HEADER); + if (provided === null) return false; + + const providedBuf = Buffer.from(provided); + const expectedBuf = Buffer.from(expected); + // timingSafeEqual requires equal lengths; a length mismatch is already a + // mismatch, so short-circuit rather than throwing. + return providedBuf.length === expectedBuf.length + && timingSafeEqual(providedBuf, expectedBuf); +} diff --git a/services/server/scripts/mcp/tools/index.ts b/services/server/scripts/mcp/tools/index.ts index 0c0ba329..e230ee39 100644 --- a/services/server/scripts/mcp/tools/index.ts +++ b/services/server/scripts/mcp/tools/index.ts @@ -15,10 +15,14 @@ import { registerHealthTool } from "./health.js"; * relayer for SEAL encrypt/decrypt + Walrus storage. */ export function registerTools(server: McpServer, session: MemWalSession): void { + // Fail closed: the relayer states the granted scope explicitly on every + // forwarded request — the resolved grant for OAuth callers, and the full + // read+write scope for legacy delegate-key callers. An absent or empty + // scope therefore means the relayer never vouched for this request, so it + // grants nothing rather than everything. const granted = new Set(session.oauthScope?.split(/\s+/).filter(Boolean)); - const unrestricted = session.oauthScope === undefined; - const canRead = unrestricted || granted.has("memwal:read"); - const canWrite = unrestricted || granted.has("memwal:write"); + const canRead = granted.has("memwal:read"); + const canWrite = granted.has("memwal:write"); if (canWrite) { registerRememberTool(server, session); diff --git a/services/server/scripts/sidecar/app.ts b/services/server/scripts/sidecar/app.ts index 663cf51a..8960358e 100644 --- a/services/server/scripts/sidecar/app.ts +++ b/services/server/scripts/sidecar/app.ts @@ -47,8 +47,13 @@ export function createSidecarApp(mode: "full" | "writer" = SIDECAR_ROUTE_MODE): // middleware: MCP traffic is forwarded by the Rust relayer with the end-user's // own delegate-key Bearer token in `Authorization`, NOT the sidecar's shared // secret. The MCP layer does its own auth (parse delegate key + account id - // from request headers). These routes are reachable only from the relayer - // over localhost — same trust boundary as the rest of the sidecar. + // from request headers). + // + // Skipping the middleware does NOT mean these routes are unauthenticated: + // `resolveAuth` requires the sidecar shared secret in + // `x-memwal-internal-sidecar-token` before it will honour any + // `x-memwal-internal-*` header, so localhost reachability alone is not + // enough to claim relayer-issued privileges (GH #685). if (mode === "full") { mountMcpRoutes(app, { relayerUrl: process.env.MEMWAL_RELAYER_URL ?? "http://localhost:3001", diff --git a/services/server/src/mcp_proxy.rs b/services/server/src/mcp_proxy.rs index b3cc682a..0c24da49 100644 --- a/services/server/src/mcp_proxy.rs +++ b/services/server/src/mcp_proxy.rs @@ -18,10 +18,12 @@ //! Ed25519 delegate key and the `X-MemWal-Account-Id` header — and the //! SDK signs every downstream relayer API call from inside the MCP tools. //! -//! Trust model: only the relayer can reach the sidecar (loopback). Forwarding -//! the user's `Authorization` header into the sidecar is safe; the sidecar's -//! own shared-secret auth middleware does not run on `/mcp/*` (mounted before -//! it in `scripts/sidecar/app.ts`). +//! Trust model: the sidecar's blanket shared-secret middleware does not run on +//! `/mcp/*` (mounted before it in `scripts/sidecar/app.ts`), because +//! `Authorization` already carries the end user's delegate key. Instead this +//! module presents the same secret in `x-memwal-internal-sidecar-token`, and +//! the sidecar refuses to honour any `x-memwal-internal-*` header without it — +//! so reaching the sidecar directly is not enough to forge one (GH #685). use std::collections::HashMap; use std::net::SocketAddr; @@ -157,36 +159,83 @@ async fn classify_and_resolve(state: &AppState, headers: &HeaderMap) -> McpAuthO } } -/// Overwrite (never merge) `authorization` + `x-memwal-account-id` on the -/// outbound headers. This MUST be an overwrite: `build_forwarded_headers` -/// already copies any client-supplied `x-memwal-*` header verbatim (that's -/// how the legacy explicit-header flow works), so a caller presenting a -/// valid OAuth token alongside a forged `X-MemWal-Account-Id` must have the -/// forged value discarded, not merged with the token's real account. -fn apply_oauth_headers( +/// Internal headers the relayer states on every forwarded `/mcp/*` request. +/// +/// Both values are written with `insert` (overwrite, never append), so a +/// client-supplied `x-memwal-internal-*` copied through by +/// `build_forwarded_headers` is always replaced and can never survive. GH #665 +/// additionally drops that prefix on the way in; this function does not depend +/// on it. +/// +/// - **sidecar token** proves to the sidecar that the request really came from +/// the relayer. `/mcp/*` is mounted before the sidecar's shared-secret +/// middleware because `authorization` already carries the end user's +/// delegate key, so the secret rides in its own header instead. +/// - **oauth scope** is stated explicitly on BOTH auth paths — the resolved +/// grant for OAuth callers, and full read+write for legacy delegate-key +/// callers. The sidecar registers no tools when it is absent, so silence +/// means "no access" rather than "unrestricted" (GH #685). +/// +/// For the OAuth path this MUST overwrite `authorization` and +/// `x-memwal-account-id`: `build_forwarded_headers` copies any client-supplied +/// `x-memwal-*` header verbatim (that's how the legacy explicit-header flow +/// works), so a caller presenting a valid OAuth token alongside a forged +/// `X-MemWal-Account-Id` must have the forged value discarded, not merged. +/// +/// Returns `Err` rather than skipping a header it cannot build: a partially +/// applied set would hand the sidecar an authenticated request with no scope, +/// and the whole point of #685 is that such a request must fail, not proceed. +fn apply_internal_headers( forwarded: &mut reqwest::header::HeaderMap, - identity: &crate::oauth::ResolvedOAuthIdentity, -) { - if let Ok(v) = reqwest::header::HeaderValue::from_str(&format!( - "Bearer {}", - identity.delegate_private_key.as_str() - )) { - forwarded.insert(reqwest::header::AUTHORIZATION, v); - } - if let (Ok(name), Ok(v)) = ( - reqwest::header::HeaderName::from_bytes(b"x-memwal-account-id"), - reqwest::header::HeaderValue::from_str(&identity.account_id), - ) { - forwarded.insert(name, v); - } - // This header is never copied from inbound traffic. Only the relayer can - // add it on the loopback request after resolving a valid OAuth token. - if let (Ok(name), Ok(v)) = ( - reqwest::header::HeaderName::from_bytes(b"x-memwal-internal-oauth-scope"), - reqwest::header::HeaderValue::from_str(&identity.scope), - ) { - forwarded.insert(name, v); + sidecar_secret: Option<&str>, + identity: Option<&crate::oauth::ResolvedOAuthIdentity>, +) -> Result<(), StatusCode> { + fn internal_error(what: &str) -> StatusCode { + tracing::error!("mcp_proxy: cannot build internal header {what}"); + StatusCode::INTERNAL_SERVER_ERROR + } + + let Some(secret) = sidecar_secret else { + return Err(internal_error( + "x-memwal-internal-sidecar-token (SIDECAR_AUTH_TOKEN unset)", + )); + }; + let token = reqwest::header::HeaderValue::from_str(secret) + .map_err(|_| internal_error("x-memwal-internal-sidecar-token"))?; + + let scope = match identity { + Some(identity) => identity.scope.clone(), + // Legacy delegate-key callers have no OAuth grant, so the relayer says + // outright that they hold every tool scope. + None => format!("{} {}", crate::oauth::SCOPE_READ, crate::oauth::SCOPE_WRITE), + }; + let scope = reqwest::header::HeaderValue::from_str(&scope) + .map_err(|_| internal_error("x-memwal-internal-oauth-scope"))?; + + if let Some(identity) = identity { + let auth = reqwest::header::HeaderValue::from_str(&format!( + "Bearer {}", + identity.delegate_private_key.as_str() + )) + .map_err(|_| internal_error("authorization"))?; + let account = reqwest::header::HeaderValue::from_str(&identity.account_id) + .map_err(|_| internal_error("x-memwal-account-id"))?; + forwarded.insert(reqwest::header::AUTHORIZATION, auth); + forwarded.insert( + reqwest::header::HeaderName::from_static("x-memwal-account-id"), + account, + ); } + + forwarded.insert( + reqwest::header::HeaderName::from_static("x-memwal-internal-sidecar-token"), + token, + ); + forwarded.insert( + reqwest::header::HeaderName::from_static("x-memwal-internal-oauth-scope"), + scope, + ); + Ok(()) } /// RFC 9728 401 challenge. `state.config.mcp_oauth` must be `Some` — only @@ -238,12 +287,19 @@ pub async fn sse_proxy( peer, state.config.trusted_proxy_hops, ); - match classify_and_resolve(&state, &headers).await { - McpAuthOutcome::Passthrough => {} - McpAuthOutcome::Oauth(identity) => apply_oauth_headers(&mut forwarded, &identity), + let identity = match classify_and_resolve(&state, &headers).await { + McpAuthOutcome::Passthrough => None, + McpAuthOutcome::Oauth(identity) => Some(identity), McpAuthOutcome::Unauthorized(err) => { return oauth_unauthorized_response(&state, err.as_ref()) } + }; + if let Err(code) = apply_internal_headers( + &mut forwarded, + state.config.sidecar_secret.as_deref(), + identity.as_deref(), + ) { + return (code, "internal error").into_response(); } let req = state .http_client @@ -341,12 +397,19 @@ pub async fn messages_proxy( peer, state.config.trusted_proxy_hops, ); - match classify_and_resolve(&state, &headers).await { - McpAuthOutcome::Passthrough => {} - McpAuthOutcome::Oauth(identity) => apply_oauth_headers(&mut forwarded, &identity), + let identity = match classify_and_resolve(&state, &headers).await { + McpAuthOutcome::Passthrough => None, + McpAuthOutcome::Oauth(identity) => Some(identity), McpAuthOutcome::Unauthorized(err) => { return oauth_unauthorized_response(&state, err.as_ref()) } + }; + if let Err(code) = apply_internal_headers( + &mut forwarded, + state.config.sidecar_secret.as_deref(), + identity.as_deref(), + ) { + return (code, "internal error").into_response(); } let upstream = state .http_client @@ -451,12 +514,19 @@ pub async fn streamable_proxy( peer, state.config.trusted_proxy_hops, ); - match classify_and_resolve(&state, &headers).await { - McpAuthOutcome::Passthrough => {} - McpAuthOutcome::Oauth(identity) => apply_oauth_headers(&mut forwarded, &identity), + let identity = match classify_and_resolve(&state, &headers).await { + McpAuthOutcome::Passthrough => None, + McpAuthOutcome::Oauth(identity) => Some(identity), McpAuthOutcome::Unauthorized(err) => { return oauth_unauthorized_response(&state, err.as_ref()) } + }; + if let Err(code) = apply_internal_headers( + &mut forwarded, + state.config.sidecar_secret.as_deref(), + identity.as_deref(), + ) { + return (code, "internal error").into_response(); } req = req.headers(forwarded); @@ -655,8 +725,97 @@ mod tests { assert!(!is_legacy_delegate_bearer(&"a".repeat(65))); // one long } + // -- internal relayer->sidecar headers (GH #685) ---------------------- + + fn test_identity(scope: &str) -> crate::oauth::ResolvedOAuthIdentity { + let key = [3u8; 32]; + let envelope = crate::oauth::encrypt_delegate_private_key(&key, &"b".repeat(64)).unwrap(); + let secret = crate::oauth::decrypt_delegate_private_key(&key, &envelope).unwrap(); + crate::oauth::ResolvedOAuthIdentity { + account_id: "0xrealaccount".to_string(), + delegate_private_key: secret, + grant_id: "mwg_test".to_string(), + scope: scope.to_string(), + } + } + + #[test] + fn internal_headers_grant_full_scope_to_legacy_passthrough() { + let mut out = reqwest::header::HeaderMap::new(); + + apply_internal_headers(&mut out, Some("shhh"), None).expect("passthrough must succeed"); + + assert_eq!( + out.get("x-memwal-internal-oauth-scope") + .and_then(|v| v.to_str().ok()), + Some("memwal:read memwal:write"), + "legacy callers must be granted read+write explicitly, not by omission" + ); + assert_eq!( + out.get("x-memwal-internal-sidecar-token") + .and_then(|v| v.to_str().ok()), + Some("shhh") + ); + } + + #[test] + fn internal_headers_forward_the_resolved_oauth_scope() { + let mut out = reqwest::header::HeaderMap::new(); + let identity = test_identity("memwal:read"); + + apply_internal_headers(&mut out, Some("shhh"), Some(&identity)) + .expect("oauth must succeed"); + + assert_eq!( + out.get("x-memwal-internal-oauth-scope") + .and_then(|v| v.to_str().ok()), + Some("memwal:read"), + "the sidecar relies on this header being set; nothing else asserts it" + ); + assert_eq!( + out.get("x-memwal-internal-sidecar-token") + .and_then(|v| v.to_str().ok()), + Some("shhh") + ); + } + + #[test] + fn internal_headers_overwrite_client_supplied_values() { + let mut out = build_forwarded_headers(&axum_headers(&[ + ("x-memwal-internal-oauth-scope", "memwal:write"), + ("x-memwal-internal-sidecar-token", "guessed"), + ])); + + apply_internal_headers(&mut out, Some("shhh"), None).unwrap(); + + assert_eq!( + out.get("x-memwal-internal-oauth-scope") + .and_then(|v| v.to_str().ok()), + Some("memwal:read memwal:write") + ); + assert_eq!( + out.get("x-memwal-internal-sidecar-token") + .and_then(|v| v.to_str().ok()), + Some("shhh") + ); + } + + #[test] + fn internal_headers_fail_closed_without_a_sidecar_secret() { + let mut out = reqwest::header::HeaderMap::new(); + + assert!( + apply_internal_headers(&mut out, None, None).is_err(), + "no shared secret must fail the request, not forward unauthenticated" + ); + assert!( + out.get("x-memwal-internal-oauth-scope").is_none(), + "must not leave partial headers behind on failure" + ); + } + #[test] - fn apply_oauth_headers_overwrites_forwarded_authorization_and_account_id() { + fn apply_internal_headers_overwrites_forwarded_authorization_and_account_id() { // Simulates the case build_forwarded_headers already copied a // client-supplied (potentially forged) x-memwal-account-id — the // OAuth resolution must win, not merge. @@ -680,7 +839,7 @@ mod tests { scope: "memwal:read".to_string(), }; - apply_oauth_headers(&mut forwarded, &identity); + apply_internal_headers(&mut forwarded, Some("shhh"), Some(&identity)).unwrap(); assert_eq!( forwarded.get("authorization").and_then(|v| v.to_str().ok()),