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
This commit is contained in:
Diego Rodrigues de Sa e Souza
2026-07-20 01:43:39 -03:00
committed by GitHub
parent 8a4a363bb9
commit cb604309ae
4 changed files with 165 additions and 44 deletions

View File

@@ -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)

View File

@@ -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, {

View File

@@ -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<string, any> | null | undefined,
incomingProviderData: Record<string, any> | 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<Record<string, any>>,
provider: string,
tokenData: Record<string, any>,
connectionId?: string
): Record<string, any> | 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, {

View File

@@ -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");
});