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
56 changes: 40 additions & 16 deletions README.md

Large diffs are not rendered by default.

1 change: 1 addition & 0 deletions src/auth/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,7 @@ export type {
SecureStore,
TokenStorageLocation,
TokenStorageResult,
CredentialStore,
UserRecord,
UserRecordStore,
} from './keyring/index.js'
1 change: 1 addition & 0 deletions src/auth/keyring/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ export type { MigrateAuthResult, MigrateLegacyAuthOptions, MigrateSkipReason } f
export type {
TokenStorageLocation,
TokenStorageResult,
CredentialStore,
UserRecord,
UserRecordStore,
} from './types.js'
55 changes: 55 additions & 0 deletions src/auth/keyring/record-write.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,42 @@ describe('writeRecordWithKeyringFallback', () => {
expect(state.records.get('42')?.fallbackToken).toBe('tok_plain')
})

it('does not write a fallbackToken in strict system mode', async () => {
const secureStore = buildSingleSlot()
secureStore.setSpy.mockRejectedValueOnce(new SecureStoreUnavailableError('no dbus'))
const { store: userRecords, state } = buildUserRecords<Account>()

await expect(
writeRecordWithKeyringFallback({
secureStore,
userRecords,
account,
token: 'tok_plain',
credentialStore: 'system',
}),
).rejects.toMatchObject({ code: 'AUTH_STORE_WRITE_FAILED' })

expect(state.records.size).toBe(0)
})

it('writes directly to fallbackToken in explicit plaintext mode', async () => {
const secureStore = buildSingleSlot()
const { store: userRecords, state } = buildUserRecords<Account>()

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

expect(result.storedSecurely).toBe(false)
expect(secureStore.setSpy).not.toHaveBeenCalled()
expect(secureStore.deleteSpy).not.toHaveBeenCalled()
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')
Expand Down Expand Up @@ -189,6 +225,25 @@ describe('writeBundleWithKeyringFallback', () => {
expect(state.records.size).toBe(0)
})

it('rolls back the access slot when strict system storage cannot write the refresh token', async () => {
const { accessStore, refreshStore, store: userRecords, state } = harness()
refreshStore.setSpy.mockRejectedValueOnce(new SecureStoreUnavailableError('no dbus'))

await expect(
writeBundleWithKeyringFallback({
accessStore,
refreshStore,
userRecords,
account,
bundle,
credentialStore: 'system',
}),
).rejects.toMatchObject({ code: 'AUTH_STORE_WRITE_FAILED' })

expect(accessStore.deleteSpy).toHaveBeenCalledTimes(1)
expect(state.records.size).toBe(0)
})

it('rolls back BOTH keyring slots when upsert fails after both writes succeeded', async () => {
const { accessStore, refreshStore, store: userRecords, upsertSpy } = harness()
upsertSpy.mockRejectedValueOnce(new Error('disk full'))
Expand Down
78 changes: 53 additions & 25 deletions src/auth/keyring/record-write.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { CliError } from '../../errors.js'
import type { AuthAccount, TokenBundle } from '../types.js'
import { type SecureStore, SecureStoreUnavailableError } from './secure-store.js'
import type { UserRecord, UserRecordStore } from './types.js'
import type { CredentialStore, UserRecord, UserRecordStore } from './types.js'

type WriteRecordOptions<TAccount extends AuthAccount> = {
/** Per-account keyring slot, already configured by the caller (e.g. via `createSecureStore`). */
Expand All @@ -16,10 +16,11 @@ type WriteRecordOptions<TAccount extends AuthAccount> = {
userRecords: UserRecordStore<TAccount>
account: TAccount
token: string
credentialStore?: CredentialStore
}

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. */
/** `true` when the secret landed in the OS keyring; `false` when it was written to `fallbackToken` on the user record. */
storedSecurely: boolean
}

Expand All @@ -31,22 +32,23 @@ type WriteBundleOptions<TAccount extends AuthAccount> = {
userRecords: UserRecordStore<TAccount>
account: TAccount
bundle: TokenBundle
credentialStore?: CredentialStore
}

type WriteBundleResult = {
/** `true` when the access token landed in the OS keyring; `false` when it fell back to `fallbackToken`. */
/** `true` when the access token landed in the OS keyring; `false` when it was written to `fallbackToken`. */
accessStoredSecurely: boolean
/**
* `true` when a refresh token landed in the OS keyring. `false` when it
* fell back to `fallbackRefreshToken`. `undefined` when the bundle
* was written to `fallbackRefreshToken`. `undefined` when the bundle
* carried no refresh token (nothing to store).
*/
refreshStoredSecurely: boolean | undefined
}

/**
* Single-token write. Thin wrapper over `writeBundleWithKeyringFallback`
* passing a refresh-less bundle, so trim/validate, access-slot fallback,
* passing a refresh-less bundle, so trim/validate, access-slot storage,
* upsert rollback, and the deferred refresh-slot wipe all share one
* implementation.
*
Expand All @@ -57,7 +59,7 @@ type WriteBundleResult = {
export async function writeRecordWithKeyringFallback<TAccount extends AuthAccount>(
options: WriteRecordOptions<TAccount>,
): Promise<WriteRecordResult> {
const { secureStore, refreshStore, userRecords, account, token } = options
const { secureStore, refreshStore, userRecords, account, token, credentialStore } = options

const { accessStoredSecurely } = await writeBundleWithKeyringFallback({
accessStore: secureStore,
Expand All @@ -68,6 +70,7 @@ export async function writeRecordWithKeyringFallback<TAccount extends AuthAccoun
userRecords,
account,
bundle: { accessToken: token },
credentialStore,
})

return { storedSecurely: accessStoredSecurely }
Expand All @@ -78,19 +81,22 @@ export async function writeRecordWithKeyringFallback<TAccount extends AuthAccoun
* refresh wipe.
*
* 1. Validate `bundle.accessToken` (non-empty after trim).
* 2. `accessStore.setSecret`. `SecureStoreUnavailableError` degrades to
* `fallbackToken` on the record; any other error rethrows.
* 3. `refreshStore.setSecret` when `bundle.refreshToken` is present.
* `SecureStoreUnavailableError` degrades to `fallbackRefreshToken`. A
* non-keyring failure rolls back the access slot before rethrowing
* (no partial credentials left behind for an unregistered user).
* 2. Under `'system'` or `'fallback'`, `accessStore.setSecret` runs.
* `'fallback'` degrades a `SecureStoreUnavailableError` to
* `fallbackToken`; `'system'` rejects it. `'plaintext'` writes the
* fallback field directly without calling the keyring.
* 3. Under `'system'` or `'fallback'`, `refreshStore.setSecret` runs when
* `bundle.refreshToken` is present. Under `'fallback'`,
* `SecureStoreUnavailableError` degrades to `fallbackRefreshToken`; under
* `'system'`, it rolls back a successful access-slot write before
* rejecting. A non-keyring failure has the same rollback behavior.
* 4. `userRecords.upsert(record)`. On failure, best-effort
* `Promise.allSettled` rollback of any slot writes that succeeded.
* 5. Only after a successful upsert: if the bundle has no refresh token,
* wipe any orphan slot from a prior `setBundle` (best-effort). Doing
* this BEFORE the upsert would lose refresh state if the upsert then
* rejected — the new record's `hasRefreshToken` would still claim
* false but the old slot would be gone with no rollback path.
* 5. Only after a successful non-plaintext upsert: if the bundle has no
* refresh token, wipe any orphan slot from a prior `setBundle`
* (best-effort). Doing this BEFORE the upsert would lose refresh state if
* the upsert then rejected — the new record's `hasRefreshToken` would
* still claim false but the old slot would be gone with no rollback path.
*
* Default promotion is external — preference, not correctness, and an
* error there must not dirty up a successful credential write.
Expand All @@ -99,6 +105,7 @@ export async function writeBundleWithKeyringFallback<TAccount extends AuthAccoun
options: WriteBundleOptions<TAccount>,
): Promise<WriteBundleResult> {
const { accessStore, refreshStore, userRecords, account, bundle } = options
const credentialStore = options.credentialStore ?? 'fallback'
const accessToken = bundle.accessToken.trim()
if (!accessToken) {
throw new CliError(
Expand All @@ -109,20 +116,23 @@ export async function writeBundleWithKeyringFallback<TAccount extends AuthAccoun
const refreshToken = bundle.refreshToken?.trim()

let accessStoredSecurely = false
try {
await accessStore.setSecret(accessToken)
accessStoredSecurely = true
} catch (error) {
if (!(error instanceof SecureStoreUnavailableError)) throw error
if (credentialStore !== 'plaintext') {
try {
await accessStore.setSecret(accessToken)
accessStoredSecurely = true
} catch (error) {
if (!(error instanceof SecureStoreUnavailableError)) throw error
if (credentialStore === 'system') throw credentialStoreUnavailableError()
}
}

let refreshStoredSecurely: boolean | undefined
if (refreshToken) {
if (refreshToken && credentialStore !== 'plaintext') {
try {
await refreshStore.setSecret(refreshToken)
refreshStoredSecurely = true
} catch (error) {
if (error instanceof SecureStoreUnavailableError) {
if (error instanceof SecureStoreUnavailableError && credentialStore === 'fallback') {
refreshStoredSecurely = false
} else {
if (accessStoredSecurely) {
Expand All @@ -132,9 +142,14 @@ export async function writeBundleWithKeyringFallback<TAccount extends AuthAccoun
// best-effort
}
}
if (error instanceof SecureStoreUnavailableError) {
throw credentialStoreUnavailableError()
}
throw error
}
}
} else if (refreshToken) {
refreshStoredSecurely = false
}

const record: UserRecord<TAccount> = {
Expand Down Expand Up @@ -168,7 +183,7 @@ export async function writeBundleWithKeyringFallback<TAccount extends AuthAccoun
// that the new record (with `hasRefreshToken: false`) is durable. If
// this fails the gate already prevents readers from consulting it; the
// worst case is a stale keyring entry that `clear()` will pick up.
if (!refreshToken) {
if (!refreshToken && credentialStore !== 'plaintext') {
try {
await refreshStore.deleteSecret()
} catch {
Expand All @@ -179,6 +194,19 @@ export async function writeBundleWithKeyringFallback<TAccount extends AuthAccoun
return { accessStoredSecurely, refreshStoredSecurely }
}

function credentialStoreUnavailableError(): CliError {
return new CliError(
'AUTH_STORE_WRITE_FAILED',
'The system credential manager could not store the credential.',
{
hints: [
'Make the system credential manager available and retry.',
'Configure plaintext credential storage explicitly to store the credential in the config file.',
],
},
)
}

/**
* Build a `UserRecord` for an access-only credential (no refresh state).
* Used by `migrateLegacyAuth`'s Phase 1 / Phase 2 record writes; both call
Expand Down
53 changes: 53 additions & 0 deletions src/auth/keyring/token-store.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,59 @@ describe('createKeyringTokenStore', () => {
expect(keyring.getSpy).not.toHaveBeenCalled()
})

it('rejects an unavailable keyring in strict system mode without writing plaintext', async () => {
const keyring = buildSingleSlot()
keyring.setSpy.mockRejectedValueOnce(new SecureStoreUnavailableError('no dbus'))
const { store, state } = fixture({
keyring,
factoryOpts: { credentialStore: 'system' },
})

await expect(store.set(account, 'tok_secret')).rejects.toMatchObject({
code: 'AUTH_STORE_WRITE_FAILED',
message: 'The system credential manager could not store the credential.',
})

expect(state.records.size).toBe(0)
expect(store.getLastStorageResult()).toBeUndefined()
})

it('uses an explicit plaintext resolver without calling the keyring', async () => {
const keyring = buildSingleSlot()
let credentialStore: 'system' | 'plaintext' = 'system'
const { store, state } = fixture({
keyring,
factoryOpts: { credentialStore: () => credentialStore },
})
credentialStore = 'plaintext'

await store.setBundle(
account,
{
accessToken: 'tok_a',
refreshToken: 'tok_r',
accessTokenExpiresAt: 1_700_000_000_000,
refreshTokenExpiresAt: 1_701_000_000_000,
},
{ promoteDefault: true },
)

expect(keyring.setSpy).not.toHaveBeenCalled()
expect(keyring.deleteSpy).not.toHaveBeenCalled()
expect(state.records.get('42')).toEqual({
account,
fallbackToken: 'tok_a',
fallbackRefreshToken: 'tok_r',
accessTokenExpiresAt: 1_700_000_000_000,
refreshTokenExpiresAt: 1_701_000_000_000,
hasRefreshToken: true,
})
expect(store.getLastStorageResult()).toEqual({
storage: 'config-file',
warning: 'credential stored as plaintext in /tmp/fake/config.json',
})
})

it('set() still succeeds when the best-effort default promotion fails', async () => {
const { store, state, setDefaultSpy } = fixture()
setDefaultSpy.mockRejectedValueOnce(new Error('default-write blew up'))
Expand Down
Loading
Loading