diff --git a/CHANGELOG.md b/CHANGELOG.md index c56f7a43b2..c44b96b38e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,7 @@ _Living section — bullets land here as PRs merge into `release/v3.8.47` (paral ### 🐛 Bug Fixes +- **fix(oauth):** Codex/ChatGPT (and every other OAuth provider) connection stays stuck showing "Auth Failed" even after a genuinely successful token refresh ([#6352](https://github.com/diegosouzapw/OmniRoute/issues/6352)) — `updateProviderCredentials()` (the shared `onPersist` callback for the manual "Refresh token" route, the reactive per-request refresh in `chat.ts`, and the Codex/Claude auth-file importers) correctly reused the stored `refresh_token`, persisted the new `access_token`, and replaced a rotated `refresh_token`, but never cleared the stale `testStatus`/`lastError*`/`errorCode` fields left over from a prior expired/invalid refresh or upstream 401/403 — only the separate background health-check sweep did that clearing. A successful refresh now resets `testStatus` to `"active"` and clears `lastError`, `lastErrorAt`, `lastErrorType`, `lastErrorSource`, and `errorCode` (an explicit `testStatus` from the caller still wins). Regression guard: `tests/unit/codex-oauth-refresh-persist-6352.test.ts`. - **fix(resilience):** `headroom` combo routing did not always select the Codex account with the most free quota ([#6379](https://github.com/diegosouzapw/OmniRoute/issues/6379)) — `orderTargetsByHeadroom` (`open-sse/services/combo/quotaStrategies.ts`) already loaded the per-connection DB snapshot (with decrypted credentials) via `expandTargetsByQuotaAwareConnections`, but discarded it before calling `getSaturation`; for Codex, `fetchCodexSaturation` forwards straight to `fetchCodexQuota(connectionId, connection)`, which needs `connection` (or a prior `registerCodexConnection()` call, which never happens before headroom ranking runs) to read `accessToken` — so it returned `null` for every candidate, saturation failed open to `0` across the board, and ranking fell back to the original combo order regardless of actual free quota. `getSaturation()` and the headroom `SaturationFetcher` seam now accept and thread the loaded connection snapshot through to `fetchCodexQuota`. Kilo's dup flag vs #5903 was a false positive — that issue is about session-sticky reset-aware/least-used selection, not headroom's Codex saturation lookup. Regression guard: `tests/unit/headroom-codex-quota-snapshot-6379.test.ts`. (thanks @eidoog) - **fix(providers):** custom models a provider actually has are no longer dropped from the Free Provider Rankings when both **"Configured only"** and **"Available only"** filters are applied ([#6368](https://github.com/diegosouzapw/OmniRoute/issues/6368)), follow-up to #6150 — `freeProviderRankings.ts::getProviderModels()` only ever walked the static `open-sse/config/providerRegistry.ts` catalog, so a user-added custom model (e.g. a Puter `claude-fable-5` model saved as "Claude Fable 5") never entered the candidate model list the ranking scores against, and could never survive the #6150 configured/available filters even when actually configured and available. It now additively merges the provider's custom models (`db/models.ts::getCustomModels`) into that candidate list via a new pure, de-duping `mergeProviderModels()` helper, before scoring/filtering runs — catalog free/paid filtering elsewhere is untouched. Regression guard: `tests/unit/free-provider-rankings-custom-models-6368.test.ts`. (thanks @shabeer) - **fix(providers):** `cloudflare-ai` no longer silently drops image/non-text content parts ([#6390](https://github.com/diegosouzapw/OmniRoute/issues/6390)) — `transformRequest()`'s `flattenContent()` (added for #2539 to satisfy the Workers AI `/ai/v1/chat/completions` plain-string `content` requirement) mapped any non-text OpenAI content part (e.g. `image_url`) to `""` and joined the rest, so a request carrying an image quietly went out as text-only with the attachment gone and no error surfaced. It now throws a clear error on the first non-text part instead of dropping it silently, which the existing top-level `chatCore.ts` catch already routes through `buildErrorBody()`/`sanitizeErrorMessage()` (same pattern as `buildUrl()`'s missing-Account-ID error). Regression guard: `tests/unit/cloudflare-ai-image-parts-6390.test.ts`. diff --git a/src/sse/services/tokenRefresh.ts b/src/sse/services/tokenRefresh.ts index ab51be2742..05585ae301 100755 --- a/src/sse/services/tokenRefresh.ts +++ b/src/sse/services/tokenRefresh.ts @@ -119,6 +119,24 @@ export async function updateProviderCredentials(connectionId: string, newCredent if (newCredentials.accessToken) { updates.accessToken = newCredentials.accessToken; + // #6352: a successful refresh proves the connection is reachable and its + // refresh_token is valid again — clear any stale auth-failure state + // (testStatus/lastError*) left over from a prior expired/invalid refresh + // token or an upstream 401/403. Without this, a genuinely successful + // rotating-refresh (e.g. Codex/OpenAI) persisted the new access/refresh + // token while leaving the dashboard showing "Auth Failed" forever, + // because the error metadata was never reset here — only the health-check + // sweep (tokenHealthCheck.ts::checkConnection) did this clearing, so any + // OTHER caller of updateProviderCredentials (the manual refresh route, + // the reactive per-request refresh in chat.ts) looked like it "didn't + // pick up" the refreshed token. Explicit `newCredentials.testStatus` + // below still wins for callers that need a specific terminal state. + updates.testStatus = "active"; + updates.lastError = null; + updates.lastErrorAt = null; + updates.lastErrorType = null; + updates.lastErrorSource = null; + updates.errorCode = null; } if (newCredentials.refreshToken) { updates.refreshToken = newCredentials.refreshToken; diff --git a/tests/unit/codex-oauth-refresh-persist-6352.test.ts b/tests/unit/codex-oauth-refresh-persist-6352.test.ts new file mode 100644 index 0000000000..12f23cd67d --- /dev/null +++ b/tests/unit/codex-oauth-refresh-persist-6352.test.ts @@ -0,0 +1,204 @@ +/** + * #6352 — Codex/ChatGPT OAuth refresh: reuse + persist + rotate + clear stale + * "auth failed" state. + * + * Reported symptom: a ChatGPT Plus account added via Codex OAuth stops working + * after ~2 days and the dashboard's manual "Refresh token" button is reported + * as "not enough" — after clicking it the connection still shows `auth failed`. + * + * Root cause traced in this PR: `updateProviderCredentials()` + * (src/sse/services/tokenRefresh.ts) is the shared onPersist callback for every + * refresh entry point (the manual refresh route, the reactive per-request + * refresh in chat.ts's `checkAndRefreshToken`, the Codex auth-file importer). + * It correctly persists the new accessToken/refreshToken/expiresAt — but it + * NEVER cleared the stale auth-failure metadata (`testStatus`, `lastError`, + * `lastErrorType`, `lastErrorSource`, `errorCode`) left over from a prior + * expired/invalid refresh or upstream 401/403. Only the separate background + * health-check sweep (tokenHealthCheck.ts::checkConnection) did this clearing + * inline in its own onPersist callback. So a refresh that ACTUALLY succeeded + * — reusing the stored refresh_token, obtaining a fresh access_token, and even + * rotating in a new refresh_token — still left the connection displaying + * "Auth Failed" forever, because nothing ever reset the error columns. + * + * This test drives `checkAndRefreshToken("codex", ...)` — the exact function + * the real per-request refresh path (src/sse/handlers/chat.ts) calls — against + * a connection pre-seeded in a stale "auth failed" state, with a mocked Codex + * token endpoint. It asserts: + * (a) the refresh REUSES the stored refresh_token (request body assertion), + * (b) the refreshed access_token is PERSISTED back to the connection row, + * (c) a ROTATED refresh_token REPLACES the previously stored one, + * (d) the stale testStatus/lastError* auth-failure fields are CLEARED so the + * dashboard stops showing "Auth Failed" after a refresh that worked. + * + * (d) is the part that reproduces the reported bug: before the fix in + * src/sse/services/tokenRefresh.ts, this assertion fails (RED) because + * updateProviderCredentials left testStatus/lastError untouched. + */ +import test from "node:test"; +import assert from "node:assert/strict"; +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; + +const TEST_DATA_DIR = fs.mkdtempSync(path.join(os.tmpdir(), "omniroute-codex-refresh-6352-")); +process.env.DATA_DIR = TEST_DATA_DIR; +process.env.API_KEY_SECRET = "test-api-key-secret-6352"; + +const core = await import("../../src/lib/db/core.ts"); +const providersDb = await import("../../src/lib/db/providers.ts"); +const tokenRefresh = await import("../../src/sse/services/tokenRefresh.ts"); +const { OAUTH_ENDPOINTS } = await import("../../open-sse/config/constants.ts"); + +async function resetStorage() { + core.resetDbInstance(); + fs.rmSync(TEST_DATA_DIR, { recursive: true, force: true }); + fs.mkdirSync(TEST_DATA_DIR, { recursive: true }); +} + +function jsonResponse(body: unknown, status = 200) { + return new Response(JSON.stringify(body), { + status, + headers: { "content-type": "application/json" }, + }); +} + +type ConnectionRecord = { + id: string; + accessToken?: string | null; + refreshToken?: string | null; + testStatus?: string | null; + lastError?: string | null; + lastErrorType?: string | null; + lastErrorSource?: string | null; + errorCode?: string | null; +}; + +type FetchOptions = { body?: string }; + +async function withMockedFetch( + fetchImpl: (url: unknown, options?: FetchOptions) => Promise, + fn: () => Promise +): Promise { + const originalFetch = globalThis.fetch; + globalThis.fetch = fetchImpl as typeof fetch; + try { + return await fn(); + } finally { + globalThis.fetch = originalFetch; + } +} + +test.beforeEach(async () => { + await resetStorage(); +}); + +test.after(async () => { + await resetStorage(); + fs.rmSync(TEST_DATA_DIR, { recursive: true, force: true }); +}); + +test("checkAndRefreshToken reuses the stored Codex refresh_token, persists the new access_token, rotates the refresh_token, and clears stale auth-failure state (#6352)", async () => { + const now = Date.now(); + + // Seed a connection in the exact "auth failed" state the issue describes: + // a prior refresh/request failure left testStatus/lastError* populated. + const connection = await providersDb.createProviderConnection({ + provider: "codex", + authType: "oauth", + name: "ChatGPT Plus (Codex OAuth)", + accessToken: "codex-stale-access", + refreshToken: "codex-stored-refresh-token", + // Already past the 5-minute Codex refresh lead → checkAndRefreshToken refreshes. + expiresAt: new Date(now - 60_000).toISOString(), + testStatus: "invalid", + lastError: "Refresh token expired. Please re-authenticate this account.", + lastErrorAt: new Date(now - 120_000).toISOString(), + lastErrorType: "upstream_auth_error", + lastErrorSource: "oauth", + errorCode: "401", + } as unknown as Record); + const connectionId = (connection as ConnectionRecord).id; + + const capturedRequests: Array<{ url: string; body: string }> = []; + + await withMockedFetch( + async (url, options: FetchOptions = {}) => { + const body = String(options?.body ?? ""); + capturedRequests.push({ url: String(url), body }); + assert.equal(String(url), OAUTH_ENDPOINTS.openai.token); + + // (a) REUSE assertion: the refresh request must present the refresh_token + // that was actually stored on the connection — not a re-run of the full + // authorization_code flow, and not a stale/blank token. + const params = new URLSearchParams(body); + assert.equal(params.get("grant_type"), "refresh_token"); + assert.equal( + params.get("refresh_token"), + "codex-stored-refresh-token", + "must reuse the connection's stored refresh_token" + ); + + // Simulate OpenAI rotating in a brand-new refresh_token on this refresh. + return jsonResponse({ + access_token: "codex-fresh-access-token", + refresh_token: "codex-rotated-refresh-token", + expires_in: 3600, + }); + }, + async () => { + const connSnapshot = connection as ConnectionRecord; + const refreshed = await tokenRefresh.checkAndRefreshToken("codex", { + connectionId, + accessToken: connSnapshot.accessToken, + refreshToken: connSnapshot.refreshToken, + expiresAt: (connection as Record).expiresAt, + }); + + assert.equal(capturedRequests.length, 1, "the Codex token endpoint must be hit exactly once"); + + // The in-memory result returned to the caller carries the fresh tokens. + assert.equal(refreshed.accessToken, "codex-fresh-access-token"); + assert.equal(refreshed.refreshToken, "codex-rotated-refresh-token"); + + const stored = (await providersDb.getProviderConnectionById( + connectionId + )) as ConnectionRecord; + + // (b) PERSIST assertion: the refreshed access_token must be saved to the row. + assert.equal( + stored.accessToken, + "codex-fresh-access-token", + "the refreshed access_token must be persisted back to the connection" + ); + + // (c) ROTATION assertion: the new refresh_token must REPLACE the old one. + assert.equal( + stored.refreshToken, + "codex-rotated-refresh-token", + "a rotated refresh_token must replace the previously stored one" + ); + assert.notEqual( + stored.refreshToken, + "codex-stored-refresh-token", + "the stale, already-consumed refresh_token must not remain stored" + ); + + // (d) CLEAR-STALE-STATE assertion: a successful refresh must clear the + // auth-failure fields that drive the dashboard's "Auth Failed" badge. + // Before the fix these remained "invalid" / "upstream_auth_error" / "401" + // even though the token had genuinely refreshed. + assert.equal( + stored.testStatus, + "active", + "testStatus must clear to active after a successful refresh" + ); + // The read path (getProviderConnectionById) strips null-valued columns via + // cleanNulls(), so a cleared column surfaces as `undefined`, not `null` — + // both mean "cleared" here. + assert.equal(stored.lastError ?? null, null, "lastError must be cleared"); + assert.equal(stored.lastErrorType ?? null, null, "lastErrorType must be cleared"); + assert.equal(stored.lastErrorSource ?? null, null, "lastErrorSource must be cleared"); + assert.equal(stored.errorCode ?? null, null, "errorCode must be cleared"); + } + ); +});