mirror of
https://github.com/diegosouzapw/OmniRoute.git
synced 2026-08-27 01:22:10 +03:00
fix(security): namespace the dedup hash by the calling API key
Follow-up to GHSA-6c7w-56xp-wpc6. `deduplicate()` shares ONE upstream call — and therefore one response object — between every concurrent caller landing on the same hash. `computeRequestHash()` canonicalized model, prompt, system and sampling params: what was being asked, but never 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 (allowedConnections / allowedModels / disableNonPublicModels), billed to the initiator, and handed to a different authenticated principal. Dedup is an optimization; it must not be a hole in the auth boundary. #10438 fixed the *prompt* half of this class — translated bodies whose prompt lives under `input`/`contents` hashed as `null`, so different prompts collided. This is the *identity* half, which that fix did not touch. The tenant id is a PLAINTEXT prefix rather than digest input, matching semanticCache.generateSignature (#3740): it is an internal namespace key, not a credential, and keeping it out of the digest avoids a false-positive CodeQL js/insufficient-password-hash on a dedup key. Omitting it keeps the un-namespaced hash: keyless local-first deployments have no tenant boundary to preserve and would otherwise silently lose dedup. That is pinned by a test, not left to chance. tests/unit/request-dedup-tenant-isolation.test.ts — 6 tests, 5 red before the fix: two keys do not share a hash, the same key still dedups, two concurrent identical requests each get their own response, the anonymous hash is unchanged, the namespace prefix cannot be forged from the body, and chatCore actually passes the key id through.
This commit is contained in:
@@ -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 () => {
|
||||
|
||||
@@ -129,8 +129,26 @@ function extractSystemContent(body: Record<string, unknown>): 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<string, unknown>;
|
||||
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 */
|
||||
|
||||
97
tests/unit/request-dedup-tenant-isolation.test.ts
Normal file
97
tests/unit/request-dedup-tenant-isolation.test.ts
Normal file
@@ -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<void>((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)"
|
||||
);
|
||||
});
|
||||
Reference in New Issue
Block a user