diff --git a/README.md b/README.md index b53d0fb..4979a26 100644 --- a/README.md +++ b/README.md @@ -178,7 +178,11 @@ export ANTHROPIC_CLI_VERSION=2.2.0 - Provides an account switcher via `opencode auth login` when multiple accounts are found; persists selection to `~/.local/share/opencode/claude-account-source.txt` - Syncs credentials to `auth.json` on startup and every 5 minutes as a fallback; that same tick proactively refreshes once the token is within an hour of expiry - On Windows, writes to both `%USERPROFILE%\.local\share\opencode\auth.json` and `%LOCALAPPDATA%\opencode\auth.json` +- Re-reads the credential source on every cache miss, so an account rotated by something other than this plugin — the `claude` CLI in another terminal, a second OpenCode instance, or a switcher like [claude-swap](https://github.com/realiti4/claude-swap) — gets picked up mid-session without a restart. Bounded by the same 30s cache, so it adds at most about two source reads a minute under load. A stored token is adopted whenever it is usable, and when it isn't only if the one already held is also unusable — otherwise a failed write-back would resurrect the pre-refresh token it left behind +- Guards credential write-back with the access token the refresh started from, so a switch landing mid-refresh can't write one account's rotated tokens into another account's slot - Retries API requests on 429 (rate limit) and 529 (overloaded) with exponential backoff, respecting `retry-after` headers +- On a 429 that outlives those backoff retries, re-reads the source once and retries only if the access token changed, so a rate limit another process has already resolved by switching accounts isn't surfaced. A changed token isn't proof of a switch — a routine refresh of the same account changes it too — so this costs at most one extra request +- On a 401, recovers in place rather than surfacing it: adopts an externally rotated token if the source now holds one, otherwise forces an OAuth refresh, then retries the request. Bounded at two attempts, so a rejected token costs at most three API calls. A 401 that survives recovery is returned unmodified, without SSE stream transformation, since it carries an error body rather than a stream - Refreshes directly via `POST https://claude.ai/v1/oauth/token` using the runtime's own `fetch` (no LLM tokens consumed, no subprocess). Requests are triggered within 60 seconds of expiry on the API request path and within an hour on the background tick; concurrent refreshes of one account share a single request, since each rotation invalidates the previous refresh token - Falls back to the `claude` CLI only within the 60-second window, the point at which Claude Code will actually rotate the token — running it earlier costs a real API request and returns the same token. New tokens are written back to Keychain (macOS) or credentials file (Linux/Windows) to keep stored credentials in sync with rotated refresh tokens - If credentials aren't OAuth-based, the auth loader returns `{}` and falls through to API key auth diff --git a/specs/2026-07-29-external-credential-rotation-design.md b/specs/2026-07-29-external-credential-rotation-design.md new file mode 100644 index 0000000..36e57b1 --- /dev/null +++ b/specs/2026-07-29-external-credential-rotation-design.md @@ -0,0 +1,290 @@ +# External credential rotation + +Date: 2026-07-29 +Status: approved, not yet implemented + +## Context + +The plugin assumes it is the only writer to the active Claude Code credential. +That assumption is encoded explicitly at `src/credentials.ts:328-334`: + +> macOS keychain sources stay on the in-memory path; their state is mutated only +> by our own `writeBackCredentials`, so no external-update vector exists for them. + +The assumption is false. Several things rotate or replace that credential: + +- `claude-swap` (`cswap`), switching accounts manually or via `cswap auto` +- the `claude` CLI running in another terminal +- a second OpenCode instance +- Anthropic revoking a token server-side + +This design makes the plugin correct in the presence of external mutation. It is +motivated by cswap but contains no cswap-specific code, no subprocess, no `PATH` +dependency, and no new configuration. + +### Why cswap is safe to cooperate with + +Verified in cswap's source rather than assumed: + +- `switcher.py:4694` — the switch-time backup copies **live** credential bytes + into the departing account's slot. A token this plugin rotates is captured by + cswap on the way out, so it is never orphaned and the account is not + quarantined for a dead refresh token. +- `switcher.py:5074` — cswap fails fast if it cannot read the live credential, + rather than overwriting state that has no safety copy. + +Consequence: refreshing the **active** credential is safe with no coordination. +The plugin is indistinguishable from Claude Code refreshing its own token. This +asymmetry is why the design targets the active credential only. + +## Problems + +1. **External rotation is invisible.** `refreshIfNeeded` re-reads only `file` + sources (`src/credentials.ts:331`). The sole re-read of a Keychain source + during normal operation is in the 401 handler (`src/index.ts:374`). An + external switch produces no 401, because the previous access token is still + valid — so the session stays pinned to the account that was active at startup + for the life of that token (~10h). + +2. **Stale-snapshot write-back.** The proactive refresh tick (`src/index.ts:224`, + every 5 min, acting within 1h of expiry) refreshes from an in-memory snapshot + and calls `writeBackCredentials`. If the active credential was replaced + externally in the meantime, the plugin writes the _stale_ account's rotated + tokens over the _current_ account's slot. Because refresh tokens rotate on + use, the losing copy is dead. + +3. **401 recovery gives up too early.** `src/index.ts:371-392` re-reads the + source and retries only if the access token changed. When the source still + holds the rejected token, it surfaces the 401. + `forceRefreshActiveAccount()` (`src/credentials.ts:606`) exists for exactly + this case and has **no non-test call sites**. `invalidateCredentialCache()` + (`:647`) and `reloadActiveAccount()` (`:585`) are likewise dead. + +4. **Rate-limit errors leak after an external switch.** When cswap switches + because an account hit its quota, the plugin keeps using the exhausted + account until its cache expires, surfacing 429s that a re-read would avoid. + +## Goals + +- An external account switch takes effect in a live session without a restart. +- A request that fails on an expired or revoked token recovers within the same + request where possible, and always leaves the next prompt working. +- The plugin never writes one account's tokens into another account's slot. + +## Non-goals + +Explicitly out of scope. Each was considered and rejected: + +- **cswap-aware UX** — account roster, email labels, or usage in + `opencode auth login`. Requires cswap on `PATH`. +- **Pinning OpenCode to a non-active cswap slot** (parallel accounts). Cold + slots carry expired access tokens; cswap exposes no "refresh slot N" command, + so the plugin would become an unsynchronized second writer to cswap's backup + store, needing its `fcntl.flock` and `.prev` retention semantics. All of the + risk, only parallelism in return. +- **Plugin-driven `cswap switch`.** cswap already handles rotation; driving it + from here would fight `cswap auto`. +- **Any env var or opt-in gate.** Nothing here is cswap-specific, so there is + nothing to gate. + +## Design + +### 1. Re-read every source in `refreshIfNeeded` + +`src/credentials.ts:331` — drop the `target.source === "file"` guard so Keychain +sources are re-read too. + +Cost is bounded: `refreshIfNeeded` runs only on a `getCachedCredentials()` cache +miss, so at most about twice a minute under load (`CREDENTIAL_CACHE_TTL_MS`, +`src/credentials.ts:26`). A `security find-generic-password` call is ~10-50ms. + +This resolves problems 1 and 2 together: the plugin refreshes and writes back +whatever is _currently_ active rather than a stale snapshot. + +**New failure path.** `readKeychainService` throws on a locked, denied, or timed +out Keychain (`src/keychain.ts:96-142`), whereas the file path swallows errors +inside `readCredentialsFile`. The re-read must therefore be wrapped so a +transient Keychain failure degrades to the in-memory credentials instead of +propagating into the request path. + +Worst-case latency for an external switch to take effect: one cache TTL (30s). + +**The adopt must be validated.** Taking any non-null stored blob over valid +in-memory credentials is unsafe, because `performRefresh` ignores +`writeBackCredentials`'s return value. When the read succeeds but the write +fails — a malformed stored blob, or an ACL allowing read but not +`add-generic-password` — memory holds freshly refreshed credentials while the +store holds the pre-refresh blob. On the reactive path that blob has under 60s +left, since that window is the only reason a refresh happened. Adopting it +re-enters `performRefresh` with a refresh token the successful refresh just +rotated dead, so OAuth fails and the code falls through to two 60s `claude` +spawns — on every cache miss, indefinitely, on the authentication path. + +The rule: **adopt a usable stored blob always; adopt an unusable one only when +what we already hold is also unusable.** + +| stored | in memory | action | +| -------- | --------- | ------------------------------------------------------------------ | +| usable | usable | adopt — this is the external-switch case | +| usable | unusable | adopt | +| unusable | usable | decline — this is the failed-write-back case | +| unusable | unusable | adopt; nothing is lost and the stored refresh token may still work | + +Accepted cost: if an external switch installs an account whose stored token is +already expired while ours is still usable, the session keeps its own +credentials until they expire and adopts the switched-in account then. cswap +freshens a target before activating it, so a newly-active slot is normally +fresh, and the alternative is the failure loop above. + +Second accepted residual, on the proactive path. The background timer refreshes +an hour ahead of expiry, so a write-back failure there orphans a blob with up to +~1h left — which reads as "usable" and is adopted, clobbering the fresh +credentials. The consequence is categorically milder than the reactive case: the +adopted token is genuinely valid, so requests keep succeeding, and the +`CLI_FALLBACK_THRESHOLD_MS` gate in `performRefresh` declines to spawn `claude` +while credentials remain usable. The cost is a handful of wasted background OAuth +round trips over that final hour, resolving in one CLI-fallback recovery once the +blob drops under 60s. + +No guard on the re-read can close this, and that is why it is out of scope here: +the re-read cannot distinguish "the store is stale because our write failed" from +"the store changed because cswap switched" — both present as _store disagrees +with memory, store is usable_. The information exists only at the +`writeBackCredentials` call in `performRefresh`, whose return value is discarded. +See Follow-ups in the plan. + +The same branch clears the borrowed-credentials flag. A read from the account's +own source returns that account's own credentials, so it is by definition no +longer running on a lender's — matching the invariant `refreshBorrowedAccount` +maintains at every other adoption site. + +### 2. Compare-and-swap on write-back + +`writeBackCredentials` (`src/keychain.ts:442`) gains an optional expected prior +access token. When supplied and the stored blob no longer carries that token, +the write is skipped and logged rather than performed. + +This closes the residual window in problem 2: the OAuth round-trip between +re-read and write-back is seconds wide, and an external switch can land inside +it. + +Nearly free — the Keychain branch already reads the existing blob at +`src/keychain.ts:475` to preserve unrelated fields, so this adds a comparison +rather than a read. Callers that legitimately have no prior token (initial +sync) omit the argument and keep today's behavior. + +On mismatch the session continues from memory; the next cache miss re-reads and +converges. + +### 3. Bounded 401 recovery loop + +Replace the single if/else at `src/index.ts:371-392` with a loop of at most two +recovery attempts. Each attempt: + +1. Re-read the source (`reloadCredentialsFromSource`). If it yields a token + different from the one just used, retry with it. +2. Otherwise call `forceRefreshActiveAccount()` — the currently dead function — + and retry with its result. +3. If neither produces a token different from the one just used, stop. + +The no-progress check is what bounds the loop; the attempt cap is defence in +depth. + +Most cases resolve on the first attempt. A switch to a slot whose access token +is cold yields _null_ from step 1, not a different token — +`reloadCredentialsFromSource` rejects anything expiring within 60s +(`src/credentials.ts:706-710`) — so step 2 runs immediately. The second attempt +exists for the narrower race where step 1 returns a token that is valid-looking +but has itself just been rotated again by a concurrent writer: the retry 401s, +and the next iteration finds the source unchanged and force-refreshes. + +One consequence to note: when step 1 returns null it does **not** update +`account.credentials`, so the force refresh runs against whatever account was +in memory. If a switch has landed, that is the previous account. The refresh +succeeds and the request recovers, but the write-back is correctly suppressed +by the compare-and-swap from change 2, since the store now holds a different +account. The session continues from memory and the next cache miss converges on +the switched-in account. + +`forceRefreshActiveAccount` already guards borrowed credentials +(`src/credentials.ts:617`), writes back on success, and updates the cache, so it +is drop-in. + +**Intentional behavior change.** `preserveResponseUnchanged` (`src/index.ts:462`) +decides whether the response bypasses `transformResponseStream()`. Today a +non-retried 401 bypasses it but a retried-then-still-401 does not. After this +change the flag is set from the status after the loop exits +(`response.status === 401`), so both surface the raw error body consistently and +a recovered request is still stream-transformed. + +### 4. Re-read on 429 with a changed token + +After the 401 block, if the response is 429, re-read the source once. Retry only +if the access token changed — which is true exactly when an external switch +happened, and false otherwise, making this free in the common case. + +No `src/http.ts` change is needed. `fetchWithRetry` already returns immediately +when `retry-after` exceeds the cap (`src/http.ts:59-66`), which is the +quota-exhaustion signal. + +This must sit **after** the 401 block and **before** the long-context beta loop +(`src/index.ts:396`). A long-context 429 produces no token change, so it falls +through to the beta loop untouched. + +### Rejected: triggering rotation inside `http.ts` + +`fetchWithRetry` is shared with the OAuth refresh path +(`src/credentials.ts:233`). A 429 from the token endpoint must never trigger +account recovery. All recovery logic therefore stays at the `src/index.ts` call +sites. + +## Error handling + +| Failure | Behavior | +| --------------------------------------------- | --------------------------------------------------------------------------------- | +| Keychain locked/denied/timeout during re-read | Caught; fall back to in-memory credentials. Never propagates to the request path. | +| CAS mismatch on write-back | Skip the write, log, continue from memory. Next cache miss converges. | +| `forceRefreshActiveAccount` returns null | Recovery loop stops; the 401 surfaces with the raw body. | +| Borrowed credentials active | `forceRefreshActiveAccount` already declines (`src/credentials.ts:617`). | +| Both recovery attempts exhausted | Return the 401 unchanged, as today. | + +## Testing + +Follows existing convention: colocated `*.test.ts`, run via +`node --test --experimental-strip-types`. + +`src/credentials.test.ts` + +- `refreshIfNeeded` re-reads a Keychain source and adopts an externally rotated token +- a throwing `refreshAccount` degrades to in-memory credentials rather than propagating +- write-back is skipped when the stored token no longer matches the expected prior token + +`src/keychain.test.ts` + +- `writeBackCredentials` honors the expected-prior-token argument and is unchanged when omitted + +`src/index.test.ts` + +- 401, external rotation present → retry succeeds, response is stream-transformed +- 401, source yields no better token (unchanged, or null because it is expiring) + → force refresh on the first attempt → retry succeeds +- 401, first retry also rejected → second attempt force-refreshes → retry succeeds +- 401, unrecoverable → response returned raw, no stream transformation +- 429 with a changed token → retried once +- 429 with an unchanged token → not retried, falls through to the beta loop +- a 429 from the OAuth token endpoint surfaces as a failed refresh and does not + loop account recovery + +## Risks + +- **Keychain read volume.** Up to ~2 subprocesses/minute per session. Acceptable, + and the cache TTL is unchanged. +- **30s convergence window.** An external switch is invisible for up to one cache + TTL. Change 4 keeps that window from surfacing rate-limit errors; the residual + is up to 30s of usage landing on the previous account. Shortening the TTL would + cost a subprocess per request and is not worth it. +- **Behavior change to `preserveResponseUnchanged`.** Described in change 3. + Low blast radius, but it is a change and should be called out in the changelog. +- **Coupling to cswap internals.** None. This design reads and writes only the + active Claude Code credential, which is a Claude Code interface, not a cswap + one. diff --git a/specs/2026-07-29-external-credential-rotation-plan.md b/specs/2026-07-29-external-credential-rotation-plan.md new file mode 100644 index 0000000..366db42 --- /dev/null +++ b/specs/2026-07-29-external-credential-rotation-plan.md @@ -0,0 +1,1222 @@ +# External Credential Rotation Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Make the plugin correct when something other than itself rotates the active Claude Code credential — cswap switching accounts, the `claude` CLI in another terminal, a second OpenCode instance, or a server-side revocation. + +**Architecture:** Four changes to existing files. Re-read every credential source (not just `file`) on cache miss; compare-and-swap on write-back so a refresh can never land in another account's slot; replace the single-shot 401 handler with a bounded recovery loop that wires up the already-written-but-never-called `forceRefreshActiveAccount()`; re-read once on a 429 so a resolved rate limit isn't surfaced. No cswap dependency, no subprocess, no new configuration. + +**Tech Stack:** TypeScript, Node's built-in test runner (`node --test --experimental-strip-types`), oxlint/oxfmt. Tests are colocated `*.test.ts` and load rewritten copies of `src/` modules from a temp dir with stubbed `keychain.ts` / `logger.ts` / `child-process.ts`. + +**Spec:** `specs/2026-07-29-external-credential-rotation-design.md` + +--- + +## File Structure + +| File | Responsibility | Change | +| --------------------------- | --------------------------------------------- | ------------------------------------------------------------ | +| `src/credentials.ts` | Account state, caching, refresh orchestration | Re-read all sources; pass expected-prior-token to write-back | +| `src/keychain.ts` | OS credential store I/O | `writeBackCredentials` gains compare-and-swap | +| `src/index.ts` | Plugin entry, auth loader, fetch interception | 401 recovery loop; 429 re-read | +| `src/credentials.test.ts` | Harness + credential tests | Source-aware stub; new + repaired tests | +| `src/keychain.test.ts` | Keychain tests | CAS tests | +| `src/index.test.ts` | Plugin/fetch tests | Repaired 401 test; new recovery tests | +| `README.md`, `CHANGELOG.md` | Docs | Behavior note | + +## Known-breaking existing tests + +These pass today and **will fail** partway through. Each is repaired in the task that breaks it — do not "fix" them by reverting the implementation. + +| File:line | Assertion | Why it breaks | Repaired in | +| ----------------------------- | --------------------------------------------------------- | ------------------------------------- | ----------- | +| `src/credentials.test.ts:315` | `__getReadCount() === 1` | `getCachedCredentials` now also reads | Task 2 | +| `src/credentials.test.ts:437` | `__getReadCount() === 0` | first call now reads | Task 2 | +| `src/credentials.test.ts:735` | `__getReadCount() === readsBefore + 1` | now two reads | Task 2 | +| `src/index.test.ts:1189` | "does not retry a 401 when the source token is unchanged" | that is the behavior being changed | Task 4 | + +Three further Task 2 breakages were found during execution that this plan did not predict. All share the root cause above — the up-front re-read makes the stub's source blob visible to accounts whose in-memory credentials differ from it: + +| Test | Fix | +| ----------------------------------------------------------------------- | ---------------------------------------------------------------------------------------------------------------------------------------------- | +| `reloadCredentialsFromSource bypasses cache` | prime via `__setCredentialsForSource` so the rotation under test still lands | +| `does not spawn the claude CLI while credentials are still usable` | pin the source to the account's own value | +| `falls back to the claude CLI once credentials reach the expiry window` | its `__setReadHook` rotated the store on read #2, which is now the pre-CLI re-read rather than the post-CLI one; move the threshold to read #3 | + +The last one matters beyond bookkeeping. Left at read #2 the result-token assertion still passes while `__getExecSyncCount()` drops to 0 — the CLI fallback silently stops being exercised, and the test keeps looking meaningful. Verified by reverting the threshold: `actual: 0, expected: 1`. Relaxing that count instead of moving the threshold would have left dead coverage behind. + +--- + +### Task 1: Make the credentials test harness source-aware + +The stub `refreshAccount(source)` ignores `source` and returns one global blob (`src/credentials.test.ts:148-153`). Harmless while only `file` sources were re-read; once every source is re-read, multi-account tests get one account's credentials for every source. This is a test-only change: the suite must stay green. + +**Files:** + +- Modify: `src/credentials.test.ts:126-190` (the `tempKeychain` stub literal) + +- [x] **Step 1: Add per-source overrides to the stub keychain** + +In the `writeFile(tempKeychain, ...)` template literal, replace the `refreshAccount` and `__setCredentials` definitions and add two exports. Keep `__setCredentials` working as the default so existing tests are untouched. + +```js +let bySource = {} + +export function refreshAccount(source) { + readCount += 1 + if (readError) throw new Error("Keychain read denied") + if (readHook) readHook() + if (Object.prototype.hasOwnProperty.call(bySource, source)) { + return bySource[source] + } + return credentials +} + +export function __setCredentials(c) { + credentials = c +} + +export function __setCredentialsForSource(source, c) { + bySource[source] = c +} +``` + +Each test calls `loadCredentialsWithCountingKeychain` fresh, which imports a new module instance with an empty `bySource`, so no reset helper is needed. + +- [x] **Step 2: Record the expected-prior-token argument on write-back** + +Replace the stub `writeBackCredentials` so Task 3 can assert on it. Four parameters, matching the real signature after Task 3. + +```js +export function writeBackCredentials( + source, + creds, + configDir, + expectedPriorAccessToken, +) { + writeCount += 1 + writes.push({ source, creds, configDir, expectedPriorAccessToken }) + return true +} +``` + +- [x] **Step 3: Widen the harness return types** + +In the `keychainModule` type annotation (both the declared return type at `src/credentials.test.ts:56-64` and the cast at `:226-235`), add the new members and widen `__getWrites`: + +```ts + __setCredentialsForSource: (source: string, c: Creds | null) => void + __getWrites: () => Array<{ + source: string + creds: Creds + configDir?: string + expectedPriorAccessToken?: string + }> +``` + +- [x] **Step 4: Run the full suite to confirm nothing regressed** + +Run: `pnpm test 2>&1 | tail -8` +Expected: `pass 249`, `fail 0` + +- [x] **Step 5: Commit** + +```bash +git add src/credentials.test.ts +git commit -m "test: make credentials keychain stub source-aware" +``` + +--- + +### Task 2: Re-read every credential source on cache miss + +**Files:** + +- Modify: `src/credentials.ts:326-334` +- Test: `src/credentials.test.ts` + +- [x] **Step 1: Write the failing tests** + +Append inside the existing top-level `describe` block in `src/credentials.test.ts`: + +```ts +it("refreshIfNeeded adopts credentials rotated externally in a keychain source", async () => { + const originalNow = Date.now + const now = 1_700_000_000_000 + Date.now = () => now + + try { + const { credentialsModule, keychainModule } = + await loadCredentialsWithCountingKeychain(now + 10 * 60_000) + + credentialsModule.initAccounts([ + { + label: "Account 1", + source: "keychain", + credentials: { + accessToken: "before-switch", + refreshToken: "rt-before", + expiresAt: now + 10 * 60_000, + }, + }, + ]) + + // An external process (cswap, the claude CLI, a second OpenCode) + // replaces the stored credential with a different account's. + keychainModule.__setCredentials({ + accessToken: "after-switch", + refreshToken: "rt-after", + expiresAt: now + 10 * 60_000, + }) + + const result = await credentialsModule.refreshIfNeeded() + + assert.equal(result?.accessToken, "after-switch") + } finally { + Date.now = originalNow + } +}) + +it("refreshIfNeeded keeps in-memory credentials when the source read throws", async () => { + const originalNow = Date.now + const now = 1_700_000_000_000 + Date.now = () => now + + try { + const { credentialsModule, keychainModule } = + await loadCredentialsWithCountingKeychain(now + 10 * 60_000) + + credentialsModule.initAccounts([ + { + label: "Account 1", + source: "keychain", + credentials: { + accessToken: "in-memory", + refreshToken: "rt", + expiresAt: now + 10 * 60_000, + }, + }, + ]) + + keychainModule.__setReadError(true) + + const result = await credentialsModule.refreshIfNeeded() + + assert.equal(result?.accessToken, "in-memory") + } finally { + Date.now = originalNow + } +}) +``` + +- [x] **Step 2: Run the new tests to verify they fail** + +Run: `node --test --experimental-strip-types --test-name-pattern="rotated externally in a keychain source|source read throws" src/credentials.test.ts 2>&1 | tail -12` +Expected: FAIL — the first asserts `'before-switch' !== 'after-switch'`; the second throws `Keychain read denied`. + +- [x] **Step 3: Implement the re-read** + +In `src/credentials.ts`, replace lines 326-334 (the comment block and the `if (target.source === "file")` guard) with: + +```ts +// Pick up credentials replaced externally — cswap switching accounts, the +// claude CLI in another terminal, or a second OpenCode instance. This used +// to be limited to file sources on the assumption that a keychain entry is +// only ever mutated by our own writeBackCredentials; that assumption is +// false. Bounded by getCachedCredentials's 30s TTL, so it fires at most +// ~2x/min under load. +// +// A keychain read shells out to `security`, which throws when the keychain +// is locked, access is denied, or the call times out. Degrade to the +// in-memory credentials rather than take down the request path. +try { + const stored = refreshAccount(target.source, target.configDir) + if (stored) target.credentials = stored +} catch (err) { + log("source_reread_failed", { + source: target.source, + error: err instanceof Error ? err.message : String(err), + }) +} +``` + +- [x] **Step 4: Run the new tests to verify they pass** + +Run: `node --test --experimental-strip-types --test-name-pattern="rotated externally in a keychain source|source read throws" src/credentials.test.ts 2>&1 | tail -8` +Expected: `pass 2`, `fail 0` + +- [x] **Step 5: Repair the three read-count assertions** + +Run: `pnpm test 2>&1 | grep -E "^not ok|✖" | head` +Expected: three failures, at `src/credentials.test.ts` lines ~315, ~437, ~735. + +`src/credentials.test.ts:315` — one extra read now happens inside `getCachedCredentials` before `__setReadError(true)`: + +```ts +assert.equal(keychainModule.__getReadCount(), 2) +``` + +`src/credentials.test.ts:437` — the first `getCachedCredentials()` is a cache miss and now re-reads; the second is a cache hit and does not: + +```ts +assert.equal( + keychainModule.__getReadCount(), + 1, + "cache miss re-reads the source once; the cached call does not", +) +``` + +`src/credentials.test.ts:729-735` (test: `fallback uses a valid in-memory account without a keychain read`) — `refreshIfNeeded` now re-reads the target's source up front, in addition to the existing post-OAuth-failure re-read. Pin the expired account's own stored value so the up-front read is a no-op rather than swapping in the other account's blob. + +Immediately after that test's `credentialsModule.initAccounts([...])` call, add: + +```ts +keychainModule.__setCredentialsForSource("Claude Code-credentials-aabbccdd", { + accessToken: "stale-suffixed", + refreshToken: "rt-suffixed", + expiresAt: now - 1_000, +}) +``` + +Then update the assertion: + +```ts +assert.equal( + keychainModule.__getReadCount(), + readsBefore + 2, + "one up-front re-read of the target's own source, one after the failed OAuth refresh", +) +``` + +- [x] **Step 6: Run the full suite** + +Run: `pnpm test 2>&1 | tail -8` +Expected: `pass 251`, `fail 0` + +- [x] **Step 7: Commit** + +```bash +git add src/credentials.ts src/credentials.test.ts +git commit -m "fix: re-read keychain sources so external credential rotation is picked up" +``` + +--- + +### Task 3: Compare-and-swap on write-back + +Closes the window between reading a credential and writing its refreshed replacement — seconds wide, because an OAuth round-trip sits in the middle. + +**Files:** + +- Modify: `src/keychain.ts:442-503` +- Modify: `src/credentials.ts:376`, `:499`, `:622-628` +- Test: `src/keychain.test.ts` + +- [x] **Step 1: Write the failing tests** + +Add `credentialBlobMatches` to the existing import from `./keychain.ts` at `src/keychain.test.ts:15-23`. + +Add a new top-level `describe` (place it directly above `describe("writeBackCredentials (file source)"` at line 585): + +```ts +describe("credentialBlobMatches", () => { + const blob = JSON.stringify({ + claudeAiOauth: { + accessToken: "account-a", + refreshToken: "rt-a", + expiresAt: 1, + }, + }) + + it("accepts a blob still holding the expected token", () => { + assert.equal(credentialBlobMatches(blob, "account-a"), true) + }) + + it("rejects a blob replaced by another account", () => { + assert.equal(credentialBlobMatches(blob, "account-b"), false) + }) + + it("rejects an unparseable blob rather than assuming a match", () => { + assert.equal(credentialBlobMatches("not json", "account-a"), false) + }) +}) +``` + +Then add this end-to-end test **inside** the existing `describe("writeBackCredentials (file source)")` block, after the test at line 599. It mirrors that test's `HOME` isolation pattern: + +```ts +it("skips the write when the stored token is no longer the expected one", async () => { + const originalHome = process.env.HOME + const tempHome = await mkdtemp(join(tmpdir(), "opencode-claude-auth-cas-")) + process.env.HOME = tempHome + + try { + const claudeDir = join(tempHome, ".claude") + mkdirSync(claudeDir, { recursive: true }) + const credPath = join(claudeDir, ".credentials.json") + // Another process switched accounts after we read "expected-at". + writeFileSync( + credPath, + JSON.stringify({ + claudeAiOauth: { + accessToken: "switched-in-at", + refreshToken: "switched-in-rt", + expiresAt: 1000, + }, + }), + { encoding: "utf-8", mode: 0o600 }, + ) + + const result = writeBackCredentials( + "file", + { + accessToken: "our-refreshed-at", + refreshToken: "our-refreshed-rt", + expiresAt: 2000, + }, + undefined, + "expected-at", + ) + + assert.equal(result, false) + const written = JSON.parse(readFileSync(credPath, "utf-8")) + assert.equal( + written.claudeAiOauth.accessToken, + "switched-in-at", + "the switched-in credential must survive untouched", + ) + } finally { + if (typeof originalHome === "string") { + process.env.HOME = originalHome + } else { + delete process.env.HOME + } + } +}) +``` + +- [x] **Step 2: Run to verify they fail** + +Run: `node --test --experimental-strip-types --test-name-pattern="credentialBlobMatches|no longer the expected one" src/keychain.test.ts 2>&1 | tail -12` +Expected: FAIL — `credentialBlobMatches is not a function`, and the skip test writes anyway (`result` is `true`). + +- [x] **Step 3: Implement the guard** + +In `src/keychain.ts`, add above `writeBackCredentials` (line 442): + +```ts +/** + * Whether a stored credential blob still carries the access token we expect. + * + * Guards write-back against an external switch landing between the read that + * produced the token being refreshed and the write of its replacement. An + * unparseable blob returns false: a write into state we cannot identify is + * exactly what this is meant to prevent. + */ +export function credentialBlobMatches( + raw: string, + expectedAccessToken: string, +): boolean { + const parsed = parseCredentials(raw) + return parsed?.accessToken === expectedAccessToken +} +``` + +Change the signature at line 442 to: + +```ts +export function writeBackCredentials( + source: string, + creds: ClaudeCredentials, + configDir?: string, + expectedPriorAccessToken?: string, +): boolean { +``` + +In the `source === "file"` branch, immediately after `const raw = readFileSync(credPath, "utf-8")`: + +```ts +if ( + expectedPriorAccessToken !== undefined && + !credentialBlobMatches(raw, expectedPriorAccessToken) +) { + log("writeback_skipped_stale", { source, configDir: dir }) + return false +} +``` + +In the `process.platform === "darwin"` branch, immediately after `if (!raw) return false`: + +```ts +if ( + expectedPriorAccessToken !== undefined && + !credentialBlobMatches(raw, expectedPriorAccessToken) +) { + log("writeback_skipped_stale", { source }) + return false +} +``` + +- [x] **Step 4: Run to verify they pass** + +Run: `node --test --experimental-strip-types --test-name-pattern="credentialBlobMatches|no longer the expected one" src/keychain.test.ts 2>&1 | tail -8` +Expected: `pass 4`, `fail 0` + +- [x] **Step 5: Pass the expected token from all three refresh call sites** + +`src/credentials.ts:376` — `creds` is the pre-refresh credential captured by `performRefresh`: + +```ts +writeBackCredentials( + target.source, + oauthCreds, + target.configDir, + creds.accessToken, +) +``` + +`src/credentials.ts:499` — inside `refreshBorrowedAccount`, `own` is this account's own stored credential: + +```ts +writeBackCredentials( + target.source, + oauthCreds, + target.configDir, + own.accessToken, +) +``` + +`src/credentials.ts:606-628` — in `forceRefreshActiveAccount`, capture the token **before** the await, because `account.credentials` is reassigned on success: + +```ts + const priorAccessToken = account.credentials.accessToken + const oauthCreds = await refresh(account.credentials.refreshToken) + if (oauthCreds && oauthCreds.expiresAt > Date.now() + 60_000) { + account.credentials = oauthCreds + if ( + !writeBackCredentials( + account.source, + oauthCreds, + account.configDir, + priorAccessToken, + ) + ) { +``` + +- [x] **Step 6: Add the write-back assertion test** + +Append inside the top-level `describe` in `src/credentials.test.ts`: + +```ts +it("performRefresh passes the pre-refresh token as the write-back guard", async () => { + const originalNow = Date.now + const now = 1_700_000_000_000 + Date.now = () => now + const originalFetch = globalThis.fetch + + try { + const { credentialsModule, keychainModule } = + await loadCredentialsWithCountingKeychain(now - 60_000) + + keychainModule.__setCredentials({ + accessToken: "stale-token", + refreshToken: "rt-stale", + expiresAt: now - 60_000, + }) + + credentialsModule.initAccounts([ + { + label: "Account 1", + source: "keychain", + credentials: { + accessToken: "stale-token", + refreshToken: "rt-stale", + expiresAt: now - 60_000, + }, + }, + ]) + + globalThis.fetch = (async () => + new Response( + JSON.stringify({ + access_token: "rotated-token", + refresh_token: "rt-rotated", + expires_in: 36_000, + }), + { status: 200 }, + )) as typeof fetch + + await credentialsModule.refreshIfNeeded() + + const writes = keychainModule.__getWrites() + assert.equal(writes.length, 1) + assert.equal(writes[0].creds.accessToken, "rotated-token") + assert.equal(writes[0].expectedPriorAccessToken, "stale-token") + } finally { + Date.now = originalNow + globalThis.fetch = originalFetch + } +}) +``` + +- [x] **Step 7: Run the full suite** + +Run: `pnpm test 2>&1 | tail -8` +Expected: `pass 259`, `fail 0` (later extended to 263 by review follow-ups) + +- [x] **Step 8: Commit** + +```bash +git add src/keychain.ts src/keychain.test.ts src/credentials.ts src/credentials.test.ts +git commit -m "fix: guard credential write-back against an external switch" +``` + +--- + +### Task 4: Bounded 401 recovery loop + +**Files:** + +- Modify: `src/index.ts:368-392` (401 block), `:462-464` (return), imports at `:20-31` +- Test: `src/index.test.ts:1189-1251` + +- [x] **Step 1: Import the currently-dead force-refresh helper** + +In the `from "./credentials.ts"` import block in `src/index.ts` (around line 20-31), add: + +```ts + forceRefreshActiveAccount, +``` + +- [x] **Step 2: Replace the 401 block with the recovery loop** + +Replace `src/index.ts:368-392` in full: + +```ts +// Recover from a rejected token: first by adopting credentials +// rotated externally (cswap switching accounts, the claude CLI, +// another OpenCode instance), then by forcing an OAuth refresh +// when the store still holds the token that was just rejected. +// +// Most cases resolve on the first attempt: a cold or unreadable +// store yields null from the reload (reloadCredentialsFromSource +// rejects anything expiring within 60s), so the force refresh runs +// immediately. The second attempt covers the narrower race where +// the reload returns a valid-looking token that a concurrent +// writer has itself just rotated again. The no-progress break is +// what actually bounds this — the cap is defence in depth. +const MAX_AUTH_RECOVERY_ATTEMPTS = 2 +let tokenInUse = latest.accessToken + +for ( + let attempt = 0; + response.status === 401 && attempt < MAX_AUTH_RECOVERY_ATTEMPTS; + attempt++ +) { + let candidate: ClaudeCredentials | null = null + try { + candidate = reloadCredentialsFromSource() + } catch {} + + if (!candidate || candidate.accessToken === tokenInUse) { + try { + candidate = await forceRefreshActiveAccount() + } catch {} + } + + if (!candidate || candidate.accessToken === tokenInUse) { + log("auth_recovery_exhausted", { + modelId, + attempt: attempt + 1, + }) + break + } + + tokenInUse = candidate.accessToken + log("auth_recovery_retry", { modelId, attempt: attempt + 1 }) + response = await fetchWithRetry(requestUrl, { + ...requestInit, + body, + headers: buildRequestHeaders( + input, + requestInit, + tokenInUse, + modelId, + excluded, + ), + }) +} +``` + +- [x] **Step 3: Derive the stream-transform decision from the final status** + +Replace `src/index.ts:462-464`: + +```ts +// A 401 that survived recovery carries an error body, not an SSE +// stream. Deciding here rather than from a flag set mid-flight +// makes the retried and non-retried paths behave identically. +return response.status === 401 ? response : transformResponseStream(response) +``` + +- [x] **Step 4: Run the suite to see the expected failure** + +Run: `pnpm test 2>&1 | grep -E "^not ok|✖" | head` +Expected: **two** failures. + +1. `auth fetch does not retry a 401 when the source token is unchanged` — its name describes the behavior being replaced. +2. `auth fetch preserves the original 401 when credential reload throws` — fails **only** on its request-count assertion. Its mock counts every `fetch` without discriminating the OAuth token endpoint, so the new force-refresh call inflates the count from 1 to 2. Every behavioral assertion (status, statusText, `content-type`, `x-request-id`, body) still passes, and the API is still hit exactly once. Fix by splitting the mock into `apiCalls` / `oauthCalls`, keeping all existing assertions and adding `oauthCalls === 1` — a thrown reload must still force a refresh. + +The neighbouring test that recovers a 401 via an externally rotated token must still pass — the loop preserves that path. If it fails, the loop is wrong, not the test. + +- [x] **Step 5: Replace that test with the two behaviors that supersede it** + +Replace the whole `it(...)` block at `src/index.test.ts:1189-1251` with: + +```ts +it("auth fetch force-refreshes on a 401 when the source token is unchanged", async () => { + const originalNow = Date.now + const originalSetInterval = globalThis.setInterval + const originalHome = process.env.HOME + const originalFetch = globalThis.fetch + const tempHome = await mkdtemp(join(tmpdir(), "opencode-claude-auth-home-")) + process.env.HOME = tempHome + Date.now = () => 1_700_000_000_000 + globalThis.setInterval = (() => ({ + unref() {}, + })) as unknown as typeof setInterval + + let apiCalls = 0 + let oauthCalls = 0 + + try { + const { helpersModule } = await loadHelpersWithCountingKeychain( + Date.now() + 10 * 60_000, + ) + + globalThis.fetch = (async (input: RequestInfo | URL) => { + const url = typeof input === "string" ? input : String(input) + if (url.includes("/oauth/token")) { + oauthCalls += 1 + return new Response( + JSON.stringify({ + access_token: "recovered-token", + refresh_token: "rt-recovered", + expires_in: 36_000, + }), + { status: 200 }, + ) + } + apiCalls += 1 + return apiCalls === 1 + ? new Response('{"error":"expired"}', { status: 401 }) + : new Response("data: {}\n\n", { status: 200 }) + }) as typeof fetch + + const plugin = await helpersModule.default({} as never) + const typedPlugin = plugin as { auth?: { loader?: TestAuthLoader } } + const authConfig = await typedPlugin.auth!.loader!( + async () => ({ + type: "oauth", + refresh: "refresh", + access: "access", + expires: Date.now() + 60_000, + }), + { models: {} }, + ) + + const response = await authConfig.fetch( + "https://api.anthropic.com/v1/messages", + { + method: "POST", + body: JSON.stringify({ model: "claude-haiku-4-5", messages: [] }), + }, + ) + + assert.equal(response.status, 200) + assert.equal(oauthCalls, 1, "the unchanged token must force a refresh") + assert.equal(apiCalls, 2, "the refreshed token must be retried once") + } finally { + Date.now = originalNow + globalThis.setInterval = originalSetInterval + globalThis.fetch = originalFetch + if (typeof originalHome === "string") { + process.env.HOME = originalHome + } else { + delete process.env.HOME + } + } +}) + +it("auth fetch surfaces the 401 unchanged when recovery cannot progress", async () => { + const originalNow = Date.now + const originalSetInterval = globalThis.setInterval + const originalHome = process.env.HOME + const originalFetch = globalThis.fetch + const tempHome = await mkdtemp(join(tmpdir(), "opencode-claude-auth-home-")) + process.env.HOME = tempHome + Date.now = () => 1_700_000_000_000 + globalThis.setInterval = (() => ({ + unref() {}, + })) as unknown as typeof setInterval + + let apiCalls = 0 + const errorBody = '{"name":"mcp_UnchangedToken"}' + + try { + const { helpersModule } = await loadHelpersWithCountingKeychain( + Date.now() + 10 * 60_000, + ) + + globalThis.fetch = (async (input: RequestInfo | URL) => { + const url = typeof input === "string" ? input : String(input) + if (url.includes("/oauth/token")) { + // A 429 from the token endpoint must read as a failed refresh, not + // as something that loops account recovery. retry-after beyond the + // 30s cap makes fetchWithRetry return instead of backing off. + return new Response('{"error":"rate_limited"}', { + status: 429, + headers: { "retry-after": "3600" }, + }) + } + apiCalls += 1 + return new Response(errorBody, { + status: 401, + headers: { "x-request-id": "unchanged-token-401" }, + }) + }) as typeof fetch + + const plugin = await helpersModule.default({} as never) + const typedPlugin = plugin as { auth?: { loader?: TestAuthLoader } } + const authConfig = await typedPlugin.auth!.loader!( + async () => ({ + type: "oauth", + refresh: "refresh", + access: "access", + expires: Date.now() + 60_000, + }), + { models: {} }, + ) + + const response = await authConfig.fetch( + "https://api.anthropic.com/v1/messages", + { + method: "POST", + body: JSON.stringify({ model: "claude-haiku-4-5", messages: [] }), + }, + ) + + assert.equal(response.status, 401) + assert.equal(response.headers.get("x-request-id"), "unchanged-token-401") + assert.equal(await response.text(), errorBody) + assert.equal(apiCalls, 1, "no retry without a different token") + } finally { + Date.now = originalNow + globalThis.setInterval = originalSetInterval + globalThis.fetch = originalFetch + if (typeof originalHome === "string") { + process.env.HOME = originalHome + } else { + delete process.env.HOME + } + } +}) +``` + +- [x] **Step 6: Add the test that justifies a second attempt** + +This is the only scenario where one attempt is insufficient: the reload returns a valid-looking token that a concurrent writer has itself just rotated again, so the first retry is also rejected. Append after the two tests from Step 5: + +```ts +it("auth fetch makes a second recovery attempt when the first retry is also rejected", async () => { + const originalNow = Date.now + const originalSetInterval = globalThis.setInterval + const originalHome = process.env.HOME + const originalFetch = globalThis.fetch + const tempHome = await mkdtemp(join(tmpdir(), "opencode-claude-auth-home-")) + process.env.HOME = tempHome + Date.now = () => 1_700_000_000_000 + globalThis.setInterval = (() => ({ + unref() {}, + })) as unknown as typeof setInterval + + let apiCalls = 0 + let oauthCalls = 0 + + try { + const { helpersModule, keychainModule } = + await loadHelpersWithCountingKeychain(Date.now() + 10 * 60_000) + + globalThis.fetch = (async (input: RequestInfo | URL) => { + const url = typeof input === "string" ? input : String(input) + if (url.includes("/oauth/token")) { + oauthCalls += 1 + return new Response( + JSON.stringify({ + access_token: "second-stage-token", + refresh_token: "rt-second-stage", + expires_in: 36_000, + }), + { status: 200 }, + ) + } + apiCalls += 1 + // Calls 1 and 2 are rejected; only the force-refreshed token works. + return apiCalls <= 2 + ? new Response('{"error":"expired"}', { status: 401 }) + : new Response("data: {}\n\n", { status: 200 }) + }) as typeof fetch + + const plugin = await helpersModule.default({} as never) + const typedPlugin = plugin as { auth?: { loader?: TestAuthLoader } } + const authConfig = await typedPlugin.auth!.loader!( + async () => ({ + type: "oauth", + refresh: "refresh", + access: "access", + expires: Date.now() + 60_000, + }), + { models: {} }, + ) + + // Attempt 1's reload finds a different, still-valid-looking token. + keychainModule.__setCredentials({ + accessToken: "concurrently-rotated", + refreshToken: "rt-concurrent", + expiresAt: Date.now() + 10 * 60_000, + }) + + const response = await authConfig.fetch( + "https://api.anthropic.com/v1/messages", + { + method: "POST", + body: JSON.stringify({ model: "claude-haiku-4-5", messages: [] }), + }, + ) + + assert.equal(response.status, 200) + assert.equal(apiCalls, 3, "original, reload retry, force-refresh retry") + assert.equal(oauthCalls, 1, "only the second attempt force-refreshes") + } finally { + Date.now = originalNow + globalThis.setInterval = originalSetInterval + globalThis.fetch = originalFetch + if (typeof originalHome === "string") { + process.env.HOME = originalHome + } else { + delete process.env.HOME + } + } +}) +``` + +- [x] **Step 7: Run the full suite** + +Run: `pnpm test 2>&1 | tail -8` +Expected: `pass 265`, `fail 0` (actual, measured — earlier tasks landed extra tests) + +- [x] **Step 8: Commit** + +```bash +git add src/index.ts src/index.test.ts +git commit -m "fix: recover from a rejected token by forcing an OAuth refresh" +``` + +--- + +### Task 5: Re-read the source on a 429 + +**Files:** + +- Modify: `src/index.ts` — insert after the 401 recovery loop, before the long-context beta loop (`:396`) +- Test: `src/index.test.ts` + +- [x] **Step 1: Write the failing test** + +Append inside the same `describe` as Task 4's tests: + +```ts +it("auth fetch retries a 429 once when the source token changed", async () => { + const originalNow = Date.now + const originalSetInterval = globalThis.setInterval + const originalHome = process.env.HOME + const originalFetch = globalThis.fetch + const tempHome = await mkdtemp(join(tmpdir(), "opencode-claude-auth-home-")) + process.env.HOME = tempHome + Date.now = () => 1_700_000_000_000 + globalThis.setInterval = (() => ({ + unref() {}, + })) as unknown as typeof setInterval + + let apiCalls = 0 + + try { + const { helpersModule, keychainModule } = + await loadHelpersWithCountingKeychain(Date.now() + 10 * 60_000) + + globalThis.fetch = (async () => { + apiCalls += 1 + if (apiCalls === 1) { + // retry-after beyond the 30s cap: quota exhaustion, not a + // transient limit, so fetchWithRetry returns immediately. + return new Response('{"error":"rate_limited"}', { + status: 429, + headers: { "retry-after": "3600" }, + }) + } + return new Response("data: {}\n\n", { status: 200 }) + }) as typeof fetch + + const plugin = await helpersModule.default({} as never) + const typedPlugin = plugin as { auth?: { loader?: TestAuthLoader } } + const authConfig = await typedPlugin.auth!.loader!( + async () => ({ + type: "oauth", + refresh: "refresh", + access: "access", + expires: Date.now() + 60_000, + }), + { models: {} }, + ) + + // An external switch lands while this session is still on the + // exhausted account. + keychainModule.__setCredentials({ + accessToken: "switched-token", + refreshToken: "rt-switched", + expiresAt: Date.now() + 10 * 60_000, + }) + + const response = await authConfig.fetch( + "https://api.anthropic.com/v1/messages", + { + method: "POST", + body: JSON.stringify({ model: "claude-haiku-4-5", messages: [] }), + }, + ) + + assert.equal(response.status, 200) + assert.equal(apiCalls, 2, "the rotated token must be retried once") + } finally { + Date.now = originalNow + globalThis.setInterval = originalSetInterval + globalThis.fetch = originalFetch + if (typeof originalHome === "string") { + process.env.HOME = originalHome + } else { + delete process.env.HOME + } + } +}) + +it("auth fetch does not retry a 429 when the source token is unchanged", async () => { + const originalNow = Date.now + const originalSetInterval = globalThis.setInterval + const originalHome = process.env.HOME + const originalFetch = globalThis.fetch + const tempHome = await mkdtemp(join(tmpdir(), "opencode-claude-auth-home-")) + process.env.HOME = tempHome + Date.now = () => 1_700_000_000_000 + globalThis.setInterval = (() => ({ + unref() {}, + })) as unknown as typeof setInterval + + let apiCalls = 0 + + try { + const { helpersModule } = await loadHelpersWithCountingKeychain( + Date.now() + 10 * 60_000, + ) + + globalThis.fetch = (async () => { + apiCalls += 1 + return new Response('{"error":"rate_limited"}', { + status: 429, + headers: { "retry-after": "3600" }, + }) + }) as typeof fetch + + const plugin = await helpersModule.default({} as never) + const typedPlugin = plugin as { auth?: { loader?: TestAuthLoader } } + const authConfig = await typedPlugin.auth!.loader!( + async () => ({ + type: "oauth", + refresh: "refresh", + access: "access", + expires: Date.now() + 60_000, + }), + { models: {} }, + ) + + const response = await authConfig.fetch( + "https://api.anthropic.com/v1/messages", + { + method: "POST", + body: JSON.stringify({ model: "claude-haiku-4-5", messages: [] }), + }, + ) + + assert.equal(response.status, 429) + assert.equal(apiCalls, 1, "an unchanged token must not be retried") + } finally { + Date.now = originalNow + globalThis.setInterval = originalSetInterval + globalThis.fetch = originalFetch + if (typeof originalHome === "string") { + process.env.HOME = originalHome + } else { + delete process.env.HOME + } + } +}) +``` + +`loadHelpersWithCountingKeychain` already returns `keychainModule` with `__setCredentials` (`src/index.test.ts:264-273`), so no harness change is needed. + +- [x] **Step 2: Run to verify the first fails and the second passes** + +Run: `node --test --experimental-strip-types --test-name-pattern="429" src/index.test.ts 2>&1 | tail -12` +Expected: the "token changed" test FAILS with `apiCalls` 1 ≠ 2; the "unchanged" test passes (no retry exists yet). + +- [x] **Step 3: Implement the 429 re-read** + +Insert in `src/index.ts` directly after the 401 recovery loop from Task 4 and before the long-context beta loop: + +```ts +// An external switch — cswap rotating off an exhausted account — +// leaves this session on the old token until the 30s credential +// cache expires. Re-read once so a rate limit that has already +// been resolved elsewhere is not surfaced. A changed token is the +// signal that a switch happened; when nothing changed this costs +// one source read and no retry. +if (response.status === 429) { + let rotated: ClaudeCredentials | null = null + try { + rotated = reloadCredentialsFromSource() + } catch {} + + if (rotated && rotated.accessToken !== tokenInUse) { + log("rate_limit_credentials_rotated", { modelId }) + tokenInUse = rotated.accessToken + response = await fetchWithRetry(requestUrl, { + ...requestInit, + body, + headers: buildRequestHeaders( + input, + requestInit, + tokenInUse, + modelId, + excluded, + ), + }) + } +} +``` + +- [x] **Step 4: Run to verify both pass** + +Run: `node --test --experimental-strip-types --test-name-pattern="429" src/index.test.ts 2>&1 | tail -8` +Expected: `fail 0` + +- [x] **Step 5: Run the full suite** + +Run: `pnpm test 2>&1 | tail -8` +Expected: `pass 260`, `fail 0` + +- [x] **Step 6: Commit** + +```bash +git add src/index.ts src/index.test.ts +git commit -m "fix: re-read credentials on a 429 so a resolved rate limit is not surfaced" +``` + +--- + +### Task 6: Lint, format, and document + +**Files:** + +- Modify: `README.md`, `CHANGELOG.md` + +- [x] **Step 1: Lint and format** + +Run: `pnpm run lint:fix && pnpm run lint` +Expected: clean exit, no diagnostics. + +- [x] **Step 2: Add the behavior to the README technical list** + +In `README.md`, under `## How it works (technical)`, insert after the bullet beginning "Syncs credentials to `auth.json`": + +```markdown +- Re-reads the credential source on each cache miss (~every 30s under load), so an account switched externally — by [claude-swap](https://github.com/realiti4/claude-swap), the `claude` CLI in another terminal, or a second OpenCode instance — is picked up in a live session without a restart +- Guards credential write-back with the token it refreshed from, so a switch landing mid-refresh can never write one account's tokens into another's slot +- On a rejected token, adopts an externally rotated credential and otherwise forces an OAuth refresh, retrying the request in place rather than surfacing the 401 +``` + +- [x] **Step 3: Add a CHANGELOG entry** + +Add under the topmost unreleased heading in `CHANGELOG.md` (create `## Unreleased` above the newest release heading if absent): + +```markdown +### Bug Fixes + +- Pick up credentials rotated by another process (claude-swap, the `claude` CLI, a second OpenCode instance) in a live session instead of staying pinned to the account that was active at startup +- Never write a refreshed token into a slot that was switched underneath us +- Force an OAuth refresh when a request is rejected and the store still holds the rejected token, so the request recovers in place +- Write refreshed credentials back to the account's own config directory on the force-refresh path, which previously fell back to the default directory and could write to the wrong file for a file-source account with a custom `CLAUDE_CONFIG_DIR` + +### Behavior Changes + +- A 401 that survives recovery is now returned without SSE stream transformation, matching what a non-retried 401 already did +``` + +- [x] **Step 4: Verify the full suite and a clean tree** + +Run: `pnpm test 2>&1 | tail -8 && git status --short` +Expected: `fail 0`; only `README.md` and `CHANGELOG.md` modified. + +- [x] **Step 5: Commit** + +```bash +git add README.md CHANGELOG.md +git commit -m "docs: document external credential rotation handling" +``` + +--- + +## Follow-ups (deliberately out of scope) + +**The post-OAuth file-source exclusion in `performRefresh` is now unjustified.** `src/credentials.ts` re-reads the source after a failed OAuth refresh, to catch credentials a sibling instance wrote during the round trip — but skips that read for `file` sources. That guard was coherent while the up-front re-read was file-only; Task 2 removed that rationale and left nothing in its place. A sibling can write a file source mid-round-trip exactly as it can a keychain entry, a file read is cheaper than shelling out to `security`, and the block is already try/catch-wrapped, so including file sources would add no throw path. + +Consequence today: a file-source account whose OAuth refresh fails goes straight to `refreshViaCli` without checking whether a sibling already wrote usable credentials — the very race the block exists to handle. + +Not fixed here because it changes behavior on the file path, which no current test covers, and it is outside the guard replacement this feature specified. The comment at the guard says so plainly rather than implying the asymmetry is principled. + +**`performRefresh` discards `writeBackCredentials`'s return value.** A failed write-back is therefore invisible: memory holds freshly refreshed credentials while the store keeps the pre-refresh blob. On the proactive path that orphaned blob can still have ~1h left, so the validated re-read reads it as usable and adopts it, clobbering the fresh credentials and wasting background OAuth attempts until it drops under 60s. + +This cannot be fixed at the re-read — it cannot distinguish "the store is stale because our write failed" from "the store changed because cswap switched"; both look like _store disagrees with memory, store is usable_. The information exists only at the write-back call site. `forceRefreshActiveAccount` already models the fix, checking the return and logging `force_refresh_writeback_failed`; `performRefresh` does neither. Pre-existing, not introduced by this feature. + +Partially closed in review: `performRefresh` now checks the return and logs `refresh_writeback_failed`, so the failure is no longer invisible. The remaining half — not re-adopting an orphaned-but-still-usable blob on the proactive path — is a control-flow change that interacts with the adoption logic this feature introduced, and stays deferred. + +**`transformResponseStream` is applied to non-401 error responses.** Task 4 scopes the bypass to 401 only, matching the prior behavior for that status. But 4xx and 5xx responses other than 401 still flow through the SSE transform, which strips tool-name prefixes from the body. The transform's `!ok` branch already passes status and headers through intact, so the practical difference for a 401 is only that prefix stripping — the carve-out is narrower than "the transform mangles errors". Still asymmetric: the comment's rationale ("carries an error body, not an SSE stream") applies equally to 400/429/500, which do go through it. Pre-existing and unchanged by this feature. Worth deciding whether the bypass should cover all `!ok` statuses rather than 401 alone. + +**~~`configDir` is honored on one re-read path but not the others.~~ Closed in review.** `refreshIfNeeded` passed `target.configDir` to `refreshAccount`; `reloadCredentialsFromSource` and `reloadActiveAccount` did not. Never reachable — every file account is assigned `CLAUDE_CONFIG_DIR ?? ~/.claude`, exactly what `readCredentialsFile` recomputes by default — but the compare-and-swap guard makes the read and the write agreeing on one file load-bearing, so both paths now forward it by construction rather than by coincidence. Pinned by a test per path. + +**`reloadActiveAccount` and `invalidateCredentialCache` are still dead.** Both docstrings claimed to be "used on 401"; `reloadActiveAccount`'s now records that it has no call sites, `invalidateCredentialCache`'s is still wrong. The 401 path uses `reloadCredentialsFromSource` instead. This feature revived the third of the three dead functions the design doc identified and left these two behind. Either wire them up or delete them. + +**No negative caching of a terminal auth failure.** On a server-side-revoked credential the store keeps returning a locally-unexpired token, so every request pays the full recovery loop — three API calls plus an OAuth exchange plus `security` spawns — indefinitely, where before it paid one request. Worst-case cost per `fetch()` is now roughly 18 Anthropic requests and several minutes of capped backoff, bounded by the caller's abort signal but with no deadline of the plugin's own. + +**The log redactor protects structured fields only.** `logger.ts` redacts by key name or an anchored `^eyJ` whole-value match, so a token embedded mid-string in an `err.message` would pass through. Unreachable today — every callee either catches internally or throws fixed strings — but two new sites log `err.message`. + +**Narrow in-memory orphaned-rotation race.** If the proactive timer's refresh is in flight while a request-path re-read adopts an externally written blob, the timer's OAuth result overwrites `target.credentials` while its write-back guard correctly refuses. Both tokens are usable so nothing breaks, but a rotation is consumed and stored nowhere. Requires an external write inside a 15s OAuth round trip plus two concurrent callers. The store-side variant is covered above; this in-memory one is distinct. + +## Manual verification + +Automated tests stub the Keychain. Verify against real cswap state once: + +- [ ] Confirm at least two cswap accounts and note the active one: `cswap list` +- [ ] Start OpenCode and send a prompt so credentials are cached +- [ ] Switch in another terminal: `cswap switch` +- [ ] Wait 30s, send another prompt in the **same** OpenCode session +- [ ] With `CLAUDE_AUTH_DEBUG=1`, confirm `~/.local/share/opencode/claude-auth-debug.log` shows the new account's token and no `writeback_skipped_stale` storm +- [ ] Confirm `cswap list` still shows both accounts healthy — no slot quarantined for a dead refresh token diff --git a/src/credentials.test.ts b/src/credentials.test.ts index d8f318e..5b7186e 100644 --- a/src/credentials.test.ts +++ b/src/credentials.test.ts @@ -57,15 +57,26 @@ async function loadCredentialsWithCountingKeychain( __getReadCount: () => number __getWriteCount: () => number __setCredentials: (c: Creds | null) => void + __setCredentialsForSource: (source: string, c: Creds | null) => void __setAccounts: (list: unknown[]) => void __setReadError: (enabled: boolean) => void __setReadHook: (hook: (() => void) | null) => void - __getWrites: () => Array<{ source: string; creds: Creds }> + __getWrites: () => Array<{ + source: string + creds: Creds + configDir?: string + expectedPriorAccessToken?: string + }> + __getReads: () => Array<{ source: string; configDir?: string }> + __setWriteResult: (v: boolean) => void } childProcessModule: { __getExecFileSyncCount: () => number __getExecSyncCount: () => number } + loggerModule: { + __getLogs: () => Array<{ event: string; data?: unknown }> + } }> { const tempDir = await mkdtemp(join(tmpdir(), "opencode-claude-auth-creds-")) const tempKeychain = join(tempDir, "keychain.ts") @@ -93,7 +104,16 @@ async function loadCredentialsWithCountingKeychain( await writeFile( tempLogger, - `export function log() {}\nexport function initLogger() {}\nexport function closeLogger() {}\n`, + `const logs = [] +export function log(event, data) { + logs.push({ event, data }) +} +export function __getLogs() { + return logs +} +export function initLogger() {} +export function closeLogger() {} +`, "utf8", ) @@ -128,6 +148,8 @@ export function __getExecSyncCount() { `let readCount = 0 let writeCount = 0 const writes = [] +const reads = [] +let writeResult = true let accounts = null // null = derive a single account from the credentials var let readError = false let readHook = null @@ -136,6 +158,7 @@ let credentials = { refreshToken: "refresh", expiresAt: ${initialExpiresAt} } +const bySource = {} export const PRIMARY_SERVICE = "Claude Code-credentials" @@ -145,10 +168,14 @@ export function readAllClaudeAccounts() { return [{ label: "Account 1", source: "keychain", credentials }] } -export function refreshAccount(source) { +export function refreshAccount(source, configDir) { readCount += 1 + reads.push({ source, configDir }) if (readError) throw new Error("Keychain read denied") if (readHook) readHook() + if (Object.prototype.hasOwnProperty.call(bySource, source)) { + return bySource[source] + } return credentials } @@ -160,16 +187,24 @@ export function __setReadHook(hook) { readHook = hook } -export function writeBackCredentials(source, creds) { +export function writeBackCredentials(source, creds, configDir, expectedPriorAccessToken) { writeCount += 1 - writes.push({ source, creds }) - return true + writes.push({ source, creds, configDir, expectedPriorAccessToken }) + return writeResult +} + +export function __setWriteResult(v) { + writeResult = v } export function __getWrites() { return writes } +export function __getReads() { + return reads +} + export function __getReadCount() { return readCount } @@ -182,6 +217,10 @@ export function __setCredentials(c) { credentials = c } +export function __setCredentialsForSource(source, c) { + bySource[source] = c +} + export function __setAccounts(list) { accounts = list } @@ -202,6 +241,7 @@ export function __setAccounts(list) { import(pathToFileURL(tempKeychain).href), import(pathToFileURL(tempChildProcess).href), ]) + const loggerModule = await import(pathToFileURL(tempLogger).href) return { credentialsModule: credentialsModule as { @@ -228,15 +268,26 @@ export function __setAccounts(list) { __getReadCount: () => number __getWriteCount: () => number __setCredentials: (c: Creds | null) => void + __setCredentialsForSource: (source: string, c: Creds | null) => void __setAccounts: (list: unknown[]) => void __setReadError: (enabled: boolean) => void __setReadHook: (hook: (() => void) | null) => void - __getWrites: () => Array<{ source: string; creds: Creds }> + __getWrites: () => Array<{ + source: string + creds: Creds + configDir?: string + expectedPriorAccessToken?: string + }> + __getReads: () => Array<{ source: string; configDir?: string }> + __setWriteResult: (v: boolean) => void }, childProcessModule: childProcessModule as { __getExecFileSyncCount: () => number __getExecSyncCount: () => number }, + loggerModule: loggerModule as { + __getLogs: () => Array<{ event: string; data?: unknown }> + }, } } @@ -262,12 +313,20 @@ describe("credential caching", () => { }, ]) + // The source agrees with memory to begin with, so priming the cache + // (which re-reads the source) leaves the account on "old-token". + keychainModule.__setCredentialsForSource("keychain", { + accessToken: "old-token", + refreshToken: "old-refresh", + expiresAt: now + 10 * 60_000, + }) + assert.equal( (await credentialsModule.getCachedCredentials())?.accessToken, "old-token", ) - keychainModule.__setCredentials({ + keychainModule.__setCredentialsForSource("keychain", { accessToken: "new-token", refreshToken: "new-refresh", expiresAt: now + 8 * 60 * 60_000, @@ -277,7 +336,11 @@ describe("credential caching", () => { const readCountAfterReload = keychainModule.__getReadCount() assert.equal(reloaded?.accessToken, "new-token") - assert.equal(readCountAfterReload, 1) + assert.equal( + readCountAfterReload, + 2, + "one re-read while priming the cache, one for the explicit reload", + ) assert.equal( (await credentialsModule.getCachedCredentials())?.accessToken, "new-token", @@ -312,7 +375,7 @@ describe("credential caching", () => { keychainModule.__setReadError(true) assert.equal(credentialsModule.reloadCredentialsFromSource(), null) - assert.equal(keychainModule.__getReadCount(), 1) + assert.equal(keychainModule.__getReadCount(), 2) } finally { Date.now = originalNow } @@ -434,7 +497,11 @@ describe("credential caching", () => { assert.ok(first) assert.ok(second) - assert.equal(keychainModule.__getReadCount(), 0) + assert.equal( + keychainModule.__getReadCount(), + 1, + "cache miss re-reads the source once; the cached call does not", + ) } finally { Date.now = originalNow } @@ -724,17 +791,27 @@ describe("credential caching", () => { }, ]) + // Pin the expired account's own stored value so the up-front re-read is + // a no-op rather than swapping in the other account's blob. + keychainModule.__setCredentialsForSource( + "Claude Code-credentials-aabbccdd", + { + accessToken: "stale-suffixed", + refreshToken: "rt-suffixed", + expiresAt: now - 1_000, + }, + ) + const readsBefore = keychainModule.__getReadCount() const result = await credentialsModule.refreshIfNeeded() assert.equal(result?.accessToken, "fresh-in-memory") - // One read only: the target re-reading its OWN source to see whether - // another process rotated it. The fallback account is still borrowed - // straight from memory rather than re-read. + // The fallback account is still borrowed straight from memory rather + // than re-read. assert.equal( keychainModule.__getReadCount(), - readsBefore + 1, - "valid in-memory fallback credentials must not trigger a keychain read", + readsBefore + 2, + "one up-front re-read of the target's own source, one after the failed OAuth refresh", ) } finally { Date.now = originalNow @@ -746,8 +823,8 @@ describe("credential caching", () => { const { credentialsModule, keychainModule } = await loadCredentialsWithCountingKeychain(now + 10 * 60_000) - // Keychain source: refreshIfNeeded never re-reads these while the local - // copy looks valid, so a 401 needs an explicit source reload. + // A 401 must reload the source immediately rather than waiting for the + // next refreshIfNeeded, which the 30s credential cache can defer. const account = { label: "Account 1", source: "keychain", @@ -770,6 +847,73 @@ describe("credential caching", () => { assert.equal(account.credentials.accessToken, "rotated") }) + // Every other refreshAccount call site in credentials.ts forwards the + // account's configDir; these two re-read paths did not. Unreachable while + // readAllClaudeAccounts assigns every file account + // `CLAUDE_CONFIG_DIR ?? ~/.claude` — exactly what readCredentialsFile + // recomputes by default — but the CAS guard added here compares a read + // against a write, so the two must resolve the same file by construction + // rather than by coincidence. + it("reloadCredentialsFromSource reads the account's own configDir", async () => { + const now = Date.now() + const { credentialsModule, keychainModule } = + await loadCredentialsWithCountingKeychain(now + 10 * 60_000) + + credentialsModule.initAccounts([ + { + label: "Account 1", + source: "file", + configDir: "/custom/config/dir", + credentials: { + accessToken: "token", + refreshToken: "refresh", + expiresAt: now + 10 * 60_000, + }, + }, + ]) + + const readsBefore = keychainModule.__getReads().length + credentialsModule.reloadCredentialsFromSource() + + const reads = keychainModule.__getReads().slice(readsBefore) + assert.ok(reads.length > 0, "expected a source read") + assert.equal( + reads[reads.length - 1].configDir, + "/custom/config/dir", + "the re-read must target the account's own config directory", + ) + }) + + it("reloadActiveAccount reads the account's own configDir", async () => { + const now = Date.now() + const { credentialsModule, keychainModule } = + await loadCredentialsWithCountingKeychain(now + 10 * 60_000) + + credentialsModule.initAccounts([ + { + label: "Account 1", + source: "file", + configDir: "/custom/config/dir", + credentials: { + accessToken: "token", + refreshToken: "refresh", + expiresAt: now + 10 * 60_000, + }, + }, + ]) + + const readsBefore = keychainModule.__getReads().length + credentialsModule.reloadActiveAccount() + + const reads = keychainModule.__getReads().slice(readsBefore) + assert.ok(reads.length > 0, "expected a source read") + assert.equal( + reads[reads.length - 1].configDir, + "/custom/config/dir", + "the re-read must target the account's own config directory", + ) + }) + it("forceRefreshActiveAccount swaps in OAuth-refreshed credentials and writes back", async () => { const now = Date.now() const { credentialsModule, keychainModule } = @@ -818,6 +962,45 @@ describe("credential caching", () => { ) }) + // The token must be captured before the await: account.credentials is + // reassigned to the refreshed pair before write-back runs, so reading it at + // the call site would compare the new token against the store and skip + // every write. + it("forceRefreshActiveAccount guards write-back with the pre-refresh token", async () => { + const now = Date.now() + const { credentialsModule, keychainModule } = + await loadCredentialsWithCountingKeychain(now + 10 * 60_000) + + const account = { + label: "Account 1", + source: "keychain", + credentials: { + accessToken: "rejected-token", + refreshToken: "refresh-token", + expiresAt: now + 10 * 60_000, + }, + } + credentialsModule.initAccounts([account]) + + const writesBefore = keychainModule.__getWrites().length + + await credentialsModule.forceRefreshActiveAccount(async () => ({ + accessToken: "oauth-refreshed", + refreshToken: "new-refresh", + expiresAt: now + 10 * 60_000, + })) + + const writes = keychainModule.__getWrites() + assert.equal(writes.length, writesBefore + 1) + const write = writes[writes.length - 1] + assert.equal(write.creds.accessToken, "oauth-refreshed") + assert.equal( + write.expectedPriorAccessToken, + "rejected-token", + "the guard must carry the token the refresh was issued against", + ) + }) + it("forceRefreshActiveAccount returns null and leaves the account untouched on failure", async () => { const now = Date.now() const { credentialsModule } = await loadCredentialsWithCountingKeychain( @@ -935,6 +1118,283 @@ describe("credential caching", () => { Date.now = originalNow } }) + + it("refreshIfNeeded adopts credentials rotated externally in a keychain source", async () => { + const originalNow = Date.now + const now = 1_700_000_000_000 + Date.now = () => now + + try { + const { credentialsModule, keychainModule } = + await loadCredentialsWithCountingKeychain(now + 10 * 60_000) + + credentialsModule.initAccounts([ + { + label: "Account 1", + source: "keychain", + credentials: { + accessToken: "before-switch", + refreshToken: "rt-before", + expiresAt: now + 10 * 60_000, + }, + }, + ]) + + // An external process (cswap, the claude CLI, a second OpenCode) + // replaces the stored credential with a different account's. + keychainModule.__setCredentials({ + accessToken: "after-switch", + refreshToken: "rt-after", + expiresAt: now + 10 * 60_000, + }) + + const result = await credentialsModule.refreshIfNeeded() + + assert.equal(result?.accessToken, "after-switch") + } finally { + Date.now = originalNow + } + }) + + // writeBackCredentials can fail while the read that precedes it succeeds: + // a malformed stored blob makes updateCredentialBlob return null, and an + // ACL can permit reads but not add-generic-password. performRefresh + // discards that false, so memory ends up holding freshly refreshed + // credentials while the store still holds the pre-refresh blob — which, + // because we only refresh inside the 60s window, has under 60s left. + // Adopting it would re-enter performRefresh with a refresh token our own + // successful refresh just rotated dead, failing over to two 60s claude + // spawns on every cache miss, forever. + it("refreshIfNeeded does not adopt a stale stored blob over valid in-memory credentials", async () => { + const originalNow = Date.now + const now = 1_700_000_000_000 + Date.now = () => now + + try { + const { credentialsModule, keychainModule, childProcessModule } = + await loadCredentialsWithCountingKeychain(now + 10 * 60_000) + + credentialsModule.initAccounts([ + { + label: "Account 1", + source: "keychain", + credentials: { + accessToken: "fresh-in-memory", + refreshToken: "rt-fresh", + expiresAt: now + 10 * 60_000, + }, + }, + ]) + + // The store still holds the pre-refresh blob the failed write-back + // never replaced. + keychainModule.__setCredentials({ + accessToken: "stale-in-store", + refreshToken: "rt-stale", + expiresAt: now + 30_000, + }) + + const result = await credentialsModule.refreshIfNeeded() + + assert.equal(result?.accessToken, "fresh-in-memory") + assert.equal( + childProcessModule.__getExecSyncCount(), + 0, + "adopting the stale blob would fail OAuth and fall through to the CLI", + ) + } finally { + Date.now = originalNow + } + }) + + // Complement of the test above: the guard must decline only the unusable + // blob. If it declined whenever memory was valid, external rotation would + // stop being picked up and this task's whole premise would regress. + it("refreshIfNeeded adopts a usable stored blob even when memory is still valid", async () => { + const originalNow = Date.now + const now = 1_700_000_000_000 + Date.now = () => now + + try { + const { credentialsModule, keychainModule } = + await loadCredentialsWithCountingKeychain(now + 10 * 60_000) + + const account = { + label: "Account 1", + source: "keychain", + credentials: { + accessToken: "in-memory-valid", + refreshToken: "rt-memory", + expiresAt: now + 10 * 60_000, + }, + } + credentialsModule.initAccounts([account]) + + keychainModule.__setCredentials({ + accessToken: "rotated-in-store", + refreshToken: "rt-rotated", + expiresAt: now + 8 * 60 * 60_000, + }) + + const result = await credentialsModule.refreshIfNeeded() + + assert.equal(result?.accessToken, "rotated-in-store") + assert.equal( + account.credentials.accessToken, + "rotated-in-store", + "the adoption must land on the account so later calls see it", + ) + } finally { + Date.now = originalNow + } + }) + + it("refreshIfNeeded keeps in-memory credentials when the source read throws", async () => { + const originalNow = Date.now + const now = 1_700_000_000_000 + Date.now = () => now + + try { + const { credentialsModule, keychainModule } = + await loadCredentialsWithCountingKeychain(now + 10 * 60_000) + + credentialsModule.initAccounts([ + { + label: "Account 1", + source: "keychain", + credentials: { + accessToken: "in-memory", + refreshToken: "rt", + expiresAt: now + 10 * 60_000, + }, + }, + ]) + + keychainModule.__setReadError(true) + + const result = await credentialsModule.refreshIfNeeded() + + assert.equal(result?.accessToken, "in-memory") + } finally { + Date.now = originalNow + } + }) + + it("performRefresh passes the pre-refresh token as the write-back guard", async () => { + const originalNow = Date.now + const now = 1_700_000_000_000 + Date.now = () => now + const originalFetch = globalThis.fetch + + try { + const { credentialsModule, keychainModule } = + await loadCredentialsWithCountingKeychain(now - 60_000) + + keychainModule.__setCredentials({ + accessToken: "stale-token", + refreshToken: "rt-stale", + expiresAt: now - 60_000, + }) + + credentialsModule.initAccounts([ + { + label: "Account 1", + source: "keychain", + credentials: { + accessToken: "stale-token", + refreshToken: "rt-stale", + expiresAt: now - 60_000, + }, + }, + ]) + + globalThis.fetch = (async () => + new Response( + JSON.stringify({ + access_token: "rotated-token", + refresh_token: "rt-rotated", + expires_in: 36_000, + }), + { status: 200 }, + )) as typeof fetch + + await credentialsModule.refreshIfNeeded() + + const writes = keychainModule.__getWrites() + assert.equal(writes.length, 1) + assert.equal(writes[0].creds.accessToken, "rotated-token") + assert.equal(writes[0].expectedPriorAccessToken, "stale-token") + } finally { + Date.now = originalNow + globalThis.fetch = originalFetch + } + }) + + // forceRefreshActiveAccount logs force_refresh_writeback_failed on the same + // condition; performRefresh discarded the return value entirely, so a + // rejected write-back left no trace anywhere in the debug log. The session + // continues from memory either way — this pins the observability, not a + // control-flow change. + it("performRefresh logs a rejected write-back", async () => { + const originalNow = Date.now + const now = 1_700_000_000_000 + Date.now = () => now + const originalFetch = globalThis.fetch + + try { + const { credentialsModule, keychainModule, loggerModule } = + await loadCredentialsWithCountingKeychain(now - 60_000) + + keychainModule.__setCredentials({ + accessToken: "stale-token", + refreshToken: "rt-stale", + expiresAt: now - 60_000, + }) + + credentialsModule.initAccounts([ + { + label: "Account 1", + source: "keychain", + credentials: { + accessToken: "stale-token", + refreshToken: "rt-stale", + expiresAt: now - 60_000, + }, + }, + ]) + + // An external switch landed inside the OAuth round trip, so the CAS in + // writeBackCredentials rejects the write. + keychainModule.__setWriteResult(false) + + globalThis.fetch = (async () => + new Response( + JSON.stringify({ + access_token: "rotated-token", + refresh_token: "rt-rotated", + expires_in: 36_000, + }), + { status: 200 }, + )) as typeof fetch + + const result = await credentialsModule.refreshIfNeeded() + + assert.equal( + result?.accessToken, + "rotated-token", + "the caller still receives the refreshed credentials", + ) + assert.ok( + loggerModule + .__getLogs() + .some((entry) => entry.event === "refresh_writeback_failed"), + "a rejected write-back must be visible in the debug log", + ) + } finally { + Date.now = originalNow + globalThis.fetch = originalFetch + } + }) }) describe("syncAuthJson file permissions", () => { @@ -1317,11 +1777,19 @@ describe("refreshIfNeeded CLI fallback scope", () => { }) as typeof fetch try { - const { credentialsModule, childProcessModule } = + const { credentialsModule, keychainModule, childProcessModule } = await loadCredentialsWithCountingKeychain(now + 30 * 60_000) const target = makeAccount(now + 30 * 60_000) credentialsModule.initAccounts([target]) + // Pin the target's own stored value so the up-front re-read is a no-op + // and the assertion below speaks to the CLI, not to source adoption. + keychainModule.__setCredentialsForSource("keychain", { + accessToken: "existing-token", + refreshToken: "existing-refresh", + expiresAt: now + 30 * 60_000, + }) + const result = await credentialsModule.refreshIfNeeded( target, 60 * 60_000, @@ -1497,8 +1965,11 @@ describe("refreshIfNeeded CLI fallback scope", () => { const target = makeAccount(now + 30_000) credentialsModule.initAccounts([target]) - // The store initially matches memory, so the pre-CLI re-read finds - // nothing new. Only once the CLI has run does the entry rotate. + // The store initially matches memory, so neither the up-front re-read + // nor the pre-CLI re-read finds anything new. Only once the CLI has run + // does the entry rotate — hence the third read, not the second: + // 1) refreshIfNeeded's up-front re-read, 2) the re-read after OAuth + // fails, 3) the read after the CLI has rotated the entry. keychainModule.__setCredentials({ accessToken: "existing-token", refreshToken: "existing-refresh", @@ -1507,7 +1978,7 @@ describe("refreshIfNeeded CLI fallback scope", () => { let reads = 0 keychainModule.__setReadHook(() => { reads += 1 - if (reads >= 2) { + if (reads >= 3) { keychainModule.__setCredentials({ accessToken: "cli-rotated-token", refreshToken: "cli-rotated-refresh", @@ -1629,6 +2100,98 @@ describe("borrowed fallback credentials", () => { Date.now = originalNow } }) + + // While borrowed, target.credentials holds the LENDER's token, so guarding + // write-back with it would fail the comparison every time and silently stop + // persisting refreshes for exactly the accounts that are recovering. The + // guard must use this account's own stored token. + it("guards a recovering borrower's write-back with its own token, not the lender's", async () => { + const originalFetch = globalThis.fetch + const originalNow = Date.now + const now = 1_700_000_000_000 + Date.now = () => now + + let oauthFails = true + globalThis.fetch = (async () => { + if (oauthFails) throw new Error("network unreachable") + return new Response( + JSON.stringify({ + access_token: "at-recovered", + refresh_token: "rt-recovered", + expires_in: 28_800, + }), + { status: 200 }, + ) + }) as typeof fetch + + try { + const { credentialsModule, keychainModule } = + await loadCredentialsWithCountingKeychain(now - 1_000) + + const borrower = { + label: "Account 1", + source: "Claude Code-credentials-aabbccdd", + credentials: { + accessToken: "at-borrower", + refreshToken: "rt-borrower", + expiresAt: now - 1_000, + }, + } + const lender = { + label: "Account 2", + source: "Claude Code-credentials", + credentials: { + accessToken: "at-lender", + refreshToken: "rt-lender", + expiresAt: now + 30 * 60_000, + }, + } + credentialsModule.initAccounts([borrower, lender]) + + // The borrower's own store holds an expired credential distinct from + // anything it has ever had in memory, so the assertion below can only + // pass if the guard came from the store rather than from memory. + keychainModule.__setCredentialsForSource(borrower.source, { + accessToken: "at-borrower-own", + refreshToken: "rt-borrower-own", + expiresAt: now - 1_000, + }) + + // OAuth and the CLI both fail (suffixed source, no configDir), so the + // borrower falls back to the lender's credentials. + assert.equal( + (await credentialsModule.refreshIfNeeded(borrower))?.accessToken, + "at-lender", + "borrow should succeed", + ) + + const writesBefore = keychainModule.__getWrites().length + oauthFails = false + + // A threshold wider than the borrowed credential's remaining life + // forces a refresh while leaving it usable, so the up-front re-read + // does not adopt the expired stored blob and clear the borrowed flag. + await credentialsModule.refreshIfNeeded(borrower, 60 * 60_000) + + const writes = keychainModule.__getWrites() + assert.equal(writes.length, writesBefore + 1) + const write = writes[writes.length - 1] + assert.equal(write.creds.accessToken, "at-recovered") + assert.equal( + write.expectedPriorAccessToken, + "at-borrower-own", + "the guard must be the borrower's own stored token", + ) + assert.notEqual( + write.expectedPriorAccessToken, + "at-lender", + "guarding with the lender's token would fail every CAS", + ) + } finally { + globalThis.fetch = originalFetch + Date.now = originalNow + } + }) }) describe("borrowed credentials after exhaustion", () => { @@ -1777,6 +2340,179 @@ describe("borrowed credentials after exhaustion", () => { } }) + // refreshIfNeeded's up-front re-read pulls from the account's OWN source, + // so anything it adopts is the account's own credentials — it is no longer + // borrowing. Leaving the flag set would make forceRefreshActiveAccount + // refuse to exchange tokens that are legitimately the account's. + it("clears the borrowed flag once the up-front re-read adopts its own credentials", async () => { + const originalFetch = globalThis.fetch + const originalNow = Date.now + const now = 1_700_000_000_000 + Date.now = () => now + globalThis.fetch = (async () => { + throw new Error("network unreachable") + }) as typeof fetch + + try { + const { credentialsModule, keychainModule } = + await loadCredentialsWithCountingKeychain(now - 1_000) + + const borrower = { + label: "Account 1", + source: "Claude Code-credentials-aabbccdd", + credentials: { + accessToken: "at-borrower", + refreshToken: "rt-borrower", + expiresAt: now - 1_000, + }, + } + const lender = { + label: "Account 2", + source: "Claude Code-credentials", + credentials: { + accessToken: "at-lender", + refreshToken: "rt-lender", + expiresAt: now + 60 * 60_000, + }, + } + credentialsModule.initAccounts([borrower, lender]) + + // Its own entry is expired, so it ends up on the lender's tokens. + keychainModule.__setCredentialsForSource(borrower.source, { + accessToken: "at-borrower-own", + refreshToken: "rt-borrower-own", + expiresAt: now - 1_000, + }) + await credentialsModule.refreshIfNeeded(borrower) + assert.equal( + borrower.credentials.accessToken, + "at-lender", + "precondition: the account is running on borrowed tokens", + ) + + // An external process repairs its entry. + keychainModule.__setCredentialsForSource(borrower.source, { + accessToken: "at-borrower-fresh", + refreshToken: "rt-borrower-fresh", + expiresAt: now + 8 * 60 * 60_000, + }) + await credentialsModule.refreshIfNeeded(borrower) + assert.equal(borrower.credentials.accessToken, "at-borrower-fresh") + + const seen: string[] = [] + const forced = await credentialsModule.forceRefreshActiveAccount( + async (token: string) => { + seen.push(token) + return { + accessToken: "at-forced", + refreshToken: "rt-forced", + expiresAt: now + 8 * 60 * 60_000, + } + }, + ) + + assert.deepEqual( + seen, + ["rt-borrower-fresh"], + "a recovered account must be free to force-refresh its own token", + ) + assert.equal(forced?.accessToken, "at-forced") + } finally { + globalThis.fetch = originalFetch + Date.now = originalNow + } + }) + + // The 401 recovery loop reaches its second attempt only through + // reloadCredentialsFromSource, so that adoption site has to clear the flag + // for the same reason refreshIfNeeded's does — matching the invariant + // refreshBorrowedAccount maintains at every other adoption site. Without + // it, a borrower whose own entry has since been repaired reloads onto its + // own credentials, gets rejected again, and then force-refresh declines on + // a flag that is false in fact: a recoverable 401 reaches the user. + it("clears the borrowed flag once a source reload adopts its own credentials", async () => { + const originalFetch = globalThis.fetch + const originalNow = Date.now + const now = 1_700_000_000_000 + Date.now = () => now + globalThis.fetch = (async () => { + throw new Error("network unreachable") + }) as typeof fetch + + try { + const { credentialsModule, keychainModule } = + await loadCredentialsWithCountingKeychain(now - 1_000) + + const borrower = { + label: "Account 1", + source: "Claude Code-credentials-aabbccdd", + credentials: { + accessToken: "at-borrower", + refreshToken: "rt-borrower", + expiresAt: now - 1_000, + }, + } + const lender = { + label: "Account 2", + source: "Claude Code-credentials", + credentials: { + accessToken: "at-lender", + refreshToken: "rt-lender", + expiresAt: now + 60 * 60_000, + }, + } + credentialsModule.initAccounts([borrower, lender]) + + // Its own entry is expired, so it ends up on the lender's tokens. + keychainModule.__setCredentialsForSource(borrower.source, { + accessToken: "at-borrower-own", + refreshToken: "rt-borrower-own", + expiresAt: now - 1_000, + }) + await credentialsModule.refreshIfNeeded(borrower) + assert.equal( + borrower.credentials.accessToken, + "at-lender", + "precondition: the account is running on borrowed tokens", + ) + + // An external process repairs its entry, and the 401 handler picks that + // up through the reload rather than through refreshIfNeeded. + keychainModule.__setCredentialsForSource(borrower.source, { + accessToken: "at-borrower-fresh", + refreshToken: "rt-borrower-fresh", + expiresAt: now + 8 * 60 * 60_000, + }) + assert.equal( + credentialsModule.reloadCredentialsFromSource()?.accessToken, + "at-borrower-fresh", + "precondition: the reload adopts the account's own credentials", + ) + + const seen: string[] = [] + const forced = await credentialsModule.forceRefreshActiveAccount( + async (token: string) => { + seen.push(token) + return { + accessToken: "at-forced", + refreshToken: "rt-forced", + expiresAt: now + 8 * 60 * 60_000, + } + }, + ) + + assert.deepEqual( + seen, + ["rt-borrower-fresh"], + "an account reloaded onto its own token must be free to force-refresh it", + ) + assert.equal(forced?.accessToken, "at-forced") + } finally { + globalThis.fetch = originalFetch + Date.now = originalNow + } + }) + it("forceRefreshActiveAccount refuses to exchange a borrowed token", async () => { const originalFetch = globalThis.fetch const originalNow = Date.now diff --git a/src/credentials.ts b/src/credentials.ts index de230f2..35ead1d 100644 --- a/src/credentials.ts +++ b/src/credentials.ts @@ -323,14 +323,55 @@ export async function refreshIfNeeded( const target = account ?? getActiveAccount() if (!target) return null - // Pick up external updates to .credentials.json (e.g. switch_claude_account - // on Windows). Bounded by getCachedCredentials's 30s TTL: fires at most - // ~2x/min under load. macOS keychain sources stay on the in-memory path; - // their state is mutated only by our own writeBackCredentials, so no - // external-update vector exists for them. - if (target.source === "file") { - const onDisk = refreshAccount(target.source) - if (onDisk) target.credentials = onDisk + // Pick up credentials replaced externally — cswap switching accounts, the + // claude CLI in another terminal, or a second OpenCode instance. This was + // once limited to file sources, on the false assumption that a keychain + // entry is only ever mutated by our own writeBackCredentials. Bounded by + // getCachedCredentials's 30s TTL, so it fires at most ~2x/min under load. + // + // A keychain read shells out to `security`, which throws when the keychain + // is locked, access is denied, or the call times out. Degrade to the + // in-memory credentials rather than take down the request path. + // + // Adopt a usable stored blob always; an unusable one only when what we + // already hold is unusable too. Do not simplify this to an unconditional + // adopt: performRefresh ignores writeBackCredentials's return value, and + // that write can fail while the read before it succeeded (malformed blob, + // or an ACL allowing read but not add-generic-password), leaving memory + // freshly refreshed and the store holding the orphaned pre-refresh blob. + // On the reactive path that blob has under 60s left — that window is the + // only reason we refreshed — so adopting it re-enters performRefresh with + // a refresh token our own refresh just rotated dead: OAuth fails and we + // fall through to two 60s claude spawns, on every cache miss, forever. + // + // Two accepted residuals. An external switch installing an already-expired + // token while ours is usable is ignored until ours expires; cswap freshens + // a target before activating it, so that is rare. And the proactive timer + // refreshes an hour ahead (index.ts), where a failed write-back orphans a + // blob that is still usable — so it IS adopted, costing wasted background + // refreshes rather than failed requests until it drops under 60s and the + // CLI fallback recovers. No guard here closes that one: the re-read cannot + // tell "stale because our write failed" from "changed because cswap + // switched", as both present as store-disagrees-with-memory-and-usable. + // Only the return value performRefresh discards carries the distinction. + try { + const stored = refreshAccount(target.source, target.configDir) + const now = Date.now() + if ( + stored && + (stored.expiresAt > now + 60_000 || + target.credentials.expiresAt <= now + 60_000) + ) { + target.credentials = stored + // Read from this account's own source, so what it returned is this + // account's own credentials — it is no longer running on a lender's. + borrowedCredentialAccounts.delete(target) + } + } catch (err) { + log("source_reread_failed", { + source: target.source, + error: err instanceof Error ? err.message : String(err), + }) } const creds = target.credentials @@ -373,7 +414,22 @@ async function performRefresh( const oauthCreds = await refreshViaOAuth(creds.refreshToken) if (oauthCreds && oauthCreds.expiresAt > Date.now() + 60_000) { target.credentials = oauthCreds - writeBackCredentials(target.source, oauthCreds, target.configDir) + if ( + !writeBackCredentials( + target.source, + oauthCreds, + target.configDir, + creds.accessToken, + ) + ) { + // Mirrors force_refresh_writeback_failed on the forced path. The + // session continues from memory either way, so this stays a log + // rather than a control-flow change: acting on the two causes + // (I/O failure vs. CAS mismatch) differs, and the proactive-path + // consequence — a still-usable orphaned blob being re-adopted by + // the validated re-read — is tracked as a follow-up. + log("refresh_writeback_failed", { source: target.source }) + } return oauthCreds } } @@ -396,8 +452,14 @@ async function performRefresh( // Every OpenCode instance refreshes independently, and a rotation // invalidates the refresh token the others are holding. When ours is // rejected, the instance that won may already have written usable - // credentials to the shared store — far cheaper to re-read than to spawn - // the CLI. (File sources are re-read up front by refreshIfNeeded.) + // credentials to the shared store during the OAuth round trip — far + // cheaper to re-read than to spawn the CLI. + // + // The file-source exclusion below is a leftover from when refreshIfNeeded + // re-read file sources only. That rationale is gone and the exclusion now + // has none: a sibling process can write a file source mid-round-trip + // exactly as it can a keychain entry. Left in place only to keep this + // change off the file path; removing it is tracked as a follow-up. if (target.source !== "file") { let stored: ClaudeCredentials | null = null try { @@ -496,7 +558,12 @@ async function refreshBorrowedAccount( if (oauthCreds && oauthCreds.expiresAt > Date.now() + 60_000) { borrowedCredentialAccounts.delete(target) target.credentials = oauthCreds - writeBackCredentials(target.source, oauthCreds, target.configDir) + writeBackCredentials( + target.source, + oauthCreds, + target.configDir, + own.accessToken, + ) log("refresh_borrowed_recovered", { source: target.source, via: "oauth" }) return oauthCreds } @@ -578,15 +645,21 @@ export function getCredentialsForSync(): ClaudeCredentials | null { /** * Re-read only the active account's credentials from its source (single - * keychain service read or credentials file) and update them in place. - * Used on 401 so an externally refreshed token is picked up without a - * full multi-account keychain rescan. + * keychain service read or credentials file) and update them in place, + * so an externally refreshed token is picked up without a full + * multi-account keychain rescan. + * + * Currently has no call sites: the 401 path uses + * reloadCredentialsFromSource, which additionally validates the result + * and refreshes the cache. Wiring this up or deleting it is tracked as a + * follow-up; until then it must stay consistent with the read paths that + * are live, hence the configDir below. */ export function reloadActiveAccount(): void { const account = getActiveAccount() if (!account) return try { - const fresh = refreshAccount(account.source) + const fresh = refreshAccount(account.source, account.configDir) if (fresh) account.credentials = fresh } catch (err) { log("account_reload_failed", { @@ -619,12 +692,24 @@ export async function forceRefreshActiveAccount( return null } + const priorAccessToken = account.credentials.accessToken const oauthCreds = await refresh(account.credentials.refreshToken) if (oauthCreds && oauthCreds.expiresAt > Date.now() + 60_000) { account.credentials = oauthCreds - if (!writeBackCredentials(account.source, oauthCreds)) { - // Session continues from memory/cache; a later source re-read may - // resurrect the rejected token and trigger another refresh. + if ( + !writeBackCredentials( + account.source, + oauthCreds, + account.configDir, + priorAccessToken, + ) + ) { + // Session continues from memory/cache either way, but the two causes + // diverge on a later source re-read. An I/O failure leaves our own + // rejected token in the store, so the re-read resurrects it and + // triggers another refresh. A CAS mismatch means the store now holds + // another account's token, so the re-read adopts that instead and this + // account stops using the credentials it just refreshed. log("force_refresh_writeback_failed", { source: account.source }) } accountCacheMap.set(account.source, { @@ -692,7 +777,9 @@ export function reloadCredentialsFromSource(): ClaudeCredentials | null { let reloaded: ClaudeCredentials | null try { - reloaded = refreshAccount(account.source) + // Same configDir the write path resolves, so the compare-and-swap in + // writeBackCredentials compares against the file this read came from. + reloaded = refreshAccount(account.source, account.configDir) } catch { accountCacheMap.delete(account.source) log("credentials_source_reload", { @@ -722,6 +809,13 @@ export function reloadCredentialsFromSource(): ClaudeCredentials | null { } account.credentials = reloaded + // Read from this account's own source, so what it returned is this + // account's own credentials — it is no longer running on a lender's. + // Same invariant as refreshIfNeeded's up-front re-read: leaving the flag + // set here makes forceRefreshActiveAccount decline to exchange a token + // that is legitimately this account's, which strands the 401 recovery + // loop's second attempt on a credential it could have refreshed. + borrowedCredentialAccounts.delete(account) accountCacheMap.set(account.source, { creds: reloaded, cachedAt: now }) log("credentials_source_reload", { source: account.source, diff --git a/src/index.test.ts b/src/index.test.ts index bd54d6c..9c356a3 100644 --- a/src/index.test.ts +++ b/src/index.test.ts @@ -1186,7 +1186,7 @@ export function buildAccountLabels(creds) { return creds.map((_, i) => \`Account } }) - it("auth fetch does not retry a 401 when the source token is unchanged", async () => { + it("auth fetch force-refreshes on a 401 when the source token is unchanged", async () => { const originalNow = Date.now const originalSetInterval = globalThis.setInterval const originalHome = process.env.HOME @@ -1198,15 +1198,100 @@ export function buildAccountLabels(creds) { return creds.map((_, i) => \`Account unref() {}, })) as unknown as typeof setInterval - let requestCount = 0 + let apiCalls = 0 + let oauthCalls = 0 + + try { + const { helpersModule } = await loadHelpersWithCountingKeychain( + Date.now() + 10 * 60_000, + ) + + globalThis.fetch = (async (input: RequestInfo | URL) => { + const url = typeof input === "string" ? input : String(input) + if (url.includes("/oauth/token")) { + oauthCalls += 1 + return new Response( + JSON.stringify({ + access_token: "recovered-token", + refresh_token: "rt-recovered", + expires_in: 36_000, + }), + { status: 200 }, + ) + } + apiCalls += 1 + return apiCalls === 1 + ? new Response('{"error":"expired"}', { status: 401 }) + : new Response("data: {}\n\n", { status: 200 }) + }) as typeof fetch + + const plugin = await helpersModule.default({} as never) + const typedPlugin = plugin as { auth?: { loader?: TestAuthLoader } } + const authConfig = await typedPlugin.auth!.loader!( + async () => ({ + type: "oauth", + refresh: "refresh", + access: "access", + expires: Date.now() + 60_000, + }), + { models: {} }, + ) + + const response = await authConfig.fetch( + "https://api.anthropic.com/v1/messages", + { + method: "POST", + body: JSON.stringify({ model: "claude-haiku-4-5", messages: [] }), + }, + ) + + assert.equal(response.status, 200) + assert.equal(oauthCalls, 1, "the unchanged token must force a refresh") + assert.equal(apiCalls, 2, "the refreshed token must be retried once") + } finally { + Date.now = originalNow + globalThis.setInterval = originalSetInterval + globalThis.fetch = originalFetch + if (typeof originalHome === "string") { + process.env.HOME = originalHome + } else { + delete process.env.HOME + } + } + }) + + it("auth fetch surfaces the 401 unchanged when recovery cannot progress", async () => { + const originalNow = Date.now + const originalSetInterval = globalThis.setInterval + const originalHome = process.env.HOME + const originalFetch = globalThis.fetch + const tempHome = await mkdtemp(join(tmpdir(), "opencode-claude-auth-home-")) + process.env.HOME = tempHome + Date.now = () => 1_700_000_000_000 + globalThis.setInterval = (() => ({ + unref() {}, + })) as unknown as typeof setInterval + + let apiCalls = 0 const errorBody = '{"name":"mcp_UnchangedToken"}' try { const { helpersModule } = await loadHelpersWithCountingKeychain( Date.now() + 10 * 60_000, ) - globalThis.fetch = (async () => { - requestCount += 1 + + globalThis.fetch = (async (input: RequestInfo | URL) => { + const url = typeof input === "string" ? input : String(input) + if (url.includes("/oauth/token")) { + // A 429 from the token endpoint must read as a failed refresh, not + // as something that loops account recovery. retry-after beyond the + // 30s cap makes fetchWithRetry return instead of backing off. + return new Response('{"error":"rate_limited"}', { + status: 429, + headers: { "retry-after": "3600" }, + }) + } + apiCalls += 1 return new Response(errorBody, { status: 401, headers: { "x-request-id": "unchanged-token-401" }, @@ -1215,7 +1300,6 @@ export function buildAccountLabels(creds) { return creds.map((_, i) => \`Account const plugin = await helpersModule.default({} as never) const typedPlugin = plugin as { auth?: { loader?: TestAuthLoader } } - assert.equal(typeof typedPlugin.auth?.loader, "function") const authConfig = await typedPlugin.auth!.loader!( async () => ({ type: "oauth", @@ -1237,7 +1321,88 @@ export function buildAccountLabels(creds) { return creds.map((_, i) => \`Account assert.equal(response.status, 401) assert.equal(response.headers.get("x-request-id"), "unchanged-token-401") assert.equal(await response.text(), errorBody) - assert.equal(requestCount, 1) + assert.equal(apiCalls, 1, "no retry without a different token") + } finally { + Date.now = originalNow + globalThis.setInterval = originalSetInterval + globalThis.fetch = originalFetch + if (typeof originalHome === "string") { + process.env.HOME = originalHome + } else { + delete process.env.HOME + } + } + }) + + it("auth fetch makes a second recovery attempt when the first retry is also rejected", async () => { + const originalNow = Date.now + const originalSetInterval = globalThis.setInterval + const originalHome = process.env.HOME + const originalFetch = globalThis.fetch + const tempHome = await mkdtemp(join(tmpdir(), "opencode-claude-auth-home-")) + process.env.HOME = tempHome + Date.now = () => 1_700_000_000_000 + globalThis.setInterval = (() => ({ + unref() {}, + })) as unknown as typeof setInterval + + let apiCalls = 0 + let oauthCalls = 0 + + try { + const { helpersModule, keychainModule } = + await loadHelpersWithCountingKeychain(Date.now() + 10 * 60_000) + + globalThis.fetch = (async (input: RequestInfo | URL) => { + const url = typeof input === "string" ? input : String(input) + if (url.includes("/oauth/token")) { + oauthCalls += 1 + return new Response( + JSON.stringify({ + access_token: "second-stage-token", + refresh_token: "rt-second-stage", + expires_in: 36_000, + }), + { status: 200 }, + ) + } + apiCalls += 1 + // Calls 1 and 2 are rejected; only the force-refreshed token works. + return apiCalls <= 2 + ? new Response('{"error":"expired"}', { status: 401 }) + : new Response("data: {}\n\n", { status: 200 }) + }) as typeof fetch + + const plugin = await helpersModule.default({} as never) + const typedPlugin = plugin as { auth?: { loader?: TestAuthLoader } } + const authConfig = await typedPlugin.auth!.loader!( + async () => ({ + type: "oauth", + refresh: "refresh", + access: "access", + expires: Date.now() + 60_000, + }), + { models: {} }, + ) + + // Attempt 1's reload finds a different, still-valid-looking token. + keychainModule.__setCredentials({ + accessToken: "concurrently-rotated", + refreshToken: "rt-concurrent", + expiresAt: Date.now() + 10 * 60_000, + }) + + const response = await authConfig.fetch( + "https://api.anthropic.com/v1/messages", + { + method: "POST", + body: JSON.stringify({ model: "claude-haiku-4-5", messages: [] }), + }, + ) + + assert.equal(response.status, 200) + assert.equal(apiCalls, 3, "original, reload retry, force-refresh retry") + assert.equal(oauthCalls, 1, "only the second attempt force-refreshes") } finally { Date.now = originalNow globalThis.setInterval = originalSetInterval @@ -1262,7 +1427,8 @@ export function buildAccountLabels(creds) { return creds.map((_, i) => \`Account unref() {}, })) as unknown as typeof setInterval - let requestCount = 0 + let apiCalls = 0 + let oauthCalls = 0 const errorBody = '{"name":"mcp_ReloadFailure"}' try { @@ -1270,8 +1436,16 @@ export function buildAccountLabels(creds) { return creds.map((_, i) => \`Account Date.now() + 10 * 60_000, { throwOnReload: true }, ) - globalThis.fetch = (async () => { - requestCount += 1 + globalThis.fetch = (async (input: RequestInfo | URL) => { + const url = typeof input === "string" ? input : String(input) + // A thrown reload leaves no candidate, so the loop falls through to + // the force refresh. Rejecting it here is what makes recovery + // terminal and surfaces the original 401 untouched. + if (url.includes("/oauth/token")) { + oauthCalls += 1 + return new Response('{"error":"invalid_grant"}', { status: 401 }) + } + apiCalls += 1 return new Response(errorBody, { status: 401, statusText: "Unauthorized", @@ -1308,7 +1482,8 @@ export function buildAccountLabels(creds) { return creds.map((_, i) => \`Account assert.equal(response.headers.get("content-type"), "text/plain") assert.equal(response.headers.get("x-request-id"), "request-401") assert.equal(await response.text(), errorBody) - assert.equal(requestCount, 1) + assert.equal(apiCalls, 1, "no retry once recovery cannot progress") + assert.equal(oauthCalls, 1, "a thrown reload still forces a refresh") } finally { Date.now = originalNow globalThis.setInterval = originalSetInterval @@ -1405,6 +1580,375 @@ export function buildAccountLabels(creds) { return creds.map((_, i) => \`Account } } }) + + it("auth fetch stops at the recovery attempt cap when the store rotates on every read", async () => { + const originalNow = Date.now + const originalSetInterval = globalThis.setInterval + const originalHome = process.env.HOME + const originalFetch = globalThis.fetch + const tempHome = await mkdtemp(join(tmpdir(), "opencode-claude-auth-home-")) + process.env.HOME = tempHome + Date.now = () => 1_700_000_000_000 + globalThis.setInterval = (() => ({ + unref() {}, + })) as unknown as typeof setInterval + + // Against a store rotated on every read, every candidate differs from the + // token in use, so the no-progress break never fires and the attempt cap + // is the only thing stopping the loop. Nothing else pins that ceiling — + // the two-attempt test exits via success, so it constrains the floor. + const authorizationHeaders: string[] = [] + + try { + const { helpersModule, keychainModule } = + await loadHelpersWithCountingKeychain(Date.now() + 10 * 60_000) + + globalThis.fetch = (async (_input, init) => { + authorizationHeaders.push( + new Headers(init?.headers).get("authorization") ?? "", + ) + // Rotate the store before answering, so the next reload always finds + // a token it has not seen. + keychainModule.__setCredentials({ + accessToken: `rotated-${authorizationHeaders.length}`, + refreshToken: `rt-${authorizationHeaders.length}`, + expiresAt: Date.now() + 10 * 60_000, + }) + return new Response('{"error":"expired"}', { status: 401 }) + }) as typeof fetch + + const plugin = await helpersModule.default({} as never) + const typedPlugin = plugin as { auth?: { loader?: TestAuthLoader } } + const authConfig = await typedPlugin.auth!.loader!( + async () => ({ + type: "oauth", + refresh: "refresh", + access: "access", + expires: Date.now() + 60_000, + }), + { models: {} }, + ) + + const response = await authConfig.fetch( + "https://api.anthropic.com/v1/messages", + { + method: "POST", + body: JSON.stringify({ model: "claude-haiku-4-5", messages: [] }), + }, + ) + + assert.equal(response.status, 401) + assert.equal( + authorizationHeaders.length, + 3, + "original request plus two capped recovery attempts, and no more", + ) + } finally { + Date.now = originalNow + globalThis.setInterval = originalSetInterval + globalThis.fetch = originalFetch + if (typeof originalHome === "string") { + process.env.HOME = originalHome + } else { + delete process.env.HOME + } + } + }) + + it("auth fetch compares the 429 re-read against the recovered token, not the original", async () => { + const originalNow = Date.now + const originalSetInterval = globalThis.setInterval + const originalHome = process.env.HOME + const originalFetch = globalThis.fetch + const tempHome = await mkdtemp(join(tmpdir(), "opencode-claude-auth-home-")) + process.env.HOME = tempHome + Date.now = () => 1_700_000_000_000 + globalThis.setInterval = (() => ({ + unref() {}, + })) as unknown as typeof setInterval + + // 401 -> recover -> the retry comes back 429. The 429 block must compare + // the source against the token it just recovered with. Comparing against + // the original `latest` instead still compiles and passes every other + // test, while retrying a quota error on a token the source already agrees + // is current — the exact hazard the block's own comment predicts. + const authorizationHeaders: string[] = [] + + try { + const { helpersModule, keychainModule } = + await loadHelpersWithCountingKeychain(Date.now() + 10 * 60_000) + + globalThis.fetch = (async (_input, init) => { + authorizationHeaders.push( + new Headers(init?.headers).get("authorization") ?? "", + ) + if (authorizationHeaders.length === 1) { + return new Response('{"error":"expired"}', { status: 401 }) + } + return new Response('{"error":"rate_limited"}', { + status: 429, + headers: { "retry-after": "3600" }, + }) + }) as typeof fetch + + const plugin = await helpersModule.default({} as never) + const typedPlugin = plugin as { auth?: { loader?: TestAuthLoader } } + const authConfig = await typedPlugin.auth!.loader!( + async () => ({ + type: "oauth", + refresh: "refresh", + access: "access", + expires: Date.now() + 60_000, + }), + { models: {} }, + ) + + // The store already holds the token the 401 recovery will adopt, so + // once recovered there is nothing further to rotate to. + keychainModule.__setCredentials({ + accessToken: "recovered-token", + refreshToken: "rt-recovered", + expiresAt: Date.now() + 10 * 60_000, + }) + + const response = await authConfig.fetch( + "https://api.anthropic.com/v1/messages", + { + method: "POST", + body: JSON.stringify({ model: "claude-haiku-4-5", messages: [] }), + }, + ) + + assert.equal(response.status, 429) + assert.deepEqual( + authorizationHeaders, + ["Bearer token", "Bearer recovered-token"], + "the 429 must not be retried once the source agrees with the token in use", + ) + } finally { + Date.now = originalNow + globalThis.setInterval = originalSetInterval + globalThis.fetch = originalFetch + if (typeof originalHome === "string") { + process.env.HOME = originalHome + } else { + delete process.env.HOME + } + } + }) + + it("auth fetch retries a 429 once when the source token changed", async () => { + const originalNow = Date.now + const originalSetInterval = globalThis.setInterval + const originalHome = process.env.HOME + const originalFetch = globalThis.fetch + const tempHome = await mkdtemp(join(tmpdir(), "opencode-claude-auth-home-")) + process.env.HOME = tempHome + Date.now = () => 1_700_000_000_000 + globalThis.setInterval = (() => ({ + unref() {}, + })) as unknown as typeof setInterval + + // Captured per call, not just counted: a count alone cannot tell a + // retry that carried the rotated token from one that replayed the + // exhausted token, which is the whole behavior under test. + const authorizationHeaders: string[] = [] + + try { + const { helpersModule, keychainModule } = + await loadHelpersWithCountingKeychain(Date.now() + 10 * 60_000) + + globalThis.fetch = (async (_input, init) => { + authorizationHeaders.push( + new Headers(init?.headers).get("authorization") ?? "", + ) + if (authorizationHeaders.length === 1) { + // retry-after beyond the 30s cap: quota exhaustion, not a + // transient limit, so fetchWithRetry returns immediately. + return new Response('{"error":"rate_limited"}', { + status: 429, + headers: { "retry-after": "3600" }, + }) + } + return new Response("data: {}\n\n", { status: 200 }) + }) as typeof fetch + + const plugin = await helpersModule.default({} as never) + const typedPlugin = plugin as { auth?: { loader?: TestAuthLoader } } + const authConfig = await typedPlugin.auth!.loader!( + async () => ({ + type: "oauth", + refresh: "refresh", + access: "access", + expires: Date.now() + 60_000, + }), + { models: {} }, + ) + + // An external switch lands while this session is still on the + // exhausted account. + keychainModule.__setCredentials({ + accessToken: "switched-token", + refreshToken: "rt-switched", + expiresAt: Date.now() + 10 * 60_000, + }) + + const response = await authConfig.fetch( + "https://api.anthropic.com/v1/messages", + { + method: "POST", + body: JSON.stringify({ model: "claude-haiku-4-5", messages: [] }), + }, + ) + + assert.equal(response.status, 200) + assert.deepEqual( + authorizationHeaders, + ["Bearer token", "Bearer switched-token"], + "the retry must carry the rotated token, not replay the exhausted one", + ) + } finally { + Date.now = originalNow + globalThis.setInterval = originalSetInterval + globalThis.fetch = originalFetch + if (typeof originalHome === "string") { + process.env.HOME = originalHome + } else { + delete process.env.HOME + } + } + }) + + it("auth fetch does not retry a 429 when the source token is unchanged", async () => { + const originalNow = Date.now + const originalSetInterval = globalThis.setInterval + const originalHome = process.env.HOME + const originalFetch = globalThis.fetch + const tempHome = await mkdtemp(join(tmpdir(), "opencode-claude-auth-home-")) + process.env.HOME = tempHome + Date.now = () => 1_700_000_000_000 + globalThis.setInterval = (() => ({ + unref() {}, + })) as unknown as typeof setInterval + + let apiCalls = 0 + + try { + const { helpersModule } = await loadHelpersWithCountingKeychain( + Date.now() + 10 * 60_000, + ) + + globalThis.fetch = (async () => { + apiCalls += 1 + return new Response('{"error":"rate_limited"}', { + status: 429, + headers: { "retry-after": "3600" }, + }) + }) as typeof fetch + + const plugin = await helpersModule.default({} as never) + const typedPlugin = plugin as { auth?: { loader?: TestAuthLoader } } + const authConfig = await typedPlugin.auth!.loader!( + async () => ({ + type: "oauth", + refresh: "refresh", + access: "access", + expires: Date.now() + 60_000, + }), + { models: {} }, + ) + + const response = await authConfig.fetch( + "https://api.anthropic.com/v1/messages", + { + method: "POST", + body: JSON.stringify({ model: "claude-haiku-4-5", messages: [] }), + }, + ) + + assert.equal(response.status, 429) + assert.equal(apiCalls, 1, "an unchanged token must not be retried") + } finally { + Date.now = originalNow + globalThis.setInterval = originalSetInterval + globalThis.fetch = originalFetch + if (typeof originalHome === "string") { + process.env.HOME = originalHome + } else { + delete process.env.HOME + } + } + }) + + it("auth fetch does not retry a 429 when the source is unreadable", async () => { + const originalNow = Date.now + const originalSetInterval = globalThis.setInterval + const originalHome = process.env.HOME + const originalFetch = globalThis.fetch + const tempHome = await mkdtemp(join(tmpdir(), "opencode-claude-auth-home-")) + process.env.HOME = tempHome + Date.now = () => 1_700_000_000_000 + globalThis.setInterval = (() => ({ + unref() {}, + })) as unknown as typeof setInterval + + let apiCalls = 0 + + try { + const { helpersModule, keychainModule } = + await loadHelpersWithCountingKeychain(Date.now() + 10 * 60_000) + + globalThis.fetch = (async () => { + apiCalls += 1 + return new Response('{"error":"rate_limited"}', { + status: 429, + headers: { "retry-after": "3600" }, + }) + }) as typeof fetch + + const plugin = await helpersModule.default({} as never) + const typedPlugin = plugin as { auth?: { loader?: TestAuthLoader } } + const authConfig = await typedPlugin.auth!.loader!( + async () => ({ + type: "oauth", + refresh: "refresh", + access: "access", + expires: Date.now() + 60_000, + }), + { models: {} }, + ) + + // Distinct from the unchanged-token arm: the token DID change, but + // reloadCredentialsFromSource rejects anything expiring within 60s, + // so it yields null. Retrying with a token the reload refused to + // vouch for would burn a request on a credential already known bad. + keychainModule.__setCredentials({ + accessToken: "expiring-token", + refreshToken: "rt-expiring", + expiresAt: Date.now() + 30_000, + }) + + const response = await authConfig.fetch( + "https://api.anthropic.com/v1/messages", + { + method: "POST", + body: JSON.stringify({ model: "claude-haiku-4-5", messages: [] }), + }, + ) + + assert.equal(response.status, 429) + assert.equal(apiCalls, 1, "an unusable reload must not be retried") + } finally { + Date.now = originalNow + globalThis.setInterval = originalSetInterval + globalThis.fetch = originalFetch + if (typeof originalHome === "string") { + process.env.HOME = originalHome + } else { + delete process.env.HOME + } + } + }) }) describe("auth hook — account resolution", () => { diff --git a/src/index.ts b/src/index.ts index 2538e77..fff9c6c 100644 --- a/src/index.ts +++ b/src/index.ts @@ -20,6 +20,7 @@ import { import { getCachedCredentials, reloadCredentialsFromSource, + forceRefreshActiveAccount, getActiveAccount, syncAuthJson, initAccounts, @@ -365,29 +366,167 @@ const plugin: Plugin = async () => { retryAttempt: 0, }) - // On 401, bypass the in-memory cache to pick up credentials rotated by - // another client, then retry once only when the access token changed. - let preserveResponseUnchanged = false - if (response.status === 401) { - let refreshed: ClaudeCredentials | null = null + // Recover from a rejected token: first by adopting credentials + // rotated externally (cswap switching accounts, the claude CLI, + // another OpenCode instance), then by forcing an OAuth refresh + // when the store still holds the token that was just rejected. + // + // Most cases resolve on the first attempt: a cold or unreadable + // store yields null from the reload (reloadCredentialsFromSource + // rejects anything expiring within 60s), so the force refresh runs + // immediately. The second attempt covers the narrower race where + // the reload returns a valid-looking token that a concurrent + // writer has itself just rotated again. + // + // The cap is the real bound. Against a store being rotated on + // every read, every candidate differs from tokenInUse, so the + // no-progress break never fires and the cap alone stops the loop. + // The break is the fast path out of the common cases, not the + // guarantee of termination. + // + // tokenInUse deliberately tracks only the last token tried rather + // than the set of all of them: a store cycling A->B->A wastes one + // request on the second attempt, which is a better trade than + // threading extra state through a loop whose whole virtue is a + // hard ceiling of three API calls. + const MAX_AUTH_RECOVERY_ATTEMPTS = 2 + let tokenInUse = latest.accessToken + + for ( + let attempt = 0; + response.status === 401 && attempt < MAX_AUTH_RECOVERY_ATTEMPTS; + attempt++ + ) { + let candidate: ClaudeCredentials | null = null + // reloadCredentialsFromSource already catches its own source + // read and returns null, so this is unreachable today. It stays + // because the guarantee worth keeping is that no reload failure + // turns a well-formed 401 into an exception thrown out of + // fetch() — degrading to the original response beats crashing + // the request. It logs so a future reload that does throw is + // diagnosable rather than silently null-coalesced. try { - refreshed = reloadCredentialsFromSource() - } catch {} - if (refreshed && refreshed.accessToken !== latest.accessToken) { - const retryHeaders = buildRequestHeaders( + candidate = reloadCredentialsFromSource() + } catch (err) { + log("auth_recovery_reload_threw", { + modelId, + attempt: attempt + 1, + error: err instanceof Error ? err.message : String(err), + }) + } + + if (!candidate || candidate.accessToken === tokenInUse) { + try { + candidate = await forceRefreshActiveAccount() + } catch (err) { + // A rejected refresh and a refresh that returned null are + // different operator-facing diagnoses; auth_recovery_ + // exhausted below collapses them, so record this one here. + log("auth_recovery_force_refresh_threw", { + modelId, + attempt: attempt + 1, + error: err instanceof Error ? err.message : String(err), + }) + } + } + + // Re-checked, not copy-pasted: the guard above decides whether + // to force a refresh, this one decides whether that refresh + // actually produced a token worth retrying with. + if (!candidate || candidate.accessToken === tokenInUse) { + log("auth_recovery_exhausted", { + modelId, + attempt: attempt + 1, + }) + break + } + + tokenInUse = candidate.accessToken + log("auth_recovery_retry", { modelId, attempt: attempt + 1 }) + response = await fetchWithRetry(requestUrl, { + ...requestInit, + body, + headers: buildRequestHeaders( input, requestInit, - refreshed.accessToken, + tokenInUse, modelId, excluded, - ) + ), + }) + } + + // An external switch — cswap rotating off an exhausted account — + // leaves this session on the old token until the 30s credential + // cache expires. Re-read once so a rate limit that has already + // been resolved elsewhere is not surfaced. A changed token is the + // signal that a switch happened; when nothing changed this costs + // one source read and no retry. + // + // Ordered AFTER the 401 recovery loop, and that is a real + // dependency, not incidental sequencing: it must compare against + // the token the loop last tried, so a 401 recovered into a 429 is + // measured against the recovered token rather than the rejected + // one. Only half of this is compiler-enforced — hoisting the block + // above `let tokenInUse` is a TDZ error, but moving it between + // that declaration and the loop still compiles and still passes, + // while silently comparing against a stale token on the + // 401 -> retry -> 429 path. + // + // Ordered before the long-context beta loop deliberately. A + // long-context 429 is a header problem, not an account one, so it + // rotates no token and falls through here untouched. In the rare + // case a switch lands on the same 429, this spends one retry that + // comes back with the same long-context error and the beta loop + // then handles it off the fresh response — one wasted request, + // same outcome. + if (response.status === 429) { + let rotated: ClaudeCredentials | null = null + // Unreachable today for the same reason as the 401 loop's + // reload catch: reloadCredentialsFromSource swallows its own + // source read and returns null. Kept, and logged, on the same + // grounds — no reload failure should turn a readable 429 into + // an exception thrown out of fetch(), and a future reload that + // does throw should be diagnosable rather than silently + // coalesced to "nothing rotated". + try { + rotated = reloadCredentialsFromSource() + } catch (err) { + log("rate_limit_reload_threw", { + modelId, + error: err instanceof Error ? err.message : String(err), + }) + } + + if (rotated && rotated.accessToken !== tokenInUse) { + // Named for what was observed, not for what it implies. A + // changed token is not proof of an account switch: a routine + // refresh of this same exhausted account by another instance + // or the claude CLI changes the token too, and that retry hits + // the same quota. Accepted cost — one request — but the log + // must not tell a quota investigation "we switched accounts" + // when all it saw was a different token. + log("rate_limit_token_changed", { modelId }) + tokenInUse = rotated.accessToken response = await fetchWithRetry(requestUrl, { ...requestInit, body, - headers: retryHeaders, + headers: buildRequestHeaders( + input, + requestInit, + tokenInUse, + modelId, + excluded, + ), + }) + // Whether rotating resolved the limit is the question this + // whole block exists to answer, so record it outright rather + // than leaving success to be inferred from the absence of a + // fetch_error_response line. + log("rate_limit_retry_response", { + modelId, + status: response.status, }) - } else { - preserveResponseUnchanged = true } } @@ -421,8 +560,10 @@ const plugin: Plugin = async () => { }) // Rebuild headers without the excluded beta and retry + // Falls back to tokenInUse, not latest: after a 401 recovery the + // latter is the token the API already rejected. const currentCreds = await getCachedCredentials() - const retryToken = currentCreds?.accessToken ?? latest.accessToken + const retryToken = currentCreds?.accessToken ?? tokenInUse const newExcluded = getExcludedBetas(modelId) const newHeaders = buildRequestHeaders( input, @@ -459,7 +600,10 @@ const plugin: Plugin = async () => { .catch(() => {}) } - return preserveResponseUnchanged + // A 401 that survived recovery carries an error body, not an SSE + // stream. Deciding here rather than from a flag set mid-flight + // makes the retried and non-retried paths behave identically. + return response.status === 401 ? response : transformResponseStream(response) }, diff --git a/src/keychain.test.ts b/src/keychain.test.ts index 3df07e3..be32e64 100644 --- a/src/keychain.test.ts +++ b/src/keychain.test.ts @@ -14,6 +14,7 @@ import { join } from "node:path" import { pathToFileURL } from "node:url" import { buildAccountLabels, + credentialBlobMatches, keychainSuffixForDir, parseCredentials, readAllClaudeAccounts as readAllClaudeAccountsReal, @@ -23,10 +24,7 @@ import { } from "./keychain.ts" // Mirrors listClaudeKeychainServices regex logic for unit testing -async function loadKeychainWithMockedSecurity( - securityDump: string, - keychainEntries: Record, -): Promise<{ +type MockedKeychain = { readAllClaudeAccounts: () => Array<{ label: string source: string @@ -38,7 +36,19 @@ async function loadKeychainWithMockedSecurity( subscriptionType?: string } }> -}> { + writeBackCredentials: ( + source: string, + creds: { accessToken: string; refreshToken: string; expiresAt: number }, + configDir?: string, + expectedPriorAccessToken?: string, + ) => boolean + __getSecurityWrites: () => Array<{ service: string; value: string }> +} + +async function loadKeychainWithMockedSecurity( + securityDump: string, + keychainEntries: Record, +): Promise { const tempDir = await mkdtemp( join(tmpdir(), "opencode-claude-auth-keychain-"), ) @@ -81,11 +91,23 @@ export function execSync(command) { throw new Error("unexpected execSync call: " + command) } +const securityWrites = [] + export function execFileSync(file, args) { if (file !== "/usr/bin/security") { throw new Error("unexpected execFileSync file: " + file) } const service = args[args.indexOf("-s") + 1] + + // add-generic-password is the write path. Record it and mutate the backing + // store, so a test can tell "the write was skipped" from "the write ran". + if (args[0] === "add-generic-password") { + const value = args[args.indexOf("-w") + 1] + securityWrites.push({ service, value }) + keychainEntries[service] = value + return "" + } + const raw = keychainEntries[service] if (raw === undefined) { const error = new Error("The specified item could not be found in the keychain.") @@ -95,25 +117,23 @@ export function execFileSync(file, args) { } return raw } + +export function __getSecurityWrites() { + return securityWrites +} `, "utf8", ) await writeFile(tempKeychain, rewritten, "utf8") - const keychainModule = await import(pathToFileURL(tempKeychain).href) - return keychainModule as { - readAllClaudeAccounts: () => Array<{ - label: string - source: string - configDir?: string - credentials: { - accessToken: string - refreshToken: string - expiresAt: number - subscriptionType?: string - } - }> - } + const [keychainModule, childProcessModule] = await Promise.all([ + import(pathToFileURL(tempKeychain).href), + import(pathToFileURL(tempChildProcess).href), + ]) + return { + ...keychainModule, + __getSecurityWrites: childProcessModule.__getSecurityWrites, + } as MockedKeychain } function extractServicesFromDump(output: string): string[] { @@ -582,6 +602,42 @@ describe("updateCredentialBlob", () => { }) }) +describe("credentialBlobMatches", () => { + const blob = JSON.stringify({ + claudeAiOauth: { + accessToken: "account-a", + refreshToken: "rt-a", + expiresAt: 1, + }, + }) + + it("accepts a blob still holding the expected token", () => { + assert.equal(credentialBlobMatches(blob, "account-a"), true) + }) + + it("rejects a blob replaced by another account", () => { + assert.equal(credentialBlobMatches(blob, "account-b"), false) + }) + + it("rejects an unparseable blob rather than assuming a match", () => { + assert.equal(credentialBlobMatches("not json", "account-a"), false) + }) + + // The guard validates through parseCredentials; the write it protects uses + // updateCredentialBlob, which would rewrite this blob happily. Pinning the + // stricter side so the divergence is a decision rather than an accident. + it("rejects a blob whose token matches but whose other fields are malformed", () => { + const malformed = JSON.stringify({ + claudeAiOauth: { + accessToken: "account-a", + refreshToken: 12345, + expiresAt: "soon", + }, + }) + assert.equal(credentialBlobMatches(malformed, "account-a"), false) + }) +}) + describe("writeBackCredentials (file source)", () => { // These tests isolate via HOME; unset CLAUDE_CONFIG_DIR so an ambient value // (e.g. in CI or a dev shell) doesn't redirect the credentials path. @@ -638,6 +694,103 @@ describe("writeBackCredentials (file source)", () => { } }) + it("writes when the stored token still matches the expected one", async () => { + const originalHome = process.env.HOME + const tempHome = await mkdtemp(join(tmpdir(), "opencode-claude-auth-cas-")) + process.env.HOME = tempHome + + try { + const claudeDir = join(tempHome, ".claude") + mkdirSync(claudeDir, { recursive: true }) + const credPath = join(claudeDir, ".credentials.json") + writeFileSync( + credPath, + JSON.stringify({ + claudeAiOauth: { + accessToken: "old-at", + refreshToken: "old-rt", + expiresAt: 1000, + subscriptionType: "pro", + }, + }), + { encoding: "utf-8", mode: 0o600 }, + ) + + const result = writeBackCredentials( + "file", + { + accessToken: "new-at", + refreshToken: "new-rt", + expiresAt: 2000, + }, + undefined, + "old-at", + ) + + assert.equal(result, true, "a matching guard must not block the write") + const written = JSON.parse(readFileSync(credPath, "utf-8")) + assert.equal(written.claudeAiOauth.accessToken, "new-at") + assert.equal(written.claudeAiOauth.subscriptionType, "pro") + } finally { + if (typeof originalHome === "string") { + process.env.HOME = originalHome + } else { + delete process.env.HOME + } + rmSync(tempHome, { recursive: true, force: true }) + } + }) + + it("skips the write when the stored token is no longer the expected one", async () => { + const originalHome = process.env.HOME + const tempHome = await mkdtemp(join(tmpdir(), "opencode-claude-auth-cas-")) + process.env.HOME = tempHome + + try { + const claudeDir = join(tempHome, ".claude") + mkdirSync(claudeDir, { recursive: true }) + const credPath = join(claudeDir, ".credentials.json") + // Another process switched accounts after we read "expected-at". + writeFileSync( + credPath, + JSON.stringify({ + claudeAiOauth: { + accessToken: "switched-in-at", + refreshToken: "switched-in-rt", + expiresAt: 1000, + }, + }), + { encoding: "utf-8", mode: 0o600 }, + ) + + const result = writeBackCredentials( + "file", + { + accessToken: "our-refreshed-at", + refreshToken: "our-refreshed-rt", + expiresAt: 2000, + }, + undefined, + "expected-at", + ) + + assert.equal(result, false) + const written = JSON.parse(readFileSync(credPath, "utf-8")) + assert.equal( + written.claudeAiOauth.accessToken, + "switched-in-at", + "the switched-in credential must survive untouched", + ) + } finally { + if (typeof originalHome === "string") { + process.env.HOME = originalHome + } else { + delete process.env.HOME + } + rmSync(tempHome, { recursive: true, force: true }) + } + }) + it("writes file with 0o600 permissions", async () => { if (process.platform === "win32") return @@ -776,6 +929,91 @@ describe("writeBackCredentials (file source)", () => { }) }) +describe("writeBackCredentials (keychain source)", () => { + // The keychain is the primary platform and the reason the compare-and-swap + // guard exists, so it needs its own coverage: the file-source tests cannot + // reach this branch, and `process.platform` is rewritten to "darwin" by the + // harness so these run on any host. + const SERVICE = "Claude Code-credentials" + const DUMP = `"${SERVICE}"` + + const blob = (accessToken: string) => + JSON.stringify({ + claudeAiOauth: { + accessToken, + refreshToken: `rt-for-${accessToken}`, + expiresAt: 1000, + subscriptionType: "max", + }, + }) + + const refreshed = { + accessToken: "our-refreshed-at", + refreshToken: "our-refreshed-rt", + expiresAt: 2000, + } + + it("skips the write when the stored token is no longer the expected one", async () => { + const { writeBackCredentials, __getSecurityWrites } = + await loadKeychainWithMockedSecurity(DUMP, { + // Another account was switched in after we read "expected-at". + [SERVICE]: blob("switched-in-at"), + }) + + const result = writeBackCredentials( + SERVICE, + refreshed, + undefined, + "expected-at", + ) + + assert.equal(result, false) + assert.deepEqual( + __getSecurityWrites(), + [], + "the switched-in credential must survive untouched", + ) + }) + + it("writes when the stored token still matches the expected one", async () => { + const { writeBackCredentials, __getSecurityWrites } = + await loadKeychainWithMockedSecurity(DUMP, { + [SERVICE]: blob("expected-at"), + }) + + const result = writeBackCredentials( + SERVICE, + refreshed, + undefined, + "expected-at", + ) + + assert.equal(result, true) + const writes = __getSecurityWrites() + assert.equal(writes.length, 1) + assert.equal(writes[0].service, SERVICE) + const written = JSON.parse(writes[0].value) as { + claudeAiOauth: { accessToken: string; subscriptionType: string } + } + assert.equal(written.claudeAiOauth.accessToken, "our-refreshed-at") + assert.equal( + written.claudeAiOauth.subscriptionType, + "max", + "unrelated fields must be preserved", + ) + }) + + it("writes without a guard when no expected token is supplied", async () => { + const { writeBackCredentials, __getSecurityWrites } = + await loadKeychainWithMockedSecurity(DUMP, { + [SERVICE]: blob("whatever-is-there"), + }) + + assert.equal(writeBackCredentials(SERVICE, refreshed), true) + assert.equal(__getSecurityWrites().length, 1) + }) +}) + describe("readAllClaudeAccounts (file source)", () => { it("reads credentials from CLAUDE_CONFIG_DIR when set", async () => { // Non-darwin reads go straight to the credentials file; on darwin the diff --git a/src/keychain.ts b/src/keychain.ts index 24c7d99..4aa9332 100644 --- a/src/keychain.ts +++ b/src/keychain.ts @@ -439,10 +439,46 @@ function getKeychainAccountName(serviceName: string): string | null { } } +/** + * A short, non-reversible tag for an access token, so write-back decisions + * can be correlated across log lines (and against another account's) without + * putting token material in the log. + */ +function tokenFingerprint(token: string): string { + return createHash("sha256").update(token).digest("hex").slice(0, 8) +} + +/** + * Whether a stored credential blob still carries the access token we expect. + * + * Guards write-back against an external switch landing between the read that + * produced the token being refreshed and the write of its replacement. An + * unparseable blob returns false: a write into state we cannot identify is + * exactly what this is meant to prevent. + * + * Deliberately stricter than the write it guards: this validates through + * parseCredentials (field types, mcpOAuth-only rejection) while + * updateCredentialBlob rewrites anything JSON.parse accepts. So a blob the + * write path would happily update can still be refused here. That gap is + * intentional — reaching this guard means an earlier parseCredentials + * succeeded, so a blob that no longer parses changed shape after we read it + * (a truncated non-atomic external write, or a hand edit), and declining to + * write over it is correct. Anyone tightening parseCredentials should know it + * narrows write-back too, not just reads. + */ +export function credentialBlobMatches( + raw: string, + expectedAccessToken: string, +): boolean { + const parsed = parseCredentials(raw) + return parsed?.accessToken === expectedAccessToken +} + export function writeBackCredentials( source: string, creds: ClaudeCredentials, configDir?: string, + expectedPriorAccessToken?: string, ): boolean { const newCreds = { accessToken: creds.accessToken, @@ -456,6 +492,26 @@ export function writeBackCredentials( configDir ?? process.env.CLAUDE_CONFIG_DIR ?? join(homedir(), ".claude") const credPath = join(dir, ".credentials.json") const raw = readFileSync(credPath, "utf-8") + if ( + expectedPriorAccessToken !== undefined && + !credentialBlobMatches(raw, expectedPriorAccessToken) + ) { + // Two operationally opposite causes: another account legitimately + // holds the slot (benign, no action) versus a blob we cannot read at + // all (corruption, needs a human). Same refusal, different events. + const stored = parseCredentials(raw) + if (stored === null) { + log("writeback_skipped_unparseable", { source, configDir: dir }) + } else { + log("writeback_skipped_stale", { + source, + configDir: dir, + expected: tokenFingerprint(expectedPriorAccessToken), + stored: tokenFingerprint(stored.accessToken), + }) + } + return false + } const updated = updateCredentialBlob(raw, newCreds) if (!updated) return false writeFileSync(credPath, updated, { encoding: "utf-8", mode: 0o600 }) @@ -474,6 +530,23 @@ export function writeBackCredentials( try { const raw = readKeychainService(source) if (!raw) return false + if ( + expectedPriorAccessToken !== undefined && + !credentialBlobMatches(raw, expectedPriorAccessToken) + ) { + // See the file branch: a mismatch is expected, an unreadable blob is not. + const stored = parseCredentials(raw) + if (stored === null) { + log("writeback_skipped_unparseable", { source }) + } else { + log("writeback_skipped_stale", { + source, + expected: tokenFingerprint(expectedPriorAccessToken), + stored: tokenFingerprint(stored.accessToken), + }) + } + return false + } const updated = updateCredentialBlob(raw, newCreds) if (!updated) return false const accountName = getKeychainAccountName(source) ?? source