diff --git a/src/app/api/v1/relay/chat/completions/route.ts b/src/app/api/v1/relay/chat/completions/route.ts index b92b9cbfe0..da8b4f4e03 100644 --- a/src/app/api/v1/relay/chat/completions/route.ts +++ b/src/app/api/v1/relay/chat/completions/route.ts @@ -10,7 +10,11 @@ import { CORS_HEADERS, handleCorsOptions } from "@/shared/utils/cors"; import { handleChat } from "@/sse/handlers/chat"; import { createInjectionGuard } from "@/middleware/promptInjectionGuard"; import { getRelayTokenByHash, checkRateLimit, recordRelayUsage } from "@/lib/db/relayProxies"; -import { buildErrorBody } from "@omniroute/open-sse/utils/error"; +import { + buildErrorBody, + parseUpstreamError, + sanitizeErrorMessage, +} from "@omniroute/open-sse/utils/error"; import { checkIpRateLimit, extractToken, @@ -108,6 +112,32 @@ async function forwardToBifrost( headers.set("Content-Type", upstream.headers.get("Content-Type") ?? "application/json"); } + // Issue #1: Bifrost (or the upstream behind it) may return plain text or HTML + // on a non-OK status (e.g. 502 from a sidecar, "invalid character 'd'" style + // proxy errors). Forwarding `upstream.body` raw leaks non-JSON into a client + // that expects OpenAI-shaped JSON, producing client-side parse failures. + // Normalize any non-OK response through parseUpstreamError + buildErrorBody so + // the client always receives a valid JSON error. (Hard rule #12.) + if (!upstream.ok) { + const parsed = await parseUpstreamError(upstream, null); + const errorBody = buildErrorBody( + parsed.statusCode, + sanitizeErrorMessage(parsed.message), + parsed.responseBody + ); + const errorHeaders = new Headers(headers); + errorHeaders.set("Content-Type", "application/json"); + if (parsed.retryAfterMs && parsed.retryAfterMs > 0) { + errorHeaders.set("Retry-After", String(Math.ceil(parsed.retryAfterMs / 1000))); + } + clearTimeout(tid); + recordUsage(token.id, request, startTime, clientIp, userAgent, "error", parsed.statusCode); + return new Response(JSON.stringify(errorBody), { + status: parsed.statusCode, + headers: errorHeaders, + }); + } + if (wantsStream && upstream.body) { const stream = finalizeReadableStream(upstream.body, (error) => { clearTimeout(tid); @@ -144,7 +174,7 @@ async function forwardToBifrost( startTime, clientIp, userAgent, - upstream.status < 500 ? "success" : "error", + upstream.status >= 200 && upstream.status < 300 ? "success" : "error", upstream.status ); diff --git a/src/sse/handlers/chat.ts b/src/sse/handlers/chat.ts index b8aff4cd2a..56d6b50801 100644 --- a/src/sse/handlers/chat.ts +++ b/src/sse/handlers/chat.ts @@ -1673,7 +1673,8 @@ async function handleSingleModelChat( model, lastError, lastStatus, - candidateAliases + candidateAliases, + isCombo ); const lastFailedConnectionId = excludedConnectionIds.size > 0 @@ -1818,7 +1819,8 @@ async function handleSingleModelChat( comboStrategy, isCombo, comboStepId: runtimeOptions.comboStepId ?? null, - comboExecutionKey: runtimeOptions.comboExecutionKey ?? runtimeOptions.comboStepId ?? null, + comboExecutionKey: + runtimeOptions.comboExecutionKey ?? runtimeOptions.comboStepId ?? null, extendedContext, modelApiFormat: apiFormat, modelTargetFormat: targetFormat, diff --git a/src/sse/handlers/chatHelpers.ts b/src/sse/handlers/chatHelpers.ts index c82333a78e..866ca28b66 100644 --- a/src/sse/handlers/chatHelpers.ts +++ b/src/sse/handlers/chatHelpers.ts @@ -630,7 +630,8 @@ export function handleNoCredentials( model: string, lastError: string | null, lastStatus: number | null, - candidateAliases?: readonly string[] + candidateAliases?: readonly string[], + isCombo: boolean = false ) { if (credentials?.allRateLimited) { const errorMsg = lastError || credentials.lastError || "Unavailable"; @@ -705,7 +706,7 @@ export function handleNoCredentials( log.warn("AUTH", `No active credentials for provider: ${provider}`); // #FIX: surface the candidate aliases (from resolveModelOrError) so the // operator can pick a working provider/model prefix instead of guessing. - // Without this, "No active credentials for provider: kiro" leaves the + // Without this, "No active credentials for provider: byNara" leaves the // user staring at a wall — most bugs in this area are actually "wrong // provider was picked", not "the provider is broken". const hint = @@ -715,6 +716,26 @@ export function handleNoCredentials( .map((a) => `${a}/${model}`) .join(", ")}.` : ""; + + // Issue #2: for single-model (non-combo) requests, a 404 leaks a misleading + // "No active credentials" status to a direct API client (e.g. OpenCode) that + // then mis-files it as "resource not found" instead of an auth/credential + // failure. The 404 is only meaningful as a combo fall-through signal, so + // remap it to an explicit error status for single-model traffic: a 401 when + // the provider exists but has no usable credentials, else 503 when the + // provider itself is unknown/unreachable. Combo routing keeps the 404 so it + // can still skip past a disabled-credentials leg. + if (!isCombo) { + const singleModelStatus = + provider && String(provider).trim().length > 0 + ? HTTP_STATUS.UNAUTHORIZED + : HTTP_STATUS.SERVICE_UNAVAILABLE; + return errorResponse( + singleModelStatus, + `No active credentials for provider: ${provider}.${hint}` + ); + } + return errorResponse( HTTP_STATUS.NOT_FOUND, `No active credentials for provider: ${provider}.${hint}` diff --git a/tests/unit/api/v1/relay-completions-errors.test.ts b/tests/unit/api/v1/relay-completions-errors.test.ts new file mode 100644 index 0000000000..85d64ec6ee --- /dev/null +++ b/tests/unit/api/v1/relay-completions-errors.test.ts @@ -0,0 +1,200 @@ +import { test } from "node:test"; +import assert from "node:assert/strict"; +import { createHash } from "node:crypto"; +import { + checkIpRateLimit, + getClientIp, + sanitizeForensicHeader, +} from "../../../../src/app/api/v1/relay/chat/completions/relaySecurity.ts"; +import { getDbInstance } from "../../../../src/lib/db/core.ts"; +import { getRelayLogs } from "../../../../src/lib/db/relayProxies.ts"; + +// ─── Relay completions route: Bifrost upstream error normalization ────────── +// +// T-issues: (1) a plain-text/HTML non-OK Bifrost response must be normalized +// into a valid OpenAI JSON error instead of leaking raw text (which produces +// client-side "invalid character 'd'" parse failures); (3) upstream 4xx must be +// recorded as analytics "error", never "success". + +const ORIGINAL_BIFROST_BASE_URL = process.env.BIFROST_BASE_URL; +const ORIGINAL_BIFROST_API_KEY = process.env.BIFROST_API_KEY; +const ORIGINAL_BIFROST_OMNI_KEY = process.env.OMNIROUTE_BIFROST_KEY; +const ORIGINAL_BIFROST_TIMEOUT = process.env.BIFROST_TIMEOUT_MS; +const ORIGINAL_BIFROST_STREAMING = process.env.BIFROST_STREAMING_ENABLED; +const ORIGINAL_RELAY_BACKEND = process.env.OMNIROUTE_RELAY_BACKEND; +const ORIGINAL_FETCH = globalThis.fetch; + +function seedRelayToken(rawToken: string) { + const id = `rl_test_${Date.now()}_${Math.random().toString(16).slice(2)}`; + const now = Math.floor(Date.now() / 1000); + getDbInstance() + .prepare( + ` + INSERT INTO relay_tokens (id, name, token_hash, token_prefix, description, combo_id, + allowed_models, max_tokens_per_request, max_requests_per_minute, max_requests_per_day, + max_cost_per_day, enabled, created_at, updated_at, expires_at, metadata) + VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, 1, ?, ?, ?, ?) + ` + ) + .run( + id, + "relay-completions-err", + createHash("sha256").update(rawToken).digest("hex"), + "rl_test", + "", + null, + JSON.stringify(["*"]), + 128000, + 60, + 10000, + 0, + now, + now, + null, + "{}" + ); + return { id, rawToken }; +} + +function restoreEnv() { + if (ORIGINAL_BIFROST_BASE_URL === undefined) delete process.env.BIFROST_BASE_URL; + else process.env.BIFROST_BASE_URL = ORIGINAL_BIFROST_BASE_URL; + if (ORIGINAL_BIFROST_API_KEY === undefined) delete process.env.BIFROST_API_KEY; + else process.env.BIFROST_API_KEY = ORIGINAL_BIFROST_API_KEY; + if (ORIGINAL_BIFROST_OMNI_KEY === undefined) delete process.env.OMNIROUTE_BIFROST_KEY; + else process.env.OMNIROUTE_BIFROST_KEY = ORIGINAL_BIFROST_OMNI_KEY; + if (ORIGINAL_BIFROST_TIMEOUT === undefined) delete process.env.BIFROST_TIMEOUT_MS; + else process.env.BIFROST_TIMEOUT_MS = ORIGINAL_BIFROST_TIMEOUT; + if (ORIGINAL_BIFROST_STREAMING === undefined) delete process.env.BIFROST_STREAMING_ENABLED; + else process.env.BIFROST_STREAMING_ENABLED = ORIGINAL_BIFROST_STREAMING; + if (ORIGINAL_RELAY_BACKEND === undefined) delete process.env.OMNIROUTE_RELAY_BACKEND; + else process.env.OMNIROUTE_RELAY_BACKEND = ORIGINAL_RELAY_BACKEND; + globalThis.fetch = ORIGINAL_FETCH; +} + +function setupBifrostEnv() { + process.env.OMNIROUTE_RELAY_BACKEND = "bifrost"; + process.env.BIFROST_BASE_URL = "http://bifrost.test.local:8080"; + process.env.BIFROST_TIMEOUT_MS = "5000"; + delete process.env.BIFROST_API_KEY; + delete process.env.OMNIROUTE_BIFROST_KEY; + delete process.env.BIFROST_STREAMING_ENABLED; +} + +test("relay route: normalizes plain-text Bifrost 404 into JSON error (Issue #1)", async () => { + setupBifrostEnv(); + const relayToken = seedRelayToken(`relay_err_${Date.now()}`); + + // Bifrost sidecar returns a raw HTML/plain-text non-OK response — the exact + // "invalid character 'd'" scenario behind client JSON parse failures. + globalThis.fetch = async () => { + return new Response("
404 page not found", { + status: 404, + headers: { "content-type": "text/html" }, + }); + }; + + const { POST } = await import( + `../../../../src/app/api/v1/relay/chat/completions/route.ts?case=${Date.now()}-${Math.random()}` + ); + + const req = new Request("http://localhost/api/v1/relay/chat/completions", { + method: "POST", + headers: { + authorization: `Bearer ${relayToken.rawToken}`, + "content-type": "application/json", + "x-request-id": "relay-err-404", + }, + body: JSON.stringify({ model: "gpt-4", messages: [{ role: "user", content: "hi" }] }), + }); + + const res = await POST(req); + // Status preserved from upstream (404), but body is valid JSON, not HTML. + assert.equal(res.status, 404); + assert.equal(res.headers.get("content-type"), "application/json"); + // The critical fix: the client receives parseable JSON, NOT a raw HTML body + // (which previously caused "invalid character 'd'" JSON.parse failures). + const raw = await res.text(); + assert.doesNotMatch(String(raw), /^, "response body must be JSON, not raw HTML"); + const body = JSON.parse(raw); + assert.ok(body?.error?.message, "must contain an error message"); + assert.match(String(body?.error?.message), /page not found/); + + restoreEnv(); +}); + +test("relay route: normalizes HTML 502 from Bifrost into JSON error (Issue #1)", async () => { + setupBifrostEnv(); + const relayToken = seedRelayToken(`relay_err_${Date.now()}`); + + globalThis.fetch = async () => { + return new Response( + "invalid character 'd'", + { status: 502, headers: { "content-type": "text/html" } } + ); + }; + + const { POST } = await import( + `../../../../src/app/api/v1/relay/chat/completions/route.ts?case=${Date.now()}-${Math.random()}` + ); + + const req = new Request("http://localhost/api/v1/relay/chat/completions", { + method: "POST", + headers: { + authorization: `Bearer ${relayToken.rawToken}`, + "content-type": "application/json", + "x-request-id": "relay-err-502", + }, + body: JSON.stringify({ model: "gpt-4", messages: [{ role: "user", content: "hi" }] }), + }); + + const res = await POST(req); + assert.equal(res.status, 502); + assert.equal(res.headers.get("content-type"), "application/json"); + const body = await res.json(); + assert.ok(body?.error?.message); + + // Upstream 4xx/5xx must be recorded as analytics "error" (Issue #3). + const logs = getRelayLogs(relayToken.id, 10); + assert.equal(logs.length, 1); + assert.equal(logs[0].status, "error"); + assert.equal(logs[0].status_code, 502); + + restoreEnv(); +}); + +test("relay route: upstream 401 recorded as analytics error not success (Issue #3)", async () => { + setupBifrostEnv(); + const relayToken = seedRelayToken(`relay_err_${Date.now()}`); + + globalThis.fetch = async () => { + return new Response(JSON.stringify({ error: { message: "unauthorized" } }), { + status: 401, + headers: { "content-type": "application/json" }, + }); + }; + + const { POST } = await import( + `../../../../src/app/api/v1/relay/chat/completions/route.ts?case=${Date.now()}-${Math.random()}` + ); + + const req = new Request("http://localhost/api/v1/relay/chat/completions", { + method: "POST", + headers: { + authorization: `Bearer ${relayToken.rawToken}`, + "content-type": "application/json", + "x-request-id": "relay-err-401", + }, + body: JSON.stringify({ model: "gpt-4", messages: [{ role: "user", content: "hi" }] }), + }); + + const res = await POST(req); + assert.equal(res.status, 401); + + const logs = getRelayLogs(relayToken.id, 10); + assert.equal(logs.length, 1); + assert.equal(logs[0].status, "error"); + assert.equal(logs[0].status_code, 401); + + restoreEnv(); +}); diff --git a/tests/unit/chat-helpers.test.ts b/tests/unit/chat-helpers.test.ts index 6b85e58291..dc4990d7f3 100644 --- a/tests/unit/chat-helpers.test.ts +++ b/tests/unit/chat-helpers.test.ts @@ -308,7 +308,18 @@ test("handleNoCredentials reports missing provider credentials and exhausted acc // open-sse/services/accountFallback.ts:1593-1599) so the next combo target is // tried. We surface "no active credentials" as 404 so combo can skip past a // disabled-credentials provider instead of failing the whole request. - const missing = handleNoCredentials(null, null, "openai", "gpt-4o-mini", null, null); + // In combo routing the no-credentials branch must stay 404 NOT_FOUND so the + // combo target loop can fall through to the next target. Pass isCombo=true. + const missing = handleNoCredentials( + null, + null, + "openai", + "gpt-4o-mini", + null, + null, + undefined, + true + ); const exhausted = handleNoCredentials( null, "conn_123", @@ -327,6 +338,65 @@ test("handleNoCredentials reports missing provider credentials and exhausted acc assert.match(exhaustedJson.error.message, /Primary account failed/); }); +test("handleNoCredentials remaps leaked 404 to 401/503 for single-model requests", async () => { + // Issue #2: a direct (non-combo) API client must not receive a misleading 404 + // "No active credentials" error — remap to an explicit auth/credential status. + const forKnownProvider = handleNoCredentials( + null, + null, + "byNara", + "claude-sonnet-4.6", + null, + null, + undefined, + /* isCombo */ false + ); + assert.equal(forKnownProvider.status, 401); + const knownJson = (await forKnownProvider.json()) as { error?: { message?: string } }; + assert.match(knownJson.error?.message ?? "", /No active credentials for provider: byNara/); + + const forUnknownProvider = handleNoCredentials( + null, + null, + "", + "gpt-4o-mini", + null, + null, + undefined, + /* isCombo */ false + ); + assert.equal(forUnknownProvider.status, 503); +}); + +test("handleNoCredentials still leaks 404 (combo fall-through) only when combo", async () => { + // Regression guard: the 404 is intentionally preserved for combo routing so it + // can skip a disabled-credentials leg. Explicitly assert isCombo=true keeps 404 + // and isCombo=false does not. (Issue #2) + const combo = handleNoCredentials( + null, + null, + "kiro", + "claude-opus-5", + null, + null, + undefined, + true + ); + assert.equal(combo.status, 404); + + const single = handleNoCredentials( + null, + null, + "byNara", + "claude-opus-5", + null, + null, + undefined, + false + ); + assert.notEqual(single.status, 404); +}); + test("handleNoCredentials returns Retry-After when every account is rate limited", async () => { const retryAfter = new Date(Date.now() + 45_000).toISOString(); const response = handleNoCredentials( @@ -506,7 +576,7 @@ test("executeChatWithBreaker preserves account TLS scope when a proxy bypasses t ], usage: { prompt_tokens: 1, completion_tokens: 1, total_tokens: 2 }, }), - { headers: { "content-type": "application/json" } }, + { headers: { "content-type": "application/json" } } ); }, }); diff --git a/tests/unit/fix-error-message-candidates.test.ts b/tests/unit/fix-error-message-candidates.test.ts index d141da40b0..3bdfa50d51 100644 --- a/tests/unit/fix-error-message-candidates.test.ts +++ b/tests/unit/fix-error-message-candidates.test.ts @@ -17,7 +17,8 @@ test("handleNoCredentials includes candidate aliases hint when supplied", async /* model */ "claude-opus-5", /* lastError */ null, /* lastStatus */ null, - /* candidateAliases */ ["anthropic", "claude", "agentrouter"] + /* candidateAliases */ ["anthropic", "claude", "agentrouter"], + /* isCombo */ true ); assert.equal(res.status, 404); @@ -42,8 +43,10 @@ test("handleNoCredentials omits hint when no candidates supplied", async () => { "kiro", "claude-opus-5", null, - null + null, /* no candidateAliases */ + undefined, + /* isCombo */ true ); assert.equal(res.status, 404); @@ -65,14 +68,18 @@ test("handleNoCredentials trims candidate list to top 3", async () => { "claude-opus-5", null, null, - ["anthropic", "claude", "agentrouter", "github", "vertex-partner"] + ["anthropic", "claude", "agentrouter", "github", "vertex-partner"], + /* isCombo */ true ); const body = (await res.json()) as { error?: { message?: string } }; const message = body?.error?.message ?? ""; // Top-3 (anthropic, claude, agentrouter) — github and vertex-partner are // dropped to keep the hint actionable. - assert.match(message, /Try one of: anthropic\/claude-opus-5, claude\/claude-opus-5, agentrouter\/claude-opus-5/); + assert.match( + message, + /Try one of: anthropic\/claude-opus-5, claude\/claude-opus-5, agentrouter\/claude-opus-5/ + ); assert.doesNotMatch(message, /github\/claude-opus-5/); assert.doesNotMatch(message, /vertex-partner\/claude-opus-5/); -}); \ No newline at end of file +});