From e40280e4cc30b1d0b01789bebf620541bdd98f53 Mon Sep 17 00:00:00 2001 From: Scott Lovegrove Date: Tue, 19 May 2026 11:05:16 +0100 Subject: [PATCH 1/5] feat(auth): add TokenBundle storage contract to TokenStore MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Widens the `TokenStore` contract to carry refresh-token state alongside the access token, without changing behaviour for existing single-token consumers. `flow.ts` still persists access only — the bundle-aware write path lands in a follow-up that wires `runOAuthFlow` and the refresh helper to it. Contract additions (all backwards-compatible): - `TokenBundle` and `RefreshInput` types. - `TokenStore.active()` widened to return optional `bundle?: TokenBundle`. - `TokenStore.setBundle?(account, bundle, options?)` optional method. - `AuthProvider.refreshToken?(input)` optional method for future PKCE refresh-grant impl. - `UserRecord` gains `accessTokenExpiresAt`, `refreshTokenExpiresAt`, `fallbackRefreshToken`, and a tri-state `hasRefreshToken` gate that lets `active()` skip the second keyring IPC for records that have no refresh token. - `UserRecordStore.tryInsert?(record)` optional atomic-insert hook for the follow-up migration PR. Implementation: - New `keyring/slot-naming.ts` single-sources the `/refresh` slot suffix so the runtime read path and any future rename can't drift. - `KeyringTokenStore.setBundle` is a required override (non-optional) that writes both slots via the new `writeBundleWithKeyringFallback` helper. Rollback on partial failure: refresh-slot non-keyring error rolls back the access slot; upsert failure rolls back both slots via `Promise.allSettled`. - `active()` reads access and refresh slots in parallel, gated by `hasRefreshToken`. `SecureStoreUnavailableError` on the refresh slot downgrades to no-refresh; any other read error propagates as `AUTH_STORE_READ_FAILED`. `undefined` gate (legacy records) probes once and best-effort backfills `false` for the next read. - `clear()` wipes both keyring slots — leaving an orphan refresh slot would defeat the whole point. - `set(account, token)` now writes `hasRefreshToken: false` so subsequent `active()` calls skip the refresh probe for accounts written via the single-token path. - New `persist.ts` ships `persistBundle({ store, account, bundle, promoteDefault? })` — prefers `setBundle` when available, falls back to `set(account, bundle.accessToken)` otherwise, and uniformly wraps non-`CliError` failures as `AUTH_STORE_WRITE_FAILED`. The bundle- aware `flow.ts` and the refresh helper both call it in the follow-up PR. 24 new tests, 384 total. `npm run check` / `type-check` / `test` / `build` all clean. Co-Authored-By: Claude Opus 4.7 (1M context) --- README.md | 34 ++-- src/auth/index.ts | 4 + src/auth/keyring/record-write.test.ts | 116 +++++++++++- src/auth/keyring/record-write.ts | 126 ++++++++++++- src/auth/keyring/slot-naming.test.ts | 9 + src/auth/keyring/slot-naming.ts | 12 ++ src/auth/keyring/token-store.test.ts | 196 +++++++++++++++++++- src/auth/keyring/token-store.ts | 254 ++++++++++++++++++++------ src/auth/keyring/types.ts | 34 ++++ src/auth/persist.test.ts | 82 +++++++++ src/auth/persist.ts | 51 ++++++ src/auth/types.ts | 56 +++++- 12 files changed, 890 insertions(+), 84 deletions(-) create mode 100644 src/auth/keyring/slot-naming.test.ts create mode 100644 src/auth/keyring/slot-naming.ts create mode 100644 src/auth/persist.test.ts create mode 100644 src/auth/persist.ts diff --git a/README.md b/README.md index 8c3b595..6f165da 100644 --- a/README.md +++ b/README.md @@ -12,20 +12,20 @@ npm install @doist/cli-core ## What's in it -| Module | Key exports | Purpose | -| -------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| `auth` (subpath) | `attachLoginCommand`, `attachLogoutCommand`, `attachStatusCommand`, `attachTokenViewCommand`, `runOAuthFlow`, `createPkceProvider`, `createSecureStore`, `createKeyringTokenStore`, `migrateLegacyAuth`, PKCE helpers, `AuthProvider` / `TokenStore` / `AccountRef` / `SecureStore` / `UserRecordStore` types, `AttachLogoutRevokeContext` | OAuth runtime plus the Commander attachers for ` [auth] login` / `logout` / `status` / `token`. `attachLogoutCommand` accepts an optional `revokeToken` hook for best-effort server-side token revocation. Ships the standard public-client PKCE flow (`createPkceProvider`), a thin cross-platform OS-keyring wrapper (`createSecureStore`), and a multi-account keyring-backed `TokenStore` (`createKeyringTokenStore`) that stores secrets in the OS credential manager and degrades to plaintext in the consumer's config when the keyring is unavailable (WSL/headless Linux/containers). `AuthProvider` and `TokenStore` remain the escape hatches for DCR or fully bespoke backends. `logout` / `status` / `token` always attach `--user ` and thread the parsed ref to `store.active(ref)` (and `store.clear(ref)` on `logout`). `commander` (when using the attachers), `open` (browser launch), and `@napi-rs/keyring` (when using `createSecureStore` or the keyring `TokenStore`) are optional peer/optional deps. | -| `commands` (subpath) | `registerChangelogCommand`, `registerUpdateCommand` (+ semver helpers) | Commander wiring for cli-core's standard commands (e.g. ` changelog`, ` update`, ` update switch`). **Requires** `commander` as an optional peer-dep. | -| `config` | `getConfigPath`, `readConfig`, `readConfigStrict`, `writeConfig`, `updateConfig`, `CoreConfig`, `UpdateChannel` | Read / write a per-CLI JSON config file with typed error codes; `CoreConfig` is the shape of fields cli-core itself owns (extend it for per-CLI fields). | -| `empty` | `printEmpty` | Print an empty-state message gated on `--json` / `--ndjson` so machine consumers never see human strings on stdout. | -| `errors` | `CliError` | Typed CLI error class with `code` and exit-code mapping. | -| `global-args` | `parseGlobalArgs`, `stripUserFlag`, `createGlobalArgsStore`, `createAccessibleGate`, `createSpinnerGate`, `getProgressJsonlPath`, `isProgressJsonlEnabled` | Parse well-known global flags (`--json`, `--ndjson`, `--quiet`, `--verbose`, `--accessible`, `--no-spinner`, `--progress-jsonl`, `--user `) and derive predicates from them. `stripUserFlag` removes `--user` tokens from argv so the cleaned array can be forwarded to Commander when the flag has no root-program attachment. | -| `json` | `formatJson`, `formatNdjson` | Stable JSON / newline-delimited JSON formatting for stdout. | -| `markdown` (subpath) | `preloadMarkdown`, `renderMarkdown`, `TerminalRendererOptions` | Lazy-init terminal markdown renderer. **Requires** `marked` and `marked-terminal-renderer` as peer-deps — install only if your CLI uses this subpath. | -| `options` | `ViewOptions` | Type contract for `{ json?, ndjson? }` per-command options that machine-output gates derive from. | -| `spinner` | `createSpinner` | Loading spinner factory wrapping `yocto-spinner` with disable gates. | -| `terminal` | `isCI`, `isStderrTTY`, `isStdinTTY`, `isStdoutTTY` | TTY / CI detection helpers. | -| `testing` (subpath) | `describeEmptyMachineOutput` | Vitest helpers reusable by consuming CLIs (e.g. parametrised empty-state suite covering `--json` / `--ndjson` / human modes). | +| Module | Key exports | Purpose | +| -------------------- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `auth` (subpath) | `attachLoginCommand`, `attachLogoutCommand`, `attachStatusCommand`, `attachTokenViewCommand`, `runOAuthFlow`, `createPkceProvider`, `createSecureStore`, `createKeyringTokenStore`, `migrateLegacyAuth`, `persistBundle`, PKCE helpers, `AuthProvider` / `TokenStore` / `TokenBundle` / `RefreshInput` / `AccountRef` / `SecureStore` / `UserRecordStore` types, `AttachLogoutRevokeContext` | OAuth runtime plus the Commander attachers for ` [auth] login` / `logout` / `status` / `token`. `attachLogoutCommand` accepts an optional `revokeToken` hook for best-effort server-side token revocation. Ships the standard public-client PKCE flow (`createPkceProvider`), a thin cross-platform OS-keyring wrapper (`createSecureStore`), and a multi-account keyring-backed `TokenStore` (`createKeyringTokenStore`) that stores secrets in the OS credential manager and degrades to plaintext in the consumer's config when the keyring is unavailable (WSL/headless Linux/containers). The store contract supports an optional `setBundle(account, bundle)` method (required on `KeyringTokenStore`) so consumers that need refresh-token persistence can opt in via `TokenBundle`. `AuthProvider` and `TokenStore` remain the escape hatches for DCR or fully bespoke backends. `logout` / `status` / `token` always attach `--user ` and thread the parsed ref to `store.active(ref)` (and `store.clear(ref)` on `logout`). `commander` (when using the attachers), `open` (browser launch), and `@napi-rs/keyring` (when using `createSecureStore` or the keyring `TokenStore`) are optional peer/optional deps. | +| `commands` (subpath) | `registerChangelogCommand`, `registerUpdateCommand` (+ semver helpers) | Commander wiring for cli-core's standard commands (e.g. ` changelog`, ` update`, ` update switch`). **Requires** `commander` as an optional peer-dep. | +| `config` | `getConfigPath`, `readConfig`, `readConfigStrict`, `writeConfig`, `updateConfig`, `CoreConfig`, `UpdateChannel` | Read / write a per-CLI JSON config file with typed error codes; `CoreConfig` is the shape of fields cli-core itself owns (extend it for per-CLI fields). | +| `empty` | `printEmpty` | Print an empty-state message gated on `--json` / `--ndjson` so machine consumers never see human strings on stdout. | +| `errors` | `CliError` | Typed CLI error class with `code` and exit-code mapping. | +| `global-args` | `parseGlobalArgs`, `stripUserFlag`, `createGlobalArgsStore`, `createAccessibleGate`, `createSpinnerGate`, `getProgressJsonlPath`, `isProgressJsonlEnabled` | Parse well-known global flags (`--json`, `--ndjson`, `--quiet`, `--verbose`, `--accessible`, `--no-spinner`, `--progress-jsonl`, `--user `) and derive predicates from them. `stripUserFlag` removes `--user` tokens from argv so the cleaned array can be forwarded to Commander when the flag has no root-program attachment. | +| `json` | `formatJson`, `formatNdjson` | Stable JSON / newline-delimited JSON formatting for stdout. | +| `markdown` (subpath) | `preloadMarkdown`, `renderMarkdown`, `TerminalRendererOptions` | Lazy-init terminal markdown renderer. **Requires** `marked` and `marked-terminal-renderer` as peer-deps — install only if your CLI uses this subpath. | +| `options` | `ViewOptions` | Type contract for `{ json?, ndjson? }` per-command options that machine-output gates derive from. | +| `spinner` | `createSpinner` | Loading spinner factory wrapping `yocto-spinner` with disable gates. | +| `terminal` | `isCI`, `isStderrTTY`, `isStdinTTY`, `isStdoutTTY` | TTY / CI detection helpers. | +| `testing` (subpath) | `describeEmptyMachineOutput` | Vitest helpers reusable by consuming CLIs (e.g. parametrised empty-state suite covering `--json` / `--ndjson` / human modes). | ## Usage @@ -288,6 +288,12 @@ export const tokenStore: TokenStore = { For multi-account storage (OS keychain, per-user config slots, …), implement the same five methods against your backend and honour `ref` on `active` / `clear` / `setDefault`. `AccountRef` is an opaque `string` — cli-core does not constrain matching semantics (id-exact, email-case-insensitive, label, …). The store impl owns that. +##### Refresh-token storage (`TokenBundle`) + +Stores that target servers issuing refresh tokens may implement the optional `setBundle(account, bundle, options?)` method and return a populated `bundle` field from `active()`. `TokenBundle` carries `{ accessToken, refreshToken?, accessTokenExpiresAt?, refreshTokenExpiresAt? }`. Stores that omit `setBundle` continue to work — cli-core helpers (`persistBundle`) fall back to `set(account, bundle.accessToken)` and silently drop refresh state. `KeyringTokenStore` implements `setBundle` as a required override and routes the refresh token to a sibling keyring slot. + +The `persistBundle({ store, account, bundle, promoteDefault? })` helper is the canonical write path — it prefers `setBundle` when available, falls back to `set` otherwise, and wraps non-`CliError` failures as `AUTH_STORE_WRITE_FAILED`. Login persistence (and a future `refreshAccessToken` helper) both use it. + #### Keyring primitive (`createSecureStore`) When the OS credential manager is the right place for your token, `createSecureStore` is a thin cross-platform wrapper around `@napi-rs/keyring`. It exposes a three-method handle (`getSecret` / `setSecret` / `deleteSecret`) for one slot identified by `serviceName` + `account`: diff --git a/src/auth/index.ts b/src/auth/index.ts index 90858c9..e0595f6 100644 --- a/src/auth/index.ts +++ b/src/auth/index.ts @@ -20,6 +20,8 @@ export { generateVerifier, } from './pkce.js' export type { GenerateVerifierOptions } from './pkce.js' +export { persistBundle } from './persist.js' +export type { PersistBundleOptions } from './persist.js' export { createPkceProvider } from './providers/pkce.js' export type { PkceLazyString, PkceProviderOptions } from './providers/pkce.js' export type { @@ -32,6 +34,8 @@ export type { ExchangeResult, PrepareInput, PrepareResult, + RefreshInput, + TokenBundle, TokenStore, ValidateInput, } from './types.js' diff --git a/src/auth/keyring/record-write.test.ts b/src/auth/keyring/record-write.test.ts index 9d79478..8596408 100644 --- a/src/auth/keyring/record-write.test.ts +++ b/src/auth/keyring/record-write.test.ts @@ -1,7 +1,8 @@ import { describe, expect, it } from 'vitest' import { buildSingleSlot, buildUserRecords } from '../../test-support/keyring-mocks.js' -import { writeRecordWithKeyringFallback } from './record-write.js' +import type { TokenBundle } from '../types.js' +import { writeBundleWithKeyringFallback, writeRecordWithKeyringFallback } from './record-write.js' import { SecureStoreUnavailableError } from './secure-store.js' type Account = { id: string; label?: string; email: string } @@ -22,7 +23,7 @@ describe('writeRecordWithKeyringFallback', () => { expect(result.storedSecurely).toBe(true) expect(secureStore.setSpy).toHaveBeenCalledWith('tok_secret') - expect(upsertSpy).toHaveBeenCalledWith({ account }) + expect(upsertSpy).toHaveBeenCalledWith({ account, hasRefreshToken: false }) expect(state.records.get('42')?.fallbackToken).toBeUndefined() }) @@ -95,3 +96,114 @@ describe('writeRecordWithKeyringFallback', () => { expect(secureStore.deleteSpy).not.toHaveBeenCalled() }) }) + +describe('writeBundleWithKeyringFallback', () => { + const bundle: TokenBundle = { + accessToken: 'tok_a', + refreshToken: 'tok_r', + accessTokenExpiresAt: 1_700_000_000_000, + refreshTokenExpiresAt: 1_701_000_000_000, + } + + function harness() { + const accessStore = buildSingleSlot() + const refreshStore = buildSingleSlot() + const records = buildUserRecords() + return { accessStore, refreshStore, ...records } + } + + it('writes both slots and persists the bundle metadata on the happy path', async () => { + const { accessStore, refreshStore, store: userRecords, state, upsertSpy } = harness() + + const result = await writeBundleWithKeyringFallback({ + accessStore, + refreshStore, + userRecords, + account, + bundle, + }) + + expect(result).toEqual({ accessStoredSecurely: true, refreshStoredSecurely: true }) + expect(accessStore.setSpy).toHaveBeenCalledWith('tok_a') + expect(refreshStore.setSpy).toHaveBeenCalledWith('tok_r') + expect(upsertSpy).toHaveBeenCalledWith({ + account, + accessTokenExpiresAt: 1_700_000_000_000, + refreshTokenExpiresAt: 1_701_000_000_000, + hasRefreshToken: true, + }) + expect(state.records.get('42')?.fallbackToken).toBeUndefined() + expect(state.records.get('42')?.fallbackRefreshToken).toBeUndefined() + }) + + it('clears the refresh slot when the bundle has no refresh token (no stale carryover)', async () => { + const { accessStore, refreshStore, store: userRecords, state } = harness() + + await writeBundleWithKeyringFallback({ + accessStore, + refreshStore, + userRecords, + account, + bundle: { accessToken: 'tok_a' }, + }) + + expect(refreshStore.deleteSpy).toHaveBeenCalledTimes(1) + expect(state.records.get('42')?.hasRefreshToken).toBe(false) + }) + + it('falls back to fallbackRefreshToken when the refresh slot is offline', async () => { + const { accessStore, refreshStore, store: userRecords, state } = harness() + refreshStore.setSpy.mockRejectedValueOnce(new SecureStoreUnavailableError('no dbus')) + + const result = await writeBundleWithKeyringFallback({ + accessStore, + refreshStore, + userRecords, + account, + bundle, + }) + + expect(result).toEqual({ accessStoredSecurely: true, refreshStoredSecurely: false }) + expect(state.records.get('42')?.fallbackRefreshToken).toBe('tok_r') + expect(state.records.get('42')?.fallbackToken).toBeUndefined() + }) + + it('rolls back the access slot when a non-keyring refresh-slot setSecret error fires', async () => { + // Refresh-slot setSecret throws an unexpected error (not + // SecureStoreUnavailable) — leaving the access slot written would + // orphan a credential against a never-persisted record. + const { accessStore, refreshStore, store: userRecords, state } = harness() + refreshStore.setSpy.mockRejectedValueOnce(new Error('refresh slot blew up')) + + await expect( + writeBundleWithKeyringFallback({ + accessStore, + refreshStore, + userRecords, + account, + bundle, + }), + ).rejects.toThrow('refresh slot blew up') + + expect(accessStore.deleteSpy).toHaveBeenCalledTimes(1) + expect(state.records.size).toBe(0) + }) + + it('rolls back BOTH keyring slots when upsert fails after both writes succeeded', async () => { + const { accessStore, refreshStore, store: userRecords, upsertSpy } = harness() + upsertSpy.mockRejectedValueOnce(new Error('disk full')) + + await expect( + writeBundleWithKeyringFallback({ + accessStore, + refreshStore, + userRecords, + account, + bundle, + }), + ).rejects.toThrow('disk full') + + expect(accessStore.deleteSpy).toHaveBeenCalledTimes(1) + expect(refreshStore.deleteSpy).toHaveBeenCalledTimes(1) + }) +}) diff --git a/src/auth/keyring/record-write.ts b/src/auth/keyring/record-write.ts index c8c850b..a835114 100644 --- a/src/auth/keyring/record-write.ts +++ b/src/auth/keyring/record-write.ts @@ -1,4 +1,4 @@ -import type { AuthAccount } from '../types.js' +import type { AuthAccount, TokenBundle } from '../types.js' import { type SecureStore, SecureStoreUnavailableError } from './secure-store.js' import type { UserRecord, UserRecordStore } from './types.js' @@ -15,6 +15,27 @@ type WriteRecordResult = { storedSecurely: boolean } +type WriteBundleOptions = { + /** Per-account access-token keyring slot. */ + accessStore: SecureStore + /** Per-account refresh-token keyring slot. */ + refreshStore: SecureStore + userRecords: UserRecordStore + account: TAccount + bundle: TokenBundle +} + +export type WriteBundleResult = { + /** `true` when the access token landed in the OS keyring; `false` when it fell back to `fallbackToken`. */ + accessStoredSecurely: boolean + /** + * `true` when a refresh token landed in the OS keyring. `false` when it + * fell back to `fallbackRefreshToken`. `undefined` when the bundle + * carried no refresh token (nothing to store). + */ + refreshStoredSecurely: boolean | undefined +} + /** * Shared keyring-then-record write used by `createKeyringTokenStore.set` and * `migrateLegacyAuth`. Encapsulates the order-of-operations contract that @@ -46,9 +67,13 @@ export async function writeRecordWithKeyringFallback = storedSecurely - ? { account } - : { account, fallbackToken: trimmed } + ? { account, hasRefreshToken: false } + : { account, fallbackToken: trimmed, hasRefreshToken: false } try { await userRecords.upsert(record) @@ -65,3 +90,98 @@ export async function writeRecordWithKeyringFallback( + options: WriteBundleOptions, +): Promise { + const { accessStore, refreshStore, userRecords, account, bundle } = options + const accessToken = bundle.accessToken.trim() + const refreshToken = bundle.refreshToken?.trim() + + let accessStoredSecurely = false + try { + await accessStore.setSecret(accessToken) + accessStoredSecurely = true + } catch (error) { + if (!(error instanceof SecureStoreUnavailableError)) throw error + } + + let refreshStoredSecurely: boolean | undefined + if (refreshToken) { + try { + await refreshStore.setSecret(refreshToken) + refreshStoredSecurely = true + } catch (error) { + if (error instanceof SecureStoreUnavailableError) { + refreshStoredSecurely = false + } else { + if (accessStoredSecurely) { + try { + await accessStore.deleteSecret() + } catch { + // best-effort + } + } + throw error + } + } + } else { + // Bundle has no refresh token; clear any stale slot so a previous + // bundle's refresh token can't be read back on the next `active()`. + try { + await refreshStore.deleteSecret() + } catch { + // best-effort + } + } + + const record: UserRecord = { + account, + ...(accessStoredSecurely ? {} : { fallbackToken: accessToken }), + ...(refreshToken && refreshStoredSecurely === false + ? { fallbackRefreshToken: refreshToken } + : {}), + ...(bundle.accessTokenExpiresAt !== undefined + ? { accessTokenExpiresAt: bundle.accessTokenExpiresAt } + : {}), + ...(bundle.refreshTokenExpiresAt !== undefined + ? { refreshTokenExpiresAt: bundle.refreshTokenExpiresAt } + : {}), + hasRefreshToken: refreshToken !== undefined, + } + + try { + await userRecords.upsert(record) + } catch (error) { + const rollbacks: Promise[] = [] + if (accessStoredSecurely) rollbacks.push(accessStore.deleteSecret()) + if (refreshStoredSecurely === true) rollbacks.push(refreshStore.deleteSecret()) + if (rollbacks.length > 0) { + await Promise.allSettled(rollbacks) + } + throw error + } + + return { accessStoredSecurely, refreshStoredSecurely } +} diff --git a/src/auth/keyring/slot-naming.test.ts b/src/auth/keyring/slot-naming.test.ts new file mode 100644 index 0000000..a09d8f7 --- /dev/null +++ b/src/auth/keyring/slot-naming.test.ts @@ -0,0 +1,9 @@ +import { describe, expect, it } from 'vitest' + +import { refreshAccountSlot } from './slot-naming.js' + +describe('refreshAccountSlot', () => { + it('derives the refresh slot suffix from the access slot slug', () => { + expect(refreshAccountSlot('user-42')).toBe('user-42/refresh') + }) +}) diff --git a/src/auth/keyring/slot-naming.ts b/src/auth/keyring/slot-naming.ts new file mode 100644 index 0000000..c8f96b0 --- /dev/null +++ b/src/auth/keyring/slot-naming.ts @@ -0,0 +1,12 @@ +/** + * Single source of truth for keyring slot suffixes derived from a base + * `accountForUser(id)` slug. Importing this helper instead of inlining the + * string ensures the runtime read path and any future rename agree byte for + * byte on the wire format — a stale literal in one place would silently + * park tokens in a slot the runtime never reads from. + * + * Internal: not re-exported from `auth/keyring/index.ts`. + */ +export function refreshAccountSlot(accountSlug: string): string { + return `${accountSlug}/refresh` +} diff --git a/src/auth/keyring/token-store.test.ts b/src/auth/keyring/token-store.test.ts index cfbfec5..4ceaa72 100644 --- a/src/auth/keyring/token-store.test.ts +++ b/src/auth/keyring/token-store.test.ts @@ -5,7 +5,9 @@ import { buildSingleSlot, buildUserRecords, } from '../../test-support/keyring-mocks.js' +import type { TokenBundle } from '../types.js' import { SecureStoreUnavailableError } from './secure-store.js' +import { refreshAccountSlot } from './slot-naming.js' import { type CreateKeyringTokenStoreOptions, createKeyringTokenStore } from './token-store.js' import type { UserRecord } from './types.js' @@ -79,14 +81,16 @@ describe('createKeyringTokenStore', () => { await store.set(account, 'tok_secret') expect(keyring.setSpy).toHaveBeenCalledWith('tok_secret') - expect(upsertSpy).toHaveBeenCalledWith({ account }) + expect(upsertSpy).toHaveBeenCalledWith({ account, hasRefreshToken: false }) expect(state.defaultId).toBe('42') expect(store.getLastStorageResult()).toEqual({ storage: 'secure-store' }) await expect(store.active()).resolves.toEqual({ token: 'tok_secret', account }) await store.clear() - expect(keyring.deleteSpy).toHaveBeenCalledTimes(1) + // Both slots are wiped (access + refresh). The single-slot mock + // counts every call regardless of which account string was passed. + expect(keyring.deleteSpy).toHaveBeenCalledTimes(2) expect(state.records.size).toBe(0) expect(state.defaultId).toBeNull() expect(store.getLastClearResult()).toEqual({ storage: 'secure-store' }) @@ -100,6 +104,9 @@ describe('createKeyringTokenStore', () => { await store.set(account, 'tok_plain') expect(state.records.get('42')?.fallbackToken).toBe('tok_plain') + // `set()` writes `hasRefreshToken: false` definitively, so the next + // `active()` skips the refresh-slot IPC entirely. + expect(state.records.get('42')?.hasRefreshToken).toBe(false) expect(store.getLastStorageResult()).toEqual({ storage: 'config-file', warning: @@ -155,7 +162,13 @@ describe('createKeyringTokenStore', () => { it('picks the lone user when no default is set', async () => { const keyring = buildSingleSlot({ secret: 'tok' }) - const { store } = fixture({ keyring, records: { '42': { account } } }) + // Explicit `hasRefreshToken: false` so the refresh-slot probe is + // skipped — the single-slot mock would otherwise return the same + // secret for both slots, polluting the bundle assertion below. + const { store } = fixture({ + keyring, + records: { '42': { account, hasRefreshToken: false } }, + }) await expect(store.active()).resolves.toEqual({ token: 'tok', account }) }) @@ -209,7 +222,8 @@ describe('createKeyringTokenStore', () => { await store.clear() - expect(keyring.deleteSpy).toHaveBeenCalledTimes(1) + // Both slots wiped — access + refresh. Single-slot mock counts both. + expect(keyring.deleteSpy).toHaveBeenCalledTimes(2) expect(state.records.size).toBe(0) expect(store.getLastClearResult()).toEqual({ storage: 'config-file', @@ -255,16 +269,17 @@ describe('createKeyringTokenStore', () => { const keyring = buildSingleSlot({ secret: 'tok' }) const { store, state, setDefaultSpy } = fixture({ keyring, - records: { '42': { account } }, + records: { '42': { account, hasRefreshToken: false } }, defaultId: '42', }) setDefaultSpy.mockRejectedValueOnce(new Error('disk full')) await store.clear() - // Default pointer write blew up, but the keyring entry was still - // cleaned up — otherwise the credential becomes an unreachable orphan. - expect(keyring.deleteSpy).toHaveBeenCalledTimes(1) + // Default pointer write blew up, but the keyring entries were still + // cleaned up — otherwise the credentials become unreachable orphans. + // Both slots (access + refresh) are wiped. + expect(keyring.deleteSpy).toHaveBeenCalledTimes(2) expect(state.records.size).toBe(0) }) @@ -409,4 +424,169 @@ describe('createKeyringTokenStore', () => { }) }) }) + + describe('setBundle / active() bundle round-trip', () => { + function mapFixture( + records: Record> = {}, + defaultId: string | null = null, + ) { + const km = buildKeyringMap() + mockedCreateSecureStore.mockImplementation(km.create) + const harness = buildUserRecords() + for (const [id, rec] of Object.entries(records)) { + harness.state.records.set(id, rec) + } + harness.state.defaultId = defaultId + const store = createKeyringTokenStore({ + serviceName: SERVICE, + userRecords: harness.store, + recordsLocation: LOCATION, + }) + return { km, store, state: harness.state, upsertSpy: harness.upsertSpy } + } + + const bundle: TokenBundle = { + accessToken: 'tok_a', + refreshToken: 'tok_r', + accessTokenExpiresAt: 1_700_000_000_000, + } + + it('round-trips set → active with the full bundle', async () => { + const { km, store, state } = mapFixture() + + await store.setBundle(account, bundle, { promoteDefault: true }) + + // Both slots written + expect(km.slots.get('user-42')?.secret).toBe('tok_a') + expect(km.slots.get(refreshAccountSlot('user-42'))?.secret).toBe('tok_r') + // Record carries the gate + expiry, no plaintext fallbacks + const record = state.records.get('42') + expect(record?.hasRefreshToken).toBe(true) + expect(record?.accessTokenExpiresAt).toBe(1_700_000_000_000) + expect(record?.fallbackToken).toBeUndefined() + expect(record?.fallbackRefreshToken).toBeUndefined() + expect(state.defaultId).toBe('42') + + // Active resolves both tokens + const active = await store.active() + expect(active?.token).toBe('tok_a') + expect(active?.bundle).toEqual({ + accessToken: 'tok_a', + refreshToken: 'tok_r', + accessTokenExpiresAt: 1_700_000_000_000, + }) + }) + + it('omits the bundle field on active() when nothing refresh-related is stored', async () => { + // A plain `set()` writes no refresh/expiry state — `active()` + // must surface the same shape it used to (no bundle) so legacy + // callers don't observe an empty-but-present bundle. + const { store } = mapFixture() + await store.set(account, 'tok_a') + + const active = await store.active() + expect(active).toEqual({ token: 'tok_a', account }) + expect(active?.bundle).toBeUndefined() + }) + + it('omits promoteDefault by default (silent-refresh path does not re-pin)', async () => { + // Initial record exists, no default pinned. + const { store, state } = mapFixture({ + '42': { account, hasRefreshToken: false }, + }) + expect(state.defaultId).toBeNull() + + await store.setBundle(account, bundle) + + expect(state.defaultId).toBeNull() + }) + + it('skips the refresh-slot IPC when hasRefreshToken is false', async () => { + const { km, store } = mapFixture({ + '42': { account, hasRefreshToken: false }, + }) + // Seed the access slot so active() resolves the token. + km.slots.set('user-42', { secret: 'tok_a' }) + // Seed an orphan refresh slot — verify it is NOT consulted. + km.slots.set(refreshAccountSlot('user-42'), { secret: 'orphan_r' }) + + const active = await store.active() + expect(active?.token).toBe('tok_a') + expect(active?.bundle).toBeUndefined() + // The refresh slot must not have been read. + expect(km.deleteCalls.has(refreshAccountSlot('user-42'))).toBe(false) + // Sanity check: orphan is still there because we didn't clear it. + expect(km.slots.get(refreshAccountSlot('user-42'))?.secret).toBe('orphan_r') + }) + + it('probes the refresh slot when hasRefreshToken is undefined (legacy record)', async () => { + const { km, store, state } = mapFixture({ + // Legacy record: pre-PR1 code never wrote hasRefreshToken. + '42': { account }, + }) + km.slots.set('user-42', { secret: 'tok_a' }) + km.slots.set(refreshAccountSlot('user-42'), { secret: 'tok_r' }) + + const active = await store.active() + expect(active?.bundle?.refreshToken).toBe('tok_r') + // The record had no authority, so no backfill expected on this + // path (refresh was found). + expect(state.records.get('42')?.hasRefreshToken).toBeUndefined() + }) + + it('backfills hasRefreshToken:false when an undefined gate probes empty', async () => { + const { km, store, state } = mapFixture({ '42': { account } }) + km.slots.set('user-42', { secret: 'tok_a' }) + + await store.active() + // Backfill is fire-and-forget; let the microtask queue drain. + await new Promise((resolve) => setImmediate(resolve)) + + expect(state.records.get('42')?.hasRefreshToken).toBe(false) + }) + + it.each([ + // Refresh-slot offline downgrades to "no refresh available"; + // the helper that consumes this re-prompts for login. + { + label: 'SecureStoreUnavailableError → downgrade to no-refresh', + err: new SecureStoreUnavailableError('locked'), + expect: 'downgrade' as const, + }, + { + label: 'non-keyring error → AUTH_STORE_READ_FAILED', + err: new Error('disk fried'), + expect: 'throw' as const, + }, + ])('refresh-slot read: $label', async ({ err, expect: outcome }) => { + const { km, store } = mapFixture({ '42': { account, hasRefreshToken: true } }) + km.slots.set('user-42', { secret: 'tok_a' }) + km.slots.set(refreshAccountSlot('user-42'), { secret: 'tok_r', getErr: err }) + + if (outcome === 'downgrade') { + const active = await store.active() + expect(active?.token).toBe('tok_a') + expect(active?.bundle?.refreshToken).toBeUndefined() + } else { + await expect(store.active()).rejects.toMatchObject({ + code: 'AUTH_STORE_READ_FAILED', + }) + } + }) + + it('clear() wipes both keyring slots', async () => { + const { km, store, state } = mapFixture({ + '42': { account, hasRefreshToken: true }, + }) + km.slots.set('user-42', { secret: 'tok_a' }) + km.slots.set(refreshAccountSlot('user-42'), { secret: 'tok_r' }) + state.defaultId = '42' + + await store.clear() + + expect(state.records.size).toBe(0) + expect(km.slots.get('user-42')?.secret).toBeNull() + expect(km.slots.get(refreshAccountSlot('user-42'))?.secret).toBeNull() + }) + }) }) diff --git a/src/auth/keyring/token-store.ts b/src/auth/keyring/token-store.ts index 3a67895..6e0831e 100644 --- a/src/auth/keyring/token-store.ts +++ b/src/auth/keyring/token-store.ts @@ -1,7 +1,7 @@ import { CliError } from '../../errors.js' -import type { AccountRef, AuthAccount, TokenStore } from '../types.js' +import type { AccountRef, AuthAccount, TokenBundle, TokenStore } from '../types.js' import { accountNotFoundError } from '../user-flag.js' -import { writeRecordWithKeyringFallback } from './record-write.js' +import { writeBundleWithKeyringFallback, writeRecordWithKeyringFallback } from './record-write.js' import { createSecureStore, DEFAULT_ACCOUNT_FOR_USER, @@ -9,6 +9,7 @@ import { SecureStoreUnavailableError, type SecureStore, } from './secure-store.js' +import { refreshAccountSlot } from './slot-naming.js' import type { TokenStorageResult, UserRecord, UserRecordStore } from './types.js' export type CreateKeyringTokenStoreOptions = { @@ -36,7 +37,17 @@ export type CreateKeyringTokenStoreOptions = { } export type KeyringTokenStore = TokenStore & { - /** Storage result from the most recent `set()` call, or `undefined` before any (and reset to `undefined` when the most recent `set()` threw). */ + /** + * Override `setBundle` as required (not optional) — the keyring store + * always knows how to persist refresh state. Lets cli-core helpers + * (`persistBundle`) call it without a non-null assertion. + */ + setBundle( + account: TAccount, + bundle: TokenBundle, + options?: { promoteDefault?: boolean }, + ): Promise + /** Storage result from the most recent `set()` / `setBundle()` call, or `undefined` before any (and reset to `undefined` when the most recent write threw). */ getLastStorageResult(): TokenStorageResult | undefined /** Storage result from the most recent `clear()` call, or `undefined` before any (and reset to `undefined` when the most recent `clear()` threw or was a no-op). */ getLastClearResult(): TokenStorageResult | undefined @@ -86,6 +97,13 @@ export function createKeyringTokenStore( return createSecureStore({ serviceName, account: accountForUser(account.id) }) } + function refreshSecureStoreFor(account: TAccount): SecureStore { + return createSecureStore({ + serviceName, + account: refreshAccountSlot(accountForUser(account.id)), + }) + } + type Snapshot = { records: UserRecord[]; defaultId: string | null } /** @@ -147,6 +165,96 @@ export function createKeyringTokenStore( } } + /** + * Resolve the access token for a record. Returns `null` when the record + * exists but the keyring slot is empty (corruption surfaced by the caller + * as `AUTH_STORE_READ_FAILED`). + */ + async function readAccessToken(record: UserRecord): Promise { + const fallback = record.fallbackToken?.trim() + if (fallback) return fallback + try { + const raw = await secureStoreFor(record.account).getSecret() + return raw?.trim() || null + } catch (error) { + if (error instanceof SecureStoreUnavailableError) { + throw new CliError( + 'AUTH_STORE_READ_FAILED', + `${SECURE_STORE_DESCRIPTION} unavailable; could not read stored token (${error.message})`, + ) + } + throw error + } + } + + /** + * Resolve the refresh token for a record, honouring the `hasRefreshToken` + * gate. Returns `undefined` when no refresh token is stored. Downgrades + * `SecureStoreUnavailableError` on the refresh slot to "no refresh" + * (silent-refresh callers will re-prompt for login when the helper + * eventually fails). Any other read error propagates as + * `AUTH_STORE_READ_FAILED` — corruption shouldn't masquerade as "no + * refresh available". + */ + async function readRefreshToken(record: UserRecord): Promise { + if (record.hasRefreshToken === false) return undefined + + const fallback = record.fallbackRefreshToken?.trim() + if (fallback) return fallback + + let raw: string | null + try { + raw = await refreshSecureStoreFor(record.account).getSecret() + } catch (error) { + if (error instanceof SecureStoreUnavailableError) return undefined + throw new CliError( + 'AUTH_STORE_READ_FAILED', + `${SECURE_STORE_DESCRIPTION} read failed for refresh slot (${error instanceof Error ? error.message : String(error)})`, + ) + } + + const token = raw?.trim() + if (token) return token + + // Legacy / migrated record (`hasRefreshToken === undefined`) probed + // the slot once and came back empty — backfill `false` so the next + // command skips the IPC. Best-effort; a concurrent setBundle is + // vanishingly unlikely (silent refresh requires a stored refresh + // token in the first place). + if (record.hasRefreshToken === undefined) { + void backfillNoRefresh(record).catch(() => undefined) + } + return undefined + } + + async function backfillNoRefresh(record: UserRecord): Promise { + await userRecords.upsert({ ...record, hasRefreshToken: false }) + } + + function bundleFromRecord( + record: UserRecord, + accessToken: string, + refreshToken: string | undefined, + ): TokenBundle | undefined { + if ( + refreshToken === undefined && + record.accessTokenExpiresAt === undefined && + record.refreshTokenExpiresAt === undefined + ) { + return undefined + } + return { + accessToken, + ...(refreshToken !== undefined ? { refreshToken } : {}), + ...(record.accessTokenExpiresAt !== undefined + ? { accessTokenExpiresAt: record.accessTokenExpiresAt } + : {}), + ...(record.refreshTokenExpiresAt !== undefined + ? { refreshTokenExpiresAt: record.refreshTokenExpiresAt } + : {}), + } + } + return { async active(ref) { // Ref-only path skips `getDefaultId()` — `resolveTarget` never @@ -159,46 +267,34 @@ export function createKeyringTokenStore( const record = resolveTarget(snapshot, ref) if (!record) return null - const fallback = record.fallbackToken?.trim() - if (fallback) { - return { token: fallback, account: record.account } - } - - let raw: string | null - try { - raw = await secureStoreFor(record.account).getSecret() - } catch (error) { - // A matching record exists but the keyring can't be read. - // Surface a typed failure instead of returning `null`, which - // would otherwise be indistinguishable from "no stored - // account" and trigger `ACCOUNT_NOT_FOUND` on `--user `. - // `attachLogoutCommand` catches this specific code so an - // explicit `logout --user ` can still clear the matching - // record without needing the unreadable token. - if (error instanceof SecureStoreUnavailableError) { - throw new CliError( - 'AUTH_STORE_READ_FAILED', - `${SECURE_STORE_DESCRIPTION} unavailable; could not read stored token (${error.message})`, - ) - } - throw error - } + // Read access + refresh slots in parallel — they're independent + // IPC round-trips and serial reads doubled `active()` latency. + // Access read drives the error path; refresh-read errors are + // already collapsed (SecureStoreUnavailable → undefined) inside + // `readRefreshToken`, so the only thing that throws here is the + // access read or a non-keyring refresh-read failure. + const [accessToken, refreshToken] = await Promise.all([ + readAccessToken(record), + readRefreshToken(record), + ]) - const token = raw?.trim() - if (token) { - return { token, account: record.account } + if (!accessToken) { + // Record exists, no `fallbackToken`, and the keyring slot is + // empty — the credential was deleted out-of-band (user ran + // `security delete-generic-password`, `secret-tool clear`, + // …). This is corrupted state, not a miss; collapsing it to + // `null` would make `--user ` surface as + // `ACCOUNT_NOT_FOUND` and hide the real problem. + throw new CliError( + 'AUTH_STORE_READ_FAILED', + `${SECURE_STORE_DESCRIPTION} returned no credential for the stored account; the keyring entry may have been removed externally.`, + ) } - // Record exists, no `fallbackToken`, and the keyring slot is - // empty — the credential was deleted out-of-band (user ran - // `security delete-generic-password`, `secret-tool clear`, …). - // This is corrupted state, not a miss; collapsing it to `null` - // would make `--user ` surface as `ACCOUNT_NOT_FOUND` and - // hide the real problem. - throw new CliError( - 'AUTH_STORE_READ_FAILED', - `${SECURE_STORE_DESCRIPTION} returned no credential for the stored account; the keyring entry may have been removed externally.`, - ) + const bundle = bundleFromRecord(record, accessToken, refreshToken) + return bundle + ? { token: accessToken, account: record.account, bundle } + : { token: accessToken, account: record.account } }, async set(account, token) { @@ -231,6 +327,46 @@ export function createKeyringTokenStore( : fallbackResult('token saved as plaintext in') }, + async setBundle(account, bundle, options) { + lastStorageResult = undefined + + const { accessStoredSecurely, refreshStoredSecurely } = + await writeBundleWithKeyringFallback({ + accessStore: secureStoreFor(account), + refreshStore: refreshSecureStoreFor(account), + userRecords, + account, + bundle, + }) + + // Default promotion is opt-in for `setBundle` — silent refresh + // paths omit `promoteDefault` so a background rotation can't + // re-pin account selection. Login passes `promoteDefault: true` + // to keep the first-account-wins behaviour `set()` has today. + if (options?.promoteDefault) { + try { + const existingDefault = await userRecords.getDefaultId() + if (!existingDefault) { + await userRecords.setDefaultId(account.id) + } + } catch { + // best-effort + } + } + + // Storage result reflects the access slot since the access token + // is the primary user-visible credential; a refresh-slot + // fallback alone doesn't downgrade the warning text (we don't + // surface separate refresh-state warnings today). When the + // refresh slot fell back but access landed in the keyring, the + // record carries `fallbackRefreshToken` and the next read picks + // it up transparently. + const fellBack = !accessStoredSecurely || refreshStoredSecurely === false + lastStorageResult = fellBack + ? fallbackResult('token saved as plaintext in') + : { storage: 'secure-store' } + }, + async clear(ref) { // Reset up front for the same reason as `set` — and so a no-op // (no matching record) clears any stale result from a previous @@ -259,21 +395,29 @@ export function createKeyringTokenStore( const fallbackClear = fallbackResult('local auth state cleared in') - // Always attempt the keyring delete. Even when the record carried - // a `fallbackToken`, an older keyring entry may still be parked - // there from a prior keyring-online write that was later replaced - // by an offline-fallback write — skipping the delete would leak - // that orphan. Downgrade *any* failure to a warning: the record - // is already gone, so re-throwing would corrupt local state - // (caller sees an exception and assumes nothing was cleared, - // even though the next `account list` will show the user gone). - try { - await secureStoreFor(record.account).deleteSecret() - lastClearResult = - record.fallbackToken !== undefined ? fallbackClear : { storage: 'secure-store' } - } catch { - lastClearResult = fallbackClear - } + // Always attempt both keyring deletes. Even when the record + // carried a `fallbackToken`, an older keyring entry may still be + // parked there from a prior keyring-online write that was later + // replaced by an offline-fallback write — skipping the delete + // would leak that orphan. Likewise, the refresh slot is wiped + // unconditionally so a previous bundle's refresh token can't + // resurface for a different user (slot names hash on the user + // id, but defensive-in-depth: a corrupted local state shouldn't + // leave any cleartext refresh material behind). Downgrade *any* + // failure to a warning: the record is already gone, so + // re-throwing would corrupt local state (caller sees an + // exception and assumes nothing was cleared, even though the + // next `account list` will show the user gone). + const [accessOutcome, refreshOutcome] = await Promise.allSettled([ + secureStoreFor(record.account).deleteSecret(), + refreshSecureStoreFor(record.account).deleteSecret(), + ]) + const fellBack = + accessOutcome.status === 'rejected' || + refreshOutcome.status === 'rejected' || + record.fallbackToken !== undefined || + record.fallbackRefreshToken !== undefined + lastClearResult = fellBack ? fallbackClear : { storage: 'secure-store' } }, async list() { diff --git a/src/auth/keyring/types.ts b/src/auth/keyring/types.ts index c873dcc..7b8b759 100644 --- a/src/auth/keyring/types.ts +++ b/src/auth/keyring/types.ts @@ -25,6 +25,32 @@ export type UserRecord = { * that would otherwise live in the OS credential manager. */ fallbackToken?: string + /** + * Plaintext refresh-token fallback. Same lifecycle and security profile + * as `fallbackToken`, but for the refresh slot. Present only when the + * keyring write to the refresh slot failed; cleared on every successful + * keyring-backed refresh-slot write. + */ + fallbackRefreshToken?: string + /** Access-token expiry, unix-epoch ms. */ + accessTokenExpiresAt?: number + /** Refresh-token expiry, unix-epoch ms. */ + refreshTokenExpiresAt?: number + /** + * Hot-path gate: avoids a second keyring IPC on every authenticated + * command. Tri-state interpretation by `KeyringTokenStore.active`: + * + * - `true` → a refresh token is stored; read the refresh slot. + * - `false` → no refresh token; skip the slot entirely. + * - `undefined` → legacy/migrated record with no authority over refresh + * state; probe the slot once and backfill `false` if it comes back + * empty. + * + * Always set explicitly on records written by post-bundle code; left + * `undefined` only on records produced by older code paths or by + * migration. + */ + hasRefreshToken?: boolean } /** @@ -43,6 +69,14 @@ export type UserRecordStore = { * `fallbackToken` over the keyring). Records are keyed by `account.id`. */ upsert(record: UserRecord): Promise + /** + * Optional atomic insert. When implemented, returns `true` if the record + * was written or `false` if a record with the same `account.id` already + * existed (no write). Migration prefers this over list-then-upsert to + * eliminate the TOCTOU race between existence check and write. Stores + * that can't offer atomicity may omit it; callers must fall back. + */ + tryInsert?(record: UserRecord): Promise /** Remove the record whose `account.id` matches. */ remove(id: string): Promise /** The pinned default's `account.id`, or `null` when nothing is pinned. */ diff --git a/src/auth/persist.test.ts b/src/auth/persist.test.ts new file mode 100644 index 0000000..8144a3c --- /dev/null +++ b/src/auth/persist.test.ts @@ -0,0 +1,82 @@ +import { describe, expect, it, vi } from 'vitest' + +import { CliError } from '../errors.js' +import { persistBundle } from './persist.js' +import type { AuthAccount, TokenBundle, TokenStore } from './types.js' + +type Account = AuthAccount & { id: string; email: string } + +const account: Account = { id: '42', email: 'a@b.c' } +const bundle: TokenBundle = { + accessToken: 'tok_access', + refreshToken: 'tok_refresh', + accessTokenExpiresAt: 1_700_000_000_000, +} + +function fakeStore(overrides: Partial> = {}): TokenStore { + return { + active: vi.fn(async () => null), + set: vi.fn(async () => undefined), + clear: vi.fn(async () => undefined), + list: vi.fn(async () => []), + setDefault: vi.fn(async () => undefined), + ...overrides, + } +} + +describe('persistBundle', () => { + it('prefers setBundle when the store implements it', async () => { + const setBundle = vi.fn(async () => undefined) + const set = vi.fn(async () => undefined) + const store = fakeStore({ setBundle, set }) + + await persistBundle({ store, account, bundle, promoteDefault: true }) + + expect(setBundle).toHaveBeenCalledWith(account, bundle, { promoteDefault: true }) + expect(set).not.toHaveBeenCalled() + }) + + it('falls back to set(accessToken) when the store does not implement setBundle', async () => { + const set = vi.fn(async () => undefined) + const store = fakeStore({ set }) + + await persistBundle({ store, account, bundle, promoteDefault: true }) + + expect(set).toHaveBeenCalledWith(account, 'tok_access') + }) + + it('omits the options argument entirely when promoteDefault is unset', async () => { + const setBundle = vi.fn(async () => undefined) + const store = fakeStore({ setBundle }) + + await persistBundle({ store, account, bundle }) + + // Presence-based handling: callers must be able to distinguish + // "default behaviour" from explicit opt-out via arg count. + expect(setBundle).toHaveBeenCalledTimes(1) + expect(setBundle.mock.calls[0]).toEqual([account, bundle]) + }) + + it('rethrows CliError without wrapping', async () => { + const cause = new CliError('AUTH_STORE_WRITE_FAILED', 'boom') + const store = fakeStore({ + setBundle: vi.fn(async () => { + throw cause + }), + }) + + await expect(persistBundle({ store, account, bundle })).rejects.toBe(cause) + }) + + it('wraps non-CliError failures as AUTH_STORE_WRITE_FAILED', async () => { + const store = fakeStore({ + setBundle: vi.fn(async () => { + throw new Error('disk full') + }), + }) + + await expect(persistBundle({ store, account, bundle })).rejects.toMatchObject({ + code: 'AUTH_STORE_WRITE_FAILED', + }) + }) +}) diff --git a/src/auth/persist.ts b/src/auth/persist.ts new file mode 100644 index 0000000..d8ae221 --- /dev/null +++ b/src/auth/persist.ts @@ -0,0 +1,51 @@ +import { CliError, getErrorMessage } from '../errors.js' +import type { AuthAccount, TokenBundle, TokenStore } from './types.js' + +export type PersistBundleOptions = { + store: TokenStore + account: TAccount + bundle: TokenBundle + /** + * Forwarded to `TokenStore.setBundle` when present. First-login persistence + * sets `true`; silent refresh omits the flag so a background rotation + * can't mutate account selection. + */ + promoteDefault?: boolean +} + +/** + * Persist a `TokenBundle` against any `TokenStore`, regardless of whether the + * store opted into the optional `setBundle` method. Stores that implement + * `setBundle` receive the full bundle (refresh token + expiries). Stores that + * don't are handed `bundle.accessToken` via the legacy `set(account, token)` + * path — refresh state is silently dropped because the store can't hold it. + * + * Throws `AUTH_STORE_WRITE_FAILED` on non-`CliError` failures so callers get + * one uniform error code regardless of which path the store used. + */ +export async function persistBundle( + options: PersistBundleOptions, +): Promise { + const { store, account, bundle, promoteDefault } = options + try { + if (store.setBundle) { + // Forward `promoteDefault` only when the caller actually set it. + // Some store implementations may distinguish "default omitted" + // (preserve current default behaviour) from + // "default: false" (explicit opt-out) via argument presence. + if (promoteDefault === undefined) { + await store.setBundle(account, bundle) + } else { + await store.setBundle(account, bundle, { promoteDefault }) + } + } else { + await store.set(account, bundle.accessToken) + } + } catch (error) { + if (error instanceof CliError) throw error + throw new CliError( + 'AUTH_STORE_WRITE_FAILED', + `Failed to persist token: ${getErrorMessage(error)}`, + ) + } +} diff --git a/src/auth/types.ts b/src/auth/types.ts index 4949389..9704beb 100644 --- a/src/auth/types.ts +++ b/src/auth/types.ts @@ -46,8 +46,13 @@ export type ExchangeInput = { export type ExchangeResult = { accessToken: string refreshToken?: string - /** Unix-epoch ms. cli-core does not refresh today. */ + /** Access-token expiry, unix-epoch ms. */ expiresAt?: number + /** + * Refresh-token expiry, unix-epoch ms. Most servers don't return one; + * the refresh helper carries the previous value forward when set. + */ + refreshTokenExpiresAt?: number /** Set when the token endpoint already identifies the account; skips `validateToken`. */ account?: TAccount } @@ -58,6 +63,16 @@ export type ValidateInput = { handshake: Record } +/** + * Input to `AuthProvider.refreshToken`. Symmetric with `ExchangeInput` so + * providers can share helpers; `handshake` is empty unless a future + * orchestrator threads state through. + */ +export type RefreshInput = { + refreshToken: string + handshake: Record +} + /** * Strategy interface every auth method implements. cli-core ships * `createPkceProvider` for the standard public-client PKCE flow; bespoke @@ -70,6 +85,25 @@ export type AuthProvider = { exchangeCode(input: ExchangeInput): Promise> /** Skipped when `exchangeCode` already returned an `account`. */ validateToken(input: ValidateInput): Promise + /** + * Optional: exchange a refresh token for a fresh bundle. Implemented by + * providers that target servers issuing refresh tokens (e.g. Outline). + * `refreshAccessToken` throws `AUTH_REFRESH_UNAVAILABLE` when this is + * absent or when the stored bundle carries no refresh token. + */ + refreshToken?(input: RefreshInput): Promise> +} + +/** + * The shape `KeyringTokenStore` persists per account. Custom `TokenStore` + * implementations that opt into refresh-token support return this from + * `active()` and accept it on `setBundle`. All time fields are unix-epoch ms. + */ +export type TokenBundle = { + accessToken: string + refreshToken?: string + accessTokenExpiresAt?: number + refreshTokenExpiresAt?: number } /** Opaque account selector. Stores own the matching rule (id, email, label, …). */ @@ -90,9 +124,27 @@ export type TokenStore = { * explicit-ref path and proceeds with `clear(ref)`; `attachStatusCommand` * and `attachTokenViewCommand` propagate it. */ - active(ref?: AccountRef): Promise<{ token: string; account: TAccount } | null> + active( + ref?: AccountRef, + ): Promise<{ token: string; account: TAccount; bundle?: TokenBundle } | null> /** Persist `token` for `account`, replacing any previous entry. Throw `CliError` for typed failures; other thrown values become `AUTH_STORE_WRITE_FAILED`. */ set(account: TAccount, token: string): Promise + /** + * Persist a full bundle (access + optional refresh + optional expiries). + * Optional on the contract — custom consumer stores that never carry + * refresh tokens can leave this unimplemented and cli-core helpers fall + * back to `set(account, bundle.accessToken)`. `KeyringTokenStore` + * implements it as a required override. + * + * `promoteDefault` is true on first-login persistence (initial account + * gets pinned as default) and omitted on silent refresh paths so a + * background rotation can't mutate account selection. + */ + setBundle?( + account: TAccount, + bundle: TokenBundle, + options?: { promoteDefault?: boolean }, + ): Promise /** Remove a stored credential. No-op when `ref` doesn't match. */ clear(ref?: AccountRef): Promise /** Every stored account with a default marker. */ From 0a5919f5a46a47f5236b59bff5d67a9916acf7e2 Mon Sep 17 00:00:00 2001 From: Scott Lovegrove Date: Tue, 19 May 2026 11:16:21 +0100 Subject: [PATCH 2/5] chore(auth): trim JSDoc bloat from PR1 No behaviour change. Removes JSDoc that restated the code, speculated about future PRs, or over-explained obvious branches. Net -93 LOC. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/auth/keyring/record-write.ts | 34 ++++++----------- src/auth/keyring/slot-naming.ts | 9 +---- src/auth/keyring/token-store.ts | 63 ++++++++------------------------ src/auth/keyring/types.ts | 31 ++++------------ src/auth/persist.ts | 24 ++++-------- src/auth/types.ts | 38 +++++-------------- 6 files changed, 53 insertions(+), 146 deletions(-) diff --git a/src/auth/keyring/record-write.ts b/src/auth/keyring/record-write.ts index a835114..0b85cc1 100644 --- a/src/auth/keyring/record-write.ts +++ b/src/auth/keyring/record-write.ts @@ -67,10 +67,8 @@ export async function writeRecordWithKeyringFallback = storedSecurely ? { account, hasRefreshToken: false } : { account, fallbackToken: trimmed, hasRefreshToken: false } @@ -92,25 +90,15 @@ export async function writeRecordWithKeyringFallback( options: WriteBundleOptions, diff --git a/src/auth/keyring/slot-naming.ts b/src/auth/keyring/slot-naming.ts index c8f96b0..ebae7f2 100644 --- a/src/auth/keyring/slot-naming.ts +++ b/src/auth/keyring/slot-naming.ts @@ -1,11 +1,6 @@ /** - * Single source of truth for keyring slot suffixes derived from a base - * `accountForUser(id)` slug. Importing this helper instead of inlining the - * string ensures the runtime read path and any future rename agree byte for - * byte on the wire format — a stale literal in one place would silently - * park tokens in a slot the runtime never reads from. - * - * Internal: not re-exported from `auth/keyring/index.ts`. + * Derives the refresh slot name from the access slug. Single-sourced so the + * write and read paths can't drift onto different suffixes. Internal. */ export function refreshAccountSlot(accountSlug: string): string { return `${accountSlug}/refresh` diff --git a/src/auth/keyring/token-store.ts b/src/auth/keyring/token-store.ts index 6e0831e..fc75182 100644 --- a/src/auth/keyring/token-store.ts +++ b/src/auth/keyring/token-store.ts @@ -165,11 +165,7 @@ export function createKeyringTokenStore( } } - /** - * Resolve the access token for a record. Returns `null` when the record - * exists but the keyring slot is empty (corruption surfaced by the caller - * as `AUTH_STORE_READ_FAILED`). - */ + /** Returns `null` on corruption (record exists, slot empty); caller throws `AUTH_STORE_READ_FAILED`. */ async function readAccessToken(record: UserRecord): Promise { const fallback = record.fallbackToken?.trim() if (fallback) return fallback @@ -188,13 +184,10 @@ export function createKeyringTokenStore( } /** - * Resolve the refresh token for a record, honouring the `hasRefreshToken` - * gate. Returns `undefined` when no refresh token is stored. Downgrades - * `SecureStoreUnavailableError` on the refresh slot to "no refresh" - * (silent-refresh callers will re-prompt for login when the helper - * eventually fails). Any other read error propagates as - * `AUTH_STORE_READ_FAILED` — corruption shouldn't masquerade as "no - * refresh available". + * Honours the `hasRefreshToken` gate. `SecureStoreUnavailableError` + * downgrades to `undefined` (silent-refresh callers re-prompt); other + * errors propagate as `AUTH_STORE_READ_FAILED` — corruption shouldn't + * masquerade as "no refresh". */ async function readRefreshToken(record: UserRecord): Promise { if (record.hasRefreshToken === false) return undefined @@ -216,11 +209,8 @@ export function createKeyringTokenStore( const token = raw?.trim() if (token) return token - // Legacy / migrated record (`hasRefreshToken === undefined`) probed - // the slot once and came back empty — backfill `false` so the next - // command skips the IPC. Best-effort; a concurrent setBundle is - // vanishingly unlikely (silent refresh requires a stored refresh - // token in the first place). + // Legacy record probed empty: backfill so the next command skips + // the IPC. Fire-and-forget; concurrent setBundle race is fine. if (record.hasRefreshToken === undefined) { void backfillNoRefresh(record).catch(() => undefined) } @@ -267,12 +257,7 @@ export function createKeyringTokenStore( const record = resolveTarget(snapshot, ref) if (!record) return null - // Read access + refresh slots in parallel — they're independent - // IPC round-trips and serial reads doubled `active()` latency. - // Access read drives the error path; refresh-read errors are - // already collapsed (SecureStoreUnavailable → undefined) inside - // `readRefreshToken`, so the only thing that throws here is the - // access read or a non-keyring refresh-read failure. + // Parallel: serial reads doubled `active()` latency. const [accessToken, refreshToken] = await Promise.all([ readAccessToken(record), readRefreshToken(record), @@ -339,10 +324,8 @@ export function createKeyringTokenStore( bundle, }) - // Default promotion is opt-in for `setBundle` — silent refresh - // paths omit `promoteDefault` so a background rotation can't - // re-pin account selection. Login passes `promoteDefault: true` - // to keep the first-account-wins behaviour `set()` has today. + // Opt-in: silent refresh omits `promoteDefault` so it can't + // re-pin selection; login passes `true` to match `set()`. if (options?.promoteDefault) { try { const existingDefault = await userRecords.getDefaultId() @@ -354,13 +337,8 @@ export function createKeyringTokenStore( } } - // Storage result reflects the access slot since the access token - // is the primary user-visible credential; a refresh-slot - // fallback alone doesn't downgrade the warning text (we don't - // surface separate refresh-state warnings today). When the - // refresh slot fell back but access landed in the keyring, the - // record carries `fallbackRefreshToken` and the next read picks - // it up transparently. + // Either fallback warrants the warning — refresh-slot plaintext + // is just as security-relevant as access-slot plaintext. const fellBack = !accessStoredSecurely || refreshStoredSecurely === false lastStorageResult = fellBack ? fallbackResult('token saved as plaintext in') @@ -395,19 +373,10 @@ export function createKeyringTokenStore( const fallbackClear = fallbackResult('local auth state cleared in') - // Always attempt both keyring deletes. Even when the record - // carried a `fallbackToken`, an older keyring entry may still be - // parked there from a prior keyring-online write that was later - // replaced by an offline-fallback write — skipping the delete - // would leak that orphan. Likewise, the refresh slot is wiped - // unconditionally so a previous bundle's refresh token can't - // resurface for a different user (slot names hash on the user - // id, but defensive-in-depth: a corrupted local state shouldn't - // leave any cleartext refresh material behind). Downgrade *any* - // failure to a warning: the record is already gone, so - // re-throwing would corrupt local state (caller sees an - // exception and assumes nothing was cleared, even though the - // next `account list` will show the user gone). + // Always attempt both deletes — a record's `fallbackToken` + // doesn't rule out an orphan keyring entry from a prior online + // write. Failures downgrade to a warning: the record is already + // gone, re-throwing would corrupt the caller's state. const [accessOutcome, refreshOutcome] = await Promise.allSettled([ secureStoreFor(record.account).deleteSecret(), refreshSecureStoreFor(record.account).deleteSecret(), diff --git a/src/auth/keyring/types.ts b/src/auth/keyring/types.ts index 7b8b759..f64f083 100644 --- a/src/auth/keyring/types.ts +++ b/src/auth/keyring/types.ts @@ -25,30 +25,17 @@ export type UserRecord = { * that would otherwise live in the OS credential manager. */ fallbackToken?: string - /** - * Plaintext refresh-token fallback. Same lifecycle and security profile - * as `fallbackToken`, but for the refresh slot. Present only when the - * keyring write to the refresh slot failed; cleared on every successful - * keyring-backed refresh-slot write. - */ + /** Same lifecycle and security profile as `fallbackToken`, for the refresh slot. */ fallbackRefreshToken?: string /** Access-token expiry, unix-epoch ms. */ accessTokenExpiresAt?: number /** Refresh-token expiry, unix-epoch ms. */ refreshTokenExpiresAt?: number /** - * Hot-path gate: avoids a second keyring IPC on every authenticated - * command. Tri-state interpretation by `KeyringTokenStore.active`: - * - * - `true` → a refresh token is stored; read the refresh slot. - * - `false` → no refresh token; skip the slot entirely. - * - `undefined` → legacy/migrated record with no authority over refresh - * state; probe the slot once and backfill `false` if it comes back - * empty. - * - * Always set explicitly on records written by post-bundle code; left - * `undefined` only on records produced by older code paths or by - * migration. + * Gate on the refresh-slot keyring read in `active()`: + * - `true` → read the slot. + * - `false` → skip the IPC. + * - `undefined` → legacy record; probe once and backfill `false` on empty. */ hasRefreshToken?: boolean } @@ -70,11 +57,9 @@ export type UserRecordStore = { */ upsert(record: UserRecord): Promise /** - * Optional atomic insert. When implemented, returns `true` if the record - * was written or `false` if a record with the same `account.id` already - * existed (no write). Migration prefers this over list-then-upsert to - * eliminate the TOCTOU race between existence check and write. Stores - * that can't offer atomicity may omit it; callers must fall back. + * Optional atomic insert. Returns `true` on write, `false` if `account.id` + * already exists. Migration prefers it to eliminate the existence-check + * TOCTOU race; callers fall back to list-then-upsert when absent. */ tryInsert?(record: UserRecord): Promise /** Remove the record whose `account.id` matches. */ diff --git a/src/auth/persist.ts b/src/auth/persist.ts index d8ae221..efda65a 100644 --- a/src/auth/persist.ts +++ b/src/auth/persist.ts @@ -5,23 +5,15 @@ export type PersistBundleOptions = { store: TokenStore account: TAccount bundle: TokenBundle - /** - * Forwarded to `TokenStore.setBundle` when present. First-login persistence - * sets `true`; silent refresh omits the flag so a background rotation - * can't mutate account selection. - */ + /** Forwarded to `setBundle` when present. See `TokenStore.setBundle`. */ promoteDefault?: boolean } /** - * Persist a `TokenBundle` against any `TokenStore`, regardless of whether the - * store opted into the optional `setBundle` method. Stores that implement - * `setBundle` receive the full bundle (refresh token + expiries). Stores that - * don't are handed `bundle.accessToken` via the legacy `set(account, token)` - * path — refresh state is silently dropped because the store can't hold it. - * - * Throws `AUTH_STORE_WRITE_FAILED` on non-`CliError` failures so callers get - * one uniform error code regardless of which path the store used. + * Persist a bundle against any `TokenStore`. Prefers `setBundle` when the + * store implements it; otherwise falls back to `set(account, accessToken)` + * and silently drops refresh state. Wraps non-`CliError` failures as + * `AUTH_STORE_WRITE_FAILED`. */ export async function persistBundle( options: PersistBundleOptions, @@ -29,10 +21,8 @@ export async function persistBundle( const { store, account, bundle, promoteDefault } = options try { if (store.setBundle) { - // Forward `promoteDefault` only when the caller actually set it. - // Some store implementations may distinguish "default omitted" - // (preserve current default behaviour) from - // "default: false" (explicit opt-out) via argument presence. + // Omit the options arg entirely when unset so presence-based + // handlers can distinguish "default" from explicit opt-out. if (promoteDefault === undefined) { await store.setBundle(account, bundle) } else { diff --git a/src/auth/types.ts b/src/auth/types.ts index 9704beb..43e8f98 100644 --- a/src/auth/types.ts +++ b/src/auth/types.ts @@ -48,10 +48,7 @@ export type ExchangeResult = { refreshToken?: string /** Access-token expiry, unix-epoch ms. */ expiresAt?: number - /** - * Refresh-token expiry, unix-epoch ms. Most servers don't return one; - * the refresh helper carries the previous value forward when set. - */ + /** Refresh-token expiry, unix-epoch ms. */ refreshTokenExpiresAt?: number /** Set when the token endpoint already identifies the account; skips `validateToken`. */ account?: TAccount @@ -63,13 +60,9 @@ export type ValidateInput = { handshake: Record } -/** - * Input to `AuthProvider.refreshToken`. Symmetric with `ExchangeInput` so - * providers can share helpers; `handshake` is empty unless a future - * orchestrator threads state through. - */ export type RefreshInput = { refreshToken: string + /** Same shape as `ExchangeInput.handshake` — empty when called outside `runOAuthFlow`. */ handshake: Record } @@ -85,20 +78,11 @@ export type AuthProvider = { exchangeCode(input: ExchangeInput): Promise> /** Skipped when `exchangeCode` already returned an `account`. */ validateToken(input: ValidateInput): Promise - /** - * Optional: exchange a refresh token for a fresh bundle. Implemented by - * providers that target servers issuing refresh tokens (e.g. Outline). - * `refreshAccessToken` throws `AUTH_REFRESH_UNAVAILABLE` when this is - * absent or when the stored bundle carries no refresh token. - */ + /** Optional: exchange a refresh token for a fresh bundle. */ refreshToken?(input: RefreshInput): Promise> } -/** - * The shape `KeyringTokenStore` persists per account. Custom `TokenStore` - * implementations that opt into refresh-token support return this from - * `active()` and accept it on `setBundle`. All time fields are unix-epoch ms. - */ +/** Access + optional refresh token + optional expiries (all unix-epoch ms). */ export type TokenBundle = { accessToken: string refreshToken?: string @@ -130,15 +114,11 @@ export type TokenStore = { /** Persist `token` for `account`, replacing any previous entry. Throw `CliError` for typed failures; other thrown values become `AUTH_STORE_WRITE_FAILED`. */ set(account: TAccount, token: string): Promise /** - * Persist a full bundle (access + optional refresh + optional expiries). - * Optional on the contract — custom consumer stores that never carry - * refresh tokens can leave this unimplemented and cli-core helpers fall - * back to `set(account, bundle.accessToken)`. `KeyringTokenStore` - * implements it as a required override. - * - * `promoteDefault` is true on first-login persistence (initial account - * gets pinned as default) and omitted on silent refresh paths so a - * background rotation can't mutate account selection. + * Persist a full bundle. Optional on the contract — stores that don't + * implement it get `bundle.accessToken` via `set()` instead (cli-core + * helpers handle the fallback). Pass `promoteDefault: true` on first + * login; omit on silent refresh so a background rotation can't re-pin + * account selection. */ setBundle?( account: TAccount, From 0073318ff2bb3bb091220ef2823f2d8885f74371 Mon Sep 17 00:00:00 2001 From: Scott Lovegrove Date: Tue, 19 May 2026 11:26:06 +0100 Subject: [PATCH 3/5] fix(auth): address PR review on TokenBundle storage contract MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses 13 findings from the automated review on #37. P1 — correctness - Drop the fire-and-forget legacy-record backfill. The previous implementation read a stale `UserRecord` snapshot and re-`upsert`ed it, which (per the replace-not-merge contract) would clobber a concurrent `setBundle` write. Legacy records now pay one keyring IPC per `active()` until a `setBundle` writes an explicit gate; that's an acceptable cost and removes the race. P2 — contract & correctness - Flatten `bundle?` off the snapshot returned by `active()`. Refresh state (`refreshToken?`, `accessTokenExpiresAt?`, `refreshTokenExpiresAt?`) now lives flat on the snapshot, removing the duplicate access-token field. New `ActiveTokenSnapshot` type captures the read shape. - `set(account, token)` now wipes the refresh keyring slot best-effort so a previous `setBundle` can't leave orphan refresh material behind. New optional `refreshStore` parameter on `writeRecordWithKeyringFallback`. - `set()` and `setBundle()` reject empty / whitespace-only access tokens with `AUTH_STORE_WRITE_FAILED` instead of writing an empty fallback. - `readAccessToken` wraps unexpected backend failures as `AUTH_STORE_READ_FAILED`, symmetric with `readRefreshToken`. - `setBundle`'s `lastStorageResult` is tied only to the access slot — a refresh-only fallback no longer downgrades the warning text (the existing copy says "token", which implies access; refresh-only fallback is silent today). - `hasRefreshToken: !!refreshToken` instead of `!== undefined` so empty strings (which trigger the no-refresh write path) don't set the gate. - `WriteBundleResult` demoted to a local type (no external consumer). - `persistBundle` JSDoc warns that the `set()` fallback can't honour `promoteDefault`; multi-account stores wanting silent-refresh-safe selection must implement `setBundle`. P3 — tests - New `getCalls` tracking on `buildKeyringMap`. The "skips refresh-slot IPC" test now asserts `getSecret` was not invoked instead of asserting on `deleteCalls` (which `active()` never populates). - The "picks the lone user" test switches to `buildKeyringMap`-style routing instead of the `hasRefreshToken: false` workaround. - New regression test: `set()` after `setBundle()` wipes the refresh slot. - Dropped the backfill timing-dependent test (now moot). 384 tests pass (was 383). `npm run check` / `type-check` / `test` / `build` all clean. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/auth/index.ts | 1 + src/auth/keyring/record-write.ts | 34 +++++++- src/auth/keyring/token-store.test.ts | 123 ++++++++++++++------------- src/auth/keyring/token-store.ts | 117 +++++++++++-------------- src/auth/persist.ts | 7 ++ src/auth/types.ts | 20 ++++- src/test-support/keyring-mocks.ts | 5 ++ 7 files changed, 172 insertions(+), 135 deletions(-) diff --git a/src/auth/index.ts b/src/auth/index.ts index e0595f6..fe0e128 100644 --- a/src/auth/index.ts +++ b/src/auth/index.ts @@ -26,6 +26,7 @@ export { createPkceProvider } from './providers/pkce.js' export type { PkceLazyString, PkceProviderOptions } from './providers/pkce.js' export type { AccountRef, + ActiveTokenSnapshot, AuthAccount, AuthorizeInput, AuthorizeResult, diff --git a/src/auth/keyring/record-write.ts b/src/auth/keyring/record-write.ts index 0b85cc1..c118414 100644 --- a/src/auth/keyring/record-write.ts +++ b/src/auth/keyring/record-write.ts @@ -1,3 +1,4 @@ +import { CliError } from '../../errors.js' import type { AuthAccount, TokenBundle } from '../types.js' import { type SecureStore, SecureStoreUnavailableError } from './secure-store.js' import type { UserRecord, UserRecordStore } from './types.js' @@ -5,6 +6,8 @@ import type { UserRecord, UserRecordStore } from './types.js' type WriteRecordOptions = { /** Per-account keyring slot, already configured by the caller (e.g. via `createSecureStore`). */ secureStore: SecureStore + /** Optional refresh-token keyring slot to wipe alongside the access write. */ + refreshStore?: SecureStore userRecords: UserRecordStore account: TAccount token: string @@ -25,7 +28,7 @@ type WriteBundleOptions = { bundle: TokenBundle } -export type WriteBundleResult = { +type WriteBundleResult = { /** `true` when the access token landed in the OS keyring; `false` when it fell back to `fallbackToken`. */ accessStoredSecurely: boolean /** @@ -56,8 +59,11 @@ export type WriteBundleResult = { export async function writeRecordWithKeyringFallback( options: WriteRecordOptions, ): Promise { - const { secureStore, userRecords, account, token } = options + const { secureStore, refreshStore, userRecords, account, token } = options const trimmed = token.trim() + if (!trimmed) { + throw new CliError('AUTH_STORE_WRITE_FAILED', 'Refusing to persist an empty access token.') + } let storedSecurely = false try { @@ -67,8 +73,22 @@ export async function writeRecordWithKeyringFallback = storedSecurely ? { account, hasRefreshToken: false } : { account, fallbackToken: trimmed, hasRefreshToken: false } @@ -105,6 +125,12 @@ export async function writeBundleWithKeyringFallback { const { accessStore, refreshStore, userRecords, account, bundle } = options const accessToken = bundle.accessToken.trim() + if (!accessToken) { + throw new CliError( + 'AUTH_STORE_WRITE_FAILED', + 'Refusing to persist a bundle with an empty access token.', + ) + } const refreshToken = bundle.refreshToken?.trim() let accessStoredSecurely = false @@ -156,7 +182,7 @@ export async function writeBundleWithKeyringFallback { }) it('round-trips set → active → clear when the keyring is online', async () => { - const { keyring, store, state, upsertSpy } = fixture() + // Per-slot map: set() now wipes the refresh slot too, so a + // single-slot mock would clobber the access secret. + const km = buildKeyringMap() + mockedCreateSecureStore.mockImplementation(km.create) + const harness = buildUserRecords() + const store = createKeyringTokenStore({ + serviceName: SERVICE, + userRecords: harness.store, + recordsLocation: LOCATION, + }) await store.set(account, 'tok_secret') - expect(keyring.setSpy).toHaveBeenCalledWith('tok_secret') - expect(upsertSpy).toHaveBeenCalledWith({ account, hasRefreshToken: false }) - expect(state.defaultId).toBe('42') + expect(km.slots.get('user-42')?.secret).toBe('tok_secret') + expect(harness.upsertSpy).toHaveBeenCalledWith({ account, hasRefreshToken: false }) + expect(harness.state.defaultId).toBe('42') expect(store.getLastStorageResult()).toEqual({ storage: 'secure-store' }) await expect(store.active()).resolves.toEqual({ token: 'tok_secret', account }) await store.clear() - // Both slots are wiped (access + refresh). The single-slot mock - // counts every call regardless of which account string was passed. - expect(keyring.deleteSpy).toHaveBeenCalledTimes(2) - expect(state.records.size).toBe(0) - expect(state.defaultId).toBeNull() + expect(km.slots.get('user-42')?.secret).toBeNull() + expect(km.slots.get(refreshAccountSlot('user-42'))?.secret).toBeNull() + expect(harness.state.records.size).toBe(0) + expect(harness.state.defaultId).toBeNull() expect(store.getLastClearResult()).toEqual({ storage: 'secure-store' }) }) @@ -161,13 +169,17 @@ describe('createKeyringTokenStore', () => { }) it('picks the lone user when no default is set', async () => { - const keyring = buildSingleSlot({ secret: 'tok' }) - // Explicit `hasRefreshToken: false` so the refresh-slot probe is - // skipped — the single-slot mock would otherwise return the same - // secret for both slots, polluting the bundle assertion below. - const { store } = fixture({ - keyring, - records: { '42': { account, hasRefreshToken: false } }, + // Per-slot keyring map so refresh probes (legacy record) hit an + // empty distinct slot rather than the same single-slot secret. + const km = buildKeyringMap() + mockedCreateSecureStore.mockImplementation(km.create) + const harness = buildUserRecords() + harness.state.records.set('42', { account }) + km.slots.set('user-42', { secret: 'tok' }) + const store = createKeyringTokenStore({ + serviceName: SERVICE, + userRecords: harness.store, + recordsLocation: LOCATION, }) await expect(store.active()).resolves.toEqual({ token: 'tok', account }) @@ -467,34 +479,26 @@ describe('createKeyringTokenStore', () => { expect(record?.fallbackRefreshToken).toBeUndefined() expect(state.defaultId).toBe('42') - // Active resolves both tokens - const active = await store.active() - expect(active?.token).toBe('tok_a') - expect(active?.bundle).toEqual({ - accessToken: 'tok_a', + // Active resolves both tokens — flat snapshot, no `bundle` wrapper. + await expect(store.active()).resolves.toEqual({ + token: 'tok_a', + account, refreshToken: 'tok_r', accessTokenExpiresAt: 1_700_000_000_000, }) }) - it('omits the bundle field on active() when nothing refresh-related is stored', async () => { - // A plain `set()` writes no refresh/expiry state — `active()` - // must surface the same shape it used to (no bundle) so legacy - // callers don't observe an empty-but-present bundle. + it('omits refresh-state fields on active() when nothing refresh-related is stored', async () => { const { store } = mapFixture() await store.set(account, 'tok_a') - const active = await store.active() - expect(active).toEqual({ token: 'tok_a', account }) - expect(active?.bundle).toBeUndefined() + await expect(store.active()).resolves.toEqual({ token: 'tok_a', account }) }) it('omits promoteDefault by default (silent-refresh path does not re-pin)', async () => { - // Initial record exists, no default pinned. const { store, state } = mapFixture({ '42': { account, hasRefreshToken: false }, }) - expect(state.defaultId).toBeNull() await store.setBundle(account, bundle) @@ -505,49 +509,30 @@ describe('createKeyringTokenStore', () => { const { km, store } = mapFixture({ '42': { account, hasRefreshToken: false }, }) - // Seed the access slot so active() resolves the token. km.slots.set('user-42', { secret: 'tok_a' }) // Seed an orphan refresh slot — verify it is NOT consulted. km.slots.set(refreshAccountSlot('user-42'), { secret: 'orphan_r' }) const active = await store.active() expect(active?.token).toBe('tok_a') - expect(active?.bundle).toBeUndefined() - // The refresh slot must not have been read. - expect(km.deleteCalls.has(refreshAccountSlot('user-42'))).toBe(false) - // Sanity check: orphan is still there because we didn't clear it. - expect(km.slots.get(refreshAccountSlot('user-42'))?.secret).toBe('orphan_r') + expect(active?.refreshToken).toBeUndefined() + // Hard assertion: refresh-slot getSecret was never invoked. + expect(km.getCalls.has(refreshAccountSlot('user-42'))).toBe(false) }) it('probes the refresh slot when hasRefreshToken is undefined (legacy record)', async () => { - const { km, store, state } = mapFixture({ - // Legacy record: pre-PR1 code never wrote hasRefreshToken. - '42': { account }, - }) + const { km, store, state } = mapFixture({ '42': { account } }) km.slots.set('user-42', { secret: 'tok_a' }) km.slots.set(refreshAccountSlot('user-42'), { secret: 'tok_r' }) - const active = await store.active() - expect(active?.bundle?.refreshToken).toBe('tok_r') - // The record had no authority, so no backfill expected on this - // path (refresh was found). + await expect(store.active()).resolves.toMatchObject({ refreshToken: 'tok_r' }) + // Legacy records pay the probe on every read — no race-prone + // backfill. A subsequent `setBundle` will write `hasRefreshToken` + // explicitly. expect(state.records.get('42')?.hasRefreshToken).toBeUndefined() }) - it('backfills hasRefreshToken:false when an undefined gate probes empty', async () => { - const { km, store, state } = mapFixture({ '42': { account } }) - km.slots.set('user-42', { secret: 'tok_a' }) - - await store.active() - // Backfill is fire-and-forget; let the microtask queue drain. - await new Promise((resolve) => setImmediate(resolve)) - - expect(state.records.get('42')?.hasRefreshToken).toBe(false) - }) - it.each([ - // Refresh-slot offline downgrades to "no refresh available"; - // the helper that consumes this re-prompts for login. { label: 'SecureStoreUnavailableError → downgrade to no-refresh', err: new SecureStoreUnavailableError('locked'), @@ -564,9 +549,9 @@ describe('createKeyringTokenStore', () => { km.slots.set(refreshAccountSlot('user-42'), { secret: 'tok_r', getErr: err }) if (outcome === 'downgrade') { - const active = await store.active() - expect(active?.token).toBe('tok_a') - expect(active?.bundle?.refreshToken).toBeUndefined() + const result = await store.active() + expect(result?.token).toBe('tok_a') + expect(result).not.toHaveProperty('refreshToken') } else { await expect(store.active()).rejects.toMatchObject({ code: 'AUTH_STORE_READ_FAILED', @@ -574,6 +559,24 @@ describe('createKeyringTokenStore', () => { } }) + it('set() wipes the refresh slot left behind by a prior setBundle', async () => { + // Regression: `set(account, token)` is documented as "replacing + // any previous entry". Without the wipe, a later set() would + // leave the refresh secret orphaned in the keyring even though + // the record's `hasRefreshToken: false` says it shouldn't be + // there. + const { km, store, state } = mapFixture() + + await store.setBundle(account, bundle, { promoteDefault: true }) + expect(km.slots.get(refreshAccountSlot('user-42'))?.secret).toBe('tok_r') + + await store.set(account, 'tok_a_replacement') + + expect(km.slots.get('user-42')?.secret).toBe('tok_a_replacement') + expect(km.slots.get(refreshAccountSlot('user-42'))?.secret).toBeNull() + expect(state.records.get('42')?.hasRefreshToken).toBe(false) + }) + it('clear() wipes both keyring slots', async () => { const { km, store, state } = mapFixture({ '42': { account, hasRefreshToken: true }, diff --git a/src/auth/keyring/token-store.ts b/src/auth/keyring/token-store.ts index fc75182..ac1cfca 100644 --- a/src/auth/keyring/token-store.ts +++ b/src/auth/keyring/token-store.ts @@ -1,4 +1,4 @@ -import { CliError } from '../../errors.js' +import { CliError, getErrorMessage } from '../../errors.js' import type { AccountRef, AuthAccount, TokenBundle, TokenStore } from '../types.js' import { accountNotFoundError } from '../user-flag.js' import { writeBundleWithKeyringFallback, writeRecordWithKeyringFallback } from './record-write.js' @@ -179,7 +179,13 @@ export function createKeyringTokenStore( `${SECURE_STORE_DESCRIPTION} unavailable; could not read stored token (${error.message})`, ) } - throw error + // Symmetric with `readRefreshToken`: wrap unexpected backend + // failures so the CLI sees a typed error code instead of crashing + // with the raw exception. + throw new CliError( + 'AUTH_STORE_READ_FAILED', + `Access-slot read failed (${getErrorMessage(error)})`, + ) } } @@ -188,6 +194,13 @@ export function createKeyringTokenStore( * downgrades to `undefined` (silent-refresh callers re-prompt); other * errors propagate as `AUTH_STORE_READ_FAILED` — corruption shouldn't * masquerade as "no refresh". + * + * Legacy records (`hasRefreshToken === undefined`) probe the slot on + * every read. The previous fire-and-forget backfill raced with + * concurrent `setBundle` writes (replace-not-merge `upsert` would + * clobber bundle metadata). One IPC per command is an acceptable cost + * — a single `setBundle` call rewrites the record with an explicit + * gate and the probes stop. */ async function readRefreshToken(record: UserRecord): Promise { if (record.hasRefreshToken === false) return undefined @@ -195,54 +208,16 @@ export function createKeyringTokenStore( const fallback = record.fallbackRefreshToken?.trim() if (fallback) return fallback - let raw: string | null try { - raw = await refreshSecureStoreFor(record.account).getSecret() + const raw = await refreshSecureStoreFor(record.account).getSecret() + return raw?.trim() || undefined } catch (error) { if (error instanceof SecureStoreUnavailableError) return undefined throw new CliError( 'AUTH_STORE_READ_FAILED', - `${SECURE_STORE_DESCRIPTION} read failed for refresh slot (${error instanceof Error ? error.message : String(error)})`, + `Refresh-slot read failed (${getErrorMessage(error)})`, ) } - - const token = raw?.trim() - if (token) return token - - // Legacy record probed empty: backfill so the next command skips - // the IPC. Fire-and-forget; concurrent setBundle race is fine. - if (record.hasRefreshToken === undefined) { - void backfillNoRefresh(record).catch(() => undefined) - } - return undefined - } - - async function backfillNoRefresh(record: UserRecord): Promise { - await userRecords.upsert({ ...record, hasRefreshToken: false }) - } - - function bundleFromRecord( - record: UserRecord, - accessToken: string, - refreshToken: string | undefined, - ): TokenBundle | undefined { - if ( - refreshToken === undefined && - record.accessTokenExpiresAt === undefined && - record.refreshTokenExpiresAt === undefined - ) { - return undefined - } - return { - accessToken, - ...(refreshToken !== undefined ? { refreshToken } : {}), - ...(record.accessTokenExpiresAt !== undefined - ? { accessTokenExpiresAt: record.accessTokenExpiresAt } - : {}), - ...(record.refreshTokenExpiresAt !== undefined - ? { refreshTokenExpiresAt: record.refreshTokenExpiresAt } - : {}), - } } return { @@ -264,22 +239,26 @@ export function createKeyringTokenStore( ]) if (!accessToken) { - // Record exists, no `fallbackToken`, and the keyring slot is - // empty — the credential was deleted out-of-band (user ran - // `security delete-generic-password`, `secret-tool clear`, - // …). This is corrupted state, not a miss; collapsing it to - // `null` would make `--user ` surface as - // `ACCOUNT_NOT_FOUND` and hide the real problem. + // Record exists, no `fallbackToken`, slot empty — corruption + // (deleted out-of-band). Collapsing to `null` would surface as + // ACCOUNT_NOT_FOUND on `--user ` and hide the real problem. throw new CliError( 'AUTH_STORE_READ_FAILED', `${SECURE_STORE_DESCRIPTION} returned no credential for the stored account; the keyring entry may have been removed externally.`, ) } - const bundle = bundleFromRecord(record, accessToken, refreshToken) - return bundle - ? { token: accessToken, account: record.account, bundle } - : { token: accessToken, account: record.account } + return { + token: accessToken, + account: record.account, + ...(refreshToken !== undefined ? { refreshToken } : {}), + ...(record.accessTokenExpiresAt !== undefined + ? { accessTokenExpiresAt: record.accessTokenExpiresAt } + : {}), + ...(record.refreshTokenExpiresAt !== undefined + ? { refreshTokenExpiresAt: record.refreshTokenExpiresAt } + : {}), + } }, async set(account, token) { @@ -290,6 +269,7 @@ export function createKeyringTokenStore( const { storedSecurely } = await writeRecordWithKeyringFallback({ secureStore: secureStoreFor(account), + refreshStore: refreshSecureStoreFor(account), userRecords, account, token, @@ -315,14 +295,13 @@ export function createKeyringTokenStore( async setBundle(account, bundle, options) { lastStorageResult = undefined - const { accessStoredSecurely, refreshStoredSecurely } = - await writeBundleWithKeyringFallback({ - accessStore: secureStoreFor(account), - refreshStore: refreshSecureStoreFor(account), - userRecords, - account, - bundle, - }) + const { accessStoredSecurely } = await writeBundleWithKeyringFallback({ + accessStore: secureStoreFor(account), + refreshStore: refreshSecureStoreFor(account), + userRecords, + account, + bundle, + }) // Opt-in: silent refresh omits `promoteDefault` so it can't // re-pin selection; login passes `true` to match `set()`. @@ -337,12 +316,16 @@ export function createKeyringTokenStore( } } - // Either fallback warrants the warning — refresh-slot plaintext - // is just as security-relevant as access-slot plaintext. - const fellBack = !accessStoredSecurely || refreshStoredSecurely === false - lastStorageResult = fellBack - ? fallbackResult('token saved as plaintext in') - : { storage: 'secure-store' } + // Storage result is tied to the access slot — that's the + // user-visible credential the existing warning text describes. + // A refresh-only fallback is silent today; if we ever want to + // call it out it needs distinct phrasing (the current copy says + // "token", which implies access). The record itself carries + // `fallbackRefreshToken` so the runtime read picks it up + // transparently regardless. + lastStorageResult = accessStoredSecurely + ? { storage: 'secure-store' } + : fallbackResult('token saved as plaintext in') }, async clear(ref) { diff --git a/src/auth/persist.ts b/src/auth/persist.ts index efda65a..df5f389 100644 --- a/src/auth/persist.ts +++ b/src/auth/persist.ts @@ -14,6 +14,13 @@ export type PersistBundleOptions = { * store implements it; otherwise falls back to `set(account, accessToken)` * and silently drops refresh state. Wraps non-`CliError` failures as * `AUTH_STORE_WRITE_FAILED`. + * + * `promoteDefault` is only honoured on the `setBundle` path — the base + * `set()` contract has no promotion control, so a custom multi-account + * store that opts out of `setBundle` will run its own promotion policy + * (typically first-account-wins). Multi-account stores that need + * silent-refresh-safe selection (no re-pinning on background rotation) + * MUST implement `setBundle`. */ export async function persistBundle( options: PersistBundleOptions, diff --git a/src/auth/types.ts b/src/auth/types.ts index 43e8f98..e219fe3 100644 --- a/src/auth/types.ts +++ b/src/auth/types.ts @@ -82,7 +82,7 @@ export type AuthProvider = { refreshToken?(input: RefreshInput): Promise> } -/** Access + optional refresh token + optional expiries (all unix-epoch ms). */ +/** Write-side bundle for `setBundle`. Time fields are unix-epoch ms. */ export type TokenBundle = { accessToken: string refreshToken?: string @@ -90,6 +90,20 @@ export type TokenBundle = { refreshTokenExpiresAt?: number } +/** + * Read-side snapshot returned by `TokenStore.active`. `token` is the access + * token (kept named that way for back-compat with the pre-bundle contract); + * refresh-state fields are flat siblings so consumers have a single source of + * truth per credential. + */ +export type ActiveTokenSnapshot = { + token: string + account: TAccount + refreshToken?: string + accessTokenExpiresAt?: number + refreshTokenExpiresAt?: number +} + /** Opaque account selector. Stores own the matching rule (id, email, label, …). */ export type AccountRef = string @@ -108,9 +122,7 @@ export type TokenStore = { * explicit-ref path and proceeds with `clear(ref)`; `attachStatusCommand` * and `attachTokenViewCommand` propagate it. */ - active( - ref?: AccountRef, - ): Promise<{ token: string; account: TAccount; bundle?: TokenBundle } | null> + active(ref?: AccountRef): Promise | null> /** Persist `token` for `account`, replacing any previous entry. Throw `CliError` for typed failures; other thrown values become `AUTH_STORE_WRITE_FAILED`. */ set(account: TAccount, token: string): Promise /** diff --git a/src/test-support/keyring-mocks.ts b/src/test-support/keyring-mocks.ts index 7125b31..4d34a7d 100644 --- a/src/test-support/keyring-mocks.ts +++ b/src/test-support/keyring-mocks.ts @@ -18,6 +18,8 @@ type KeyringSlot = { type KeyringMap = { create: (args: { serviceName: string; account: string }) => SecureStore slots: Map + /** Per-slot count of `getSecret()` invocations. */ + getCalls: Map deleteCalls: Map } @@ -28,6 +30,7 @@ type KeyringMap = { */ export function buildKeyringMap(): KeyringMap { const slots = new Map() + const getCalls = new Map() const deleteCalls = new Map() function getSlot(account: string): KeyringSlot { let slot = slots.get(account) @@ -39,10 +42,12 @@ export function buildKeyringMap(): KeyringMap { } return { slots, + getCalls, deleteCalls, create({ account }) { return { async getSecret() { + getCalls.set(account, (getCalls.get(account) ?? 0) + 1) const slot = getSlot(account) if (slot.getErr) throw slot.getErr return slot.secret From e9ad53d478f1d8f987376b230b705432d55551cd Mon Sep 17 00:00:00 2001 From: Scott Lovegrove Date: Tue, 19 May 2026 11:34:44 +0100 Subject: [PATCH 4/5] style(auth): Boolean(refreshToken) over double-bang MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Per Scott on #37 — codebase convention is `Boolean(x)` for boolean coercion, not `!!x`. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/auth/keyring/record-write.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/auth/keyring/record-write.ts b/src/auth/keyring/record-write.ts index c118414..f0802f7 100644 --- a/src/auth/keyring/record-write.ts +++ b/src/auth/keyring/record-write.ts @@ -182,7 +182,7 @@ export async function writeBundleWithKeyringFallback Date: Tue, 19 May 2026 11:58:33 +0100 Subject: [PATCH 5/5] fix(auth): address round 3 of PR review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 11 findings on #37, addressed in one push. P1 — correctness - Defer the refresh-slot wipe (when bundle has no refresh token) until AFTER `userRecords.upsert` succeeds. The previous order — wipe before upsert — would destroy refresh state on an upsert rollback, leaving the old record with `hasRefreshToken: undefined` and no slot to recover from. `writeRecordWithKeyringFallback` and the no-refresh branch of `writeBundleWithKeyringFallback` both now wipe post-upsert. P2 — read shape - Revert `active()` to its pre-PR1 shape `{ token, account }`. PR1 is storage-only on the contract; the read path stays narrow so common commands (status / logout / token-view) don't pay an extra keyring IPC for refresh state they never touch. Bundle-aware reads land alongside the silent-refresh helper in PR3. Drop `ActiveTokenSnapshot` and `readRefreshToken`; remove the tests that exercised reading refresh material from `active()`. The `hasRefreshToken` field stays on the record for that future read path. P2 — write helper consolidation - `writeRecordWithKeyringFallback` now delegates to `writeBundleWithKeyringFallback` with a refresh-less bundle. Trim / validate / setSecret / fallback / upsert / rollback live in one place. The optional `refreshStore` parameter is the only difference for legacy callers (migration). Empty-token guard is single-sourced. P2 — storage-result mapping - New `bundleStorageResult(accessStored, refreshStored)` composes the `lastStorageResult`. Either falsy slot downgrades to `config-file` with subject-specific text ("access token" / "refresh token" / "access + refresh tokens saved as plaintext"). Refresh-only fallback is no longer invisible. - New shared `promoteDefaultIfNeeded` helper closure; `set` and `setBundle` both call it. P2 — test hygiene - Hoist `mapFixture` to module level; "round-trips" and "picks the lone user" now use it instead of inlining the wiring. - New regression test: `writeBundleWithKeyringFallback` defers the refresh-slot wipe until after upsert (callOrder assertion). - New regression test: headless / WSL bundle path (both slots offline) persists both tokens as fallbacks on the record. - Extend the `active()` failure matrix to cover non-keyring backend errors (in addition to `SecureStoreUnavailableError`) so the new wrap-as-`AUTH_STORE_READ_FAILED` logic is pinned. P3 — comment drift - `hasRefreshToken` JSDoc no longer references the dropped backfill. 382 tests pass. `npm run check` / `type-check` / `test` / `build` clean. Co-Authored-By: Claude Opus 4.7 (1M context) --- README.md | 34 ++--- src/auth/index.ts | 1 - src/auth/keyring/record-write.test.ts | 46 +++++++ src/auth/keyring/record-write.ts | 145 ++++++++++----------- src/auth/keyring/token-store.test.ts | 171 +++++++----------------- src/auth/keyring/token-store.ts | 181 +++++++++++--------------- src/auth/keyring/types.ts | 9 +- src/auth/types.ts | 16 +-- 8 files changed, 262 insertions(+), 341 deletions(-) diff --git a/README.md b/README.md index 6f165da..f8eee3f 100644 --- a/README.md +++ b/README.md @@ -12,20 +12,20 @@ npm install @doist/cli-core ## What's in it -| Module | Key exports | Purpose | -| -------------------- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| `auth` (subpath) | `attachLoginCommand`, `attachLogoutCommand`, `attachStatusCommand`, `attachTokenViewCommand`, `runOAuthFlow`, `createPkceProvider`, `createSecureStore`, `createKeyringTokenStore`, `migrateLegacyAuth`, `persistBundle`, PKCE helpers, `AuthProvider` / `TokenStore` / `TokenBundle` / `RefreshInput` / `AccountRef` / `SecureStore` / `UserRecordStore` types, `AttachLogoutRevokeContext` | OAuth runtime plus the Commander attachers for ` [auth] login` / `logout` / `status` / `token`. `attachLogoutCommand` accepts an optional `revokeToken` hook for best-effort server-side token revocation. Ships the standard public-client PKCE flow (`createPkceProvider`), a thin cross-platform OS-keyring wrapper (`createSecureStore`), and a multi-account keyring-backed `TokenStore` (`createKeyringTokenStore`) that stores secrets in the OS credential manager and degrades to plaintext in the consumer's config when the keyring is unavailable (WSL/headless Linux/containers). The store contract supports an optional `setBundle(account, bundle)` method (required on `KeyringTokenStore`) so consumers that need refresh-token persistence can opt in via `TokenBundle`. `AuthProvider` and `TokenStore` remain the escape hatches for DCR or fully bespoke backends. `logout` / `status` / `token` always attach `--user ` and thread the parsed ref to `store.active(ref)` (and `store.clear(ref)` on `logout`). `commander` (when using the attachers), `open` (browser launch), and `@napi-rs/keyring` (when using `createSecureStore` or the keyring `TokenStore`) are optional peer/optional deps. | -| `commands` (subpath) | `registerChangelogCommand`, `registerUpdateCommand` (+ semver helpers) | Commander wiring for cli-core's standard commands (e.g. ` changelog`, ` update`, ` update switch`). **Requires** `commander` as an optional peer-dep. | -| `config` | `getConfigPath`, `readConfig`, `readConfigStrict`, `writeConfig`, `updateConfig`, `CoreConfig`, `UpdateChannel` | Read / write a per-CLI JSON config file with typed error codes; `CoreConfig` is the shape of fields cli-core itself owns (extend it for per-CLI fields). | -| `empty` | `printEmpty` | Print an empty-state message gated on `--json` / `--ndjson` so machine consumers never see human strings on stdout. | -| `errors` | `CliError` | Typed CLI error class with `code` and exit-code mapping. | -| `global-args` | `parseGlobalArgs`, `stripUserFlag`, `createGlobalArgsStore`, `createAccessibleGate`, `createSpinnerGate`, `getProgressJsonlPath`, `isProgressJsonlEnabled` | Parse well-known global flags (`--json`, `--ndjson`, `--quiet`, `--verbose`, `--accessible`, `--no-spinner`, `--progress-jsonl`, `--user `) and derive predicates from them. `stripUserFlag` removes `--user` tokens from argv so the cleaned array can be forwarded to Commander when the flag has no root-program attachment. | -| `json` | `formatJson`, `formatNdjson` | Stable JSON / newline-delimited JSON formatting for stdout. | -| `markdown` (subpath) | `preloadMarkdown`, `renderMarkdown`, `TerminalRendererOptions` | Lazy-init terminal markdown renderer. **Requires** `marked` and `marked-terminal-renderer` as peer-deps — install only if your CLI uses this subpath. | -| `options` | `ViewOptions` | Type contract for `{ json?, ndjson? }` per-command options that machine-output gates derive from. | -| `spinner` | `createSpinner` | Loading spinner factory wrapping `yocto-spinner` with disable gates. | -| `terminal` | `isCI`, `isStderrTTY`, `isStdinTTY`, `isStdoutTTY` | TTY / CI detection helpers. | -| `testing` (subpath) | `describeEmptyMachineOutput` | Vitest helpers reusable by consuming CLIs (e.g. parametrised empty-state suite covering `--json` / `--ndjson` / human modes). | +| Module | Key exports | Purpose | +| -------------------- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `auth` (subpath) | `attachLoginCommand`, `attachLogoutCommand`, `attachStatusCommand`, `attachTokenViewCommand`, `runOAuthFlow`, `createPkceProvider`, `createSecureStore`, `createKeyringTokenStore`, `migrateLegacyAuth`, `persistBundle`, PKCE helpers, `AuthProvider` / `TokenStore` / `TokenBundle` / `RefreshInput` / `AccountRef` / `SecureStore` / `UserRecordStore` types, `AttachLogoutRevokeContext` | OAuth runtime plus the Commander attachers for ` [auth] login` / `logout` / `status` / `token`. `attachLogoutCommand` accepts an optional `revokeToken` hook for best-effort server-side token revocation. Ships the standard public-client PKCE flow (`createPkceProvider`), a thin cross-platform OS-keyring wrapper (`createSecureStore`), and a multi-account keyring-backed `TokenStore` (`createKeyringTokenStore`) that stores secrets in the OS credential manager and degrades to plaintext in the consumer's config when the keyring is unavailable (WSL/headless Linux/containers). The store contract supports an optional `setBundle(account, bundle)` write method (required on `KeyringTokenStore`) so consumers that need refresh-token persistence can opt in via `TokenBundle`; `active()` stays narrow (access token + account only) so callers that don't need refresh state don't pay extra keyring IPC. `AuthProvider` and `TokenStore` remain the escape hatches for DCR or fully bespoke backends. `logout` / `status` / `token` always attach `--user ` and thread the parsed ref to `store.active(ref)` (and `store.clear(ref)` on `logout`). `commander` (when using the attachers), `open` (browser launch), and `@napi-rs/keyring` (when using `createSecureStore` or the keyring `TokenStore`) are optional peer/optional deps. | +| `commands` (subpath) | `registerChangelogCommand`, `registerUpdateCommand` (+ semver helpers) | Commander wiring for cli-core's standard commands (e.g. ` changelog`, ` update`, ` update switch`). **Requires** `commander` as an optional peer-dep. | +| `config` | `getConfigPath`, `readConfig`, `readConfigStrict`, `writeConfig`, `updateConfig`, `CoreConfig`, `UpdateChannel` | Read / write a per-CLI JSON config file with typed error codes; `CoreConfig` is the shape of fields cli-core itself owns (extend it for per-CLI fields). | +| `empty` | `printEmpty` | Print an empty-state message gated on `--json` / `--ndjson` so machine consumers never see human strings on stdout. | +| `errors` | `CliError` | Typed CLI error class with `code` and exit-code mapping. | +| `global-args` | `parseGlobalArgs`, `stripUserFlag`, `createGlobalArgsStore`, `createAccessibleGate`, `createSpinnerGate`, `getProgressJsonlPath`, `isProgressJsonlEnabled` | Parse well-known global flags (`--json`, `--ndjson`, `--quiet`, `--verbose`, `--accessible`, `--no-spinner`, `--progress-jsonl`, `--user `) and derive predicates from them. `stripUserFlag` removes `--user` tokens from argv so the cleaned array can be forwarded to Commander when the flag has no root-program attachment. | +| `json` | `formatJson`, `formatNdjson` | Stable JSON / newline-delimited JSON formatting for stdout. | +| `markdown` (subpath) | `preloadMarkdown`, `renderMarkdown`, `TerminalRendererOptions` | Lazy-init terminal markdown renderer. **Requires** `marked` and `marked-terminal-renderer` as peer-deps — install only if your CLI uses this subpath. | +| `options` | `ViewOptions` | Type contract for `{ json?, ndjson? }` per-command options that machine-output gates derive from. | +| `spinner` | `createSpinner` | Loading spinner factory wrapping `yocto-spinner` with disable gates. | +| `terminal` | `isCI`, `isStderrTTY`, `isStdinTTY`, `isStdoutTTY` | TTY / CI detection helpers. | +| `testing` (subpath) | `describeEmptyMachineOutput` | Vitest helpers reusable by consuming CLIs (e.g. parametrised empty-state suite covering `--json` / `--ndjson` / human modes). | ## Usage @@ -290,9 +290,11 @@ For multi-account storage (OS keychain, per-user config slots, …), implement t ##### Refresh-token storage (`TokenBundle`) -Stores that target servers issuing refresh tokens may implement the optional `setBundle(account, bundle, options?)` method and return a populated `bundle` field from `active()`. `TokenBundle` carries `{ accessToken, refreshToken?, accessTokenExpiresAt?, refreshTokenExpiresAt? }`. Stores that omit `setBundle` continue to work — cli-core helpers (`persistBundle`) fall back to `set(account, bundle.accessToken)` and silently drop refresh state. `KeyringTokenStore` implements `setBundle` as a required override and routes the refresh token to a sibling keyring slot. +Stores that target servers issuing refresh tokens may implement the optional `setBundle(account, bundle, options?)` method. `TokenBundle` carries `{ accessToken, refreshToken?, accessTokenExpiresAt?, refreshTokenExpiresAt? }`. Stores that omit `setBundle` continue to work — cli-core helpers (`persistBundle`) fall back to `set(account, bundle.accessToken)` and silently drop refresh state. `KeyringTokenStore` implements `setBundle` as a required override and routes the refresh token to a sibling keyring slot. -The `persistBundle({ store, account, bundle, promoteDefault? })` helper is the canonical write path — it prefers `setBundle` when available, falls back to `set` otherwise, and wraps non-`CliError` failures as `AUTH_STORE_WRITE_FAILED`. Login persistence (and a future `refreshAccessToken` helper) both use it. +`active()` still returns the narrow `{ token, account }` snapshot — refresh-state material is stored but not surfaced on the hot read path so commands that only need the access token don't pay an extra keyring IPC. A bundle-aware read path lands alongside the silent-refresh helper in a follow-up PR. + +The `persistBundle({ store, account, bundle, promoteDefault? })` helper is the recommended write path for bundle-capable consumers — it prefers `setBundle` when available and falls back to `set` otherwise (the `set` fallback can't honour `promoteDefault`, so multi-account stores wanting silent-refresh-safe selection must implement `setBundle`). cli-core's own `runOAuthFlow` still persists via `store.set()` today; it switches to `persistBundle` when the refresh helper lands. #### Keyring primitive (`createSecureStore`) diff --git a/src/auth/index.ts b/src/auth/index.ts index fe0e128..e0595f6 100644 --- a/src/auth/index.ts +++ b/src/auth/index.ts @@ -26,7 +26,6 @@ export { createPkceProvider } from './providers/pkce.js' export type { PkceLazyString, PkceProviderOptions } from './providers/pkce.js' export type { AccountRef, - ActiveTokenSnapshot, AuthAccount, AuthorizeInput, AuthorizeResult, diff --git a/src/auth/keyring/record-write.test.ts b/src/auth/keyring/record-write.test.ts index 8596408..476f73c 100644 --- a/src/auth/keyring/record-write.test.ts +++ b/src/auth/keyring/record-write.test.ts @@ -206,4 +206,50 @@ describe('writeBundleWithKeyringFallback', () => { expect(accessStore.deleteSpy).toHaveBeenCalledTimes(1) expect(refreshStore.deleteSpy).toHaveBeenCalledTimes(1) }) + + it('falls back to fallbackToken when the access slot is offline (headless/WSL bundle path)', async () => { + // Real-world headless / WSL / locked-Keychain scenario: D-Bus is + // unavailable, so BOTH slot writes throw SecureStoreUnavailable. + // The record must persist both tokens as plaintext fallbacks. + const { accessStore, refreshStore, store: userRecords, state } = harness() + accessStore.setSpy.mockRejectedValueOnce(new SecureStoreUnavailableError('no dbus')) + refreshStore.setSpy.mockRejectedValueOnce(new SecureStoreUnavailableError('no dbus')) + + const result = await writeBundleWithKeyringFallback({ + accessStore, + refreshStore, + userRecords, + account, + bundle, + }) + + expect(result).toEqual({ accessStoredSecurely: false, refreshStoredSecurely: false }) + expect(state.records.get('42')?.fallbackToken).toBe('tok_a') + expect(state.records.get('42')?.fallbackRefreshToken).toBe('tok_r') + expect(state.records.get('42')?.hasRefreshToken).toBe(true) + }) + + it('defers the no-refresh slot wipe until after upsert succeeds', async () => { + // Regression: wiping before upsert would lose refresh state if the + // upsert then rejected. Order must be set-access → upsert → wipe. + const { accessStore, refreshStore, store: userRecords, upsertSpy } = harness() + const callOrder: string[] = [] + refreshStore.deleteSpy.mockImplementationOnce(async () => { + callOrder.push('refresh-delete') + return false + }) + upsertSpy.mockImplementationOnce(async () => { + callOrder.push('upsert') + }) + + await writeBundleWithKeyringFallback({ + accessStore, + refreshStore, + userRecords, + account, + bundle: { accessToken: 'tok_a' }, + }) + + expect(callOrder).toEqual(['upsert', 'refresh-delete']) + }) }) diff --git a/src/auth/keyring/record-write.ts b/src/auth/keyring/record-write.ts index f0802f7..40039ca 100644 --- a/src/auth/keyring/record-write.ts +++ b/src/auth/keyring/record-write.ts @@ -6,7 +6,12 @@ import type { UserRecord, UserRecordStore } from './types.js' type WriteRecordOptions = { /** Per-account keyring slot, already configured by the caller (e.g. via `createSecureStore`). */ secureStore: SecureStore - /** Optional refresh-token keyring slot to wipe alongside the access write. */ + /** + * Optional refresh-token keyring slot. When supplied, any orphan refresh + * material from a prior `setBundle` is wiped best-effort AFTER the user + * record is upserted (see the deferred-cleanup contract on + * `writeBundleWithKeyringFallback`). + */ refreshStore?: SecureStore userRecords: UserRecordStore account: TAccount @@ -40,85 +45,55 @@ type WriteBundleResult = { } /** - * Shared keyring-then-record write used by `createKeyringTokenStore.set` and - * `migrateLegacyAuth`. Encapsulates the order-of-operations contract that - * matters for credential safety: - * - * 1. Keyring `setSecret` first. On `SecureStoreUnavailableError`, swallow - * the failure and record a `fallbackToken` on the user record instead. - * Any other error rethrows. - * 2. `userRecords.upsert(record)`. On failure, best-effort rollback the - * keyring write so we don't leave an orphan credential for an account - * cli-core never managed to register. Original error rethrows. + * Single-token write. Thin wrapper over `writeBundleWithKeyringFallback` + * passing a refresh-less bundle, so trim/validate, access-slot fallback, + * upsert rollback, and the deferred refresh-slot wipe all share one + * implementation. * - * Default promotion (`setDefaultId`) is intentionally **not** in here — both - * call sites do it best-effort outside the critical section because it is a - * preference, not a correctness requirement, and an error there must not - * dirty up a successful credential write. + * `refreshStore` is optional purely for legacy callers (`migrateLegacyAuth`) + * that don't have one wired; the migrate path never had refresh state so + * skipping the wipe is correct there. */ export async function writeRecordWithKeyringFallback( options: WriteRecordOptions, ): Promise { const { secureStore, refreshStore, userRecords, account, token } = options - const trimmed = token.trim() - if (!trimmed) { - throw new CliError('AUTH_STORE_WRITE_FAILED', 'Refusing to persist an empty access token.') - } - let storedSecurely = false - try { - await secureStore.setSecret(trimmed) - storedSecurely = true - } catch (error) { - if (!(error instanceof SecureStoreUnavailableError)) throw error - } - - // Wipe any orphan refresh slot so a prior `setBundle` can't leave - // material behind after a `set()` replaces the credential. Best-effort: - // the cleanup is a security hardening on the contract's - // "replacing any previous entry" promise, not a hard correctness - // requirement (the `hasRefreshToken: false` gate already prevents - // readers from consulting it). - if (refreshStore) { - try { - await refreshStore.deleteSecret() - } catch { - // best-effort - } - } - - // Single-token path; assert no refresh state so `active()` skips the - // refresh-slot IPC instead of probing on every command. - const record: UserRecord = storedSecurely - ? { account, hasRefreshToken: false } - : { account, fallbackToken: trimmed, hasRefreshToken: false } - - try { - await userRecords.upsert(record) - } catch (error) { - if (storedSecurely) { - try { - await secureStore.deleteSecret() - } catch { - // best-effort — the user record failure is the real cause - } - } - throw error - } + const { accessStoredSecurely } = await writeBundleWithKeyringFallback({ + accessStore: secureStore, + // No-op store when the caller didn't wire one — the deferred wipe + // becomes inert and we don't accidentally create a refresh slot + // for legacy/migrate paths. + refreshStore: refreshStore ?? NOOP_SECURE_STORE, + userRecords, + account, + bundle: { accessToken: token }, + }) - return { storedSecurely } + return { storedSecurely: accessStoredSecurely } } /** - * Two-slot variant of `writeRecordWithKeyringFallback`. Order: access slot → - * refresh slot → upsert. `SecureStoreUnavailableError` on either slot degrades - * to the matching `fallback*Token` field. A non-keyring refresh-slot failure - * rolls back the access slot before rethrowing (no partial credentials). An - * upsert failure rolls back both slots via `Promise.allSettled` (no orphan - * credentials for an unregistered user). When the bundle has no refresh - * token, the refresh slot is wiped best-effort so a prior bundle can't - * resurface on the next read. Default promotion is external (same as the - * single-slot helper). + * Two-slot write. Order: access slot → refresh slot → upsert → deferred + * refresh wipe. + * + * 1. Validate `bundle.accessToken` (non-empty after trim). + * 2. `accessStore.setSecret`. `SecureStoreUnavailableError` degrades to + * `fallbackToken` on the record; any other error rethrows. + * 3. `refreshStore.setSecret` when `bundle.refreshToken` is present. + * `SecureStoreUnavailableError` degrades to `fallbackRefreshToken`. A + * non-keyring failure rolls back the access slot before rethrowing + * (no partial credentials left behind for an unregistered user). + * 4. `userRecords.upsert(record)`. On failure, best-effort + * `Promise.allSettled` rollback of any slot writes that succeeded. + * 5. Only after a successful upsert: if the bundle has no refresh token, + * wipe any orphan slot from a prior `setBundle` (best-effort). Doing + * this BEFORE the upsert would lose refresh state if the upsert then + * rejected — the new record's `hasRefreshToken` would still claim + * false but the old slot would be gone with no rollback path. + * + * Default promotion is external — preference, not correctness, and an + * error there must not dirty up a successful credential write. */ export async function writeBundleWithKeyringFallback( options: WriteBundleOptions, @@ -160,14 +135,6 @@ export async function writeBundleWithKeyringFallback = { @@ -197,5 +164,29 @@ export async function writeBundleWithKeyringFallback> = {}, + defaultId: string | null = null, +) { + const km = buildKeyringMap() + mockedCreateSecureStore.mockImplementation(km.create) + const harness = buildUserRecords() + for (const [id, rec] of Object.entries(records)) { + harness.state.records.set(id, rec) + } + harness.state.defaultId = defaultId + const store = createKeyringTokenStore({ + serviceName: SERVICE, + userRecords: harness.store, + recordsLocation: LOCATION, + }) + return { km, store, state: harness.state, upsertSpy: harness.upsertSpy } +} + describe('createKeyringTokenStore', () => { beforeEach(() => { mockedCreateSecureStore.mockReset() }) it('round-trips set → active → clear when the keyring is online', async () => { - // Per-slot map: set() now wipes the refresh slot too, so a - // single-slot mock would clobber the access secret. - const km = buildKeyringMap() - mockedCreateSecureStore.mockImplementation(km.create) - const harness = buildUserRecords() - const store = createKeyringTokenStore({ - serviceName: SERVICE, - userRecords: harness.store, - recordsLocation: LOCATION, - }) + // Per-slot map: set() wipes the refresh slot too, so a single-slot + // mock would clobber the access secret. + const { km, store, state, upsertSpy } = mapFixture() await store.set(account, 'tok_secret') expect(km.slots.get('user-42')?.secret).toBe('tok_secret') - expect(harness.upsertSpy).toHaveBeenCalledWith({ account, hasRefreshToken: false }) - expect(harness.state.defaultId).toBe('42') + expect(upsertSpy).toHaveBeenCalledWith({ account, hasRefreshToken: false }) + expect(state.defaultId).toBe('42') expect(store.getLastStorageResult()).toEqual({ storage: 'secure-store' }) await expect(store.active()).resolves.toEqual({ token: 'tok_secret', account }) @@ -99,8 +116,8 @@ describe('createKeyringTokenStore', () => { await store.clear() expect(km.slots.get('user-42')?.secret).toBeNull() expect(km.slots.get(refreshAccountSlot('user-42'))?.secret).toBeNull() - expect(harness.state.records.size).toBe(0) - expect(harness.state.defaultId).toBeNull() + expect(state.records.size).toBe(0) + expect(state.defaultId).toBeNull() expect(store.getLastClearResult()).toEqual({ storage: 'secure-store' }) }) @@ -118,7 +135,7 @@ describe('createKeyringTokenStore', () => { expect(store.getLastStorageResult()).toEqual({ storage: 'config-file', warning: - 'system credential manager unavailable; token saved as plaintext in /tmp/fake/config.json', + 'system credential manager unavailable; access token saved as plaintext in /tmp/fake/config.json', }) await expect(store.active()).resolves.toEqual({ token: 'tok_plain', account }) @@ -148,11 +165,19 @@ describe('createKeyringTokenStore', () => { it.each([ [ - 'the keyring read throws', + 'the keyring read throws SecureStoreUnavailableError', (k: SingleSlot) => { k.getSpy.mockRejectedValueOnce(new SecureStoreUnavailableError('locked')) }, ], + [ + 'the keyring read throws a non-keyring backend error', + (k: SingleSlot) => { + // Generic backend failure must also wrap into the typed code; + // a raw exception would crash the CLI with no exit signal. + k.getSpy.mockRejectedValueOnce(new Error('disk fried')) + }, + ], [ 'the keyring slot is empty (out-of-band deletion)', () => { @@ -169,18 +194,8 @@ describe('createKeyringTokenStore', () => { }) it('picks the lone user when no default is set', async () => { - // Per-slot keyring map so refresh probes (legacy record) hit an - // empty distinct slot rather than the same single-slot secret. - const km = buildKeyringMap() - mockedCreateSecureStore.mockImplementation(km.create) - const harness = buildUserRecords() - harness.state.records.set('42', { account }) + const { km, store } = mapFixture({ '42': { account } }) km.slots.set('user-42', { secret: 'tok' }) - const store = createKeyringTokenStore({ - serviceName: SERVICE, - userRecords: harness.store, - recordsLocation: LOCATION, - }) await expect(store.active()).resolves.toEqual({ token: 'tok', account }) }) @@ -437,41 +452,21 @@ describe('createKeyringTokenStore', () => { }) }) - describe('setBundle / active() bundle round-trip', () => { - function mapFixture( - records: Record> = {}, - defaultId: string | null = null, - ) { - const km = buildKeyringMap() - mockedCreateSecureStore.mockImplementation(km.create) - const harness = buildUserRecords() - for (const [id, rec] of Object.entries(records)) { - harness.state.records.set(id, rec) - } - harness.state.defaultId = defaultId - const store = createKeyringTokenStore({ - serviceName: SERVICE, - userRecords: harness.store, - recordsLocation: LOCATION, - }) - return { km, store, state: harness.state, upsertSpy: harness.upsertSpy } - } - + describe('setBundle storage', () => { const bundle: TokenBundle = { accessToken: 'tok_a', refreshToken: 'tok_r', accessTokenExpiresAt: 1_700_000_000_000, } - it('round-trips set → active with the full bundle', async () => { + it('persists access slot, refresh slot, and record metadata; active() returns the access snapshot', async () => { const { km, store, state } = mapFixture() await store.setBundle(account, bundle, { promoteDefault: true }) - // Both slots written + // Storage: both slots written, record carries gate + expiry. expect(km.slots.get('user-42')?.secret).toBe('tok_a') expect(km.slots.get(refreshAccountSlot('user-42'))?.secret).toBe('tok_r') - // Record carries the gate + expiry, no plaintext fallbacks const record = state.records.get('42') expect(record?.hasRefreshToken).toBe(true) expect(record?.accessTokenExpiresAt).toBe(1_700_000_000_000) @@ -479,19 +474,8 @@ describe('createKeyringTokenStore', () => { expect(record?.fallbackRefreshToken).toBeUndefined() expect(state.defaultId).toBe('42') - // Active resolves both tokens — flat snapshot, no `bundle` wrapper. - await expect(store.active()).resolves.toEqual({ - token: 'tok_a', - account, - refreshToken: 'tok_r', - accessTokenExpiresAt: 1_700_000_000_000, - }) - }) - - it('omits refresh-state fields on active() when nothing refresh-related is stored', async () => { - const { store } = mapFixture() - await store.set(account, 'tok_a') - + // Read side stays narrow: active() returns only token + account. + // Reading the stored refresh state is a PR3 concern. await expect(store.active()).resolves.toEqual({ token: 'tok_a', account }) }) @@ -505,66 +489,11 @@ describe('createKeyringTokenStore', () => { expect(state.defaultId).toBeNull() }) - it('skips the refresh-slot IPC when hasRefreshToken is false', async () => { - const { km, store } = mapFixture({ - '42': { account, hasRefreshToken: false }, - }) - km.slots.set('user-42', { secret: 'tok_a' }) - // Seed an orphan refresh slot — verify it is NOT consulted. - km.slots.set(refreshAccountSlot('user-42'), { secret: 'orphan_r' }) - - const active = await store.active() - expect(active?.token).toBe('tok_a') - expect(active?.refreshToken).toBeUndefined() - // Hard assertion: refresh-slot getSecret was never invoked. - expect(km.getCalls.has(refreshAccountSlot('user-42'))).toBe(false) - }) - - it('probes the refresh slot when hasRefreshToken is undefined (legacy record)', async () => { - const { km, store, state } = mapFixture({ '42': { account } }) - km.slots.set('user-42', { secret: 'tok_a' }) - km.slots.set(refreshAccountSlot('user-42'), { secret: 'tok_r' }) - - await expect(store.active()).resolves.toMatchObject({ refreshToken: 'tok_r' }) - // Legacy records pay the probe on every read — no race-prone - // backfill. A subsequent `setBundle` will write `hasRefreshToken` - // explicitly. - expect(state.records.get('42')?.hasRefreshToken).toBeUndefined() - }) - - it.each([ - { - label: 'SecureStoreUnavailableError → downgrade to no-refresh', - err: new SecureStoreUnavailableError('locked'), - expect: 'downgrade' as const, - }, - { - label: 'non-keyring error → AUTH_STORE_READ_FAILED', - err: new Error('disk fried'), - expect: 'throw' as const, - }, - ])('refresh-slot read: $label', async ({ err, expect: outcome }) => { - const { km, store } = mapFixture({ '42': { account, hasRefreshToken: true } }) - km.slots.set('user-42', { secret: 'tok_a' }) - km.slots.set(refreshAccountSlot('user-42'), { secret: 'tok_r', getErr: err }) - - if (outcome === 'downgrade') { - const result = await store.active() - expect(result?.token).toBe('tok_a') - expect(result).not.toHaveProperty('refreshToken') - } else { - await expect(store.active()).rejects.toMatchObject({ - code: 'AUTH_STORE_READ_FAILED', - }) - } - }) - it('set() wipes the refresh slot left behind by a prior setBundle', async () => { // Regression: `set(account, token)` is documented as "replacing - // any previous entry". Without the wipe, a later set() would - // leave the refresh secret orphaned in the keyring even though - // the record's `hasRefreshToken: false` says it shouldn't be - // there. + // any previous entry". A later set() must leave no orphan + // refresh material — otherwise a future bundle-aware reader + // would see stale data. const { km, store, state } = mapFixture() await store.setBundle(account, bundle, { promoteDefault: true }) diff --git a/src/auth/keyring/token-store.ts b/src/auth/keyring/token-store.ts index ac1cfca..a2cbd47 100644 --- a/src/auth/keyring/token-store.ts +++ b/src/auth/keyring/token-store.ts @@ -165,58 +165,44 @@ export function createKeyringTokenStore( } } - /** Returns `null` on corruption (record exists, slot empty); caller throws `AUTH_STORE_READ_FAILED`. */ - async function readAccessToken(record: UserRecord): Promise { - const fallback = record.fallbackToken?.trim() - if (fallback) return fallback - try { - const raw = await secureStoreFor(record.account).getSecret() - return raw?.trim() || null - } catch (error) { - if (error instanceof SecureStoreUnavailableError) { - throw new CliError( - 'AUTH_STORE_READ_FAILED', - `${SECURE_STORE_DESCRIPTION} unavailable; could not read stored token (${error.message})`, - ) - } - // Symmetric with `readRefreshToken`: wrap unexpected backend - // failures so the CLI sees a typed error code instead of crashing - // with the raw exception. - throw new CliError( - 'AUTH_STORE_READ_FAILED', - `Access-slot read failed (${getErrorMessage(error)})`, - ) - } + /** + * Compose a storage result for a write that may have fallen back on + * either slot. `accessStored === false` indicates the access token went + * to `fallbackToken`; `refreshStored === false` indicates the refresh + * token went to `fallbackRefreshToken`. Either falsy slot downgrades + * the result to `config-file` so consumers see the warning — refresh + * plaintext is just as security-relevant as access plaintext. + */ + function bundleStorageResult( + accessStored: boolean, + refreshStored: boolean | undefined, + ): TokenStorageResult { + const accessFallback = !accessStored + const refreshFallback = refreshStored === false + if (!accessFallback && !refreshFallback) return { storage: 'secure-store' } + const subject = + accessFallback && refreshFallback + ? 'access + refresh tokens' + : accessFallback + ? 'access token' + : 'refresh token' + return fallbackResult(`${subject} saved as plaintext in`) } /** - * Honours the `hasRefreshToken` gate. `SecureStoreUnavailableError` - * downgrades to `undefined` (silent-refresh callers re-prompt); other - * errors propagate as `AUTH_STORE_READ_FAILED` — corruption shouldn't - * masquerade as "no refresh". - * - * Legacy records (`hasRefreshToken === undefined`) probe the slot on - * every read. The previous fire-and-forget backfill raced with - * concurrent `setBundle` writes (replace-not-merge `upsert` would - * clobber bundle metadata). One IPC per command is an acceptable cost - * — a single `setBundle` call rewrites the record with an explicit - * gate and the probes stop. + * Best-effort default promotion shared by `set` and `setBundle`. The + * record is already persisted, so a failure here must not surface as + * `AUTH_STORE_WRITE_FAILED` — the user can recover by setting a + * default later. */ - async function readRefreshToken(record: UserRecord): Promise { - if (record.hasRefreshToken === false) return undefined - - const fallback = record.fallbackRefreshToken?.trim() - if (fallback) return fallback - + async function promoteDefaultIfNeeded(accountId: string): Promise { try { - const raw = await refreshSecureStoreFor(record.account).getSecret() - return raw?.trim() || undefined - } catch (error) { - if (error instanceof SecureStoreUnavailableError) return undefined - throw new CliError( - 'AUTH_STORE_READ_FAILED', - `Refresh-slot read failed (${getErrorMessage(error)})`, - ) + const existingDefault = await userRecords.getDefaultId() + if (!existingDefault) { + await userRecords.setDefaultId(accountId) + } + } catch { + // best-effort } } @@ -232,33 +218,41 @@ export function createKeyringTokenStore( const record = resolveTarget(snapshot, ref) if (!record) return null - // Parallel: serial reads doubled `active()` latency. - const [accessToken, refreshToken] = await Promise.all([ - readAccessToken(record), - readRefreshToken(record), - ]) + // Reads the access slot only. Refresh-state material lives in + // the keyring and on the record, but `active()` stays cheap and + // returns the pre-PR1 snapshot shape — a future bundle-aware + // read path lights up the refresh slot only when callers + // actually need it (silent refresh). + const fallback = record.fallbackToken?.trim() + if (fallback) return { token: fallback, account: record.account } - if (!accessToken) { - // Record exists, no `fallbackToken`, slot empty — corruption - // (deleted out-of-band). Collapsing to `null` would surface as - // ACCOUNT_NOT_FOUND on `--user ` and hide the real problem. + let raw: string | null + try { + raw = await secureStoreFor(record.account).getSecret() + } catch (error) { + if (error instanceof SecureStoreUnavailableError) { + throw new CliError( + 'AUTH_STORE_READ_FAILED', + `${SECURE_STORE_DESCRIPTION} unavailable; could not read stored token (${error.message})`, + ) + } + // Non-keyring backend failures wrap into the typed code too — + // a raw exception escaping `active()` would crash the CLI + // with no useful exit signal. throw new CliError( 'AUTH_STORE_READ_FAILED', - `${SECURE_STORE_DESCRIPTION} returned no credential for the stored account; the keyring entry may have been removed externally.`, + `Access-slot read failed (${getErrorMessage(error)})`, ) } - return { - token: accessToken, - account: record.account, - ...(refreshToken !== undefined ? { refreshToken } : {}), - ...(record.accessTokenExpiresAt !== undefined - ? { accessTokenExpiresAt: record.accessTokenExpiresAt } - : {}), - ...(record.refreshTokenExpiresAt !== undefined - ? { refreshTokenExpiresAt: record.refreshTokenExpiresAt } - : {}), - } + const token = raw?.trim() + if (token) return { token, account: record.account } + + // Record exists, no `fallbackToken`, slot empty — corruption. + throw new CliError( + 'AUTH_STORE_READ_FAILED', + `${SECURE_STORE_DESCRIPTION} returned no credential for the stored account; the keyring entry may have been removed externally.`, + ) }, async set(account, token) { @@ -275,57 +269,30 @@ export function createKeyringTokenStore( token, }) - // Best-effort default promotion: the record is already persisted, - // so a failure here must not turn into `AUTH_STORE_WRITE_FAILED` - // (the user can recover by setting a default later). - try { - const existingDefault = await userRecords.getDefaultId() - if (!existingDefault) { - await userRecords.setDefaultId(account.id) - } - } catch { - // best-effort - } + await promoteDefaultIfNeeded(account.id) - lastStorageResult = storedSecurely - ? { storage: 'secure-store' } - : fallbackResult('token saved as plaintext in') + lastStorageResult = bundleStorageResult(storedSecurely, undefined) }, async setBundle(account, bundle, options) { lastStorageResult = undefined - const { accessStoredSecurely } = await writeBundleWithKeyringFallback({ - accessStore: secureStoreFor(account), - refreshStore: refreshSecureStoreFor(account), - userRecords, - account, - bundle, - }) + const { accessStoredSecurely, refreshStoredSecurely } = + await writeBundleWithKeyringFallback({ + accessStore: secureStoreFor(account), + refreshStore: refreshSecureStoreFor(account), + userRecords, + account, + bundle, + }) // Opt-in: silent refresh omits `promoteDefault` so it can't // re-pin selection; login passes `true` to match `set()`. if (options?.promoteDefault) { - try { - const existingDefault = await userRecords.getDefaultId() - if (!existingDefault) { - await userRecords.setDefaultId(account.id) - } - } catch { - // best-effort - } + await promoteDefaultIfNeeded(account.id) } - // Storage result is tied to the access slot — that's the - // user-visible credential the existing warning text describes. - // A refresh-only fallback is silent today; if we ever want to - // call it out it needs distinct phrasing (the current copy says - // "token", which implies access). The record itself carries - // `fallbackRefreshToken` so the runtime read picks it up - // transparently regardless. - lastStorageResult = accessStoredSecurely - ? { storage: 'secure-store' } - : fallbackResult('token saved as plaintext in') + lastStorageResult = bundleStorageResult(accessStoredSecurely, refreshStoredSecurely) }, async clear(ref) { diff --git a/src/auth/keyring/types.ts b/src/auth/keyring/types.ts index f64f083..53ce80a 100644 --- a/src/auth/keyring/types.ts +++ b/src/auth/keyring/types.ts @@ -32,10 +32,11 @@ export type UserRecord = { /** Refresh-token expiry, unix-epoch ms. */ refreshTokenExpiresAt?: number /** - * Gate on the refresh-slot keyring read in `active()`: - * - `true` → read the slot. - * - `false` → skip the IPC. - * - `undefined` → legacy record; probe once and backfill `false` on empty. + * `true` when a refresh secret is stored (in the keyring or as + * `fallbackRefreshToken`); `false` when explicitly cleared by `set()` + * or by a no-refresh `setBundle`; `undefined` on legacy records that + * predate the bundle contract. Read by future bundle-aware accessors; + * `active()` itself doesn't consult it. */ hasRefreshToken?: boolean } diff --git a/src/auth/types.ts b/src/auth/types.ts index e219fe3..934d9a9 100644 --- a/src/auth/types.ts +++ b/src/auth/types.ts @@ -90,20 +90,6 @@ export type TokenBundle = { refreshTokenExpiresAt?: number } -/** - * Read-side snapshot returned by `TokenStore.active`. `token` is the access - * token (kept named that way for back-compat with the pre-bundle contract); - * refresh-state fields are flat siblings so consumers have a single source of - * truth per credential. - */ -export type ActiveTokenSnapshot = { - token: string - account: TAccount - refreshToken?: string - accessTokenExpiresAt?: number - refreshTokenExpiresAt?: number -} - /** Opaque account selector. Stores own the matching rule (id, email, label, …). */ export type AccountRef = string @@ -122,7 +108,7 @@ export type TokenStore = { * explicit-ref path and proceeds with `clear(ref)`; `attachStatusCommand` * and `attachTokenViewCommand` propagate it. */ - active(ref?: AccountRef): Promise | null> + active(ref?: AccountRef): Promise<{ token: string; account: TAccount } | null> /** Persist `token` for `account`, replacing any previous entry. Throw `CliError` for typed failures; other thrown values become `AUTH_STORE_WRITE_FAILED`. */ set(account: TAccount, token: string): Promise /**