fix(security): bind MCP memory tools to the authenticated caller (IDOR)

resolveMemoryOwnerId() let a caller-supplied `apiKeyId` win over the resolved
caller principal, so any MCP caller could read/write/delete another principal's
memories by putting a different id in the tool arguments. The resolved caller
(HTTP auth headers on SSE/Streamable HTTP, OMNIROUTE_API_KEY on stdio) now wins;
the explicit argument is only honored as a fallback when no caller can be
resolved (a bare local stdio process, already trusted).

Reported by @rafaelfiguereod-stack via GHSA-cpv3-xr7r-xf8q.
This commit is contained in:
Xiangzhe
2026-08-21 13:49:45 -03:00
parent efece42fec
commit c46f048ca2
2 changed files with 49 additions and 7 deletions

View File

@@ -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<string> {
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({

View File

@@ -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;
}
});