From 564d7dec77c0ff210f405f4d1ff8890364e19714 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Wed, 3 Jun 2026 08:48:12 +0200 Subject: [PATCH] fix(ai): never 5xx from the connection test and report a categorised reason The connection-test route mapped a provider 5xx to a 502, which a reverse proxy rewrites to its own HTML error page; the browser's res.json() then threw "Unexpected token '<'". Every provider-call failure now returns HTTP 200 with a secret-free { ok:false, providerType, reason }. Reasons are categorised: credential/auth (401/403, or a 5xx whose body signals an invalidated/expired session), rate-limit (429), provider server error (other 5xx) and network/timeout. The empty-override "test my saved config" path now resolves through the same provider chain generation uses, instead of the legacy codex->admin probe that could test a different provider than the one generation runs. The chain runner already advances past an auth-class failure to the next enabled provider; add explicit coverage for that on the raw generation path (401 -> next succeeds, fallback count incremented). --- src/app/api/ai/test/__tests__/route.test.ts | 82 +++++++++++++------- src/app/api/ai/test/route.ts | 67 ++++++++++------ src/lib/ai/__tests__/provider-runner.test.ts | 30 +++++++ src/lib/ai/__tests__/provider.test.ts | 53 ++++++++++++- src/lib/ai/provider.ts | 18 ++++- 5 files changed, 191 insertions(+), 59 deletions(-) diff --git a/src/app/api/ai/test/__tests__/route.test.ts b/src/app/api/ai/test/__tests__/route.test.ts index ea8072237..8ae65d816 100644 --- a/src/app/api/ai/test/__tests__/route.test.ts +++ b/src/app/api/ai/test/__tests__/route.test.ts @@ -65,11 +65,22 @@ function jsonRequest(body: unknown): Request { }); } +interface TestFailureEnvelope { + data: { ok: false; providerType: string; reason: string }; + error: null; +} + // V3 audit: /api/ai/test was returning provider err.message + bodyExcerpt // directly to the client, leaking provider URLs / partial keys / internal // headers. Server now logs full details via annotate() and responds with -// a categorised, generic message. -describe("POST /api/ai/test — provider error leak guard", () => { +// a categorised, secret-free reason. +// +// This route MUST NEVER return a 5xx: a 5xx origin response is rewritten +// by Cloudflare to its own HTML error page, so the browser's res.json() +// crashes with `Unexpected token '<', " { function makeProviderThatThrows( err: Error & { httpStatus?: number; bodyExcerpt?: string }, ) { @@ -81,7 +92,7 @@ describe("POST /api/ai/test — provider error leak guard", () => { } as never); } - it("does not echo provider err.message back to the client (HIGH coverage gap)", async () => { + it("never returns a 5xx and does not echo provider err.message back to the client", async () => { const err = Object.assign( new Error("OpenAI 401 from https://api.openai.com/v1 sk-leaked-key"), { @@ -92,49 +103,60 @@ describe("POST /api/ai/test — provider error leak guard", () => { makeProviderThatThrows(err); const response = await POST(emptyRequest() as never); - const body = (await response.json()) as ApiErrorEnvelope; - - // v1.4.5: credential failures map to 422 (not 502) so Cloudflare - // doesn't replace our JSON body with its HTML error page — that - // rewrite was the root cause of the "Unexpected token '<'…" client - // crash the maintainer hit when typing a bad OpenAI key. - expect(response.status).toBe(422); - expect(body.error ?? "").not.toMatch(/sk-/); - expect(body.error ?? "").not.toMatch(/api\.openai\.com/); - expect(body.error ?? "").not.toMatch(/invalid api key/i); - expect(body.error).toBe("Provider rejected the credentials"); + // Always JSON-parseable, never a 5xx (the Cloudflare HTML-rewrite + // root cause of "Unexpected token '<'…"). + expect(response.status).toBe(200); + const body = (await response.json()) as TestFailureEnvelope; + expect(body.data.ok).toBe(false); + expect(body.data.reason).not.toMatch(/sk-/); + expect(body.data.reason).not.toMatch(/api\.openai\.com/); + expect(body.data.reason).not.toMatch(/invalid api key/i); + expect(body.data.reason).toMatch(/re-authenticate/i); }); - it("returns the 429-categorised message when the provider rate-limits", async () => { + it("reclassifies a 5xx whose body signals an invalidated session as a credential failure", async () => { + // The shape an operator hit: re-auth, gateway answers 500 with + // "authentication token has been invalidated" instead of a 401. + makeProviderThatThrows( + Object.assign(new Error("upstream 500"), { + httpStatus: 500 as const, + bodyExcerpt: + '{"error":{"message":"Your authentication token has been invalidated. Please try signing in again."}}', + }), + ); + const response = await POST(emptyRequest() as never); + expect(response.status).toBe(200); + const body = (await response.json()) as TestFailureEnvelope; + expect(body.data.ok).toBe(false); + expect(body.data.reason).toMatch(/re-authenticate/i); + }); + + it("categorises a 429 rate-limit", async () => { makeProviderThatThrows( Object.assign(new Error("429 from openai"), { httpStatus: 429 as const }), ); const response = await POST(emptyRequest() as never); - // v1.4.5: rate-limit passes through as 429 (was 502 in v1.4.4) so - // the React Query mutation can read the JSON body instead of - // tripping over Cloudflare's HTML 502. - expect(response.status).toBe(429); - expect(((await response.json()) as ApiErrorEnvelope).error).toBe( - "Provider rate-limited the request", - ); + expect(response.status).toBe(200); + const body = (await response.json()) as TestFailureEnvelope; + expect(body.data.reason).toMatch(/rate-limited/i); }); - it("returns the 5xx-categorised message when the provider has a server error", async () => { + it("categorises a plain 5xx provider server error (no auth signal)", async () => { makeProviderThatThrows( Object.assign(new Error("503 upstream"), { httpStatus: 503 as const }), ); const response = await POST(emptyRequest() as never); - expect(((await response.json()) as ApiErrorEnvelope).error).toBe( - "Provider returned a server error", - ); + expect(response.status).toBe(200); + const body = (await response.json()) as TestFailureEnvelope; + expect(body.data.reason).toMatch(/server error/i); }); - it("returns the unknown-error fallback otherwise", async () => { + it("categorises a network/timeout failure", async () => { makeProviderThatThrows(new Error("ECONNRESET")); const response = await POST(emptyRequest() as never); - expect(((await response.json()) as ApiErrorEnvelope).error).toBe( - "Provider connection failed", - ); + expect(response.status).toBe(200); + const body = (await response.json()) as TestFailureEnvelope; + expect(body.data.reason).toMatch(/could not reach/i); }); }); diff --git a/src/app/api/ai/test/route.ts b/src/app/api/ai/test/route.ts index c866f3726..d152de3a2 100644 --- a/src/app/api/ai/test/route.ts +++ b/src/app/api/ai/test/route.ts @@ -81,7 +81,7 @@ export const POST = apiHandler(async (request: NextRequest) => { // V3 audit: do not return provider error message + bodyExcerpt to the // client (leaks provider URL / partial keys / internal headers). // Log full details server-side for the operator and respond with a - // categorised, generic message. + // categorised, generic reason. const err = e as Error & { httpStatus?: number; bodyExcerpt?: string }; annotate({ meta: { @@ -91,28 +91,47 @@ export const POST = apiHandler(async (request: NextRequest) => { ai_test_provider: provider.type, }, }); - // Map upstream status to a *client-side* status code that won't be - // rewritten by Cloudflare. 5xx from origin causes Cloudflare to swap - // the body for its own HTML error page — `await res.json()` in the - // browser then crashes with `Unexpected token '<', "= 500) { - outboundStatus = 502; - safeMessage = "Provider returned a server error"; - } else { - outboundStatus = 422; - safeMessage = "Provider connection failed"; - } - return apiError(safeMessage, outboundStatus); + // This route MUST NEVER return a 5xx. A 5xx origin response is + // rewritten by Cloudflare / the reverse proxy to its own HTML error + // page, so the browser's `res.json()` crashes with + // `Unexpected token '<', "= 500 && looksLikeAuth)) { + return "Provider rejected the credentials — re-authenticate in AI settings."; + } + if (status === 429) { + return "Provider rate-limited the request — try again shortly."; + } + if (status >= 500) { + return "The AI provider returned a server error."; + } + if (status === 0) { + return "Could not reach the AI provider."; + } + return "Could not reach the AI provider."; +} diff --git a/src/lib/ai/__tests__/provider-runner.test.ts b/src/lib/ai/__tests__/provider-runner.test.ts index 130ad909d..74e02efde 100644 --- a/src/lib/ai/__tests__/provider-runner.test.ts +++ b/src/lib/ai/__tests__/provider-runner.test.ts @@ -360,6 +360,36 @@ describe("runRawCompletionWithFallback — legacy route shim", () => { expect(result.fallbackHops[0].providerType).toBe("codex"); }); + it("advances past an auth-class (401) failure to the next provider and counts the fallback", async () => { + // Generation routes consume this raw shim. An invalidated credential + // on the primary (401/403, or a 5xx whose body signals an + // invalidated session) must not abort the run — it advances to the + // next enabled provider and bumps the fallback count. + const codex = new ScriptedProvider({ + type: "codex", + script: [{ ok: false, error: err(401, "token invalidated") }], + }); + const openai = new ScriptedProvider({ + type: "admin-key", + script: [{ ok: true, content: '{ "ok": true }' }], + }); + const result = await runRawCompletionWithFallback({ + userId: "u-raw-auth-advance", + providers: [ + { providerType: "codex", instance: codex }, + { providerType: "openai", instance: openai }, + ], + params: { systemPrompt: "s", userPrompt: "u" }, + }); + expect(result.result.content).toBe('{ "ok": true }'); + expect(result.workingProvider.providerType).toBe("openai"); + expect(result.fallbackHops).toHaveLength(1); + expect(result.fallbackHops[0].providerType).toBe("codex"); + expect(result.fallbackHops[0].httpStatus).toBe(401); + expect(codex.callCount).toBe(1); + expect(openai.callCount).toBe(1); + }); + it("throws AllProvidersFailedError when every entry fails hard", async () => { const a = new ScriptedProvider({ type: "codex", diff --git a/src/lib/ai/__tests__/provider.test.ts b/src/lib/ai/__tests__/provider.test.ts index e076dbd88..eecbadbf5 100644 --- a/src/lib/ai/__tests__/provider.test.ts +++ b/src/lib/ai/__tests__/provider.test.ts @@ -21,7 +21,8 @@ vi.mock("@/lib/ai/codex-oauth", () => ({ decryptCodexCreds: vi.fn(), })); -import { resolveProvider } from "../provider"; +import { resolveProvider, resolveProviderForTest } from "../provider"; +import { OpenAIClient } from "../openai-client"; import { prisma } from "@/lib/db"; import { decrypt, encrypt } from "@/lib/crypto"; import { decryptCodexCreds, encryptCodexCreds } from "@/lib/ai/codex-oauth"; @@ -298,3 +299,53 @@ describe("resolveProvider", () => { expect(provider.type).toBe("codex"); }); }); + +/** + * The connection-test "test my saved config" path (empty override body) + * MUST resolve via the SAME chain generation uses, so a green test means + * generation will actually work. Previously it probed codex→admin + * directly, which could test a different provider than generation ran. + */ +describe("resolveProviderForTest — empty override resolves via the chain", () => { + beforeEach(() => { + vi.resetAllMocks(); + vi.mocked(decrypt).mockImplementation((v: string) => `decrypted:${v}`); + }); + + it("returns the first enabled+credentialed chain entry (the same provider generation runs)", async () => { + // resolveProviderForTest first reads the test row (no chain), then + // resolveProviderChain reads the chain row. The chain enables openai + // and the user has an OpenAI key → first chain entry is OpenAIClient. + vi.mocked(prisma.user.findUnique) + // 1st: resolveProviderForTest's own read + .mockResolvedValueOnce({ + aiProvider: null, + aiModel: null, + aiBaseUrl: null, + aiAnthropicKeyEncrypted: null, + aiLocalKeyEncrypted: null, + aiOpenaiKeyEncrypted: "enc-user-openai", + } as never) + // 2nd: resolveProviderChain's read + .mockResolvedValueOnce({ + aiProvider: null, + aiModel: null, + aiBaseUrl: null, + aiAnthropicKeyEncrypted: null, + aiLocalKeyEncrypted: null, + aiOpenaiKeyEncrypted: "enc-user-openai", + aiProviderChain: [ + { providerType: "openai", priority: 1, enabled: true }, + ], + } as never); + + const provider = await resolveProviderForTest("user-123", {}); + // The user-OpenAI client is an OpenAIClient; its runtime tag defaults + // to "admin-key" (no providerType passed). What matters is that the + // empty-override path materialised the chain's first entry rather than + // the legacy codex→admin probe — a NoProvider/none would mean the + // chain was bypassed. + expect(provider.type).not.toBe("none"); + expect(provider).toBeInstanceOf(OpenAIClient); + }); +}); diff --git a/src/lib/ai/provider.ts b/src/lib/ai/provider.ts index 527e1f4a8..26e1a10ae 100644 --- a/src/lib/ai/provider.ts +++ b/src/lib/ai/provider.ts @@ -424,11 +424,21 @@ export async function resolveProviderForTest( .toString() .trim(); - // Empty selection: fall back to Codex → admin OpenAI like the regular path. + // Empty selection ("test my saved config" — the ai-section calls + // /api/ai/test with an empty body): resolve via the SAME path generation + // uses — the first enabled+credentialed entry of `resolveProviderChain`. + // The legacy codex→admin fallback used to probe a different provider than + // generation ran (observed: test→admin-key while generation→codex), so a + // green test gave no signal about whether overnight generation would + // work. Falling through to the chain keeps the two paths in lock-step. if (!provider) { - const codex = await resolveCodexProvider(userId); - if (codex) return codex; - return resolveAdminProvider(); + const chain = await resolveProviderChain(userId); + if (chain.length > 0) return chain[0].instance; + // Chain empty (no enabled+credentialed entry) — mirror the regular + // single-provider resolution one more time before giving up. + const legacy = await resolveProvider(userId); + if (legacy.type !== "none") return legacy; + throw new AITestConfigError(422, "No AI provider configured"); } switch (provider) {