From 16f26bd9a14e08b7151e92d509760e6c392f8036 Mon Sep 17 00:00:00 2001 From: Scott Lovegrove Date: Sat, 16 May 2026 10:54:23 +0100 Subject: [PATCH 1/4] feat(auth): add keyring-backed multi-account TokenStore MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Ship a reusable secure-token implementation so todoist-cli and twist-cli can drop their duplicate `secure-store.ts` wrappers and converge on the same multi-account model. Wraps `@napi-rs/keyring` for cross-platform secret storage (Keychain / Credential Manager / libsecret), and falls back to a plaintext token on the consumer's user record when the keyring is unreachable (WSL without D-Bus, headless Linux, containers, CI). New under `src/auth/keyring/`: - `createSecureStore({ serviceName, account })` + `SecureStoreUnavailableError` — keyring primitive. - `createKeyringTokenStore({ serviceName, userRecords, … })` — full multi-account `TokenStore` (including `list` / `setDefault` and `--user ` matching) that composes the primitive with a consumer-supplied `UserRecordStore` port over their config file. - `migrateLegacyAuth({ … })` — generic v1→v2 helper for postinstall hooks. `@napi-rs/keyring` is declared as an optional dependency and dynamic-imported so a missing native binary surfaces as `SecureStoreUnavailableError` and triggers the plaintext fallback instead of crashing module load. Co-Authored-By: Claude Opus 4.7 (1M context) --- README.md | 97 +++++- package-lock.json | 223 ++++++++++++++ package.json | 3 + src/auth/index.ts | 19 ++ src/auth/keyring/index.ts | 19 ++ src/auth/keyring/migrate.test.ts | 272 +++++++++++++++++ src/auth/keyring/migrate.ts | 198 ++++++++++++ src/auth/keyring/secure-store.test.ts | 104 +++++++ src/auth/keyring/secure-store.ts | 95 ++++++ src/auth/keyring/token-store.test.ts | 425 ++++++++++++++++++++++++++ src/auth/keyring/token-store.ts | 231 ++++++++++++++ src/auth/keyring/types.ts | 47 +++ 12 files changed, 1718 insertions(+), 15 deletions(-) create mode 100644 src/auth/keyring/index.ts create mode 100644 src/auth/keyring/migrate.test.ts create mode 100644 src/auth/keyring/migrate.ts create mode 100644 src/auth/keyring/secure-store.test.ts create mode 100644 src/auth/keyring/secure-store.ts create mode 100644 src/auth/keyring/token-store.test.ts create mode 100644 src/auth/keyring/token-store.ts create mode 100644 src/auth/keyring/types.ts diff --git a/README.md b/README.md index 4717c5f..49c63a4 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`, PKCE helpers, `AuthProvider` / `TokenStore` / `AccountRef` 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`); `AuthProvider` and `TokenStore` are the escape hatches for DCR, OS-keychain, multi-account, etc. — consumers implement `TokenStore` directly (single-user store implements `list` / `setDefault` trivially against the one account). `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) and `open` (browser launch) are optional peer-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`, PKCE helpers, `createKeyringTokenStore`, `createSecureStore`, `migrateLegacyAuth`, `AuthProvider` / `TokenStore` / `AccountRef` / `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`) 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 custom 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 the keyring store) 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 @@ -286,7 +286,74 @@ 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. +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. For the OS-keychain case, see the `createKeyringTokenStore` factory below. + +#### Multi-account keyring-backed `TokenStore` + +`createKeyringTokenStore` wires `@napi-rs/keyring` into the `TokenStore` contract for multi-account CLIs. Secrets live in the OS credential manager (Keychain / Credential Manager / libsecret); per-user metadata stays in the consumer's config via a small `UserRecordStore` port. When the keyring is unreachable — common on WSL without D-Bus, headless Linux, containers, and CI — the store transparently falls back to a `fallbackToken` field on the user record and exposes a warning on `getLastStorageResult()` for the login command to surface. + +```ts +import { createKeyringTokenStore, type UserRecordStore } from '@doist/cli-core/auth' + +type Account = { id: string; label?: string; email: string } + +// Adapter over the consumer's existing config.json shape. +const userRecords: UserRecordStore = { + async list() { + /* read from config */ + }, + async getById(id) { + /* … */ + }, + async upsert(record) { + /* … */ + }, + async remove(id) { + /* … */ + }, + async getDefaultId() { + /* … */ + }, + async setDefaultId(id) { + /* … */ + }, + describeLocation() { + return '~/.config/todoist-cli/config.json' + }, +} + +export const tokenStore = createKeyringTokenStore({ + serviceName: 'todoist-cli', + userRecords, +}) + +// In your login command's onSuccess: +const storage = tokenStore.getLastStorageResult() +if (storage?.warning) console.error('Warning:', storage.warning) +``` + +The returned store satisfies the full `TokenStore` contract — including `list()` / `setDefault(ref)` / `ref`-aware `active` / `clear` — so it plugs straight into the `logout` / `status` / `token` attachers. Default ref matching is `account.id === ref || account.label === ref`; override `matchAccount` to broaden it (e.g. email-case-insensitive). + +Add `@napi-rs/keyring` as an optional/peer dep of your CLI. The library ships pre-built native binaries for Windows, macOS, and Linux (glibc + musl); cli-core dynamic-imports it so a missing binary on an exotic arch surfaces as `SecureStoreUnavailableError` and triggers the same plaintext fallback path rather than crashing module load. + +For one-time migration of a v1 single-user token into the v2 multi-user shape, use `migrateLegacyAuth` from a postinstall hook: + +```ts +import { migrateLegacyAuth } from '@doist/cli-core/auth' + +await migrateLegacyAuth({ + serviceName: 'todoist-cli', + legacyAccount: 'api-token', + userRecords, + loadLegacyPlaintextToken: async () => readConfig().api_token ?? null, + identifyAccount: async (token) => fetchUser(token), + cleanupLegacyConfig: async () => clearLegacyAuthFields(), + silent: true, + logPrefix: 'todoist-cli', +}) +``` + +For sync/lazy-decrypt or fully bespoke backends, implement `TokenStore` directly. #### `--user ` and multi-user wiring diff --git a/package-lock.json b/package-lock.json index 8b0389e..487a68a 100644 --- a/package-lock.json +++ b/package-lock.json @@ -32,6 +32,9 @@ "engines": { "node": ">=20.18.1" }, + "optionalDependencies": { + "@napi-rs/keyring": "1.3.0" + }, "peerDependencies": { "commander": ">=14", "marked": ">=18", @@ -654,6 +657,226 @@ "dev": true, "license": "MIT" }, + "node_modules/@napi-rs/keyring": { + "version": "1.3.0", + "resolved": "https://registry.npmjs.org/@napi-rs/keyring/-/keyring-1.3.0.tgz", + "integrity": "sha512-WrOw/bcXm0f9qHkumlT1QlArXSTWqaY9sunsDpOk+yCCorCKMxvWT/a3xko4EYHVdeZoh00yI2TydXn6eyICDA==", + "license": "MIT", + "optional": true, + "engines": { + "node": ">= 10" + }, + "funding": { + "type": "github", + "url": "https://github.com/sponsors/Brooooooklyn" + }, + "optionalDependencies": { + "@napi-rs/keyring-darwin-arm64": "1.3.0", + "@napi-rs/keyring-darwin-x64": "1.3.0", + "@napi-rs/keyring-freebsd-x64": "1.3.0", + "@napi-rs/keyring-linux-arm-gnueabihf": "1.3.0", + "@napi-rs/keyring-linux-arm64-gnu": "1.3.0", + "@napi-rs/keyring-linux-arm64-musl": "1.3.0", + "@napi-rs/keyring-linux-riscv64-gnu": "1.3.0", + "@napi-rs/keyring-linux-x64-gnu": "1.3.0", + "@napi-rs/keyring-linux-x64-musl": "1.3.0", + "@napi-rs/keyring-win32-arm64-msvc": "1.3.0", + "@napi-rs/keyring-win32-ia32-msvc": "1.3.0", + "@napi-rs/keyring-win32-x64-msvc": "1.3.0" + } + }, + "node_modules/@napi-rs/keyring-darwin-arm64": { + "version": "1.3.0", + "resolved": "https://registry.npmjs.org/@napi-rs/keyring-darwin-arm64/-/keyring-darwin-arm64-1.3.0.tgz", + "integrity": "sha512-pl76hJvdYUBn6I24bXiOBMA9nbDapo3I5B+f3OorjDU4dUMSypXeKbOVehJe8fhgTiH24flMyTS3aAIy43xegQ==", + "cpu": [ + "arm64" + ], + "license": "MIT", + "optional": true, + "os": [ + "darwin" + ], + "engines": { + "node": ">= 10" + } + }, + "node_modules/@napi-rs/keyring-darwin-x64": { + "version": "1.3.0", + "resolved": "https://registry.npmjs.org/@napi-rs/keyring-darwin-x64/-/keyring-darwin-x64-1.3.0.tgz", + "integrity": "sha512-YcJtEV5LA3cvA4z3BurgxH5IhTsW1JfIvcAAcqcecwk06Si9F9NqkxbZVIfDwQ8oRHgaBmT3zZJnLAotCrVahw==", + "cpu": [ + "x64" + ], + "license": "MIT", + "optional": true, + "os": [ + "darwin" + ], + "engines": { + "node": ">= 10" + } + }, + "node_modules/@napi-rs/keyring-freebsd-x64": { + "version": "1.3.0", + "resolved": "https://registry.npmjs.org/@napi-rs/keyring-freebsd-x64/-/keyring-freebsd-x64-1.3.0.tgz", + "integrity": "sha512-vlLf31TGhfRAaxLDBhg8b89ss0HHD/lyNmL5F3UjSaz5CUXElsJmKYq9fqA/B+cZKUEUcLHHGhF0I/CqcFdaVw==", + "cpu": [ + "x64" + ], + "license": "MIT", + "optional": true, + "os": [ + "freebsd" + ], + "engines": { + "node": ">= 10" + } + }, + "node_modules/@napi-rs/keyring-linux-arm-gnueabihf": { + "version": "1.3.0", + "resolved": "https://registry.npmjs.org/@napi-rs/keyring-linux-arm-gnueabihf/-/keyring-linux-arm-gnueabihf-1.3.0.tgz", + "integrity": "sha512-KiWdMMu/Inz/bHHIAGrnF7r54FZDYXuHO6UFF/rhIrshUsxbMG1Rl9lEymNtqqsVo927G0VYcb02FzWQ3iBQRQ==", + "cpu": [ + "arm" + ], + "license": "MIT", + "optional": true, + "os": [ + "linux" + ], + "engines": { + "node": ">= 10" + } + }, + "node_modules/@napi-rs/keyring-linux-arm64-gnu": { + "version": "1.3.0", + "resolved": "https://registry.npmjs.org/@napi-rs/keyring-linux-arm64-gnu/-/keyring-linux-arm64-gnu-1.3.0.tgz", + "integrity": "sha512-eyKGpY40lm9Jvs1aD294XRH4y7+TlJM0YVAryZeXA6TX0mb4gMkxVXwSQv7MCwgah7raeUd0dKUb4BPAYIgcMg==", + "cpu": [ + "arm64" + ], + "license": "MIT", + "optional": true, + "os": [ + "linux" + ], + "engines": { + "node": ">= 10" + } + }, + "node_modules/@napi-rs/keyring-linux-arm64-musl": { + "version": "1.3.0", + "resolved": "https://registry.npmjs.org/@napi-rs/keyring-linux-arm64-musl/-/keyring-linux-arm64-musl-1.3.0.tgz", + "integrity": "sha512-iIK6JWHXAJqDrEyLY3TmswwloVyt2vj+04TZnew+uSJ9gnDO8EwRbp3/iw3LpWaXiDO7VomGO6y8I0Id8uBZSw==", + "cpu": [ + "arm64" + ], + "license": "MIT", + "optional": true, + "os": [ + "linux" + ], + "engines": { + "node": ">= 10" + } + }, + "node_modules/@napi-rs/keyring-linux-riscv64-gnu": { + "version": "1.3.0", + "resolved": "https://registry.npmjs.org/@napi-rs/keyring-linux-riscv64-gnu/-/keyring-linux-riscv64-gnu-1.3.0.tgz", + "integrity": "sha512-/PGqrwn6EwgtK6vccASSXJRfOSP4vN1F4ASsIQ+7MdrK6hNvAJ1FZPrIuD5gGGdxezo3F++To2Wq7DbuGIeuNQ==", + "cpu": [ + "riscv64" + ], + "license": "MIT", + "optional": true, + "os": [ + "linux" + ], + "engines": { + "node": ">= 10" + } + }, + "node_modules/@napi-rs/keyring-linux-x64-gnu": { + "version": "1.3.0", + "resolved": "https://registry.npmjs.org/@napi-rs/keyring-linux-x64-gnu/-/keyring-linux-x64-gnu-1.3.0.tgz", + "integrity": "sha512-2PDK1WKWTu9lBGq9VvNEkSlQD3O7YwVpmnyN2M3cy4v7NJ/8gDMd9GXv3G+FVXN13uhp4gnnPBS+ScefmEeD2A==", + "cpu": [ + "x64" + ], + "license": "MIT", + "optional": true, + "os": [ + "linux" + ], + "engines": { + "node": ">= 10" + } + }, + "node_modules/@napi-rs/keyring-linux-x64-musl": { + "version": "1.3.0", + "resolved": "https://registry.npmjs.org/@napi-rs/keyring-linux-x64-musl/-/keyring-linux-x64-musl-1.3.0.tgz", + "integrity": "sha512-oJ2HkX8YUo46QBkn0pG+HuIKQNqr523q6vBobCn+P95s4C4K6/kLBqHY/1bg5J4ap31DzsznhnFKcfBNBsjCnw==", + "cpu": [ + "x64" + ], + "license": "MIT", + "optional": true, + "os": [ + "linux" + ], + "engines": { + "node": ">= 10" + } + }, + "node_modules/@napi-rs/keyring-win32-arm64-msvc": { + "version": "1.3.0", + "resolved": "https://registry.npmjs.org/@napi-rs/keyring-win32-arm64-msvc/-/keyring-win32-arm64-msvc-1.3.0.tgz", + "integrity": "sha512-tOd3c/uAaeoE4ycVlmAdSvygz0Zt3zdca6Y7gokBeIbaRDWpjDIUOpU3MvML59XAaqyuKGsVVu0F/DZb1lHPmw==", + "cpu": [ + "arm64" + ], + "license": "MIT", + "optional": true, + "os": [ + "win32" + ], + "engines": { + "node": ">= 10" + } + }, + "node_modules/@napi-rs/keyring-win32-ia32-msvc": { + "version": "1.3.0", + "resolved": "https://registry.npmjs.org/@napi-rs/keyring-win32-ia32-msvc/-/keyring-win32-ia32-msvc-1.3.0.tgz", + "integrity": "sha512-sPSqeAFZMGqP1R++M2JTza7GQJJ/TpCo6JU6Vcd4jnebvOaEDs9b7eipakU1PJdSvhpC2yXMCNRk9gXfrhuwHQ==", + "cpu": [ + "ia32" + ], + "license": "MIT", + "optional": true, + "os": [ + "win32" + ], + "engines": { + "node": ">= 10" + } + }, + "node_modules/@napi-rs/keyring-win32-x64-msvc": { + "version": "1.3.0", + "resolved": "https://registry.npmjs.org/@napi-rs/keyring-win32-x64-msvc/-/keyring-win32-x64-msvc-1.3.0.tgz", + "integrity": "sha512-4DnCWXwDc0HRKwyRlG5y0VhKZW2tNRQfKKfyj6IX/KWfDNyq9hn4n+GL1auyDcOO/v8PwnhmYo2+rOOqCkvvOg==", + "cpu": [ + "x64" + ], + "license": "MIT", + "optional": true, + "os": [ + "win32" + ], + "engines": { + "node": ">= 10" + } + }, "node_modules/@napi-rs/wasm-runtime": { "version": "1.1.4", "resolved": "https://registry.npmjs.org/@napi-rs/wasm-runtime/-/wasm-runtime-1.1.4.tgz", diff --git a/package.json b/package.json index a7ed32e..2f83310 100644 --- a/package.json +++ b/package.json @@ -83,6 +83,9 @@ "chalk": "5.6.2", "yocto-spinner": "1.2.0" }, + "optionalDependencies": { + "@napi-rs/keyring": "1.3.0" + }, "peerDependencies": { "commander": ">=14", "marked": ">=18", diff --git a/src/auth/index.ts b/src/auth/index.ts index 579ceac..d5fbc0f 100644 --- a/src/auth/index.ts +++ b/src/auth/index.ts @@ -35,3 +35,22 @@ export type { TokenStore, ValidateInput, } from './types.js' +export { + SECURE_STORE_DESCRIPTION, + SecureStoreUnavailableError, + createKeyringTokenStore, + createSecureStore, + migrateLegacyAuth, +} from './keyring/index.js' +export type { + CreateKeyringTokenStoreOptions, + CreateSecureStoreOptions, + KeyringTokenStore, + MigrateAuthResult, + MigrateLegacyAuthOptions, + SecureStore, + TokenStorageLocation, + TokenStorageResult, + UserRecord, + UserRecordStore, +} from './keyring/index.js' diff --git a/src/auth/keyring/index.ts b/src/auth/keyring/index.ts new file mode 100644 index 0000000..741151b --- /dev/null +++ b/src/auth/keyring/index.ts @@ -0,0 +1,19 @@ +export { + SECURE_STORE_DESCRIPTION, + SecureStoreUnavailableError, + createSecureStore, +} from './secure-store.js' +export type { CreateSecureStoreOptions, SecureStore } from './secure-store.js' + +export { createKeyringTokenStore } from './token-store.js' +export type { CreateKeyringTokenStoreOptions, KeyringTokenStore } from './token-store.js' + +export { migrateLegacyAuth } from './migrate.js' +export type { MigrateAuthResult, MigrateLegacyAuthOptions } from './migrate.js' + +export type { + TokenStorageLocation, + TokenStorageResult, + UserRecord, + UserRecordStore, +} from './types.js' diff --git a/src/auth/keyring/migrate.test.ts b/src/auth/keyring/migrate.test.ts new file mode 100644 index 0000000..e0d2c84 --- /dev/null +++ b/src/auth/keyring/migrate.test.ts @@ -0,0 +1,272 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' + +import { migrateLegacyAuth } from './migrate.js' +import { SecureStoreUnavailableError, type SecureStore } from './secure-store.js' +import type { UserRecord, UserRecordStore } from './types.js' + +vi.mock('./secure-store.js', async () => { + const actual = await vi.importActual('./secure-store.js') + return { + ...actual, + createSecureStore: vi.fn(), + } +}) + +const { createSecureStore } = await import('./secure-store.js') +const mockedCreateSecureStore = vi.mocked(createSecureStore) + +type Account = { + id: string + label?: string + email: string +} + +function buildKeyringMap(): { + create: (args: { serviceName: string; account: string }) => SecureStore + slots: Map< + string, + { secret: string | null; getErr?: unknown; setErr?: unknown; delErr?: unknown } + > +} { + const slots = new Map< + string, + { + secret: string | null + getErr?: unknown + setErr?: unknown + delErr?: unknown + } + >() + function getSlot(account: string) { + let slot = slots.get(account) + if (!slot) { + slot = { secret: null } + slots.set(account, slot) + } + return slot + } + return { + slots, + create({ account }) { + return { + async getSecret() { + const slot = getSlot(account) + if (slot.getErr) throw slot.getErr + return slot.secret + }, + async setSecret(secret) { + const slot = getSlot(account) + if (slot.setErr) throw slot.setErr + slot.secret = secret + }, + async deleteSecret() { + const slot = getSlot(account) + if (slot.delErr) throw slot.delErr + const had = slot.secret !== null + slot.secret = null + return had + }, + } + }, + } +} + +function buildUserRecords(): { + store: UserRecordStore + state: { + records: Map> + defaultId: string | null + } + upsertSpy: ReturnType +} { + const state = { + records: new Map>(), + defaultId: null as string | null, + } + const upsertSpy = vi.fn(async (record: UserRecord) => { + state.records.set(record.id, record) + }) + const store: UserRecordStore = { + async list() { + return [...state.records.values()] + }, + async getById(id) { + return state.records.get(id) ?? null + }, + upsert: upsertSpy, + async remove(id) { + state.records.delete(id) + }, + async getDefaultId() { + return state.defaultId + }, + async setDefaultId(id) { + state.defaultId = id + }, + describeLocation() { + return '/tmp/fake/config.json' + }, + } + return { store, state, upsertSpy } +} + +const SERVICE = 'cli-core-test' +const LEGACY = 'api-token' + +describe('migrateLegacyAuth', () => { + beforeEach(() => { + mockedCreateSecureStore.mockReset() + }) + + it('returns already-migrated when user records already exist', async () => { + mockedCreateSecureStore.mockReturnValue( + buildKeyringMap().create({ serviceName: SERVICE, account: LEGACY }), + ) + const { store: userRecords, state } = buildUserRecords() + state.records.set('1', { id: '1', account: { id: '1', email: 'a@b' } }) + + const result = await migrateLegacyAuth({ + serviceName: SERVICE, + legacyAccount: LEGACY, + userRecords, + loadLegacyPlaintextToken: async () => null, + identifyAccount: async () => ({ id: '1', email: 'a@b' }), + silent: true, + }) + + expect(result.status).toBe('already-migrated') + }) + + it('returns no-legacy-state when neither slot has a token', async () => { + const km = buildKeyringMap() + mockedCreateSecureStore.mockImplementation(km.create) + const { store: userRecords } = buildUserRecords() + + const result = await migrateLegacyAuth({ + serviceName: SERVICE, + legacyAccount: LEGACY, + userRecords, + loadLegacyPlaintextToken: async () => null, + identifyAccount: async () => ({ id: '1', email: 'a@b' }), + silent: true, + }) + + expect(result.status).toBe('no-legacy-state') + }) + + it('migrates a legacy keyring token into a per-user slot and clears the legacy entry', async () => { + const km = buildKeyringMap() + km.slots.set(LEGACY, { secret: 'legacy_tok' }) + mockedCreateSecureStore.mockImplementation(km.create) + const { store: userRecords, state, upsertSpy } = buildUserRecords() + const cleanup = vi.fn(async () => undefined) + + const result = await migrateLegacyAuth({ + serviceName: SERVICE, + legacyAccount: LEGACY, + userRecords, + loadLegacyPlaintextToken: async () => null, + identifyAccount: async (token) => { + expect(token).toBe('legacy_tok') + return { id: '99', email: 'me@x.io', label: 'me@x.io' } + }, + cleanupLegacyConfig: cleanup, + silent: true, + }) + + expect(result.status).toBe('migrated') + expect(result.migratedAccount?.id).toBe('99') + expect(km.slots.get('user-99')?.secret).toBe('legacy_tok') + expect(km.slots.get(LEGACY)?.secret).toBeNull() + expect(state.records.get('99')?.fallbackToken).toBeUndefined() + expect(state.defaultId).toBe('99') + expect(upsertSpy).toHaveBeenCalledTimes(1) + expect(cleanup).toHaveBeenCalledTimes(1) + }) + + it('falls back to loadLegacyPlaintextToken when the legacy keyring slot is empty', async () => { + const km = buildKeyringMap() + mockedCreateSecureStore.mockImplementation(km.create) + const { store: userRecords, state } = buildUserRecords() + + const result = await migrateLegacyAuth({ + serviceName: SERVICE, + legacyAccount: LEGACY, + userRecords, + loadLegacyPlaintextToken: async () => 'plain_legacy', + identifyAccount: async () => ({ id: '7', email: 'p@l.x' }), + silent: true, + }) + + expect(result.status).toBe('migrated') + expect(km.slots.get('user-7')?.secret).toBe('plain_legacy') + expect(state.records.get('7')?.fallbackToken).toBeUndefined() + }) + + it('stores fallbackToken on the record when the per-user keyring write fails', async () => { + const km = buildKeyringMap() + km.slots.set(LEGACY, { secret: 'legacy_tok' }) + km.slots.set('user-99', { + secret: null, + setErr: new SecureStoreUnavailableError('offline'), + }) + mockedCreateSecureStore.mockImplementation(km.create) + const { store: userRecords, state } = buildUserRecords() + + const result = await migrateLegacyAuth({ + serviceName: SERVICE, + legacyAccount: LEGACY, + userRecords, + loadLegacyPlaintextToken: async () => null, + identifyAccount: async () => ({ id: '99', email: 'me@x.io' }), + silent: true, + }) + + expect(result.status).toBe('migrated') + expect(state.records.get('99')?.fallbackToken).toBe('legacy_tok') + }) + + it('returns skipped when identifyAccount throws', async () => { + const km = buildKeyringMap() + km.slots.set(LEGACY, { secret: 'legacy_tok' }) + mockedCreateSecureStore.mockImplementation(km.create) + const { store: userRecords, state } = buildUserRecords() + + const result = await migrateLegacyAuth({ + serviceName: SERVICE, + legacyAccount: LEGACY, + userRecords, + loadLegacyPlaintextToken: async () => null, + identifyAccount: async () => { + throw new Error('HTTP 401') + }, + silent: true, + }) + + expect(result.status).toBe('skipped') + expect(result.reason).toContain('HTTP 401') + expect(state.records.size).toBe(0) + // Legacy entry must remain so a retry can find it. + expect(km.slots.get(LEGACY)?.secret).toBe('legacy_tok') + }) + + it('rolls back the keyring write when user-record upsert fails', async () => { + const km = buildKeyringMap() + km.slots.set(LEGACY, { secret: 'legacy_tok' }) + mockedCreateSecureStore.mockImplementation(km.create) + const { store: userRecords, upsertSpy } = buildUserRecords() + upsertSpy.mockRejectedValueOnce(new Error('disk full')) + + const result = await migrateLegacyAuth({ + serviceName: SERVICE, + legacyAccount: LEGACY, + userRecords, + loadLegacyPlaintextToken: async () => null, + identifyAccount: async () => ({ id: '99', email: 'me@x.io' }), + silent: true, + }) + + expect(result.status).toBe('skipped') + expect(km.slots.get('user-99')?.secret).toBeNull() + }) +}) diff --git a/src/auth/keyring/migrate.ts b/src/auth/keyring/migrate.ts new file mode 100644 index 0000000..5a8a6d5 --- /dev/null +++ b/src/auth/keyring/migrate.ts @@ -0,0 +1,198 @@ +import type { AuthAccount } from '../types.js' +import { createSecureStore, SecureStoreUnavailableError } from './secure-store.js' +import type { UserRecord, UserRecordStore } from './types.js' + +export type MigrateLegacyAuthOptions = { + serviceName: string + /** Legacy single-user keyring account slug, e.g. `'api-token'`. */ + legacyAccount: string + /** v2 user-record store the migrated record is written into. */ + userRecords: UserRecordStore + /** Per-user keyring slug for the new entry. Defaults to `user-${id}`. */ + accountForUser?: (id: string) => string + /** + * Returns the v1 token from the consumer's *plaintext* config slot, or + * `null` if absent. cli-core handles the legacy keyring slot itself. + */ + loadLegacyPlaintextToken: () => Promise + /** + * Identifies the user behind the v1 token. Implementations typically hit + * the product API with the token to fetch the canonical `id` / `email` + * for the new account record. + */ + identifyAccount: (token: string) => Promise + /** + * Optional best-effort cleanup of v1-only config fields after a + * successful migration (e.g. unset legacy `api_token` / `auth_mode`). + * Failures are swallowed; the user record is the source of truth. + */ + cleanupLegacyConfig?: () => Promise + /** Suppress stderr output (postinstall hooks set this). */ + silent?: boolean + /** Label used in the stderr log line. Defaults to `'cli'`. */ + logPrefix?: string +} + +export type MigrateAuthResult = { + status: 'already-migrated' | 'no-legacy-state' | 'migrated' | 'skipped' + reason?: string + migratedAccount?: TAccount +} + +const DEFAULT_ACCOUNT_FOR_USER = (id: string) => `user-${id}` + +/** + * One-time migration of a v1 single-user auth state into a v2 multi-user + * shape. Best-effort: any failure (offline keyring, network error fetching + * the user, identifier mismatch, …) leaves the v1 state untouched so the + * consumer's runtime fallback can keep serving the legacy token until the + * next attempt. + * + * Steps: + * 1. Skip if `userRecords` already has any records. + * 2. Read the v1 token — legacy keyring slot first, then the consumer's + * plaintext slot. + * 3. Identify the user via `identifyAccount(token)`. + * 4. Write the token to the per-user keyring slot (or fall back to a + * `fallbackToken` on the record if keyring is unreachable). + * 5. Upsert the v2 record + set it as the default. + * 6. Best-effort delete of the legacy keyring slot. + * 7. Best-effort `cleanupLegacyConfig()`. + */ +export async function migrateLegacyAuth( + options: MigrateLegacyAuthOptions, +): Promise> { + const { + serviceName, + legacyAccount, + userRecords, + loadLegacyPlaintextToken, + identifyAccount, + cleanupLegacyConfig, + silent, + } = options + const accountForUser = options.accountForUser ?? DEFAULT_ACCOUNT_FOR_USER + const logPrefix = options.logPrefix ?? 'cli' + + const existing = await userRecords.list() + if (existing.length > 0) { + return { status: 'already-migrated' } + } + + const legacyToken = await readLegacyToken({ + serviceName, + legacyAccount, + loadLegacyPlaintextToken, + }) + if (!legacyToken) { + return { status: 'no-legacy-state' } + } + + let account: TAccount + try { + account = await identifyAccount(legacyToken) + } catch (error) { + return skipped(silent, logPrefix, `could not identify user (${describe(error)})`) + } + + const perUserStore = createSecureStore({ + serviceName, + account: accountForUser(account.id), + }) + + let storedSecurely = false + try { + await perUserStore.setSecret(legacyToken) + storedSecurely = true + } catch (error) { + if (!(error instanceof SecureStoreUnavailableError)) { + return skipped( + silent, + logPrefix, + `failed to write user-scoped credential (${describe(error)})`, + ) + } + } + + const record: UserRecord = storedSecurely + ? { id: account.id, account } + : { id: account.id, account, fallbackToken: legacyToken } + + try { + await userRecords.upsert(record) + } catch (error) { + if (storedSecurely) { + try { + await perUserStore.deleteSecret() + } catch { + // best-effort rollback + } + } + return skipped(silent, logPrefix, `failed to update user records (${describe(error)})`) + } + + try { + await userRecords.setDefaultId(account.id) + } catch { + // non-fatal — the record is written; setting a default can be retried later. + } + + try { + const legacyStore = createSecureStore({ serviceName, account: legacyAccount }) + await legacyStore.deleteSecret() + } catch { + // best-effort — legacy slot may already be empty or the keyring may be offline. + } + + if (cleanupLegacyConfig) { + try { + await cleanupLegacyConfig() + } catch { + // best-effort — the user record is the source of truth. + } + } + + if (!silent) { + const label = account.label ?? account.id + console.error(`${logPrefix}: migrated existing token to multi-user store (${label}).`) + } + + return { status: 'migrated', migratedAccount: account } +} + +async function readLegacyToken(opts: { + serviceName: string + legacyAccount: string + loadLegacyPlaintextToken: () => Promise +}): Promise { + try { + const legacyStore = createSecureStore({ + serviceName: opts.serviceName, + account: opts.legacyAccount, + }) + const stored = await legacyStore.getSecret() + if (stored?.trim()) return stored.trim() + } catch (error) { + if (!(error instanceof SecureStoreUnavailableError)) throw error + } + + const plaintext = await opts.loadLegacyPlaintextToken() + if (plaintext?.trim()) return plaintext.trim() + + return null +} + +function describe(error: unknown): string { + return error instanceof Error && error.message ? error.message : String(error) +} + +function skipped( + silent: boolean | undefined, + logPrefix: string, + reason: string, +): MigrateAuthResult { + if (!silent) { + console.error(`${logPrefix}: skipped legacy auth migration — ${reason}.`) + } + return { status: 'skipped', reason } +} diff --git a/src/auth/keyring/secure-store.test.ts b/src/auth/keyring/secure-store.test.ts new file mode 100644 index 0000000..7c06c33 --- /dev/null +++ b/src/auth/keyring/secure-store.test.ts @@ -0,0 +1,104 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' + +const keyringMocks = vi.hoisted(() => { + const entry = { + getPassword: vi.fn(), + setPassword: vi.fn(), + deleteCredential: vi.fn(), + } + return { + AsyncEntry: vi.fn().mockImplementation(function AsyncEntry() { + return entry + }), + entry, + } +}) + +vi.mock('@napi-rs/keyring', () => ({ + AsyncEntry: keyringMocks.AsyncEntry, +})) + +const SERVICE = 'cli-core-test' +const ACCOUNT = 'user-42' + +describe('createSecureStore', () => { + beforeEach(() => { + vi.clearAllMocks() + vi.resetModules() + }) + + it('reads, writes, and deletes secrets via @napi-rs/keyring with the configured service/account', async () => { + keyringMocks.entry.getPassword.mockResolvedValue('tok_abcdef') + keyringMocks.entry.setPassword.mockResolvedValue(undefined) + keyringMocks.entry.deleteCredential.mockResolvedValue(true) + + const { createSecureStore } = await import('./secure-store.js') + const store = createSecureStore({ serviceName: SERVICE, account: ACCOUNT }) + + await expect(store.getSecret()).resolves.toBe('tok_abcdef') + await expect(store.setSecret('tok_abcdef')).resolves.toBeUndefined() + await expect(store.deleteSecret()).resolves.toBe(true) + + expect(keyringMocks.AsyncEntry).toHaveBeenCalledWith(SERVICE, ACCOUNT) + expect(keyringMocks.entry.setPassword).toHaveBeenCalledWith('tok_abcdef') + }) + + it('returns null when the keyring has no entry for the slot', async () => { + keyringMocks.entry.getPassword.mockResolvedValue(null) + + const { createSecureStore } = await import('./secure-store.js') + const store = createSecureStore({ serviceName: SERVICE, account: ACCOUNT }) + + await expect(store.getSecret()).resolves.toBeNull() + }) + + it('wraps a get failure as SecureStoreUnavailableError', async () => { + keyringMocks.entry.getPassword.mockRejectedValue(new Error('Keychain locked')) + + const { createSecureStore, SecureStoreUnavailableError } = await import('./secure-store.js') + + await expect( + createSecureStore({ serviceName: SERVICE, account: ACCOUNT }).getSecret(), + ).rejects.toBeInstanceOf(SecureStoreUnavailableError) + }) + + it('wraps a set failure as SecureStoreUnavailableError', async () => { + keyringMocks.entry.setPassword.mockRejectedValue(new Error('libsecret missing')) + + const { createSecureStore, SecureStoreUnavailableError } = await import('./secure-store.js') + + await expect( + createSecureStore({ serviceName: SERVICE, account: ACCOUNT }).setSecret('x'), + ).rejects.toBeInstanceOf(SecureStoreUnavailableError) + }) + + it('wraps a delete failure as SecureStoreUnavailableError', async () => { + keyringMocks.entry.deleteCredential.mockRejectedValue(new Error('D-Bus down')) + + const { createSecureStore, SecureStoreUnavailableError } = await import('./secure-store.js') + + await expect( + createSecureStore({ serviceName: SERVICE, account: ACCOUNT }).deleteSecret(), + ).rejects.toBeInstanceOf(SecureStoreUnavailableError) + }) +}) + +describe('createSecureStore — missing native binary', () => { + beforeEach(() => { + vi.resetModules() + }) + + it('surfaces an import failure as SecureStoreUnavailableError instead of crashing module load', async () => { + vi.doMock('@napi-rs/keyring', () => { + throw new Error('no native binary for this arch') + }) + + const { createSecureStore, SecureStoreUnavailableError } = await import('./secure-store.js') + + await expect( + createSecureStore({ serviceName: SERVICE, account: ACCOUNT }).getSecret(), + ).rejects.toBeInstanceOf(SecureStoreUnavailableError) + + vi.doUnmock('@napi-rs/keyring') + }) +}) diff --git a/src/auth/keyring/secure-store.ts b/src/auth/keyring/secure-store.ts new file mode 100644 index 0000000..4aad8e6 --- /dev/null +++ b/src/auth/keyring/secure-store.ts @@ -0,0 +1,95 @@ +export const SECURE_STORE_DESCRIPTION = 'system credential manager' + +/** + * Thrown when the OS credential manager cannot be reached — missing native + * binary for the current architecture, libsecret/D-Bus unavailable + * (common in WSL / headless Linux / containers / CI), Keychain locked, or + * any other error returned by the underlying keyring backend. + * + * Callers that want to degrade to a plaintext fallback should catch this + * specific class rather than swallowing every `Error` — anything else + * coming out of `SecureStore` is a programmer error. + */ +export class SecureStoreUnavailableError extends Error { + constructor(message = 'System credential storage is unavailable') { + super(message) + this.name = 'SecureStoreUnavailableError' + } +} + +export type SecureStore = { + getSecret(): Promise + setSecret(secret: string): Promise + deleteSecret(): Promise +} + +export type CreateSecureStoreOptions = { + /** Stable per-application identifier (Keychain "service", Credential Manager "target prefix", libsecret "service"). */ + serviceName: string + /** Per-credential identifier within the service. For multi-account CLIs, typically `user-${id}`. */ + account: string +} + +/** + * Thin wrapper around `@napi-rs/keyring` that normalizes every failure mode + * into `SecureStoreUnavailableError`. `serviceName` + `account` together + * identify one credential slot in the OS keyring. + */ +export function createSecureStore(options: CreateSecureStoreOptions): SecureStore { + const { serviceName, account } = options + + return { + async getSecret(): Promise { + const entry = await getEntry(serviceName, account) + try { + return (await entry.getPassword()) ?? null + } catch (error) { + throw toUnavailableError(error) + } + }, + + async setSecret(secret: string): Promise { + const entry = await getEntry(serviceName, account) + try { + await entry.setPassword(secret) + } catch (error) { + throw toUnavailableError(error) + } + }, + + async deleteSecret(): Promise { + const entry = await getEntry(serviceName, account) + try { + return await entry.deleteCredential() + } catch (error) { + throw toUnavailableError(error) + } + }, + } +} + +async function getEntry( + serviceName: string, + account: string, +): Promise { + try { + // Dynamic import: `@napi-rs/keyring` is an optional dependency. On + // unsupported architectures (or when the native binary failed to + // install) a static import would crash module load before we get a + // chance to surface `SecureStoreUnavailableError` and let the caller + // fall back to plaintext config storage. + const { AsyncEntry } = await import('@napi-rs/keyring') + return new AsyncEntry(serviceName, account) + } catch (error) { + throw toUnavailableError(error) + } +} + +function toUnavailableError(error: unknown): SecureStoreUnavailableError { + if (error instanceof SecureStoreUnavailableError) { + return error + } + const message = + error instanceof Error ? error.message : 'System credential storage is unavailable' + return new SecureStoreUnavailableError(message) +} diff --git a/src/auth/keyring/token-store.test.ts b/src/auth/keyring/token-store.test.ts new file mode 100644 index 0000000..add0442 --- /dev/null +++ b/src/auth/keyring/token-store.test.ts @@ -0,0 +1,425 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' + +import { SecureStoreUnavailableError, type SecureStore } from './secure-store.js' +import { createKeyringTokenStore } from './token-store.js' +import type { UserRecord, UserRecordStore } from './types.js' + +vi.mock('./secure-store.js', async () => { + const actual = await vi.importActual('./secure-store.js') + return { + ...actual, + createSecureStore: vi.fn(), + } +}) + +const { createSecureStore } = await import('./secure-store.js') +const mockedCreateSecureStore = vi.mocked(createSecureStore) + +type Account = { + id: string + label?: string + email: string +} + +function buildSecureStoreMock(initial: { secret?: string | null } = {}): SecureStore & { + _state: { secret: string | null } + getSpy: ReturnType + setSpy: ReturnType + deleteSpy: ReturnType +} { + const state = { secret: initial.secret ?? null } + const getSpy = vi.fn(async () => state.secret) + const setSpy = vi.fn(async (secret: string) => { + state.secret = secret + }) + const deleteSpy = vi.fn(async () => { + const had = state.secret !== null + state.secret = null + return had + }) + return { + _state: state, + getSpy, + setSpy, + deleteSpy, + async getSecret() { + return getSpy() + }, + async setSecret(secret: string) { + return setSpy(secret) + }, + async deleteSecret() { + return deleteSpy() + }, + } +} + +function buildUserRecords(): { + store: UserRecordStore + state: { + records: Map> + defaultId: string | null + } + upsertSpy: ReturnType + removeSpy: ReturnType +} { + const state = { + records: new Map>(), + defaultId: null as string | null, + } + const upsertSpy = vi.fn(async (record: UserRecord) => { + state.records.set(record.id, record) + }) + const removeSpy = vi.fn(async (id: string) => { + state.records.delete(id) + }) + const store: UserRecordStore = { + async list() { + return [...state.records.values()] + }, + async getById(id) { + return state.records.get(id) ?? null + }, + upsert: upsertSpy, + remove: removeSpy, + async getDefaultId() { + return state.defaultId + }, + async setDefaultId(id) { + state.defaultId = id + }, + describeLocation() { + return '/tmp/fake/config.json' + }, + } + return { store, state, upsertSpy, removeSpy } +} + +const SERVICE = 'cli-core-test' +const account: Account = { id: '42', label: 'me', email: 'a@b.c' } + +describe('createKeyringTokenStore', () => { + beforeEach(() => { + mockedCreateSecureStore.mockReset() + }) + + it('round-trips set → active → clear when the keyring is online', async () => { + const keyring = buildSecureStoreMock() + mockedCreateSecureStore.mockReturnValue(keyring) + const { store: userRecords, state, upsertSpy } = buildUserRecords() + + const store = createKeyringTokenStore({ serviceName: SERVICE, userRecords }) + + await store.set(account, 'tok_secret') + expect(keyring.setSpy).toHaveBeenCalledWith('tok_secret') + expect(upsertSpy).toHaveBeenCalledWith({ id: '42', account }) + expect(state.defaultId).toBe('42') + expect(store.getLastStorageResult()).toEqual({ storage: 'secure-store' }) + + const active = await store.active() + expect(active).toEqual({ token: 'tok_secret', account }) + + await store.clear() + expect(keyring.deleteSpy).toHaveBeenCalledTimes(1) + expect(state.records.size).toBe(0) + expect(state.defaultId).toBeNull() + expect(store.getLastClearResult()).toEqual({ storage: 'secure-store' }) + }) + + it('falls back to a plaintext token on the user record when the keyring is offline', async () => { + const keyring = buildSecureStoreMock() + keyring.setSpy.mockRejectedValueOnce(new SecureStoreUnavailableError('no dbus')) + mockedCreateSecureStore.mockReturnValue(keyring) + const { store: userRecords, state } = buildUserRecords() + + const store = createKeyringTokenStore({ serviceName: SERVICE, userRecords }) + + await store.set(account, 'tok_plain') + + const record = state.records.get('42') + expect(record?.fallbackToken).toBe('tok_plain') + expect(store.getLastStorageResult()).toEqual({ + storage: 'config-file', + warning: + 'system credential manager unavailable; token saved as plaintext in /tmp/fake/config.json', + }) + + const active = await store.active() + expect(active).toEqual({ token: 'tok_plain', account }) + expect(keyring.getSpy).not.toHaveBeenCalled() + }) + + it('rolls back the keyring write when the user record upsert fails', async () => { + const keyring = buildSecureStoreMock() + mockedCreateSecureStore.mockReturnValue(keyring) + const { store: userRecords, upsertSpy } = buildUserRecords() + upsertSpy.mockRejectedValueOnce(new Error('disk full')) + + const store = createKeyringTokenStore({ serviceName: SERVICE, userRecords }) + + await expect(store.set(account, 'tok')).rejects.toThrow('disk full') + expect(keyring.setSpy).toHaveBeenCalledWith('tok') + expect(keyring.deleteSpy).toHaveBeenCalledTimes(1) + }) + + it('returns null from active() when the keyring is unreachable mid-session', async () => { + const keyring = buildSecureStoreMock({ secret: 'tok' }) + keyring.getSpy.mockRejectedValueOnce(new SecureStoreUnavailableError('locked')) + mockedCreateSecureStore.mockReturnValue(keyring) + const { store: userRecords, state } = buildUserRecords() + state.records.set('42', { id: '42', account }) + state.defaultId = '42' + + const store = createKeyringTokenStore({ serviceName: SERVICE, userRecords }) + + await expect(store.active()).resolves.toBeNull() + }) + + it('returns null from active() when no records exist', async () => { + mockedCreateSecureStore.mockReturnValue(buildSecureStoreMock()) + const { store: userRecords } = buildUserRecords() + + const store = createKeyringTokenStore({ serviceName: SERVICE, userRecords }) + + await expect(store.active()).resolves.toBeNull() + }) + + it('picks the lone user when no default is set', async () => { + const keyring = buildSecureStoreMock({ secret: 'tok' }) + mockedCreateSecureStore.mockReturnValue(keyring) + const { store: userRecords, state } = buildUserRecords() + state.records.set('42', { id: '42', account }) + + const store = createKeyringTokenStore({ serviceName: SERVICE, userRecords }) + + await expect(store.active()).resolves.toEqual({ token: 'tok', account }) + }) + + it('returns null when multiple users exist and no default is set', async () => { + mockedCreateSecureStore.mockReturnValue(buildSecureStoreMock()) + const { store: userRecords, state } = buildUserRecords() + state.records.set('1', { id: '1', account: { ...account, id: '1' } }) + state.records.set('2', { id: '2', account: { ...account, id: '2' } }) + + const store = createKeyringTokenStore({ serviceName: SERVICE, userRecords }) + + await expect(store.active()).resolves.toBeNull() + }) + + it('does not overwrite an existing default when a second user is added', async () => { + const keyring = buildSecureStoreMock() + mockedCreateSecureStore.mockReturnValue(keyring) + const { store: userRecords, state } = buildUserRecords() + state.defaultId = '1' + state.records.set('1', { id: '1', account: { ...account, id: '1' } }) + + const store = createKeyringTokenStore({ serviceName: SERVICE, userRecords }) + + await store.set({ ...account, id: '2' }, 'tok_b') + + expect(state.defaultId).toBe('1') + }) + + it('clear() is a no-op when no record exists', async () => { + mockedCreateSecureStore.mockReturnValue(buildSecureStoreMock()) + const { store: userRecords } = buildUserRecords() + + const store = createKeyringTokenStore({ serviceName: SERVICE, userRecords }) + + await expect(store.clear()).resolves.toBeUndefined() + expect(store.getLastClearResult()).toBeUndefined() + }) + + it('clear() with a fallback-token record skips the keyring delete', async () => { + const keyring = buildSecureStoreMock() + mockedCreateSecureStore.mockReturnValue(keyring) + const { store: userRecords, state } = buildUserRecords() + state.records.set('42', { id: '42', account, fallbackToken: 'tok_plain' }) + state.defaultId = '42' + + const store = createKeyringTokenStore({ serviceName: SERVICE, userRecords }) + + await store.clear() + + expect(keyring.deleteSpy).not.toHaveBeenCalled() + expect(state.records.size).toBe(0) + expect(store.getLastClearResult()).toEqual({ + storage: 'config-file', + warning: + 'system credential manager unavailable; local auth state cleared in /tmp/fake/config.json', + }) + }) + + it('clear() downgrades to a warning when the keyring delete fails', async () => { + const keyring = buildSecureStoreMock({ secret: 'tok' }) + keyring.deleteSpy.mockRejectedValueOnce(new SecureStoreUnavailableError('offline')) + mockedCreateSecureStore.mockReturnValue(keyring) + const { store: userRecords, state } = buildUserRecords() + state.records.set('42', { id: '42', account }) + state.defaultId = '42' + + const store = createKeyringTokenStore({ serviceName: SERVICE, userRecords }) + + await store.clear() + + expect(state.records.size).toBe(0) + expect(store.getLastClearResult()).toMatchObject({ storage: 'config-file' }) + }) + + it('uses a custom accountForUser slug when provided', async () => { + const keyring = buildSecureStoreMock() + mockedCreateSecureStore.mockReturnValue(keyring) + const { store: userRecords } = buildUserRecords() + + const store = createKeyringTokenStore({ + serviceName: SERVICE, + userRecords, + accountForUser: (id) => `custom-${id}`, + }) + + await store.set(account, 'tok') + + expect(mockedCreateSecureStore).toHaveBeenCalledWith({ + serviceName: SERVICE, + account: 'custom-42', + }) + }) + + describe('AccountRef support', () => { + it('active(ref) matches on id', async () => { + const keyring = buildSecureStoreMock({ secret: 'tok_a' }) + mockedCreateSecureStore.mockReturnValue(keyring) + const { store: userRecords, state } = buildUserRecords() + state.records.set('1', { id: '1', account: { id: '1', label: 'me', email: 'a@b' } }) + state.records.set('2', { + id: '2', + account: { id: '2', label: 'you', email: 'c@d' }, + fallbackToken: 'tok_b', + }) + + const store = createKeyringTokenStore({ serviceName: SERVICE, userRecords }) + + const snapshot = await store.active('2') + expect(snapshot?.account.id).toBe('2') + expect(snapshot?.token).toBe('tok_b') + }) + + it('active(ref) matches on label', async () => { + const keyring = buildSecureStoreMock({ secret: 'tok' }) + mockedCreateSecureStore.mockReturnValue(keyring) + const { store: userRecords, state } = buildUserRecords() + state.records.set('1', { id: '1', account: { id: '1', label: 'alice', email: 'a@b' } }) + + const store = createKeyringTokenStore({ serviceName: SERVICE, userRecords }) + + const snapshot = await store.active('alice') + expect(snapshot?.account.id).toBe('1') + }) + + it('active(ref) returns null on a miss (attacher translates to ACCOUNT_NOT_FOUND)', async () => { + mockedCreateSecureStore.mockReturnValue(buildSecureStoreMock()) + const { store: userRecords, state } = buildUserRecords() + state.records.set('1', { id: '1', account }) + + const store = createKeyringTokenStore({ serviceName: SERVICE, userRecords }) + + await expect(store.active('does-not-exist')).resolves.toBeNull() + }) + + it('clear(ref) removes only the matching record', async () => { + const keyring = buildSecureStoreMock() + mockedCreateSecureStore.mockReturnValue(keyring) + const { store: userRecords, state } = buildUserRecords() + state.records.set('1', { id: '1', account: { id: '1', label: 'a', email: 'a@b' } }) + state.records.set('2', { id: '2', account: { id: '2', label: 'b', email: 'c@d' } }) + state.defaultId = '1' + + const store = createKeyringTokenStore({ serviceName: SERVICE, userRecords }) + + await store.clear('2') + + expect(state.records.has('1')).toBe(true) + expect(state.records.has('2')).toBe(false) + expect(state.defaultId).toBe('1') + }) + + it('clear(ref) on a miss is a no-op (attacher rejects via active() pre-check)', async () => { + mockedCreateSecureStore.mockReturnValue(buildSecureStoreMock()) + const { store: userRecords, state } = buildUserRecords() + state.records.set('1', { id: '1', account }) + + const store = createKeyringTokenStore({ serviceName: SERVICE, userRecords }) + + await store.clear('nope') + expect(state.records.has('1')).toBe(true) + }) + + it('honours a custom matchAccount predicate', async () => { + const keyring = buildSecureStoreMock({ secret: 'tok' }) + mockedCreateSecureStore.mockReturnValue(keyring) + const { store: userRecords, state } = buildUserRecords() + state.records.set('1', { id: '1', account: { id: '1', email: 'Alice@x.io' } }) + + const store = createKeyringTokenStore({ + serviceName: SERVICE, + userRecords, + matchAccount: (acc, ref) => acc.email.toLowerCase() === ref.toLowerCase(), + }) + + await expect(store.active('alice@x.io')).resolves.toMatchObject({ + account: { id: '1' }, + }) + }) + }) + + describe('list() + setDefault()', () => { + it('list() returns every account with the default marker', async () => { + mockedCreateSecureStore.mockReturnValue(buildSecureStoreMock()) + const { store: userRecords, state } = buildUserRecords() + state.records.set('1', { id: '1', account: { id: '1', email: 'a@b' } }) + state.records.set('2', { id: '2', account: { id: '2', email: 'c@d' } }) + state.defaultId = '2' + + const store = createKeyringTokenStore({ serviceName: SERVICE, userRecords }) + + const all = await store.list() + expect(all).toHaveLength(2) + expect(all.find((entry) => entry.account.id === '2')?.isDefault).toBe(true) + expect(all.find((entry) => entry.account.id === '1')?.isDefault).toBe(false) + }) + + it('list() returns an empty array when nothing is stored', async () => { + mockedCreateSecureStore.mockReturnValue(buildSecureStoreMock()) + const { store: userRecords } = buildUserRecords() + + const store = createKeyringTokenStore({ serviceName: SERVICE, userRecords }) + + await expect(store.list()).resolves.toEqual([]) + }) + + it('setDefault(ref) marks the matching account as default', async () => { + mockedCreateSecureStore.mockReturnValue(buildSecureStoreMock()) + const { store: userRecords, state } = buildUserRecords() + state.records.set('1', { id: '1', account: { id: '1', label: 'a', email: 'a@b' } }) + state.records.set('2', { id: '2', account: { id: '2', label: 'b', email: 'c@d' } }) + state.defaultId = '1' + + const store = createKeyringTokenStore({ serviceName: SERVICE, userRecords }) + + await store.setDefault('b') + expect(state.defaultId).toBe('2') + }) + + it('setDefault(ref) throws ACCOUNT_NOT_FOUND on a miss', async () => { + mockedCreateSecureStore.mockReturnValue(buildSecureStoreMock()) + const { store: userRecords, state } = buildUserRecords() + state.records.set('1', { id: '1', account }) + + const store = createKeyringTokenStore({ serviceName: SERVICE, userRecords }) + + await expect(store.setDefault('nope')).rejects.toMatchObject({ + code: 'ACCOUNT_NOT_FOUND', + }) + }) + }) +}) diff --git a/src/auth/keyring/token-store.ts b/src/auth/keyring/token-store.ts new file mode 100644 index 0000000..be5a282 --- /dev/null +++ b/src/auth/keyring/token-store.ts @@ -0,0 +1,231 @@ +import { CliError } from '../../errors.js' +import type { AccountRef, AuthAccount, TokenStore } from '../types.js' +import { + createSecureStore, + SECURE_STORE_DESCRIPTION, + SecureStoreUnavailableError, + type SecureStore, +} from './secure-store.js' +import type { TokenStorageResult, UserRecord, UserRecordStore } from './types.js' + +export type CreateKeyringTokenStoreOptions = { + /** Application identifier used for every keyring entry (e.g. `'todoist-cli'`). */ + serviceName: string + /** Consumer-owned per-user record store (typically backed by their config file). */ + userRecords: UserRecordStore + /** + * Builds the keyring `account` slug for a user id. Defaults to + * `user-${id}`. Override only when migrating from a legacy naming scheme. + */ + accountForUser?: (id: string) => string + /** + * Decides whether an account matches an `AccountRef` supplied via + * `--user `. Defaults to id-or-label equality. Override to broaden + * (e.g. case-insensitive email, alias map). + */ + matchAccount?: (account: TAccount, ref: AccountRef) => boolean +} + +export type KeyringTokenStore = TokenStore & { + /** Storage result from the most recent `set()` call, or `undefined` before any. */ + getLastStorageResult(): TokenStorageResult | undefined + /** Storage result from the most recent `clear()` call, or `undefined` before any. */ + getLastClearResult(): TokenStorageResult | undefined +} + +const DEFAULT_ACCOUNT_FOR_USER = (id: string) => `user-${id}` +const DEFAULT_MATCH_ACCOUNT = ( + account: TAccount, + ref: AccountRef, +): boolean => account.id === ref || account.label === ref + +/** + * Multi-account `TokenStore` that keeps secrets in the OS credential manager + * and per-user metadata in the consumer's `UserRecordStore`. Falls back to a + * plaintext token on the user record when the keyring is unreachable (WSL + * without D-Bus, missing native binary, locked Keychain, …) so the CLI keeps + * working at the cost of a visible warning. + * + * Write order is deliberate: keyring first, then `userRecords.upsert`. If the + * upsert fails after a successful keyring write, the keyring entry is rolled + * back via `deleteSecret()` to avoid orphan credentials for a user that + * cli-core never managed to record. + * + * Clear order is the inverse: record removal first (the source of truth that + * the rest of the CLI reads), then keyring delete. A keyring delete failure + * after a successful removal is downgraded to a warning — the orphan secret + * is harmless because no record references it anymore. + */ +export function createKeyringTokenStore( + options: CreateKeyringTokenStoreOptions, +): KeyringTokenStore { + const { serviceName, userRecords } = options + const accountForUser = options.accountForUser ?? DEFAULT_ACCOUNT_FOR_USER + const matchAccount = options.matchAccount ?? DEFAULT_MATCH_ACCOUNT + + let lastStorageResult: TokenStorageResult | undefined + let lastClearResult: TokenStorageResult | undefined + + function secureStoreFor(id: string): SecureStore { + return createSecureStore({ serviceName, account: accountForUser(id) }) + } + + async function findByRef(ref: AccountRef): Promise | null> { + const all = await userRecords.list() + return all.find((record) => matchAccount(record.account, ref)) ?? null + } + + async function resolveDefaultRecord(): Promise | null> { + const defaultId = await userRecords.getDefaultId() + if (defaultId) { + const record = await userRecords.getById(defaultId) + if (record) return record + } + const all = await userRecords.list() + if (all.length === 1) return all[0] + return null + } + + async function readToken(record: UserRecord): Promise { + if (record.fallbackToken?.trim()) { + return record.fallbackToken.trim() + } + try { + const token = await secureStoreFor(record.id).getSecret() + return token?.trim() ? token.trim() : null + } catch (error) { + if (error instanceof SecureStoreUnavailableError) return null + throw error + } + } + + return { + async active(ref) { + const record = ref === undefined ? await resolveDefaultRecord() : await findByRef(ref) + if (!record) return null + + const token = await readToken(record) + if (!token) return null + return { token, account: record.account } + }, + + async set(account, token) { + const trimmed = token.trim() + const secureStore = secureStoreFor(account.id) + + let storedSecurely = false + try { + await secureStore.setSecret(trimmed) + storedSecurely = true + } catch (error) { + if (!(error instanceof SecureStoreUnavailableError)) throw error + } + + const record: UserRecord = storedSecurely + ? { id: account.id, account } + : { id: account.id, account, fallbackToken: trimmed } + + try { + await userRecords.upsert(record) + } catch (error) { + // The user record is the source of truth. If we can't write it, + // a stranded keyring entry would leak credentials for an account + // cli-core never managed to register. Best-effort rollback, then + // re-raise the original error so the caller sees the real cause. + if (storedSecurely) { + try { + await secureStore.deleteSecret() + } catch { + // ignore — the user record failure is what matters + } + } + throw error + } + + const existingDefault = await userRecords.getDefaultId() + if (!existingDefault) { + await userRecords.setDefaultId(account.id) + } + + lastStorageResult = storedSecurely + ? { storage: 'secure-store' } + : { + storage: 'config-file', + warning: buildFallbackWarning( + 'token saved as plaintext in', + userRecords.describeLocation(), + ), + } + }, + + async clear(ref) { + const record = ref === undefined ? await resolveDefaultRecord() : await findByRef(ref) + if (!record) { + lastClearResult = undefined + return + } + + await userRecords.remove(record.id) + + const currentDefault = await userRecords.getDefaultId() + if (currentDefault === record.id) { + await userRecords.setDefaultId(null) + } + + // No keyring entry to delete when the token was already plaintext. + if (record.fallbackToken !== undefined) { + lastClearResult = { + storage: 'config-file', + warning: buildFallbackWarning( + 'local auth state cleared in', + userRecords.describeLocation(), + ), + } + return + } + + try { + await secureStoreFor(record.id).deleteSecret() + lastClearResult = { storage: 'secure-store' } + } catch (error) { + if (!(error instanceof SecureStoreUnavailableError)) throw error + lastClearResult = { + storage: 'config-file', + warning: buildFallbackWarning( + 'local auth state cleared in', + userRecords.describeLocation(), + ), + } + } + }, + + async list() { + const all = await userRecords.list() + const defaultId = await userRecords.getDefaultId() + return all.map((record) => ({ + account: record.account, + isDefault: record.id === defaultId, + })) + }, + + async setDefault(ref) { + const record = await findByRef(ref) + if (!record) { + throw new CliError('ACCOUNT_NOT_FOUND', `No stored account matches "${ref}".`) + } + await userRecords.setDefaultId(record.id) + }, + + getLastStorageResult() { + return lastStorageResult + }, + + getLastClearResult() { + return lastClearResult + }, + } +} + +function buildFallbackWarning(action: string, location: string): string { + return `${SECURE_STORE_DESCRIPTION} unavailable; ${action} ${location}` +} diff --git a/src/auth/keyring/types.ts b/src/auth/keyring/types.ts new file mode 100644 index 0000000..d85563c --- /dev/null +++ b/src/auth/keyring/types.ts @@ -0,0 +1,47 @@ +import type { AuthAccount } from '../types.js' + +/** Where a token was (or wasn't) persisted on the most recent write/clear. */ +export type TokenStorageLocation = 'secure-store' | 'config-file' + +export type TokenStorageResult = { + storage: TokenStorageLocation + /** + * Present when the OS keyring was unavailable and the operation fell back + * to (or left state in) the consumer's config file. Suitable for surfacing + * to the user as a `Warning:` line on stderr. + */ + warning?: string +} + +export type UserRecord = { + id: string + account: TAccount + /** + * Plaintext token, present only when the keyring was unavailable at write + * time. cli-core prefers a keyring read over this field; consumers should + * surface its presence as a security-relevant fact (it is the same + * material that would otherwise live in the OS credential manager). + */ + fallbackToken?: string +} + +/** + * Port the consumer implements to expose their per-user config records to + * cli-core's keyring-backed `TokenStore`. The shape of the record map (file + * format, path, schema versioning) stays in the consumer — cli-core only + * needs CRUD on these primitives plus a default-user pointer. + */ +export type UserRecordStore = { + list(): Promise[]> + getById(id: string): Promise | null> + upsert(record: UserRecord): Promise + remove(id: string): Promise + getDefaultId(): Promise + setDefaultId(id: string | null): Promise + /** + * Human-readable location used in the fallback warning text (e.g. + * `~/.config/todoist-cli/config.json`). Plain string; cli-core does not + * interpret it. + */ + describeLocation(): string +} From 565fbeedde611673bf86f16c6527504222ec0ff0 Mon Sep 17 00:00:00 2001 From: Scott Lovegrove Date: Sat, 16 May 2026 11:17:13 +0100 Subject: [PATCH 2/4] fix(auth): address keyring TokenStore review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit P2: - `clear()` always attempts the keyring delete, even when the record carries a `fallbackToken`, so an orphan secret from an earlier online write can't survive. - `active()` throws `AUTH_STORE_READ_FAILED` when a matching record exists but the keyring can't be read, instead of returning `null` (which would otherwise translate to a misleading `ACCOUNT_NOT_FOUND` on `--user `). - `set()` makes the default-user promotion best-effort so a `setDefaultId` failure doesn't undo a successful credential write via `AUTH_STORE_WRITE_FAILED`. - `active()` and `clear()` resolve the default record from a single `list`+`getDefaultId` snapshot (was 2–3 sequential reads). - `DEFAULT_ACCOUNT_FOR_USER` lives in `secure-store.ts` and is shared by `createKeyringTokenStore` and `migrateLegacyAuth` so a future rename can't park tokens in a slot the runtime no longer reads. - `UserRecordStore.upsert` is now documented as replace-not-merge so a consumer's merge impl can't strand a stale `fallbackToken` on top of a later keyring-backed write. - `migrateLegacyAuth` logs only `account.id` (was `label`, which is typically an email — PII on stderr). - Multi-account tests use a keyed `createSecureStore` mock so per-user slot routing is actually exercised; offline-keyring migration case added. - `vi.doUnmock` moved into `afterEach` so the mock is torn down even when an assertion fails earlier in the block. P3: - `migrateLegacyAuth` uses the shared `getErrorMessage` util. - `setDefault` and `requireSnapshotForRef` go through a shared `accountNotFoundError(ref)` helper. - README clarifies that `@napi-rs/keyring` arrives transitively via cli-core's own `optionalDependencies`. Co-Authored-By: Claude Opus 4.7 (1M context) --- README.md | 2 +- src/auth/errors.ts | 1 + src/auth/keyring/index.ts | 1 + src/auth/keyring/migrate.test.ts | 31 ++++ src/auth/keyring/migrate.ts | 29 ++-- src/auth/keyring/secure-store.test.ts | 11 +- src/auth/keyring/secure-store.ts | 7 + src/auth/keyring/token-store.test.ts | 197 ++++++++++++++++++-------- src/auth/keyring/token-store.ts | 125 +++++++++------- src/auth/keyring/types.ts | 7 + src/auth/user-flag.ts | 7 +- 11 files changed, 289 insertions(+), 129 deletions(-) diff --git a/README.md b/README.md index 49c63a4..e3f70e8 100644 --- a/README.md +++ b/README.md @@ -334,7 +334,7 @@ if (storage?.warning) console.error('Warning:', storage.warning) The returned store satisfies the full `TokenStore` contract — including `list()` / `setDefault(ref)` / `ref`-aware `active` / `clear` — so it plugs straight into the `logout` / `status` / `token` attachers. Default ref matching is `account.id === ref || account.label === ref`; override `matchAccount` to broaden it (e.g. email-case-insensitive). -Add `@napi-rs/keyring` as an optional/peer dep of your CLI. The library ships pre-built native binaries for Windows, macOS, and Linux (glibc + musl); cli-core dynamic-imports it so a missing binary on an exotic arch surfaces as `SecureStoreUnavailableError` and triggers the same plaintext fallback path rather than crashing module load. +`@napi-rs/keyring` is already declared in cli-core's own `optionalDependencies`, so npm pulls it in transitively when you install `@doist/cli-core` — your consumer CLI does not need to add it explicitly. The library ships pre-built native binaries for Windows, macOS, and Linux (glibc + musl); cli-core dynamic-imports it so a missing binary on an exotic arch surfaces as `SecureStoreUnavailableError` and triggers the same plaintext fallback path rather than crashing module load. For one-time migration of a v1 single-user token into the v2 multi-user shape, use `migrateLegacyAuth` from a postinstall hook: diff --git a/src/auth/errors.ts b/src/auth/errors.ts index 26d6318..bf77311 100644 --- a/src/auth/errors.ts +++ b/src/auth/errors.ts @@ -9,6 +9,7 @@ export type AuthErrorCode = | 'AUTH_PORT_BIND_FAILED' | 'AUTH_TOKEN_EXCHANGE_FAILED' | 'AUTH_STORE_WRITE_FAILED' + | 'AUTH_STORE_READ_FAILED' | 'NOT_AUTHENTICATED' | 'TOKEN_FROM_ENV' | 'NO_ACCOUNT_SELECTED' diff --git a/src/auth/keyring/index.ts b/src/auth/keyring/index.ts index 741151b..44778b2 100644 --- a/src/auth/keyring/index.ts +++ b/src/auth/keyring/index.ts @@ -1,4 +1,5 @@ export { + DEFAULT_ACCOUNT_FOR_USER, SECURE_STORE_DESCRIPTION, SecureStoreUnavailableError, createSecureStore, diff --git a/src/auth/keyring/migrate.test.ts b/src/auth/keyring/migrate.test.ts index e0d2c84..9a13d4b 100644 --- a/src/auth/keyring/migrate.test.ts +++ b/src/auth/keyring/migrate.test.ts @@ -203,6 +203,37 @@ describe('migrateLegacyAuth', () => { expect(state.records.get('7')?.fallbackToken).toBeUndefined() }) + it('migrates against an entirely offline keyring (WSL/headless)', async () => { + const km = buildKeyringMap() + // The whole keyring is dead: reading the legacy slot throws and + // writing the per-user slot would too. Migration must still complete + // by sourcing the token from the consumer's plaintext slot and + // parking it on the user record as `fallbackToken`. + km.slots.set(LEGACY, { + secret: null, + getErr: new SecureStoreUnavailableError('no dbus'), + }) + km.slots.set('user-7', { + secret: null, + setErr: new SecureStoreUnavailableError('no dbus'), + }) + mockedCreateSecureStore.mockImplementation(km.create) + const { store: userRecords, state } = buildUserRecords() + + const result = await migrateLegacyAuth({ + serviceName: SERVICE, + legacyAccount: LEGACY, + userRecords, + loadLegacyPlaintextToken: async () => 'plain_legacy', + identifyAccount: async () => ({ id: '7', email: 'p@l.x' }), + silent: true, + }) + + expect(result.status).toBe('migrated') + expect(state.records.get('7')?.fallbackToken).toBe('plain_legacy') + expect(state.defaultId).toBe('7') + }) + it('stores fallbackToken on the record when the per-user keyring write fails', async () => { const km = buildKeyringMap() km.slots.set(LEGACY, { secret: 'legacy_tok' }) diff --git a/src/auth/keyring/migrate.ts b/src/auth/keyring/migrate.ts index 5a8a6d5..8a2bb24 100644 --- a/src/auth/keyring/migrate.ts +++ b/src/auth/keyring/migrate.ts @@ -1,5 +1,10 @@ +import { getErrorMessage } from '../../errors.js' import type { AuthAccount } from '../types.js' -import { createSecureStore, SecureStoreUnavailableError } from './secure-store.js' +import { + createSecureStore, + DEFAULT_ACCOUNT_FOR_USER, + SecureStoreUnavailableError, +} from './secure-store.js' import type { UserRecord, UserRecordStore } from './types.js' export type MigrateLegacyAuthOptions = { @@ -39,8 +44,6 @@ export type MigrateAuthResult = { migratedAccount?: TAccount } -const DEFAULT_ACCOUNT_FOR_USER = (id: string) => `user-${id}` - /** * One-time migration of a v1 single-user auth state into a v2 multi-user * shape. Best-effort: any failure (offline keyring, network error fetching @@ -92,7 +95,7 @@ export async function migrateLegacyAuth( try { account = await identifyAccount(legacyToken) } catch (error) { - return skipped(silent, logPrefix, `could not identify user (${describe(error)})`) + return skipped(silent, logPrefix, `could not identify user (${getErrorMessage(error)})`) } const perUserStore = createSecureStore({ @@ -109,7 +112,7 @@ export async function migrateLegacyAuth( return skipped( silent, logPrefix, - `failed to write user-scoped credential (${describe(error)})`, + `failed to write user-scoped credential (${getErrorMessage(error)})`, ) } } @@ -128,7 +131,11 @@ export async function migrateLegacyAuth( // best-effort rollback } } - return skipped(silent, logPrefix, `failed to update user records (${describe(error)})`) + return skipped( + silent, + logPrefix, + `failed to update user records (${getErrorMessage(error)})`, + ) } try { @@ -153,8 +160,10 @@ export async function migrateLegacyAuth( } if (!silent) { - const label = account.label ?? account.id - console.error(`${logPrefix}: migrated existing token to multi-user store (${label}).`) + // Log the stable id only — `account.label` is typically an email or + // other user-facing identifier, and these diagnostics flow to stderr + // (and possibly to log aggregators) where PII shouldn't appear. + console.error(`${logPrefix}: migrated existing token to multi-user store (${account.id}).`) } return { status: 'migrated', migratedAccount: account } @@ -182,10 +191,6 @@ async function readLegacyToken(opts: { return null } -function describe(error: unknown): string { - return error instanceof Error && error.message ? error.message : String(error) -} - function skipped( silent: boolean | undefined, logPrefix: string, diff --git a/src/auth/keyring/secure-store.test.ts b/src/auth/keyring/secure-store.test.ts index 7c06c33..1a3ddba 100644 --- a/src/auth/keyring/secure-store.test.ts +++ b/src/auth/keyring/secure-store.test.ts @@ -1,4 +1,4 @@ -import { beforeEach, describe, expect, it, vi } from 'vitest' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' const keyringMocks = vi.hoisted(() => { const entry = { @@ -88,6 +88,13 @@ describe('createSecureStore — missing native binary', () => { vi.resetModules() }) + // Ensure the throw-on-import mock is torn down even when the assertion + // inside the `it` body fails. Inline cleanup would otherwise be skipped + // and leave the mock active for later tests. + afterEach(() => { + vi.doUnmock('@napi-rs/keyring') + }) + it('surfaces an import failure as SecureStoreUnavailableError instead of crashing module load', async () => { vi.doMock('@napi-rs/keyring', () => { throw new Error('no native binary for this arch') @@ -98,7 +105,5 @@ describe('createSecureStore — missing native binary', () => { await expect( createSecureStore({ serviceName: SERVICE, account: ACCOUNT }).getSecret(), ).rejects.toBeInstanceOf(SecureStoreUnavailableError) - - vi.doUnmock('@napi-rs/keyring') }) }) diff --git a/src/auth/keyring/secure-store.ts b/src/auth/keyring/secure-store.ts index 4aad8e6..d4b660c 100644 --- a/src/auth/keyring/secure-store.ts +++ b/src/auth/keyring/secure-store.ts @@ -1,5 +1,12 @@ export const SECURE_STORE_DESCRIPTION = 'system credential manager' +/** + * Default keyring account slug for a user id. Shared between the runtime + * `createKeyringTokenStore` and `migrateLegacyAuth` so a future rename can't + * silently park tokens in a slot the runtime no longer reads from. + */ +export const DEFAULT_ACCOUNT_FOR_USER = (id: string): string => `user-${id}` + /** * Thrown when the OS credential manager cannot be reached — missing native * binary for the current architecture, libsecret/D-Bus unavailable diff --git a/src/auth/keyring/token-store.test.ts b/src/auth/keyring/token-store.test.ts index add0442..0d7405a 100644 --- a/src/auth/keyring/token-store.test.ts +++ b/src/auth/keyring/token-store.test.ts @@ -21,8 +21,57 @@ type Account = { email: string } -function buildSecureStoreMock(initial: { secret?: string | null } = {}): SecureStore & { - _state: { secret: string | null } +type Slot = { + secret: string | null + getErr?: unknown + setErr?: unknown + delErr?: unknown +} + +function buildKeyringMap(): { + create: (args: { serviceName: string; account: string }) => SecureStore + slots: Map + deleteCalls: Map +} { + const slots = new Map() + const deleteCalls = new Map() + function getSlot(account: string): Slot { + let slot = slots.get(account) + if (!slot) { + slot = { secret: null } + slots.set(account, slot) + } + return slot + } + return { + slots, + deleteCalls, + create({ account }) { + return { + async getSecret() { + const slot = getSlot(account) + if (slot.getErr) throw slot.getErr + return slot.secret + }, + async setSecret(secret) { + const slot = getSlot(account) + if (slot.setErr) throw slot.setErr + slot.secret = secret + }, + async deleteSecret() { + deleteCalls.set(account, (deleteCalls.get(account) ?? 0) + 1) + const slot = getSlot(account) + if (slot.delErr) throw slot.delErr + const had = slot.secret !== null + slot.secret = null + return had + }, + } + }, + } +} + +function buildSingleSlot(initial: { secret?: string | null } = {}): SecureStore & { getSpy: ReturnType setSpy: ReturnType deleteSpy: ReturnType @@ -38,7 +87,6 @@ function buildSecureStoreMock(initial: { secret?: string | null } = {}): SecureS return had }) return { - _state: state, getSpy, setSpy, deleteSpy, @@ -62,6 +110,7 @@ function buildUserRecords(): { } upsertSpy: ReturnType removeSpy: ReturnType + setDefaultSpy: ReturnType } { const state = { records: new Map>(), @@ -73,6 +122,9 @@ function buildUserRecords(): { const removeSpy = vi.fn(async (id: string) => { state.records.delete(id) }) + const setDefaultSpy = vi.fn(async (id: string | null) => { + state.defaultId = id + }) const store: UserRecordStore = { async list() { return [...state.records.values()] @@ -85,14 +137,12 @@ function buildUserRecords(): { async getDefaultId() { return state.defaultId }, - async setDefaultId(id) { - state.defaultId = id - }, + setDefaultId: setDefaultSpy, describeLocation() { return '/tmp/fake/config.json' }, } - return { store, state, upsertSpy, removeSpy } + return { store, state, upsertSpy, removeSpy, setDefaultSpy } } const SERVICE = 'cli-core-test' @@ -104,7 +154,7 @@ describe('createKeyringTokenStore', () => { }) it('round-trips set → active → clear when the keyring is online', async () => { - const keyring = buildSecureStoreMock() + const keyring = buildSingleSlot() mockedCreateSecureStore.mockReturnValue(keyring) const { store: userRecords, state, upsertSpy } = buildUserRecords() @@ -127,7 +177,7 @@ describe('createKeyringTokenStore', () => { }) it('falls back to a plaintext token on the user record when the keyring is offline', async () => { - const keyring = buildSecureStoreMock() + const keyring = buildSingleSlot() keyring.setSpy.mockRejectedValueOnce(new SecureStoreUnavailableError('no dbus')) mockedCreateSecureStore.mockReturnValue(keyring) const { store: userRecords, state } = buildUserRecords() @@ -150,7 +200,7 @@ describe('createKeyringTokenStore', () => { }) it('rolls back the keyring write when the user record upsert fails', async () => { - const keyring = buildSecureStoreMock() + const keyring = buildSingleSlot() mockedCreateSecureStore.mockReturnValue(keyring) const { store: userRecords, upsertSpy } = buildUserRecords() upsertSpy.mockRejectedValueOnce(new Error('disk full')) @@ -162,8 +212,22 @@ describe('createKeyringTokenStore', () => { expect(keyring.deleteSpy).toHaveBeenCalledTimes(1) }) - it('returns null from active() when the keyring is unreachable mid-session', async () => { - const keyring = buildSecureStoreMock({ secret: 'tok' }) + it('set() still succeeds when the best-effort default promotion fails', async () => { + const keyring = buildSingleSlot() + mockedCreateSecureStore.mockReturnValue(keyring) + const { store: userRecords, state, setDefaultSpy } = buildUserRecords() + setDefaultSpy.mockRejectedValueOnce(new Error('default-write blew up')) + + const store = createKeyringTokenStore({ serviceName: SERVICE, userRecords }) + + await expect(store.set(account, 'tok')).resolves.toBeUndefined() + expect(state.records.get('42')?.account).toEqual(account) + // Default never got set because the write failed, but the user record is durable. + expect(state.defaultId).toBeNull() + }) + + it('throws AUTH_STORE_READ_FAILED when active() finds a record but the keyring is offline', async () => { + const keyring = buildSingleSlot({ secret: 'tok' }) keyring.getSpy.mockRejectedValueOnce(new SecureStoreUnavailableError('locked')) mockedCreateSecureStore.mockReturnValue(keyring) const { store: userRecords, state } = buildUserRecords() @@ -172,11 +236,11 @@ describe('createKeyringTokenStore', () => { const store = createKeyringTokenStore({ serviceName: SERVICE, userRecords }) - await expect(store.active()).resolves.toBeNull() + await expect(store.active()).rejects.toMatchObject({ code: 'AUTH_STORE_READ_FAILED' }) }) it('returns null from active() when no records exist', async () => { - mockedCreateSecureStore.mockReturnValue(buildSecureStoreMock()) + mockedCreateSecureStore.mockReturnValue(buildSingleSlot()) const { store: userRecords } = buildUserRecords() const store = createKeyringTokenStore({ serviceName: SERVICE, userRecords }) @@ -185,7 +249,7 @@ describe('createKeyringTokenStore', () => { }) it('picks the lone user when no default is set', async () => { - const keyring = buildSecureStoreMock({ secret: 'tok' }) + const keyring = buildSingleSlot({ secret: 'tok' }) mockedCreateSecureStore.mockReturnValue(keyring) const { store: userRecords, state } = buildUserRecords() state.records.set('42', { id: '42', account }) @@ -196,7 +260,7 @@ describe('createKeyringTokenStore', () => { }) it('returns null when multiple users exist and no default is set', async () => { - mockedCreateSecureStore.mockReturnValue(buildSecureStoreMock()) + mockedCreateSecureStore.mockReturnValue(buildSingleSlot()) const { store: userRecords, state } = buildUserRecords() state.records.set('1', { id: '1', account: { ...account, id: '1' } }) state.records.set('2', { id: '2', account: { ...account, id: '2' } }) @@ -207,7 +271,7 @@ describe('createKeyringTokenStore', () => { }) it('does not overwrite an existing default when a second user is added', async () => { - const keyring = buildSecureStoreMock() + const keyring = buildSingleSlot() mockedCreateSecureStore.mockReturnValue(keyring) const { store: userRecords, state } = buildUserRecords() state.defaultId = '1' @@ -221,7 +285,7 @@ describe('createKeyringTokenStore', () => { }) it('clear() is a no-op when no record exists', async () => { - mockedCreateSecureStore.mockReturnValue(buildSecureStoreMock()) + mockedCreateSecureStore.mockReturnValue(buildSingleSlot()) const { store: userRecords } = buildUserRecords() const store = createKeyringTokenStore({ serviceName: SERVICE, userRecords }) @@ -230,8 +294,8 @@ describe('createKeyringTokenStore', () => { expect(store.getLastClearResult()).toBeUndefined() }) - it('clear() with a fallback-token record skips the keyring delete', async () => { - const keyring = buildSecureStoreMock() + it('clear() still calls the keyring delete when a fallbackToken is present (orphan cleanup)', async () => { + const keyring = buildSingleSlot({ secret: 'orphan_from_earlier_write' }) mockedCreateSecureStore.mockReturnValue(keyring) const { store: userRecords, state } = buildUserRecords() state.records.set('42', { id: '42', account, fallbackToken: 'tok_plain' }) @@ -241,7 +305,7 @@ describe('createKeyringTokenStore', () => { await store.clear() - expect(keyring.deleteSpy).not.toHaveBeenCalled() + expect(keyring.deleteSpy).toHaveBeenCalledTimes(1) expect(state.records.size).toBe(0) expect(store.getLastClearResult()).toEqual({ storage: 'config-file', @@ -251,7 +315,7 @@ describe('createKeyringTokenStore', () => { }) it('clear() downgrades to a warning when the keyring delete fails', async () => { - const keyring = buildSecureStoreMock({ secret: 'tok' }) + const keyring = buildSingleSlot({ secret: 'tok' }) keyring.deleteSpy.mockRejectedValueOnce(new SecureStoreUnavailableError('offline')) mockedCreateSecureStore.mockReturnValue(keyring) const { store: userRecords, state } = buildUserRecords() @@ -267,7 +331,7 @@ describe('createKeyringTokenStore', () => { }) it('uses a custom accountForUser slug when provided', async () => { - const keyring = buildSecureStoreMock() + const keyring = buildSingleSlot() mockedCreateSecureStore.mockReturnValue(keyring) const { store: userRecords } = buildUserRecords() @@ -285,31 +349,47 @@ describe('createKeyringTokenStore', () => { }) }) - describe('AccountRef support', () => { - it('active(ref) matches on id', async () => { - const keyring = buildSecureStoreMock({ secret: 'tok_a' }) - mockedCreateSecureStore.mockReturnValue(keyring) - const { store: userRecords, state } = buildUserRecords() - state.records.set('1', { id: '1', account: { id: '1', label: 'me', email: 'a@b' } }) + describe('AccountRef support (keyed per-user slots)', () => { + function buildMultiUser() { + const km = buildKeyringMap() + mockedCreateSecureStore.mockImplementation(km.create) + const { store: userRecords, state, removeSpy } = buildUserRecords() + state.records.set('1', { id: '1', account: { id: '1', label: 'alice', email: 'a@b' } }) state.records.set('2', { id: '2', - account: { id: '2', label: 'you', email: 'c@d' }, + account: { id: '2', label: 'bob', email: 'c@d' }, fallbackToken: 'tok_b', }) + km.slots.set('user-1', { secret: 'tok_a' }) + return { km, userRecords, state, removeSpy } + } + + it('active(ref) reads from the matching per-user slot', async () => { + const { km, userRecords } = buildMultiUser() + const store = createKeyringTokenStore({ serviceName: SERVICE, userRecords }) + + const snapshot = await store.active('1') + expect(snapshot?.account.id).toBe('1') + expect(snapshot?.token).toBe('tok_a') + + // Sanity check: matched user 1 only — user 2's keyring slot was + // never touched (its record carries `fallbackToken`). + expect(km.slots.has('user-2')).toBe(false) + }) + + it('active(ref) prefers the fallbackToken over a stale keyring entry', async () => { + const { km, userRecords } = buildMultiUser() + // Simulate an orphan keyring entry left from a prior online write. + km.slots.set('user-2', { secret: 'tok_b_stale' }) const store = createKeyringTokenStore({ serviceName: SERVICE, userRecords }) const snapshot = await store.active('2') - expect(snapshot?.account.id).toBe('2') expect(snapshot?.token).toBe('tok_b') }) it('active(ref) matches on label', async () => { - const keyring = buildSecureStoreMock({ secret: 'tok' }) - mockedCreateSecureStore.mockReturnValue(keyring) - const { store: userRecords, state } = buildUserRecords() - state.records.set('1', { id: '1', account: { id: '1', label: 'alice', email: 'a@b' } }) - + const { userRecords } = buildMultiUser() const store = createKeyringTokenStore({ serviceName: SERVICE, userRecords }) const snapshot = await store.active('alice') @@ -317,48 +397,43 @@ describe('createKeyringTokenStore', () => { }) it('active(ref) returns null on a miss (attacher translates to ACCOUNT_NOT_FOUND)', async () => { - mockedCreateSecureStore.mockReturnValue(buildSecureStoreMock()) - const { store: userRecords, state } = buildUserRecords() - state.records.set('1', { id: '1', account }) - + const { userRecords } = buildMultiUser() const store = createKeyringTokenStore({ serviceName: SERVICE, userRecords }) await expect(store.active('does-not-exist')).resolves.toBeNull() }) - it('clear(ref) removes only the matching record', async () => { - const keyring = buildSecureStoreMock() - mockedCreateSecureStore.mockReturnValue(keyring) - const { store: userRecords, state } = buildUserRecords() - state.records.set('1', { id: '1', account: { id: '1', label: 'a', email: 'a@b' } }) - state.records.set('2', { id: '2', account: { id: '2', label: 'b', email: 'c@d' } }) + it('clear(ref) removes the matching record and deletes only its keyring slot', async () => { + const { km, userRecords, state } = buildMultiUser() state.defaultId = '1' - const store = createKeyringTokenStore({ serviceName: SERVICE, userRecords }) - await store.clear('2') + await store.clear('1') - expect(state.records.has('1')).toBe(true) - expect(state.records.has('2')).toBe(false) - expect(state.defaultId).toBe('1') + expect(state.records.has('1')).toBe(false) + expect(state.records.has('2')).toBe(true) + expect(state.defaultId).toBeNull() + // user-1's slot was cleared; user-2's slot was never touched. + expect(km.slots.get('user-1')?.secret).toBeNull() + expect(km.deleteCalls.get('user-1') ?? 0).toBeGreaterThan(0) + expect(km.deleteCalls.has('user-2')).toBe(false) }) it('clear(ref) on a miss is a no-op (attacher rejects via active() pre-check)', async () => { - mockedCreateSecureStore.mockReturnValue(buildSecureStoreMock()) - const { store: userRecords, state } = buildUserRecords() - state.records.set('1', { id: '1', account }) - + const { userRecords, state } = buildMultiUser() const store = createKeyringTokenStore({ serviceName: SERVICE, userRecords }) await store.clear('nope') expect(state.records.has('1')).toBe(true) + expect(state.records.has('2')).toBe(true) }) it('honours a custom matchAccount predicate', async () => { - const keyring = buildSecureStoreMock({ secret: 'tok' }) - mockedCreateSecureStore.mockReturnValue(keyring) + const km = buildKeyringMap() + mockedCreateSecureStore.mockImplementation(km.create) const { store: userRecords, state } = buildUserRecords() state.records.set('1', { id: '1', account: { id: '1', email: 'Alice@x.io' } }) + km.slots.set('user-1', { secret: 'tok' }) const store = createKeyringTokenStore({ serviceName: SERVICE, @@ -374,7 +449,7 @@ describe('createKeyringTokenStore', () => { describe('list() + setDefault()', () => { it('list() returns every account with the default marker', async () => { - mockedCreateSecureStore.mockReturnValue(buildSecureStoreMock()) + mockedCreateSecureStore.mockReturnValue(buildSingleSlot()) const { store: userRecords, state } = buildUserRecords() state.records.set('1', { id: '1', account: { id: '1', email: 'a@b' } }) state.records.set('2', { id: '2', account: { id: '2', email: 'c@d' } }) @@ -389,7 +464,7 @@ describe('createKeyringTokenStore', () => { }) it('list() returns an empty array when nothing is stored', async () => { - mockedCreateSecureStore.mockReturnValue(buildSecureStoreMock()) + mockedCreateSecureStore.mockReturnValue(buildSingleSlot()) const { store: userRecords } = buildUserRecords() const store = createKeyringTokenStore({ serviceName: SERVICE, userRecords }) @@ -398,7 +473,7 @@ describe('createKeyringTokenStore', () => { }) it('setDefault(ref) marks the matching account as default', async () => { - mockedCreateSecureStore.mockReturnValue(buildSecureStoreMock()) + mockedCreateSecureStore.mockReturnValue(buildSingleSlot()) const { store: userRecords, state } = buildUserRecords() state.records.set('1', { id: '1', account: { id: '1', label: 'a', email: 'a@b' } }) state.records.set('2', { id: '2', account: { id: '2', label: 'b', email: 'c@d' } }) @@ -411,7 +486,7 @@ describe('createKeyringTokenStore', () => { }) it('setDefault(ref) throws ACCOUNT_NOT_FOUND on a miss', async () => { - mockedCreateSecureStore.mockReturnValue(buildSecureStoreMock()) + mockedCreateSecureStore.mockReturnValue(buildSingleSlot()) const { store: userRecords, state } = buildUserRecords() state.records.set('1', { id: '1', account }) diff --git a/src/auth/keyring/token-store.ts b/src/auth/keyring/token-store.ts index be5a282..1f1e677 100644 --- a/src/auth/keyring/token-store.ts +++ b/src/auth/keyring/token-store.ts @@ -1,7 +1,9 @@ import { CliError } from '../../errors.js' import type { AccountRef, AuthAccount, TokenStore } from '../types.js' +import { accountNotFoundError } from '../user-flag.js' import { createSecureStore, + DEFAULT_ACCOUNT_FOR_USER, SECURE_STORE_DESCRIPTION, SecureStoreUnavailableError, type SecureStore, @@ -33,7 +35,6 @@ export type KeyringTokenStore = TokenStore `user-${id}` const DEFAULT_MATCH_ACCOUNT = ( account: TAccount, ref: AccountRef, @@ -70,43 +71,56 @@ export function createKeyringTokenStore( return createSecureStore({ serviceName, account: accountForUser(id) }) } - async function findByRef(ref: AccountRef): Promise | null> { - const all = await userRecords.list() - return all.find((record) => matchAccount(record.account, ref)) ?? null + type Snapshot = { records: UserRecord[]; defaultId: string | null } + async function readSnapshot(): Promise { + const [records, defaultId] = await Promise.all([ + userRecords.list(), + userRecords.getDefaultId(), + ]) + return { records, defaultId } } - async function resolveDefaultRecord(): Promise | null> { - const defaultId = await userRecords.getDefaultId() - if (defaultId) { - const record = await userRecords.getById(defaultId) - if (record) return record - } - const all = await userRecords.list() - if (all.length === 1) return all[0] - return null + function findByRef(snapshot: Snapshot, ref: AccountRef): UserRecord | null { + return snapshot.records.find((record) => matchAccount(record.account, ref)) ?? null } - async function readToken(record: UserRecord): Promise { - if (record.fallbackToken?.trim()) { - return record.fallbackToken.trim() - } - try { - const token = await secureStoreFor(record.id).getSecret() - return token?.trim() ? token.trim() : null - } catch (error) { - if (error instanceof SecureStoreUnavailableError) return null - throw error + function resolveDefault(snapshot: Snapshot): UserRecord | null { + if (snapshot.defaultId) { + const found = snapshot.records.find((r) => r.id === snapshot.defaultId) + if (found) return found } + return snapshot.records.length === 1 ? snapshot.records[0] : null } return { async active(ref) { - const record = ref === undefined ? await resolveDefaultRecord() : await findByRef(ref) + const snapshot = await readSnapshot() + const record = ref === undefined ? resolveDefault(snapshot) : findByRef(snapshot, ref) if (!record) return null - const token = await readToken(record) - if (!token) return null - return { token, account: record.account } + if (record.fallbackToken?.trim()) { + return { token: record.fallbackToken.trim(), account: record.account } + } + + try { + const token = await secureStoreFor(record.id).getSecret() + if (token?.trim()) { + return { token: token.trim(), account: record.account } + } + return null + } 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 `. + if (error instanceof SecureStoreUnavailableError) { + throw new CliError( + 'AUTH_STORE_READ_FAILED', + `${SECURE_STORE_DESCRIPTION} unavailable; could not read stored token (${error.message})`, + ) + } + throw error + } }, async set(account, token) { @@ -142,9 +156,16 @@ export function createKeyringTokenStore( throw error } - const existingDefault = await userRecords.getDefaultId() - if (!existingDefault) { - await userRecords.setDefaultId(account.id) + // 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 with ` account use`). + try { + const existingDefault = await userRecords.getDefaultId() + if (!existingDefault) { + await userRecords.setDefaultId(account.id) + } + } catch { + // best-effort } lastStorageResult = storedSecurely @@ -159,7 +180,8 @@ export function createKeyringTokenStore( }, async clear(ref) { - const record = ref === undefined ? await resolveDefaultRecord() : await findByRef(ref) + const snapshot = await readSnapshot() + const record = ref === undefined ? resolveDefault(snapshot) : findByRef(snapshot, ref) if (!record) { lastClearResult = undefined return @@ -167,26 +189,27 @@ export function createKeyringTokenStore( await userRecords.remove(record.id) - const currentDefault = await userRecords.getDefaultId() - if (currentDefault === record.id) { + if (snapshot.defaultId === record.id) { await userRecords.setDefaultId(null) } - // No keyring entry to delete when the token was already plaintext. - if (record.fallbackToken !== undefined) { - lastClearResult = { - storage: 'config-file', - warning: buildFallbackWarning( - 'local auth state cleared in', - userRecords.describeLocation(), - ), - } - return - } - + // 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. try { await secureStoreFor(record.id).deleteSecret() - lastClearResult = { storage: 'secure-store' } + lastClearResult = + record.fallbackToken !== undefined + ? { + storage: 'config-file', + warning: buildFallbackWarning( + 'local auth state cleared in', + userRecords.describeLocation(), + ), + } + : { storage: 'secure-store' } } catch (error) { if (!(error instanceof SecureStoreUnavailableError)) throw error lastClearResult = { @@ -200,18 +223,18 @@ export function createKeyringTokenStore( }, async list() { - const all = await userRecords.list() - const defaultId = await userRecords.getDefaultId() - return all.map((record) => ({ + const { records, defaultId } = await readSnapshot() + return records.map((record) => ({ account: record.account, isDefault: record.id === defaultId, })) }, async setDefault(ref) { - const record = await findByRef(ref) + const all = await userRecords.list() + const record = all.find((r) => matchAccount(r.account, ref)) if (!record) { - throw new CliError('ACCOUNT_NOT_FOUND', `No stored account matches "${ref}".`) + throw accountNotFoundError(ref) } await userRecords.setDefaultId(record.id) }, diff --git a/src/auth/keyring/types.ts b/src/auth/keyring/types.ts index d85563c..5f2a82e 100644 --- a/src/auth/keyring/types.ts +++ b/src/auth/keyring/types.ts @@ -34,6 +34,13 @@ export type UserRecord = { export type UserRecordStore = { list(): Promise[]> getById(id: string): Promise | null> + /** + * **Replace**, do not merge. The persisted record must equal `record` field + * for field — an absent `fallbackToken` means "no plaintext token", and a + * merge-style implementation would let a stale plaintext token outlive a + * later keyring-backed write (the runtime preferentially reads + * `fallbackToken` over the keyring). + */ upsert(record: UserRecord): Promise remove(id: string): Promise getDefaultId(): Promise diff --git a/src/auth/user-flag.ts b/src/auth/user-flag.ts index 280402e..262e29d 100644 --- a/src/auth/user-flag.ts +++ b/src/auth/user-flag.ts @@ -18,6 +18,11 @@ export function extractUserRef(cmd: Record): AccountRef | undef return typeof cmd.user === 'string' ? cmd.user : undefined } +/** Shared constructor so `--user` attachers and the keyring `setDefault` can't drift on wording. */ +export function accountNotFoundError(ref: AccountRef): CliError { + return new CliError('ACCOUNT_NOT_FOUND', `No stored account matches "${ref}".`) +} + /** * Read `store.active(ref)` and throw `ACCOUNT_NOT_FOUND` if the explicit * `ref` doesn't match. With `ref === undefined` returns the snapshot @@ -29,7 +34,7 @@ export async function requireSnapshotForRef( ): Promise<{ token: string; account: TAccount } | null> { const snapshot = await store.active(ref) if (ref !== undefined && snapshot === null) { - throw new CliError('ACCOUNT_NOT_FOUND', `No stored account matches "${ref}".`) + throw accountNotFoundError(ref) } return snapshot } From 0726440ebfeedb8edc25e93f6195a83e4ba808e1 Mon Sep 17 00:00:00 2001 From: Scott Lovegrove Date: Sat, 16 May 2026 12:53:53 +0100 Subject: [PATCH 3/4] fix(auth): address second keyring TokenStore review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit P2: - `clear()` wraps `setDefaultId(null)` in best-effort try/catch so a default-pointer write failure can't skip the keyring delete that follows and leave an unreachable orphan secret. - `attachLogoutCommand` catches `AUTH_STORE_READ_FAILED` from the snapshot pre-flight and proceeds with `clear(ref)` — `logout --user ` can now clear a secure-only record even when the keyring is offline. Other typed errors (notably `ACCOUNT_NOT_FOUND`) still propagate. - `list()` resolves the implicit single-record default via `resolveDefault(snapshot)` so its `isDefault` markers match `active()`'s behaviour (single record with no pinned default → `isDefault: true`). - `readSnapshot()` skips `getDefaultId()` when an explicit ref is supplied, halving the config-file reads on the `--user ` hot path. - `UserRecordStore.getById` removed: unused after the snapshot refactor; the README example and consumer surface are simpler. - `writeRecordWithKeyringFallback` in `record-write.ts` is the single source of truth for the keyring-write → fallback → upsert → rollback choreography shared by `createKeyringTokenStore.set` and `migrateLegacyAuth`. - `migrateLegacyAuth` stderr emits a fixed phrase keyed off a `SkipReason` enum so consumer-supplied error text (which may contain emails, paths, or auth diagnostics) can't leak into logs. The raw detail is still attached to `MigrateAuthResult.reason` for in-process callers. - `src/test-support/keyring-mocks.ts` is the shared `buildKeyringMap` / `buildSingleSlot` / `buildUserRecords` used by both keyring test files (excluded from build by `tsconfig.build.json`). - New tests cover: `clear()` keyring delete when `setDefaultId(null)` throws, `set()` survives `setDefaultId` failure, `active()` keyring read failure surfaces `AUTH_STORE_READ_FAILED`, `list()` matches `active()`'s implicit default, attacher-level end-to-end coverage for `AUTH_STORE_READ_FAILED` propagation in status/logout/token-view, and a non-silent migrate log that asserts only `account.id` (no `label`/email) and a generic skip phrase. P3: - `UserRecord.fallbackToken` doc now matches the runtime read order (`fallbackToken` first, then keyring). - `clear()` hoists the fallback `TokenStorageResult` to a single `const`, removing the duplicate object literal across the try/catch. - `DEFAULT_ACCOUNT_FOR_USER` no longer exported from the keyring barrel (it's already accessible via direct import from `secure-store`; dead surface from `src/auth/index.ts`). - Already-migrated, list/setDefault, and "no records" tests drop the unnecessary `createSecureStore` mock and assert that the keyring is never reached. Co-Authored-By: Claude Opus 4.7 (1M context) --- README.md | 5 +- src/auth/keyring/index.ts | 1 - src/auth/keyring/migrate.test.ts | 198 +++++++++-------------- src/auth/keyring/migrate.ts | 91 ++++++----- src/auth/keyring/record-write.ts | 71 +++++++++ src/auth/keyring/token-store.test.ts | 230 +++++++++------------------ src/auth/keyring/token-store.ts | 144 ++++++++--------- src/auth/keyring/types.ts | 10 +- src/auth/logout.test.ts | 52 ++++++ src/auth/logout.ts | 15 +- src/auth/status.test.ts | 17 ++ src/auth/token-view.test.ts | 21 +++ src/test-support/keyring-mocks.ts | 149 +++++++++++++++++ 13 files changed, 605 insertions(+), 399 deletions(-) create mode 100644 src/auth/keyring/record-write.ts create mode 100644 src/test-support/keyring-mocks.ts diff --git a/README.md b/README.md index e3f70e8..0bf9ff8 100644 --- a/README.md +++ b/README.md @@ -302,11 +302,8 @@ const userRecords: UserRecordStore = { async list() { /* read from config */ }, - async getById(id) { - /* … */ - }, async upsert(record) { - /* … */ + /* replace, do not merge — see UserRecordStore docs */ }, async remove(id) { /* … */ diff --git a/src/auth/keyring/index.ts b/src/auth/keyring/index.ts index 44778b2..741151b 100644 --- a/src/auth/keyring/index.ts +++ b/src/auth/keyring/index.ts @@ -1,5 +1,4 @@ export { - DEFAULT_ACCOUNT_FOR_USER, SECURE_STORE_DESCRIPTION, SecureStoreUnavailableError, createSecureStore, diff --git a/src/auth/keyring/migrate.test.ts b/src/auth/keyring/migrate.test.ts index 9a13d4b..d258472 100644 --- a/src/auth/keyring/migrate.test.ts +++ b/src/auth/keyring/migrate.test.ts @@ -1,8 +1,8 @@ -import { beforeEach, describe, expect, it, vi } from 'vitest' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { buildKeyringMap, buildUserRecords } from '../../test-support/keyring-mocks.js' import { migrateLegacyAuth } from './migrate.js' -import { SecureStoreUnavailableError, type SecureStore } from './secure-store.js' -import type { UserRecord, UserRecordStore } from './types.js' +import { SecureStoreUnavailableError } from './secure-store.js' vi.mock('./secure-store.js', async () => { const actual = await vi.importActual('./secure-store.js') @@ -21,95 +21,6 @@ type Account = { email: string } -function buildKeyringMap(): { - create: (args: { serviceName: string; account: string }) => SecureStore - slots: Map< - string, - { secret: string | null; getErr?: unknown; setErr?: unknown; delErr?: unknown } - > -} { - const slots = new Map< - string, - { - secret: string | null - getErr?: unknown - setErr?: unknown - delErr?: unknown - } - >() - function getSlot(account: string) { - let slot = slots.get(account) - if (!slot) { - slot = { secret: null } - slots.set(account, slot) - } - return slot - } - return { - slots, - create({ account }) { - return { - async getSecret() { - const slot = getSlot(account) - if (slot.getErr) throw slot.getErr - return slot.secret - }, - async setSecret(secret) { - const slot = getSlot(account) - if (slot.setErr) throw slot.setErr - slot.secret = secret - }, - async deleteSecret() { - const slot = getSlot(account) - if (slot.delErr) throw slot.delErr - const had = slot.secret !== null - slot.secret = null - return had - }, - } - }, - } -} - -function buildUserRecords(): { - store: UserRecordStore - state: { - records: Map> - defaultId: string | null - } - upsertSpy: ReturnType -} { - const state = { - records: new Map>(), - defaultId: null as string | null, - } - const upsertSpy = vi.fn(async (record: UserRecord) => { - state.records.set(record.id, record) - }) - const store: UserRecordStore = { - async list() { - return [...state.records.values()] - }, - async getById(id) { - return state.records.get(id) ?? null - }, - upsert: upsertSpy, - async remove(id) { - state.records.delete(id) - }, - async getDefaultId() { - return state.defaultId - }, - async setDefaultId(id) { - state.defaultId = id - }, - describeLocation() { - return '/tmp/fake/config.json' - }, - } - return { store, state, upsertSpy } -} - const SERVICE = 'cli-core-test' const LEGACY = 'api-token' @@ -119,10 +30,8 @@ describe('migrateLegacyAuth', () => { }) it('returns already-migrated when user records already exist', async () => { - mockedCreateSecureStore.mockReturnValue( - buildKeyringMap().create({ serviceName: SERVICE, account: LEGACY }), - ) - const { store: userRecords, state } = buildUserRecords() + // No keyring mock needed — `migrateLegacyAuth` returns before ever calling `createSecureStore`. + const { store: userRecords, state } = buildUserRecords() state.records.set('1', { id: '1', account: { id: '1', email: 'a@b' } }) const result = await migrateLegacyAuth({ @@ -135,12 +44,13 @@ describe('migrateLegacyAuth', () => { }) expect(result.status).toBe('already-migrated') + expect(mockedCreateSecureStore).not.toHaveBeenCalled() }) it('returns no-legacy-state when neither slot has a token', async () => { const km = buildKeyringMap() mockedCreateSecureStore.mockImplementation(km.create) - const { store: userRecords } = buildUserRecords() + const { store: userRecords } = buildUserRecords() const result = await migrateLegacyAuth({ serviceName: SERVICE, @@ -158,7 +68,7 @@ describe('migrateLegacyAuth', () => { const km = buildKeyringMap() km.slots.set(LEGACY, { secret: 'legacy_tok' }) mockedCreateSecureStore.mockImplementation(km.create) - const { store: userRecords, state, upsertSpy } = buildUserRecords() + const { store: userRecords, state, upsertSpy } = buildUserRecords() const cleanup = vi.fn(async () => undefined) const result = await migrateLegacyAuth({ @@ -187,7 +97,7 @@ describe('migrateLegacyAuth', () => { it('falls back to loadLegacyPlaintextToken when the legacy keyring slot is empty', async () => { const km = buildKeyringMap() mockedCreateSecureStore.mockImplementation(km.create) - const { store: userRecords, state } = buildUserRecords() + const { store: userRecords, state } = buildUserRecords() const result = await migrateLegacyAuth({ serviceName: SERVICE, @@ -218,7 +128,7 @@ describe('migrateLegacyAuth', () => { setErr: new SecureStoreUnavailableError('no dbus'), }) mockedCreateSecureStore.mockImplementation(km.create) - const { store: userRecords, state } = buildUserRecords() + const { store: userRecords, state } = buildUserRecords() const result = await migrateLegacyAuth({ serviceName: SERVICE, @@ -234,70 +144,110 @@ describe('migrateLegacyAuth', () => { expect(state.defaultId).toBe('7') }) - it('stores fallbackToken on the record when the per-user keyring write fails', async () => { + it('returns skipped when identifyAccount throws', async () => { const km = buildKeyringMap() km.slots.set(LEGACY, { secret: 'legacy_tok' }) - km.slots.set('user-99', { - secret: null, - setErr: new SecureStoreUnavailableError('offline'), - }) mockedCreateSecureStore.mockImplementation(km.create) - const { store: userRecords, state } = buildUserRecords() + const { store: userRecords, state } = buildUserRecords() const result = await migrateLegacyAuth({ serviceName: SERVICE, legacyAccount: LEGACY, userRecords, loadLegacyPlaintextToken: async () => null, - identifyAccount: async () => ({ id: '99', email: 'me@x.io' }), + identifyAccount: async () => { + throw new Error('HTTP 401') + }, silent: true, }) - expect(result.status).toBe('migrated') - expect(state.records.get('99')?.fallbackToken).toBe('legacy_tok') + expect(result.status).toBe('skipped') + expect(result.reason).toContain('HTTP 401') + expect(state.records.size).toBe(0) + // Legacy entry must remain so a retry can find it. + expect(km.slots.get(LEGACY)?.secret).toBe('legacy_tok') }) - it('returns skipped when identifyAccount throws', async () => { + it('rolls back the keyring write when user-record upsert fails', async () => { const km = buildKeyringMap() km.slots.set(LEGACY, { secret: 'legacy_tok' }) mockedCreateSecureStore.mockImplementation(km.create) - const { store: userRecords, state } = buildUserRecords() + const { store: userRecords, upsertSpy } = buildUserRecords() + upsertSpy.mockRejectedValueOnce(new Error('disk full')) const result = await migrateLegacyAuth({ serviceName: SERVICE, legacyAccount: LEGACY, userRecords, loadLegacyPlaintextToken: async () => null, - identifyAccount: async () => { - throw new Error('HTTP 401') - }, + identifyAccount: async () => ({ id: '99', email: 'me@x.io' }), silent: true, }) expect(result.status).toBe('skipped') - expect(result.reason).toContain('HTTP 401') - expect(state.records.size).toBe(0) - // Legacy entry must remain so a retry can find it. - expect(km.slots.get(LEGACY)?.secret).toBe('legacy_tok') + expect(km.slots.get('user-99')?.secret).toBeNull() }) +}) - it('rolls back the keyring write when user-record upsert fails', async () => { +describe('migrateLegacyAuth — stderr privacy', () => { + let consoleError: ReturnType + beforeEach(() => { + mockedCreateSecureStore.mockReset() + consoleError = vi.spyOn(console, 'error').mockImplementation(() => undefined) + }) + afterEach(() => { + consoleError.mockRestore() + }) + + it('the success line carries only account.id (no label/email)', async () => { const km = buildKeyringMap() km.slots.set(LEGACY, { secret: 'legacy_tok' }) mockedCreateSecureStore.mockImplementation(km.create) - const { store: userRecords, upsertSpy } = buildUserRecords() - upsertSpy.mockRejectedValueOnce(new Error('disk full')) + const { store: userRecords } = buildUserRecords() + + await migrateLegacyAuth({ + serviceName: SERVICE, + legacyAccount: LEGACY, + userRecords, + loadLegacyPlaintextToken: async () => null, + identifyAccount: async () => ({ + id: 'user-99', + label: 'sensitive@email.example', + email: 'sensitive@email.example', + }), + silent: false, + logPrefix: 'td', + }) + + const lines = consoleError.mock.calls.flat().join('\n') + expect(lines).toContain('user-99') + expect(lines).not.toContain('sensitive@email.example') + }) + + it('the skip line is generic and does not echo the raw exception text', async () => { + const km = buildKeyringMap() + km.slots.set(LEGACY, { secret: 'legacy_tok' }) + mockedCreateSecureStore.mockImplementation(km.create) + const { store: userRecords } = buildUserRecords() const result = await migrateLegacyAuth({ serviceName: SERVICE, legacyAccount: LEGACY, userRecords, loadLegacyPlaintextToken: async () => null, - identifyAccount: async () => ({ id: '99', email: 'me@x.io' }), - silent: true, + identifyAccount: async () => { + throw new Error('email leak: sensitive@email.example at /Users/me/.config/x') + }, + silent: false, + logPrefix: 'td', }) expect(result.status).toBe('skipped') - expect(km.slots.get('user-99')?.secret).toBeNull() + const lines = consoleError.mock.calls.flat().join('\n') + expect(lines).toContain('could not identify user') + expect(lines).not.toContain('sensitive@email.example') + expect(lines).not.toContain('/Users/me/.config/x') + // The raw detail is preserved on the result for in-process callers. + expect(result.reason).toContain('sensitive@email.example') }) }) diff --git a/src/auth/keyring/migrate.ts b/src/auth/keyring/migrate.ts index 8a2bb24..50479a1 100644 --- a/src/auth/keyring/migrate.ts +++ b/src/auth/keyring/migrate.ts @@ -1,11 +1,12 @@ import { getErrorMessage } from '../../errors.js' import type { AuthAccount } from '../types.js' +import { writeRecordWithKeyringFallback } from './record-write.js' import { createSecureStore, DEFAULT_ACCOUNT_FOR_USER, SecureStoreUnavailableError, } from './secure-store.js' -import type { UserRecord, UserRecordStore } from './types.js' +import type { UserRecordStore } from './types.js' export type MigrateLegacyAuthOptions = { serviceName: string @@ -40,10 +41,28 @@ export type MigrateLegacyAuthOptions = { export type MigrateAuthResult = { status: 'already-migrated' | 'no-legacy-state' | 'migrated' | 'skipped' + /** + * Internal-only failure detail for the call site (e.g. doctor commands). + * **Not** emitted to stderr — see `skipped()` for why. + */ reason?: string migratedAccount?: TAccount } +/** Internal `SkipReason` codes — kept generic for stderr so consumer-supplied error text can't leak PII to logs. */ +type SkipReason = + | 'identify-failed' + | 'keyring-write-failed' + | 'user-record-write-failed' + | 'cleanup-failed' + +const SKIP_REASON_MESSAGES: Record = { + 'identify-failed': 'could not identify user', + 'keyring-write-failed': 'failed to write user-scoped credential', + 'user-record-write-failed': 'failed to update user records', + 'cleanup-failed': 'failed to clean up legacy state', +} + /** * One-time migration of a v1 single-user auth state into a v2 multi-user * shape. Best-effort: any failure (offline keyring, network error fetching @@ -95,53 +114,38 @@ export async function migrateLegacyAuth( try { account = await identifyAccount(legacyToken) } catch (error) { - return skipped(silent, logPrefix, `could not identify user (${getErrorMessage(error)})`) - } - - const perUserStore = createSecureStore({ - serviceName, - account: accountForUser(account.id), - }) - - let storedSecurely = false - try { - await perUserStore.setSecret(legacyToken) - storedSecurely = true - } catch (error) { - if (!(error instanceof SecureStoreUnavailableError)) { - return skipped( - silent, - logPrefix, - `failed to write user-scoped credential (${getErrorMessage(error)})`, - ) - } + return skipped(silent, logPrefix, 'identify-failed', getErrorMessage(error)) } - const record: UserRecord = storedSecurely - ? { id: account.id, account } - : { id: account.id, account, fallbackToken: legacyToken } - try { - await userRecords.upsert(record) + await writeRecordWithKeyringFallback({ + serviceName, + accountForUser, + userRecords, + account, + token: legacyToken, + }) } catch (error) { - if (storedSecurely) { - try { - await perUserStore.deleteSecret() - } catch { - // best-effort rollback - } - } + // `writeRecordWithKeyringFallback` only rethrows non-keyring errors + // (i.e. `userRecords.upsert` failures or unexpected keyring errors). + // SecureStoreUnavailableError is internally caught by the helper and + // never propagates here. return skipped( silent, logPrefix, - `failed to update user records (${getErrorMessage(error)})`, + error instanceof SecureStoreUnavailableError + ? 'keyring-write-failed' + : 'user-record-write-failed', + getErrorMessage(error), ) } + // Default promotion is best-effort — the user record is durable and + // ` account use` can fix things up later. try { await userRecords.setDefaultId(account.id) } catch { - // non-fatal — the record is written; setting a default can be retried later. + // non-fatal } try { @@ -191,13 +195,24 @@ async function readLegacyToken(opts: { return null } +/** + * Emit the migration skip line. The stderr text is a fixed phrase keyed off + * `SkipReason` so consumer-supplied callbacks (`identifyAccount`, the + * `UserRecordStore`, …) can't leak emails, paths, or auth diagnostics into + * logs. The raw error message is still attached to the returned + * `MigrateAuthResult.reason` for in-process callers that need it (doctor + * commands, tests). + */ function skipped( silent: boolean | undefined, logPrefix: string, - reason: string, + reasonCode: SkipReason, + detail: string, ): MigrateAuthResult { if (!silent) { - console.error(`${logPrefix}: skipped legacy auth migration — ${reason}.`) + console.error( + `${logPrefix}: skipped legacy auth migration — ${SKIP_REASON_MESSAGES[reasonCode]}.`, + ) } - return { status: 'skipped', reason } + return { status: 'skipped', reason: `${reasonCode}: ${detail}` } } diff --git a/src/auth/keyring/record-write.ts b/src/auth/keyring/record-write.ts new file mode 100644 index 0000000..e506ab5 --- /dev/null +++ b/src/auth/keyring/record-write.ts @@ -0,0 +1,71 @@ +import type { AuthAccount } from '../types.js' +import { createSecureStore, type SecureStore, SecureStoreUnavailableError } from './secure-store.js' +import type { UserRecord, UserRecordStore } from './types.js' + +export type WriteRecordOptions = { + serviceName: string + accountForUser: (id: string) => string + userRecords: UserRecordStore + account: TAccount + token: string +} + +export type WriteRecordResult = { + /** `true` when the secret landed in the OS keyring; `false` when the keyring was unavailable and the token was written to `fallbackToken` on the user record. */ + storedSecurely: boolean +} + +/** + * 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. + * + * 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. + */ +export async function writeRecordWithKeyringFallback( + options: WriteRecordOptions, +): Promise { + const { serviceName, accountForUser, userRecords, account, token } = options + const trimmed = token.trim() + const secureStore: SecureStore = createSecureStore({ + serviceName, + account: accountForUser(account.id), + }) + + let storedSecurely = false + try { + await secureStore.setSecret(trimmed) + storedSecurely = true + } catch (error) { + if (!(error instanceof SecureStoreUnavailableError)) throw error + } + + const record: UserRecord = storedSecurely + ? { id: account.id, account } + : { id: account.id, account, fallbackToken: trimmed } + + 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 + } + + return { storedSecurely } +} diff --git a/src/auth/keyring/token-store.test.ts b/src/auth/keyring/token-store.test.ts index 0d7405a..f315f12 100644 --- a/src/auth/keyring/token-store.test.ts +++ b/src/auth/keyring/token-store.test.ts @@ -1,8 +1,12 @@ import { beforeEach, describe, expect, it, vi } from 'vitest' -import { SecureStoreUnavailableError, type SecureStore } from './secure-store.js' +import { + buildKeyringMap, + buildSingleSlot, + buildUserRecords, +} from '../../test-support/keyring-mocks.js' +import { SecureStoreUnavailableError } from './secure-store.js' import { createKeyringTokenStore } from './token-store.js' -import type { UserRecord, UserRecordStore } from './types.js' vi.mock('./secure-store.js', async () => { const actual = await vi.importActual('./secure-store.js') @@ -21,130 +25,6 @@ type Account = { email: string } -type Slot = { - secret: string | null - getErr?: unknown - setErr?: unknown - delErr?: unknown -} - -function buildKeyringMap(): { - create: (args: { serviceName: string; account: string }) => SecureStore - slots: Map - deleteCalls: Map -} { - const slots = new Map() - const deleteCalls = new Map() - function getSlot(account: string): Slot { - let slot = slots.get(account) - if (!slot) { - slot = { secret: null } - slots.set(account, slot) - } - return slot - } - return { - slots, - deleteCalls, - create({ account }) { - return { - async getSecret() { - const slot = getSlot(account) - if (slot.getErr) throw slot.getErr - return slot.secret - }, - async setSecret(secret) { - const slot = getSlot(account) - if (slot.setErr) throw slot.setErr - slot.secret = secret - }, - async deleteSecret() { - deleteCalls.set(account, (deleteCalls.get(account) ?? 0) + 1) - const slot = getSlot(account) - if (slot.delErr) throw slot.delErr - const had = slot.secret !== null - slot.secret = null - return had - }, - } - }, - } -} - -function buildSingleSlot(initial: { secret?: string | null } = {}): SecureStore & { - getSpy: ReturnType - setSpy: ReturnType - deleteSpy: ReturnType -} { - const state = { secret: initial.secret ?? null } - const getSpy = vi.fn(async () => state.secret) - const setSpy = vi.fn(async (secret: string) => { - state.secret = secret - }) - const deleteSpy = vi.fn(async () => { - const had = state.secret !== null - state.secret = null - return had - }) - return { - getSpy, - setSpy, - deleteSpy, - async getSecret() { - return getSpy() - }, - async setSecret(secret: string) { - return setSpy(secret) - }, - async deleteSecret() { - return deleteSpy() - }, - } -} - -function buildUserRecords(): { - store: UserRecordStore - state: { - records: Map> - defaultId: string | null - } - upsertSpy: ReturnType - removeSpy: ReturnType - setDefaultSpy: ReturnType -} { - const state = { - records: new Map>(), - defaultId: null as string | null, - } - const upsertSpy = vi.fn(async (record: UserRecord) => { - state.records.set(record.id, record) - }) - const removeSpy = vi.fn(async (id: string) => { - state.records.delete(id) - }) - const setDefaultSpy = vi.fn(async (id: string | null) => { - state.defaultId = id - }) - const store: UserRecordStore = { - async list() { - return [...state.records.values()] - }, - async getById(id) { - return state.records.get(id) ?? null - }, - upsert: upsertSpy, - remove: removeSpy, - async getDefaultId() { - return state.defaultId - }, - setDefaultId: setDefaultSpy, - describeLocation() { - return '/tmp/fake/config.json' - }, - } - return { store, state, upsertSpy, removeSpy, setDefaultSpy } -} - const SERVICE = 'cli-core-test' const account: Account = { id: '42', label: 'me', email: 'a@b.c' } @@ -156,7 +36,7 @@ describe('createKeyringTokenStore', () => { it('round-trips set → active → clear when the keyring is online', async () => { const keyring = buildSingleSlot() mockedCreateSecureStore.mockReturnValue(keyring) - const { store: userRecords, state, upsertSpy } = buildUserRecords() + const { store: userRecords, state, upsertSpy } = buildUserRecords() const store = createKeyringTokenStore({ serviceName: SERVICE, userRecords }) @@ -180,7 +60,7 @@ describe('createKeyringTokenStore', () => { const keyring = buildSingleSlot() keyring.setSpy.mockRejectedValueOnce(new SecureStoreUnavailableError('no dbus')) mockedCreateSecureStore.mockReturnValue(keyring) - const { store: userRecords, state } = buildUserRecords() + const { store: userRecords, state } = buildUserRecords() const store = createKeyringTokenStore({ serviceName: SERVICE, userRecords }) @@ -202,7 +82,7 @@ describe('createKeyringTokenStore', () => { it('rolls back the keyring write when the user record upsert fails', async () => { const keyring = buildSingleSlot() mockedCreateSecureStore.mockReturnValue(keyring) - const { store: userRecords, upsertSpy } = buildUserRecords() + const { store: userRecords, upsertSpy } = buildUserRecords() upsertSpy.mockRejectedValueOnce(new Error('disk full')) const store = createKeyringTokenStore({ serviceName: SERVICE, userRecords }) @@ -215,7 +95,7 @@ describe('createKeyringTokenStore', () => { it('set() still succeeds when the best-effort default promotion fails', async () => { const keyring = buildSingleSlot() mockedCreateSecureStore.mockReturnValue(keyring) - const { store: userRecords, state, setDefaultSpy } = buildUserRecords() + const { store: userRecords, state, setDefaultSpy } = buildUserRecords() setDefaultSpy.mockRejectedValueOnce(new Error('default-write blew up')) const store = createKeyringTokenStore({ serviceName: SERVICE, userRecords }) @@ -230,7 +110,7 @@ describe('createKeyringTokenStore', () => { const keyring = buildSingleSlot({ secret: 'tok' }) keyring.getSpy.mockRejectedValueOnce(new SecureStoreUnavailableError('locked')) mockedCreateSecureStore.mockReturnValue(keyring) - const { store: userRecords, state } = buildUserRecords() + const { store: userRecords, state } = buildUserRecords() state.records.set('42', { id: '42', account }) state.defaultId = '42' @@ -240,18 +120,17 @@ describe('createKeyringTokenStore', () => { }) it('returns null from active() when no records exist', async () => { - mockedCreateSecureStore.mockReturnValue(buildSingleSlot()) - const { store: userRecords } = buildUserRecords() - + const { store: userRecords } = buildUserRecords() const store = createKeyringTokenStore({ serviceName: SERVICE, userRecords }) await expect(store.active()).resolves.toBeNull() + expect(mockedCreateSecureStore).not.toHaveBeenCalled() }) it('picks the lone user when no default is set', async () => { const keyring = buildSingleSlot({ secret: 'tok' }) mockedCreateSecureStore.mockReturnValue(keyring) - const { store: userRecords, state } = buildUserRecords() + const { store: userRecords, state } = buildUserRecords() state.records.set('42', { id: '42', account }) const store = createKeyringTokenStore({ serviceName: SERVICE, userRecords }) @@ -260,8 +139,7 @@ describe('createKeyringTokenStore', () => { }) it('returns null when multiple users exist and no default is set', async () => { - mockedCreateSecureStore.mockReturnValue(buildSingleSlot()) - const { store: userRecords, state } = buildUserRecords() + const { store: userRecords, state } = buildUserRecords() state.records.set('1', { id: '1', account: { ...account, id: '1' } }) state.records.set('2', { id: '2', account: { ...account, id: '2' } }) @@ -273,7 +151,7 @@ describe('createKeyringTokenStore', () => { it('does not overwrite an existing default when a second user is added', async () => { const keyring = buildSingleSlot() mockedCreateSecureStore.mockReturnValue(keyring) - const { store: userRecords, state } = buildUserRecords() + const { store: userRecords, state } = buildUserRecords() state.defaultId = '1' state.records.set('1', { id: '1', account: { ...account, id: '1' } }) @@ -285,19 +163,18 @@ describe('createKeyringTokenStore', () => { }) it('clear() is a no-op when no record exists', async () => { - mockedCreateSecureStore.mockReturnValue(buildSingleSlot()) - const { store: userRecords } = buildUserRecords() - + const { store: userRecords } = buildUserRecords() const store = createKeyringTokenStore({ serviceName: SERVICE, userRecords }) await expect(store.clear()).resolves.toBeUndefined() expect(store.getLastClearResult()).toBeUndefined() + expect(mockedCreateSecureStore).not.toHaveBeenCalled() }) it('clear() still calls the keyring delete when a fallbackToken is present (orphan cleanup)', async () => { const keyring = buildSingleSlot({ secret: 'orphan_from_earlier_write' }) mockedCreateSecureStore.mockReturnValue(keyring) - const { store: userRecords, state } = buildUserRecords() + const { store: userRecords, state } = buildUserRecords() state.records.set('42', { id: '42', account, fallbackToken: 'tok_plain' }) state.defaultId = '42' @@ -318,7 +195,7 @@ describe('createKeyringTokenStore', () => { const keyring = buildSingleSlot({ secret: 'tok' }) keyring.deleteSpy.mockRejectedValueOnce(new SecureStoreUnavailableError('offline')) mockedCreateSecureStore.mockReturnValue(keyring) - const { store: userRecords, state } = buildUserRecords() + const { store: userRecords, state } = buildUserRecords() state.records.set('42', { id: '42', account }) state.defaultId = '42' @@ -330,10 +207,29 @@ describe('createKeyringTokenStore', () => { expect(store.getLastClearResult()).toMatchObject({ storage: 'config-file' }) }) + it('clear() still deletes the keyring slot even when setDefaultId(null) throws', async () => { + const keyring = buildSingleSlot({ secret: 'tok' }) + mockedCreateSecureStore.mockReturnValue(keyring) + const { store: userRecords, state, setDefaultSpy } = buildUserRecords() + state.records.set('42', { id: '42', account }) + state.defaultId = '42' + setDefaultSpy.mockRejectedValueOnce(new Error('disk full')) + + const store = createKeyringTokenStore({ serviceName: SERVICE, userRecords }) + + await store.clear() + + // Default pointer write blew up, but the keyring entry was still + // cleaned up — otherwise the record's old credential would become + // an unreachable orphan. + expect(keyring.deleteSpy).toHaveBeenCalledTimes(1) + expect(state.records.size).toBe(0) + }) + it('uses a custom accountForUser slug when provided', async () => { const keyring = buildSingleSlot() mockedCreateSecureStore.mockReturnValue(keyring) - const { store: userRecords } = buildUserRecords() + const { store: userRecords } = buildUserRecords() const store = createKeyringTokenStore({ serviceName: SERVICE, @@ -349,11 +245,24 @@ describe('createKeyringTokenStore', () => { }) }) + it('skips getDefaultId when an explicit ref is supplied (hot path)', async () => { + const keyring = buildSingleSlot({ secret: 'tok' }) + mockedCreateSecureStore.mockReturnValue(keyring) + const { store: userRecords, state } = buildUserRecords() + state.records.set('42', { id: '42', account }) + const getDefaultSpy = vi.spyOn(userRecords, 'getDefaultId') + + const store = createKeyringTokenStore({ serviceName: SERVICE, userRecords }) + + await store.active('42') + expect(getDefaultSpy).not.toHaveBeenCalled() + }) + describe('AccountRef support (keyed per-user slots)', () => { function buildMultiUser() { const km = buildKeyringMap() mockedCreateSecureStore.mockImplementation(km.create) - const { store: userRecords, state, removeSpy } = buildUserRecords() + const { store: userRecords, state, removeSpy } = buildUserRecords() state.records.set('1', { id: '1', account: { id: '1', label: 'alice', email: 'a@b' } }) state.records.set('2', { id: '2', @@ -415,7 +324,7 @@ describe('createKeyringTokenStore', () => { expect(state.defaultId).toBeNull() // user-1's slot was cleared; user-2's slot was never touched. expect(km.slots.get('user-1')?.secret).toBeNull() - expect(km.deleteCalls.get('user-1') ?? 0).toBeGreaterThan(0) + expect((km.deleteCalls.get('user-1') ?? 0) > 0).toBe(true) expect(km.deleteCalls.has('user-2')).toBe(false) }) @@ -431,7 +340,7 @@ describe('createKeyringTokenStore', () => { it('honours a custom matchAccount predicate', async () => { const km = buildKeyringMap() mockedCreateSecureStore.mockImplementation(km.create) - const { store: userRecords, state } = buildUserRecords() + const { store: userRecords, state } = buildUserRecords() state.records.set('1', { id: '1', account: { id: '1', email: 'Alice@x.io' } }) km.slots.set('user-1', { secret: 'tok' }) @@ -449,8 +358,7 @@ describe('createKeyringTokenStore', () => { describe('list() + setDefault()', () => { it('list() returns every account with the default marker', async () => { - mockedCreateSecureStore.mockReturnValue(buildSingleSlot()) - const { store: userRecords, state } = buildUserRecords() + const { store: userRecords, state } = buildUserRecords() state.records.set('1', { id: '1', account: { id: '1', email: 'a@b' } }) state.records.set('2', { id: '2', account: { id: '2', email: 'c@d' } }) state.defaultId = '2' @@ -463,18 +371,28 @@ describe('createKeyringTokenStore', () => { expect(all.find((entry) => entry.account.id === '1')?.isDefault).toBe(false) }) - it('list() returns an empty array when nothing is stored', async () => { - mockedCreateSecureStore.mockReturnValue(buildSingleSlot()) - const { store: userRecords } = buildUserRecords() + it('list() marks a single record as default even when no defaultId is pinned (matches active())', async () => { + const { store: userRecords, state } = buildUserRecords() + state.records.set('42', { id: '42', account }) + // No defaultId set — active() treats the lone record as the + // implicit default, so list() must too. const store = createKeyringTokenStore({ serviceName: SERVICE, userRecords }) + const all = await store.list() + expect(all).toEqual([{ account, isDefault: true }]) + }) + + it('list() returns an empty array when nothing is stored', async () => { + const { store: userRecords } = buildUserRecords() + const store = createKeyringTokenStore({ serviceName: SERVICE, userRecords }) + await expect(store.list()).resolves.toEqual([]) + expect(mockedCreateSecureStore).not.toHaveBeenCalled() }) it('setDefault(ref) marks the matching account as default', async () => { - mockedCreateSecureStore.mockReturnValue(buildSingleSlot()) - const { store: userRecords, state } = buildUserRecords() + const { store: userRecords, state } = buildUserRecords() state.records.set('1', { id: '1', account: { id: '1', label: 'a', email: 'a@b' } }) state.records.set('2', { id: '2', account: { id: '2', label: 'b', email: 'c@d' } }) state.defaultId = '1' @@ -483,11 +401,11 @@ describe('createKeyringTokenStore', () => { await store.setDefault('b') expect(state.defaultId).toBe('2') + expect(mockedCreateSecureStore).not.toHaveBeenCalled() }) it('setDefault(ref) throws ACCOUNT_NOT_FOUND on a miss', async () => { - mockedCreateSecureStore.mockReturnValue(buildSingleSlot()) - const { store: userRecords, state } = buildUserRecords() + const { store: userRecords, state } = buildUserRecords() state.records.set('1', { id: '1', account }) const store = createKeyringTokenStore({ serviceName: SERVICE, userRecords }) diff --git a/src/auth/keyring/token-store.ts b/src/auth/keyring/token-store.ts index 1f1e677..441950b 100644 --- a/src/auth/keyring/token-store.ts +++ b/src/auth/keyring/token-store.ts @@ -1,6 +1,7 @@ import { CliError } from '../../errors.js' import type { AccountRef, AuthAccount, TokenStore } from '../types.js' import { accountNotFoundError } from '../user-flag.js' +import { writeRecordWithKeyringFallback } from './record-write.js' import { createSecureStore, DEFAULT_ACCOUNT_FOR_USER, @@ -47,10 +48,16 @@ const DEFAULT_MATCH_ACCOUNT = ( * without D-Bus, missing native binary, locked Keychain, …) so the CLI keeps * working at the cost of a visible warning. * - * Write order is deliberate: keyring first, then `userRecords.upsert`. If the - * upsert fails after a successful keyring write, the keyring entry is rolled - * back via `deleteSecret()` to avoid orphan credentials for a user that - * cli-core never managed to record. + * Read order in `active()` is `fallbackToken` first, then the keyring. That + * matches the write semantics in `writeRecordWithKeyringFallback`: when the + * keyring is online the record is written with no `fallbackToken`, so the + * keyring read is the only path. When the keyring is offline the token is + * parked on the record and must be reachable on every subsequent read. + * + * Write order is keyring first, then `userRecords.upsert`. If the upsert + * fails after a successful keyring write, the keyring entry is rolled back + * via `deleteSecret()` to avoid orphan credentials for a user that cli-core + * never managed to record. * * Clear order is the inverse: record removal first (the source of truth that * the rest of the CLI reads), then keyring delete. A keyring delete failure @@ -72,12 +79,19 @@ export function createKeyringTokenStore( } type Snapshot = { records: UserRecord[]; defaultId: string | null } - async function readSnapshot(): Promise { - const [records, defaultId] = await Promise.all([ - userRecords.list(), - userRecords.getDefaultId(), - ]) - return { records, defaultId } + + // `getDefaultId` is only needed when no ref is supplied — every + // authenticated command sits on `active()`, so skipping the extra config + // read on the `--user ` path matters for latency. + async function readSnapshot(needsDefault: boolean): Promise { + if (needsDefault) { + const [records, defaultId] = await Promise.all([ + userRecords.list(), + userRecords.getDefaultId(), + ]) + return { records, defaultId } + } + return { records: await userRecords.list(), defaultId: null } } function findByRef(snapshot: Snapshot, ref: AccountRef): UserRecord | null { @@ -92,9 +106,16 @@ export function createKeyringTokenStore( return snapshot.records.length === 1 ? snapshot.records[0] : null } + function fallbackResult(action: string): TokenStorageResult { + return { + storage: 'config-file', + warning: buildFallbackWarning(action, userRecords.describeLocation()), + } + } + return { async active(ref) { - const snapshot = await readSnapshot() + const snapshot = await readSnapshot(ref === undefined) const record = ref === undefined ? resolveDefault(snapshot) : findByRef(snapshot, ref) if (!record) return null @@ -113,6 +134,9 @@ export function createKeyringTokenStore( // 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', @@ -124,37 +148,13 @@ export function createKeyringTokenStore( }, async set(account, token) { - const trimmed = token.trim() - const secureStore = secureStoreFor(account.id) - - let storedSecurely = false - try { - await secureStore.setSecret(trimmed) - storedSecurely = true - } catch (error) { - if (!(error instanceof SecureStoreUnavailableError)) throw error - } - - const record: UserRecord = storedSecurely - ? { id: account.id, account } - : { id: account.id, account, fallbackToken: trimmed } - - try { - await userRecords.upsert(record) - } catch (error) { - // The user record is the source of truth. If we can't write it, - // a stranded keyring entry would leak credentials for an account - // cli-core never managed to register. Best-effort rollback, then - // re-raise the original error so the caller sees the real cause. - if (storedSecurely) { - try { - await secureStore.deleteSecret() - } catch { - // ignore — the user record failure is what matters - } - } - throw error - } + const { storedSecurely } = await writeRecordWithKeyringFallback({ + serviceName, + accountForUser, + userRecords, + account, + token, + }) // Best-effort default promotion: the record is already persisted, // so a failure here must not turn into `AUTH_STORE_WRITE_FAILED` @@ -170,17 +170,11 @@ export function createKeyringTokenStore( lastStorageResult = storedSecurely ? { storage: 'secure-store' } - : { - storage: 'config-file', - warning: buildFallbackWarning( - 'token saved as plaintext in', - userRecords.describeLocation(), - ), - } + : fallbackResult('token saved as plaintext in') }, async clear(ref) { - const snapshot = await readSnapshot() + const snapshot = await readSnapshot(ref === undefined) const record = ref === undefined ? resolveDefault(snapshot) : findByRef(snapshot, ref) if (!record) { lastClearResult = undefined @@ -189,10 +183,20 @@ export function createKeyringTokenStore( await userRecords.remove(record.id) - if (snapshot.defaultId === record.id) { - await userRecords.setDefaultId(null) + // Default un-pinning is best-effort: a failure here must not + // skip the keyring delete below, otherwise we leave an + // unreachable orphan secret behind for the just-removed record. + const wasDefault = await readWasDefault(userRecords, record.id, snapshot) + if (wasDefault) { + try { + await userRecords.setDefaultId(null) + } catch { + // best-effort + } } + 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 @@ -201,32 +205,19 @@ export function createKeyringTokenStore( try { await secureStoreFor(record.id).deleteSecret() lastClearResult = - record.fallbackToken !== undefined - ? { - storage: 'config-file', - warning: buildFallbackWarning( - 'local auth state cleared in', - userRecords.describeLocation(), - ), - } - : { storage: 'secure-store' } + record.fallbackToken !== undefined ? fallbackClear : { storage: 'secure-store' } } catch (error) { if (!(error instanceof SecureStoreUnavailableError)) throw error - lastClearResult = { - storage: 'config-file', - warning: buildFallbackWarning( - 'local auth state cleared in', - userRecords.describeLocation(), - ), - } + lastClearResult = fallbackClear } }, async list() { - const { records, defaultId } = await readSnapshot() - return records.map((record) => ({ + const snapshot = await readSnapshot(true) + const implicitDefault = resolveDefault(snapshot) + return snapshot.records.map((record) => ({ account: record.account, - isDefault: record.id === defaultId, + isDefault: record.id === implicitDefault?.id, })) }, @@ -249,6 +240,17 @@ export function createKeyringTokenStore( } } +async function readWasDefault( + userRecords: UserRecordStore, + id: string, + snapshot: { defaultId: string | null }, +): Promise { + // `clear(ref)` skipped the default read in `readSnapshot`, so check now. + if (snapshot.defaultId !== null) return snapshot.defaultId === id + const current = await userRecords.getDefaultId() + return current === id +} + function buildFallbackWarning(action: string, location: string): string { return `${SECURE_STORE_DESCRIPTION} unavailable; ${action} ${location}` } diff --git a/src/auth/keyring/types.ts b/src/auth/keyring/types.ts index 5f2a82e..bdf724d 100644 --- a/src/auth/keyring/types.ts +++ b/src/auth/keyring/types.ts @@ -18,9 +18,12 @@ export type UserRecord = { account: TAccount /** * Plaintext token, present only when the keyring was unavailable at write - * time. cli-core prefers a keyring read over this field; consumers should - * surface its presence as a security-relevant fact (it is the same - * material that would otherwise live in the OS credential manager). + * time. The runtime reads it in preference to the keyring slot, so a + * stale fallback would mask a fresh keyring-backed write — consumers + * implementing `upsert` as replace-not-merge (per the contract below) + * guarantees the field is cleared on every successful keyring write. + * Surface its presence as security-relevant: it is the same material + * that would otherwise live in the OS credential manager. */ fallbackToken?: string } @@ -33,7 +36,6 @@ export type UserRecord = { */ export type UserRecordStore = { list(): Promise[]> - getById(id: string): Promise | null> /** * **Replace**, do not merge. The persisted record must equal `record` field * for field — an absent `fallbackToken` means "no plaintext token", and a diff --git a/src/auth/logout.test.ts b/src/auth/logout.test.ts index e161583..f4a4a7f 100644 --- a/src/auth/logout.test.ts +++ b/src/auth/logout.test.ts @@ -364,4 +364,56 @@ describe('attachLogoutCommand', () => { expect(built.clearSpy).not.toHaveBeenCalled() expect(logSpy).not.toHaveBeenCalled() }) + + it('proceeds with clear(ref) when active(ref) throws AUTH_STORE_READ_FAILED', async () => { + // A matching record exists but the keyring is offline, so the + // pre-flight can't return a snapshot. `logout --user me` should + // still clear the record (it doesn't need the token); only the + // optional `revokeToken` is skipped because there's no token to + // send to the server. + const activeSpy = vi.fn(async () => { + throw new CliError('AUTH_STORE_READ_FAILED', 'keyring offline') + }) + const clearSpy = vi.fn(async () => undefined) + const revokeSpy = vi.fn() + const store: TokenStore = { + active: activeSpy, + set: vi.fn(), + clear: clearSpy, + list: vi.fn(async () => [{ account, isDefault: true }]), + setDefault: vi.fn(), + } + const { program, onCleared } = build({ revokeToken: revokeSpy }, store) + + await program.parseAsync(['node', 'cli', 'auth', 'logout', '--user', 'me']) + + expect(clearSpy).toHaveBeenCalledWith('me') + expect(revokeSpy).not.toHaveBeenCalled() + expect(logSpy).toHaveBeenCalledWith('✓ Logged out') + expect(onCleared).toHaveBeenCalledWith({ + account: null, + view: { json: false, ndjson: false }, + flags: {}, + }) + }) + + it('still propagates non-read errors from the snapshot pre-flight', async () => { + const activeSpy = vi.fn(async () => { + throw new CliError('AUTH_STORE_WRITE_FAILED', 'something else') + }) + const clearSpy = vi.fn(async () => undefined) + const store: TokenStore = { + active: activeSpy, + set: vi.fn(), + clear: clearSpy, + list: vi.fn(async () => []), + setDefault: vi.fn(), + } + const { program } = build({ onCleared: undefined }, store) + + await expect( + program.parseAsync(['node', 'cli', 'auth', 'logout', '--user', 'me']), + ).rejects.toMatchObject({ code: 'AUTH_STORE_WRITE_FAILED' }) + expect(clearSpy).not.toHaveBeenCalled() + }) }) diff --git a/src/auth/logout.ts b/src/auth/logout.ts index 00a1026..88b3fc2 100644 --- a/src/auth/logout.ts +++ b/src/auth/logout.ts @@ -1,4 +1,5 @@ import type { Command } from 'commander' +import { CliError } from '../errors.js' import { formatJson } from '../json.js' import type { ViewOptions } from '../options.js' import type { AuthAccount, TokenStore } from './types.js' @@ -73,7 +74,19 @@ export function attachLogoutCommand( let snapshot: { token: string; account: TAccount } | null = null if (needsSnapshot) { if (ref !== undefined) { - snapshot = await requireSnapshotForRef(options.store, ref) + try { + snapshot = await requireSnapshotForRef(options.store, ref) + } catch (error) { + // `AUTH_STORE_READ_FAILED` means the ref matched a stored + // record but the token couldn't be read (e.g. keyring + // offline). Local clear can still proceed without the + // token; we just skip the revoke and continue. Any other + // typed failure (notably `ACCOUNT_NOT_FOUND` from a + // genuine miss) propagates as before. + if (!(error instanceof CliError && error.code === 'AUTH_STORE_READ_FAILED')) { + throw error + } + } } else { try { snapshot = await options.store.active(ref) diff --git a/src/auth/status.test.ts b/src/auth/status.test.ts index d789064..35be11f 100644 --- a/src/auth/status.test.ts +++ b/src/auth/status.test.ts @@ -246,4 +246,21 @@ describe('attachStatusCommand', () => { code: 'ACCOUNT_NOT_FOUND', }) }) + + it('surfaces AUTH_STORE_READ_FAILED end-to-end (does not collapse to ACCOUNT_NOT_FOUND)', async () => { + const store: TokenStore = { + active: vi.fn(async () => { + throw new CliError('AUTH_STORE_READ_FAILED', 'keyring offline') + }), + set: vi.fn(), + clear: vi.fn(), + list: vi.fn(async () => [{ account, isDefault: true }]), + setDefault: vi.fn(), + } + const { program } = build({}, store) + + await expect( + program.parseAsync(['node', 'cli', 'auth', 'status', '--user', 'me']), + ).rejects.toMatchObject({ code: 'AUTH_STORE_READ_FAILED' }) + }) }) diff --git a/src/auth/token-view.test.ts b/src/auth/token-view.test.ts index 2907151..96f810d 100644 --- a/src/auth/token-view.test.ts +++ b/src/auth/token-view.test.ts @@ -189,4 +189,25 @@ describe('attachTokenViewCommand', () => { }) expect(stdoutSpy).not.toHaveBeenCalled() }) + + it('surfaces AUTH_STORE_READ_FAILED end-to-end (does not collapse to ACCOUNT_NOT_FOUND)', async () => { + const program = new Command() + program.exitOverride() + const auth = program.command('auth') + const store: TokenStore = { + active: vi.fn(async () => { + throw new CliError('AUTH_STORE_READ_FAILED', 'keyring offline') + }), + set: vi.fn(), + clear: vi.fn(), + list: vi.fn(async () => [{ account, isDefault: true }]), + setDefault: vi.fn(), + } + attachTokenViewCommand(auth, { store }) + + await expect( + program.parseAsync(['node', 'cli', 'auth', 'token', '--user', 'me']), + ).rejects.toMatchObject({ code: 'AUTH_STORE_READ_FAILED' }) + expect(stdoutSpy).not.toHaveBeenCalled() + }) }) diff --git a/src/test-support/keyring-mocks.ts b/src/test-support/keyring-mocks.ts new file mode 100644 index 0000000..d125f65 --- /dev/null +++ b/src/test-support/keyring-mocks.ts @@ -0,0 +1,149 @@ +import { vi } from 'vitest' + +import type { SecureStore } from '../auth/keyring/secure-store.js' +import type { UserRecord, UserRecordStore } from '../auth/keyring/types.js' +import type { AuthAccount } from '../auth/types.js' + +// Test mocks shared between the keyring unit suites. Lives under +// `src/test-support/` so it's excluded from the build (per +// `tsconfig.build.json`) and never reaches consumers via `dist/`. + +export type KeyringSlot = { + secret: string | null + getErr?: unknown + setErr?: unknown + delErr?: unknown +} + +export type KeyringMap = { + create: (args: { serviceName: string; account: string }) => SecureStore + slots: Map + deleteCalls: Map +} + +/** + * Keyed multi-slot keyring mock. Each `account` (slug) gets its own state + * blob so tests can verify that `active()` / `clear()` actually route to the + * right per-user slot. Errors can be pre-seeded per slot. + */ +export function buildKeyringMap(): KeyringMap { + const slots = new Map() + const deleteCalls = new Map() + function getSlot(account: string): KeyringSlot { + let slot = slots.get(account) + if (!slot) { + slot = { secret: null } + slots.set(account, slot) + } + return slot + } + return { + slots, + deleteCalls, + create({ account }) { + return { + async getSecret() { + const slot = getSlot(account) + if (slot.getErr) throw slot.getErr + return slot.secret + }, + async setSecret(secret) { + const slot = getSlot(account) + if (slot.setErr) throw slot.setErr + slot.secret = secret + }, + async deleteSecret() { + deleteCalls.set(account, (deleteCalls.get(account) ?? 0) + 1) + const slot = getSlot(account) + if (slot.delErr) throw slot.delErr + const had = slot.secret !== null + slot.secret = null + return had + }, + } + }, + } +} + +export type SingleSlotMock = SecureStore & { + getSpy: ReturnType + setSpy: ReturnType + deleteSpy: ReturnType +} + +/** + * Simple single-slot keyring mock with spies on each method. Use for tests + * that don't care about per-user slot routing. + */ +export function buildSingleSlot(initial: { secret?: string | null } = {}): SingleSlotMock { + const state = { secret: initial.secret ?? null } + const getSpy = vi.fn(async () => state.secret) + const setSpy = vi.fn(async (secret: string) => { + state.secret = secret + }) + const deleteSpy = vi.fn(async () => { + const had = state.secret !== null + state.secret = null + return had + }) + return { + getSpy, + setSpy, + deleteSpy, + async getSecret() { + return getSpy() + }, + async setSecret(secret: string) { + return setSpy(secret) + }, + async deleteSecret() { + return deleteSpy() + }, + } +} + +export type UserRecordsHarness = { + store: UserRecordStore + state: { + records: Map> + defaultId: string | null + } + upsertSpy: ReturnType + removeSpy: ReturnType + setDefaultSpy: ReturnType +} + +/** In-memory `UserRecordStore` with spies on the mutating methods. */ +export function buildUserRecords( + options: { location?: string } = {}, +): UserRecordsHarness { + const location = options.location ?? '/tmp/fake/config.json' + const state = { + records: new Map>(), + defaultId: null as string | null, + } + const upsertSpy = vi.fn(async (record: UserRecord) => { + state.records.set(record.id, record) + }) + const removeSpy = vi.fn(async (id: string) => { + state.records.delete(id) + }) + const setDefaultSpy = vi.fn(async (id: string | null) => { + state.defaultId = id + }) + const store: UserRecordStore = { + async list() { + return [...state.records.values()] + }, + upsert: upsertSpy, + remove: removeSpy, + async getDefaultId() { + return state.defaultId + }, + setDefaultId: setDefaultSpy, + describeLocation() { + return location + }, + } + return { store, state, upsertSpy, removeSpy, setDefaultSpy } +} From 34be9245b36c8d6701d16022329f21ec17d6323a Mon Sep 17 00:00:00 2001 From: Scott Lovegrove Date: Sat, 16 May 2026 13:01:58 +0100 Subject: [PATCH 4/4] test(auth): drop paranoid keyring tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Trims 11 tests that asserted trivial empty-state pass-throughs, duplicated code paths via different inputs, or locked in implementation details rather than behaviour: - secure-store.test.ts: drop the `?? null` empty-slot test and the set/delete variants of the error-wrapping test (all three methods share the same wrap helper, so the get test covers them). - token-store.test.ts: drop the "no record" trio (`active`/`clear`/`list` empty cases), the implementation-detail test asserting `getDefaultId` is skipped on the `--user` path, the label-matching test (already exercised via `setDefault('b')`), and the trivial `clear(ref)` miss no-op. - migrate.test.ts: drop the upsert-rollback test — the rollback now lives in `writeRecordWithKeyringFallback` and is covered by the equivalent token-store test. - token-view.test.ts: drop the `AUTH_STORE_READ_FAILED` propagation test — status.test.ts covers the same code path (both go through `requireSnapshotForRef` with no local catch). Net: 346 → 336 tests passing. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/auth/keyring/migrate.test.ts | 20 ---------- src/auth/keyring/secure-store.test.ts | 31 +-------------- src/auth/keyring/token-store.test.ts | 55 --------------------------- src/auth/token-view.test.ts | 21 ---------- 4 files changed, 1 insertion(+), 126 deletions(-) diff --git a/src/auth/keyring/migrate.test.ts b/src/auth/keyring/migrate.test.ts index d258472..103e6a2 100644 --- a/src/auth/keyring/migrate.test.ts +++ b/src/auth/keyring/migrate.test.ts @@ -167,26 +167,6 @@ describe('migrateLegacyAuth', () => { // Legacy entry must remain so a retry can find it. expect(km.slots.get(LEGACY)?.secret).toBe('legacy_tok') }) - - it('rolls back the keyring write when user-record upsert fails', async () => { - const km = buildKeyringMap() - km.slots.set(LEGACY, { secret: 'legacy_tok' }) - mockedCreateSecureStore.mockImplementation(km.create) - const { store: userRecords, upsertSpy } = buildUserRecords() - upsertSpy.mockRejectedValueOnce(new Error('disk full')) - - const result = await migrateLegacyAuth({ - serviceName: SERVICE, - legacyAccount: LEGACY, - userRecords, - loadLegacyPlaintextToken: async () => null, - identifyAccount: async () => ({ id: '99', email: 'me@x.io' }), - silent: true, - }) - - expect(result.status).toBe('skipped') - expect(km.slots.get('user-99')?.secret).toBeNull() - }) }) describe('migrateLegacyAuth — stderr privacy', () => { diff --git a/src/auth/keyring/secure-store.test.ts b/src/auth/keyring/secure-store.test.ts index 1a3ddba..0fb4875 100644 --- a/src/auth/keyring/secure-store.test.ts +++ b/src/auth/keyring/secure-store.test.ts @@ -43,16 +43,7 @@ describe('createSecureStore', () => { expect(keyringMocks.entry.setPassword).toHaveBeenCalledWith('tok_abcdef') }) - it('returns null when the keyring has no entry for the slot', async () => { - keyringMocks.entry.getPassword.mockResolvedValue(null) - - const { createSecureStore } = await import('./secure-store.js') - const store = createSecureStore({ serviceName: SERVICE, account: ACCOUNT }) - - await expect(store.getSecret()).resolves.toBeNull() - }) - - it('wraps a get failure as SecureStoreUnavailableError', async () => { + it('wraps a keyring failure as SecureStoreUnavailableError', async () => { keyringMocks.entry.getPassword.mockRejectedValue(new Error('Keychain locked')) const { createSecureStore, SecureStoreUnavailableError } = await import('./secure-store.js') @@ -61,26 +52,6 @@ describe('createSecureStore', () => { createSecureStore({ serviceName: SERVICE, account: ACCOUNT }).getSecret(), ).rejects.toBeInstanceOf(SecureStoreUnavailableError) }) - - it('wraps a set failure as SecureStoreUnavailableError', async () => { - keyringMocks.entry.setPassword.mockRejectedValue(new Error('libsecret missing')) - - const { createSecureStore, SecureStoreUnavailableError } = await import('./secure-store.js') - - await expect( - createSecureStore({ serviceName: SERVICE, account: ACCOUNT }).setSecret('x'), - ).rejects.toBeInstanceOf(SecureStoreUnavailableError) - }) - - it('wraps a delete failure as SecureStoreUnavailableError', async () => { - keyringMocks.entry.deleteCredential.mockRejectedValue(new Error('D-Bus down')) - - const { createSecureStore, SecureStoreUnavailableError } = await import('./secure-store.js') - - await expect( - createSecureStore({ serviceName: SERVICE, account: ACCOUNT }).deleteSecret(), - ).rejects.toBeInstanceOf(SecureStoreUnavailableError) - }) }) describe('createSecureStore — missing native binary', () => { diff --git a/src/auth/keyring/token-store.test.ts b/src/auth/keyring/token-store.test.ts index f315f12..34a8a44 100644 --- a/src/auth/keyring/token-store.test.ts +++ b/src/auth/keyring/token-store.test.ts @@ -119,14 +119,6 @@ describe('createKeyringTokenStore', () => { await expect(store.active()).rejects.toMatchObject({ code: 'AUTH_STORE_READ_FAILED' }) }) - it('returns null from active() when no records exist', async () => { - const { store: userRecords } = buildUserRecords() - const store = createKeyringTokenStore({ serviceName: SERVICE, userRecords }) - - await expect(store.active()).resolves.toBeNull() - expect(mockedCreateSecureStore).not.toHaveBeenCalled() - }) - it('picks the lone user when no default is set', async () => { const keyring = buildSingleSlot({ secret: 'tok' }) mockedCreateSecureStore.mockReturnValue(keyring) @@ -162,15 +154,6 @@ describe('createKeyringTokenStore', () => { expect(state.defaultId).toBe('1') }) - it('clear() is a no-op when no record exists', async () => { - const { store: userRecords } = buildUserRecords() - const store = createKeyringTokenStore({ serviceName: SERVICE, userRecords }) - - await expect(store.clear()).resolves.toBeUndefined() - expect(store.getLastClearResult()).toBeUndefined() - expect(mockedCreateSecureStore).not.toHaveBeenCalled() - }) - it('clear() still calls the keyring delete when a fallbackToken is present (orphan cleanup)', async () => { const keyring = buildSingleSlot({ secret: 'orphan_from_earlier_write' }) mockedCreateSecureStore.mockReturnValue(keyring) @@ -245,19 +228,6 @@ describe('createKeyringTokenStore', () => { }) }) - it('skips getDefaultId when an explicit ref is supplied (hot path)', async () => { - const keyring = buildSingleSlot({ secret: 'tok' }) - mockedCreateSecureStore.mockReturnValue(keyring) - const { store: userRecords, state } = buildUserRecords() - state.records.set('42', { id: '42', account }) - const getDefaultSpy = vi.spyOn(userRecords, 'getDefaultId') - - const store = createKeyringTokenStore({ serviceName: SERVICE, userRecords }) - - await store.active('42') - expect(getDefaultSpy).not.toHaveBeenCalled() - }) - describe('AccountRef support (keyed per-user slots)', () => { function buildMultiUser() { const km = buildKeyringMap() @@ -297,14 +267,6 @@ describe('createKeyringTokenStore', () => { expect(snapshot?.token).toBe('tok_b') }) - it('active(ref) matches on label', async () => { - const { userRecords } = buildMultiUser() - const store = createKeyringTokenStore({ serviceName: SERVICE, userRecords }) - - const snapshot = await store.active('alice') - expect(snapshot?.account.id).toBe('1') - }) - it('active(ref) returns null on a miss (attacher translates to ACCOUNT_NOT_FOUND)', async () => { const { userRecords } = buildMultiUser() const store = createKeyringTokenStore({ serviceName: SERVICE, userRecords }) @@ -328,15 +290,6 @@ describe('createKeyringTokenStore', () => { expect(km.deleteCalls.has('user-2')).toBe(false) }) - it('clear(ref) on a miss is a no-op (attacher rejects via active() pre-check)', async () => { - const { userRecords, state } = buildMultiUser() - const store = createKeyringTokenStore({ serviceName: SERVICE, userRecords }) - - await store.clear('nope') - expect(state.records.has('1')).toBe(true) - expect(state.records.has('2')).toBe(true) - }) - it('honours a custom matchAccount predicate', async () => { const km = buildKeyringMap() mockedCreateSecureStore.mockImplementation(km.create) @@ -383,14 +336,6 @@ describe('createKeyringTokenStore', () => { expect(all).toEqual([{ account, isDefault: true }]) }) - it('list() returns an empty array when nothing is stored', async () => { - const { store: userRecords } = buildUserRecords() - const store = createKeyringTokenStore({ serviceName: SERVICE, userRecords }) - - await expect(store.list()).resolves.toEqual([]) - expect(mockedCreateSecureStore).not.toHaveBeenCalled() - }) - it('setDefault(ref) marks the matching account as default', async () => { const { store: userRecords, state } = buildUserRecords() state.records.set('1', { id: '1', account: { id: '1', label: 'a', email: 'a@b' } }) diff --git a/src/auth/token-view.test.ts b/src/auth/token-view.test.ts index 96f810d..2907151 100644 --- a/src/auth/token-view.test.ts +++ b/src/auth/token-view.test.ts @@ -189,25 +189,4 @@ describe('attachTokenViewCommand', () => { }) expect(stdoutSpy).not.toHaveBeenCalled() }) - - it('surfaces AUTH_STORE_READ_FAILED end-to-end (does not collapse to ACCOUNT_NOT_FOUND)', async () => { - const program = new Command() - program.exitOverride() - const auth = program.command('auth') - const store: TokenStore = { - active: vi.fn(async () => { - throw new CliError('AUTH_STORE_READ_FAILED', 'keyring offline') - }), - set: vi.fn(), - clear: vi.fn(), - list: vi.fn(async () => [{ account, isDefault: true }]), - setDefault: vi.fn(), - } - attachTokenViewCommand(auth, { store }) - - await expect( - program.parseAsync(['node', 'cli', 'auth', 'token', '--user', 'me']), - ).rejects.toMatchObject({ code: 'AUTH_STORE_READ_FAILED' }) - expect(stdoutSpy).not.toHaveBeenCalled() - }) })