diff --git a/open-sse/handlers/chatCore.ts b/open-sse/handlers/chatCore.ts index 495f25d486..e0616946e5 100644 --- a/open-sse/handlers/chatCore.ts +++ b/open-sse/handlers/chatCore.ts @@ -2984,7 +2984,10 @@ export async function handleChatCore({ const dedupRequestBody = { ...translatedBody, model: `${provider}/${model}`, stream }; const dedupEnabled = shouldDeduplicate(dedupRequestBody); - const dedupHash = dedupEnabled ? computeRequestHash(dedupRequestBody) : null; + // Namespaced by the calling API key: dedup hands the SAME response object to + // every joiner, so a shared hash across keys is a cross-principal response + // leak (GHSA-6c7w-56xp-wpc6). + const dedupHash = dedupEnabled ? computeRequestHash(dedupRequestBody, apiKeyInfo?.id) : null; const executeProviderRequest = async (modelToCall = effectiveModel, allowDedup = false) => { const execute = async () => { diff --git a/open-sse/services/requestDedup.ts b/open-sse/services/requestDedup.ts index 1a39216197..b09b501164 100644 --- a/open-sse/services/requestDedup.ts +++ b/open-sse/services/requestDedup.ts @@ -129,8 +129,26 @@ function extractSystemContent(body: Record): unknown { * `translatedBody`), so the body shape here is whatever the target provider * format produced — see `extractPromptContent`/`extractSystemContent` for the * full list of shapes this must cover (#10249, #10438). + * + * `tenantId` (the calling API key's id) namespaces the hash. Dedup shares ONE + * upstream call, and therefore one response, between everyone landing on the + * same hash — so the hash has to answer "who is asking", not just "what is + * being asked". Without it, two distinct API keys issuing the same request + * joined the same in-flight promise: the response was produced with the + * initiator's provider connection, under the initiator's per-key policy + * (allowedConnections / allowedModels), billed to the initiator, and handed to + * a different authenticated principal (GHSA-6c7w-56xp-wpc6). + * + * It is a PLAINTEXT prefix rather than digest input, matching + * `semanticCache.generateSignature` (#3740): the id is an internal namespace + * key, not a credential, and keeping it out of the digest avoids the + * false-positive CodeQL js/insufficient-password-hash on a cache/dedup key. + * + * Omitting `tenantId` keeps the un-namespaced hash. Keyless local-first + * deployments have no tenant boundary to preserve, and every such install would + * otherwise silently lose dedup. */ -export function computeRequestHash(requestBody: unknown): string { +export function computeRequestHash(requestBody: unknown, tenantId?: string | null): string { const body = requestBody as Record; const canonical = { model: body.model ?? null, @@ -145,7 +163,8 @@ export function computeRequestHash(requestBody: unknown): string { frequency_penalty: body.frequency_penalty ?? null, presence_penalty: body.presence_penalty ?? null, }; - return createHash("sha256").update(JSON.stringify(canonical)).digest("hex").slice(0, 16); + const digest = createHash("sha256").update(JSON.stringify(canonical)).digest("hex").slice(0, 16); + return tenantId ? `${tenantId}.${digest}` : digest; } /** Determine whether a request should be deduplicated */ diff --git a/tests/unit/request-dedup-tenant-isolation.test.ts b/tests/unit/request-dedup-tenant-isolation.test.ts new file mode 100644 index 0000000000..15f4e5b10f --- /dev/null +++ b/tests/unit/request-dedup-tenant-isolation.test.ts @@ -0,0 +1,97 @@ +import { test } from "node:test"; +import assert from "node:assert/strict"; +import { + computeRequestHash, + deduplicate, + clearInflight, +} from "../../open-sse/services/requestDedup.ts"; + +// GHSA-6c7w-56xp-wpc6 follow-up. The dedup layer shares ONE upstream call — and +// therefore one response object — between every concurrent caller landing on the +// same hash. The hash canonicalized model + prompt + sampling params and nothing +// about WHO was asking, so two distinct OmniRoute API keys issuing the same +// request joined the same in-flight promise: the response was produced with the +// initiator's provider connection, under the initiator's per-key policy, and +// handed to a different authenticated principal. +// +// #10438 fixed the *prompt* half of this class (translated bodies hashing the +// prompt as `null`). This is the *identity* half. + +const body = { + messages: [{ role: "user", content: "Summarize the incident report." }], + temperature: 0, + model: "codex/gpt-5.6-terra", + stream: false, +}; + +test("the same request from two different API keys does NOT share a dedup hash", () => { + const hashKey1 = computeRequestHash(body, "apikey-1"); + const hashKey2 = computeRequestHash(body, "apikey-2"); + assert.notEqual( + hashKey1, + hashKey2, + "an identical request from a different API key must not join the first key's in-flight call" + ); +}); + +test("the same request from the same API key still dedups", () => { + assert.equal(computeRequestHash(body, "apikey-1"), computeRequestHash(body, "apikey-1")); +}); + +test("concurrent identical requests on two keys each get their own response", async () => { + clearInflight(); + const hash1 = computeRequestHash(body, "apikey-1"); + const hash2 = computeRequestHash(body, "apikey-2"); + + let released!: () => void; + const gate = new Promise((resolve) => { + released = resolve; + }); + + const first = deduplicate(hash1, async () => { + await gate; + return "RESPONSE_FOR_KEY_1"; + }); + const second = deduplicate(hash2, async () => { + await gate; + return "RESPONSE_FOR_KEY_2"; + }); + released(); + + const [a, b] = await Promise.all([first, second]); + assert.equal(a.result, "RESPONSE_FOR_KEY_1"); + assert.equal(b.result, "RESPONSE_FOR_KEY_2"); + assert.equal(b.wasDeduplicated, false, "key 2 must not have joined key 1's in-flight call"); +}); + +test("an unkeyed (anonymous) caller keeps the un-namespaced hash", () => { + // Keyless local-first deployments have no tenant boundary to preserve, so the + // behaviour there is unchanged — and must stay stable, or every such install + // silently loses dedup. + const anonymous = computeRequestHash(body); + assert.equal(computeRequestHash(body, undefined), anonymous); + assert.equal(computeRequestHash(body, null as unknown as undefined), anonymous); + assert.notEqual(computeRequestHash(body, "apikey-1"), anonymous); +}); + +test("the tenant namespace cannot be forged by a colliding request body", () => { + // The namespace is a plaintext prefix, so it must not be possible to move + // between namespaces by crafting a body — the separator has to survive. + const h1 = computeRequestHash(body, "a"); + const h2 = computeRequestHash(body, "a.b"); + assert.notEqual(h1, h2); + assert.ok(h1.startsWith("a."), "namespace must prefix the digest"); +}); + +test("chatCore passes the caller's API key id into the dedup hash", async () => { + const { readFileSync } = await import("node:fs"); + const { fileURLToPath } = await import("node:url"); + const source = readFileSync( + fileURLToPath(new URL("../../open-sse/handlers/chatCore.ts", import.meta.url)), + "utf8" + ); + assert.ok( + /computeRequestHash\(\s*dedupRequestBody\s*,\s*apiKeyInfo\?\.id/.test(source), + "the dedup hash must be namespaced by the calling API key (GHSA-6c7w-56xp-wpc6)" + ); +});