[WRONG BRANCH] fix: validate manual Codex import identity - #228
Conversation
|
✅ Deterministic PR hygiene checks passed. |
⏳ DRAFT
What to do
Its title has been prefixed with |
github-actions
Bot
changed the title
📝 WalkthroughWalkthroughManual Codex imports now derive email and account identity from access-token claims. Imports reject tokens without email claims, detect duplicate ChatGPT account IDs, and persist the derived email. Tests cover token-based duplicates and unchanged configuration. ChangesManual import identity validation
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant ManualImportAPI
participant AccessToken
participant extractEmail
participant checkManualImportCollision
participant PoolAccountStorage
ManualImportAPI->>AccessToken: read access-token claims
ManualImportAPI->>extractEmail: extract email claim
extractEmail-->>ManualImportAPI: token-derived email
ManualImportAPI->>checkManualImportCollision: ChatGPT account ID
checkManualImportCollision-->>ManualImportAPI: collision result
ManualImportAPI->>PoolAccountStorage: persist token-derived email
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 2⚔️ Resolve merge conflicts 💡
🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/codex/auth-api.ts (1)
1241-1252: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRepeat the account-ID collision check immediately before persistence.
The check at Line 1241 runs before
await verifyCodexAccountWarmup(...). Two requests with different pool IDs and the same token account ID can both pass this check while warmup is pending.commitConflictthen checks onlybody.id, so both credentials and pool rows can be persisted.After Line 1250, run
checkManualImportCollision(derivedAccountId)again and reject a conflict beforesaveCodexAccountCredential. Add a concurrent-import regression test with a warmup barrier.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/codex/auth-api.ts` around lines 1241 - 1252, Repeat the derivedAccountId collision check immediately after verifyCodexAccountWarmup and before saveCodexAccountCredential, returning the same 400 error response when a conflict exists. Add a regression test that uses a warmup barrier to simulate concurrent imports and verifies only one request persists credentials and pool rows.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/codex/auth-api.ts`:
- Around line 1235-1241: Require extractAccountId(undefined, body.accessToken)
to return a value in the manual-import flow before calling
checkManualImportCollision; remove the fallback to body.chatgptAccountId and
reject tokens without an account-ID claim. Add a regression test covering a
token with an email but no supported account-ID claim plus a supplied
chatgptAccountId, asserting that neither credential nor account rows are
created.
---
Outside diff comments:
In `@src/codex/auth-api.ts`:
- Around line 1241-1252: Repeat the derivedAccountId collision check immediately
after verifyCodexAccountWarmup and before saveCodexAccountCredential, returning
the same 400 error response when a conflict exists. Add a regression test that
uses a warmup barrier to simulate concurrent imports and verifies only one
request persists credentials and pool rows.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 58cbc81f-96f4-427d-9144-64bd11f4ee7d
📒 Files selected for processing (3)
src/codex/auth-api.tssrc/codex/auth-collision.tstests/codex-auth-api.test.ts
| // Manual-import identity must come from the token, not request-controlled metadata. | ||
| const derivedAccountId = extractAccountId(undefined, body.accessToken) ?? body.chatgptAccountId; | ||
| const collision = checkAccountIdCollision(derivedAccountId, body.email, body.plan); | ||
| const derivedEmail = extractEmail(undefined, body.accessToken); | ||
| if (!derivedEmail) { | ||
| return jsonResponse({ error: "Access token does not contain an email claim" }, 400); | ||
| } | ||
| const collision = checkManualImportCollision(derivedAccountId); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Require a ChatGPT account-ID claim from the access token.
Line 1236 falls back to request-controlled body.chatgptAccountId. A token with an email claim but no supported account-ID claim can therefore select a different collision identity and bypass checkManualImportCollision.
Reject the import when extractAccountId(undefined, body.accessToken) returns undefined. Add a regression test that submits such a token with a supplied chatgptAccountId and expects no credential or account row.
Proposed fix
- const derivedAccountId = extractAccountId(undefined, body.accessToken) ?? body.chatgptAccountId;
+ const derivedAccountId = extractAccountId(undefined, body.accessToken);
+ if (!derivedAccountId) {
+ return jsonResponse({ error: "Access token does not contain a ChatGPT account ID claim" }, 400);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Manual-import identity must come from the token, not request-controlled metadata. | |
| const derivedAccountId = extractAccountId(undefined, body.accessToken) ?? body.chatgptAccountId; | |
| const collision = checkAccountIdCollision(derivedAccountId, body.email, body.plan); | |
| const derivedEmail = extractEmail(undefined, body.accessToken); | |
| if (!derivedEmail) { | |
| return jsonResponse({ error: "Access token does not contain an email claim" }, 400); | |
| } | |
| const collision = checkManualImportCollision(derivedAccountId); | |
| // Manual-import identity must come from the token, not request-controlled metadata. | |
| const derivedAccountId = extractAccountId(undefined, body.accessToken); | |
| if (!derivedAccountId) { | |
| return jsonResponse({ error: "Access token does not contain a ChatGPT account ID claim" }, 400); | |
| } | |
| const derivedEmail = extractEmail(undefined, body.accessToken); | |
| if (!derivedEmail) { | |
| return jsonResponse({ error: "Access token does not contain an email claim" }, 400); | |
| } | |
| const collision = checkManualImportCollision(derivedAccountId); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/codex/auth-api.ts` around lines 1235 - 1241, Require
extractAccountId(undefined, body.accessToken) to return a value in the
manual-import flow before calling checkManualImportCollision; remove the
fallback to body.chatgptAccountId and reject tokens without an account-ID claim.
Add a regression test covering a token with an email but no supported account-ID
claim plus a supplied chatgptAccountId, asserting that neither credential nor
account rows are created.
![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: 3efca17e3b
ℹ️ 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".
| if (!derivedEmail) { | ||
| return jsonResponse({ error: "Access token does not contain an email claim" }, 400); | ||
| } | ||
| const collision = checkManualImportCollision(derivedAccountId); |
There was a problem hiding this comment.
When a warmup-valid access token has an email but no account-id claim recognized by extractAccountId, derivedAccountId still falls back to request-controlled body.chatgptAccountId; this new collision check therefore compares and later persists an unverified identity, allowing the same token to bypass duplicate detection by submitting another account ID that the upstream warmup accepts. Reject tokens without a token-derived account ID, as is already done for email, or obtain the ID from a provider-verified response before checking the collision.
AGENTS.md reference: src/AGENTS.md:L20-L20
Useful? React with 👍 / 👎.
1 participant
Motivation
emailfrom bypassing account-id collision checks during the manual/api/codex-auth/accountsimport path.Description
checkManualImportCollisionwhich only treats a matchingchatgptAccountIdas a collision for manual imports, avoiding untrusted request metadata.accessTokento contain an email claim and derive the canonical email viaextractEmail(...), rejecting imports when the token lacks an email.codexAccountsentry instead of the JSON-suppliedbody.emailto ensure future collision checks use provider-verified identity evidence.Testing
bun test tests/codex-auth-api.test.ts tests/codex-auth-collision.test.ts(using the repository Bun binary), and the modified tests passed; overall 182 tests in those files passed.bun run typecheckandbun run privacy:scan, both of which succeeded.bun run testin this environment; several unrelated integration tests timed out in this run and were not caused by these changes, but the focused auth tests above (and the collision coverage) exercised and validated the fix.Codex Task
Summary by CodeRabbit