From cb21fe26814c44cfc627e4668164ee1cd5ffa475 Mon Sep 17 00:00:00 2001 From: Scott Lovegrove Date: Sat, 16 May 2026 13:15:13 +0100 Subject: [PATCH 1/2] feat(auth): add createSecureStore keyring primitive MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Thin cross-platform wrapper around `@napi-rs/keyring` exposed from the `./auth` subpath as a building block for any consumer that needs OS-keyring backed token storage. Every failure mode (missing native binary on an exotic arch, libsecret unreachable on headless Linux / WSL without D-Bus, Keychain locked, …) is normalised into `SecureStoreUnavailableError`. The `@napi-rs/keyring` import is dynamic so a missing binary doesn't crash module load before the typed error can surface. Declared in cli-core's own `optionalDependencies` so it ships transitively to consumers without them having to add it themselves. This is the first slice of a 4-PR series that splits the original keyring-backed multi-account TokenStore work into independently reviewable units: primitive → AUTH_STORE_READ_FAILED + logout tolerance → multi-account TokenStore → migration helper. Co-Authored-By: Claude Opus 4.7 (1M context) --- README.md | 55 +++++-- package-lock.json | 223 ++++++++++++++++++++++++++ package.json | 3 + src/auth/index.ts | 6 + src/auth/keyring/index.ts | 6 + src/auth/keyring/secure-store.test.ts | 80 +++++++++ src/auth/keyring/secure-store.ts | 102 ++++++++++++ 7 files changed, 461 insertions(+), 14 deletions(-) create mode 100644 src/auth/keyring/index.ts create mode 100644 src/auth/keyring/secure-store.test.ts create mode 100644 src/auth/keyring/secure-store.ts diff --git a/README.md b/README.md index 4717c5f..1ea057f 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`, `createSecureStore`, PKCE helpers, `AuthProvider` / `TokenStore` / `AccountRef` / `SecureStore` 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 thin cross-platform OS-keyring wrapper (`createSecureStore`) backed by `@napi-rs/keyring`. `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), `open` (browser launch), and `@napi-rs/keyring` (when using `createSecureStore`) are optional peer/optional deps. | +| `commands` (subpath) | `registerChangelogCommand`, `registerUpdateCommand` (+ semver helpers) | Commander wiring for cli-core's standard commands (e.g. ` changelog`, ` update`, ` update switch`). **Requires** `commander` as an optional peer-dep. | +| `config` | `getConfigPath`, `readConfig`, `readConfigStrict`, `writeConfig`, `updateConfig`, `CoreConfig`, `UpdateChannel` | Read / write a per-CLI JSON config file with typed error codes; `CoreConfig` is the shape of fields cli-core itself owns (extend it for per-CLI fields). | +| `empty` | `printEmpty` | Print an empty-state message gated on `--json` / `--ndjson` so machine consumers never see human strings on stdout. | +| `errors` | `CliError` | Typed CLI error class with `code` and exit-code mapping. | +| `global-args` | `parseGlobalArgs`, `stripUserFlag`, `createGlobalArgsStore`, `createAccessibleGate`, `createSpinnerGate`, `getProgressJsonlPath`, `isProgressJsonlEnabled` | Parse well-known global flags (`--json`, `--ndjson`, `--quiet`, `--verbose`, `--accessible`, `--no-spinner`, `--progress-jsonl`, `--user `) and derive predicates from them. `stripUserFlag` removes `--user` tokens from argv so the cleaned array can be forwarded to Commander when the flag has no root-program attachment. | +| `json` | `formatJson`, `formatNdjson` | Stable JSON / newline-delimited JSON formatting for stdout. | +| `markdown` (subpath) | `preloadMarkdown`, `renderMarkdown`, `TerminalRendererOptions` | Lazy-init terminal markdown renderer. **Requires** `marked` and `marked-terminal-renderer` as peer-deps — install only if your CLI uses this subpath. | +| `options` | `ViewOptions` | Type contract for `{ json?, ndjson? }` per-command options that machine-output gates derive from. | +| `spinner` | `createSpinner` | Loading spinner factory wrapping `yocto-spinner` with disable gates. | +| `terminal` | `isCI`, `isStderrTTY`, `isStdinTTY`, `isStdoutTTY` | TTY / CI detection helpers. | +| `testing` (subpath) | `describeEmptyMachineOutput` | Vitest helpers reusable by consuming CLIs (e.g. parametrised empty-state suite covering `--json` / `--ndjson` / human modes). | ## Usage @@ -288,6 +288,33 @@ 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. +#### Keyring primitive (`createSecureStore`) + +When the OS credential manager is the right place for your token, `createSecureStore` is a thin cross-platform wrapper around `@napi-rs/keyring`. It exposes a three-method handle (`getSecret` / `setSecret` / `deleteSecret`) for one slot identified by `serviceName` + `account`: + +```ts +import { createSecureStore, SecureStoreUnavailableError } from '@doist/cli-core/auth' + +const slot = createSecureStore({ serviceName: 'todoist-cli', account: 'api-token' }) + +try { + await slot.setSecret(token) +} catch (error) { + if (error instanceof SecureStoreUnavailableError) { + // Keyring unreachable (WSL without D-Bus, missing native binary on an + // exotic arch, Keychain locked, …). Fall back to wherever your CLI + // stores fallback state — config file with a clear plaintext warning, + // an env-var prompt, etc. + } else { + throw error + } +} +``` + +Every failure mode — `@napi-rs/keyring` failing to load on an arch without a prebuilt binary, libsecret not running on a headless Linux box, the Keychain prompting and the user denying — is normalised into `SecureStoreUnavailableError`. The `@napi-rs/keyring` module is dynamic-imported so a missing native binary doesn't crash module load before the error can surface. + +`@napi-rs/keyring` is 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 (Credential Manager), macOS (Keychain), and Linux glibc + musl (libsecret / Secret Service). + #### `--user ` and multi-user wiring The three account-touching attachers (`attachLogoutCommand` / `attachStatusCommand` / `attachTokenViewCommand`) always attach `--user ` on their subcommand. `attachLogoutCommand` threads the parsed ref to both `store.active(ref)` and `store.clear(ref)`; `attachStatusCommand` and `attachTokenViewCommand` only call `store.active(ref)`. When `--user` is supplied but `store.active(ref)` returns `null`, each attacher throws `CliError('ACCOUNT_NOT_FOUND', …)` so the user sees a typed miss rather than `NOT_AUTHENTICATED` or a silent `✓ Logged out`. Single-user stores returning `null` for a non-matching ref is the supported way to feed this guard. 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..9836697 100644 --- a/src/auth/index.ts +++ b/src/auth/index.ts @@ -35,3 +35,9 @@ export type { TokenStore, ValidateInput, } from './types.js' +export { + SECURE_STORE_DESCRIPTION, + SecureStoreUnavailableError, + createSecureStore, +} from './keyring/index.js' +export type { CreateSecureStoreOptions, SecureStore } from './keyring/index.js' diff --git a/src/auth/keyring/index.ts b/src/auth/keyring/index.ts new file mode 100644 index 0000000..11bca55 --- /dev/null +++ b/src/auth/keyring/index.ts @@ -0,0 +1,6 @@ +export { + SECURE_STORE_DESCRIPTION, + SecureStoreUnavailableError, + createSecureStore, +} from './secure-store.js' +export type { CreateSecureStoreOptions, SecureStore } from './secure-store.js' diff --git a/src/auth/keyring/secure-store.test.ts b/src/auth/keyring/secure-store.test.ts new file mode 100644 index 0000000..0fb4875 --- /dev/null +++ b/src/auth/keyring/secure-store.test.ts @@ -0,0 +1,80 @@ +import { afterEach, 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('wraps a keyring 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) + }) +}) + +describe('createSecureStore — missing native binary', () => { + beforeEach(() => { + 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') + }) + + const { createSecureStore, SecureStoreUnavailableError } = await import('./secure-store.js') + + await expect( + createSecureStore({ serviceName: SERVICE, account: ACCOUNT }).getSecret(), + ).rejects.toBeInstanceOf(SecureStoreUnavailableError) + }) +}) diff --git a/src/auth/keyring/secure-store.ts b/src/auth/keyring/secure-store.ts new file mode 100644 index 0000000..d4b660c --- /dev/null +++ b/src/auth/keyring/secure-store.ts @@ -0,0 +1,102 @@ +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 + * (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) +} From 5b0cfd76106682f7481eb96be80267c9812d9b38 Mon Sep 17 00:00:00 2001 From: Scott Lovegrove Date: Sat, 16 May 2026 13:28:12 +0100 Subject: [PATCH 2/2] fix(auth): tighten createSecureStore (review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Drop the unused `DEFAULT_ACCOUNT_FOR_USER` const — no caller in this diff reaches for it; a future change that needs a shared per-user slug helper can add it back where the consumer lands. - Drop the public `SECURE_STORE_DESCRIPTION` export. The constant is still defined for internal use within `secure-store.ts` (it composes the fallback warning string elsewhere in the module), but it is no longer reachable from `@doist/cli-core/auth` until a real public caller asks for it. - Extract a `withEntry` helper inside `createSecureStore` so the per-method try/catch/getEntry boilerplate collapses into one place, and memoise the dynamic-import + `AsyncEntry` construction in a per-store `entryPromise` so repeated reads/writes share one entry and a missing native binary fast-fails on subsequent calls. - `toUnavailableError` now uses the shared `getErrorMessage` helper from `src/errors.ts` and attaches the original throwable as `cause` on `SecureStoreUnavailableError`, so platform-specific stack/native error codes aren't lost on the way to the consumer. - Tests rework: a single hoisted `vi.mock` with a getter checks a `throwOnImport` toggle on every access — replaces `vi.doUnmock` cleanup that was leaving the real keyring exposed to any test added later. Adds back the empty-slot `getSecret` case and a parameterised failure test covering `getSecret` / `setSecret` / `deleteSecret` (each asserts the cause is preserved). Adds a memoisation test that locks the entry-construction caching. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/auth/index.ts | 6 +- src/auth/keyring/index.ts | 6 +- src/auth/keyring/secure-store.test.ts | 104 ++++++++++++++++++++------ src/auth/keyring/secure-store.ts | 100 +++++++++++-------------- 4 files changed, 127 insertions(+), 89 deletions(-) diff --git a/src/auth/index.ts b/src/auth/index.ts index 9836697..cf0f89a 100644 --- a/src/auth/index.ts +++ b/src/auth/index.ts @@ -35,9 +35,5 @@ export type { TokenStore, ValidateInput, } from './types.js' -export { - SECURE_STORE_DESCRIPTION, - SecureStoreUnavailableError, - createSecureStore, -} from './keyring/index.js' +export { SecureStoreUnavailableError, createSecureStore } from './keyring/index.js' export type { CreateSecureStoreOptions, SecureStore } from './keyring/index.js' diff --git a/src/auth/keyring/index.ts b/src/auth/keyring/index.ts index 11bca55..22ed4d7 100644 --- a/src/auth/keyring/index.ts +++ b/src/auth/keyring/index.ts @@ -1,6 +1,2 @@ -export { - SECURE_STORE_DESCRIPTION, - SecureStoreUnavailableError, - createSecureStore, -} from './secure-store.js' +export { SecureStoreUnavailableError, createSecureStore } from './secure-store.js' export type { CreateSecureStoreOptions, SecureStore } from './secure-store.js' diff --git a/src/auth/keyring/secure-store.test.ts b/src/auth/keyring/secure-store.test.ts index 0fb4875..bc3d8ca 100644 --- a/src/auth/keyring/secure-store.test.ts +++ b/src/auth/keyring/secure-store.test.ts @@ -1,4 +1,4 @@ -import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { beforeEach, describe, expect, it, vi } from 'vitest' const keyringMocks = vi.hoisted(() => { const entry = { @@ -6,16 +6,31 @@ const keyringMocks = vi.hoisted(() => { setPassword: vi.fn(), deleteCredential: vi.fn(), } + // Toggle the getter on the mocked module reads on each property access, + // so a single test can simulate a missing native binary by flipping this + // boolean without `vi.doUnmock` (which would leave the real keyring + // exposed to subsequent tests). + const state = { throwOnImport: false } return { AsyncEntry: vi.fn().mockImplementation(function AsyncEntry() { return entry }), entry, + state, } }) +// `AsyncEntry` is read via a getter so we can throw on access, which is what +// `@napi-rs/keyring` does when the prebuilt native binary is missing for the +// current arch. A plain factory would only run once per file and couldn't +// vary per test. vi.mock('@napi-rs/keyring', () => ({ - AsyncEntry: keyringMocks.AsyncEntry, + get AsyncEntry() { + if (keyringMocks.state.throwOnImport) { + throw new Error('no native binary for this arch') + } + return keyringMocks.AsyncEntry + }, })) const SERVICE = 'cli-core-test' @@ -23,8 +38,16 @@ const ACCOUNT = 'user-42' describe('createSecureStore', () => { beforeEach(() => { - vi.clearAllMocks() + // `mockReset` (not `clearAllMocks`) so the resolved/rejected values + // set by `mockResolvedValueOnce` etc. don't leak between tests and + // make this suite order-dependent. The `AsyncEntry` constructor + // mock is left untouched so `new AsyncEntry()` still returns `entry`. + keyringMocks.entry.getPassword.mockReset() + keyringMocks.entry.setPassword.mockReset() + keyringMocks.entry.deleteCredential.mockReset() + keyringMocks.AsyncEntry.mockClear() vi.resetModules() + keyringMocks.state.throwOnImport = false }) it('reads, writes, and deletes secrets via @napi-rs/keyring with the configured service/account', async () => { @@ -43,38 +66,71 @@ describe('createSecureStore', () => { expect(keyringMocks.entry.setPassword).toHaveBeenCalledWith('tok_abcdef') }) - it('wraps a keyring failure as SecureStoreUnavailableError', async () => { - keyringMocks.entry.getPassword.mockRejectedValue(new Error('Keychain locked')) + it('resolves to null when the keyring has no credential for the slot', async () => { + keyringMocks.entry.getPassword.mockResolvedValue(null) - const { createSecureStore, SecureStoreUnavailableError } = await import('./secure-store.js') + const { createSecureStore } = await import('./secure-store.js') await expect( createSecureStore({ serviceName: SERVICE, account: ACCOUNT }).getSecret(), - ).rejects.toBeInstanceOf(SecureStoreUnavailableError) + ).resolves.toBeNull() }) -}) -describe('createSecureStore — missing native binary', () => { - beforeEach(() => { - vi.resetModules() - }) + it.each([ + ['getSecret', () => keyringMocks.entry.getPassword] as const, + ['setSecret', () => keyringMocks.entry.setPassword] as const, + ['deleteSecret', () => keyringMocks.entry.deleteCredential] as const, + ])( + 'wraps a %s keyring failure as SecureStoreUnavailableError and preserves cause', + async (method, pickSpy) => { + const cause = new Error(`backend down (${method})`) + pickSpy().mockRejectedValueOnce(cause) + + const { createSecureStore, SecureStoreUnavailableError } = + await import('./secure-store.js') + const store = createSecureStore({ serviceName: SERVICE, account: ACCOUNT }) + + const invoke = async () => { + if (method === 'getSecret') return store.getSecret() + if (method === 'setSecret') return store.setSecret('x') + return store.deleteSecret() + } + const error = await invoke().then( + () => undefined, + (rejection: unknown) => rejection, + ) + expect(error).toBeInstanceOf(SecureStoreUnavailableError) + expect((error as Error).message).toContain(`backend down (${method})`) + expect((error as { cause?: unknown }).cause).toBe(cause) + }, + ) + + it('memoises the AsyncEntry across calls on the same store', async () => { + keyringMocks.entry.getPassword.mockResolvedValue('tok') + + const { createSecureStore } = await import('./secure-store.js') + const store = createSecureStore({ serviceName: SERVICE, account: ACCOUNT }) - // 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') + await store.getSecret() + await store.getSecret() + await store.deleteSecret() + + expect(keyringMocks.AsyncEntry).toHaveBeenCalledTimes(1) }) - 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') - }) + it('surfaces an import failure as SecureStoreUnavailableError and memoises the rejection (no retry on later calls)', async () => { + keyringMocks.state.throwOnImport = true const { createSecureStore, SecureStoreUnavailableError } = await import('./secure-store.js') + const store = createSecureStore({ serviceName: SERVICE, account: ACCOUNT }) - await expect( - createSecureStore({ serviceName: SERVICE, account: ACCOUNT }).getSecret(), - ).rejects.toBeInstanceOf(SecureStoreUnavailableError) + await expect(store.getSecret()).rejects.toBeInstanceOf(SecureStoreUnavailableError) + + // Flip the toggle back on so a re-import would now succeed — the + // store's memoised `entryPromise` should still replay the original + // rejection and the import factory should not be re-evaluated. + keyringMocks.state.throwOnImport = false + await expect(store.getSecret()).rejects.toBeInstanceOf(SecureStoreUnavailableError) + expect(keyringMocks.AsyncEntry).not.toHaveBeenCalled() }) }) diff --git a/src/auth/keyring/secure-store.ts b/src/auth/keyring/secure-store.ts index d4b660c..cd0e745 100644 --- a/src/auth/keyring/secure-store.ts +++ b/src/auth/keyring/secure-store.ts @@ -1,11 +1,4 @@ -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}` +import { getErrorMessage } from '../../errors.js' /** * Thrown when the OS credential manager cannot be reached — missing native @@ -13,13 +6,17 @@ export const DEFAULT_ACCOUNT_FOR_USER = (id: string): string => `user-${id}` * (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. + * The original throwable is preserved on `cause` so platform-specific + * diagnostics (stack, native error code) aren't lost — callers that want + * to degrade to a plaintext fallback should catch this specific class + * rather than swallowing every `Error`. */ export class SecureStoreUnavailableError extends Error { - constructor(message = 'System credential storage is unavailable') { - super(message) + constructor( + message = 'System credential storage is unavailable', + options?: { cause?: unknown }, + ) { + super(message, options) this.name = 'SecureStoreUnavailableError' } } @@ -37,66 +34,59 @@ export type CreateSecureStoreOptions = { account: string } +type AsyncEntry = import('@napi-rs/keyring').AsyncEntry + /** * Thin wrapper around `@napi-rs/keyring` that normalizes every failure mode * into `SecureStoreUnavailableError`. `serviceName` + `account` together * identify one credential slot in the OS keyring. + * + * The dynamic import + `AsyncEntry` construction is memoised per-store so + * repeated reads/writes share one entry and a missing native binary + * fast-fails on subsequent calls instead of retrying the import every time + * (the rejected promise replays its rejection on each `await`). */ export function createSecureStore(options: CreateSecureStoreOptions): SecureStore { const { serviceName, account } = options + let entryPromise: Promise | undefined + + async function withEntry(fn: (entry: AsyncEntry) => Promise): Promise { + if (!entryPromise) { + // Dynamic import: `@napi-rs/keyring` is an optional dependency. + // On unsupported architectures the native binary is absent and + // a static import would crash module load before we can surface + // `SecureStoreUnavailableError`. + entryPromise = (async () => { + const { AsyncEntry } = await import('@napi-rs/keyring') + return new AsyncEntry(serviceName, account) + })() + } + try { + const entry = await entryPromise + return await fn(entry) + } catch (error) { + throw toUnavailableError(error) + } + } return { - async getSecret(): Promise { - const entry = await getEntry(serviceName, account) - try { - return (await entry.getPassword()) ?? null - } catch (error) { - throw toUnavailableError(error) - } + async getSecret() { + return withEntry(async (entry) => (await entry.getPassword()) ?? null) }, - - async setSecret(secret: string): Promise { - const entry = await getEntry(serviceName, account) - try { + async setSecret(secret) { + return withEntry(async (entry) => { 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 deleteSecret() { + return withEntry((entry) => entry.deleteCredential()) }, } } -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) + return new SecureStoreUnavailableError(getErrorMessage(error), { cause: error }) }