Repository navigation
fix(codex): stop pool accounts from spending the native main account (#7398) - #7414
Conversation
…t and fix access token email extraction
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configuration
📒 Files selected for processing: 24
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
✅ Deterministic PR hygiene checks passed. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e2ab0d778d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| export function reconcileMainCodexAccountRuntimeState(): boolean { | ||
| const read = readCodexTokensResult(); | ||
| const read = readMainAuth(); | ||
| if (read.status === "missing" || read.status === "api-key-only") observeMainCodexCollisionIdentity(); |
There was a problem hiding this comment.
When auth.json is briefly absent during a non-atomic rewrite, this clears the observed main identity before the function's own unknown-identity branch preserves the remaining safety state. fetchMainAccountInfoWhileOwned explicitly treats a missing file as a possible rewrite gap (main-account-probe.ts:242-247), so a request arriving in that window can make the stored duplicate eligible and send its credential, defeating this change's protection until the main login is observed again. Only a positively identified API-key-only login or another confirmed identity transition should retire the collision observation; a single missing read should preserve it like other transient failures.
Useful? React with 👍 / 👎.
| ...(duplicateMain ? { | ||
| selectionExcludedReason: "duplicate_main" as const, | ||
| selectionExcludedAction: CODEX_MAIN_DUPLICATE_ACTION, |
There was a problem hiding this comment.
For an existing stored duplicate, the server now returns this reason and action, but neither account-list consumer recognizes them: src/cli/account-api.ts:337,411-414 accepts and projects only plan_excluded, while gui/src/hooks/useCodexAccountPool.ts:94 and gui/src/components/codex-account-pool-cards.tsx:103-120 do the same. Consequently the CLI silently strips the exclusion even from JSON output, and the dashboard shows no reason or remediation—and may still display the unusable row as selected. Extend both client DTOs and renderers to preserve and present duplicate_main and its action.
Useful? React with 👍 / 👎.
Preserve the implementation and regression coverage carried into dev by PR lidge-jun#7414, including the original PR lidge-jun#7399 fixes and its additional native-main admission and stored-duplicate protections. The resulting tree matches dev at 1202598.
…origin hints, store read sizing - Move the three new test registrations from inline layout.json (2002 lines, over the 2000-line file-size ratchet per lidge-jun#7413) into scripts/test-layout/layout-persistent-dashboard.json, following the lidge-jun#7414 explicit-shard precedent. - Clarify the remember-device hint: remembered sessions require a same-origin HTTPS dashboard (or loopback), all locales. - readRecords sizes its buffer from the validated stat and rejects mid-read store growth instead of silently truncating at a fixed limit. - structure/dashboard-and-usage.md: ordinary logout invalidates only the current browser session; remembered-device logout also forgets the device record and derived sessions.
Summary
Closes #7398. Builds on #7399 by @terrytan95, preserving both original commits/authorship; the additional commits cover already-stored duplicates and native relogin.
src/codex/auth-collision.ts:100previously checked only pool rows, allowing a clone of the native main login to masquerade as an alternate quota account.src/oauth/chatgpt-shared.ts:44omitted the namespaced profile email;src/codex/auth-api/login-flow.ts:266substituted a local label before identity checks and lost the JWT plan when WHAM failed. Keep identity evidence until admission, then apply the display fallback.account-list.ts:155exposesselectionExcludedReason: "duplicate_main"andselectionExcludedActionin the dashboard DTO. No GUI layout changes.Decisions and security review
C3 task, security-depth review. Missing email cannot prove a distinct member: same ChatGPT account ID + same personal/workspace bucket + equal or missing email is conservatively excluded. Known different members, workspace IDs and plan buckets remain independent. Legacy local-ID email placeholders use token metadata.
Item 5: yes, a manual Pool preference can still use caller main. It is not an exclusive binding. Four synthetic tests prove preference/excluded/cooldown paths may select caller main, exact binding refuses, and an observed paused main refuses. Preserve this established policy; Clarify manual pool pin versus caller-main fallback and consider pool-only policy #7410 tracks product policy/UX. Successful caller-main selection/fallback emits static reason-only diagnostics, with no account IDs, emails or credentials.
Independent review repairs. Sol found unowned native reads and a read-fence exemption in the first draft, unowned OAuth reads in fix(codex): prevent duplicate pool registration of native main account and fix access token email extraction #7399, refresh-time reauth poisoning, and stale exclusion after API-key-only native login. All accepted and repaired with regressions. Final independent static review: PASS at
e2ab0d778d4f8ebb270f0dd4f79c2c81c27c309f; reviewer tests NOT RUN, coordinator tests below. Argon was available in the catalog but failed at runtime with 403 (Argon delegated audit terminates with provider 403 on local Responses endpoint #7411); it supplies no approval evidence.Scope and ownership. No real credential reads, user settings changes, new dependency declarations, account deletion, proxy/app shutdown, force push, direct dev push or merge. Original author credit retained. This PR does not modify/comment on/close fix(codex): prevent duplicate pool registration of native main account and fix access token email extraction #7399.
Validation constraints. Preservation fixtures now represent a legitimate separate plan bucket, retaining their policy assertions. Layout registrations use a shard rather than raising size limits. Bun 1.4.3 is a scratch-local test runner; no global installation/config changes.
Review-ready disposition. Exact-head hosted CI and independent static security review passed, and all 565 focused regressions passed. The five macOS changed-mode timeouts remain recorded in Service update recovery tests exceed default 5-second timeout on macOS #7417/Messages ingress alternate-main success-control times out in changed-mode suite #7418; this is not a claim that the full local suite passed. Marked ready for maintainer review, with no merge or approval bypass.
Issue ledger
Verification
Base:
eb4bf967001327624acca1341a9a51aba0a3ca05; final local head:e2ab0d778d4f8ebb270f0dd4f79c2c81c27c309f.All behavioral tests use isolated temporary homes and fake credentials/JWTs. No live inference.
bun run typecheckbun run structure:checkbun run privacy:scanbun test tests/test-layout*.test.ts tests/ci-workflows/file-size-ratchet.test.tsbun run buildbun run test:changedci, gates, storage/API usage, keyring, Docker/npm smokes, docs/structure and hygiene completed successfully. CI run. macOS/Windows and other non-applicable jobs were SKIPPED per policy, not certified. Local timeout evidence remains explicit.Focused final command:
OCX_TEST_RUNNER_BUN=<scratch Bun 1.4.3> bun test tests/codex-integration/codex-auth-api.test.ts tests/codex-integration/codex-auth-collision.test.ts tests/oauth/chatgpt-oauth.test.ts tests/codex-integration/codex-auth-context.test.ts tests/codex-integration/codex-stored-main-duplicate.test.ts tests/codex-integration/codex-pool-caller-fallback-policy.test.ts tests/codex-integration/codex-preserve-signed-in.test.ts tests/lab/core-lab-boundary.test.ts(executed using the scratch 1.4.3 binary directly).Checklist
Co-authored-by: terrytan95 10609214+terrytan95@users.noreply.github.com