fix(codex): verify auth.json identity, not just content, before publishing - #3199
Conversation
…shing The native-main publisher hashed auth.json and re-checked the hash before renaming its staged file over it. Both guards asked whether the CONTENT matched; neither asked whether it was still the same file. That is the weaker question at a boundary where rename(2) replaces unconditionally. A Codex writer that rewrites auth.json with identical bytes owns the target afterwards, and the publisher would overwrite it - silently replacing the user's own codex login result with a token staged from an earlier read. Carry the target's dev+ino alongside the hash and compare both. An unreadable identity on either side fails closed: unprovable is not the same as equal. This narrows the window rather than eliminating it. A truly atomic compare-and-swap needs renameat2(RENAME_EXCHANGE) or equivalent, which Bun does not expose. Refs #2999
|
✅ 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: 16a9dad96c
ℹ️ 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".
| return { | ||
| path, | ||
| rawSha256: sha256(raw), | ||
| identity: statIdentity(path), |
There was a problem hiding this comment.
If an external writer replaces auth.json with identical bytes between readFileSync and this subsequent stat, the snapshot combines the old read with the replacement's identity; every pre-rename check can then pass and overwrite that replacement. Read through an open descriptor and obtain its identity with fstatSync, or use a stat-read-stat sequence that rejects an identity change, so the hash and identity describe the same filesystem object.
AGENTS.md reference: src/AGENTS.md:L20-L20
Useful? React with 👍 / 👎.
| function statIdentity(path: string): { dev: number; ino: number } | null { | ||
| try { | ||
| const stat = statSync(path); | ||
| return { dev: Number(stat.dev), ino: Number(stat.ino) }; |
There was a problem hiding this comment.
When auth.json resides on a filesystem whose 64-bit device or inode value exceeds Number.MAX_SAFE_INTEGER, the default numeric stat result—and the explicit Number(...) conversion—can lose low bits, allowing distinct file identities to compare equal and defeating this credential-publication guard. Request bigint stats with statSync(path, { bigint: true }) and retain bigint values for the comparison.
AGENTS.md reference: src/AGENTS.md:L20-L20
Useful? React with 👍 / 👎.
리뷰 · 우선순위 74 / 80이 PR은 이미 구체적으로 테스트도 이슈 #2999가 가리킨 재현 모양을 그대로 따라갑니다. 다만 PR 본문이 솔직히 말한 것처럼, 이 가드는 창을 줄일 뿐 원자적 비교-교환은 아닙니다. 점수 74는 “현재 라인 105-127 ( 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
1 participant
Summary
#2999 named two credential-safety races in native-main refresh. #3112 (
fecb77a9) closed the coordination half — refresh now serializes on the canonicalCODEX_HOMEclaim. This is the publication half, which that PR explicitly did not cover.persistRefreshedMainAuthJsonhashedauth.jsonand re-checked the hash in two hooks before renaming its staged file over it. Both guards asked whether the content matched. Neither asked whether it was still the same file.That is the weaker question to ask at this boundary, because
src/config/atomic-write.tsruns three separate syscalls:rename(2)replaces unconditionally. A Codex writer that rewroteauth.jsonwith identical bytes owns the target afterwards — different inode, same hash — and the publisher would overwrite it. The user's owncodex loginresult is silently replaced by a token OpenCodex staged from an earlier read, with no error and no recovery path: the nextcodexinvocation just uses a credential the user did not authorize.The change
MainAuthJsonCredentialnow carries the target'sdev/inoalongsiderawSha256, andassertMainAuthJsonSnapshotUnchangedcompares both. An unreadable identity on either side fails closed — unprovable is not the same as equal.Refusal keeps the existing signal,
MainAuthJsonChangedDuringRefreshError, so callers retry against the new state rather than proceeding with a credential they no longer own.Honest scope
This narrows the window; it does not eliminate it. An identity check still happens before the rename rather than atomically with it. A genuine compare-and-swap needs
renameat2(RENAME_EXCHANGE)on Linux or an equivalent elsewhere, and Bun exposes neither —rgforrenameat2,RENAME_EXCH,linkSync,O_EXCL, andexchangedataacrosssrc/finds nothing to build on.So this closes the case the old guard provably missed and shrinks the remaining one. #2999 should stay open for the atomic primitive, or be re-scoped to it.
Verification
Exact head
16a9dad96:bun test ./tests/codex-main-account-refresh.test.ts— 7 pass, 0 fail, 24 expect() calls, including four new cases.bun test ./tests/native-main-claim.test.ts ./tests/responses-native-main-refresh.test.ts ./tests/codex-auth-context.test.ts— 77 pass, 0 fail.bun x tsc --noEmit— clean.The new cases follow the issue's own reproduction, using the
setMainAuthJsonBeforeRenameHookForTestsseam it names in step 5:auth.jsonwould be worse than losing the refresh.Red-green: removing the identity check while keeping the content hash turns case 2 red (6 pass / 1 fail) and leaves the rest green. That is the proof the check earns its place rather than duplicating the hash.
Checklist
dev