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 (#11649)
Merged via /merge-batch (2026-08-26, v3.8.51). Boarded no worktree combinado; validação única: typecheck/complexity/cognitive-complexity/file-size/changelog verdes, lint nos mesmos 228 achados pré-existentes confirmados contra o tip puro, testes focados passando. Obrigado pela contribuição.
This commit is contained in:
committed by
GitHub
parent
756b47fdd7
commit
ff25849530
@@ -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