From cb604309ae68d5d557e5a7a7d9bfa54e238e9292 Mon Sep 17 00:00:00 2001 From: Diego Rodrigues de Sa e Souza <8016841+diegosouzapw@users.noreply.github.com> Date: Mon, 20 Jul 2026 01:43:39 -0300 Subject: [PATCH] fix(oauth): require chatgptUserId agreement for Codex account dedup (#7737) (#7825) Codex OAuth completion (persistOAuthConnection, and the duplicated exchange/poll/poll-callback pre-checks in the OAuth completion route) matched an incoming login to an existing connection by email alone whenever neither side had a workspaceId, silently overwriting a second distinct Codex account that happens to share an email with the first. createProviderConnection already disambiguates by chatgptUserId (#6706), but that path was never reached because the pre-checks always found an email match first. Extract the matching logic into a shared findExistingOAuthConnectionMatch() helper in connectionPersistence.ts and use it at all 4 OAuth-completion call sites. When neither the incoming nor existing Codex connection has a workspaceId, only merge if chatgptUserId agrees; otherwise fall through to createProviderConnection so its existing disambiguation applies. Regression test: tests/unit/oauth-connection-persistence-codex-dedup.test.ts --- changelog.d/fixes/7737-codex-multiaccount.md | 1 + .../api/oauth/[provider]/[action]/route.ts | 48 +++------ src/lib/oauth/connectionPersistence.ts | 62 +++++++++--- ...connection-persistence-codex-dedup.test.ts | 98 +++++++++++++++++++ 4 files changed, 165 insertions(+), 44 deletions(-) create mode 100644 changelog.d/fixes/7737-codex-multiaccount.md create mode 100644 tests/unit/oauth-connection-persistence-codex-dedup.test.ts diff --git a/changelog.d/fixes/7737-codex-multiaccount.md b/changelog.d/fixes/7737-codex-multiaccount.md new file mode 100644 index 0000000000..51ae1ad207 --- /dev/null +++ b/changelog.d/fixes/7737-codex-multiaccount.md @@ -0,0 +1 @@ +- fix(oauth): stop Codex OAuth completion from collapsing distinct accounts that share an email but have no workspaceId, by requiring chatgptUserId agreement before merging (#7737) diff --git a/src/app/api/oauth/[provider]/[action]/route.ts b/src/app/api/oauth/[provider]/[action]/route.ts index 154f47bbf8..5b228d053c 100755 --- a/src/app/api/oauth/[provider]/[action]/route.ts +++ b/src/app/api/oauth/[provider]/[action]/route.ts @@ -12,6 +12,7 @@ import { import { persistOAuthConnection, buildOAuthConnectionCreatePayload, + findExistingOAuthConnectionMatch, } from "@/lib/oauth/connectionPersistence"; import { createDeviceFlowTicket, getDeviceFlowTicketStatus } from "@/lib/oauth/deviceFlowTickets"; import { @@ -520,17 +521,9 @@ export async function POST( let connection: any; if (tokenData.email) { const existing = await getProviderConnections({ provider }); - const match = existing.find((c: any) => { - if (c.id && safeEqual(connectionId, c.id)) return true; - // safeEqual: constant-time comparison to prevent timing attacks (CWE-208, finding #258-6/7) - if (!safeEqual(c.email, tokenData.email) || c.authType !== "oauth") return false; - // For Codex, also check workspaceId to avoid overwriting different workspace connections - if (provider === "codex" && tokenData.providerSpecificData?.workspaceId) { - const existingWorkspace = c.providerSpecificData?.workspaceId; - return safeEqual(existingWorkspace, tokenData.providerSpecificData.workspaceId); - } - return true; - }); + // Codex accounts sharing an email require workspaceId/chatgptUserId + // agreement to be treated as the same account (#7737). + const match = findExistingOAuthConnectionMatch(existing, provider, tokenData, connectionId); const matchId = typeof match?.id === "string" ? match.id : null; if (matchId) { connection = await updateProviderConnection(matchId, { @@ -618,17 +611,14 @@ export async function POST( let connection: any; if (result.tokens.email) { const existing = await getProviderConnections({ provider }); - const match = existing.find((c: any) => { - if (c.id && safeEqual(connectionId, c.id)) return true; - // safeEqual: constant-time comparison to prevent timing attacks (CWE-208, finding #258-8/9) - if (!safeEqual(c.email, result.tokens.email) || c.authType !== "oauth") return false; - // For Codex, also check workspaceId to avoid overwriting different workspace connections - if (provider === "codex" && result.tokens.providerSpecificData?.workspaceId) { - const existingWorkspace = c.providerSpecificData?.workspaceId; - return safeEqual(existingWorkspace, result.tokens.providerSpecificData.workspaceId); - } - return true; - }); + // Codex accounts sharing an email require workspaceId/chatgptUserId + // agreement to be treated as the same account (#7737). + const match = findExistingOAuthConnectionMatch( + existing, + provider, + result.tokens, + connectionId + ); const matchId = typeof match?.id === "string" ? match.id : null; if (matchId) { connection = await updateProviderConnection(matchId, { @@ -754,17 +744,9 @@ export async function POST( let connection: any; if (tokenData.email) { const existing = await getProviderConnections({ provider }); - const match = existing.find((c: any) => { - if (c.id && safeEqual(connectionId, c.id)) return true; - // safeEqual: constant-time comparison to prevent timing attacks (CWE-208, finding #258-6/7) - if (!safeEqual(c.email, tokenData.email) || c.authType !== "oauth") return false; - // For Codex, also check workspaceId to avoid overwriting different workspace connections - if (provider === "codex" && tokenData.providerSpecificData?.workspaceId) { - const existingWorkspace = c.providerSpecificData?.workspaceId; - return safeEqual(existingWorkspace, tokenData.providerSpecificData.workspaceId); - } - return true; - }); + // Codex accounts sharing an email require workspaceId/chatgptUserId + // agreement to be treated as the same account (#7737). + const match = findExistingOAuthConnectionMatch(existing, provider, tokenData, connectionId); const matchId = typeof match?.id === "string" ? match.id : null; if (matchId) { connection = await updateProviderConnection(matchId, { diff --git a/src/lib/oauth/connectionPersistence.ts b/src/lib/oauth/connectionPersistence.ts index 5ecc4e6b90..da64863787 100644 --- a/src/lib/oauth/connectionPersistence.ts +++ b/src/lib/oauth/connectionPersistence.ts @@ -19,7 +19,7 @@ import { syncToCloud } from "@/lib/cloudSync"; * Constant-time string comparison to prevent timing-oracle attacks (CWE-208). * Handles null/undefined safely and different-length strings. */ -function safeEqual(a: string | null | undefined, b: string | null | undefined): boolean { +export function safeEqual(a: string | null | undefined, b: string | null | undefined): boolean { if (a == null || b == null) return a === b; const ba = Buffer.from(String(a)); const bb = Buffer.from(String(b)); @@ -27,6 +27,55 @@ function safeEqual(a: string | null | undefined, b: string | null | undefined): return timingSafeEqual(ba, bb); } +/** + * #7737: does this existing Codex connection represent the SAME account as the + * incoming login? Prefer workspaceId when either side has one (Team plans). + * When NEITHER side has a workspaceId (Personal-plan logins, or two accounts + * that both lack a Team workspace), a bare email match is not enough to prove + * it's the same account — two different ChatGPT accounts can share an email + * alias. Require chatgptUserId to agree; otherwise treat it as a distinct + * account so the caller falls through to createProviderConnection (which + * already does this same disambiguation, added under #6706). + */ +function isSameCodexAccount( + existingProviderData: Record | null | undefined, + incomingProviderData: Record | null | undefined +): boolean { + const incomingWorkspace = incomingProviderData?.workspaceId; + const existingWorkspace = existingProviderData?.workspaceId; + if (incomingWorkspace || existingWorkspace) { + return safeEqual(existingWorkspace, incomingWorkspace); + } + const incomingUserId = incomingProviderData?.chatgptUserId; + const existingUserId = existingProviderData?.chatgptUserId; + return Boolean(incomingUserId) && safeEqual(existingUserId, incomingUserId); +} + +/** + * Find the existing OAuth connection (if any) that an incoming token payload + * should be merged into, shared by every OAuth-completion call site + * (persistOAuthConnection, and the exchange/poll/poll-callback branches in + * `src/app/api/oauth/[provider]/[action]/route.ts`). Matches by explicit + * connectionId first, then by same email + auth type — with Codex requiring + * workspaceId/chatgptUserId agreement (#7737) to avoid silently overwriting + * a different Codex account that merely shares an email. + */ +export function findExistingOAuthConnectionMatch( + existing: Array>, + provider: string, + tokenData: Record, + connectionId?: string +): Record | undefined { + return existing.find((c) => { + if (c.id && safeEqual(connectionId, c.id)) return true; + if (!safeEqual(c.email, tokenData.email) || c.authType !== "oauth") return false; + if (provider === "codex") { + return isSameCodexAccount(c.providerSpecificData, tokenData.providerSpecificData); + } + return true; + }); +} + /** * Build the create payload for a brand-new OAuth connection. * @@ -83,16 +132,7 @@ export async function persistOAuthConnection( let connection: any; if (tokenData.email) { const existing = await getProviderConnections({ provider }); - const match = existing.find((c: any) => { - if (c.id && safeEqual(connectionId, c.id)) return true; - if (!safeEqual(c.email, tokenData.email) || c.authType !== "oauth") return false; - // For Codex, also check workspaceId to avoid overwriting a different workspace. - if (provider === "codex" && tokenData.providerSpecificData?.workspaceId) { - const existingWorkspace = c.providerSpecificData?.workspaceId; - return safeEqual(existingWorkspace, tokenData.providerSpecificData.workspaceId); - } - return true; - }); + const match = findExistingOAuthConnectionMatch(existing, provider, tokenData, connectionId); const matchId = typeof match?.id === "string" ? match.id : null; if (matchId) { connection = await updateProviderConnection(matchId, { diff --git a/tests/unit/oauth-connection-persistence-codex-dedup.test.ts b/tests/unit/oauth-connection-persistence-codex-dedup.test.ts new file mode 100644 index 0000000000..30d5a29adf --- /dev/null +++ b/tests/unit/oauth-connection-persistence-codex-dedup.test.ts @@ -0,0 +1,98 @@ +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-7737-")); +process.env.DATA_DIR = TEST_DATA_DIR; + +const core = await import("../../src/lib/db/core.ts"); +const providersDb = await import("../../src/lib/db/providers.ts"); +const { persistOAuthConnection } = await import("../../src/lib/oauth/connectionPersistence.ts"); + +async function resetStorage() { + core.resetDbInstance(); + for (let attempt = 0; attempt < 10; attempt++) { + try { + if (fs.existsSync(TEST_DATA_DIR)) { + fs.rmSync(TEST_DATA_DIR, { recursive: true, force: true }); + } + break; + } catch (error: unknown) { + const code = (error as NodeJS.ErrnoException)?.code; + if ((code === "EBUSY" || code === "EPERM") && attempt < 9) { + await new Promise((resolve) => setTimeout(resolve, 50 * (attempt + 1))); + } else { + throw error; + } + } + } + fs.mkdirSync(TEST_DATA_DIR, { recursive: true }); +} + +test.beforeEach(async () => { + await resetStorage(); +}); +test.after(async () => { + core.resetDbInstance(); + fs.rmSync(TEST_DATA_DIR, { recursive: true, force: true }); +}); + +test("persistOAuthConnection must not merge two distinct Codex accounts that share an email but have different chatgptUserId and no workspaceId", async () => { + const accountA = await persistOAuthConnection("codex", { + email: "shared@example.com", + accessToken: "token-account-a", + refreshToken: "refresh-account-a", + expiresIn: 3600, + providerSpecificData: { chatgptUserId: "user-a" }, + }); + + const accountB = await persistOAuthConnection("codex", { + email: "shared@example.com", + accessToken: "token-account-b", + refreshToken: "refresh-account-b", + expiresIn: 3600, + providerSpecificData: { chatgptUserId: "user-b" }, + }); + + const rows = await providersDb.getProviderConnections({ provider: "codex" }); + + assert.notEqual( + accountB.id, + accountA.id, + "second Codex login must create a distinct connection, not reuse the first account's row" + ); + assert.equal(rows.length, 2, "both Codex accounts must persist as separate connections"); + + const rowA = rows.find((row: { id: string }) => row.id === accountA.id); + assert.equal( + rowA?.accessToken, + "token-account-a", + "account A's access token must survive account B's login unmodified" + ); +}); + +test("persistOAuthConnection still merges a re-login for the SAME Codex chatgptUserId with no workspaceId", async () => { + const first = await persistOAuthConnection("codex", { + email: "solo@example.com", + accessToken: "token-first", + refreshToken: "refresh-first", + expiresIn: 3600, + providerSpecificData: { chatgptUserId: "user-solo" }, + }); + + const second = await persistOAuthConnection("codex", { + email: "solo@example.com", + accessToken: "token-second", + refreshToken: "refresh-second", + expiresIn: 3600, + providerSpecificData: { chatgptUserId: "user-solo" }, + }); + + assert.equal(second.id, first.id, "re-authenticating the same Codex user must update the same row"); + + const rows = await providersDb.getProviderConnections({ provider: "codex" }); + assert.equal(rows.length, 1, "no duplicate connection should be created for the same chatgptUserId"); + assert.equal(rows[0]?.accessToken, "token-second", "the row must reflect the latest tokens"); +});