From 4749cd7bb98d65c7b8c3d2ebf649d539bb4696bb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Richard=20Sol=C3=A1r?= Date: Thu, 24 Sep 2026 14:23:53 +0200 Subject: [PATCH 1/5] feat: key secrets by user ID on both backends Secrets lived under one fixed name per kind, so a second account would overwrite the first one's token. Both the keyring and the file backend are now keyed by user ID, and existing secrets are re-keyed in place. Co-Authored-By: Claude Opus 5 --- src/commands/auth/logout.ts | 8 +- src/lib/auth-file.ts | 89 +++++++- src/lib/auth.ts | 28 ++- src/lib/credentials.ts | 261 ++++++++++++++------- src/lib/utils.ts | 36 ++- test/__setup__/auth-file.ts | 24 ++ test/__setup__/keyring-mock.ts | 12 +- test/local/commands/auth.test.ts | 58 +++-- test/local/lib/auth-file.test.ts | 41 ++-- test/local/lib/auth.test.ts | 10 +- test/local/lib/credentials.test.ts | 353 ++++++++++++++++++++++------- 11 files changed, 673 insertions(+), 247 deletions(-) diff --git a/src/commands/auth/logout.ts b/src/commands/auth/logout.ts index 68f3b4768..9f84a2092 100644 --- a/src/commands/auth/logout.ts +++ b/src/commands/auth/logout.ts @@ -1,6 +1,6 @@ import { APIFY_ENV_VARS } from '@apify/consts'; -import { removeActiveProfile } from '../../lib/auth-file.js'; +import { getActiveProfileId, removeActiveProfile } from '../../lib/auth-file.js'; import { invalidEnvTokenMessage, readEnvToken } from '../../lib/auth.js'; import { ApifyCommand } from '../../lib/command-framework/apify-command.js'; import { AUTH_FILE_PATH } from '../../lib/consts.js'; @@ -28,10 +28,10 @@ export class AuthLogoutCommand extends ApifyCommand { static override docsUrl = 'https://docs.apify.com/cli/docs/reference#apify-logout'; async run() { - // The file goes first: it is the step that can refuse, and refusing before the keyring is - // cleared leaves a logged-in state rather than half a logout. + // The keyring goes first: `auth.json` is the only index of what it holds, so removing the + // profile would strand its entries. + await clearKeyringSecrets(getActiveProfileId()); removeActiveProfile(); - await clearKeyringSecrets(); await updateUserId(null); diff --git a/src/lib/auth-file.ts b/src/lib/auth-file.ts index 9e7149c2d..6d2d66142 100644 --- a/src/lib/auth-file.ts +++ b/src/lib/auth-file.ts @@ -3,7 +3,7 @@ import { existsSync, readFileSync, renameSync, rmSync, writeFileSync } from 'nod import { cryptoRandomObjectId } from '@apify/utilities'; import { AUTH_FILE_PATH } from './consts.js'; -import type { CredentialsBackend } from './credentials.js'; +import type { CredentialsBackend, SecretKind } from './credentials.js'; import { ensureApifyDirectory } from './files.js'; import { warning } from './outputs.js'; import { cliDebugPrint } from './utils/cliDebugPrint.js'; @@ -26,14 +26,21 @@ export interface AuthProfile { authMethod: 'token'; expiresAt: string | null; hasRefreshToken: boolean; - /** Reserved: a keyring failure on one profile must not redirect another profile's reads. */ + /** + * Where this profile's secrets live. Unused while the file holds one account, so the file-level + * `secretsBackend` is still the answer for every profile. Reserved for Stage-2, where a keyring + * failure on one profile must not silently redirect another profile's reads. + */ secretsBackend?: CredentialsBackend; loggedInAt: string | null; + /** File backend only. The keyring backend keeps these in the OS store instead. */ + token?: string; + proxy?: { password?: string }; } /** - * `auth.json` as this CLI writes it. `token` and `proxy` are the file backend's secret storage; - * they stay outside the profiles until each profile gets its own keys. + * `auth.json` as this CLI writes it. Top-level `token` and `proxy` are where the file backend kept + * secrets before they were keyed per profile; `ensureSecretsKeyed()` moves them into the profile. */ export interface AuthFile { version?: number; @@ -127,8 +134,8 @@ function v1Profile(file: LegacyAuthFile): AuthProfile { function toV2(file: LegacyAuthFile): AuthFile { const migrated: AuthFile = { version: AUTH_FILE_VERSION, profiles: {} }; - // A v1 file with a token but no ID has no key to store the profile under. Keep the secrets so - // the next command reports stale credentials instead of a silent logged-out state. + // A v1 file with a token but no ID has no key to store the profile under. The secrets are + // carried over here and dropped by `ensureSecretsKeyed()`, which is what forces the re-login. if (typeof file.id === 'string') { migrated.activeProfile = file.id; migrated.profiles![file.id] = v1Profile(file); @@ -254,10 +261,72 @@ export function getActiveProfile(): (AuthProfile & { id: string }) | undefined { } /** - * Replaces the file with this one account. A second profile would name an account that cannot - * authenticate until each has its own secret, and dropping the old secrets is what keeps the write - * safe: the caller writes the new token next, so a failure there leaves nobody logged in rather - * than the old token beside the new name. Additive login is #1386. + * The user ID every secret is keyed by. Taken from `activeProfile` rather than from the profile + * object, so a file whose `activeProfile` names a missing profile still resolves its secrets and + * reports the dangling profile instead of looking logged out. + */ +export function getActiveProfileId(): string | undefined { + const file = readAuthFile(); + + if (file.version !== AUTH_FILE_VERSION) { + const legacy = file as LegacyAuthFile; + return typeof legacy.id === 'string' ? legacy.id : undefined; + } + + return file.activeProfile; +} + +/** The file backend's stored secret, or `undefined` when the profile does not hold one. */ +export function readProfileSecret(userId: string, kind: SecretKind): string | undefined { + const profile = readAuthFile().profiles?.[userId]; + if (!profile) return undefined; + + return kind === 'token' ? profile.token : profile.proxy?.password; +} + +/** + * Stores a file-backend secret on the profile. A missing profile is left alone: inventing one + * would fabricate the account metadata the CLI reads. + */ +export function writeProfileSecret(userId: string, kind: SecretKind, value: string) { + updateProfile(userId, (profile) => { + if (kind === 'token') { + profile.token = value; + } else { + profile.proxy = { ...profile.proxy, password: value }; + } + }); +} + +/** Forgets one of a profile's file-backend secrets. */ +export function deleteProfileSecret(userId: string, kind: SecretKind) { + if (readProfileSecret(userId, kind) === undefined) return; + + updateProfile(userId, (profile) => { + if (kind === 'token') { + delete profile.token; + } else { + // The profile's proxy object carries nothing but the password. + delete profile.proxy; + } + }); +} + +function updateProfile(userId: string, edit: (profile: AuthProfile) => void) { + const file = readAuthFile(); + const profile = file.profiles?.[userId]; + if (!profile) return; + + edit(profile); + file.secretsBackend = 'file'; + writeAuthFile(file); +} + +/** + * Replaces the file with this one account, dropping any previous profile and its secrets. Nothing + * puts a second profile there yet; additive login is #1386. Dropping the old secrets is what keeps + * the write safe: the caller writes the new token next, so a failure there leaves nobody logged in + * rather than the old token beside the new name. */ export function replaceStoredAccount(userId: string, profile: AuthProfile, secretsBackend: CredentialsBackend) { assertSupportedAuthFileVersion(); diff --git a/src/lib/auth.ts b/src/lib/auth.ts index 636dfb1cd..e7a5b7704 100644 --- a/src/lib/auth.ts +++ b/src/lib/auth.ts @@ -6,15 +6,16 @@ import { AxiosHeaders } from 'axios'; import { APIFY_ENV_VARS } from '@apify/consts'; -import { ensureAuthFileCurrent, replaceStoredAccount } from './auth-file.js'; +import { ensureAuthFileCurrent, getActiveProfileId, replaceStoredAccount } from './auth-file.js'; import { APIFY_CLIENT_DEFAULT_HEADERS, AUTH_FILE_PATH, CommandExitCodes } from './consts.js'; import { - deleteProxyPassword, + clearKeyringSecrets, + deleteSecret, ensureMigrated, + ensureSecretsKeyed, getBackend, - getToken, - setProxyPassword, - setToken, + getSecret, + setSecret, } from './credentials.js'; import { warning } from './outputs.js'; import type { AuthJSON } from './types.js'; @@ -98,8 +99,10 @@ export const resolveAuth = async (): Promise => { // Only now, because the stored file is not this command's credential when APIFY_TOKEN is // set. A file a newer CLI wrote would otherwise stop a platform run that never reads it. await ensureAuthFileCurrent(); + await ensureSecretsKeyed(); - const storedToken = await getToken(); + const userId = getActiveProfileId(); + const storedToken = userId ? await getSecret(userId, 'token') : undefined; return storedToken ? ({ token: storedToken, source: 'stored' } as const) : undefined; })(); @@ -177,6 +180,13 @@ export async function loginWithToken( const proxyPassword = userInfo.proxy?.password; + // `auth.json` is the only index of what the keyring holds, so the outgoing account's entries + // have to go before its ID leaves the file. + const previousUserId = getActiveProfileId(); + if (previousUserId && previousUserId !== userInfo.id) { + await clearKeyringSecrets(previousUserId); + } + const { organizationOwnerUserId } = userInfo as { organizationOwnerUserId?: string }; replaceStoredAccount( userInfo.id, @@ -193,12 +203,12 @@ export async function loginWithToken( ); // After the account, which drops the previous secrets. `skipIfUnchanged` avoids a Keychain prompt. - await setToken(token, { skipIfUnchanged: true }); + await setSecret(userInfo.id, 'token', token, { skipIfUnchanged: true }); if (proxyPassword) { - await setProxyPassword(proxyPassword, { skipIfUnchanged: true }); + await setSecret(userInfo.id, 'proxy-password', proxyPassword, { skipIfUnchanged: true }); } else { - await deleteProxyPassword(); + await deleteSecret(userInfo.id, 'proxy-password'); } return { client: apifyClient, userInfo }; diff --git a/src/lib/credentials.ts b/src/lib/credentials.ts index 849ed51b5..633fe7127 100644 --- a/src/lib/credentials.ts +++ b/src/lib/credentials.ts @@ -1,15 +1,48 @@ import process from 'node:process'; -import { AUTH_FILE_VERSION, readAuthFile, writeAuthFile } from './auth-file.js'; +import type { AuthFile } from './auth-file.js'; +import { + AUTH_FILE_VERSION, + deleteProfileSecret, + readAuthFile, + readProfileSecret, + writeAuthFile, + writeProfileSecret, +} from './auth-file.js'; import { useCLIMetadata } from './hooks/useCLIMetadata.js'; import { cliDebugPrint } from './utils/cliDebugPrint.js'; +/** + * Base service name. The per-kind services hang off it; the bare name is where secrets sat before + * they were keyed by user. + */ const KEYRING_SERVICE = 'com.apify.cli'; -const TOKEN_ACCOUNT = 'token'; -const PROXY_PASSWORD_ACCOUNT = 'proxy-password'; export type CredentialsBackend = 'keyring' | 'file'; +export type SecretKind = 'token' | 'proxy-password'; + +const SECRET_KINDS: readonly SecretKind[] = ['token', 'proxy-password']; + +interface KeyringKey { + service: string; + account: string; +} + +/** + * One keyring service per kind, with the user ID as the account. A composite account name + * (`token:` under a single service) would depend on `:` being legal in an account name on + * macOS Keychain, libsecret and Windows Credential Manager, and it reads worse in keyring UIs. + */ +function keyringKey(userId: string, kind: SecretKind): KeyringKey { + return { service: `${KEYRING_SERVICE}.${kind}`, account: userId }; +} + +/** Where a secret sat before it was keyed by user: one service, the kind as the account. */ +function legacyKeyringKey(kind: SecretKind): KeyringKey { + return { service: KEYRING_SERVICE, account: kind }; +} + interface KeyringEntry { getPassword(): string | null; setPassword(password: string): void; @@ -23,12 +56,14 @@ interface KeyringModule { let cachedKeyringModule: KeyringModule | null | undefined; let backendPromise: Promise | undefined; let migrationPromise: Promise | undefined; +let keyingPromise: Promise | undefined; /** Test-only: clear cached module/backend/migration so each test starts fresh. */ export function __resetCredentialsForTests() { cachedKeyringModule = undefined; backendPromise = undefined; migrationPromise = undefined; + keyingPromise = undefined; } async function loadKeyringModule(): Promise { @@ -106,90 +141,64 @@ export function stripProxyPassword(data: { proxy?: { password?: string } }) { if (Object.keys(data.proxy).length === 0) delete data.proxy; } -async function getKeyringEntry(account: string): Promise { +async function getKeyringEntry({ service, account }: KeyringKey): Promise { const mod = await loadKeyringModule(); if (!mod) return null; - return new mod.Entry(KEYRING_SERVICE, account); + return new mod.Entry(service, account); } -async function readKeyring(account: string): Promise { +async function readKeyring(key: KeyringKey): Promise { try { - const entry = await getKeyringEntry(account); + const entry = await getKeyringEntry(key); if (!entry) return undefined; return entry.getPassword() ?? undefined; } catch (err) { - cliDebugPrint('credentials', `failed to read ${account} from keyring`, err); + cliDebugPrint('credentials', `failed to read ${key.service}/${key.account} from keyring`, err); return undefined; } } -async function writeKeyring(account: string, value: string): Promise { - const entry = await getKeyringEntry(account); +async function writeKeyring(key: KeyringKey, value: string): Promise { + const entry = await getKeyringEntry(key); if (!entry) { throw new Error('OS keyring is not available.'); } entry.setPassword(value); } -async function deleteKeyring(account: string): Promise { +async function deleteKeyring(key: KeyringKey): Promise { try { - const entry = await getKeyringEntry(account); + const entry = await getKeyringEntry(key); if (!entry) return; entry.deletePassword(); } catch (err) { - cliDebugPrint('credentials', `failed to delete ${account} from keyring`, err); + cliDebugPrint('credentials', `failed to delete ${key.service}/${key.account} from keyring`, err); } } -export async function getToken(): Promise { - const backend = await getBackend(); - if (backend === 'keyring') return readKeyring(TOKEN_ACCOUNT); - return readAuthFile().token; -} - -export async function getProxyPassword(): Promise { +/** One account's secret of the given kind, from whichever backend holds it. */ +export async function getSecret(userId: string, kind: SecretKind): Promise { const backend = await getBackend(); - if (backend === 'keyring') return readKeyring(PROXY_PASSWORD_ACCOUNT); - return readAuthFile().proxy?.password; + if (backend === 'keyring') return readKeyring(keyringKey(userId, kind)); + return readProfileSecret(userId, kind); } /** - * Persist token. When `skipIfUnchanged` is true and the stored value already matches, - * the write is skipped. This avoids macOS Keychain prompts on every command. + * Persist one account's secret. When `skipIfUnchanged` is true and the stored value already + * matches, the write is skipped. This avoids macOS Keychain prompts on every command. */ -export async function setToken(token: string, opts: { skipIfUnchanged?: boolean } = {}): Promise { - const backend = await getBackend(); - if (opts.skipIfUnchanged) { - const existing = backend === 'keyring' ? await readKeyring(TOKEN_ACCOUNT) : readAuthFile().token; - if (existing === token) return; - } - - if (backend === 'keyring') { - try { - await writeKeyring(TOKEN_ACCOUNT, token); - return; - } catch (err) { - cliDebugPrint('credentials', 'keyring write failed; falling back to file', err); - downgradeBackendToFile(); - } - } - - const data = readAuthFile(); - data.token = token; - data.secretsBackend = 'file'; - writeAuthFile(data); -} - -export async function setProxyPassword(password: string, opts: { skipIfUnchanged?: boolean } = {}): Promise { +export async function setSecret( + userId: string, + kind: SecretKind, + value: string, + opts: { skipIfUnchanged?: boolean } = {}, +): Promise { const backend = await getBackend(); - if (opts.skipIfUnchanged) { - const existing = backend === 'keyring' ? await readKeyring(PROXY_PASSWORD_ACCOUNT) : readAuthFile().proxy?.password; - if (existing === password) return; - } + if (opts.skipIfUnchanged && (await getSecret(userId, kind)) === value) return; if (backend === 'keyring') { try { - await writeKeyring(PROXY_PASSWORD_ACCOUNT, password); + await writeKeyring(keyringKey(userId, kind), value); return; } catch (err) { cliDebugPrint('credentials', 'keyring write failed; falling back to file', err); @@ -197,40 +206,38 @@ export async function setProxyPassword(password: string, opts: { skipIfUnchanged } } - const data = readAuthFile(); - data.proxy = { ...data.proxy, password }; - data.secretsBackend = 'file'; - writeAuthFile(data); + writeProfileSecret(userId, kind, value); } /** - * Forget the stored proxy password. Called when an account has none, so the previous account's - * does not survive a re-login — the keyring outlives the auth.json rewrite that replaces - * everything else. + * Forget one of an account's secrets. Called for a proxy password when the account has none, so + * the previous account's does not survive a re-login — the keyring outlives the auth.json rewrite + * that replaces everything else. */ -export async function deleteProxyPassword(): Promise { +export async function deleteSecret(userId: string, kind: SecretKind): Promise { if ((await getBackend()) === 'keyring') { - await deleteKeyring(PROXY_PASSWORD_ACCOUNT); + await deleteKeyring(keyringKey(userId, kind)); return; } - const data = readAuthFile(); - if (!data.proxy?.password) return; - - stripProxyPassword(data); - writeAuthFile(data); + deleteProfileSecret(userId, kind); } /** - * Remove the token and proxy-password entries from the OS keyring. Always attempts the - * keyring deletes even when the current backend is `file`, so toggling - * `APIFY_DISABLE_KEYRING=1` between login and logout does not orphan entries the user - * has no in-CLI way to discover. Plaintext secrets in `auth.json` are the caller's - * responsibility (e.g. `logout` removes the whole file). + * Remove one profile's keyring entries, plus the fixed-name entries used before secrets were keyed + * by user. Always attempts the keyring deletes even when the current backend is `file`, so toggling + * `APIFY_DISABLE_KEYRING=1` between login and logout does not orphan entries the user has no + * in-CLI way to discover. + * + * The keyring has no listing API, so `auth.json` is the only index of what it holds. Call this + * before the profile leaves the file, or its entries become unreachable. Secrets stored in + * `auth.json` itself go with the profile that holds them. */ -export async function clearKeyringSecrets(): Promise { - await deleteKeyring(TOKEN_ACCOUNT); - await deleteKeyring(PROXY_PASSWORD_ACCOUNT); +export async function clearKeyringSecrets(userId?: string): Promise { + for (const kind of SECRET_KINDS) { + if (userId) await deleteKeyring(keyringKey(userId, kind)); + await deleteKeyring(legacyKeyringKey(kind)); + } } /** @@ -262,8 +269,10 @@ export async function ensureMigrated(): Promise { } try { - if (file.token) await writeKeyring(TOKEN_ACCOUNT, file.token); - if (file.proxy?.password) await writeKeyring(PROXY_PASSWORD_ACCOUNT, file.proxy.password); + if (file.token) await writeKeyring(legacyKeyringKey('token'), file.token); + if (file.proxy?.password) { + await writeKeyring(legacyKeyringKey('proxy-password'), file.proxy.password); + } } catch (err) { cliDebugPrint('credentials', 'keyring write failed during migration; falling back to file', err); downgradeBackendToFile(); @@ -282,3 +291,101 @@ export async function ensureMigrated(): Promise { })(); return migrationPromise; } + +/** + * Drops secrets there is no user ID to file under. That state already required a re-login — the + * CLI has no account to attach the token to — so nothing reachable is lost. + */ +async function dropUnkeyedSecrets(file: AuthFile): Promise { + for (const kind of SECRET_KINDS) await deleteKeyring(legacyKeyringKey(kind)); + + if (file.token === undefined && file.proxy === undefined) return; + + delete file.token; + delete file.proxy; + writeAuthFile(file); +} + +/** + * Write the new entry, verify it reads back, then delete the old one. The reverse order loses the + * secret when the delete succeeds and the write does not. + */ +async function keyKeyringSecrets(userId: string): Promise { + for (const kind of SECRET_KINDS) { + const legacy = legacyKeyringKey(kind); + const value = await readKeyring(legacy); + if (value === undefined) continue; + + // A failure earlier in this loop downgrades the backend for the rest of the process, so + // the secrets after it belong in the file rather than under a name nothing will read. + if ((await getBackend()) === 'keyring') { + const target = keyringKey(userId, kind); + + try { + await writeKeyring(target, value); + if ((await readKeyring(target)) === value) await deleteKeyring(legacy); + continue; + } catch (err) { + cliDebugPrint('credentials', 'keyring write failed while keying secrets by user', err); + downgradeBackendToFile(); + } + } + + writeProfileSecret(userId, kind, value); + if (readProfileSecret(userId, kind) === value) await deleteKeyring(legacy); + } +} + +/** One atomic write moves the secrets into the profile and clears the top level. */ +function keyFileSecrets(userId: string, file: AuthFile): void { + const profile = file.profiles?.[userId]; + if (!profile) return; + + const { token } = file; + const proxyPassword = file.proxy?.password; + if (token === undefined && proxyPassword === undefined) return; + + if (token !== undefined) profile.token = token; + if (proxyPassword !== undefined) profile.proxy = { password: proxyPassword }; + + delete file.token; + delete file.proxy; + file.secretsBackend = 'file'; + writeAuthFile(file); +} + +/** + * Moves secrets off the fixed names they shared onto keys that carry the user ID, so a second + * account cannot overwrite the first one's token. + * + * Runs after `ensureAuthFileCurrent()` — the user ID comes from the v2 file. A v2 file whose + * secrets still sit under the old names is a supported state: every user is in it between the two + * releases, and the two migrations stay independent. + * + * Idempotent, single-flight, and it never throws — a migration failure must not block a command. + */ +export async function ensureSecretsKeyed(): Promise { + keyingPromise ??= (async () => { + try { + const file = readAuthFile(); + if (file.version !== AUTH_FILE_VERSION) return; + + const userId = file.activeProfile; + if (!userId) { + await dropUnkeyedSecrets(file); + return; + } + + if ((await getBackend()) === 'keyring') { + await keyKeyringSecrets(userId); + return; + } + + keyFileSecrets(userId, file); + } catch (err) { + cliDebugPrint('credentials', 'keying secrets by user failed', err); + } + })(); + + return keyingPromise; +} diff --git a/src/lib/utils.ts b/src/lib/utils.ts index b1ce8e476..08bcb94a1 100644 --- a/src/lib/utils.ts +++ b/src/lib/utils.ts @@ -42,7 +42,7 @@ import { MINIMUM_SUPPORTED_PYTHON_VERSION, SUPPORTED_NODEJS_VERSION, } from './consts.js'; -import { ensureMigrated, getProxyPassword, getToken } from './credentials.js'; +import { ensureMigrated, ensureSecretsKeyed, getSecret } from './credentials.js'; import { deleteFile, ensureFolderExistsSync, rimrafPromised } from './files.js'; import { useCLIMetadata } from './hooks/useCLIMetadata.js'; import { inputFileRegExp, TEMP_INPUT_KEY_PREFIX } from './input-key.js'; @@ -92,34 +92,30 @@ export const getLocalRequestQueuePath = (storeId?: string) => { export const getLocalUserInfo = async (): Promise => { await ensureMigrated(); await ensureAuthFileCurrent(); + await ensureSecretsKeyed(); const { profile, missingProfile } = lookUpActiveProfile(); - const result: AuthJSON = {}; - if (profile) { - result.id = profile.id; - if (profile.username) result.username = profile.username; - if (profile.organizationOwnerUserId) result.organizationOwnerUserId = profile.organizationOwnerUserId; - } - - const token = await getToken(); - if (token) result.token = token; - - const proxyPassword = await getProxyPassword(); - if (proxyPassword) result.proxy = { password: proxyPassword }; - // Reported rather than swallowed: the commands that build `/` lookups would // otherwise fail with a misleading "not found". - if (!profile) { - if (!result.token) return {}; - + if (missingProfile) { throw new Error( - missingProfile - ? `Your active profile "${missingProfile}" is missing from ${AUTH_FILE_PATH()}. Run "apify login" to log in again.` - : 'Stale credentials found without user metadata. Run "apify login" again.', + `Your active profile "${missingProfile}" is missing from ${AUTH_FILE_PATH()}. Run "apify login" to log in again.`, ); } + if (!profile) return {}; + + const result: AuthJSON = { id: profile.id }; + if (profile.username) result.username = profile.username; + if (profile.organizationOwnerUserId) result.organizationOwnerUserId = profile.organizationOwnerUserId; + + const token = await getSecret(profile.id, 'token'); + if (token) result.token = token; + + const proxyPassword = await getSecret(profile.id, 'proxy-password'); + if (proxyPassword) result.proxy = { password: proxyPassword }; + return result; }; diff --git a/test/__setup__/auth-file.ts b/test/__setup__/auth-file.ts index 6e617345d..26588be38 100644 --- a/test/__setup__/auth-file.ts +++ b/test/__setup__/auth-file.ts @@ -3,8 +3,12 @@ import { readFileSync } from 'node:fs'; import type { AuthFile, AuthProfile } from '../../src/lib/auth-file.js'; +import { AUTH_FILE_VERSION } from '../../src/lib/auth-file.js'; import { AUTH_FILE_PATH } from '../../src/lib/consts.js'; +/** The user ID the fixtures below key their single profile by. */ +export const TEST_USER_ID = 'uid'; + /** The raw file, for assertions about the version, the backend marker, or where secrets landed. */ export function readAuthFile(): AuthFile { return JSON.parse(readFileSync(AUTH_FILE_PATH(), 'utf-8')) as AuthFile; @@ -19,6 +23,26 @@ export function readActiveProfile(): (AuthProfile & { id: string }) | undefined return profile ? { id: activeProfile, ...profile } : undefined; } +/** A v2 `auth.json` holding one profile, the shape a login writes. */ +export function v2AuthFile(profile: Partial = {}, rest: Partial = {}): AuthFile { + return { + version: AUTH_FILE_VERSION, + activeProfile: TEST_USER_ID, + profiles: { + [TEST_USER_ID]: { + username: 'me', + name: null, + authMethod: 'token', + expiresAt: null, + hasRefreshToken: false, + loggedInAt: null, + ...profile, + }, + }, + ...rest, + }; +} + /** A v1 `auth.json`, the shape every CLI before the profile migration wrote. */ export function v1AuthFile(overrides: Record = {}) { return { diff --git a/test/__setup__/keyring-mock.ts b/test/__setup__/keyring-mock.ts index 902896793..6bfdf53cb 100644 --- a/test/__setup__/keyring-mock.ts +++ b/test/__setup__/keyring-mock.ts @@ -3,8 +3,16 @@ * `vi.mock('@napi-rs/keyring', () => import('/keyring-mock.js'))`. */ -export const KEYRING_TOKEN_KEY = 'com.apify.cli:token'; -export const KEYRING_PROXY_PASSWORD_KEY = 'com.apify.cli:proxy-password'; +/** The fixed names secrets shared before they were keyed by user. */ +export const LEGACY_KEYRING_TOKEN_KEY = 'com.apify.cli:token'; +export const LEGACY_KEYRING_PROXY_PASSWORD_KEY = 'com.apify.cli:proxy-password'; + +/** + * One service per kind, the user ID as the account. Spelled out here rather than imported so the + * test fails when the production key scheme changes without anyone meaning to change it. + */ +export const keyringTokenKey = (userId: string) => `com.apify.cli.token:${userId}`; +export const keyringProxyPasswordKey = (userId: string) => `com.apify.cli.proxy-password:${userId}`; export const keyringStore = new Map(); diff --git a/test/local/commands/auth.test.ts b/test/local/commands/auth.test.ts index bdcf3cc2f..f7109e8af 100644 --- a/test/local/commands/auth.test.ts +++ b/test/local/commands/auth.test.ts @@ -2,19 +2,22 @@ import { existsSync, statSync } from 'node:fs'; import process from 'node:process'; import { AUTH_FILE_PATH, CommandExitCodes } from '../../../src/lib/consts.js'; -import { getToken } from '../../../src/lib/credentials.js'; +import { getSecret } from '../../../src/lib/credentials.js'; import { clientState, resetApifyClientMock } from '../../__setup__/apify-client-mock.js'; import { readActiveProfile, readAuthFile } from '../../__setup__/auth-file.js'; import { useAuthSetup, useKeyringBackend } from '../../__setup__/hooks/useAuthSetup.js'; import { useConsoleSpy } from '../../__setup__/hooks/useConsoleSpy.js'; import { - KEYRING_PROXY_PASSWORD_KEY, - KEYRING_TOKEN_KEY, + keyringProxyPasswordKey, keyringSetKeys, keyringStore, + keyringTokenKey, resetKeyringMock, } from '../../__setup__/keyring-mock.js'; +const TOKEN_KEY = keyringTokenKey('uid'); +const PROXY_PASSWORD_KEY = keyringProxyPasswordKey('uid'); + vi.mock('@napi-rs/keyring', () => import('../../__setup__/keyring-mock.js')); vi.mock('apify-client', async (importOriginal) => ({ @@ -44,7 +47,8 @@ describe('auth commands', () => { it('login stores the token and one profile keyed by user ID', async () => { await login(); - expect(readAuthFile()).toMatchObject({ version: 2, token: TOKEN, secretsBackend: 'file' }); + expect(readAuthFile()).toMatchObject({ version: 2, secretsBackend: 'file' }); + expect(readAuthFile().token).toBeUndefined(); expect(readActiveProfile()).toEqual({ id: 'uid', username: 'me', @@ -53,6 +57,8 @@ describe('auth commands', () => { expiresAt: null, hasRefreshToken: false, loggedInAt: expect.stringMatching(/^\d{4}-\d{2}-\d{2}T/), + token: TOKEN, + proxy: { password: 'pw' }, }); expect(lastErrorMessage()).toContain('You are logged in to Apify as me'); }); @@ -75,7 +81,7 @@ describe('auth commands', () => { await testRunCommand(AuthLogoutCommand, {}); expect(existsSync(AUTH_FILE_PATH())).toBe(false); - expect(await getToken()).toBeUndefined(); + expect(await getSecret('uid', 'token')).toBeUndefined(); }); it('logging in as another account replaces the stored profile', async () => { @@ -86,10 +92,10 @@ describe('auth commands', () => { await login('apify_api_other_token'); const authFile = readAuthFile(); - expect(authFile).toMatchObject({ activeProfile: 'uid2', token: 'apify_api_other_token' }); + expect(authFile).toMatchObject({ activeProfile: 'uid2' }); // Additive login is a later stage; until then the old profile must not linger. expect(Object.keys(authFile.profiles!)).toEqual(['uid2']); - expect(readActiveProfile()).toMatchObject({ username: 'other' }); + expect(readActiveProfile()).toMatchObject({ username: 'other', token: 'apify_api_other_token' }); }); it('login with an invalid token stores nothing and fails the command', async () => { @@ -119,7 +125,7 @@ describe('auth commands', () => { await login(); - expect(await getToken()).toBe(TOKEN); + expect(await getSecret('uid', 'token')).toBe(TOKEN); expect(lastErrorMessage()).toContain('You are logged in to Apify as me'); }); @@ -128,7 +134,7 @@ describe('auth commands', () => { await login(); - expect(await getToken()).toBe(TOKEN); + expect(await getSecret('uid', 'token')).toBe(TOKEN); expect(lastErrorMessage()).toContain('You are logged in to Apify as me'); }); @@ -174,7 +180,7 @@ describe('auth commands', () => { await testRunCommand(AuthTokenCommand, {}); expect(lastLogMessage()).toBe('apify_api_env_token'); - expect(await getToken()).toBe(TOKEN); + expect(await getSecret('uid', 'token')).toBe(TOKEN); expect(readActiveProfile()).toMatchObject({ username: 'me' }); }); }); @@ -185,8 +191,8 @@ describe('auth commands', () => { it('login stores the secrets in the keyring and keeps them out of auth.json', async () => { await login(); - expect(keyringStore.get(KEYRING_TOKEN_KEY)).toBe(TOKEN); - expect(keyringStore.get(KEYRING_PROXY_PASSWORD_KEY)).toBe('pw'); + expect(keyringStore.get(TOKEN_KEY)).toBe(TOKEN); + expect(keyringStore.get(PROXY_PASSWORD_KEY)).toBe('pw'); const authFile = readAuthFile(); expect(authFile).toMatchObject({ version: 2, secretsBackend: 'keyring' }); @@ -198,21 +204,43 @@ describe('auth commands', () => { it('logging in as an account with no proxy password forgets the previous one', async () => { await login(); - expect(keyringStore.get(KEYRING_PROXY_PASSWORD_KEY)).toBe('pw'); + expect(keyringStore.get(PROXY_PASSWORD_KEY)).toBe('pw'); clientState.user = { id: 'uid2', username: 'other' }; await login('apify_api_other_token'); // The keyring outlives the auth.json rewrite, so without an explicit delete the child // Actor would run with the previous account's proxy credential. - expect(keyringStore.has(KEYRING_PROXY_PASSWORD_KEY)).toBe(false); + expect(keyringStore.has(PROXY_PASSWORD_KEY)).toBe(false); + expect(keyringStore.has(keyringProxyPasswordKey('uid2'))).toBe(false); + }); + + it('switching accounts clears the outgoing account entries', async () => { + await login(); + expect(keyringStore.get(TOKEN_KEY)).toBe(TOKEN); + + clientState.user = { id: 'uid2', username: 'other', proxy: { password: 'pw2' } }; + await login('apify_api_other_token'); + + // auth.json no longer names uid, and the keyring has no listing API, so anything left + // under its key would be unreachable for good. + expect(keyringStore.get(TOKEN_KEY)).toBeUndefined(); + expect(keyringStore.get(keyringTokenKey('uid2'))).toBe('apify_api_other_token'); + }); + + it('logging in again as the same account keeps its entries', async () => { + await login(); + await login(); + + expect(keyringStore.get(TOKEN_KEY)).toBe(TOKEN); + expect(keyringStore.get(PROXY_PASSWORD_KEY)).toBe('pw'); }); it('logging in twice with the same token writes the keyring once', async () => { await login(); await login(); - expect(keyringSetKeys.filter((key) => key === KEYRING_TOKEN_KEY)).toHaveLength(1); + expect(keyringSetKeys.filter((key) => key === TOKEN_KEY)).toHaveLength(1); }); it('auth token prints the token from the keyring', async () => { diff --git a/test/local/lib/auth-file.test.ts b/test/local/lib/auth-file.test.ts index f3b17a90c..4c3980196 100644 --- a/test/local/lib/auth-file.test.ts +++ b/test/local/lib/auth-file.test.ts @@ -13,14 +13,14 @@ import { } from '../../../src/lib/auth-file.js'; import { resolveAuth } from '../../../src/lib/auth.js'; import { AUTH_FILE_PATH, GLOBAL_CONFIGS_FOLDER } from '../../../src/lib/consts.js'; -import { ensureMigrated, getProxyPassword, getToken } from '../../../src/lib/credentials.js'; +import { ensureMigrated, ensureSecretsKeyed, getSecret } from '../../../src/lib/credentials.js'; import { getLocalUserInfo } from '../../../src/lib/utils.js'; import { readActiveProfile, readAuthFile, v1AuthFile } from '../../__setup__/auth-file.js'; import { useAuthSetup, useKeyringBackend } from '../../__setup__/hooks/useAuthSetup.js'; import { useConsoleSpy } from '../../__setup__/hooks/useConsoleSpy.js'; import { - KEYRING_PROXY_PASSWORD_KEY, - KEYRING_TOKEN_KEY, + LEGACY_KEYRING_PROXY_PASSWORD_KEY, + LEGACY_KEYRING_TOKEN_KEY, keyringStore, resetKeyringMock, } from '../../__setup__/keyring-mock.js'; @@ -77,9 +77,12 @@ describe('auth.json v2', () => { await ensureAuthFileCurrent(); - expect(readAuthFile()).toMatchObject({ version: 2, secretsBackend: 'file', token: 'apify_api_v1_token' }); - expect(await getToken()).toBe('apify_api_v1_token'); - expect(await getProxyPassword()).toBe('pw'); + await ensureSecretsKeyed(); + + expect(readAuthFile()).toMatchObject({ version: 2, secretsBackend: 'file' }); + expect(readActiveProfile()).toMatchObject({ token: 'apify_api_v1_token', proxy: { password: 'pw' } }); + expect(await getSecret('uid', 'token')).toBe('apify_api_v1_token'); + expect(await getSecret('uid', 'proxy-password')).toBe('pw'); }); it('drops the fields nothing in the CLI reads', async () => { @@ -208,20 +211,16 @@ describe('auth.json v2', () => { expect(existsSync(AUTH_BACKUP_FILE_PATH())).toBe(false); }); - it('keeps the secrets of a v1 file that has no user ID, so the next command asks for a re-login', async () => { + it('drops the secrets of a v1 file that has no user ID, so the next command asks for a re-login', async () => { write({ token: 'apify_api_v1_token', secretsBackend: 'file' }); await ensureAuthFileCurrent(); + await ensureSecretsKeyed(); - expect(readAuthFile()).toEqual({ - version: 2, - profiles: {}, - secretsBackend: 'file', - token: 'apify_api_v1_token', - }); - // The token stays in auth.json, where the re-login prompt can see it, not in the backup. + // That state already needed a re-login: there is no account to attach the token to. + expect(readAuthFile()).toEqual({ version: 2, profiles: {}, secretsBackend: 'file' }); expect(readBackup()).toEqual({ secretsBackend: 'file' }); - await expect(getLocalUserInfo()).rejects.toThrow('Stale credentials found without user metadata'); + await expect(getLocalUserInfo()).resolves.toEqual({}); }); }); @@ -250,10 +249,10 @@ describe('auth.json v2', () => { await expect(getLocalUserInfo()).rejects.toThrow('Your active profile "gone" is missing'); }); - it('is logged out when the missing profile leaves no token behind either', async () => { + it('names the missing profile even when no secret is left behind', async () => { write({ version: 2, activeProfile: 'gone', profiles: {}, secretsBackend: 'file' }); - await expect(getLocalUserInfo()).resolves.toEqual({}); + await expect(getLocalUserInfo()).rejects.toThrow('Your active profile "gone" is missing'); }); }); @@ -321,7 +320,7 @@ describe('auth.json v2', () => { replaceStoredAccount('new', { ...V2_PROFILE, username: 'new' }, 'file'); // Logged out, rather than logged in as the account that just went away. - await expect(getToken()).resolves.toBeUndefined(); + await expect(getSecret('new', 'token')).resolves.toBeUndefined(); }); }); @@ -376,8 +375,8 @@ describe('auth.json v2', () => { // State B in the wild: secrets already in the keyring, auth.json holding only metadata. it('migrates state B without touching the keyring', async () => { - keyringStore.set(KEYRING_TOKEN_KEY, 'tok_kr'); - keyringStore.set(KEYRING_PROXY_PASSWORD_KEY, 'pw_kr'); + keyringStore.set(LEGACY_KEYRING_TOKEN_KEY, 'tok_kr'); + keyringStore.set(LEGACY_KEYRING_PROXY_PASSWORD_KEY, 'pw_kr'); write({ id: 'uid', username: 'me', email: 'me@example.com', secretsBackend: 'keyring' }); await ensureMigrated(); @@ -404,7 +403,7 @@ describe('auth.json v2', () => { await ensureMigrated(); await ensureAuthFileCurrent(); - expect(keyringStore.get(KEYRING_TOKEN_KEY)).toBe('apify_api_v1_token'); + expect(keyringStore.get(LEGACY_KEYRING_TOKEN_KEY)).toBe('apify_api_v1_token'); expect(readAuthFile()).toEqual({ version: 2, activeProfile: 'uid', diff --git a/test/local/lib/auth.test.ts b/test/local/lib/auth.test.ts index 0e3f4baf7..0ce3d90f1 100644 --- a/test/local/lib/auth.test.ts +++ b/test/local/lib/auth.test.ts @@ -4,7 +4,7 @@ import { ApifyApiError } from 'apify-client'; import { loginWithToken, resolveAuth } from '../../../src/lib/auth.js'; import { AUTH_FILE_PATH, CommandExitCodes } from '../../../src/lib/consts.js'; -import { getProxyPassword, getToken, setToken } from '../../../src/lib/credentials.js'; +import { getSecret } from '../../../src/lib/credentials.js'; import { getCurrentUserInfo, getLoggedClientOrThrow } from '../../../src/lib/utils.js'; import { clientState, resetApifyClientMock } from '../../__setup__/apify-client-mock.js'; import { readActiveProfile } from '../../__setup__/auth-file.js'; @@ -132,8 +132,8 @@ describe('auth', () => { it('saves the token, the proxy password and the account metadata', async () => { await loginWithToken(STORED); - expect(await getToken()).toBe(STORED); - expect(await getProxyPassword()).toBe('pw'); + expect(await getSecret('uid', 'token')).toBe(STORED); + expect(await getSecret('uid', 'proxy-password')).toBe('pw'); expect(readActiveProfile()).toMatchObject({ id: 'uid', username: 'me' }); }); @@ -149,7 +149,7 @@ describe('auth', () => { await loginWithToken(STORED); - expect(await getToken()).toBe(STORED); + expect(await getSecret('uid', 'token')).toBe(STORED); }); }); @@ -237,7 +237,7 @@ describe('auth', () => { await resolveAuth(); - expect(await getToken()).toBe(STORED); + expect(await getSecret('uid', 'token')).toBe(STORED); expect(readActiveProfile()).toMatchObject({ username: 'me' }); }); }); diff --git a/test/local/lib/credentials.test.ts b/test/local/lib/credentials.test.ts index bb46f5c63..6555d0213 100644 --- a/test/local/lib/credentials.test.ts +++ b/test/local/lib/credentials.test.ts @@ -10,20 +10,23 @@ import { AUTH_FILE_PATH, GLOBAL_CONFIGS_FOLDER } from '../../../src/lib/consts.j import { __resetCredentialsForTests, clearKeyringSecrets, + deleteSecret, ensureMigrated, + ensureSecretsKeyed, getBackend, - getProxyPassword, - getToken, - setProxyPassword, - setToken, + getSecret, + setSecret, } from '../../../src/lib/credentials.js'; import { getLocalUserInfo } from '../../../src/lib/utils.js'; +import { TEST_USER_ID, v2AuthFile } from '../../__setup__/auth-file.js'; import { - KEYRING_PROXY_PASSWORD_KEY, - KEYRING_TOKEN_KEY, + LEGACY_KEYRING_PROXY_PASSWORD_KEY, + LEGACY_KEYRING_TOKEN_KEY, keyringFailures, + keyringProxyPasswordKey, keyringSetKeys, keyringStore, + keyringTokenKey, resetKeyringMock, } from '../../__setup__/keyring-mock.js'; @@ -46,6 +49,14 @@ const writeAuthFile = (data: Record) => { const readAuthFile = () => JSON.parse(readFileSync(AUTH_FILE_PATH(), 'utf-8')); +const readProfile = () => readAuthFile().profiles[TEST_USER_ID]; + +const writeV2AuthFile = (...args: Parameters) => + writeAuthFile(v2AuthFile(...args) as Record); + +const TOKEN_KEY = keyringTokenKey(TEST_USER_ID); +const PROXY_PASSWORD_KEY = keyringProxyPasswordKey(TEST_USER_ID); + describe('credentials', () => { beforeEach(() => { vitest.stubEnv('__APIFY_INTERNAL_TEST_AUTH_PATH__', cryptoRandomObjectId(12)); @@ -92,59 +103,80 @@ describe('credentials', () => { describe('file backend', () => { beforeEach(() => { vitest.stubEnv('APIFY_DISABLE_KEYRING', '1'); + writeV2AuthFile(); + writeFileSyncSpy.mockClear(); }); - it('round-trips the token through auth.json', async () => { - await setToken('tok_123'); - expect(await getToken()).toBe('tok_123'); - const file = readAuthFile(); - expect(file.token).toBe('tok_123'); - expect(file.secretsBackend).toBe('file'); + it('round-trips the token through the profile', async () => { + await setSecret(TEST_USER_ID, 'token', 'tok_123'); + expect(await getSecret(TEST_USER_ID, 'token')).toBe('tok_123'); + expect(readProfile().token).toBe('tok_123'); + expect(readAuthFile().token).toBeUndefined(); + expect(readAuthFile().secretsBackend).toBe('file'); + }); + + it('round-trips the proxy password through the profile', async () => { + await setSecret(TEST_USER_ID, 'proxy-password', 'pw_abc'); + expect(await getSecret(TEST_USER_ID, 'proxy-password')).toBe('pw_abc'); + expect(readProfile().proxy).toEqual({ password: 'pw_abc' }); }); - it('round-trips the proxy password through auth.json', async () => { - await setProxyPassword('pw_abc'); - expect(await getProxyPassword()).toBe('pw_abc'); - expect(readAuthFile().proxy).toEqual({ password: 'pw_abc' }); + it('deleteSecret() forgets the proxy password and leaves the token alone', async () => { + await setSecret(TEST_USER_ID, 'token', 'tok_123'); + await setSecret(TEST_USER_ID, 'proxy-password', 'pw_abc'); + + await deleteSecret(TEST_USER_ID, 'proxy-password'); + + expect(readProfile().proxy).toBeUndefined(); + expect(readProfile().token).toBe('tok_123'); }); - it('preserves other proxy fields when only the password changes', async () => { - writeAuthFile({ proxy: { password: 'old', groups: [{ name: 'g' }] } } as never); - await setProxyPassword('new'); - expect(readAuthFile().proxy).toEqual({ password: 'new', groups: [{ name: 'g' }] }); + it('leaves another profile alone', async () => { + const file = v2AuthFile(); + file.profiles!.other = { ...file.profiles![TEST_USER_ID], token: 'tok_other' }; + writeAuthFile(file as Record); + + await setSecret(TEST_USER_ID, 'token', 'tok_123'); + expect(readAuthFile().profiles.other.token).toBe('tok_other'); + }); + + it('does nothing when the profile is not in the file', async () => { + writeAuthFile({ version: 2, activeProfile: 'gone', profiles: {} }); + await setSecret('gone', 'token', 'tok_123'); + expect(await getSecret('gone', 'token')).toBeUndefined(); }); it('skipIfUnchanged skips the write when the stored token matches', async () => { - await setToken('tok_123'); + await setSecret(TEST_USER_ID, 'token', 'tok_123'); writeFileSyncSpy.mockClear(); - await setToken('tok_123', { skipIfUnchanged: true }); + await setSecret(TEST_USER_ID, 'token', 'tok_123', { skipIfUnchanged: true }); expect(authFileWrites()).toHaveLength(0); }); it('skipIfUnchanged skips the write when the stored proxy password matches', async () => { - await setProxyPassword('pw_abc'); + await setSecret(TEST_USER_ID, 'proxy-password', 'pw_abc'); writeFileSyncSpy.mockClear(); - await setProxyPassword('pw_abc', { skipIfUnchanged: true }); + await setSecret(TEST_USER_ID, 'proxy-password', 'pw_abc', { skipIfUnchanged: true }); expect(authFileWrites()).toHaveLength(0); }); it('skipIfUnchanged still writes when the value differs', async () => { - await setToken('tok_123'); + await setSecret(TEST_USER_ID, 'token', 'tok_123'); writeFileSyncSpy.mockClear(); - await setToken('tok_456', { skipIfUnchanged: true }); + await setSecret(TEST_USER_ID, 'token', 'tok_456', { skipIfUnchanged: true }); expect(authFileWrites()).toHaveLength(1); - expect(await getToken()).toBe('tok_456'); + expect(await getSecret(TEST_USER_ID, 'token')).toBe('tok_456'); }); it('writes auth.json with mode 0600', async () => { - await setToken('tok_123'); + await setSecret(TEST_USER_ID, 'token', 'tok_123'); expect(writeFileSyncSpy).toHaveBeenCalledWith(expect.stringContaining(AUTH_FILE_PATH()), expect.any(String), { mode: 0o600, }); }); it.skipIf(process.platform === 'win32')('creates auth.json readable only by the owner', async () => { - await setToken('tok_123'); + await setSecret(TEST_USER_ID, 'token', 'tok_123'); expect(statSync(AUTH_FILE_PATH()).mode & 0o777).toBe(0o600); }); }); @@ -154,83 +186,121 @@ describe('credentials', () => { vitest.stubEnv('APIFY_DISABLE_KEYRING', ''); }); - it('round-trips the token through the keyring and keeps it out of auth.json', async () => { - await setToken('tok_123'); - expect(await getToken()).toBe('tok_123'); - expect(keyringStore.get(KEYRING_TOKEN_KEY)).toBe('tok_123'); + it('keys the token by user ID and keeps it out of auth.json', async () => { + await setSecret(TEST_USER_ID, 'token', 'tok_123'); + expect(await getSecret(TEST_USER_ID, 'token')).toBe('tok_123'); + expect(keyringStore.get(TOKEN_KEY)).toBe('tok_123'); + expect(keyringStore.get(LEGACY_KEYRING_TOKEN_KEY)).toBeUndefined(); expect(existsSync(AUTH_FILE_PATH())).toBe(false); }); - it('round-trips the proxy password through the keyring and keeps it out of auth.json', async () => { - await setProxyPassword('pw_abc'); - expect(await getProxyPassword()).toBe('pw_abc'); - expect(keyringStore.get(KEYRING_PROXY_PASSWORD_KEY)).toBe('pw_abc'); + it('keys the proxy password by user ID and keeps it out of auth.json', async () => { + await setSecret(TEST_USER_ID, 'proxy-password', 'pw_abc'); + expect(await getSecret(TEST_USER_ID, 'proxy-password')).toBe('pw_abc'); + expect(keyringStore.get(PROXY_PASSWORD_KEY)).toBe('pw_abc'); expect(existsSync(AUTH_FILE_PATH())).toBe(false); }); - it('clearKeyringSecrets() removes the token and proxy entries from the keyring', async () => { - await setToken('tok_123'); - await setProxyPassword('pw_abc'); - await clearKeyringSecrets(); - expect(await getToken()).toBeUndefined(); - expect(await getProxyPassword()).toBeUndefined(); + it('gives two accounts their own entries', async () => { + await setSecret(TEST_USER_ID, 'token', 'tok_123'); + await setSecret('other', 'token', 'tok_other'); + expect(keyringStore.get(TOKEN_KEY)).toBe('tok_123'); + expect(keyringStore.get(keyringTokenKey('other'))).toBe('tok_other'); + }); + + it('deleteSecret() removes only that account and kind', async () => { + await setSecret(TEST_USER_ID, 'proxy-password', 'pw_abc'); + await setSecret('other', 'proxy-password', 'pw_other'); + + await deleteSecret(TEST_USER_ID, 'proxy-password'); + + expect(keyringStore.get(PROXY_PASSWORD_KEY)).toBeUndefined(); + expect(keyringStore.get(keyringProxyPasswordKey('other'))).toBe('pw_other'); }); it('skipIfUnchanged skips the keyring write when the stored token matches', async () => { - await setToken('tok_123'); - await setToken('tok_123', { skipIfUnchanged: true }); - expect(keyringSetKeys.filter((key) => key === KEYRING_TOKEN_KEY)).toHaveLength(1); + await setSecret(TEST_USER_ID, 'token', 'tok_123'); + await setSecret(TEST_USER_ID, 'token', 'tok_123', { skipIfUnchanged: true }); + expect(keyringSetKeys.filter((key) => key === TOKEN_KEY)).toHaveLength(1); expect(authFileWrites()).toHaveLength(0); }); it('skipIfUnchanged skips the keyring write when the stored proxy password matches', async () => { - await setProxyPassword('pw_abc'); - await setProxyPassword('pw_abc', { skipIfUnchanged: true }); - expect(keyringSetKeys.filter((key) => key === KEYRING_PROXY_PASSWORD_KEY)).toHaveLength(1); + await setSecret(TEST_USER_ID, 'proxy-password', 'pw_abc'); + await setSecret(TEST_USER_ID, 'proxy-password', 'pw_abc', { skipIfUnchanged: true }); + expect(keyringSetKeys.filter((key) => key === PROXY_PASSWORD_KEY)).toHaveLength(1); expect(authFileWrites()).toHaveLength(0); }); - it('falls back to auth.json when the keyring token write fails', async () => { - keyringFailures.add(KEYRING_TOKEN_KEY); - await setToken('tok_123'); + it('falls back to the profile when the keyring token write fails', async () => { + writeV2AuthFile(); + keyringFailures.add(TOKEN_KEY); + await setSecret(TEST_USER_ID, 'token', 'tok_123'); - expect(keyringStore.get(KEYRING_TOKEN_KEY)).toBeUndefined(); - expect(readAuthFile()).toEqual({ token: 'tok_123', secretsBackend: 'file' }); + expect(keyringStore.get(TOKEN_KEY)).toBeUndefined(); + expect(readProfile().token).toBe('tok_123'); + expect(readAuthFile().secretsBackend).toBe('file'); expect(await getBackend()).toBe('file'); - expect(await getToken()).toBe('tok_123'); + expect(await getSecret(TEST_USER_ID, 'token')).toBe('tok_123'); }); it('keeps using auth.json for later writes after a keyring failure', async () => { - keyringFailures.add(KEYRING_TOKEN_KEY); - await setToken('tok_123'); + writeV2AuthFile(); + keyringFailures.add(TOKEN_KEY); + await setSecret(TEST_USER_ID, 'token', 'tok_123'); - await setProxyPassword('pw_abc'); - expect(keyringStore.get(KEYRING_PROXY_PASSWORD_KEY)).toBeUndefined(); - expect(readAuthFile().proxy).toEqual({ password: 'pw_abc' }); + await setSecret(TEST_USER_ID, 'proxy-password', 'pw_abc'); + expect(keyringStore.get(PROXY_PASSWORD_KEY)).toBeUndefined(); + expect(readProfile().proxy).toEqual({ password: 'pw_abc' }); }); - it('falls back to auth.json when the keyring proxy password write fails', async () => { - keyringFailures.add(KEYRING_PROXY_PASSWORD_KEY); - await setProxyPassword('pw_abc'); + it('falls back to the profile when the keyring proxy password write fails', async () => { + writeV2AuthFile(); + keyringFailures.add(PROXY_PASSWORD_KEY); + await setSecret(TEST_USER_ID, 'proxy-password', 'pw_abc'); - expect(keyringStore.get(KEYRING_PROXY_PASSWORD_KEY)).toBeUndefined(); - expect(readAuthFile()).toEqual({ proxy: { password: 'pw_abc' }, secretsBackend: 'file' }); - expect(await getProxyPassword()).toBe('pw_abc'); + expect(keyringStore.get(PROXY_PASSWORD_KEY)).toBeUndefined(); + expect(readProfile().proxy).toEqual({ password: 'pw_abc' }); + expect(await getSecret(TEST_USER_ID, 'proxy-password')).toBe('pw_abc'); }); }); describe('clearKeyringSecrets()', () => { - it('clears the keyring token entry even when APIFY_DISABLE_KEYRING=1 is set at logout time', async () => { + beforeEach(() => { vitest.stubEnv('APIFY_DISABLE_KEYRING', ''); - await setToken('tok_123'); - expect(keyringStore.get(KEYRING_TOKEN_KEY)).toBe('tok_123'); + }); + + it('removes the profile entries and the fixed-name ones left from before', async () => { + keyringStore.set(LEGACY_KEYRING_TOKEN_KEY, 'tok_old'); + keyringStore.set(LEGACY_KEYRING_PROXY_PASSWORD_KEY, 'pw_old'); + await setSecret(TEST_USER_ID, 'token', 'tok_123'); + await setSecret(TEST_USER_ID, 'proxy-password', 'pw_abc'); + + await clearKeyringSecrets(TEST_USER_ID); + + expect(keyringStore.size).toBe(0); + }); + + it('leaves other profiles alone', async () => { + await setSecret(TEST_USER_ID, 'token', 'tok_123'); + await setSecret('other', 'token', 'tok_other'); + + await clearKeyringSecrets(TEST_USER_ID); + + expect(keyringStore.get(TOKEN_KEY)).toBeUndefined(); + expect(keyringStore.get(keyringTokenKey('other'))).toBe('tok_other'); + }); + + it('clears the keyring entries even when APIFY_DISABLE_KEYRING=1 is set at logout time', async () => { + await setSecret(TEST_USER_ID, 'token', 'tok_123'); + expect(keyringStore.get(TOKEN_KEY)).toBe('tok_123'); __resetCredentialsForTests(); vitest.stubEnv('APIFY_DISABLE_KEYRING', '1'); expect(await getBackend()).toBe('file'); - await clearKeyringSecrets(); - expect(keyringStore.get(KEYRING_TOKEN_KEY)).toBeUndefined(); + await clearKeyringSecrets(TEST_USER_ID); + expect(keyringStore.get(TOKEN_KEY)).toBeUndefined(); }); }); @@ -254,7 +324,7 @@ describe('credentials', () => { vitest.stubEnv('APIFY_DISABLE_KEYRING', ''); writeAuthFile({ token: 'tok', proxy: { password: 'pw' }, secretsBackend: 'keyring' }); await ensureMigrated(); - expect(keyringStore.get(KEYRING_TOKEN_KEY)).toBeUndefined(); + expect(keyringStore.get(LEGACY_KEYRING_TOKEN_KEY)).toBeUndefined(); expect(readAuthFile()).toEqual({ token: 'tok', proxy: { password: 'pw' }, secretsBackend: 'keyring' }); }); @@ -278,8 +348,8 @@ describe('credentials', () => { vitest.stubEnv('APIFY_DISABLE_KEYRING', ''); writeAuthFile({ token: 'tok', proxy: { password: 'pw' }, username: 'u' }); await ensureMigrated(); - expect(keyringStore.get(KEYRING_TOKEN_KEY)).toBe('tok'); - expect(keyringStore.get(KEYRING_PROXY_PASSWORD_KEY)).toBe('pw'); + expect(keyringStore.get(LEGACY_KEYRING_TOKEN_KEY)).toBe('tok'); + expect(keyringStore.get(LEGACY_KEYRING_PROXY_PASSWORD_KEY)).toBe('pw'); const file = readAuthFile(); expect(file.token).toBeUndefined(); expect(file.proxy).toBeUndefined(); @@ -291,7 +361,7 @@ describe('credentials', () => { vitest.stubEnv('APIFY_DISABLE_KEYRING', ''); writeAuthFile({ token: 'tok', proxy: { password: 'pw', groups: [{ name: 'g' }] }, username: 'u' }); await ensureMigrated(); - expect(keyringStore.get(KEYRING_PROXY_PASSWORD_KEY)).toBe('pw'); + expect(keyringStore.get(LEGACY_KEYRING_PROXY_PASSWORD_KEY)).toBe('pw'); const file = readAuthFile(); expect(file.proxy).toEqual({ groups: [{ name: 'g' }] }); expect(file.secretsBackend).toBe('keyring'); @@ -301,7 +371,7 @@ describe('credentials', () => { vitest.stubEnv('APIFY_DISABLE_KEYRING', ''); writeAuthFile({ proxy: { password: 'pw' }, username: 'u' }); await ensureMigrated(); - expect(keyringStore.get(KEYRING_PROXY_PASSWORD_KEY)).toBe('pw'); + expect(keyringStore.get(LEGACY_KEYRING_PROXY_PASSWORD_KEY)).toBe('pw'); const file = readAuthFile(); expect(file.proxy).toBeUndefined(); expect(file.username).toBe('u'); @@ -319,7 +389,7 @@ describe('credentials', () => { it('falls back to file backend when the proxy keyring write fails after token succeeds', async () => { vitest.stubEnv('APIFY_DISABLE_KEYRING', ''); - keyringFailures.add(KEYRING_PROXY_PASSWORD_KEY); + keyringFailures.add(LEGACY_KEYRING_PROXY_PASSWORD_KEY); writeAuthFile({ token: 'tok', proxy: { password: 'pw' }, username: 'u' }); await ensureMigrated(); const file = readAuthFile(); @@ -342,7 +412,115 @@ describe('credentials', () => { }); }); + describe('ensureSecretsKeyed()', () => { + it('moves keyring entries off the fixed names onto the user ID', async () => { + vitest.stubEnv('APIFY_DISABLE_KEYRING', ''); + writeV2AuthFile({}, { secretsBackend: 'keyring' }); + keyringStore.set(LEGACY_KEYRING_TOKEN_KEY, 'tok'); + keyringStore.set(LEGACY_KEYRING_PROXY_PASSWORD_KEY, 'pw'); + + await ensureSecretsKeyed(); + + expect(keyringStore.get(TOKEN_KEY)).toBe('tok'); + expect(keyringStore.get(PROXY_PASSWORD_KEY)).toBe('pw'); + expect(keyringStore.get(LEGACY_KEYRING_TOKEN_KEY)).toBeUndefined(); + expect(keyringStore.get(LEGACY_KEYRING_PROXY_PASSWORD_KEY)).toBeUndefined(); + }); + + it('moves top-level file secrets into the profile', async () => { + vitest.stubEnv('APIFY_DISABLE_KEYRING', '1'); + writeV2AuthFile({}, { secretsBackend: 'file', token: 'tok', proxy: { password: 'pw' } }); + + await ensureSecretsKeyed(); + + expect(readProfile()).toMatchObject({ token: 'tok', proxy: { password: 'pw' } }); + const file = readAuthFile(); + expect(file.token).toBeUndefined(); + expect(file.proxy).toBeUndefined(); + expect(file.secretsBackend).toBe('file'); + }); + + it('drops secrets it has no user ID to file under', async () => { + vitest.stubEnv('APIFY_DISABLE_KEYRING', ''); + writeAuthFile({ version: 2, profiles: {}, secretsBackend: 'keyring', token: 'tok' }); + keyringStore.set(LEGACY_KEYRING_TOKEN_KEY, 'tok_kr'); + + await ensureSecretsKeyed(); + + expect(readAuthFile().token).toBeUndefined(); + expect(keyringStore.get(LEGACY_KEYRING_TOKEN_KEY)).toBeUndefined(); + }); + + it('drops the legacy entries even under APIFY_DISABLE_KEYRING=1', async () => { + vitest.stubEnv('APIFY_DISABLE_KEYRING', '1'); + writeAuthFile({ version: 2, profiles: {}, secretsBackend: 'keyring' }); + keyringStore.set(LEGACY_KEYRING_TOKEN_KEY, 'tok_kr'); + + await ensureSecretsKeyed(); + + expect(keyringStore.get(LEGACY_KEYRING_TOKEN_KEY)).toBeUndefined(); + }); + + it('is a no-op on a file whose secrets are already keyed', async () => { + vitest.stubEnv('APIFY_DISABLE_KEYRING', '1'); + writeV2AuthFile({ token: 'tok' }, { secretsBackend: 'file' }); + writeFileSyncSpy.mockClear(); + + await ensureSecretsKeyed(); + + expect(authFileWrites()).toHaveLength(0); + expect(readProfile().token).toBe('tok'); + }); + + it('is a no-op on a file the shape migration has not reached', async () => { + vitest.stubEnv('APIFY_DISABLE_KEYRING', '1'); + writeAuthFile({ id: 'uid', token: 'tok' }); + writeFileSyncSpy.mockClear(); + + await ensureSecretsKeyed(); + + expect(authFileWrites()).toHaveLength(0); + expect(readAuthFile().token).toBe('tok'); + }); + + it('downgrades to the file backend when the keyring write fails mid-migration', async () => { + vitest.stubEnv('APIFY_DISABLE_KEYRING', ''); + writeV2AuthFile({}, { secretsBackend: 'keyring' }); + keyringStore.set(LEGACY_KEYRING_TOKEN_KEY, 'tok'); + keyringStore.set(LEGACY_KEYRING_PROXY_PASSWORD_KEY, 'pw'); + keyringFailures.add(TOKEN_KEY); + + await ensureSecretsKeyed(); + + // Both secrets land in the file: the downgrade holds for the rest of the loop. + expect(readProfile()).toMatchObject({ token: 'tok', proxy: { password: 'pw' } }); + expect(readAuthFile().secretsBackend).toBe('file'); + expect(await getBackend()).toBe('file'); + expect(keyringStore.size).toBe(0); + }); + + it('is memoized within a process', async () => { + vitest.stubEnv('APIFY_DISABLE_KEYRING', '1'); + writeV2AuthFile({}, { secretsBackend: 'file', token: 'tok' }); + await ensureSecretsKeyed(); + expect(readProfile().token).toBe('tok'); + + writeV2AuthFile({}, { secretsBackend: 'file', token: 'tok2' }); + await ensureSecretsKeyed(); + expect(readAuthFile().token).toBe('tok2'); + }); + }); + describe('getLocalUserInfo()', () => { + it('on file backend, reads the token and proxy password from the profile', async () => { + vitest.stubEnv('APIFY_DISABLE_KEYRING', '1'); + writeV2AuthFile({ token: 'tok', proxy: { password: 'pw' } }, { secretsBackend: 'file' }); + + const info = await getLocalUserInfo(); + expect(info.token).toBe('tok'); + expect(info.proxy).toEqual({ password: 'pw' }); + }); + it('on file backend, keeps the proxy password and drops the groups nothing reads', async () => { vitest.stubEnv('APIFY_DISABLE_KEYRING', '1'); writeAuthFile({ @@ -358,8 +536,8 @@ describe('credentials', () => { it('on keyring backend, overlays token and proxy password from keyring', async () => { vitest.stubEnv('APIFY_DISABLE_KEYRING', ''); - keyringStore.set(KEYRING_TOKEN_KEY, 'tok_kr'); - keyringStore.set(KEYRING_PROXY_PASSWORD_KEY, 'pw_kr'); + keyringStore.set(LEGACY_KEYRING_TOKEN_KEY, 'tok_kr'); + keyringStore.set(LEGACY_KEYRING_PROXY_PASSWORD_KEY, 'pw_kr'); writeAuthFile({ username: 'me', id: 'uid', secretsBackend: 'keyring' }); const info = await getLocalUserInfo(); expect(info.token).toBe('tok_kr'); @@ -371,16 +549,23 @@ describe('credentials', () => { expect(await getLocalUserInfo()).toEqual({}); }); - it('on file backend, throws when a token is stored without user metadata', async () => { + it('on file backend, reports logged out for a token stored without user metadata', async () => { vitest.stubEnv('APIFY_DISABLE_KEYRING', '1'); writeAuthFile({ token: 'tok', secretsBackend: 'file' }); - await expect(getLocalUserInfo()).rejects.toThrow('Stale credentials found without user metadata'); + + expect(await getLocalUserInfo()).toEqual({}); + // The secret is dropped rather than left unreachable, so the next command asks for a login. + expect(readAuthFile().token).toBeUndefined(); }); - it('on keyring backend, throws when the keyring holds a token but auth.json is gone', async () => { + it('on keyring backend, reports logged out when the keyring holds a token but auth.json is gone', async () => { vitest.stubEnv('APIFY_DISABLE_KEYRING', ''); - keyringStore.set(KEYRING_TOKEN_KEY, 'tok_kr'); - await expect(getLocalUserInfo()).rejects.toThrow('Stale credentials found without user metadata'); + keyringStore.set(LEGACY_KEYRING_TOKEN_KEY, 'tok_kr'); + + expect(await getLocalUserInfo()).toEqual({}); + // auth.json is the only index of the keyring, so a hand-deleted file strands the entry. + // Reaching for it on a machine with no account would touch the keyring on every command. + expect(keyringStore.get(LEGACY_KEYRING_TOKEN_KEY)).toBe('tok_kr'); }); }); }); From 85ac3fd011a70e12bfda27270bee725e0bd21309 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Richard=20Sol=C3=A1r?= Date: Thu, 24 Sep 2026 14:21:42 +0200 Subject: [PATCH 2/5] feat: let a profile record its own secrets backend A keyring write that fails for one account used to flip the file-level marker, sending every other account to a file that does not hold their secrets. The fallback is now recorded on the profile that hit it. Co-Authored-By: Claude Opus 5 --- src/lib/auth-file.ts | 36 +++++++++++++++++------ src/lib/credentials.ts | 46 +++++++++++++++++++----------- test/local/lib/credentials.test.ts | 43 ++++++++++++++++++++-------- 3 files changed, 88 insertions(+), 37 deletions(-) diff --git a/src/lib/auth-file.ts b/src/lib/auth-file.ts index 6d2d66142..005716824 100644 --- a/src/lib/auth-file.ts +++ b/src/lib/auth-file.ts @@ -27,9 +27,9 @@ export interface AuthProfile { expiresAt: string | null; hasRefreshToken: boolean; /** - * Where this profile's secrets live. Unused while the file holds one account, so the file-level - * `secretsBackend` is still the answer for every profile. Reserved for Stage-2, where a keyring - * failure on one profile must not silently redirect another profile's reads. + * Where this profile's secrets live, when that differs from the file-level `secretsBackend`. + * Written only when a keyring write for this profile fails, so one profile falling back to the + * file cannot silently redirect another profile's reads to a place its secrets are not. */ secretsBackend?: CredentialsBackend; loggedInAt: string | null; @@ -284,20 +284,39 @@ export function readProfileSecret(userId: string, kind: SecretKind): string | un return kind === 'token' ? profile.token : profile.proxy?.password; } +/** Where this profile's secrets live, or `undefined` when it follows the file-level default. */ +export function readProfileBackend(userId: string): CredentialsBackend | undefined { + return readAuthFile().profiles?.[userId]?.secretsBackend; +} + /** * Stores a file-backend secret on the profile. A missing profile is left alone: inventing one * would fabricate the account metadata the CLI reads. */ export function writeProfileSecret(userId: string, kind: SecretKind, value: string) { + updateProfile(userId, (profile) => setProfileSecret(profile, kind, value)); +} + +/** + * Stores the secret and records that this profile reads from the file from now on, in one write. + * Called when a keyring write for this profile failed: splitting the two would leave a window + * where the profile looks logged out, or where it still points at a keyring entry that is not there. + */ +export function moveProfileSecretToFile(userId: string, kind: SecretKind, value: string) { updateProfile(userId, (profile) => { - if (kind === 'token') { - profile.token = value; - } else { - profile.proxy = { ...profile.proxy, password: value }; - } + setProfileSecret(profile, kind, value); + profile.secretsBackend = 'file'; }); } +function setProfileSecret(profile: AuthProfile, kind: SecretKind, value: string) { + if (kind === 'token') { + profile.token = value; + } else { + profile.proxy = { ...profile.proxy, password: value }; + } +} + /** Forgets one of a profile's file-backend secrets. */ export function deleteProfileSecret(userId: string, kind: SecretKind) { if (readProfileSecret(userId, kind) === undefined) return; @@ -318,7 +337,6 @@ function updateProfile(userId: string, edit: (profile: AuthProfile) => void) { if (!profile) return; edit(profile); - file.secretsBackend = 'file'; writeAuthFile(file); } diff --git a/src/lib/credentials.ts b/src/lib/credentials.ts index 633fe7127..38d495f43 100644 --- a/src/lib/credentials.ts +++ b/src/lib/credentials.ts @@ -4,7 +4,9 @@ import type { AuthFile } from './auth-file.js'; import { AUTH_FILE_VERSION, deleteProfileSecret, + moveProfileSecretToFile, readAuthFile, + readProfileBackend, readProfileSecret, writeAuthFile, writeProfileSecret, @@ -105,9 +107,11 @@ async function importKeyringModule(): Promise { * Single-flight via a promise so concurrent callers share the same lookup. * Order: APIFY_DISABLE_KEYRING env override -> persisted marker in auth.json -> module load. * + * This is the default every profile follows; a profile whose keyring write failed records its own + * `secretsBackend` and reads through {@link backendFor} instead. + * * No write-probe runs here: on macOS that would pop a keychain prompt before the user has - * authorized one. The first real write is the probe — failure is caught and downgraded - * via `downgradeBackendToFile()`, persisting the file marker so future runs skip the keyring. + * authorized one. The first real write is the probe, and a failure falls back to the file. */ export async function getBackend(): Promise { if (backendPromise) return backendPromise; @@ -123,8 +127,9 @@ export async function getBackend(): Promise { } /** - * Called when a keyring write fails at runtime. Flips the cached backend so subsequent - * reads/writes use the file path immediately, without waiting for the marker on disk. + * Called when a keyring write fails before any profile exists, so there is nothing to record the + * fallback on but the file itself. Flips the cached backend so subsequent reads and writes use the + * file path immediately, without waiting for the marker on disk. */ function downgradeBackendToFile() { backendPromise = Promise.resolve('file'); @@ -176,10 +181,17 @@ async function deleteKeyring(key: KeyringKey): Promise { } } +/** + * Where one account's secrets live. A profile that fell back to the file after a keyring failure + * says so itself; every other profile follows the file-level choice. + */ +async function backendFor(userId: string): Promise { + return readProfileBackend(userId) ?? (await getBackend()); +} + /** One account's secret of the given kind, from whichever backend holds it. */ export async function getSecret(userId: string, kind: SecretKind): Promise { - const backend = await getBackend(); - if (backend === 'keyring') return readKeyring(keyringKey(userId, kind)); + if ((await backendFor(userId)) === 'keyring') return readKeyring(keyringKey(userId, kind)); return readProfileSecret(userId, kind); } @@ -193,7 +205,7 @@ export async function setSecret( value: string, opts: { skipIfUnchanged?: boolean } = {}, ): Promise { - const backend = await getBackend(); + const backend = await backendFor(userId); if (opts.skipIfUnchanged && (await getSecret(userId, kind)) === value) return; if (backend === 'keyring') { @@ -201,8 +213,11 @@ export async function setSecret( await writeKeyring(keyringKey(userId, kind), value); return; } catch (err) { + // Recorded on the profile rather than on the file, so an account whose secrets are in + // the keyring is not redirected to a file that does not hold them. cliDebugPrint('credentials', 'keyring write failed; falling back to file', err); - downgradeBackendToFile(); + moveProfileSecretToFile(userId, kind, value); + return; } } @@ -215,7 +230,7 @@ export async function setSecret( * that replaces everything else. */ export async function deleteSecret(userId: string, kind: SecretKind): Promise { - if ((await getBackend()) === 'keyring') { + if ((await backendFor(userId)) === 'keyring') { await deleteKeyring(keyringKey(userId, kind)); return; } @@ -316,9 +331,9 @@ async function keyKeyringSecrets(userId: string): Promise { const value = await readKeyring(legacy); if (value === undefined) continue; - // A failure earlier in this loop downgrades the backend for the rest of the process, so - // the secrets after it belong in the file rather than under a name nothing will read. - if ((await getBackend()) === 'keyring') { + // A failure earlier in this loop moved this profile to the file, so the secrets after it + // belong there too rather than under a keyring name nothing will read. + if ((await backendFor(userId)) === 'keyring') { const target = keyringKey(userId, kind); try { @@ -327,11 +342,10 @@ async function keyKeyringSecrets(userId: string): Promise { continue; } catch (err) { cliDebugPrint('credentials', 'keyring write failed while keying secrets by user', err); - downgradeBackendToFile(); } } - writeProfileSecret(userId, kind, value); + moveProfileSecretToFile(userId, kind, value); if (readProfileSecret(userId, kind) === value) await deleteKeyring(legacy); } } @@ -348,9 +362,9 @@ function keyFileSecrets(userId: string, file: AuthFile): void { if (token !== undefined) profile.token = token; if (proxyPassword !== undefined) profile.proxy = { password: proxyPassword }; + // The file-level marker already says `file`: nothing else puts secrets at the top level. delete file.token; delete file.proxy; - file.secretsBackend = 'file'; writeAuthFile(file); } @@ -376,7 +390,7 @@ export async function ensureSecretsKeyed(): Promise { return; } - if ((await getBackend()) === 'keyring') { + if ((await backendFor(userId)) === 'keyring') { await keyKeyringSecrets(userId); return; } diff --git a/test/local/lib/credentials.test.ts b/test/local/lib/credentials.test.ts index 6555d0213..a620d2a92 100644 --- a/test/local/lib/credentials.test.ts +++ b/test/local/lib/credentials.test.ts @@ -103,7 +103,7 @@ describe('credentials', () => { describe('file backend', () => { beforeEach(() => { vitest.stubEnv('APIFY_DISABLE_KEYRING', '1'); - writeV2AuthFile(); + writeV2AuthFile({}, { secretsBackend: 'file' }); writeFileSyncSpy.mockClear(); }); @@ -112,7 +112,8 @@ describe('credentials', () => { expect(await getSecret(TEST_USER_ID, 'token')).toBe('tok_123'); expect(readProfile().token).toBe('tok_123'); expect(readAuthFile().token).toBeUndefined(); - expect(readAuthFile().secretsBackend).toBe('file'); + // The profile follows the file-level choice, so it records no backend of its own. + expect(readProfile().secretsBackend).toBeUndefined(); }); it('round-trips the proxy password through the profile', async () => { @@ -233,19 +234,22 @@ describe('credentials', () => { }); it('falls back to the profile when the keyring token write fails', async () => { - writeV2AuthFile(); + writeV2AuthFile({}, { secretsBackend: 'keyring' }); keyringFailures.add(TOKEN_KEY); await setSecret(TEST_USER_ID, 'token', 'tok_123'); expect(keyringStore.get(TOKEN_KEY)).toBeUndefined(); expect(readProfile().token).toBe('tok_123'); - expect(readAuthFile().secretsBackend).toBe('file'); - expect(await getBackend()).toBe('file'); + // Recorded on the profile. The file-level choice is left alone, so it still describes + // every account whose secrets did reach the keyring. + expect(readProfile().secretsBackend).toBe('file'); + expect(readAuthFile().secretsBackend).toBe('keyring'); + expect(await getBackend()).toBe('keyring'); expect(await getSecret(TEST_USER_ID, 'token')).toBe('tok_123'); }); it('keeps using auth.json for later writes after a keyring failure', async () => { - writeV2AuthFile(); + writeV2AuthFile({}, { secretsBackend: 'keyring' }); keyringFailures.add(TOKEN_KEY); await setSecret(TEST_USER_ID, 'token', 'tok_123'); @@ -254,13 +258,29 @@ describe('credentials', () => { expect(readProfile().proxy).toEqual({ password: 'pw_abc' }); }); + it('leaves another profile on the keyring after one profile falls back', async () => { + const file = v2AuthFile({}, { secretsBackend: 'keyring' }); + file.profiles!.other = { ...file.profiles![TEST_USER_ID] }; + writeAuthFile(file as Record); + keyringFailures.add(TOKEN_KEY); + + await setSecret(TEST_USER_ID, 'token', 'tok_123'); + await setSecret('other', 'token', 'tok_other'); + + expect(readAuthFile().profiles.other.secretsBackend).toBeUndefined(); + expect(keyringStore.get(keyringTokenKey('other'))).toBe('tok_other'); + expect(await getSecret('other', 'token')).toBe('tok_other'); + expect(await getSecret(TEST_USER_ID, 'token')).toBe('tok_123'); + }); + it('falls back to the profile when the keyring proxy password write fails', async () => { - writeV2AuthFile(); + writeV2AuthFile({}, { secretsBackend: 'keyring' }); keyringFailures.add(PROXY_PASSWORD_KEY); await setSecret(TEST_USER_ID, 'proxy-password', 'pw_abc'); expect(keyringStore.get(PROXY_PASSWORD_KEY)).toBeUndefined(); expect(readProfile().proxy).toEqual({ password: 'pw_abc' }); + expect(readProfile().secretsBackend).toBe('file'); expect(await getSecret(TEST_USER_ID, 'proxy-password')).toBe('pw_abc'); }); }); @@ -483,7 +503,7 @@ describe('credentials', () => { expect(readAuthFile().token).toBe('tok'); }); - it('downgrades to the file backend when the keyring write fails mid-migration', async () => { + it('moves the profile to the file when the keyring write fails mid-migration', async () => { vitest.stubEnv('APIFY_DISABLE_KEYRING', ''); writeV2AuthFile({}, { secretsBackend: 'keyring' }); keyringStore.set(LEGACY_KEYRING_TOKEN_KEY, 'tok'); @@ -492,10 +512,9 @@ describe('credentials', () => { await ensureSecretsKeyed(); - // Both secrets land in the file: the downgrade holds for the rest of the loop. - expect(readProfile()).toMatchObject({ token: 'tok', proxy: { password: 'pw' } }); - expect(readAuthFile().secretsBackend).toBe('file'); - expect(await getBackend()).toBe('file'); + // Both secrets land in the file: the fallback holds for the rest of the loop. + expect(readProfile()).toMatchObject({ token: 'tok', proxy: { password: 'pw' }, secretsBackend: 'file' }); + expect(readAuthFile().secretsBackend).toBe('keyring'); expect(keyringStore.size).toBe(0); }); From ac5a5963c842c84d8d378a36df459266d36d5bba Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Richard=20Sol=C3=A1r?= Date: Thu, 24 Sep 2026 18:07:45 +0200 Subject: [PATCH 3/5] fix: clear keyring entries only once the file write has succeeded auth.json is the only index of what the keyring holds, so both commands destroyed entries before a step that can throw. A failed account switch left the outgoing account's token deleted and the new one unwritten, and a failed logout destroyed the secrets while auth.json still named the account. Login now clears after the switch is on disk; logout attempts both steps and reports what is left behind instead of claiming success. Co-Authored-By: Claude Opus 5 --- src/commands/auth/logout.ts | 50 ++++++++++++++++++++++++++++---- src/lib/auth.ts | 10 +++---- test/local/commands/auth.test.ts | 49 +++++++++++++++++++++++++++++-- 3 files changed, 96 insertions(+), 13 deletions(-) diff --git a/src/commands/auth/logout.ts b/src/commands/auth/logout.ts index 9f84a2092..e7593068e 100644 --- a/src/commands/auth/logout.ts +++ b/src/commands/auth/logout.ts @@ -1,12 +1,14 @@ +import process from 'node:process'; + import { APIFY_ENV_VARS } from '@apify/consts'; import { getActiveProfileId, removeActiveProfile } from '../../lib/auth-file.js'; import { invalidEnvTokenMessage, readEnvToken } from '../../lib/auth.js'; import { ApifyCommand } from '../../lib/command-framework/apify-command.js'; -import { AUTH_FILE_PATH } from '../../lib/consts.js'; +import { AUTH_FILE_PATH, CommandExitCodes } from '../../lib/consts.js'; import { clearKeyringSecrets } from '../../lib/credentials.js'; import { updateUserId } from '../../lib/hooks/telemetry/useTelemetryState.js'; -import { success, warning } from '../../lib/outputs.js'; +import { error, success, warning } from '../../lib/outputs.js'; import { tildify } from '../../lib/utils.js'; export class AuthLogoutCommand extends ApifyCommand { @@ -28,10 +30,28 @@ export class AuthLogoutCommand extends ApifyCommand { static override docsUrl = 'https://docs.apify.com/cli/docs/reference#apify-logout'; async run() { - // The keyring goes first: `auth.json` is the only index of what it holds, so removing the - // profile would strand its entries. - await clearKeyringSecrets(getActiveProfileId()); - removeActiveProfile(); + // Read before either step runs: once the profile is gone, nothing names the keyring entries it owns. + const activeProfileId = getActiveProfileId(); + + // Both steps are attempted even when the first one fails, so neither the secrets nor the + // profile are left behind just because the other could not be removed. + const keyringError = await clearKeyringSecrets(activeProfileId).then( + () => null, + (err: unknown) => err, + ); + + let profileError: unknown = null; + try { + removeActiveProfile(); + } catch (err) { + profileError = err; + } + + if (keyringError || profileError) { + error({ message: partialLogoutMessage(activeProfileId, keyringError, profileError) }); + process.exitCode = CommandExitCodes.RunFailed; + return; + } await updateUserId(null); @@ -47,3 +67,21 @@ export class AuthLogoutCommand extends ApifyCommand { } } } + +function reasonOf(err: unknown) { + return err instanceof Error ? err.message : String(err); +} + +function partialLogoutMessage(activeProfileId: string | undefined, keyringError: unknown, profileError: unknown) { + const keyringPart = keyringError + ? `Your secrets are still in the OS keyring${activeProfileId ? ` under the account ${activeProfileId}` : ''}; delete them with your OS keyring app.` + : 'Your secrets were removed from the OS keyring.'; + + const profilePart = profileError + ? `Your account is still in ${AUTH_FILE_PATH()}; delete that file to finish logging out.` + : `Your account was removed from ${AUTH_FILE_PATH()}.`; + + const reasons = [keyringError, profileError].filter(Boolean).map(reasonOf).join(' '); + + return `Logout did not finish. ${keyringPart} ${profilePart} ${reasons}`; +} diff --git a/src/lib/auth.ts b/src/lib/auth.ts index e7a5b7704..7085797e3 100644 --- a/src/lib/auth.ts +++ b/src/lib/auth.ts @@ -180,12 +180,7 @@ export async function loginWithToken( const proxyPassword = userInfo.proxy?.password; - // `auth.json` is the only index of what the keyring holds, so the outgoing account's entries - // have to go before its ID leaves the file. const previousUserId = getActiveProfileId(); - if (previousUserId && previousUserId !== userInfo.id) { - await clearKeyringSecrets(previousUserId); - } const { organizationOwnerUserId } = userInfo as { organizationOwnerUserId?: string }; replaceStoredAccount( @@ -202,6 +197,11 @@ export async function loginWithToken( await getBackend(), ); + // Only once the switch is on disk: a failed write leaves auth.json naming the previous account, whose entries nothing else can find. + if (previousUserId && previousUserId !== userInfo.id) { + await clearKeyringSecrets(previousUserId); + } + // After the account, which drops the previous secrets. `skipIfUnchanged` avoids a Keychain prompt. await setSecret(userInfo.id, 'token', token, { skipIfUnchanged: true }); diff --git a/test/local/commands/auth.test.ts b/test/local/commands/auth.test.ts index f7109e8af..e678ba951 100644 --- a/test/local/commands/auth.test.ts +++ b/test/local/commands/auth.test.ts @@ -1,7 +1,7 @@ -import { existsSync, statSync } from 'node:fs'; +import { chmodSync, existsSync, statSync } from 'node:fs'; import process from 'node:process'; -import { AUTH_FILE_PATH, CommandExitCodes } from '../../../src/lib/consts.js'; +import { AUTH_FILE_PATH, CommandExitCodes, GLOBAL_CONFIGS_FOLDER } from '../../../src/lib/consts.js'; import { getSecret } from '../../../src/lib/credentials.js'; import { clientState, resetApifyClientMock } from '../../__setup__/apify-client-mock.js'; import { readActiveProfile, readAuthFile } from '../../__setup__/auth-file.js'; @@ -257,5 +257,50 @@ describe('auth commands', () => { expect(keyringStore.size).toBe(0); expect(existsSync(AUTH_FILE_PATH())).toBe(false); }); + + // Clearing the keyring before the switch is written left both accounts unreachable: the + // keyring has no listing API, so auth.json is the only index of what it holds. + it.skipIf(process.platform === 'win32')( + 'a switch that cannot be written keeps the outgoing account entries', + async () => { + await login(); + expect(keyringStore.get(TOKEN_KEY)).toBe(TOKEN); + + clientState.user = { id: 'uid2', username: 'other' }; + chmodSync(GLOBAL_CONFIGS_FOLDER(), 0o500); + + try { + await login('apify_api_other_token'); + + expect(readActiveProfile()).toMatchObject({ id: 'uid' }); + expect(keyringStore.get(TOKEN_KEY)).toBe(TOKEN); + expect(keyringStore.get(PROXY_PASSWORD_KEY)).toBe('pw'); + } finally { + chmodSync(GLOBAL_CONFIGS_FOLDER(), 0o700); + process.exitCode = 0; + } + }, + ); + + // Exiting 0 with a success line told the user they were logged out while auth.json still + // held the account the keyring entries were just deleted for. + it.skipIf(process.platform === 'win32')('logout says so when the profile cannot be removed', async () => { + await login(); + chmodSync(GLOBAL_CONFIGS_FOLDER(), 0o500); + + try { + await testRunCommand(AuthLogoutCommand, {}); + + expect(keyringStore.size).toBe(0); + expect(existsSync(AUTH_FILE_PATH())).toBe(true); + expect(lastErrorMessage()).toContain('Logout did not finish'); + expect(lastErrorMessage()).toContain('Your secrets were removed from the OS keyring.'); + expect(lastErrorMessage()).toContain(`Your account is still in ${AUTH_FILE_PATH()}`); + expect(process.exitCode).toBe(CommandExitCodes.RunFailed); + } finally { + chmodSync(GLOBAL_CONFIGS_FOLDER(), 0o700); + process.exitCode = 0; + } + }); }); }); From d949e0a12e5a598475a3dee338e7c23130855fc4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Richard=20Sol=C3=A1r?= Date: Thu, 24 Sep 2026 18:17:26 +0200 Subject: [PATCH 4/5] fix: say the stored login cannot be read when the migration fails Secret reads are v2-only, so a failed shape migration leaves the CLI unable to read the token. It still told the user their login worked, and the next command said they were not logged in. The warning now states what is true and how to fix it. Co-Authored-By: Claude Opus 5 --- src/lib/auth-file.ts | 5 ++--- test/local/lib/auth-file.test.ts | 31 ++++++++++++++++++------------- 2 files changed, 20 insertions(+), 16 deletions(-) diff --git a/src/lib/auth-file.ts b/src/lib/auth-file.ts index 005716824..e39b033f9 100644 --- a/src/lib/auth-file.ts +++ b/src/lib/auth-file.ts @@ -197,11 +197,10 @@ async function migrateAuthFile(): Promise { writeAuthFile(migrated); } catch (err) { - // The readers understand the old shape, so nothing is broken and the next command tries - // again. Still said out loud, because failing on every run should not be invisible. + // Not rethrown: the migration must not abort the command, which fails at the auth step. cliDebugPrint('auth-file', 'auth file migration failed', err); warning({ - message: `Your login still works, but ${AUTH_FILE_PATH()} could not be updated to the current format. Set APIFY_CLI_DEBUG=1 to see why.`, + message: `Your stored login cannot be read until ${AUTH_FILE_PATH()} is updated to the current format, and the update failed. Make the file and the directory it is in writable, then run the command again. Set APIFY_CLI_DEBUG=1 to see why.`, }); } })(); diff --git a/test/local/lib/auth-file.test.ts b/test/local/lib/auth-file.test.ts index 4c3980196..637dbbbd2 100644 --- a/test/local/lib/auth-file.test.ts +++ b/test/local/lib/auth-file.test.ts @@ -181,19 +181,24 @@ describe('auth.json v2', () => { }); // The failure path had no cover: the whole migration sits in one try/catch. - it.skipIf(process.platform === 'win32')('says so when it cannot write, and still logs you in', async () => { - write(v1AuthFile({ secretsBackend: 'file' })); - chmodSync(GLOBAL_CONFIGS_FOLDER(), 0o500); - - try { - // The old shape still reads, so the command that triggered this keeps working. - await expect(getLocalUserInfo()).resolves.toMatchObject({ id: 'uid', username: 'me' }); - expect(lastErrorMessage()).toContain('Your login still works'); - expect(readAuthFile().version).toBeUndefined(); - } finally { - chmodSync(GLOBAL_CONFIGS_FOLDER(), 0o700); - } - }); + it.skipIf(process.platform === 'win32')( + 'says the stored login cannot be read when it cannot write, and hands back no token', + async () => { + write(v1AuthFile({ secretsBackend: 'file' })); + chmodSync(GLOBAL_CONFIGS_FOLDER(), 0o500); + + try { + const info = await getLocalUserInfo(); + + expect(info).toMatchObject({ id: 'uid', username: 'me' }); + expect(info).not.toHaveProperty('token'); + expect(lastErrorMessage()).toContain('Your stored login cannot be read'); + expect(readAuthFile().version).toBeUndefined(); + } finally { + chmodSync(GLOBAL_CONFIGS_FOLDER(), 0o700); + } + }, + ); it('does nothing when there is no file', async () => { await ensureAuthFileCurrent(); From a1511d0094d4e311d94fe1de75378d704af4c8c3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Richard=20Sol=C3=A1r?= Date: Thu, 24 Sep 2026 18:23:47 +0200 Subject: [PATCH 5/5] fix: point the migration failure at the directory, not the file auth.json is replaced through a temp file and a rename, so the write needs the directory to be writable and the file's own mode never matters. Telling the user to make the file writable sends them to change something that has no effect. Co-Authored-By: Claude Opus 5 --- src/lib/auth-file.ts | 2 +- test/local/lib/auth-file.test.ts | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/src/lib/auth-file.ts b/src/lib/auth-file.ts index e39b033f9..88bc59d92 100644 --- a/src/lib/auth-file.ts +++ b/src/lib/auth-file.ts @@ -200,7 +200,7 @@ async function migrateAuthFile(): Promise { // Not rethrown: the migration must not abort the command, which fails at the auth step. cliDebugPrint('auth-file', 'auth file migration failed', err); warning({ - message: `Your stored login cannot be read until ${AUTH_FILE_PATH()} is updated to the current format, and the update failed. Make the file and the directory it is in writable, then run the command again. Set APIFY_CLI_DEBUG=1 to see why.`, + message: `Your stored login cannot be read until ${AUTH_FILE_PATH()} is updated to the current format, and the update failed. Make the directory it is in writable, then run the command again. Set APIFY_CLI_DEBUG=1 to see why.`, }); } })(); diff --git a/test/local/lib/auth-file.test.ts b/test/local/lib/auth-file.test.ts index 637dbbbd2..ec1029357 100644 --- a/test/local/lib/auth-file.test.ts +++ b/test/local/lib/auth-file.test.ts @@ -193,6 +193,8 @@ describe('auth.json v2', () => { expect(info).toMatchObject({ id: 'uid', username: 'me' }); expect(info).not.toHaveProperty('token'); expect(lastErrorMessage()).toContain('Your stored login cannot be read'); + // The write goes through a temp file and a rename, so the directory is what must be writable. + expect(lastErrorMessage()).toContain('Make the directory it is in writable'); expect(readAuthFile().version).toBeUndefined(); } finally { chmodSync(GLOBAL_CONFIGS_FOLDER(), 0o700);