diff --git a/changelog.d/fixes/vision-bridge-no-credentialed-hijack.md b/changelog.d/fixes/vision-bridge-no-credentialed-hijack.md new file mode 100644 index 0000000000..637d661ea8 --- /dev/null +++ b/changelog.d/fixes/vision-bridge-no-credentialed-hijack.md @@ -0,0 +1 @@ +- fix(guardrails/chat): do not whole-request-reroute Vision Bridge away from credentialed models (e.g. combo target zai/glm-5.2 or grok-cli → opencode-zen noauth 401); align body.model with X-Route-Model so post-guardrail cannot undo the routing header \ No newline at end of file diff --git a/src/lib/guardrails/visionBridge.ts b/src/lib/guardrails/visionBridge.ts index 4e5b45f966..1229b418b4 100644 --- a/src/lib/guardrails/visionBridge.ts +++ b/src/lib/guardrails/visionBridge.ts @@ -92,6 +92,79 @@ export interface VisionBridgeDependencies { ) => Promise; /** Override combo-target vision check — return true to force processing, false to skip. */ checkModelHasComboMapping?: (model: string) => Promise; + /** + * Whether a model string has a usable active credential (true/false). + * Return `null` when indeterminate (no DB / error) so callers can fail-open. + */ + hasUsableCredentials?: (model: string) => Promise; +} + +/** + * True when a provider connection can actually authenticate upstream. + * `noauth` with no real API key is NOT usable (opencode-zen free tier often + * surfaces as noauth and then 401 "Missing API key"). + */ +type ProviderConnectionLike = { + authType?: string | null; + apiKey?: string | null; + accessToken?: string | null; + refreshToken?: string | null; + idToken?: string | null; + testStatus?: string | null; +}; + +const TERMINAL_CONNECTION_STATUSES = new Set(["disabled", "banned", "expired"]); +// Free/noauth only counts when a real key is still present; apikey/cookie need the same. +const KEY_ONLY_AUTH_TYPES = new Set(["noauth", "none", "", "apikey", "cookie"]); +const TOKEN_AUTH_TYPES = new Set(["oauth", "access_token", "external_idp"]); + +function hasNonEmptyString(value: unknown): boolean { + return typeof value === "string" && value.trim().length > 0; +} + +function hasOAuthCredential(connection: ProviderConnectionLike): boolean { + return ( + hasNonEmptyString(connection.refreshToken) || + hasNonEmptyString(connection.accessToken) || + hasNonEmptyString(connection.idToken) + ); +} + +export function isProviderConnectionUsable(connection: ProviderConnectionLike): boolean { + const status = String(connection.testStatus || "").toLowerCase(); + if (TERMINAL_CONNECTION_STATUSES.has(status)) { + return false; + } + + const auth = String(connection.authType || "").toLowerCase(); + const hasKey = hasNonEmptyString(connection.apiKey); + + if (KEY_ONLY_AUTH_TYPES.has(auth)) { + return hasKey; + } + if (TOKEN_AUTH_TYPES.has(auth)) { + return hasOAuthCredential(connection) || hasKey; + } + return hasKey; +} + +/** + * Resolve whether `provider/model` has at least one usable active connection. + * Returns `null` when the credential store is unavailable (unit tests / early boot). + */ +export async function hasUsableCredentialsForModel(model: string): Promise { + const provider = typeof model === "string" ? model.split("/")[0]?.trim() : ""; + if (!provider) return null; + try { + const { getProviderConnections } = await import("@/lib/db/providers"); + const connections = await getProviderConnections({ provider, isActive: true }); + if (!Array.isArray(connections)) return null; + // Empty active set is a definitive "no" only when the table is readable. + if (connections.length === 0) return false; + return connections.some((c: any) => isProviderConnectionUsable(c)); + } catch { + return null; + } } export class VisionBridgeGuardrail extends BaseGuardrail { @@ -183,37 +256,66 @@ export class VisionBridgeGuardrail extends BaseGuardrail { return { block: false }; } - // 9. Individual non-combo model with images → REROUTE to best vision-capable model + // 9. Individual non-combo model with images → optionally REROUTE to best vision-capable model // instead of describing images through an intermediate vision call. - // This lets a downstream vision model process the image natively. + // + // CRITICAL (VibeProxy combo / explicit provider models): + // When the original model already has usable credentials (e.g. combo target + // zai/glm-5.2), NEVER whole-request-reroute to another provider. Auto-select + // prefers opencode-* (priority 0) even when only a broken noauth connection + // exists, which produced: HTTP log zai → Guardrail reroute → opencode-zen 401 + // "Missing API key" while the combo UI still showed body=zai. Fall through to + // the image-describe path so the user's chosen model still answers. if (comboVisionBridgeDecision === "not-combo" && !forceVisionBridge) { - // Honor an explicit operator override from the Vision Bridge settings tab - // (settings.visionBridgeModel) as the fixed reroute target, for consistency - // with the combo/describe path below (step 10) which always honors it via - // getVisionBridgeConfig. When unset, auto-select the fastest available - // vision-capable model from available providers. - const configuredModel = - typeof settings.visionBridgeModel === "string" && settings.visionBridgeModel.trim() - ? settings.visionBridgeModel.trim() - : undefined; - const bestModel = getBestVisionModel({ fixedModel: configuredModel }); - if (bestModel && bestModel !== model) { - const modifiedBody = { - ...(body as Record), - model: bestModel, - }; - return { - block: false, - modifiedPayload: modifiedBody as unknown, - meta: { - rerouted: true, - fromModel: model, - toModel: bestModel, - imagesKept: imageParts.length, - }, - }; + const checkCreds = + this.deps.hasUsableCredentials ?? hasUsableCredentialsForModel; + const originalUsable = await checkCreds(model); + + if (originalUsable === true) { + // Keep the credentialed model; describe images below if needed. + context.log?.debug?.( + "VISION_BRIDGE", + `Skipping whole-request vision reroute; keeping credentialed model ${model}` + ); + } else { + // Honor an explicit operator override from the Vision Bridge settings tab + // (settings.visionBridgeModel) as the fixed reroute target, for consistency + // with the combo/describe path below (step 10) which always honors it via + // getVisionBridgeConfig. When unset, auto-select the fastest available + // vision-capable model from available providers. + const configuredModel = + typeof settings.visionBridgeModel === "string" && settings.visionBridgeModel.trim() + ? settings.visionBridgeModel.trim() + : undefined; + const bestModel = getBestVisionModel({ fixedModel: configuredModel }); + if (bestModel && bestModel !== model) { + const bestUsable = await checkCreds(bestModel); + // Only block the reroute when we KNOW the target is unusable (false). + // `null` (no DB / tests) fails open so existing unit tests keep working. + if (bestUsable === false) { + context.log?.warn?.( + "VISION_BRIDGE", + `Vision reroute target ${bestModel} has no usable credentials; describing images instead of hijacking ${model}` + ); + } else { + const modifiedBody = { + ...(body as Record), + model: bestModel, + }; + return { + block: false, + modifiedPayload: modifiedBody as unknown, + meta: { + rerouted: true, + fromModel: model, + toModel: bestModel, + imagesKept: imageParts.length, + }, + }; + } + } } - // Fall through: if no vision model found, describe images as text instead + // Fall through: describe images as text (or no-op if describe path can't run) } // 10. Get configuration diff --git a/src/lib/guardrails/visionBridgeRouter.ts b/src/lib/guardrails/visionBridgeRouter.ts index 76b1331d05..caafda1fe0 100644 --- a/src/lib/guardrails/visionBridgeRouter.ts +++ b/src/lib/guardrails/visionBridgeRouter.ts @@ -110,12 +110,17 @@ function getVisionCapableModels(): VisionModelCandidate[] { const caps = getResolvedModelCapabilities(fullModelId); if (caps.supportsVision === true) { - // Determine priority based on provider type + // Determine priority based on provider type (lower = better). + // Do NOT prefer opencode-* first: those catalog entries often resolve to a + // noauth connection and 401 "Missing API key", hijacking working providers + // (e.g. zai/glm-5.2 combo targets) when Vision Bridge auto-reroutes. let priority = 100; - if (providerAlias.startsWith("opencode-")) { - priority = 0; // Local/free models first - } else if (providerAlias === "openai" || providerAlias === "anthropic") { - priority = 50; // Major providers + if (providerAlias === "openai" || providerAlias === "anthropic") { + priority = 50; // Major providers with real API keys + } else if (providerAlias === "vertex" || providerAlias === "gemini") { + priority = 55; + } else if (providerAlias.startsWith("opencode-")) { + priority = 95; // Free/catalog — only if nothing credentialed is available } else { priority = 75; // Other providers } diff --git a/src/sse/handlers/chat.ts b/src/sse/handlers/chat.ts index 3f63968159..2e9230322c 100644 --- a/src/sse/handlers/chat.ts +++ b/src/sse/handlers/chat.ts @@ -1,7 +1,7 @@ import { randomUUID } from "crypto"; import { resolveChatRequestBody } from "./requestBody"; import { normalizeReasoningRequest } from "@/shared/reasoning/effortStandardization"; -import { resolveRoutingModel } from "./resolveRoutingModel"; +import { resolveRoutingModel, RoutingModelOps } from "./resolveRoutingModel"; import { getProviderCredentialsWithQuotaPreflight, markAccountUnavailable, @@ -370,6 +370,8 @@ export async function handleChat( // resolveRoutingModel). The resolved model still passes through // enforceApiKeyPolicy below, so it cannot bypass per-key allowlists. let modelStr = resolveRoutingModel(request, body); + // Align body.model with the routing model immediately (see applyRoutingModelAlignment). + body = RoutingModelOps.align(body, modelStr, log); // Count messages (support both messages[] and input[] formats) const msgCount = body.messages?.length || body.input?.length || 0; @@ -472,24 +474,19 @@ export async function handleChat( preCallGuardrails.message || "Request rejected: suspicious content detected" ); } + // Snapshot model BEFORE the guardrail payload (see reconcileGuardrailReroute). + const modelBeforeGuardrails = + typeof body?.model === "string" && body.model.length > 0 ? body.model : modelStr; body = preCallGuardrails.payload; - if (body?.model && typeof body.model === "string" && body.model !== modelStr) { - const rerouteModel = body.model; - // A guardrail (e.g. Vision Bridge auto-reroute) can swap body.model AFTER - // enforceApiKeyPolicy already validated modelStr's allowlist/budget above. - // Re-check the new target against the same per-key allowlist so a - // policy-restricted key cannot be silently routed to an unchecked model. - const rerouteAllowed = await isModelAllowedForKey(apiKey, rerouteModel); - if (!rerouteAllowed) { - log.warn( - "POLICY", - `Guardrail reroute to "${rerouteModel}" rejected by API key policy (key=${apiKeyInfo?.id || "unknown"}); keeping original model "${modelStr}"` - ); - body = { ...body, model: modelStr }; - } else { - modelStr = rerouteModel; - } - } + ({ body, modelStr } = await RoutingModelOps.reconcileGuardrailReroute({ + body, + modelBeforeGuardrails, + modelStr, + apiKey, + apiKeyId: apiKeyInfo?.id, + isModelAllowedForKey, + log, + })); telemetry.endPhase(); // T08: per-key active session limit (0 = unlimited). @@ -530,9 +527,13 @@ export async function handleChat( // Apply hook mutations body = hookCtx.body as any; - if (hookCtx.model && hookCtx.model !== modelStr) { - modelStr = hookCtx.model; - } + ({ body, modelStr } = RoutingModelOps.reconcileModelOverride({ + body, + modelStr, + overrideModel: hookCtx.model, + logTag: "Hook model override", + log, + })); // Short-circuit if a hook returned a direct response if (hookResponse) { diff --git a/src/sse/handlers/resolveRoutingModel.ts b/src/sse/handlers/resolveRoutingModel.ts index c4c466b88e..347abcc08d 100644 --- a/src/sse/handlers/resolveRoutingModel.ts +++ b/src/sse/handlers/resolveRoutingModel.ts @@ -5,6 +5,12 @@ // proxy can send `X-Route-Model` to restore routing control without mutating the // request body. The resolved value still flows through `enforceApiKeyPolicy`, so // it cannot bypass per-key model/combo allowlists. See PR #4863. +// +// IMPORTANT: callers MUST then align `body.model` with the resolved value via +// `alignBodyModelWithRouting` (or equivalent). Otherwise the post-guardrail +// "body.model !== modelStr → adopt body.model" path silently undoes the header +// override and routes to the original body model (e.g. opencode-zen 401 while +// logs still show the X-Route-Model target like zai/glm-5.2). type HeaderCarrier = { headers: { get(name: string): string | null } }; @@ -15,3 +21,141 @@ export function resolveRoutingModel( const headerModel = request.headers.get("x-route-model")?.trim(); return headerModel || body.model; } + +/** + * Keep body.model in sync with the routing model after resolveRoutingModel. + * Returns the (possibly new) body object and whether body.model was rewritten. + */ +export function alignBodyModelWithRouting( + body: T, + modelStr: string | null | undefined +): { body: T; aligned: boolean; previousModel: string | null } { + const previousModel = typeof body?.model === "string" ? body.model : null; + if (!modelStr || typeof modelStr !== "string" || modelStr.length === 0) { + return { body, aligned: false, previousModel }; + } + if (previousModel === modelStr) { + return { body, aligned: false, previousModel }; + } + return { + body: { ...body, model: modelStr }, + aligned: true, + previousModel, + }; +} + +type RoutingLogger = { info: (tag: string, msg: string) => void }; + +/** + * Thin wrapper around alignBodyModelWithRouting that also emits the ROUTING + * log line, kept out of chat.ts so the caller stays a one-liner. + * + * Callers MUST run this immediately after resolveRoutingModel. Without it, + * the post-guardrail "body.model !== modelStr → adopt body.model" reconcile + * path (see reconcileGuardrailReroute below) treats a mismatched body.model + * as a guardrail reroute and silently restores it — undoing an X-Route-Model + * header override (e.g. header zai/glm-5.2 + body opencode-zen/gpt-5.4 → 401 + * Missing API key while HTTP logs still show zai). + */ +export function applyRoutingModelAlignment( + body: T, + modelStr: string | null | undefined, + log: RoutingLogger +): T { + const aligned = alignBodyModelWithRouting(body, modelStr); + if (aligned.aligned) { + log.info( + "ROUTING", + `Aligned body.model to routing model: ${aligned.previousModel || "(none)"} → ${modelStr}` + ); + } + return aligned.body; +} + +type GuardrailRerouteLogger = { + info: (tag: string, msg: string) => void; + warn: (tag: string, msg: string) => void; +}; + +/** + * Keep body.model glued to modelStr after a stage that may override modelStr + * (e.g. a pre-request hook) without necessarily rewriting body.model itself. + * Returns the (possibly new) body/modelStr and logs the override when the + * hook actually changed modelStr. + */ +export function reconcileModelOverride(params: { + body: T; + modelStr: string; + overrideModel: string | null | undefined; + logTag: string; + log: GuardrailRerouteLogger; +}): { body: T; modelStr: string } { + const { body, overrideModel, logTag, log } = params; + let { modelStr } = params; + + if (overrideModel && overrideModel !== modelStr) { + log.info("ROUTING", `${logTag}: ${modelStr} → ${overrideModel}`); + modelStr = overrideModel; + if (typeof body?.model !== "string" || body.model !== modelStr) { + return { body: { ...body, model: modelStr }, modelStr }; + } + return { body, modelStr }; + } + if (modelStr && typeof body?.model === "string" && body.model !== modelStr) { + // The stage rewrote body without updating model — restore the routing model. + return { body: { ...body, model: modelStr }, modelStr }; + } + return { body, modelStr }; +} + +/** + * Reconcile body.model after the pre-call guardrail pipeline runs. A guardrail + * (e.g. Vision Bridge auto-reroute) can swap body.model AFTER + * enforceApiKeyPolicy already validated modelStr's allowlist/budget, so any + * genuine change must be re-checked against the same per-key allowlist before + * being adopted as the new routing model. `modelBeforeGuardrails` must be a + * snapshot of body.model taken immediately before the guardrail payload was + * applied — comparing against a stale/aligned value would misclassify a + * legitimate X-Route-Model alignment as a guardrail reroute. + */ +export async function reconcileGuardrailReroute(params: { + body: T; + modelBeforeGuardrails: string; + modelStr: string; + apiKey: string | null | undefined; + apiKeyId: string | undefined; + isModelAllowedForKey: (apiKey: string | null | undefined, model: string) => Promise; + log: GuardrailRerouteLogger; +}): Promise<{ body: T; modelStr: string }> { + const { body, modelBeforeGuardrails, apiKey, apiKeyId, isModelAllowedForKey, log } = params; + let { modelStr } = params; + + if (body?.model && typeof body.model === "string" && body.model !== modelBeforeGuardrails) { + const rerouteModel = body.model; + const rerouteAllowed = await isModelAllowedForKey(apiKey, rerouteModel); + if (!rerouteAllowed) { + log.warn( + "POLICY", + `Guardrail reroute to "${rerouteModel}" rejected by API key policy (key=${apiKeyId || "unknown"}); keeping original model "${modelStr}"` + ); + return { body: { ...body, model: modelStr }, modelStr }; + } + log.info("ROUTING", `Guardrail model reroute: ${modelBeforeGuardrails} → ${rerouteModel}`); + return { body, modelStr: rerouteModel }; + } + // Guardrails returned a payload whose model drifted from modelStr without + // changing from the pre-guardrail value (should not happen after align), or + // stripped model — keep body.model glued to modelStr. + if (modelStr && typeof body?.model === "string" && body.model !== modelStr) { + return { body: { ...body, model: modelStr }, modelStr }; + } + return { body, modelStr }; +} + +// Grouped under one namespace so chat.ts needs a single extra import name +// alongside resolveRoutingModel — see each function's own doc comment above. +export const RoutingModelOps = { + align: applyRoutingModelAlignment, + reconcileGuardrailReroute, + reconcileModelOverride, +}; diff --git a/tests/unit/guardrails/vision-bridge-callmodel.test.ts b/tests/unit/guardrails/vision-bridge-callmodel.test.ts index 74b9ee6531..8408ba8be4 100644 --- a/tests/unit/guardrails/vision-bridge-callmodel.test.ts +++ b/tests/unit/guardrails/vision-bridge-callmodel.test.ts @@ -41,9 +41,13 @@ const TINY_PNG = "data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAf test("callVisionModel falls through to next model when primary fails", async () => { let fetchCallCount = 0; - const FALLBACK_RESPONSE = JSON.stringify({ - choices: [{ message: { content: "fallback model description" } }], - }); + // The fallback candidate can legitimately resolve to either an OpenAI-compatible + // model (POST .../chat/completions, { choices: [{ message: { content } }] }) or an + // Anthropic model (POST .../v1/messages, { content: [{ type: "text", text }] }) — + // vision-bridge router priority (#7204) now ranks credentialed providers (openai/ + // anthropic) ahead of opencode-*, so the mock must match whichever shape the + // fallback attempt actually requests instead of assuming OpenAI's shape. + const FALLBACK_TEXT = "fallback model description"; globalThis.fetch = async (url: RequestInfo | URL, _init?: RequestInit) => { fetchCallCount++; @@ -51,8 +55,14 @@ test("callVisionModel falls through to next model when primary fails", async () // First call (primary model) — simulate API error throw new Error("mock: primary model unavailable"); } - // Second call (fallback model) — return valid response - return new Response(FALLBACK_RESPONSE, { + // Second call (fallback model) — return a valid response shaped for whichever + // API the fallback model actually calls. + const urlStr = typeof url === "string" ? url : url.toString(); + const isAnthropicCall = urlStr.includes("/v1/messages"); + const body = isAnthropicCall + ? JSON.stringify({ content: [{ type: "text", text: FALLBACK_TEXT }] }) + : JSON.stringify({ choices: [{ message: { content: FALLBACK_TEXT } }] }); + return new Response(body, { status: 200, headers: { "Content-Type": "application/json" }, }); @@ -72,7 +82,7 @@ test("callVisionModel falls through to next model when primary fails", async () ); assert.equal( result, - "fallback model description", + FALLBACK_TEXT, "must return the fallback model's response" ); }); diff --git a/tests/unit/guardrails/visionBridge.test.ts b/tests/unit/guardrails/visionBridge.test.ts index c59713b371..b4a8dcbef5 100644 --- a/tests/unit/guardrails/visionBridge.test.ts +++ b/tests/unit/guardrails/visionBridge.test.ts @@ -38,6 +38,9 @@ function createGuardrail(options?: Parameters[0]) } return mockVisionResponse; }, + // Fail-open (null) so classic VB-S01/S07/S10 reroute tests keep working without a + // live credential DB. Credential-aware cases inject an explicit mock. + hasUsableCredentials: async () => null, ...(options?.deps ?? {}), }, }); @@ -695,3 +698,96 @@ test("VB-S11b: passthroughs when vision-capable model has NO combo mapping", asy assert.strictEqual(result.modifiedPayload, undefined); assert.strictEqual(visionCallCount, 0); }); + +// ── Credential-aware whole-request reroute (combo zai hijack fix) ─────────── + +test("VB-CRED-01: does NOT whole-request-reroute when original model has usable credentials", async () => { + // Repro: OpenCode 94-msg body with images + combo target zai/glm-5.2 was + // hijacked to opencode-zen/gpt-5.4 (priority 0, noauth) → 401 Missing API key. + const guardrail = createGuardrail({ + deps: { + hasUsableCredentials: async (m: string) => + m.startsWith("zai/") ? true : m.startsWith("opencode-") ? false : null, + }, + }); + + const payload = createPayload({ + model: "zai/glm-5.2", + messages: [ + { + role: "user", + content: [ + { type: "text", text: "What is in this screenshot?" }, + { + type: "image_url", + image_url: { url: "https://example.com/shot.png" }, + }, + ], + }, + ], + }); + + const result = await guardrail.preCall(payload, createContext({ model: "zai/glm-5.2" })); + assert.strictEqual(result.block, false); + // Must NOT swap model to opencode-zen / openai vision target + if (result.modifiedPayload) { + const modified = result.modifiedPayload as { model?: string }; + assert.strictEqual( + modified.model, + "zai/glm-5.2", + "credentialed original model must not be whole-request-rerouted" + ); + } + const meta = result.meta as Record | undefined; + assert.notStrictEqual(meta?.rerouted, true, "must not set rerouted meta for credentialed model"); +}); + +test("VB-CRED-02: does NOT reroute to a vision model known to lack credentials", async () => { + mockSettings.visionBridgeModel = "opencode-zen/gpt-5.4"; + const guardrail = createGuardrail({ + deps: { + // Original unusable, best vision model also unusable + hasUsableCredentials: async () => false, + }, + }); + + const payload = createPayload({ + model: "minimax/minimax-01", + messages: [ + { + role: "user", + content: [ + { type: "text", text: "Describe" }, + { type: "image_url", image_url: { url: "https://example.com/a.png" } }, + ], + }, + ], + }); + + const result = await guardrail.preCall(payload, createContext({ model: "minimax/minimax-01" })); + assert.strictEqual(result.block, false); + const meta = result.meta as Record | undefined; + assert.notStrictEqual(meta?.rerouted, true, "must not reroute to unusable vision model"); +}); + +test("isProviderConnectionUsable rejects noauth without api key", async () => { + const { isProviderConnectionUsable } = await import( + "../../../src/lib/guardrails/visionBridge.ts" + ); + assert.strictEqual( + isProviderConnectionUsable({ authType: "noauth", apiKey: null }), + false + ); + assert.strictEqual( + isProviderConnectionUsable({ authType: "apikey", apiKey: "sk-real" }), + true + ); + assert.strictEqual( + isProviderConnectionUsable({ authType: "oauth", refreshToken: "rt" }), + true + ); + assert.strictEqual( + isProviderConnectionUsable({ authType: "apikey", apiKey: "x", testStatus: "banned" }), + false + ); +}); diff --git a/tests/unit/resolve-routing-model.test.ts b/tests/unit/resolve-routing-model.test.ts index b9a3c53b9e..3b41a8121a 100644 --- a/tests/unit/resolve-routing-model.test.ts +++ b/tests/unit/resolve-routing-model.test.ts @@ -1,7 +1,12 @@ // Regression guard for #4863: X-Route-Model header overrides body.model for routing. +// Also covers alignBodyModelWithRouting — without body alignment the post-guardrail +// path silently restores body.model and undoes the header (zai header + opencode body → 401). import { describe, it } from "node:test"; import assert from "node:assert/strict"; -import { resolveRoutingModel } from "../../src/sse/handlers/resolveRoutingModel.ts"; +import { + alignBodyModelWithRouting, + resolveRoutingModel, +} from "../../src/sse/handlers/resolveRoutingModel.ts"; function req(headers: Record) { return { headers: { get: (n: string) => headers[n.toLowerCase()] ?? null } }; @@ -30,3 +35,31 @@ describe("resolveRoutingModel (#4863)", () => { assert.equal(resolveRoutingModel(req({ "x-route-model": " " }), { model: "fallback" }), "fallback"); }); }); + +describe("alignBodyModelWithRouting (X-Route-Model body lockstep)", () => { + it("rewrites body.model when it differs from the routing model", () => { + const body = { model: "opencode-zen/gpt-5.4", messages: [{ role: "user", content: "hi" }] }; + const routed = resolveRoutingModel(req({ "x-route-model": "zai/glm-5.2" }), body); + const result = alignBodyModelWithRouting(body, routed); + assert.equal(routed, "zai/glm-5.2"); + assert.equal(result.aligned, true); + assert.equal(result.previousModel, "opencode-zen/gpt-5.4"); + assert.equal(result.body.model, "zai/glm-5.2"); + // Original body object is not mutated + assert.equal(body.model, "opencode-zen/gpt-5.4"); + }); + + it("is a no-op when body.model already matches", () => { + const body = { model: "zai/glm-5.2" }; + const result = alignBodyModelWithRouting(body, "zai/glm-5.2"); + assert.equal(result.aligned, false); + assert.equal(result.body, body); + }); + + it("is a no-op when routing model is empty", () => { + const body = { model: "opencode-zen/gpt-5.4" }; + const result = alignBodyModelWithRouting(body, null); + assert.equal(result.aligned, false); + assert.equal(result.body.model, "opencode-zen/gpt-5.4"); + }); +}); diff --git a/tests/unit/vision-bridge-policy-reroute-6640.test.ts b/tests/unit/vision-bridge-policy-reroute-6640.test.ts index 62a5ae0d4f..e710bb068b 100644 --- a/tests/unit/vision-bridge-policy-reroute-6640.test.ts +++ b/tests/unit/vision-bridge-policy-reroute-6640.test.ts @@ -5,15 +5,29 @@ // against the original model. Without a re-check, a key restricted via // `allowedModels` could silently execute against an unvetted reroute target. // +// UPDATED for PR #7204 (Vision Bridge no-credentialed-hijack fix): when the +// ORIGINAL model already has a usable, credentialed connection (as seeded +// below via `seedConnection("openai", ...)`), Vision Bridge now deliberately +// never whole-request-reroutes it to another model (see +// `VisionBridgeGuardrail.preCall` step 9 / `VB-CRED-01` in +// tests/unit/guardrails/visionBridge.test.ts) — it always falls through to +// describe-then-forward: an internal call describes the image via the vision +// model, then the description is forwarded as text to the ORIGINAL, +// already-approved model, which produces the final, user-facing answer. +// // These tests exercise the real `handleChat()` pipeline end-to-end (real DB, -// real guardrail registry, mocked upstream fetch) to prove: -// 1. A guardrail-driven reroute to a model NOT in the key's `allowedModels` -// is rejected — the request falls back to the original, already-approved -// model instead of silently escaping the policy. -// 2. A guardrail-driven reroute to a model that DOES pass the allowlist is -// still honored (the fix must not break the legitimate reroute). -// 3. The reroute honors an explicit `settings.visionBridgeModel` operator -// override, consistent with the combo/describe path. +// real guardrail registry, mocked upstream fetch) to prove the policy +// invariant that motivated #6640 still holds under the new #7204 behavior: +// 1. A vision-bridge target NOT in the key's `allowedModels` is only ever +// used internally (image description) — it never becomes the final, +// user-facing answering model. That role always stays with the +// original, already-approved model. +// 2. Even when the vision target IS inside `allowedModels`, a credentialed +// original model is still never whole-request-rerouted — the original +// model remains the final answerer (no regression from #7204's intent). +// 3. An explicit `settings.visionBridgeModel` operator override is honored +// as the internal describe-path model, but — consistent with #7204 — +// does not override a credentialed original model as the final answerer. import test from "node:test"; import assert from "node:assert/strict"; @@ -71,27 +85,37 @@ test("#6640: guardrail reroute to a model outside allowedModels is rejected — }) ); - // The reroute target (gpt-4o-mini) is not in allowedModels — the request - // must NOT be silently executed against it. It must either fall back to the - // original, already-approved model (gpt-3.5-turbo) or be rejected outright, - // but it must never reach the upstream with the disallowed model. + // The vision-bridge target (gpt-4o-mini) is not in allowedModels — the + // request must NOT be silently executed against it as the final answering + // model. Under #7204, the credentialed original model (gpt-3.5-turbo) is + // never whole-request-rerouted in the first place, so the vision target can + // only ever appear as an internal, non-final describe call. Either way, the + // FINAL upstream call (the one whose response the user actually receives) + // must never be the disallowed model. if (response.status === 200) { - assert.equal(fetchCalls.length, 1, "exactly one upstream call expected"); + assert.ok(fetchCalls.length >= 1, "at least one upstream call expected"); + const finalCall = fetchCalls[fetchCalls.length - 1]; assert.equal( - fetchCalls[0].body?.model, + finalCall.body?.model, "gpt-3.5-turbo", - "must fall back to the original allowed model, not silently execute the disallowed reroute target" + "the final, user-facing answer must come from the original allowed model, not the disallowed vision target" ); } else { assert.equal(fetchCalls.length, 0, "a rejected request must never reach the upstream"); } }); -test("#6640: guardrail reroute to a model inside allowedModels is honored (no regression)", async () => { +test("#6640: a credentialed original model is never whole-request-rerouted, even when the vision target is inside allowedModels (PR #7204)", async () => { await seedConnection("openai", { apiKey: "sk-openai-primary" }); await settingsDb.updateSettings({ visionBridgeModel: "openai/gpt-4o-mini" }); - // This key allows BOTH the original model and the reroute target. + // This key allows BOTH the original model and the vision target — before + // #7204 this would have made the whole-request reroute to gpt-4o-mini the + // final answering model. #7204 deliberately changes this: a credentialed + // original model (gpt-3.5-turbo, seeded above) is never whole-request- + // rerouted regardless of what's allowed — it always stays the final + // answerer, with the vision target used only for the internal image + // description (see VB-CRED-01 in tests/unit/guardrails/visionBridge.test.ts). const apiKey = await seedApiKey({ allowedModels: ["openai/gpt-3.5-turbo", "openai/gpt-4o-mini"], }); @@ -110,19 +134,20 @@ test("#6640: guardrail reroute to a model inside allowedModels is honored (no re ); assert.equal(response.status, 200); - assert.equal(fetchCalls.length, 1); + assert.ok(fetchCalls.length >= 1, "at least one upstream call expected"); + const finalCall = fetchCalls[fetchCalls.length - 1]; assert.equal( - fetchCalls[0].body?.model, - "gpt-4o-mini", - "reroute to an allowed vision model must still be honored" + finalCall.body?.model, + "gpt-3.5-turbo", + "the credentialed original model must remain the final answerer — no whole-request reroute (PR #7204), even though gpt-4o-mini is allowed" ); }); -test("#6640: reroute honors an explicit settings.visionBridgeModel override (consistency with the describe path)", async () => { +test("#6640: settings.visionBridgeModel override does not displace a credentialed original model as the final answerer (PR #7204)", async () => { await seedConnection("openai", { apiKey: "sk-openai-primary" }); await settingsDb.updateSettings({ visionBridgeModel: "openai/gpt-4o-mini" }); - // No allowlist restriction — nothing to enforce here, this proves the + // No allowlist restriction — nothing to enforce here; this proves the // settings threading itself (independent of the policy re-check above). const apiKey = await seedApiKey(); @@ -140,10 +165,11 @@ test("#6640: reroute honors an explicit settings.visionBridgeModel override (con ); assert.equal(response.status, 200); - assert.equal(fetchCalls.length, 1); + assert.ok(fetchCalls.length >= 1, "at least one upstream call expected"); + const finalCall = fetchCalls[fetchCalls.length - 1]; assert.equal( - fetchCalls[0].body?.model, - "gpt-4o-mini", - "the configured settings.visionBridgeModel must be honored as the reroute target" + finalCall.body?.model, + "gpt-3.5-turbo", + "the configured settings.visionBridgeModel must not override the credentialed original model as the final answerer (PR #7204)" ); });