fix(oauth): normalize imported token expiry - #335
Conversation
|
🦞👀 Pull request received. I will update this pull request when review starts. |
|
Codex review: needs maintainer review before merge. Reviewed August 25, 2026, 5:31 AM ET / 09:31 UTC. ClawSweeper reviewWhat this changesThe branch normalizes OAuth tokens imported through Regression provenancePossible regression — probable (reproduction; reviewed change). No predecessor PR is attributed. Merge readinessThis PR remains necessary: current main still bypasses the canonical expiry normalizer for Priority: P1 Review scores
Verification
Live VerificationCommand: Result: FAIL (partial) — step 2 Assertions:
How this fits together
flowchart LR
A[Token JSON input] --> B[Vault set command]
B --> C[Expiry normalization]
C --> D[Shared credential vault]
D --> E[Token freshness check]
E --> F[Use bearer token]
E --> G[Refresh token request]
Decision needed
Why: A token payload has no issuance timestamp, so the implementation cannot distinguish a fresh response from a replayed one without choosing between immediate refresh and treating expires_in as remaining lifetime. Before merge
Agent review detailsSecurityNone. Review metrics
Root-cause clusterRelationship: Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Merge the shared-normalizer reuse while retaining the documented absolute-expiry escape hatch for delayed or replayed token responses. Do we have a high-confidence way to reproduce the issue? Yes. The PR body supplies a real built-CLI loopback reproduction on main and after-fix runs that observe both token-endpoint redemption and the bearer sent to the MCP server. Is this the best way to solve the issue? Yes. Reusing the existing persistence normalizer fixes the divergent write path without introducing a parallel expiry implementation, while the docs state the necessary delayed-import constraint. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against ae3d9000c320. LabelsLabel justifications:
EvidenceWhat I checked:
Likely related people:
Rank-up movesOptional improvements that raise the rating; they are not merge blockers.
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (5 earlier review cycles)
|
|
Added the requested after-fix real CLI behavior proof to the PR body at head The isolated loopback run used the built CLI for
All credentials were non-secret fixtures and their values were omitted from the transcript. This directly covers the P1 auth-provider merge risk. @clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
`expires_in` is relative to token issuance, but `vault set` imports credentials that may already be held, so the payload carries no issuance timestamp. Normalization can only read a relative expiry as lifetime remaining at import, which over-extends a stale response and lets refreshable_bearer send a dead token instead of refreshing. State that contract in the config guide: an explicit `expires_at` / `expiresAt` is stored verbatim and is the way to import delayed credentials, while `expires_in` alone means remaining lifetime. Cover both aliases with a delayed-import regression so the passthrough cannot regress into the relative reading. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`expires_at` and `expiresAt` are persistence-only fields, so reading them straight off `OAuthTokens` failed `tsc` on CI. Narrow the loaded entry to a local shape the way tests/oauth-persistence.test.ts already does, and assert the effective stored expiry equals the supplied value rather than merely differing from the relative one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
Summary
vault setthrough the canonical persistence preparation pathexpires_atfromexpires_inwithout changing explicit expiry aliasesCloses #334
Real behavior proof
One harness, three runs, all driving the real built CLI (
node dist/cli.js, not the test runner) against a loopback fake OAuth provider + MCP server, with isolatedHOMEand XDG roots so the developer vault is never touched. The provider counts everyPOST /tokenand records everyAuthorizationheader presented at/mcp; it honors both the imported and the rotated access token, so nothing below is an artifact of the fixture rejecting a valid credential. All credentials are non-secret fixtures and are redacted anyway.Run 1 and 2 differ only by the patch. Run 3 covers the delayed-import case raised in review.
1. Before — main
ae3d900: the fresh token is redeemed on first use2. After — this branch
c5f11eb: the imported token is used as-is3. After — delayed import declaring its real expiry: still refreshes first
What the transcripts show
ae3d900, fresh importc5f11eb, fresh importc5f11eb, delayed importexpires_in: 3600expires_in: 3600expires_in: 3600+expires_at: now+30expires_atpersistednow + 3600snow + 30s(verbatim)POST /tokenon first use/mcpmcporter list demoRun 1 is the P1 defect observed end to end: a one-hour-valid grant is spent on its very first use, because
shouldRefreshCachedTokenfalls back to "expires_in+refresh_token⇒ due" when no absolute expiry was persisted (src/oauth-token-refresh.ts:188). Note that the command still succeeds — that is what makes it worth fixing rather than merely noisy. Nothing surfaces to the user, but with rotating refresh tokens (RFC 9700 §4.14.2) the grant the operator just imported has already been consumed.Run 2 is the fix: the absolute expiry is persisted, the timestamp comparison wins, and the provider is never contacted.
Run 3 answers the review finding directly. A payload that declares its real remaining lifetime keeps that value verbatim and still takes the conservative refresh path before the token is used — the patch does not blanket-suppress refresh for imported credentials, it suppresses it only where the payload says the token is genuinely live.
Expiry import contract (addresses the delayed-import finding)
expires_inis relative to token issuance, butvault setimports credentials that may already be held, so the payload carries no issuance timestamp. Normalization can only read a relative expiry as lifetime remaining at import — which over-extends a stale response, andreadExplicitRefreshableBearerToken(src/oauth-token-refresh.ts:456) then returns that token directly instead of refreshing.The mechanism to express a real expiry already exists and is untouched by this patch:
withStoredExpiryreturns tokens verbatim whenexpires_atorexpiresAtis present, andvalidateOAuthTokensaccepts both aliases. What was missing is that this was neither documented nor guarded, so this branch adds both:docs/config.mdnow states the contract — an explicitexpires_at/expiresAt(Unix seconds) is stored verbatim and is how delayed credentials should be imported;expires_inalone means lifetime remaining at import. Scripts that replay a stored token response should convert once at capture time.tests/vault-command.test.tscovers a delayed import through both aliases: a token issued 55 minutes before import keeps its real 5-minutes-remaining expiry and is not rewritten tonow + 3600. Run 3 above is the same case through the real CLI.Both unit guards were mutation-checked — deleting the alias short-circuit in
withStoredExpiryfails them, so they are not vacuous.This keeps the #334 fix intact for the fresh-import case rather than reverting to the unconditional conservative refresh, which would reintroduce the reported bug.
Reproduction harness (drop in
tmp/vault-expiry-proof.mjs, thenpnpm build && node tmp/vault-expiry-proof.mjs . after-fix fresh/... delayed)Tests
pnpm exec vitest run tests/vault-command.test.ts(8 passed)pnpm exec vitest run tests/vault-command.test.ts tests/vault-cli.integration.test.ts tests/oauth-persistence-stores.test.ts(22 passed: 8 + 9 + 5)pnpm lint:oxlint(passed)pnpm exec oxfmt --check docs/config.md tests/vault-command.test.ts src/cli/vault-command.ts src/oauth-persistence-stores.ts(passed)pnpm test(1,637 passed, 26 skipped; 1 pre-existing failure inoauth-refresh-process.integration.test.ts, untouched by this branch)pnpm check(fails atformat:checkontests/cli-list-stdio-logs.test.tsandtests/list-inline-stdio.test.ts— both pre-existing onmainand not among the four files this branch changes)