mirror of
https://github.com/diegosouzapw/OmniRoute.git
synced 2026-08-21 22:52:19 +03:00
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:
@@ -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({
|
||||
|
||||
@@ -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;
|
||||
}
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user