Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
75 changes: 61 additions & 14 deletions README.md

Large diffs are not rendered by default.

17 changes: 15 additions & 2 deletions src/auth/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -35,5 +35,18 @@ export type {
TokenStore,
ValidateInput,
} from './types.js'
export { SecureStoreUnavailableError, createSecureStore } from './keyring/index.js'
export type { CreateSecureStoreOptions, SecureStore } from './keyring/index.js'
export {
SecureStoreUnavailableError,
createKeyringTokenStore,
createSecureStore,
} from './keyring/index.js'
export type {
CreateKeyringTokenStoreOptions,
CreateSecureStoreOptions,
KeyringTokenStore,
SecureStore,
TokenStorageLocation,
TokenStorageResult,
UserRecord,
UserRecordStore,
} from './keyring/index.js'
10 changes: 10 additions & 0 deletions src/auth/keyring/index.ts
Original file line number Diff line number Diff line change
@@ -1,2 +1,12 @@
export { 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 type {
TokenStorageLocation,
TokenStorageResult,
UserRecord,
UserRecordStore,
} from './types.js'
97 changes: 97 additions & 0 deletions src/auth/keyring/record-write.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,97 @@
import { describe, expect, it } from 'vitest'

import { buildSingleSlot, buildUserRecords } from '../../test-support/keyring-mocks.js'
import { writeRecordWithKeyringFallback } from './record-write.js'
import { SecureStoreUnavailableError } from './secure-store.js'

type Account = { id: string; label?: string; email: string }

const account: Account = { id: '42', label: 'me', email: 'a@b.c' }

describe('writeRecordWithKeyringFallback', () => {
it('writes to the keyring slot and upserts a record with no fallbackToken on the happy path', async () => {
const secureStore = buildSingleSlot()
const { store: userRecords, state, upsertSpy } = buildUserRecords<Account>()

const result = await writeRecordWithKeyringFallback({
secureStore,
userRecords,
account,
token: ' tok_secret ',
})

expect(result.storedSecurely).toBe(true)
expect(secureStore.setSpy).toHaveBeenCalledWith('tok_secret')
expect(upsertSpy).toHaveBeenCalledWith({ account })
expect(state.records.get('42')?.fallbackToken).toBeUndefined()
})

it('falls back to fallbackToken on the user record when the keyring is offline', async () => {
const secureStore = buildSingleSlot()
secureStore.setSpy.mockRejectedValueOnce(new SecureStoreUnavailableError('no dbus'))
const { store: userRecords, state } = buildUserRecords<Account>()

const result = await writeRecordWithKeyringFallback({
secureStore,
userRecords,
account,
token: 'tok_plain',
})

expect(result.storedSecurely).toBe(false)
expect(state.records.get('42')?.fallbackToken).toBe('tok_plain')
})

it('rethrows non-keyring errors from setSecret without writing the record', async () => {
const secureStore = buildSingleSlot()
const cause = new Error('unexpected backend explosion')
secureStore.setSpy.mockRejectedValueOnce(cause)
const { store: userRecords, state } = buildUserRecords<Account>()

await expect(
writeRecordWithKeyringFallback({
secureStore,
userRecords,
account,
token: 'tok',
}),
).rejects.toBe(cause)
expect(state.records.size).toBe(0)
})

it('rolls back the keyring write when upsert fails (no orphan credential)', async () => {
const secureStore = buildSingleSlot()
const { store: userRecords, upsertSpy } = buildUserRecords<Account>()
upsertSpy.mockRejectedValueOnce(new Error('disk full'))

await expect(
writeRecordWithKeyringFallback({
secureStore,
userRecords,
account,
token: 'tok',
}),
).rejects.toThrow('disk full')
expect(secureStore.deleteSpy).toHaveBeenCalledTimes(1)
})

it('does not rollback the keyring on upsert failure when the write went to fallbackToken', async () => {
// No successful keyring write happened, so there is nothing to roll
// back. Verify the helper doesn't accidentally call deleteSecret
// in this branch.
const secureStore = buildSingleSlot()
secureStore.setSpy.mockRejectedValueOnce(new SecureStoreUnavailableError('no dbus'))
const { store: userRecords, upsertSpy } = buildUserRecords<Account>()
upsertSpy.mockRejectedValueOnce(new Error('disk full'))

await expect(
writeRecordWithKeyringFallback({
secureStore,
userRecords,
account,
token: 'tok',
}),
).rejects.toThrow('disk full')
expect(secureStore.deleteSpy).not.toHaveBeenCalled()
})
})
67 changes: 67 additions & 0 deletions src/auth/keyring/record-write.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
import type { AuthAccount } from '../types.js'
import { type SecureStore, SecureStoreUnavailableError } from './secure-store.js'
import type { UserRecord, UserRecordStore } from './types.js'

type WriteRecordOptions<TAccount extends AuthAccount> = {
Comment thread
scottlovegrove marked this conversation as resolved.
/** Per-account keyring slot, already configured by the caller (e.g. via `createSecureStore`). */
secureStore: SecureStore
userRecords: UserRecordStore<TAccount>
account: TAccount
token: string
}

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<TAccount extends AuthAccount>(
Comment thread
scottlovegrove marked this conversation as resolved.
options: WriteRecordOptions<TAccount>,
): Promise<WriteRecordResult> {
const { secureStore, userRecords, account, token } = options
const trimmed = token.trim()

let storedSecurely = false
try {
await secureStore.setSecret(trimmed)
storedSecurely = true
} catch (error) {
if (!(error instanceof SecureStoreUnavailableError)) throw error
}

const record: UserRecord<TAccount> = storedSecurely
? { account }
: { 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 }
}
15 changes: 15 additions & 0 deletions src/auth/keyring/secure-store.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,20 @@
import { getErrorMessage } from '../../errors.js'

/**
* User-facing label for the OS credential manager. Used by the keyring
* `TokenStore`'s fallback-warning composition; internal to the keyring
* module until a public caller asks for it.
*/
export const SECURE_STORE_DESCRIPTION = 'system credential manager'

/**
* Default keyring `account` slug for a stored user. Lives here so every
* caller that derives a per-user slot name agrees on the wire format — 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
Expand Down
Loading
Loading