fix(oauth): guard expires_in parsing against NaN across token responses - #1418
fix(oauth): guard expires_in parsing against NaN across token responses#1418Bruce-Yii wants to merge 3 commits into
Conversation
github-actions
Bot
added
the
intake: hygiene-blocked
|
✅ READY
Review readiness checklist
✅ 4/4 boxes ticked. This pull request has been marked Ready for Review. |
📝 WalkthroughWalkthroughThe change validates OAuth ChangesOAuth expiry hardening
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
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 |
![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
🤖 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/account-store.ts`:
- Around line 496-502: Validate computed expiry values before storing or
returning them: in src/codex/account-store.ts lines 496-502,
src/oauth/anthropic.ts lines 86-91, and src/oauth/chatgpt.ts lines 54-59, use
the fallback when the calculated expiry timestamp expiresAt or expires is
non-finite; in src/oauth/kimi.ts lines 156-165, reject the payload when the
calculated expires value is non-finite.
🪄 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: fe5a5394-d030-4bfd-b195-a2cb61707fab
📒 Files selected for processing (8)
src/codex/account-store.tssrc/oauth/anthropic.tssrc/oauth/chatgpt.tssrc/oauth/kimi.tstests/anthropic-hardening.test.tstests/chatgpt-token-expiry.test.tstests/codex-account-store.test.tstests/kimi-oauth-identity.test.ts
|
The hygiene check still requires a maintainer security review for the OAuth changes in I am replying to the latest comment only. The earlier comment chain was not available.
|
|
Addressed the CodeRabbit Major finding (computed-expiry overflow) in commit |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== HEAD =='
git rev-parse HEAD
echo '== Changed files from parent =='
git diff --name-status HEAD^
echo '== Diff =='
git diff --unified=50 HEAD^ -- \
src/codex/account-store.ts \
src/oauth/anthropic.ts \
src/oauth/chatgpt.ts \
src/oauth/kimi.ts \
tests/anthropic-hardening.test.ts \
tests/chatgpt-token-expiry.test.ts \
tests/codex-account-store.test.ts \
tests/kimi-oauth-identity.test.ts
echo '== Production symbol outline =='
ast-grep outline src/codex/account-store.ts --items all --match 'getValidCodexToken|expiresAt|expires_in' || true
ast-grep outline src/oauth/anthropic.ts --items all --match 'credsFrom|expiresAt|expires_in' || true
ast-grep outline src/oauth/chatgpt.ts --items all --match 'credsFromToken|expiresAt|expires_in' || true
ast-grep outline src/oauth/kimi.ts --items all --match 'parseTokenPayload|expiresAt|expires_in' || trueLength of output: 38981 🏁 Script executed: #!/bin/bash
set -euo pipefail
for spec in \
'src/codex/account-store.ts:480:495' \
'src/oauth/anthropic.ts:80:100' \
'src/oauth/chatgpt.ts:45:68' \
'src/oauth/kimi.ts:150:177' \
'tests/anthropic-hardening.test.ts:50:85' \
'tests/chatgpt-token-expiry.test.ts:20:50' \
'tests/codex-account-store.test.ts:285:340' \
'tests/kimi-oauth-identity.test.ts:70:105'
do
file="${spec%%:*}"
rest="${spec#*:}"
start="${rest%%:*}"
end="${rest##*:}"
echo "== ${file}, Lines ${start}-${end} =="
nl -ba "$file" | sed -n "${start},${end}p"
doneLength of output: 949 🏁 Script executed: #!/bin/bash
set -euo pipefail
for spec in \
'src/codex/account-store.ts:480:495' \
'src/oauth/anthropic.ts:80:100' \
'src/oauth/chatgpt.ts:45:68' \
'src/oauth/kimi.ts:150:177' \
'tests/anthropic-hardening.test.ts:50:85' \
'tests/chatgpt-token-expiry.test.ts:20:50' \
'tests/codex-account-store.test.ts:285:340' \
'tests/kimi-oauth-identity.test.ts:70:105'
do
file="${spec%%:*}"
rest="${spec#*:}"
start="${rest%%:*}"
end="${rest##*:}"
echo "== ${file}, Lines ${start}-${end} =="
awk -v start="$start" -v end="$end" 'NR >= start && NR <= end { printf "%6d\t%s\n", NR, $0 }' "$file"
doneLength of output: 13927
I inspected the source and tests only. I did not execute the test suite in this sandbox. You are interacting with an AI system. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
git status --short
git show --no-ext-diff --format=fuller --stat 3b7b7471
git diff --no-ext-diff 3b7b7471^ 3b7b7471 -- \
src/oauth/anthropic.ts \
src/oauth/chatgpt.ts \
src/oauth/kimi.ts \
src/codex/account-store.ts \
tests/anthropic-hardening.test.ts \
tests/chatgpt-token-expiry.test.ts \
tests/codex-account-store.test.ts \
tests/kimi-oauth-identity.test.ts
for f in \
src/oauth/anthropic.ts \
src/oauth/chatgpt.ts \
src/oauth/kimi.ts \
src/codex/account-store.ts \
tests/anthropic-hardening.test.ts \
tests/chatgpt-token-expiry.test.ts \
tests/codex-account-store.test.ts \
tests/kimi-oauth-identity.test.ts
do
echo "===== $f ====="
ast-grep outline "$f" --items all || true
rg -n -C 8 'expires_in|expiresAt|Number\.isFinite|MAX_VALUE|1e999' "$f" || true
doneLength of output: 64293 🏁 Script executed: #!/bin/bash
set -euo pipefail
base="$(git merge-base HEAD origin/dev 2>/dev/null || git merge-base HEAD dev)"
echo "Merge base: $base"
git diff --no-ext-diff --name-status "$base"...HEAD
git diff --no-ext-diff --check "$base"...HEAD
git diff --no-ext-diff --unified=80 "$base"...HEAD -- \
src/oauth/anthropic.ts \
src/oauth/chatgpt.ts \
src/oauth/kimi.ts \
src/codex/account-store.ts \
tests/anthropic-hardening.test.ts \
tests/chatgpt-token-expiry.test.ts \
tests/codex-account-store.test.ts \
tests/kimi-oauth-identity.test.ts
sed -n '1,115p' tests/anthropic-hardening.test.ts
sed -n '1,85p' tests/chatgpt-token-expiry.test.ts
sed -n '1,125p' tests/kimi-oauth-identity.test.ts
sed -n '1,335p' tests/codex-account-store.test.tsLength of output: 1091 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== Local refs ==="
git branch -a
git remote -v
echo "=== PR metadata ==="
gh pr view 1418 --repo lidge-jun/opencodex \
--json number,headRefName,headRefOid,baseRefName,baseRefOid,files \
--jq '{number,headRefName,headRefOid,baseRefName,baseRefOid,files: [.files[].path]}'
echo "=== PR file patch metadata ==="
gh api "repos/lidge-jun/opencodex/pulls/1418/files?per_page=100" \
--jq '.[] | {filename, status, additions, deletions, patch}'Length of output: 15466
The full PR diff at
I did not run the test suite in this sandbox. The code review found no remaining finding. You are interacting with an AI system. |
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 (2)
tests/anthropic-hardening.test.ts (1)
46-70: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winVerify the documented fallback duration in the regression tests.
The new tests prove only that the expiry is finite and future-dated. They do not prove that invalid values use the required 3600-second fallback.
tests/anthropic-hardening.test.ts#L46-L70: Assert an expiry near3_300_000milliseconds because the five-minute skew is applied.tests/chatgpt-token-expiry.test.ts#L7-L44: Assert an expiry near3_600_000milliseconds.tests/codex-account-store.test.ts#L264-L312: Assert persistedexpiresAtnear3_600_000milliseconds.Use a small timing tolerance around the expected values.
This is required to verify the PR’s 3600-second fallback objective.
🤖 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 `@tests/anthropic-hardening.test.ts` around lines 46 - 70, Update the regression assertions to verify the documented 3600-second fallback using a small timing tolerance: in tests/anthropic-hardening.test.ts lines 46-70, assert expiry is near 3,300,000 ms from now after the five-minute skew; in tests/chatgpt-token-expiry.test.ts lines 7-44, assert expiry is near 3,600,000 ms; and in tests/codex-account-store.test.ts lines 264-312, assert persisted expiresAt is near 3,600,000 ms. Keep the existing invalid and overflowing expiry scenarios intact.tests/chatgpt-token-expiry.test.ts (1)
7-19: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert the fallback duration.
The tests accept any finite future expiry. A regression that falls back to one second would still pass. Capture timestamps around the refresh and assert that ChatGPT’s expiry is close to
3_600_000milliseconds.This is required to verify the PR’s 3600-second fallback objective.
Also applies to: 21-44
🤖 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 `@tests/chatgpt-token-expiry.test.ts` around lines 7 - 19, Strengthen the test “refresh with a non-finite expires_in falls back to a finite default expiry” by capturing the current time before and after refreshChatGPTToken, then asserting cred.expires is approximately 3,600,000 milliseconds after the refresh timestamp. Retain the existing finite and future-expiry checks while allowing a reasonable timing tolerance.
🤖 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/account-store.ts`:
- Around line 500-501: Reject negative expiry durations across all four OAuth
paths: in src/codex/account-store.ts lines 500-501, src/oauth/anthropic.ts lines
90-91, and src/oauth/chatgpt.ts lines 58-59, require expires_in to be
non-negative before calculating the expiry timestamp; in src/oauth/kimi.ts lines
158-163, treat payload.expires_in < 0 as malformed. Preserve the existing
finite-value overflow fallback for non-negative durations.
---
Outside diff comments:
In `@tests/anthropic-hardening.test.ts`:
- Around line 46-70: Update the regression assertions to verify the documented
3600-second fallback using a small timing tolerance: in
tests/anthropic-hardening.test.ts lines 46-70, assert expiry is near 3,300,000
ms from now after the five-minute skew; in tests/chatgpt-token-expiry.test.ts
lines 7-44, assert expiry is near 3,600,000 ms; and in
tests/codex-account-store.test.ts lines 264-312, assert persisted expiresAt is
near 3,600,000 ms. Keep the existing invalid and overflowing expiry scenarios
intact.
In `@tests/chatgpt-token-expiry.test.ts`:
- Around line 7-19: Strengthen the test “refresh with a non-finite expires_in
falls back to a finite default expiry” by capturing the current time before and
after refreshChatGPTToken, then asserting cred.expires is approximately
3,600,000 milliseconds after the refresh timestamp. Retain the existing finite
and future-expiry checks while allowing a reasonable timing tolerance.
🪄 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: e5dba4bc-108f-4b3e-a92f-1b37590b5dd9
📒 Files selected for processing (8)
src/codex/account-store.tssrc/oauth/anthropic.tssrc/oauth/chatgpt.tssrc/oauth/kimi.tstests/anthropic-hardening.test.tstests/chatgpt-token-expiry.test.tstests/codex-account-store.test.tstests/kimi-oauth-identity.test.ts
flyingsquirrel0419
left a comment
flyingsquirrel0419
left a comment
There was a problem hiding this comment.
The fix direction is correct and the computed-expiry overflow guard closes the original NaN/Infinity failure mode, but I cannot sponsor or approve this authentication-boundary change on the current head.
One blocker remains across all four parse sites: finite negative expires_in values are still accepted. In src/codex/account-store.ts, src/oauth/anthropic.ts, and src/oauth/chatgpt.ts, a negative duration produces a finite timestamp in the past instead of taking the malformed-response fallback; in src/oauth/kimi.ts, it should fail the required-field contract. Require expires_in >= 0 in the three fallback paths and reject < 0 in Kimi, then add negative-value regression coverage for each behavior.
Please also tighten the new fallback tests to assert the actual fallback window (approximately 3600 seconds, or approximately 3300 seconds for Anthropic after its five-minute skew), rather than only finite && future. That prevents a one-second or otherwise incorrect fallback from satisfying the tests.
The failing hygiene check is the expected unsponsored_surface gate, not a product-test failure. I will reconsider maintainer-sponsored only after these input-boundary cases are fixed and the exact-head checks are green.
|
Tip For best results, initiate chat on the files or code changes.
The remaining hygiene requirement is a maintainer security review of the OAuth changes in:
After that review, a maintainer can apply the You are interacting with an AI system. |
|
Addressed the maintainer review (flyingsquirrel0419) in commit 1. Negative 2. Exact fallback window in tests: the fallback tests now assert the actual window, not just
3. Regression coverage: +4 negative-value tests (one per site), 55/55 targeted, OAuth+anthropic+account-store suite 414/414, The |
23a4245 to
355b69e
Compare
|
Tip For best results, initiate chat on the files or code changes.
You are interacting with an AI system. |
Ingwannu
added
the
maintainer-sponsored
github-actions
Bot
removed
intake: hygiene-blocked
|
Tip For best results, initiate chat on the files or code changes.
All four readiness checks and the deterministic hygiene checks passed. The ready state applies to head You are interacting with an AI system. |
Ingwannu
left a comment
Ingwannu
left a comment
There was a problem hiding this comment.
Approved on exact head 355b69e5b003edebe3f543d1818a9618fa075b14 after an explicit authentication-boundary review.
The four token-adoption paths now validate the raw duration as a non-negative finite number and validate the computed timestamp before storage or return. Anthropic, ChatGPT, and the Codex account store take the bounded one-hour fallback; Kimi rejects malformed input. The negative, overflow, non-finite, and fallback-window regressions cover the failure modes without logging or widening credential destinations.
Exact-head evidence:
- focused OAuth/account-store tests: 414/414 pass;
- typecheck and privacy review: pass;
- CodeRabbit: pass, both inline blockers resolved on this head;
- cross-platform CI rerun: Linux shards, macOS, keyring, storage policy, and package installs all pass;
- current-
devdistance: two documentation commits, within policy.
The earlier hygiene failure is the superseded pre-sponsorship run; the later hygiene run and current cross-platform gate are green. The author still needs to tick the now-proven local/CI checklist item so the automated readiness gate can move the PR out of draft. This approval does not authorize self-merge; authentication changes should still receive the additional maintainer review requested on the PR when practical.
|
Tip For best results, initiate chat on the files or code changes.
All four readiness checks passed at head You are interacting with an AI system. |
3 participants
Summary
Fixes #1417: OAuth token responses with an invalid
expires_in(missing,NaN, string, negative, or numeric-overflowing) produce aNaN/Infinity/past expiry in stored credentials.NaN <= xis always false andInfinitynever expires, so such credentials break the refresh loop. This is the token-response counterpart of the already-merged #1369 fix — same data-quality class, remaining at the upstream parse sites.Root cause
src/oauth/anthropic.ts—Date.now() + data.expires_in * 1000— missingexpires_in→undefined * 1000=NaNsrc/oauth/kimi.ts—typeof !== "number"check letsNaNthrough (typeof NaN === "number")src/oauth/chatgpt.ts—?? 3600only guardsnull/undefined;NaNor a numeric string passes throughsrc/codex/account-store.ts— no validation at allAdditional failure modes found in review:
expires_in(e.g.Number.MAX_VALUE) passesNumber.isFinitebut overflows toInfinityonce multiplied by 1000 — the computed expiry must also be guarded.expires_inproduces a finite already-past expiry, silently bypassing the malformed-response fallback.Changes
All four sites now require
typeof === "number" && Number.isFinite(...) && >= 0(falling back to a 3600s default so the credential stays refreshable), plus a guard on the computed expiry timestamp:src/oauth/anthropic.ts—credsFrom(): non-finite/negative/overflowing → 3600s default.src/oauth/kimi.ts—parseTokenPayload(): non-finite, negative, or overflowingexpires_inrejected as malformed.src/oauth/chatgpt.ts—credsFromToken(): non-finite/negative/overflowing → 3600s default.src/codex/account-store.ts— refresh path: non-finite/negative/overflowing → 3600s default.Per the lazy-refresh contract established in #1369, this change only guards at parse/adoption time — it does not touch
getLoginStatusor expiry comparison logic.Tests
tests/chatgpt-token-expiry.test.ts(new): non-finite, string, overflowing, and negativeexpires_inall fall back to exactly ~3600s (tolerance 30s)tests/anthropic-hardening.test.ts: non-finite, overflowing, and negative → ~3300s (3600s minus the 5-minute skew)tests/codex-account-store.test.ts: non-finite, overflowing, and negative → persistedexpiresAt~3600stests/kimi-oauth-identity.test.ts: non-finite, overflowing, and negativeexpires_inall rejectedVerification (head
355b69e5, rebased ontodev9c05134):bun x tsc --noEmit→ cleanmergeable_state: cleanFixes #1417
Review readiness checklist
This PR stays in draft until every box below is ticked. Tick all four boxes once the requirements are met:
All CI tests are green on my local testing.
I pushed my PR to the latest dev commit.
I resolved all correct Codex and CodeRabbit findings.
My PR is ready for review.