diff --git a/open-sse/mcp-server/tools/memoryTools.ts b/open-sse/mcp-server/tools/memoryTools.ts index 16c835fd8e..12a8c99c0d 100644 --- a/open-sse/mcp-server/tools/memoryTools.ts +++ b/open-sse/mcp-server/tools/memoryTools.ts @@ -10,16 +10,24 @@ import { import { resolveMcpCallerApiKeyId } from "../mcpCallerIdentity.ts"; /** - * Resolve the memory owner id for an MCP tool call: - * explicit arg wins, otherwise fall back to the authenticated caller's - * principal id (HTTP auth headers on SSE/Streamable HTTP transports, - * OMNIROUTE_API_KEY env var on stdio). Keeps MCP-stored memories under - * the same owner id that chat-context memory uses, so retrieval in the - * chat pipeline finds entries written via MCP. + * Resolve the memory owner id for an MCP tool call. + * + * The authenticated caller's principal ALWAYS wins over a caller-supplied + * `apiKeyId` — otherwise any MCP caller could read, write, or delete another + * principal's memories by putting a different id in the tool arguments + * (GHSA-cpv3-xr7r-xf8q, IDOR). The caller is resolved from the per-request HTTP + * auth headers on SSE / Streamable HTTP transports, or from OMNIROUTE_API_KEY on + * stdio. The explicit argument is only honored as a fallback when no caller can + * be resolved (a bare local stdio process with no configured key — already + * trusted), preserving the local-tooling flow. Keeps MCP-stored memories under + * the same owner id that chat-context memory uses, so retrieval in the chat + * pipeline finds entries written via MCP. */ async function resolveMemoryOwnerId(explicit?: string): Promise { + const caller = await resolveMcpCallerApiKeyId().catch(() => undefined); + if (caller) return caller; if (explicit && explicit.trim() !== "") return explicit.trim(); - return (await resolveMcpCallerApiKeyId().catch(() => undefined)) || "mcp"; + return "mcp"; } export const MemorySearchSchema = z.object({ diff --git a/tests/unit/mcp-memory-tools-strategy.test.ts b/tests/unit/mcp-memory-tools-strategy.test.ts index a0db428663..ba740797ca 100644 --- a/tests/unit/mcp-memory-tools-strategy.test.ts +++ b/tests/unit/mcp-memory-tools-strategy.test.ts @@ -187,3 +187,37 @@ test("omniroute_memory_search: hardcoded fallback config has retrievalStrategy=e "fallback from catch path must use retrievalStrategy=exact" ); }); + +// ── IDOR: the authenticated caller's principal must win over a caller-supplied +// apiKeyId (GHSA-cpv3-xr7r-xf8q). With a resolvable caller (here: OMNIROUTE_API_KEY +// on the stdio path → "env-key"), omniroute_memory_add must store under the +// caller, NOT under the arbitrary apiKeyId in the tool arguments. +test("omniroute_memory_add: caller principal wins over a spoofed apiKeyId (GHSA-cpv3)", async () => { + const db = core.getDbInstance(); + const prevEnvKey = process.env.OMNIROUTE_API_KEY; + process.env.OMNIROUTE_API_KEY = "test-mcp-caller-key"; + try { + const { memoryTools } = await import("../../open-sse/mcp-server/tools/memoryTools.ts"); + const result = await memoryTools.omniroute_memory_add.handler({ + apiKeyId: "victim-b", + type: "factual", + key: "idor-k1", + content: "owned-by-caller", + }); + assert.equal(result.success, true, "add must succeed"); + + const rows = db + .prepare("SELECT api_key_id FROM memories WHERE key = 'idor-k1'") + .all() as Array<{ api_key_id: string }>; + assert.equal(rows.length, 1, "exactly one memory row expected"); + assert.equal( + rows[0].api_key_id, + "env-key", + "memory must be stored under the resolved caller (env-key), not the spoofed apiKeyId" + ); + assert.notEqual(rows[0].api_key_id, "victim-b", "must NOT store under the caller-supplied id"); + } finally { + if (prevEnvKey === undefined) delete process.env.OMNIROUTE_API_KEY; + else process.env.OMNIROUTE_API_KEY = prevEnvKey; + } +});