mirror of
https://github.com/diegosouzapw/OmniRoute.git
synced 2026-08-27 09:32:11 +03:00
Compare commits
1 Commits
fix/dedup-
...
fix/11433-
| Author | SHA1 | Date | |
|---|---|---|---|
|
|
2981cdb5a9 |
1
changelog.d/fixes/11433-opencode-alias-collision.md
Normal file
1
changelog.d/fixes/11433-opencode-alias-collision.md
Normal file
@@ -0,0 +1 @@
|
||||
- **fix(combos):** the combo builder's precision-select, global-model-search, and manual-entry flows now serialize a model step's `model` string using the provider's already-computed routing-alias prefix (e.g. `oc/`) instead of rebuilding it from the raw canonical `providerId`, fixing the no-auth "OpenCode Free" provider (`opencode`) being routed to the unrelated paid "OpenCode Zen" provider (`opencode-zen`) because `opencode` doubles as a manual routing-prefix override ([#11433](https://github.com/diegosouzapw/OmniRoute/issues/11433)).
|
||||
@@ -2984,10 +2984,7 @@ export async function handleChatCore({
|
||||
|
||||
const dedupRequestBody = { ...translatedBody, model: `${provider}/${model}`, stream };
|
||||
const dedupEnabled = shouldDeduplicate(dedupRequestBody);
|
||||
// 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 dedupHash = dedupEnabled ? computeRequestHash(dedupRequestBody) : null;
|
||||
|
||||
const executeProviderRequest = async (modelToCall = effectiveModel, allowDedup = false) => {
|
||||
const execute = async () => {
|
||||
|
||||
@@ -129,26 +129,8 @@ 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, tenantId?: string | null): string {
|
||||
export function computeRequestHash(requestBody: unknown): string {
|
||||
const body = requestBody as Record<string, unknown>;
|
||||
const canonical = {
|
||||
model: body.model ?? null,
|
||||
@@ -163,8 +145,7 @@ export function computeRequestHash(requestBody: unknown, tenantId?: string | nul
|
||||
frequency_penalty: body.frequency_penalty ?? null,
|
||||
presence_penalty: body.presence_penalty ?? null,
|
||||
};
|
||||
const digest = createHash("sha256").update(JSON.stringify(canonical)).digest("hex").slice(0, 16);
|
||||
return tenantId ? `${tenantId}.${digest}` : digest;
|
||||
return createHash("sha256").update(JSON.stringify(canonical)).digest("hex").slice(0, 16);
|
||||
}
|
||||
|
||||
/** Determine whether a request should be deduplicated */
|
||||
|
||||
@@ -2177,6 +2177,9 @@ function ComboFormModal({ isOpen, combo, onClose, onSave, activeProviders, combo
|
||||
builderConnectionId !== COMBO_BUILDER_AUTO_CONNECTION ? builderConnectionId : null,
|
||||
connectionLabel: selectedBuilderConnection?.label || null,
|
||||
allowedConnectionIds: builderEffectiveAllowedConnectionIds,
|
||||
// #11433: use the already-corrected routing prefix (e.g. "oc" for
|
||||
// OpenCode Free) instead of letting it default to the raw providerId.
|
||||
modelPrefix: parseQualifiedModel(selectedBuilderModel.qualifiedModel)?.providerId,
|
||||
})
|
||||
: null;
|
||||
const builderHasDuplicate =
|
||||
@@ -2501,6 +2504,9 @@ function ComboFormModal({ isOpen, combo, onClose, onSave, activeProviders, combo
|
||||
builderConnectionId !== COMBO_BUILDER_AUTO_CONNECTION ? builderConnectionId : null,
|
||||
connectionLabel: selectedBuilderConnection?.label || null,
|
||||
allowedConnectionIds: builderEffectiveAllowedConnectionIds,
|
||||
// #11433: use the already-corrected routing prefix (e.g. "oc" for
|
||||
// OpenCode Free) instead of letting it default to the raw providerId.
|
||||
modelPrefix: parseQualifiedModel(selectedBuilderModel.qualifiedModel)?.providerId,
|
||||
});
|
||||
|
||||
if (hasExactModelStepDuplicate(models, nextStep)) {
|
||||
|
||||
@@ -83,6 +83,7 @@ export function buildPrecisionComboModelStep({
|
||||
connectionLabel,
|
||||
allowedConnectionIds = null,
|
||||
weight = 0,
|
||||
modelPrefix,
|
||||
}: {
|
||||
providerId: string;
|
||||
modelId: string;
|
||||
@@ -91,9 +92,22 @@ export function buildPrecisionComboModelStep({
|
||||
/** #3266: account allowlist scoping round-robin to a subset of connections. */
|
||||
allowedConnectionIds?: string[] | null;
|
||||
weight?: number;
|
||||
/**
|
||||
* #11433: the routing-prefix segment to serialize into `model` (e.g. "oc"
|
||||
* for the no-auth OpenCode Free provider), when it differs from the
|
||||
* canonical `providerId`. Some canonical provider ids collide with an
|
||||
* unrelated manual `ALIAS_TO_PROVIDER_ID` routing override (`opencode` →
|
||||
* `opencode-zen`), so reconstructing `model` from the raw `providerId`
|
||||
* alone can round-trip to the wrong provider on request routing. Falls
|
||||
* back to `providerId` when omitted/blank. `step.providerId` always stays
|
||||
* the canonical id regardless, so routing/duplicate-detection identity is
|
||||
* unaffected.
|
||||
*/
|
||||
modelPrefix?: string | null;
|
||||
}): ComboModelStep {
|
||||
const normalizedProviderId = toTrimmedString(providerId) || "provider";
|
||||
const normalizedModelId = toTrimmedString(modelId) || "model";
|
||||
const normalizedModelPrefix = toTrimmedString(modelPrefix) || normalizedProviderId;
|
||||
const normalizedConnectionId = toTrimmedString(connectionId);
|
||||
const normalizedConnectionLabel = toTrimmedString(connectionLabel);
|
||||
// A pinned single connection wins over an allowlist, so only carry the allowlist
|
||||
@@ -110,7 +124,7 @@ export function buildPrecisionComboModelStep({
|
||||
return {
|
||||
kind: "model",
|
||||
providerId: normalizedProviderId,
|
||||
model: `${normalizedProviderId}/${normalizedModelId}`,
|
||||
model: `${normalizedModelPrefix}/${normalizedModelId}`,
|
||||
...(normalizedConnectionId ? { connectionId: normalizedConnectionId } : {}),
|
||||
...(normalizedConnectionLabel ? { label: normalizedConnectionLabel } : {}),
|
||||
...(normalizedAllowed.length > 0 ? { allowedConnectionIds: normalizedAllowed } : {}),
|
||||
@@ -160,10 +174,15 @@ export function buildManualComboModelStep({
|
||||
const providerId = resolveComboBuilderProviderId(parsed.providerId, providers);
|
||||
if (!providerId) return null;
|
||||
|
||||
// #11433: preserve the user-typed prefix (e.g. "oc") as the routing prefix
|
||||
// instead of letting buildPrecisionComboModelStep rebuild `model` from the
|
||||
// resolved canonical providerId, which can collide with an unrelated
|
||||
// manual alias override (e.g. "opencode" -> "opencode-zen").
|
||||
return buildPrecisionComboModelStep({
|
||||
providerId,
|
||||
modelId: parsed.modelId,
|
||||
weight,
|
||||
modelPrefix: parsed.providerId,
|
||||
});
|
||||
}
|
||||
|
||||
@@ -225,7 +244,7 @@ type ComboBuilderGlobalProvider = {
|
||||
displayName?: unknown;
|
||||
connectionCount?: unknown;
|
||||
connections?: unknown[];
|
||||
models?: Array<{ id?: unknown; name?: unknown }>;
|
||||
models?: Array<{ id?: unknown; name?: unknown; qualifiedModel?: unknown }>;
|
||||
};
|
||||
|
||||
/**
|
||||
@@ -252,12 +271,18 @@ export function buildGlobalModelList(
|
||||
const modelId = toTrimmedString(model?.id);
|
||||
if (!modelId) return;
|
||||
const modelName = toTrimmedString(model?.name) || modelId;
|
||||
// #11433: derive the routing prefix from the model's already-corrected
|
||||
// `qualifiedModel` (e.g. "oc/<model>" for the OpenCode Free provider)
|
||||
// instead of defaulting to the raw providerId, which can collide with
|
||||
// an unrelated manual alias override.
|
||||
const modelPrefix = parseQualifiedModel(model?.qualifiedModel)?.providerId || providerId;
|
||||
const step = buildPrecisionComboModelStep({
|
||||
providerId,
|
||||
modelId,
|
||||
connectionId: null,
|
||||
connectionLabel: null,
|
||||
allowedConnectionIds: [],
|
||||
modelPrefix,
|
||||
});
|
||||
list.push({
|
||||
providerId,
|
||||
|
||||
@@ -42,6 +42,12 @@ test("buildPrecisionComboModelStep preserves provider/model/account triple", ()
|
||||
});
|
||||
|
||||
test("buildManualComboModelStep resolves provider aliases and uses dynamic account", () => {
|
||||
// #11433: `providerId` resolves to the canonical id ("codex") for
|
||||
// duplicate-detection/routing identity, but the serialized `model` string
|
||||
// now preserves the user-typed prefix ("cx/") verbatim instead of
|
||||
// collapsing back to the canonical id — some canonical ids (e.g.
|
||||
// "opencode") collide with an unrelated manual routing-alias override, so
|
||||
// rebuilding `model` from the canonical id alone can silently misroute.
|
||||
assert.deepEqual(
|
||||
builderDraft.buildManualComboModelStep({
|
||||
value: "cx/gpt-5.5",
|
||||
@@ -50,7 +56,7 @@ test("buildManualComboModelStep resolves provider aliases and uses dynamic accou
|
||||
{
|
||||
kind: "model",
|
||||
providerId: "codex",
|
||||
model: "codex/gpt-5.5",
|
||||
model: "cx/gpt-5.5",
|
||||
weight: 0,
|
||||
}
|
||||
);
|
||||
|
||||
83
tests/unit/issue-11433-opencode-alias-collision.test.ts
Normal file
83
tests/unit/issue-11433-opencode-alias-collision.test.ts
Normal file
@@ -0,0 +1,83 @@
|
||||
import { test } from "node:test";
|
||||
import assert from "node:assert/strict";
|
||||
|
||||
import {
|
||||
buildPrecisionComboModelStep,
|
||||
buildGlobalModelList,
|
||||
buildManualComboModelStep,
|
||||
} from "../../src/lib/combos/builderDraft.ts";
|
||||
import { resolveProviderAlias, parseModel } from "../../open-sse/services/model.ts";
|
||||
|
||||
// Issue #11433: the combo builder's precision-select path builds a step's
|
||||
// `model` string as `${providerId}/${modelId}` using the CANONICAL provider id.
|
||||
// For the no-auth "opencode" (OpenCode Free) provider this produces
|
||||
// `model: "opencode/<modelId>"`, but `opencode` is ALSO a manual routing-prefix
|
||||
// override (`ALIAS_TO_PROVIDER_ID["opencode"] = "opencode-zen"`) intended only
|
||||
// for user-typed `opencode/` prefixes referring to the OpenCode Zen (api-key)
|
||||
// tier. Parsing the step's own `model` string therefore resolves to a
|
||||
// DIFFERENT provider than the one recorded in `step.providerId`.
|
||||
|
||||
test('sanity: resolveProviderAlias("opencode") is the manual override causing the collision', () => {
|
||||
// Documents the root cause directly: the manual alias override in
|
||||
// open-sse/services/model.ts unconditionally rewrites "opencode" to
|
||||
// "opencode-zen", even though "opencode" is also a registered canonical
|
||||
// provider id (src/shared/constants/providers/noauth.ts).
|
||||
assert.equal(resolveProviderAlias("opencode"), "opencode-zen");
|
||||
});
|
||||
|
||||
test("issue #11433 fix: buildPrecisionComboModelStep honors an explicit modelPrefix override", () => {
|
||||
// The combo builder call sites now thread through the already-computed
|
||||
// routing-alias prefix (e.g. "oc") instead of letting the step default to
|
||||
// the raw providerId, so the serialized `model` field round-trips to the
|
||||
// correct provider.
|
||||
const step = buildPrecisionComboModelStep({
|
||||
providerId: "opencode",
|
||||
modelId: "big-pickle",
|
||||
modelPrefix: "oc",
|
||||
});
|
||||
|
||||
assert.equal(step.providerId, "opencode");
|
||||
assert.equal(step.model, "oc/big-pickle");
|
||||
|
||||
const parsed = parseModel(step.model);
|
||||
assert.equal(parsed.provider, step.providerId);
|
||||
});
|
||||
|
||||
test("issue #11433 fix: buildGlobalModelList derives modelPrefix from qualifiedModel for the no-auth OpenCode Free provider", () => {
|
||||
// Mirrors what src/lib/combos/builderOptions.ts::rewriteQualifiedModelPrefix
|
||||
// produces for the no-auth "opencode" provider entry: `qualifiedModel` is
|
||||
// already rewritten to the "oc/" alias prefix, but (pre-fix)
|
||||
// buildGlobalModelList ignored it and rebuilt `model` from the raw
|
||||
// providerId, producing "opencode/big-pickle" which parses back to the
|
||||
// wrong provider ("opencode-zen").
|
||||
const [entry] = buildGlobalModelList([
|
||||
{
|
||||
providerId: "opencode",
|
||||
displayName: "OpenCode Free",
|
||||
connectionCount: 0,
|
||||
connections: [],
|
||||
models: [{ id: "big-pickle", name: "Big Pickle", qualifiedModel: "oc/big-pickle" }],
|
||||
},
|
||||
]);
|
||||
|
||||
assert.equal(entry.step.providerId, "opencode");
|
||||
assert.equal(entry.step.model, "oc/big-pickle");
|
||||
assert.equal(parseModel(entry.step.model).provider, entry.step.providerId);
|
||||
});
|
||||
|
||||
test("issue #11433 fix: buildManualComboModelStep preserves a user-typed oc/<model> prefix", () => {
|
||||
// buildManualComboModelStep resolves the typed alias ("oc") back to the
|
||||
// canonical providerId ("opencode") before building the step. Pre-fix, it
|
||||
// then handed that canonical id straight to buildPrecisionComboModelStep,
|
||||
// which rebuilt `model` from it and collapsed "oc/<model>" back down to
|
||||
// "opencode/<model>" — reproducing the same collision for manual entry.
|
||||
const step = buildManualComboModelStep({
|
||||
value: "oc/big-pickle",
|
||||
providers: [{ providerId: "opencode", alias: "oc" }],
|
||||
});
|
||||
|
||||
assert.ok(step);
|
||||
assert.equal(step?.providerId, "opencode");
|
||||
assert.equal(step?.model, "oc/big-pickle");
|
||||
assert.equal(parseModel(step!.model).provider, step!.providerId);
|
||||
});
|
||||
@@ -1,97 +0,0 @@
|
||||
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