From 3cc01547f32bdf045bd04beff862412269f75736 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Richard=20Sol=C3=A1r?= Date: Fri, 11 Sep 2026 16:38:37 +0200 Subject: [PATCH 01/33] feat: auth.json v2 with profiles keyed by user ID auth.json was a flat blob describing one account, holding the whole user('me') response. It is now { version, activeProfile, profiles, secretsBackend }, so it can hold N accounts. Nothing puts a second one there yet, and users see no change. New src/lib/auth-file.ts owns the file: reading, an atomic write, the v1 to v2 migration, and the profile accessors. credentials.ts, login, logout, getLocalUserInfo() and the rental notice all go through it. The migration backs the old file up as auth.json.v1.bak, runs after ensureMigrated() as a separate step, is idempotent and single-flight, and never throws. Fields nothing reads are dropped: email, plan, effectivePlatformFeatures, isPaying, createdAt and proxy.groups. Closes #1419 Co-Authored-By: Claude Opus 5 --- src/commands/auth/logout.ts | 6 +- src/lib/auth-file.ts | 247 +++++++++++++++++++ src/lib/auth.ts | 32 ++- src/lib/credentials.ts | 26 +- src/lib/hooks/useRentalSunsetNotice.ts | 22 +- src/lib/types.ts | 1 - src/lib/utils.ts | 46 ++-- test/__setup__/auth-file.ts | 35 +++ test/__setup__/hooks/useAuthSetup.ts | 2 + test/local/commands/auth.test.ts | 40 ++- test/local/commands/run.test.ts | 15 +- test/local/lib/auth-file.test.ts | 255 ++++++++++++++++++++ test/local/lib/auth.test.ts | 9 +- test/local/lib/credentials.test.ts | 14 +- test/local/lib/rental-sunset-notice.test.ts | 12 +- 15 files changed, 651 insertions(+), 111 deletions(-) create mode 100644 src/lib/auth-file.ts create mode 100644 test/__setup__/auth-file.ts create mode 100644 test/local/lib/auth-file.test.ts diff --git a/src/commands/auth/logout.ts b/src/commands/auth/logout.ts index 3ece55890..68f3b4768 100644 --- a/src/commands/auth/logout.ts +++ b/src/commands/auth/logout.ts @@ -1,10 +1,10 @@ import { APIFY_ENV_VARS } from '@apify/consts'; +import { 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 { clearKeyringSecrets } from '../../lib/credentials.js'; -import { rimrafPromised } from '../../lib/files.js'; import { updateUserId } from '../../lib/hooks/telemetry/useTelemetryState.js'; import { success, warning } from '../../lib/outputs.js'; import { tildify } from '../../lib/utils.js'; @@ -28,8 +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. + removeActiveProfile(); await clearKeyringSecrets(); - await rimrafPromised(AUTH_FILE_PATH()); await updateUserId(null); diff --git a/src/lib/auth-file.ts b/src/lib/auth-file.ts new file mode 100644 index 000000000..f74625c3f --- /dev/null +++ b/src/lib/auth-file.ts @@ -0,0 +1,247 @@ +import { copyFileSync, existsSync, readFileSync, renameSync, rmSync, writeFileSync } from 'node:fs'; + +import { cryptoRandomObjectId } from '@apify/utilities'; + +import { AUTH_FILE_PATH } from './consts.js'; +import type { CredentialsBackend } from './credentials.js'; +import { ensureApifyDirectory } from './files.js'; +import { cliDebugPrint } from './utils/cliDebugPrint.js'; + +const AUTH_FILE_VERSION = 2; + +/** The way back to a CLI that only reads the v1 shape. */ +export const AUTH_BACKUP_FILE_PATH = () => `${AUTH_FILE_PATH()}.v1.bak`; + +/** + * One account. Keyed by user ID in {@link AuthFile.profiles}, so renaming a profile can never + * orphan the secret that key names. + */ +export interface AuthProfile { + username?: string; + /** Human label for `--profile `. Unused until profiles get names. */ + name: string | null; + /** Set means the profile is an organization rather than a personal account. */ + organizationOwnerUserId?: string; + /** How the token was obtained. Unused until the device flow lands. */ + authMethod: 'token'; + /** When the access token expires. Unused until the device flow lands. */ + expiresAt: string | null; + /** Whether a refresh token came with the access token. Unused until the device flow lands. */ + hasRefreshToken: boolean; +} + +/** + * `auth.json` as it sits on disk. `token` and `proxy` are the file backend's secret storage; they + * stay outside the profiles until each profile gets its own keys. + */ +export interface AuthFile { + version?: number; + activeProfile?: string; + profiles?: Record; + secretsBackend?: CredentialsBackend; + token?: string; + proxy?: { password?: string; [k: string]: unknown }; + [k: string]: unknown; +} + +export interface ActiveProfileLookup { + profile?: AuthProfile & { id: string }; + /** Set when `activeProfile` names a profile the file does not contain. */ + missingProfile?: string; +} + +let migrationPromise: Promise | undefined; + +/** Test-only: let each test run the v2 migration again. */ +export function __resetAuthFileForTests() { + migrationPromise = undefined; +} + +/** `null` tells a corrupt file from an absent one, which the migration must not overwrite. */ +function parseAuthFile(): AuthFile | null { + if (!existsSync(AUTH_FILE_PATH())) return {}; + + try { + return JSON.parse(readFileSync(AUTH_FILE_PATH(), 'utf-8')) as AuthFile; + } catch { + return null; + } +} + +/** The parsed file, or an empty object when it is missing or unreadable. */ +export function readAuthFile(): AuthFile { + return parseAuthFile() ?? {}; +} + +/** + * Atomic write: a temp file next to the target, then a rename. Two CLI processes can run at once, + * and a half-written auth.json reads as logged out. + */ +export function writeAuthFile(data: AuthFile) { + const path = AUTH_FILE_PATH(); + ensureApifyDirectory(path); + + const tempPath = `${path}.tmp-${cryptoRandomObjectId(8)}`; + + try { + writeFileSync(tempPath, JSON.stringify(data, null, '\t'), { mode: 0o600 }); + renameSync(tempPath, path); + } catch (err) { + rmSync(tempPath, { force: true }); + throw err; + } +} + +/** The one account a v1 file described, as a profile. */ +function v1Profile(file: AuthFile): AuthProfile { + return { + ...(typeof file.username === 'string' ? { username: file.username } : {}), + name: null, + ...(typeof file.organizationOwnerUserId === 'string' + ? { organizationOwnerUserId: file.organizationOwnerUserId } + : {}), + authMethod: 'token', + expiresAt: null, + hasRefreshToken: false, + }; +} + +/** + * A v1 file described one account, so everything in it belongs to one profile. `email`, `plan`, + * `effectivePlatformFeatures`, `isPaying`, `createdAt` and `proxy.groups` are dropped — nothing in + * the CLI reads them. + */ +function toV2(file: AuthFile): 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. + if (typeof file.id === 'string') { + migrated.activeProfile = file.id; + migrated.profiles![file.id] = v1Profile(file); + } + + if (file.secretsBackend) migrated.secretsBackend = file.secretsBackend; + if (typeof file.token === 'string') migrated.token = file.token; + if (typeof file.proxy?.password === 'string') migrated.proxy = { password: file.proxy.password }; + + return migrated; +} + +/** Never overwrites an existing backup: the first one is the file the user started with. */ +function backUpV1File() { + if (existsSync(AUTH_BACKUP_FILE_PATH())) return; + copyFileSync(AUTH_FILE_PATH(), AUTH_BACKUP_FILE_PATH()); +} + +async function migrateToV2(): Promise { + migrationPromise ??= (async () => { + try { + const file = parseAuthFile(); + + // A corrupt file is left alone: readers already treat it as logged out, and rewriting + // it would destroy what the user could still recover by hand. + if (!file) return; + // A numbered version is either already current or from another CLI; either way there + // is nothing to migrate. `assertSupportedAuthFileVersion` reports a newer one. + if (typeof file.version === 'number') return; + if (Object.keys(file).length === 0) return; + + backUpV1File(); + writeAuthFile(toV2(file)); + } catch (err) { + cliDebugPrint('auth-file', 'migration to v2 failed', err); + } + })(); + + return migrationPromise; +} + +/** + * A file from a newer CLI is not something to guess at — migrating it backwards would drop + * whatever that version stores. + */ +function assertSupportedAuthFileVersion() { + const { version } = readAuthFile(); + + if (typeof version === 'number' && version > AUTH_FILE_VERSION) { + throw new Error( + `Your credentials in ${AUTH_FILE_PATH()} were written by a newer Apify CLI (auth file version ${version}, this one reads ${AUTH_FILE_VERSION}). Upgrade the CLI to use them.`, + ); + } +} + +/** + * Brings `auth.json` to the v2 profile shape and refuses a file a newer CLI wrote. Runs after + * `ensureMigrated()`, which moves v1 secrets into the keyring; the two steps stay separate so a + * keyring failure and a shape failure cannot mask each other. + * + * The migration itself is idempotent, single-flight and never throws — it must not block a command. + */ +export async function ensureAuthFileCurrent(): Promise { + await migrateToV2(); + assertSupportedAuthFileVersion(); +} + +/** + * The active profile with its user ID. Reads a v1 file too, so a command that runs before the + * migration still finds the account. + */ +export function lookUpActiveProfile(): ActiveProfileLookup { + const file = readAuthFile(); + + if (file.version !== AUTH_FILE_VERSION) { + return typeof file.id === 'string' ? { profile: { id: file.id, ...v1Profile(file) } } : {}; + } + + if (!file.activeProfile) return {}; + + const profile = file.profiles?.[file.activeProfile]; + if (!profile) return { missingProfile: file.activeProfile }; + + return { profile: { id: file.activeProfile, ...profile } }; +} + +/** The active profile, or `undefined` when nothing usable is stored. */ +export function getActiveProfile(): (AuthProfile & { id: string }) | undefined { + return lookUpActiveProfile().profile; +} + +/** + * Stores one account and makes it active, replacing whatever was there. Nothing puts a second + * profile in the file yet, so `apify login` owns all of it. + */ +export function setActiveProfile(userId: string, profile: AuthProfile, secretsBackend: CredentialsBackend) { + assertSupportedAuthFileVersion(); + + writeAuthFile({ + version: AUTH_FILE_VERSION, + activeProfile: userId, + profiles: { [userId]: profile }, + secretsBackend, + }); +} + +/** + * Drops the active profile together with the secrets stored beside it. The file and the v1 backup + * go away once no profile is left, so logging out leaves no token on disk. + */ +export function removeActiveProfile() { + assertSupportedAuthFileVersion(); + + const file = readAuthFile(); + const active = file.version === AUTH_FILE_VERSION ? file.activeProfile : undefined; + + if (active && file.profiles) delete file.profiles[active]; + delete file.activeProfile; + delete file.token; + delete file.proxy; + + if (Object.keys(file.profiles ?? {}).length === 0) { + rmSync(AUTH_FILE_PATH(), { force: true }); + rmSync(AUTH_BACKUP_FILE_PATH(), { force: true }); + return; + } + + writeAuthFile(file); +} diff --git a/src/lib/auth.ts b/src/lib/auth.ts index b2793e193..6dd005e7b 100644 --- a/src/lib/auth.ts +++ b/src/lib/auth.ts @@ -1,4 +1,4 @@ -import { existsSync, writeFileSync } from 'node:fs'; +import { existsSync } from 'node:fs'; import process from 'node:process'; import { ApifyApiError, ApifyClient, type ApifyClientOptions } from 'apify-client'; @@ -6,6 +6,7 @@ import { AxiosHeaders } from 'axios'; import { APIFY_ENV_VARS } from '@apify/consts'; +import { ensureAuthFileCurrent, setActiveProfile } from './auth-file.js'; import { APIFY_CLIENT_DEFAULT_HEADERS, AUTH_FILE_PATH, CommandExitCodes } from './consts.js'; import { deleteProxyPassword, @@ -14,9 +15,7 @@ import { getToken, setProxyPassword, setToken, - stripProxyPassword, } from './credentials.js'; -import { ensureApifyDirectory } from './files.js'; import { warning } from './outputs.js'; import type { AuthJSON } from './types.js'; import { cliDebugPrint } from './utils/cliDebugPrint.js'; @@ -81,6 +80,7 @@ export function __resetAuthForTests() { export const resolveAuth = async (): Promise => { authPromise ??= (async () => { await ensureMigrated(); + await ensureAuthFileCurrent(); const envToken = readEnvToken(); if (envToken.kind === 'invalid') { @@ -168,15 +168,27 @@ export async function loginWithToken( return null; } - const proxyPassword = userInfo.proxy?.password; + if (!userInfo.id) { + throw new Error('The Apify API returned no user ID for this token, so the login cannot be stored.'); + } - // Replaces the previous account rather than merging, so stale fields cannot linger. The spread - // is shallow, so stripping here also clears userInfo.proxy — read the password first. - const fileContents = { ...userInfo, secretsBackend: await getBackend() }; - stripProxyPassword(fileContents); + const proxyPassword = userInfo.proxy?.password; - ensureApifyDirectory(AUTH_FILE_PATH()); - writeFileSync(AUTH_FILE_PATH(), JSON.stringify(fileContents, null, '\t'), { mode: 0o600 }); + // The profile is keyed by user ID, and it replaces whatever was stored rather than merging + // into it, so fields the new account does not have cannot linger from the old one. + const { organizationOwnerUserId } = userInfo as { organizationOwnerUserId?: string }; + setActiveProfile( + userInfo.id, + { + username: userInfo.username, + name: null, + ...(organizationOwnerUserId ? { organizationOwnerUserId } : {}), + authMethod: 'token', + expiresAt: null, + hasRefreshToken: false, + }, + await getBackend(), + ); // After the metadata file, which would clobber them on the file backend. `skipIfUnchanged` avoids a Keychain prompt. await setToken(token, { skipIfUnchanged: true }); diff --git a/src/lib/credentials.ts b/src/lib/credentials.ts index 3758cd502..c6e5ff1d6 100644 --- a/src/lib/credentials.ts +++ b/src/lib/credentials.ts @@ -1,8 +1,6 @@ -import { existsSync, readFileSync, writeFileSync } from 'node:fs'; import process from 'node:process'; -import { AUTH_FILE_PATH } from './consts.js'; -import { ensureApifyDirectory } from './files.js'; +import { readAuthFile, writeAuthFile } from './auth-file.js'; import { useCLIMetadata } from './hooks/useCLIMetadata.js'; import { cliDebugPrint } from './utils/cliDebugPrint.js'; @@ -22,13 +20,6 @@ interface KeyringModule { Entry: new (service: string, account: string) => KeyringEntry; } -interface StoredAuthFile { - token?: string; - proxy?: { password?: string; [k: string]: unknown }; - secretsBackend?: CredentialsBackend; - [k: string]: unknown; -} - let cachedKeyringModule: KeyringModule | null | undefined; let backendPromise: Promise | undefined; let migrationPromise: Promise | undefined; @@ -104,16 +95,6 @@ function downgradeBackendToFile() { backendPromise = Promise.resolve('file'); } -function readAuthFile(): StoredAuthFile { - if (!existsSync(AUTH_FILE_PATH())) return {}; - try { - const raw = readFileSync(AUTH_FILE_PATH(), 'utf-8'); - return JSON.parse(raw) as StoredAuthFile; - } catch { - return {}; - } -} - /** * Remove the proxy password, keeping any sibling field like `groups` and dropping `proxy` * entirely when the secret was all it carried. @@ -125,11 +106,6 @@ export function stripProxyPassword(data: { proxy?: { password?: string } }) { if (Object.keys(data.proxy).length === 0) delete data.proxy; } -function writeAuthFile(data: StoredAuthFile) { - ensureApifyDirectory(AUTH_FILE_PATH()); - writeFileSync(AUTH_FILE_PATH(), JSON.stringify(data, null, '\t'), { mode: 0o600 }); -} - async function getKeyringEntry(account: string): Promise { const mod = await loadKeyringModule(); if (!mod) return null; diff --git a/src/lib/hooks/useRentalSunsetNotice.ts b/src/lib/hooks/useRentalSunsetNotice.ts index 9d2467b74..57d3dea41 100644 --- a/src/lib/hooks/useRentalSunsetNotice.ts +++ b/src/lib/hooks/useRentalSunsetNotice.ts @@ -1,19 +1,17 @@ -import { readFile } from 'node:fs/promises'; import process from 'node:process'; import axios from 'axios'; import chalk from 'chalk'; import { isCI } from 'ci-info'; +import { getActiveProfile } from '../auth-file.js'; import { APIFY_CLIENT_DEFAULT_HEADERS, - AUTH_FILE_PATH, CHECK_RENTAL_ACTORS_EVERY_MILLIS, RENTAL_SUNSET_NOTICE_EVERY_MILLIS, RENTAL_SUNSET_NOTICE_UNTIL, } from '../consts.js'; import { simpleLog, warning } from '../outputs.js'; -import type { AuthJSON } from '../types.js'; import { cliDebugPrint } from '../utils/cliDebugPrint.js'; import { useCLIMetadata } from './useCLIMetadata.js'; import { type LatestState, updateLocalState, useLocalState } from './useLocalState.js'; @@ -92,18 +90,12 @@ export function renderRentalSunsetNotice(rentalActorCount: number) { } /** - * Reads the logged in username straight from auth.json instead of going through `getLocalUserInfo`, - * which resolves the token from the OS keyring and would trigger a keychain prompt on commands that - * do not need authentication at all. + * Reads the username straight out of auth.json instead of going through `getLocalUserInfo`, which + * resolves the token from the OS keyring and would trigger a keychain prompt on commands that do + * not need authentication at all. */ -async function getLocalUsername() { - try { - const raw = await readFile(AUTH_FILE_PATH(), 'utf-8'); - - return (JSON.parse(raw) as AuthJSON).username; - } catch { - return undefined; - } +function getLocalUsername() { + return getActiveProfile()?.username; } /** @@ -207,7 +199,7 @@ export async function useRentalSunsetNotice() { return; } - const username = await getLocalUsername(); + const username = getLocalUsername(); if (!username) { cliDebugPrint('useRentalSunsetNotice', 'Not logged in, skipping the check'); diff --git a/src/lib/types.ts b/src/lib/types.ts index f639688ed..cbb83cd7d 100644 --- a/src/lib/types.ts +++ b/src/lib/types.ts @@ -4,7 +4,6 @@ export interface AuthJSON { token?: string; id?: string; username?: string; - email?: string; proxy?: { password: string; }; diff --git a/src/lib/utils.ts b/src/lib/utils.ts index 0bdfea073..74d47b873 100644 --- a/src/lib/utils.ts +++ b/src/lib/utils.ts @@ -32,6 +32,7 @@ import { SOURCE_FILE_FORMATS, } from '@apify/consts'; +import { ensureAuthFileCurrent, lookUpActiveProfile } from './auth-file.js'; import { describeAuthFailure, getApifyClientOptionsForToken, resolveAuth, type ResolvedAuth } from './auth.js'; import { AUTH_FILE_PATH, @@ -41,7 +42,7 @@ import { MINIMUM_SUPPORTED_PYTHON_VERSION, SUPPORTED_NODEJS_VERSION, } from './consts.js'; -import { ensureMigrated, getBackend, getProxyPassword, getToken } from './credentials.js'; +import { ensureMigrated, getProxyPassword, getToken } 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'; @@ -85,33 +86,38 @@ export const getLocalRequestQueuePath = (storeId?: string) => { }; /** - * Returns object from auth file or empty object. Secrets (token, proxy password) are - * pulled from the keyring when that backend is active; user metadata lives in auth.json. + * The active profile in the flat shape the CLI consumes, or an empty object when nothing is + * stored. Secrets come from whichever backend holds them; the metadata comes from auth.json. */ export const getLocalUserInfo = async (): Promise => { await ensureMigrated(); + await ensureAuthFileCurrent(); - let result: AuthJSON = {}; - try { - const raw = await readFile(AUTH_FILE_PATH(), 'utf-8'); - result = JSON.parse(raw) as AuthJSON; - } catch { - // auth.json may not exist yet (fresh keyring-only state); fall through + 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; } - if ((await getBackend()) === 'keyring') { - const token = await getToken(); - if (token) result.token = token; + const token = await getToken(); + if (token) result.token = token; - const proxyPassword = await getProxyPassword(); - if (proxyPassword) result.proxy = { ...result.proxy, password: proxyPassword }; - } + const proxyPassword = await getProxyPassword(); + if (proxyPassword) result.proxy = { password: proxyPassword }; - const hasUserMetadata = !!(result.username || result.id); - const isComplete = hasUserMetadata || !!result.token; - if (!isComplete) return {}; - if (!hasUserMetadata) { - throw new Error('Stale credentials found without user metadata. Please run "apify login" again.'); + // A token with no profile behind it is reported rather than swallowed: the commands that build + // `/` lookups would otherwise fail with a misleading "not found". + if (!profile) { + if (!result.token) return {}; + + 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.', + ); } return result; diff --git a/test/__setup__/auth-file.ts b/test/__setup__/auth-file.ts new file mode 100644 index 000000000..6e617345d --- /dev/null +++ b/test/__setup__/auth-file.ts @@ -0,0 +1,35 @@ +/** Reading `auth.json` in tests, so no test has to know the profile shape by hand. */ + +import { readFileSync } from 'node:fs'; + +import type { AuthFile, AuthProfile } from '../../src/lib/auth-file.js'; +import { AUTH_FILE_PATH } from '../../src/lib/consts.js'; + +/** 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; +} + +/** The active profile with its user ID, read straight off disk rather than through the CLI. */ +export function readActiveProfile(): (AuthProfile & { id: string }) | undefined { + const { activeProfile, profiles } = readAuthFile(); + if (!activeProfile) return undefined; + + const profile = profiles?.[activeProfile]; + return profile ? { id: activeProfile, ...profile } : undefined; +} + +/** A v1 `auth.json`, the shape every CLI before the profile migration wrote. */ +export function v1AuthFile(overrides: Record = {}) { + return { + id: 'uid', + username: 'me', + email: 'me@example.com', + token: 'apify_api_v1_token', + proxy: { password: 'pw', groups: [{ name: 'g' }] }, + plan: { id: 'FREE' }, + isPaying: false, + createdAt: '2021-03-27T22:27:56.809Z', + ...overrides, + }; +} diff --git a/test/__setup__/hooks/useAuthSetup.ts b/test/__setup__/hooks/useAuthSetup.ts index e43d1a153..932e6d26b 100644 --- a/test/__setup__/hooks/useAuthSetup.ts +++ b/test/__setup__/hooks/useAuthSetup.ts @@ -6,6 +6,7 @@ import { isCI } from 'ci-info'; import { cryptoRandomObjectId } from '@apify/utilities'; import { LoginCommand } from '../../../src/commands/login.js'; +import { __resetAuthFileForTests } from '../../../src/lib/auth-file.js'; import { __resetAuthForTests } from '../../../src/lib/auth.js'; import { testRunCommand } from '../../../src/lib/command-framework/apify-command.js'; import { GLOBAL_CONFIGS_FOLDER } from '../../../src/lib/consts.js'; @@ -20,6 +21,7 @@ function resetAuthCaches() { __resetCredentialsForTests(); __resetUserInfoCacheForTests(); __resetAuthForTests(); + __resetAuthFileForTests(); } export interface UseAuthSetupOptions { diff --git a/test/local/commands/auth.test.ts b/test/local/commands/auth.test.ts index 02f24375f..681f1ab89 100644 --- a/test/local/commands/auth.test.ts +++ b/test/local/commands/auth.test.ts @@ -1,9 +1,10 @@ -import { existsSync, readFileSync, statSync } from 'node:fs'; +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 { 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 { @@ -31,7 +32,6 @@ const { testRunCommand } = await import('../../../src/lib/command-framework/apif const TOKEN = 'apify_api_test_token'; -const readAuthFile = () => JSON.parse(readFileSync(AUTH_FILE_PATH(), 'utf-8')); const login = (token = TOKEN) => testRunCommand(AuthLoginCommand, { flags_token: token }); describe('auth commands', () => { @@ -41,14 +41,17 @@ describe('auth commands', () => { }); describe('file backend', () => { - it('login stores the token and user metadata in auth.json', async () => { + it('login stores the token and one profile keyed by user ID', async () => { await login(); - expect(readAuthFile()).toMatchObject({ - token: TOKEN, + expect(readAuthFile()).toMatchObject({ version: 2, token: TOKEN, secretsBackend: 'file' }); + expect(readActiveProfile()).toEqual({ id: 'uid', username: 'me', - secretsBackend: 'file', + name: null, + authMethod: 'token', + expiresAt: null, + hasRefreshToken: false, }); expect(lastErrorMessage()).toContain('You are logged in to Apify as me'); }); @@ -74,7 +77,7 @@ describe('auth commands', () => { expect(await getToken()).toBeUndefined(); }); - it('logging in as another account replaces the stored metadata', async () => { + it('logging in as another account replaces the stored profile', async () => { clientState.user = { id: 'uid', username: 'me', email: 'me@example.com' }; await login(); @@ -82,9 +85,10 @@ describe('auth commands', () => { await login('apify_api_other_token'); const authFile = readAuthFile(); - expect(authFile).toMatchObject({ token: 'apify_api_other_token', id: 'uid2', username: 'other' }); - // The new account has no email, so the old one must not linger. - expect(authFile.email).toBeUndefined(); + expect(authFile).toMatchObject({ activeProfile: 'uid2', token: 'apify_api_other_token' }); + // 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' }); }); it('login with an invalid token stores nothing and fails the command', async () => { @@ -170,7 +174,7 @@ describe('auth commands', () => { expect(lastLogMessage()).toBe('apify_api_env_token'); expect(await getToken()).toBe(TOKEN); - expect(readAuthFile()).toMatchObject({ username: 'me' }); + expect(readActiveProfile()).toMatchObject({ username: 'me' }); }); }); @@ -184,17 +188,11 @@ describe('auth commands', () => { expect(keyringStore.get(KEYRING_PROXY_PASSWORD_KEY)).toBe('pw'); const authFile = readAuthFile(); - expect(authFile).toMatchObject({ id: 'uid', username: 'me', secretsBackend: 'keyring' }); + expect(authFile).toMatchObject({ version: 2, secretsBackend: 'keyring' }); expect(authFile.token).toBeUndefined(); - expect(authFile.proxy).toEqual({ groups: [{ name: 'g' }] }); - }); - - it('login drops the proxy object from auth.json when it only held the password', async () => { - clientState.user.proxy = { password: 'pw' }; - await login(); - - expect(readAuthFile()).not.toHaveProperty('proxy'); - expect(keyringStore.get(KEYRING_PROXY_PASSWORD_KEY)).toBe('pw'); + // Proxy groups are not a secret, but nothing reads them either. + expect(authFile).not.toHaveProperty('proxy'); + expect(readActiveProfile()).toMatchObject({ id: 'uid', username: 'me' }); }); it('logging in as an account with no proxy password forgets the previous one', async () => { diff --git a/test/local/commands/run.test.ts b/test/local/commands/run.test.ts index 75c1e141b..edb48ded2 100644 --- a/test/local/commands/run.test.ts +++ b/test/local/commands/run.test.ts @@ -4,13 +4,14 @@ import { dirname } from 'node:path'; import { ACTOR_ENV_VARS, APIFY_ENV_VARS } from '@apify/consts'; import { testRunCommand } from '../../../src/lib/command-framework/apify-command.js'; -import { AUTH_FILE_PATH, EMPTY_LOCAL_CONFIG, LOCAL_CONFIG_PATH } from '../../../src/lib/consts.js'; +import { EMPTY_LOCAL_CONFIG, LOCAL_CONFIG_PATH } from '../../../src/lib/consts.js'; import { rimrafPromised } from '../../../src/lib/files.js'; import { getLocalDatasetPath, getLocalKeyValueStorePath, getLocalRequestQueuePath, getLocalStorageDir, + getLocalUserInfo, } from '../../../src/lib/utils.js'; import { TEST_TIMEOUT } from '../../__setup__/consts.js'; import { safeLogin, useAuthSetup } from '../../__setup__/hooks/useAuthSetup.js'; @@ -123,9 +124,9 @@ describe('apify run', () => { const actOutputPath = joinPath(getLocalKeyValueStorePath(), 'OUTPUT.json'); const localEnvVars = JSON.parse(readFileSync(actOutputPath, 'utf8')); - const auth = JSON.parse(readFileSync(AUTH_FILE_PATH(), 'utf8')); + const auth = await getLocalUserInfo(); - expect(localEnvVars[APIFY_ENV_VARS.PROXY_PASSWORD]).toStrictEqual(auth.proxy.password); + expect(localEnvVars[APIFY_ENV_VARS.PROXY_PASSWORD]).toStrictEqual(auth.proxy!.password); expect(localEnvVars[APIFY_ENV_VARS.USER_ID]).toStrictEqual(auth.id); expect(localEnvVars[APIFY_ENV_VARS.TOKEN]).toStrictEqual(auth.token); expect(localEnvVars.TEST_LOCAL).toStrictEqual(testEnvVars.TEST_LOCAL); @@ -164,9 +165,9 @@ describe('apify run', () => { const actOutputPath = joinPath(getLocalKeyValueStorePath(), 'OUTPUT.json'); const localEnvVars = JSON.parse(readFileSync(actOutputPath, 'utf8')); - const auth = JSON.parse(readFileSync(AUTH_FILE_PATH(), 'utf8')); + const auth = await getLocalUserInfo(); - expect(localEnvVars[APIFY_ENV_VARS.PROXY_PASSWORD]).toStrictEqual(auth.proxy.password); + expect(localEnvVars[APIFY_ENV_VARS.PROXY_PASSWORD]).toStrictEqual(auth.proxy!.password); expect(localEnvVars[APIFY_ENV_VARS.USER_ID]).toStrictEqual(auth.id); expect(localEnvVars[APIFY_ENV_VARS.TOKEN]).toStrictEqual(auth.token); expect(localEnvVars.TEST_LOCAL).toStrictEqual(testEnvVars.TEST_LOCAL); @@ -204,9 +205,9 @@ describe('apify run', () => { const actOutputPath = joinPath(getLocalKeyValueStorePath(), 'OUTPUT.json'); const localEnvVars = JSON.parse(readFileSync(actOutputPath, 'utf8')); - const auth = JSON.parse(readFileSync(AUTH_FILE_PATH(), 'utf8')); + const auth = await getLocalUserInfo(); - expect(localEnvVars[APIFY_ENV_VARS.PROXY_PASSWORD]).toStrictEqual(auth.proxy.password); + expect(localEnvVars[APIFY_ENV_VARS.PROXY_PASSWORD]).toStrictEqual(auth.proxy!.password); expect(localEnvVars[APIFY_ENV_VARS.USER_ID]).toStrictEqual(auth.id); expect(localEnvVars[APIFY_ENV_VARS.TOKEN]).toStrictEqual(auth.token); expect(localEnvVars.TEST_LOCAL).toStrictEqual(testEnvVars.TEST_LOCAL); diff --git a/test/local/lib/auth-file.test.ts b/test/local/lib/auth-file.test.ts new file mode 100644 index 000000000..8afc82c81 --- /dev/null +++ b/test/local/lib/auth-file.test.ts @@ -0,0 +1,255 @@ +import { existsSync, mkdirSync, readFileSync, writeFileSync } from 'node:fs'; + +import { + __resetAuthFileForTests, + AUTH_BACKUP_FILE_PATH, + type AuthProfile, + ensureAuthFileCurrent, + getActiveProfile, + lookUpActiveProfile, + removeActiveProfile, + setActiveProfile, +} from '../../../src/lib/auth-file.js'; +import { AUTH_FILE_PATH, GLOBAL_CONFIGS_FOLDER } from '../../../src/lib/consts.js'; +import { ensureMigrated, getProxyPassword, getToken } 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 { + KEYRING_PROXY_PASSWORD_KEY, + KEYRING_TOKEN_KEY, + keyringStore, + resetKeyringMock, +} from '../../__setup__/keyring-mock.js'; + +vi.mock('@napi-rs/keyring', () => import('../../__setup__/keyring-mock.js')); + +useAuthSetup(); + +const write = (contents: unknown) => { + mkdirSync(GLOBAL_CONFIGS_FOLDER(), { recursive: true }); + writeFileSync(AUTH_FILE_PATH(), typeof contents === 'string' ? contents : JSON.stringify(contents)); +}; + +const readBackup = () => JSON.parse(readFileSync(AUTH_BACKUP_FILE_PATH(), 'utf-8')); + +const V2_PROFILE: AuthProfile = { + username: 'me', + name: null, + authMethod: 'token', + expiresAt: null, + hasRefreshToken: false, +}; + +const V1_PROFILE = { id: 'uid', ...V2_PROFILE }; + +describe('auth.json v2', () => { + beforeEach(() => { + resetKeyringMock(); + }); + + describe('migration', () => { + // State A in the wild: plaintext secrets and no backend marker, written before the keyring. + it('migrates state A, after ensureMigrated() has stamped the marker', async () => { + write(v1AuthFile()); + + await ensureMigrated(); + await ensureAuthFileCurrent(); + + expect(readAuthFile()).toEqual({ + version: 2, + activeProfile: 'uid', + profiles: { uid: V2_PROFILE }, + secretsBackend: 'file', + token: 'apify_api_v1_token', + proxy: { password: 'pw' }, + }); + }); + + // State C: plaintext secrets with the file marker already on them. + it('migrates state C and keeps the secrets in the file', async () => { + write(v1AuthFile({ secretsBackend: 'file' })); + + 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'); + }); + + it('drops the fields nothing in the CLI reads', async () => { + write(v1AuthFile({ secretsBackend: 'file' })); + + await ensureAuthFileCurrent(); + + const file = readAuthFile(); + for (const key of ['email', 'plan', 'isPaying', 'createdAt', 'id', 'username']) { + expect(file).not.toHaveProperty(key); + } + expect(file.proxy).toEqual({ password: 'pw' }); + }); + + it('carries organizationOwnerUserId into the profile', async () => { + write(v1AuthFile({ secretsBackend: 'file', organizationOwnerUserId: 'owner-id' })); + + await ensureAuthFileCurrent(); + + expect(readActiveProfile()).toMatchObject({ organizationOwnerUserId: 'owner-id' }); + }); + + it('backs the v1 file up and never overwrites the backup', async () => { + write(v1AuthFile({ secretsBackend: 'file' })); + + await ensureAuthFileCurrent(); + expect(readBackup()).toMatchObject({ id: 'uid', email: 'me@example.com' }); + + // A later process migrating another v1 file must leave the first backup alone. + write(v1AuthFile({ secretsBackend: 'file', username: 'someone-else' })); + __resetAuthFileForTests(); + await ensureAuthFileCurrent(); + + expect(readActiveProfile()).toMatchObject({ username: 'someone-else' }); + expect(readBackup()).toMatchObject({ username: 'me' }); + }); + + it('is a no-op on a file that is already v2', async () => { + write(v1AuthFile({ secretsBackend: 'file' })); + await ensureAuthFileCurrent(); + const migrated = readAuthFile(); + + await ensureAuthFileCurrent(); + + expect(readAuthFile()).toEqual(migrated); + }); + + it('does nothing when there is no file', async () => { + await ensureAuthFileCurrent(); + + expect(existsSync(AUTH_FILE_PATH())).toBe(false); + expect(existsSync(AUTH_BACKUP_FILE_PATH())).toBe(false); + }); + + it('leaves a corrupt file alone rather than rewriting it', async () => { + write('{ not json'); + + await ensureAuthFileCurrent(); + + expect(readFileSync(AUTH_FILE_PATH(), 'utf-8')).toBe('{ not json'); + 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 () => { + write({ token: 'apify_api_v1_token', secretsBackend: 'file' }); + + await ensureAuthFileCurrent(); + + expect(readAuthFile()).toEqual({ + version: 2, + profiles: {}, + secretsBackend: 'file', + token: 'apify_api_v1_token', + }); + expect(readBackup()).toEqual({ token: 'apify_api_v1_token', secretsBackend: 'file' }); + await expect(getLocalUserInfo()).rejects.toThrow('Stale credentials found without user metadata'); + }); + }); + + describe('reading the active profile', () => { + it('reads a v1 file that has not been migrated yet', () => { + write(v1AuthFile()); + + expect(getActiveProfile()).toEqual(V1_PROFILE); + }); + + it('returns nothing when no profile is stored', () => { + write({ version: 2, profiles: {} }); + + expect(lookUpActiveProfile()).toEqual({}); + }); + + it('names the profile activeProfile points at when the file does not contain it', () => { + write({ version: 2, activeProfile: 'gone', profiles: {} }); + + expect(lookUpActiveProfile()).toEqual({ missingProfile: 'gone' }); + }); + + it('names the missing profile rather than reporting a silent logged-out state', async () => { + write({ version: 2, activeProfile: 'gone', profiles: {}, secretsBackend: 'file', token: 'tok' }); + + 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 () => { + write({ version: 2, activeProfile: 'gone', profiles: {}, secretsBackend: 'file' }); + + await expect(getLocalUserInfo()).resolves.toEqual({}); + }); + }); + + describe('a file a newer CLI wrote', () => { + it('is refused rather than migrated backwards', async () => { + write({ version: 3, activeProfile: 'uid', profiles: {} }); + + await expect(ensureAuthFileCurrent()).rejects.toThrow('written by a newer Apify CLI'); + }); + + it('is not replaced by a login', () => { + const newer = { version: 3, activeProfile: 'uid', profiles: { uid: { username: 'me' } } }; + write(newer); + + expect(() => setActiveProfile('uid2', V2_PROFILE, 'file')).toThrow('written by a newer Apify CLI'); + expect(readAuthFile()).toEqual(newer); + }); + + it('is not touched by a logout', () => { + const newer = { version: 3, activeProfile: 'uid', profiles: { uid: { username: 'me' } }, token: 'tok' }; + write(newer); + + expect(() => removeActiveProfile()).toThrow('written by a newer Apify CLI'); + expect(readAuthFile()).toEqual(newer); + }); + }); + + describe('keyring backend', () => { + useKeyringBackend(); + + // 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'); + write({ id: 'uid', username: 'me', email: 'me@example.com', secretsBackend: 'keyring' }); + + await ensureMigrated(); + await ensureAuthFileCurrent(); + + expect(readAuthFile()).toEqual({ + version: 2, + activeProfile: 'uid', + profiles: { uid: V2_PROFILE }, + secretsBackend: 'keyring', + }); + expect(await getLocalUserInfo()).toEqual({ + id: 'uid', + username: 'me', + token: 'tok_kr', + proxy: { password: 'pw_kr' }, + }); + }); + + // State A on a machine where the keyring works: ensureMigrated() moves the secrets first. + it('migrates state A to the keyring and then to v2', async () => { + write(v1AuthFile()); + + await ensureMigrated(); + await ensureAuthFileCurrent(); + + expect(keyringStore.get(KEYRING_TOKEN_KEY)).toBe('apify_api_v1_token'); + expect(readAuthFile()).toEqual({ + version: 2, + activeProfile: 'uid', + profiles: { uid: V2_PROFILE }, + secretsBackend: 'keyring', + }); + }); + }); +}); diff --git a/test/local/lib/auth.test.ts b/test/local/lib/auth.test.ts index 62c441b36..0e3f4baf7 100644 --- a/test/local/lib/auth.test.ts +++ b/test/local/lib/auth.test.ts @@ -1,4 +1,4 @@ -import { existsSync, readFileSync } from 'node:fs'; +import { existsSync } from 'node:fs'; import { ApifyApiError } from 'apify-client'; @@ -7,6 +7,7 @@ import { AUTH_FILE_PATH, CommandExitCodes } from '../../../src/lib/consts.js'; import { getProxyPassword, getToken, setToken } 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'; import { useAuthSetup } from '../../__setup__/hooks/useAuthSetup.js'; import { useConsoleSpy } from '../../__setup__/hooks/useConsoleSpy.js'; @@ -21,8 +22,6 @@ const { lastErrorMessage, logMessages } = useConsoleSpy(); const STORED = 'apify_api_stored'; const ENV = 'apify_api_env'; -const readAuthFile = () => JSON.parse(readFileSync(AUTH_FILE_PATH(), 'utf-8')); - // A real ApifyApiError, not a look-alike: describeAuthFailure narrows on the class, so a // hand-built error would let the 401/403 branch rot without failing a test. const apiError = (statusCode: number) => @@ -135,7 +134,7 @@ describe('auth', () => { expect(await getToken()).toBe(STORED); expect(await getProxyPassword()).toBe('pw'); - expect(readAuthFile()).toMatchObject({ id: 'uid', username: 'me' }); + expect(readActiveProfile()).toMatchObject({ id: 'uid', username: 'me' }); }); it('writes nothing when the API rejects the token', async () => { @@ -239,7 +238,7 @@ describe('auth', () => { await resolveAuth(); expect(await getToken()).toBe(STORED); - expect(readAuthFile()).toMatchObject({ username: 'me' }); + expect(readActiveProfile()).toMatchObject({ username: 'me' }); }); }); }); diff --git a/test/local/lib/credentials.test.ts b/test/local/lib/credentials.test.ts index 991cd5a46..bb46f5c63 100644 --- a/test/local/lib/credentials.test.ts +++ b/test/local/lib/credentials.test.ts @@ -4,6 +4,7 @@ import process from 'node:process'; import { cryptoRandomObjectId } from '@apify/utilities'; +import { __resetAuthFileForTests } 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 { @@ -35,7 +36,8 @@ vi.mock('node:fs', async (importOriginal) => { }); const writeFileSyncSpy = vi.mocked(writeFileSync); -const authFileWrites = () => writeFileSyncSpy.mock.calls.filter((call) => call[0] === AUTH_FILE_PATH()); +// auth.json is written through a temp file and a rename, so the spied path carries a suffix. +const authFileWrites = () => writeFileSyncSpy.mock.calls.filter((call) => String(call[0]).startsWith(AUTH_FILE_PATH())); const writeAuthFile = (data: Record) => { mkdirSync(GLOBAL_CONFIGS_FOLDER(), { recursive: true }); @@ -52,12 +54,14 @@ describe('credentials', () => { resetKeyringMock(); writeFileSyncSpy.mockClear(); __resetCredentialsForTests(); + __resetAuthFileForTests(); }); afterEach(async () => { await rm(GLOBAL_CONFIGS_FOLDER(), { recursive: true, force: true }); vitest.unstubAllEnvs(); __resetCredentialsForTests(); + __resetAuthFileForTests(); }); describe('getBackend()', () => { @@ -134,7 +138,9 @@ describe('credentials', () => { it('writes auth.json with mode 0600', async () => { await setToken('tok_123'); - expect(writeFileSyncSpy).toHaveBeenCalledWith(AUTH_FILE_PATH(), expect.any(String), { mode: 0o600 }); + 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 () => { @@ -337,7 +343,7 @@ describe('credentials', () => { }); describe('getLocalUserInfo()', () => { - it('on file backend, preserves non-secret proxy fields', async () => { + it('on file backend, keeps the proxy password and drops the groups nothing reads', async () => { vitest.stubEnv('APIFY_DISABLE_KEYRING', '1'); writeAuthFile({ username: 'me', @@ -347,7 +353,7 @@ describe('credentials', () => { secretsBackend: 'file', }); const info = await getLocalUserInfo(); - expect(info.proxy).toEqual({ password: 'pw', groups: [{ name: 'g' }] }); + expect(info.proxy).toEqual({ password: 'pw' }); }); it('on keyring backend, overlays token and proxy password from keyring', async () => { diff --git a/test/local/lib/rental-sunset-notice.test.ts b/test/local/lib/rental-sunset-notice.test.ts index 9a038adb0..947503ac3 100644 --- a/test/local/lib/rental-sunset-notice.test.ts +++ b/test/local/lib/rental-sunset-notice.test.ts @@ -27,7 +27,17 @@ async function writeAuthFile(username: string | undefined) { const path = AUTH_FILE_PATH(); await mkdir(dirname(path), { recursive: true }); - await writeFile(path, JSON.stringify({ id: 'user-id', username, token: 'apify_api_token' })); + await writeFile( + path, + JSON.stringify({ + version: 2, + activeProfile: 'user-id', + profiles: { + 'user-id': { username, name: null, authMethod: 'token', expiresAt: null, hasRefreshToken: false }, + }, + token: 'apify_api_token', + }), + ); } interface StoredRentalSunset { From f9a75def7869366d5ee27ee415e8d3bb02790e74 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Richard=20Sol=C3=A1r?= Date: Thu, 17 Sep 2026 14:06:34 +0200 Subject: [PATCH 02/33] test: update the API auth tests to the v2 file shape MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both parsed auth.json by hand and asserted the v1 flat shape, so neither could pass against a v2 file. log_in_out deep-equalled the file against the whole user('me') response, which v2 deliberately no longer stores; info read a top-level id that is now the profile key. Both now read the active profile through the test helper, and log_in_out checks the token through getToken() rather than the file. Not run here — test:api needs a live token. Co-Authored-By: Claude Opus 5 --- test/api/commands/info.test.ts | 8 +--- test/api/commands/log_in_out.test.ts | 61 ++++++++-------------------- 2 files changed, 19 insertions(+), 50 deletions(-) diff --git a/test/api/commands/info.test.ts b/test/api/commands/info.test.ts index 5b037f66a..f034397f4 100644 --- a/test/api/commands/info.test.ts +++ b/test/api/commands/info.test.ts @@ -1,8 +1,6 @@ -import { readFileSync } from 'node:fs'; - import { InfoCommand } from '../../../src/commands/info.js'; import { testRunCommand } from '../../../src/lib/command-framework/apify-command.js'; -import { AUTH_FILE_PATH } from '../../../src/lib/consts.js'; +import { readActiveProfile } from '../../__setup__/auth-file.js'; import { safeLogin, useAuthSetup } from '../../__setup__/hooks/useAuthSetup.js'; import { useConsoleSpy } from '../../__setup__/hooks/useConsoleSpy.js'; @@ -21,12 +19,10 @@ describe('[api] apify info', () => { await safeLogin(); await testRunCommand(InfoCommand, {}); - const userInfoFromConfig = JSON.parse(readFileSync(AUTH_FILE_PATH(), 'utf8')); - const spy = logSpy(); expect(spy).toHaveBeenCalledTimes(3); - expect(spy.mock.calls[1][0]).to.include(userInfoFromConfig.id); + expect(spy.mock.calls[1][0]).to.include(readActiveProfile()!.id); expect(spy.mock.calls[2][0]).to.include('apify login'); }); }); diff --git a/test/api/commands/log_in_out.test.ts b/test/api/commands/log_in_out.test.ts index 5ae48d0ed..28969e264 100644 --- a/test/api/commands/log_in_out.test.ts +++ b/test/api/commands/log_in_out.test.ts @@ -1,9 +1,11 @@ -import { existsSync, readFileSync } from 'node:fs'; +import { existsSync } from 'node:fs'; import axios from 'axios'; import { testRunCommand } from '../../../src/lib/command-framework/apify-command.js'; import { AUTH_FILE_PATH } from '../../../src/lib/consts.js'; +import { getToken } from '../../../src/lib/credentials.js'; +import { readActiveProfile } from '../../__setup__/auth-file.js'; import { TEST_USER_BAD_TOKEN, TEST_USER_TOKEN, testUserClient } from '../../__setup__/config.js'; import { safeLogin, useAuthSetup } from '../../__setup__/hooks/useAuthSetup.js'; import { useConsoleSpy } from '../../__setup__/hooks/useConsoleSpy.js'; @@ -31,31 +33,16 @@ describe('[api] apify login and logout', () => { it('should work with correct token', async () => { await safeLogin(TEST_USER_TOKEN); - const expectedUserInfo = Object.assign(await testUserClient.user('me').get(), { - token: TEST_USER_TOKEN, - }) as unknown as Record; - const userInfoFromConfig = JSON.parse(readFileSync(AUTH_FILE_PATH(), 'utf8')); + const expectedUserInfo = await testUserClient.user('me').get(); expect(lastErrorMessage()).to.include('Success:'); - // Omit currentBillingPeriod, It can change during tests - - const { - currentBillingPeriod: _1, - plan: _2, - createdAt: _3, - ...expectedUserInfoWithoutFloatFields - } = expectedUserInfo; - - const { - currentBillingPeriod: _4, - plan: _5, - createdAt: _6, - secretsBackend: _7, - ...userInfoFromConfigWithoutFloatFields - } = userInfoFromConfig; - - expect(expectedUserInfoWithoutFloatFields).to.eql(userInfoFromConfigWithoutFloatFields); + // v2 stores the account as a profile keyed by user ID, not the whole user('me') response. + expect(readActiveProfile()).toMatchObject({ + id: expectedUserInfo.id, + username: expectedUserInfo.username, + }); + expect(await getToken()).to.eql(TEST_USER_TOKEN); await testRunCommand(LogoutCommand, {}); const isGlobalConfig = existsSync(AUTH_FILE_PATH()); @@ -83,29 +70,15 @@ describe('[api] apify login and logout', () => { expect(response.status).to.be.eql(200); - const expectedUserInfo = Object.assign(await testUserClient.user('me').get(), { - token: TEST_USER_TOKEN, - }) as unknown as Record; - const userInfoFromConfig = JSON.parse(readFileSync(AUTH_FILE_PATH(), 'utf8')); + const expectedUserInfo = await testUserClient.user('me').get(); expect(lastErrorMessage()).to.include('Success:'); - // Omit currentBillingPeriod, It can change during tests - - const { - currentBillingPeriod: _1, - plan: _2, - createdAt: _3, - ...expectedUserInfoWithoutFloatFields - } = expectedUserInfo; - const { - currentBillingPeriod: _4, - plan: _5, - createdAt: _6, - secretsBackend: _7, - ...userInfoFromConfigWithoutFloatFields - } = userInfoFromConfig; - - expect(expectedUserInfoWithoutFloatFields).to.eql(userInfoFromConfigWithoutFloatFields); + // v2 stores the account as a profile keyed by user ID, not the whole user('me') response. + expect(readActiveProfile()).toMatchObject({ + id: expectedUserInfo.id, + username: expectedUserInfo.username, + }); + expect(await getToken()).to.eql(TEST_USER_TOKEN); }); }); From 240387c9bc6dfe9faec2a83776b827c850b01435 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Richard=20Sol=C3=A1r?= Date: Thu, 17 Sep 2026 21:00:27 +0200 Subject: [PATCH 03/33] fix: keep the v1 backup readable only by the owner MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit copyFileSync inherits the source mode. An auth.json written before the CLI started passing mode 0600 is still 0644, and writeFileSync's mode applies only on create, so it stayed that way. The new atomic write fixes auth.json on the first v2 write, but the backup is copied before that and never rewritten — leaving a plaintext token at 0644. Also fixes two tests: apify info prints three rows since the token source line landed, and the idempotency check called the migration twice without resetting the memoised promise, so the second call never touched the file. Adds the missing cover for logout removing the backup, which is the only path that erases that token from disk. Co-Authored-By: Claude Opus 5 --- src/lib/auth-file.ts | 11 +++++++++-- test/local/lib/auth-file.test.ts | 25 ++++++++++++++++++++++++- 2 files changed, 33 insertions(+), 3 deletions(-) diff --git a/src/lib/auth-file.ts b/src/lib/auth-file.ts index f74625c3f..09f182f2b 100644 --- a/src/lib/auth-file.ts +++ b/src/lib/auth-file.ts @@ -1,4 +1,4 @@ -import { copyFileSync, existsSync, readFileSync, renameSync, rmSync, writeFileSync } from 'node:fs'; +import { chmodSync, copyFileSync, existsSync, readFileSync, renameSync, rmSync, writeFileSync } from 'node:fs'; import { cryptoRandomObjectId } from '@apify/utilities'; @@ -128,10 +128,17 @@ function toV2(file: AuthFile): AuthFile { return migrated; } -/** Never overwrites an existing backup: the first one is the file the user started with. */ +/** + * Never overwrites an existing backup: the first one is the file the user started with, as it + * stood after `ensureMigrated()` — on the keyring backend that means the secrets are already out + * of it. `copyFileSync` inherits the source mode, and an auth.json written before the CLI set + * 0600 is still 0644, so the mode is re-asserted rather than carried over. + */ function backUpV1File() { if (existsSync(AUTH_BACKUP_FILE_PATH())) return; + copyFileSync(AUTH_FILE_PATH(), AUTH_BACKUP_FILE_PATH()); + chmodSync(AUTH_BACKUP_FILE_PATH(), 0o600); } async function migrateToV2(): Promise { diff --git a/test/local/lib/auth-file.test.ts b/test/local/lib/auth-file.test.ts index 8afc82c81..b095c5f6d 100644 --- a/test/local/lib/auth-file.test.ts +++ b/test/local/lib/auth-file.test.ts @@ -1,4 +1,4 @@ -import { existsSync, mkdirSync, readFileSync, writeFileSync } from 'node:fs'; +import { chmodSync, existsSync, mkdirSync, readFileSync, statSync, writeFileSync } from 'node:fs'; import { __resetAuthFileForTests, @@ -117,11 +117,34 @@ describe('auth.json v2', () => { await ensureAuthFileCurrent(); const migrated = readAuthFile(); + // Without the reset the memoised promise short-circuits and the file is never re-read. + __resetAuthFileForTests(); await ensureAuthFileCurrent(); expect(readAuthFile()).toEqual(migrated); }); + // The only code path that erases the plaintext v1 token from disk. + it('logout removes the backup along with the file', async () => { + write(v1AuthFile({ secretsBackend: 'file' })); + await ensureAuthFileCurrent(); + expect(existsSync(AUTH_BACKUP_FILE_PATH())).toBe(true); + + removeActiveProfile(); + + expect(existsSync(AUTH_FILE_PATH())).toBe(false); + expect(existsSync(AUTH_BACKUP_FILE_PATH())).toBe(false); + }); + + it('writes the backup readable only by the owner, whatever mode the v1 file had', async () => { + write(v1AuthFile({ secretsBackend: 'file' })); + chmodSync(AUTH_FILE_PATH(), 0o644); + + await ensureAuthFileCurrent(); + + expect(statSync(AUTH_BACKUP_FILE_PATH()).mode & 0o777).toBe(0o600); + }); + it('does nothing when there is no file', async () => { await ensureAuthFileCurrent(); From 0452abfa82865db74afed0bad52e883e6e2c1f83 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Richard=20Sol=C3=A1r?= Date: Thu, 17 Sep 2026 22:05:53 +0200 Subject: [PATCH 04/33] fix: keep secrets out of the v1 backup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The backup is written once and never refreshed, and only logout deletes it. So after `apify login` as a second account, auth.json holds the new token while auth.json.v1.bak still holds the previous one — for as long as the user never logs out. Nothing reads the backup, and a downgraded CLI finds its token through the keyring or auth.json rather than here, so the secrets are dropped when writing it. Also pins the two lines that make the migration run for users. Deleting `await ensureAuthFileCurrent()` from either resolveAuth() or getLocalUserInfo() left the whole suite green: every migration test called it by hand. Co-Authored-By: Claude Opus 5 --- src/lib/auth-file.ts | 18 +++++++------- test/local/lib/auth-file.test.ts | 42 +++++++++++++++++++++++++++++++- 2 files changed, 50 insertions(+), 10 deletions(-) diff --git a/src/lib/auth-file.ts b/src/lib/auth-file.ts index 09f182f2b..bc0f492a7 100644 --- a/src/lib/auth-file.ts +++ b/src/lib/auth-file.ts @@ -1,4 +1,4 @@ -import { chmodSync, copyFileSync, existsSync, readFileSync, renameSync, rmSync, writeFileSync } from 'node:fs'; +import { existsSync, readFileSync, renameSync, rmSync, writeFileSync } from 'node:fs'; import { cryptoRandomObjectId } from '@apify/utilities'; @@ -129,16 +129,16 @@ function toV2(file: AuthFile): AuthFile { } /** - * Never overwrites an existing backup: the first one is the file the user started with, as it - * stood after `ensureMigrated()` — on the keyring backend that means the secrets are already out - * of it. `copyFileSync` inherits the source mode, and an auth.json written before the CLI set - * 0600 is still 0644, so the mode is re-asserted rather than carried over. + * A snapshot of the pre-v2 file, kept so an upgrade is inspectable. Written once and never + * refreshed, which is why the secrets are left out: `apify login` replaces auth.json but cannot + * reach this file, so a copy of a rotated token would sit here until the next logout. Nothing + * reads it, and a downgraded CLI finds its token through the usual backends rather than here. */ -function backUpV1File() { +function backUpV1File(file: AuthFile) { if (existsSync(AUTH_BACKUP_FILE_PATH())) return; - copyFileSync(AUTH_FILE_PATH(), AUTH_BACKUP_FILE_PATH()); - chmodSync(AUTH_BACKUP_FILE_PATH(), 0o600); + const { token: _token, proxy: _proxy, ...withoutSecrets } = file; + writeFileSync(AUTH_BACKUP_FILE_PATH(), JSON.stringify(withoutSecrets, null, '\t'), { mode: 0o600 }); } async function migrateToV2(): Promise { @@ -154,7 +154,7 @@ async function migrateToV2(): Promise { if (typeof file.version === 'number') return; if (Object.keys(file).length === 0) return; - backUpV1File(); + backUpV1File(file); writeAuthFile(toV2(file)); } catch (err) { cliDebugPrint('auth-file', 'migration to v2 failed', err); diff --git a/test/local/lib/auth-file.test.ts b/test/local/lib/auth-file.test.ts index b095c5f6d..3c9a7da6c 100644 --- a/test/local/lib/auth-file.test.ts +++ b/test/local/lib/auth-file.test.ts @@ -10,6 +10,7 @@ import { removeActiveProfile, setActiveProfile, } 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 { getLocalUserInfo } from '../../../src/lib/utils.js'; @@ -145,6 +146,18 @@ describe('auth.json v2', () => { expect(statSync(AUTH_BACKUP_FILE_PATH()).mode & 0o777).toBe(0o600); }); + // The backup is never refreshed, so a token in it would outlive the account it belongs to. + it('keeps the secrets out of the backup', async () => { + write(v1AuthFile({ secretsBackend: 'file' })); + + await ensureAuthFileCurrent(); + + const backup = readBackup(); + expect(backup).not.toHaveProperty('token'); + expect(backup).not.toHaveProperty('proxy'); + expect(backup).toMatchObject({ id: 'uid', username: 'me', email: 'me@example.com' }); + }); + it('does nothing when there is no file', async () => { await ensureAuthFileCurrent(); @@ -172,7 +185,8 @@ describe('auth.json v2', () => { secretsBackend: 'file', token: 'apify_api_v1_token', }); - expect(readBackup()).toEqual({ token: 'apify_api_v1_token', secretsBackend: 'file' }); + // The token stays in auth.json, where the re-login prompt can see it, not in the backup. + expect(readBackup()).toEqual({ secretsBackend: 'file' }); await expect(getLocalUserInfo()).rejects.toThrow('Stale credentials found without user metadata'); }); }); @@ -233,6 +247,32 @@ describe('auth.json v2', () => { }); }); + // Both were deletable with a green suite: every other test calls ensureAuthFileCurrent() by hand. + describe('the command paths that trigger the migration', () => { + it('getLocalUserInfo() migrates the file it reads', async () => { + write(v1AuthFile({ secretsBackend: 'file' })); + + await expect(getLocalUserInfo()).resolves.toMatchObject({ id: 'uid', username: 'me' }); + + expect(readAuthFile().version).toBe(2); + }); + + it('resolving a token migrates the file it reads', async () => { + write(v1AuthFile({ secretsBackend: 'file' })); + + await expect(resolveAuth()).resolves.toMatchObject({ source: 'stored' }); + + expect(readAuthFile().version).toBe(2); + }); + + it('a file a newer CLI wrote stops a command rather than being read as v1', async () => { + write({ version: 3, activeProfile: 'uid', profiles: {}, secretsBackend: 'file', token: 'tok' }); + + await expect(getLocalUserInfo()).rejects.toThrow('written by a newer Apify CLI'); + await expect(resolveAuth()).rejects.toThrow('written by a newer Apify CLI'); + }); + }); + describe('keyring backend', () => { useKeyringBackend(); From 4970bbcdd137cb5869ff9e6a5a63c2f91e98fad7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Richard=20Sol=C3=A1r?= Date: Wed, 23 Sep 2026 11:25:21 +0200 Subject: [PATCH 05/33] test: skip the backup mode check on Windows Windows has no POSIX modes. Node reports 0o666 and chmod only moves the read-only bit, so the assertion read 438 where it wanted 384. The two other mode tests in the suite already skip on win32; this one now matches them. Co-Authored-By: Claude Opus 5 --- test/local/lib/auth-file.test.ts | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/test/local/lib/auth-file.test.ts b/test/local/lib/auth-file.test.ts index 3c9a7da6c..a86267e16 100644 --- a/test/local/lib/auth-file.test.ts +++ b/test/local/lib/auth-file.test.ts @@ -1,4 +1,5 @@ import { chmodSync, existsSync, mkdirSync, readFileSync, statSync, writeFileSync } from 'node:fs'; +import process from 'node:process'; import { __resetAuthFileForTests, @@ -137,14 +138,18 @@ describe('auth.json v2', () => { expect(existsSync(AUTH_BACKUP_FILE_PATH())).toBe(false); }); - it('writes the backup readable only by the owner, whatever mode the v1 file had', async () => { - write(v1AuthFile({ secretsBackend: 'file' })); - chmodSync(AUTH_FILE_PATH(), 0o644); + // Windows has no POSIX modes: Node reports 0o666 there and chmod only moves the read-only bit. + it.skipIf(process.platform === 'win32')( + 'writes the backup readable only by the owner, whatever mode the v1 file had', + async () => { + write(v1AuthFile({ secretsBackend: 'file' })); + chmodSync(AUTH_FILE_PATH(), 0o644); - await ensureAuthFileCurrent(); + await ensureAuthFileCurrent(); - expect(statSync(AUTH_BACKUP_FILE_PATH()).mode & 0o777).toBe(0o600); - }); + expect(statSync(AUTH_BACKUP_FILE_PATH()).mode & 0o777).toBe(0o600); + }, + ); // The backup is never refreshed, so a token in it would outlive the account it belongs to. it('keeps the secrets out of the backup', async () => { From 7a094373919c10a31261d8c407e732e1eb97ff22 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Richard=20Sol=C3=A1r?= Date: Wed, 23 Sep 2026 13:09:02 +0200 Subject: [PATCH 06/33] refactor: make the auth file migration a version step chain Moving data between shapes later needed this module reworked: the migration was one function gated on "no version field", so the next format change had nowhere to go. It is now a table keyed by the version each step upgrades from, and a file runs every step from its own version upwards. Adding a step is an entry in the table. A failed migration now says so once instead of only under APIFY_DEBUG. It still never blocks a command, because the readers understand the old shape, but failing on every run should be visible. Other fixes from review: - Reserve AuthProfile.secretsBackend. Once secrets are keyed per profile, a keyring failure on one profile must not silently redirect another profile's reads to the file backend. - ensureMigrated() skips a file a newer CLI wrote. It runs before the shape migration reports the version, and would otherwise rewrite it. - Write the backup through the atomic writer, the one plain write left in a module built around temp file plus rename. - Treat a non-object JSON payload as unusable. JSON.parse('"abc"') succeeds and Object.keys('abc') is ['0','1','2'], so it passed both migration guards and got replaced. - Drop the comment calling the backup a way back. Nothing reads it and no procedure restores it; the comment beside it already said so. Tests: - getLocalUserInfo() returns organizationOwnerUserId. Deleting that line left the suite green while demoting every organization login to a personal account. - A pre-existing 0644 auth.json is tightened to 0600. Only temp file plus rename does that; writeFileSync's mode applies on create only. - The three apify run tests read the token and proxy password from disk again. They had come to assert getLocalUserInfo() against itself. Co-Authored-By: Claude Opus 5 --- src/lib/auth-file.ts | 67 +++++++++++++++++++++++++------- src/lib/credentials.ts | 5 ++- test/local/commands/run.test.ts | 33 ++++++++-------- test/local/lib/auth-file.test.ts | 14 ++++++- 4 files changed, 88 insertions(+), 31 deletions(-) diff --git a/src/lib/auth-file.ts b/src/lib/auth-file.ts index bc0f492a7..dc5ec1627 100644 --- a/src/lib/auth-file.ts +++ b/src/lib/auth-file.ts @@ -5,11 +5,12 @@ import { cryptoRandomObjectId } from '@apify/utilities'; import { AUTH_FILE_PATH } from './consts.js'; import type { CredentialsBackend } from './credentials.js'; import { ensureApifyDirectory } from './files.js'; +import { warning } from './outputs.js'; import { cliDebugPrint } from './utils/cliDebugPrint.js'; -const AUTH_FILE_VERSION = 2; +export const AUTH_FILE_VERSION = 2; -/** The way back to a CLI that only reads the v1 shape. */ +/** Snapshot of the pre-v2 file. Nothing reads it; see {@link backUpV1File}. */ export const AUTH_BACKUP_FILE_PATH = () => `${AUTH_FILE_PATH()}.v1.bak`; /** @@ -28,6 +29,12 @@ export interface AuthProfile { expiresAt: string | null; /** Whether a refresh token came with the access token. Unused until the device flow lands. */ hasRefreshToken: boolean; + /** + * Where this profile's secrets live. Unused until secrets are keyed per profile; the file-level + * `secretsBackend` is the answer for every profile until then. Reserved here because a keyring + * failure on one profile must not silently redirect another profile's reads. + */ + secretsBackend?: CredentialsBackend; } /** @@ -62,7 +69,11 @@ function parseAuthFile(): AuthFile | null { if (!existsSync(AUTH_FILE_PATH())) return {}; try { - return JSON.parse(readFileSync(AUTH_FILE_PATH(), 'utf-8')) as AuthFile; + const parsed: unknown = JSON.parse(readFileSync(AUTH_FILE_PATH(), 'utf-8')); + // A valid JSON string or array is as unusable as a parse error, and must not be rewritten. + if (typeof parsed !== 'object' || parsed === null || Array.isArray(parsed)) return null; + + return parsed as AuthFile; } catch { return null; } @@ -78,7 +89,10 @@ export function readAuthFile(): AuthFile { * and a half-written auth.json reads as logged out. */ export function writeAuthFile(data: AuthFile) { - const path = AUTH_FILE_PATH(); + atomicWriteJson(AUTH_FILE_PATH(), data); +} + +function atomicWriteJson(path: string, data: unknown) { ensureApifyDirectory(path); const tempPath = `${path}.tmp-${cryptoRandomObjectId(8)}`; @@ -138,10 +152,21 @@ function backUpV1File(file: AuthFile) { if (existsSync(AUTH_BACKUP_FILE_PATH())) return; const { token: _token, proxy: _proxy, ...withoutSecrets } = file; - writeFileSync(AUTH_BACKUP_FILE_PATH(), JSON.stringify(withoutSecrets, null, '\t'), { mode: 0o600 }); + atomicWriteJson(AUTH_BACKUP_FILE_PATH(), withoutSecrets); } -async function migrateToV2(): Promise { +/** + * One entry per format bump, keyed by the version it upgrades from. A file at version N runs every + * step from N upwards, so moving data between shapes later means adding an entry here rather than + * reworking this module. The first shape carried no `version` field at all; it counts as 1. + */ +const MIGRATION_STEPS: Record AuthFile> = { + 1: toV2, +}; + +const FIRST_AUTH_FILE_VERSION = 1; + +async function migrateAuthFile(): Promise { migrationPromise ??= (async () => { try { const file = parseAuthFile(); @@ -149,15 +174,31 @@ async function migrateToV2(): Promise { // A corrupt file is left alone: readers already treat it as logged out, and rewriting // it would destroy what the user could still recover by hand. if (!file) return; - // A numbered version is either already current or from another CLI; either way there - // is nothing to migrate. `assertSupportedAuthFileVersion` reports a newer one. - if (typeof file.version === 'number') return; if (Object.keys(file).length === 0) return; - backUpV1File(file); - writeAuthFile(toV2(file)); + const from = typeof file.version === 'number' ? file.version : FIRST_AUTH_FILE_VERSION; + // A file from a newer CLI has no steps to run. `assertSupportedAuthFileVersion` reports it. + if (from >= AUTH_FILE_VERSION) return; + + // The backup captures the shape the user arrived with, before any step touches it. + if (from === FIRST_AUTH_FILE_VERSION) backUpV1File(file); + + let migrated = file; + for (let version = from; version < AUTH_FILE_VERSION; version++) { + const step = MIGRATION_STEPS[version]; + if (!step) throw new Error(`No migration step from auth file version ${version}.`); + + migrated = step(migrated); + } + + writeAuthFile(migrated); } catch (err) { - cliDebugPrint('auth-file', 'migration to v2 failed', err); + // Never blocks a command: the readers understand the old shape, so a failed migration + // costs nothing this run. Said once, because failing on every run should be visible. + cliDebugPrint('auth-file', 'auth file migration failed', err); + warning({ + message: `Could not update ${AUTH_FILE_PATH()} to the current format, so it was left as it is. Run with APIFY_CLI_DEBUG=1 to see why.`, + }); } })(); @@ -186,7 +227,7 @@ function assertSupportedAuthFileVersion() { * The migration itself is idempotent, single-flight and never throws — it must not block a command. */ export async function ensureAuthFileCurrent(): Promise { - await migrateToV2(); + await migrateAuthFile(); assertSupportedAuthFileVersion(); } diff --git a/src/lib/credentials.ts b/src/lib/credentials.ts index c6e5ff1d6..849ed51b5 100644 --- a/src/lib/credentials.ts +++ b/src/lib/credentials.ts @@ -1,6 +1,6 @@ import process from 'node:process'; -import { readAuthFile, writeAuthFile } from './auth-file.js'; +import { AUTH_FILE_VERSION, readAuthFile, writeAuthFile } from './auth-file.js'; import { useCLIMetadata } from './hooks/useCLIMetadata.js'; import { cliDebugPrint } from './utils/cliDebugPrint.js'; @@ -248,6 +248,9 @@ export async function ensureMigrated(): Promise { migrationPromise = (async () => { try { const file = readAuthFile(); + // A file a newer CLI wrote is not ours to rewrite, and this runs before the shape + // migration reports it. + if (typeof file.version === 'number' && file.version > AUTH_FILE_VERSION) return; if (file.secretsBackend) return; if (!file.token && !file.proxy?.password) return; diff --git a/test/local/commands/run.test.ts b/test/local/commands/run.test.ts index edb48ded2..6cb13665e 100644 --- a/test/local/commands/run.test.ts +++ b/test/local/commands/run.test.ts @@ -5,14 +5,15 @@ import { ACTOR_ENV_VARS, APIFY_ENV_VARS } from '@apify/consts'; import { testRunCommand } from '../../../src/lib/command-framework/apify-command.js'; import { EMPTY_LOCAL_CONFIG, LOCAL_CONFIG_PATH } from '../../../src/lib/consts.js'; +import { getProxyPassword, getToken } from '../../../src/lib/credentials.js'; import { rimrafPromised } from '../../../src/lib/files.js'; import { getLocalDatasetPath, getLocalKeyValueStorePath, getLocalRequestQueuePath, getLocalStorageDir, - getLocalUserInfo, } from '../../../src/lib/utils.js'; +import { readActiveProfile } from '../../__setup__/auth-file.js'; import { TEST_TIMEOUT } from '../../__setup__/consts.js'; import { safeLogin, useAuthSetup } from '../../__setup__/hooks/useAuthSetup.js'; import { useConsoleSpy } from '../../__setup__/hooks/useConsoleSpy.js'; @@ -124,11 +125,11 @@ describe('apify run', () => { const actOutputPath = joinPath(getLocalKeyValueStorePath(), 'OUTPUT.json'); const localEnvVars = JSON.parse(readFileSync(actOutputPath, 'utf8')); - const auth = await getLocalUserInfo(); - - expect(localEnvVars[APIFY_ENV_VARS.PROXY_PASSWORD]).toStrictEqual(auth.proxy!.password); - expect(localEnvVars[APIFY_ENV_VARS.USER_ID]).toStrictEqual(auth.id); - expect(localEnvVars[APIFY_ENV_VARS.TOKEN]).toStrictEqual(auth.token); + // Read from disk, not through getLocalUserInfo: `run` sources these from that same + // function, so asserting against it would only prove it agrees with itself. + expect(localEnvVars[APIFY_ENV_VARS.PROXY_PASSWORD]).toStrictEqual(await getProxyPassword()); + expect(localEnvVars[APIFY_ENV_VARS.USER_ID]).toStrictEqual(readActiveProfile()!.id); + expect(localEnvVars[APIFY_ENV_VARS.TOKEN]).toStrictEqual(await getToken()); expect(localEnvVars.TEST_LOCAL).toStrictEqual(testEnvVars.TEST_LOCAL); }); @@ -165,11 +166,11 @@ describe('apify run', () => { const actOutputPath = joinPath(getLocalKeyValueStorePath(), 'OUTPUT.json'); const localEnvVars = JSON.parse(readFileSync(actOutputPath, 'utf8')); - const auth = await getLocalUserInfo(); - - expect(localEnvVars[APIFY_ENV_VARS.PROXY_PASSWORD]).toStrictEqual(auth.proxy!.password); - expect(localEnvVars[APIFY_ENV_VARS.USER_ID]).toStrictEqual(auth.id); - expect(localEnvVars[APIFY_ENV_VARS.TOKEN]).toStrictEqual(auth.token); + // Read from disk, not through getLocalUserInfo: `run` sources these from that same + // function, so asserting against it would only prove it agrees with itself. + expect(localEnvVars[APIFY_ENV_VARS.PROXY_PASSWORD]).toStrictEqual(await getProxyPassword()); + expect(localEnvVars[APIFY_ENV_VARS.USER_ID]).toStrictEqual(readActiveProfile()!.id); + expect(localEnvVars[APIFY_ENV_VARS.TOKEN]).toStrictEqual(await getToken()); expect(localEnvVars.TEST_LOCAL).toStrictEqual(testEnvVars.TEST_LOCAL); const actOutputPath2 = joinPath(getLocalKeyValueStorePath(), 'owo.json'); @@ -205,11 +206,11 @@ describe('apify run', () => { const actOutputPath = joinPath(getLocalKeyValueStorePath(), 'OUTPUT.json'); const localEnvVars = JSON.parse(readFileSync(actOutputPath, 'utf8')); - const auth = await getLocalUserInfo(); - - expect(localEnvVars[APIFY_ENV_VARS.PROXY_PASSWORD]).toStrictEqual(auth.proxy!.password); - expect(localEnvVars[APIFY_ENV_VARS.USER_ID]).toStrictEqual(auth.id); - expect(localEnvVars[APIFY_ENV_VARS.TOKEN]).toStrictEqual(auth.token); + // Read from disk, not through getLocalUserInfo: `run` sources these from that same + // function, so asserting against it would only prove it agrees with itself. + expect(localEnvVars[APIFY_ENV_VARS.PROXY_PASSWORD]).toStrictEqual(await getProxyPassword()); + expect(localEnvVars[APIFY_ENV_VARS.USER_ID]).toStrictEqual(readActiveProfile()!.id); + expect(localEnvVars[APIFY_ENV_VARS.TOKEN]).toStrictEqual(await getToken()); expect(localEnvVars.TEST_LOCAL).toStrictEqual(testEnvVars.TEST_LOCAL); const actOutputPath2 = joinPath(getLocalKeyValueStorePath(), 'two.json'); diff --git a/test/local/lib/auth-file.test.ts b/test/local/lib/auth-file.test.ts index a86267e16..b09d56b70 100644 --- a/test/local/lib/auth-file.test.ts +++ b/test/local/lib/auth-file.test.ts @@ -91,12 +91,14 @@ describe('auth.json v2', () => { expect(file.proxy).toEqual({ password: 'pw' }); }); - it('carries organizationOwnerUserId into the profile', async () => { + it('carries organizationOwnerUserId into the profile, and back out again', async () => { write(v1AuthFile({ secretsBackend: 'file', organizationOwnerUserId: 'owner-id' })); await ensureAuthFileCurrent(); expect(readActiveProfile()).toMatchObject({ organizationOwnerUserId: 'owner-id' }); + // `push` and the Console URL read it from here; the file alone is not enough. + await expect(getLocalUserInfo()).resolves.toMatchObject({ organizationOwnerUserId: 'owner-id' }); }); it('backs the v1 file up and never overwrites the backup', async () => { @@ -138,6 +140,16 @@ describe('auth.json v2', () => { expect(existsSync(AUTH_BACKUP_FILE_PATH())).toBe(false); }); + // Only the temp-file + rename repairs an existing file's mode; a direct write would leave it. + it.skipIf(process.platform === 'win32')('tightens a pre-existing 0644 auth.json to 0600', async () => { + write(v1AuthFile({ secretsBackend: 'file' })); + chmodSync(AUTH_FILE_PATH(), 0o644); + + await ensureAuthFileCurrent(); + + expect(statSync(AUTH_FILE_PATH()).mode & 0o777).toBe(0o600); + }); + // Windows has no POSIX modes: Node reports 0o666 there and chmod only moves the read-only bit. it.skipIf(process.platform === 'win32')( 'writes the backup readable only by the owner, whatever mode the v1 file had', From dc0d1b9ca74faf7a46fb4880c9fccd348abdbbac Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Richard=20Sol=C3=A1r?= Date: Wed, 23 Sep 2026 14:40:16 +0200 Subject: [PATCH 07/33] refactor: name the login writer for what it does MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit setActiveProfile(userId, profile, backend) read as "mark this one active". It means "make this the only account, and drop the previous one's secrets". It is now replaceStoredAccount, and the docblock says why it replaces rather than adds. Replacing is deliberate twice over. Until each profile has its own secret, a second profile would name an account that cannot authenticate. And dropping the old secrets is what makes the write safe: loginWithToken writes the new token straight after, so a failure there leaves no token at all rather than the previous account's token sitting beside the new account's name. Two tests pin that second half, which nothing covered. Making the write preserve siblings and root secrets — the shape Stage-2 will need — fails both: the old profile survives, and so does the old token. Co-Authored-By: Claude Opus 5 --- src/lib/auth-file.ts | 14 ++++++++--- src/lib/auth.ts | 8 +++--- test/local/lib/auth-file.test.ts | 43 ++++++++++++++++++++++++++++++-- 3 files changed, 55 insertions(+), 10 deletions(-) diff --git a/src/lib/auth-file.ts b/src/lib/auth-file.ts index dc5ec1627..ac9a38732 100644 --- a/src/lib/auth-file.ts +++ b/src/lib/auth-file.ts @@ -256,10 +256,18 @@ export function getActiveProfile(): (AuthProfile & { id: string }) | undefined { } /** - * Stores one account and makes it active, replacing whatever was there. Nothing puts a second - * profile in the file yet, so `apify login` owns all of it. + * Stores one account as the only one in the file, dropping any previous profile and the secrets + * stored beside it. + * + * Replacing rather than adding is deliberate twice over. Until each profile has its own secret, a + * second profile would name an account that cannot authenticate. And dropping the old secrets is + * what makes the write safe: the caller writes the new token straight after, so a failure there + * leaves no token at all — a logged-out state — rather than the previous account's token sitting + * beside the new account's name, which authenticates as the wrong user. + * + * Adding a profile without disturbing the others is {@link https://github.com/apify/apify-cli/issues/1386 | Stage-2}. */ -export function setActiveProfile(userId: string, profile: AuthProfile, secretsBackend: CredentialsBackend) { +export function replaceStoredAccount(userId: string, profile: AuthProfile, secretsBackend: CredentialsBackend) { assertSupportedAuthFileVersion(); writeAuthFile({ diff --git a/src/lib/auth.ts b/src/lib/auth.ts index 6dd005e7b..7b6239d6e 100644 --- a/src/lib/auth.ts +++ b/src/lib/auth.ts @@ -6,7 +6,7 @@ import { AxiosHeaders } from 'axios'; import { APIFY_ENV_VARS } from '@apify/consts'; -import { ensureAuthFileCurrent, setActiveProfile } from './auth-file.js'; +import { ensureAuthFileCurrent, replaceStoredAccount } from './auth-file.js'; import { APIFY_CLIENT_DEFAULT_HEADERS, AUTH_FILE_PATH, CommandExitCodes } from './consts.js'; import { deleteProxyPassword, @@ -174,10 +174,8 @@ export async function loginWithToken( const proxyPassword = userInfo.proxy?.password; - // The profile is keyed by user ID, and it replaces whatever was stored rather than merging - // into it, so fields the new account does not have cannot linger from the old one. const { organizationOwnerUserId } = userInfo as { organizationOwnerUserId?: string }; - setActiveProfile( + replaceStoredAccount( userInfo.id, { username: userInfo.username, @@ -190,7 +188,7 @@ export async function loginWithToken( await getBackend(), ); - // After the metadata file, which would clobber them on the file backend. `skipIfUnchanged` avoids a Keychain prompt. + // After the account, which drops the previous secrets. `skipIfUnchanged` avoids a Keychain prompt. await setToken(token, { skipIfUnchanged: true }); if (proxyPassword) { diff --git a/test/local/lib/auth-file.test.ts b/test/local/lib/auth-file.test.ts index b09d56b70..4351dfa9d 100644 --- a/test/local/lib/auth-file.test.ts +++ b/test/local/lib/auth-file.test.ts @@ -9,7 +9,7 @@ import { getActiveProfile, lookUpActiveProfile, removeActiveProfile, - setActiveProfile, + replaceStoredAccount, } 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'; @@ -251,7 +251,7 @@ describe('auth.json v2', () => { const newer = { version: 3, activeProfile: 'uid', profiles: { uid: { username: 'me' } } }; write(newer); - expect(() => setActiveProfile('uid2', V2_PROFILE, 'file')).toThrow('written by a newer Apify CLI'); + expect(() => replaceStoredAccount('uid2', V2_PROFILE, 'file')).toThrow('written by a newer Apify CLI'); expect(readAuthFile()).toEqual(newer); }); @@ -265,6 +265,45 @@ describe('auth.json v2', () => { }); // Both were deletable with a green suite: every other test calls ensureAuthFileCurrent() by hand. + // The write drops the previous account's secrets, and loginWithToken writes the new token + // straight after. Nothing pinned either half before. + describe('replacing the stored account', () => { + it('drops the previous account and its secrets', () => { + write({ + version: 2, + activeProfile: 'old', + profiles: { old: { ...V2_PROFILE, username: 'old' } }, + secretsBackend: 'file', + token: 'apify_api_old', + proxy: { password: 'old_pw' }, + }); + + replaceStoredAccount('new', { ...V2_PROFILE, username: 'new' }, 'file'); + + const file = readAuthFile(); + expect(Object.keys(file.profiles!)).toEqual(['new']); + expect(file.activeProfile).toBe('new'); + // A leftover token beside the new account authenticates as the wrong user. + expect(file).not.toHaveProperty('token'); + expect(file).not.toHaveProperty('proxy'); + }); + + it('leaves no token when the caller never writes one', async () => { + write({ + version: 2, + activeProfile: 'old', + profiles: { old: { ...V2_PROFILE, username: 'old' } }, + secretsBackend: 'file', + token: 'apify_api_old', + }); + + 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(); + }); + }); + describe('the command paths that trigger the migration', () => { it('getLocalUserInfo() migrates the file it reads', async () => { write(v1AuthFile({ secretsBackend: 'file' })); From 2733ea8510df727fec164aab46c599ccd6e9ae11 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Richard=20Sol=C3=A1r?= Date: Wed, 23 Sep 2026 14:58:43 +0200 Subject: [PATCH 08/33] refactor: drop the catch-all index signature from AuthFile MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit AuthFile carried `[k: string]: unknown`, so it typed the v1 shape, the v2 shape and an empty object identically. Reading a field that no longer exists stayed legal, which matters because #1420 moves the token and proxy password off the top level and into the profile. Measured: remove `token` from the type and the old signature reported 2 errors. It now reports 12 — every reader, across auth-file.ts and credentials.ts. Ten sites would have gone unnamed. The v1 fields move to LegacyAuthFile, which extends AuthFile with the three the flat shape carried. Only v1Profile, toV2 and the fallback in lookUpActiveProfile take it; that fallback is the one cast left, and it is where a file this CLI cannot version lands. Co-Authored-By: Claude Opus 5 --- src/lib/auth-file.ts | 25 +++++++++++++++++++------ 1 file changed, 19 insertions(+), 6 deletions(-) diff --git a/src/lib/auth-file.ts b/src/lib/auth-file.ts index ac9a38732..307e463f4 100644 --- a/src/lib/auth-file.ts +++ b/src/lib/auth-file.ts @@ -38,8 +38,9 @@ export interface AuthProfile { } /** - * `auth.json` as it sits on disk. `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. `token` and `proxy` are the file backend's secret storage; + * they stay outside the profiles until each profile gets its own keys. No index signature: the + * fields listed here are the whole surface, so removing one names every reader at compile time. */ export interface AuthFile { version?: number; @@ -48,7 +49,16 @@ export interface AuthFile { secretsBackend?: CredentialsBackend; token?: string; proxy?: { password?: string; [k: string]: unknown }; - [k: string]: unknown; +} + +/** + * The flat shape written before profiles existed: one account spread across the top level, and no + * `version` field. Only the migration and the pre-migration read path see it. + */ +export interface LegacyAuthFile extends AuthFile { + id?: string; + username?: string; + organizationOwnerUserId?: string; } export interface ActiveProfileLookup { @@ -107,7 +117,7 @@ function atomicWriteJson(path: string, data: unknown) { } /** The one account a v1 file described, as a profile. */ -function v1Profile(file: AuthFile): AuthProfile { +function v1Profile(file: LegacyAuthFile): AuthProfile { return { ...(typeof file.username === 'string' ? { username: file.username } : {}), name: null, @@ -125,7 +135,7 @@ function v1Profile(file: AuthFile): AuthProfile { * `effectivePlatformFeatures`, `isPaying`, `createdAt` and `proxy.groups` are dropped — nothing in * the CLI reads them. */ -function toV2(file: AuthFile): AuthFile { +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 @@ -239,7 +249,10 @@ export function lookUpActiveProfile(): ActiveProfileLookup { const file = readAuthFile(); if (file.version !== AUTH_FILE_VERSION) { - return typeof file.id === 'string' ? { profile: { id: file.id, ...v1Profile(file) } } : {}; + // Pre-migration, or a version this CLI does not know. Either way the only account it can + // name is the flat one, and a newer file has no top-level id to find. + const legacy = file as LegacyAuthFile; + return typeof legacy.id === 'string' ? { profile: { id: legacy.id, ...v1Profile(legacy) } } : {}; } if (!file.activeProfile) return {}; From 47fdf538b08d267eff217aa300e750c0190ca5be Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Richard=20Sol=C3=A1r?= Date: Wed, 23 Sep 2026 20:42:55 +0200 Subject: [PATCH 09/33] fix: stop a newer auth file from blocking APIFY_TOKEN and logout MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The version guard ran before resolveAuth read APIFY_TOKEN, so a stored file written by a newer CLI stopped every command — including ones that never read that file. A platform run or a CI job has APIFY_TOKEN as its only credential and no interest in the stored login, and `apify run` calls resolveAuth uncaught while deliberately catching the account lookup on the next line. The guard now runs on the stored-login path only. Logout was refused by the same guard, which left no way out of the state: the error offered "Upgrade the CLI" and never mentioned the file. Logout exists to discard credentials, so it no longer checks the version. A shape this CLI cannot read is discarded whole rather than edited — the old code deleted fields from it and wrote it back, which left a mangled file when it held more than one profile. The error names `apify logout` as the escape, and drops the parenthetical aside the repo's copy style does not take. Removing the auth files also passes maxRetries again. rimrafPromised carried 10 retries against Windows EBUSY when an antivirus or a second process holds the file; the bare rmSync that replaced it had none. Co-Authored-By: Claude Opus 5 --- src/lib/auth-file.ts | 22 ++++++++++++++++------ src/lib/auth.ts | 5 ++++- test/local/lib/auth-file.test.ts | 25 ++++++++++++++++++++----- 3 files changed, 40 insertions(+), 12 deletions(-) diff --git a/src/lib/auth-file.ts b/src/lib/auth-file.ts index 307e463f4..8c9820c32 100644 --- a/src/lib/auth-file.ts +++ b/src/lib/auth-file.ts @@ -224,7 +224,7 @@ function assertSupportedAuthFileVersion() { if (typeof version === 'number' && version > AUTH_FILE_VERSION) { throw new Error( - `Your credentials in ${AUTH_FILE_PATH()} were written by a newer Apify CLI (auth file version ${version}, this one reads ${AUTH_FILE_VERSION}). Upgrade the CLI to use them.`, + `Your credentials in ${AUTH_FILE_PATH()} were written by a newer Apify CLI. It uses auth file version ${version} and this one reads ${AUTH_FILE_VERSION}. Upgrade the CLI, or run "apify logout" to discard them.`, ); } } @@ -296,21 +296,31 @@ export function replaceStoredAccount(userId: string, profile: AuthProfile, secre * go away once no profile is left, so logging out leaves no token on disk. */ export function removeActiveProfile() { - assertSupportedAuthFileVersion(); - const file = readAuthFile(); - const active = file.version === AUTH_FILE_VERSION ? file.activeProfile : undefined; + // No version guard. Logout exists to discard credentials, so refusing a file this CLI cannot + // read would leave the user no way out of that state. A shape we do not understand goes whole + // rather than edited, because editing it would leave something worse than either outcome. + if (file.version !== AUTH_FILE_VERSION) { + discardAuthFiles(); + return; + } + + const active = file.activeProfile; if (active && file.profiles) delete file.profiles[active]; delete file.activeProfile; delete file.token; delete file.proxy; if (Object.keys(file.profiles ?? {}).length === 0) { - rmSync(AUTH_FILE_PATH(), { force: true }); - rmSync(AUTH_BACKUP_FILE_PATH(), { force: true }); + discardAuthFiles(); return; } writeAuthFile(file); } + +function discardAuthFiles() { + rmSync(AUTH_FILE_PATH(), { force: true, maxRetries: 10, retryDelay: 100 }); + rmSync(AUTH_BACKUP_FILE_PATH(), { force: true, maxRetries: 10, retryDelay: 100 }); +} diff --git a/src/lib/auth.ts b/src/lib/auth.ts index 7b6239d6e..1ab32c7ae 100644 --- a/src/lib/auth.ts +++ b/src/lib/auth.ts @@ -80,7 +80,6 @@ export function __resetAuthForTests() { export const resolveAuth = async (): Promise => { authPromise ??= (async () => { await ensureMigrated(); - await ensureAuthFileCurrent(); const envToken = readEnvToken(); if (envToken.kind === 'invalid') { @@ -96,6 +95,10 @@ export const resolveAuth = async (): Promise => { return { token: envToken.token, source: 'env' } as const; } + // 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(); + const storedToken = await getToken(); return storedToken ? ({ token: storedToken, source: 'stored' } as const) : undefined; })(); diff --git a/test/local/lib/auth-file.test.ts b/test/local/lib/auth-file.test.ts index 4351dfa9d..9e332e479 100644 --- a/test/local/lib/auth-file.test.ts +++ b/test/local/lib/auth-file.test.ts @@ -255,12 +255,19 @@ describe('auth.json v2', () => { expect(readAuthFile()).toEqual(newer); }); - it('is not touched by a logout', () => { - const newer = { version: 3, activeProfile: 'uid', profiles: { uid: { username: 'me' } }, token: 'tok' }; - write(newer); + // Logout is the only way out of this state, so it is the one command that must not refuse. + it('is discarded by a logout', () => { + write({ version: 3, activeProfile: 'uid', profiles: { uid: { username: 'me' } }, token: 'tok' }); - expect(() => removeActiveProfile()).toThrow('written by a newer Apify CLI'); - expect(readAuthFile()).toEqual(newer); + removeActiveProfile(); + + expect(existsSync(AUTH_FILE_PATH())).toBe(false); + }); + + it('says how to get out of the state', async () => { + write({ version: 3, activeProfile: 'uid', profiles: {} }); + + await expect(ensureAuthFileCurrent()).rejects.toThrow('apify logout'); }); }); @@ -327,6 +334,14 @@ describe('auth.json v2', () => { await expect(getLocalUserInfo()).rejects.toThrow('written by a newer Apify CLI'); await expect(resolveAuth()).rejects.toThrow('written by a newer Apify CLI'); }); + + // A platform run never reads the stored file, so a newer one must not stop it. + it('a file a newer CLI wrote does not stop a command running on APIFY_TOKEN', async () => { + write({ version: 3, activeProfile: 'uid', profiles: {}, secretsBackend: 'file', token: 'stored' }); + vitest.stubEnv('APIFY_TOKEN', 'apify_api_from_env'); + + await expect(resolveAuth()).resolves.toEqual({ token: 'apify_api_from_env', source: 'env' }); + }); }); describe('keyring backend', () => { From 8d66a2b3fca27c9e3c126a17e9959e499dee4df2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Richard=20Sol=C3=A1r?= Date: Wed, 23 Sep 2026 21:22:42 +0200 Subject: [PATCH 10/33] docs: trim the comments this branch added Three kinds went: - Restating the signature. "The parsed file, or an empty object when it is missing or unreadable" above a function that returns exactly that. - Narrating the branch. "No index signature: removing one names every reader at compile time" justifies a commit, in a place that will rot once nobody remembers there was one. - Repeated verbatim. The same three-line rationale sat above all three apify run assertions; one earns its keep. Three fields carrying the same "unused until the device flow lands" share one line now, and two of the longer blocks say the same thing shorter. Co-Authored-By: Claude Opus 5 --- src/lib/auth-file.ts | 17 +++++------------ src/lib/utils.ts | 4 ++-- test/api/commands/log_in_out.test.ts | 2 -- test/local/commands/run.test.ts | 4 ---- test/local/lib/auth-file.test.ts | 4 ---- 5 files changed, 7 insertions(+), 24 deletions(-) diff --git a/src/lib/auth-file.ts b/src/lib/auth-file.ts index 8c9820c32..d7b352786 100644 --- a/src/lib/auth-file.ts +++ b/src/lib/auth-file.ts @@ -23,11 +23,9 @@ export interface AuthProfile { name: string | null; /** Set means the profile is an organization rather than a personal account. */ organizationOwnerUserId?: string; - /** How the token was obtained. Unused until the device flow lands. */ + /** These three are unread until the device flow lands, and reserved so it needs no migration. */ authMethod: 'token'; - /** When the access token expires. Unused until the device flow lands. */ expiresAt: string | null; - /** Whether a refresh token came with the access token. Unused until the device flow lands. */ hasRefreshToken: boolean; /** * Where this profile's secrets live. Unused until secrets are keyed per profile; the file-level @@ -39,8 +37,7 @@ export interface AuthProfile { /** * `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. No index signature: the - * fields listed here are the whole surface, so removing one names every reader at compile time. + * they stay outside the profiles until each profile gets its own keys. */ export interface AuthFile { version?: number; @@ -89,7 +86,6 @@ function parseAuthFile(): AuthFile | null { } } -/** The parsed file, or an empty object when it is missing or unreadable. */ export function readAuthFile(): AuthFile { return parseAuthFile() ?? {}; } @@ -116,7 +112,6 @@ function atomicWriteJson(path: string, data: unknown) { } } -/** The one account a v1 file described, as a profile. */ function v1Profile(file: LegacyAuthFile): AuthProfile { return { ...(typeof file.username === 'string' ? { username: file.username } : {}), @@ -153,10 +148,9 @@ function toV2(file: LegacyAuthFile): AuthFile { } /** - * A snapshot of the pre-v2 file, kept so an upgrade is inspectable. Written once and never - * refreshed, which is why the secrets are left out: `apify login` replaces auth.json but cannot - * reach this file, so a copy of a rotated token would sit here until the next logout. Nothing - * reads it, and a downgraded CLI finds its token through the usual backends rather than here. + * A snapshot of the pre-v2 file, so an upgrade is inspectable. Written once and never refreshed, + * which is why the secrets are left out: a rotated token copied here would outlive the account it + * belongs to. Nothing reads it. */ function backUpV1File(file: AuthFile) { if (existsSync(AUTH_BACKUP_FILE_PATH())) return; @@ -187,7 +181,6 @@ async function migrateAuthFile(): Promise { if (Object.keys(file).length === 0) return; const from = typeof file.version === 'number' ? file.version : FIRST_AUTH_FILE_VERSION; - // A file from a newer CLI has no steps to run. `assertSupportedAuthFileVersion` reports it. if (from >= AUTH_FILE_VERSION) return; // The backup captures the shape the user arrived with, before any step touches it. diff --git a/src/lib/utils.ts b/src/lib/utils.ts index 74d47b873..b1ce8e476 100644 --- a/src/lib/utils.ts +++ b/src/lib/utils.ts @@ -108,8 +108,8 @@ export const getLocalUserInfo = async (): Promise => { const proxyPassword = await getProxyPassword(); if (proxyPassword) result.proxy = { password: proxyPassword }; - // A token with no profile behind it is reported rather than swallowed: the commands that build - // `/` lookups would otherwise fail with a misleading "not found". + // Reported rather than swallowed: the commands that build `/` lookups would + // otherwise fail with a misleading "not found". if (!profile) { if (!result.token) return {}; diff --git a/test/api/commands/log_in_out.test.ts b/test/api/commands/log_in_out.test.ts index 28969e264..d86998f92 100644 --- a/test/api/commands/log_in_out.test.ts +++ b/test/api/commands/log_in_out.test.ts @@ -37,7 +37,6 @@ describe('[api] apify login and logout', () => { expect(lastErrorMessage()).to.include('Success:'); - // v2 stores the account as a profile keyed by user ID, not the whole user('me') response. expect(readActiveProfile()).toMatchObject({ id: expectedUserInfo.id, username: expectedUserInfo.username, @@ -74,7 +73,6 @@ describe('[api] apify login and logout', () => { expect(lastErrorMessage()).to.include('Success:'); - // v2 stores the account as a profile keyed by user ID, not the whole user('me') response. expect(readActiveProfile()).toMatchObject({ id: expectedUserInfo.id, username: expectedUserInfo.username, diff --git a/test/local/commands/run.test.ts b/test/local/commands/run.test.ts index 6cb13665e..0811f67b4 100644 --- a/test/local/commands/run.test.ts +++ b/test/local/commands/run.test.ts @@ -166,8 +166,6 @@ describe('apify run', () => { const actOutputPath = joinPath(getLocalKeyValueStorePath(), 'OUTPUT.json'); const localEnvVars = JSON.parse(readFileSync(actOutputPath, 'utf8')); - // Read from disk, not through getLocalUserInfo: `run` sources these from that same - // function, so asserting against it would only prove it agrees with itself. expect(localEnvVars[APIFY_ENV_VARS.PROXY_PASSWORD]).toStrictEqual(await getProxyPassword()); expect(localEnvVars[APIFY_ENV_VARS.USER_ID]).toStrictEqual(readActiveProfile()!.id); expect(localEnvVars[APIFY_ENV_VARS.TOKEN]).toStrictEqual(await getToken()); @@ -206,8 +204,6 @@ describe('apify run', () => { const actOutputPath = joinPath(getLocalKeyValueStorePath(), 'OUTPUT.json'); const localEnvVars = JSON.parse(readFileSync(actOutputPath, 'utf8')); - // Read from disk, not through getLocalUserInfo: `run` sources these from that same - // function, so asserting against it would only prove it agrees with itself. expect(localEnvVars[APIFY_ENV_VARS.PROXY_PASSWORD]).toStrictEqual(await getProxyPassword()); expect(localEnvVars[APIFY_ENV_VARS.USER_ID]).toStrictEqual(readActiveProfile()!.id); expect(localEnvVars[APIFY_ENV_VARS.TOKEN]).toStrictEqual(await getToken()); diff --git a/test/local/lib/auth-file.test.ts b/test/local/lib/auth-file.test.ts index 9e332e479..64f6576b4 100644 --- a/test/local/lib/auth-file.test.ts +++ b/test/local/lib/auth-file.test.ts @@ -128,7 +128,6 @@ describe('auth.json v2', () => { expect(readAuthFile()).toEqual(migrated); }); - // The only code path that erases the plaintext v1 token from disk. it('logout removes the backup along with the file', async () => { write(v1AuthFile({ secretsBackend: 'file' })); await ensureAuthFileCurrent(); @@ -271,9 +270,6 @@ describe('auth.json v2', () => { }); }); - // Both were deletable with a green suite: every other test calls ensureAuthFileCurrent() by hand. - // The write drops the previous account's secrets, and loginWithToken writes the new token - // straight after. Nothing pinned either half before. describe('replacing the stored account', () => { it('drops the previous account and its secrets', () => { write({ From 36a7004d8e961ed983e8e04e79dd84a67b65e6c8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Richard=20Sol=C3=A1r?= Date: Wed, 23 Sep 2026 21:44:59 +0200 Subject: [PATCH 11/33] fix: say the login still works when the migration cannot write MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit "Could not update auth.json to the current format, so it was left as it is" reads like something broke. Nothing did — the readers understand the old shape, so the command that triggered it carries on and the next one tries again. A user with a read-only ~/.apify saw an alarming line on every command with no action to take. The message now leads with what matters to them and names the debug variable for the part that does not. Adds the failure-path test. The whole migration sits in one try/catch and nothing covered it: removing the warning left the suite green. Co-Authored-By: Claude Opus 5 --- src/lib/auth-file.ts | 6 +++--- test/local/lib/auth-file.test.ts | 17 +++++++++++++++++ 2 files changed, 20 insertions(+), 3 deletions(-) diff --git a/src/lib/auth-file.ts b/src/lib/auth-file.ts index d7b352786..e0840c970 100644 --- a/src/lib/auth-file.ts +++ b/src/lib/auth-file.ts @@ -196,11 +196,11 @@ async function migrateAuthFile(): Promise { writeAuthFile(migrated); } catch (err) { - // Never blocks a command: the readers understand the old shape, so a failed migration - // costs nothing this run. Said once, because failing on every run should be visible. + // 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. cliDebugPrint('auth-file', 'auth file migration failed', err); warning({ - message: `Could not update ${AUTH_FILE_PATH()} to the current format, so it was left as it is. Run with APIFY_CLI_DEBUG=1 to see why.`, + 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.`, }); } })(); diff --git a/test/local/lib/auth-file.test.ts b/test/local/lib/auth-file.test.ts index 64f6576b4..8c2e8ed49 100644 --- a/test/local/lib/auth-file.test.ts +++ b/test/local/lib/auth-file.test.ts @@ -17,6 +17,7 @@ import { ensureMigrated, getProxyPassword, getToken } from '../../../src/lib/cre 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, @@ -27,6 +28,7 @@ import { vi.mock('@napi-rs/keyring', () => import('../../__setup__/keyring-mock.js')); useAuthSetup(); +const { lastErrorMessage } = useConsoleSpy(); const write = (contents: unknown) => { mkdirSync(GLOBAL_CONFIGS_FOLDER(), { recursive: true }); @@ -174,6 +176,21 @@ describe('auth.json v2', () => { expect(backup).toMatchObject({ id: 'uid', username: 'me', email: 'me@example.com' }); }); + // 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('does nothing when there is no file', async () => { await ensureAuthFileCurrent(); From 6c73f451b02edbd637083e05761cdb4ad7e2f5a3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Richard=20Sol=C3=A1r?= Date: Thu, 24 Sep 2026 09:19:05 +0200 Subject: [PATCH 12/33] docs: fix the stale and padded comments in auth-file MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two were wrong. ensureAuthFileCurrent said it brings the file "to the v2 profile shape", which predates the step chain, and that migrating "never throws" — the function does, through the version assert one line below. lookUpActiveProfile described the pre-profile read path as something that covers commands running before the migration, which reads like scaffolding; useRentalSunsetNotice calls it without migrating on purpose, so that path is permanent. The rest were restating the signature, saying the same thing in two docblocks, or taking five lines for one idea. Comment-only: the diff has no non-comment lines. Co-Authored-By: Claude Opus 5 --- src/lib/auth-file.ts | 45 ++++++++++++++------------------------------ 1 file changed, 14 insertions(+), 31 deletions(-) diff --git a/src/lib/auth-file.ts b/src/lib/auth-file.ts index e0840c970..beabdbd36 100644 --- a/src/lib/auth-file.ts +++ b/src/lib/auth-file.ts @@ -10,7 +10,6 @@ import { cliDebugPrint } from './utils/cliDebugPrint.js'; export const AUTH_FILE_VERSION = 2; -/** Snapshot of the pre-v2 file. Nothing reads it; see {@link backUpV1File}. */ export const AUTH_BACKUP_FILE_PATH = () => `${AUTH_FILE_PATH()}.v1.bak`; /** @@ -27,11 +26,7 @@ export interface AuthProfile { authMethod: 'token'; expiresAt: string | null; hasRefreshToken: boolean; - /** - * Where this profile's secrets live. Unused until secrets are keyed per profile; the file-level - * `secretsBackend` is the answer for every profile until then. Reserved here because a keyring - * failure on one profile must not silently redirect another profile's reads. - */ + /** Reserved: a keyring failure on one profile must not redirect another profile's reads. */ secretsBackend?: CredentialsBackend; } @@ -66,7 +61,7 @@ export interface ActiveProfileLookup { let migrationPromise: Promise | undefined; -/** Test-only: let each test run the v2 migration again. */ +/** Test-only: let each test run the migration again. */ export function __resetAuthFileForTests() { migrationPromise = undefined; } @@ -90,14 +85,11 @@ export function readAuthFile(): AuthFile { return parseAuthFile() ?? {}; } -/** - * Atomic write: a temp file next to the target, then a rename. Two CLI processes can run at once, - * and a half-written auth.json reads as logged out. - */ export function writeAuthFile(data: AuthFile) { atomicWriteJson(AUTH_FILE_PATH(), data); } +/** Temp file then rename: two CLI processes can run at once, and a torn file reads as logged out. */ function atomicWriteJson(path: string, data: unknown) { ensureApifyDirectory(path); @@ -223,11 +215,10 @@ function assertSupportedAuthFileVersion() { } /** - * Brings `auth.json` to the v2 profile shape and refuses a file a newer CLI wrote. Runs after - * `ensureMigrated()`, which moves v1 secrets into the keyring; the two steps stay separate so a - * keyring failure and a shape failure cannot mask each other. + * Runs after `ensureMigrated()`, which moves v1 secrets into the keyring. The two stay separate so + * a keyring failure and a shape failure cannot mask each other. * - * The migration itself is idempotent, single-flight and never throws — it must not block a command. + * Migrating never throws — it must not block a command. Throws only for a file a newer CLI wrote. */ export async function ensureAuthFileCurrent(): Promise { await migrateAuthFile(); @@ -235,8 +226,8 @@ export async function ensureAuthFileCurrent(): Promise { } /** - * The active profile with its user ID. Reads a v1 file too, so a command that runs before the - * migration still finds the account. + * Reads the pre-profile shape as well, and keeps doing so: `useRentalSunsetNotice` calls this + * without migrating first, to avoid a keychain prompt on commands that need no login. */ export function lookUpActiveProfile(): ActiveProfileLookup { const file = readAuthFile(); @@ -256,22 +247,15 @@ export function lookUpActiveProfile(): ActiveProfileLookup { return { profile: { id: file.activeProfile, ...profile } }; } -/** The active profile, or `undefined` when nothing usable is stored. */ export function getActiveProfile(): (AuthProfile & { id: string }) | undefined { return lookUpActiveProfile().profile; } /** - * Stores one account as the only one in the file, dropping any previous profile and the secrets - * stored beside it. - * - * Replacing rather than adding is deliberate twice over. Until each profile has its own secret, a - * second profile would name an account that cannot authenticate. And dropping the old secrets is - * what makes the write safe: the caller writes the new token straight after, so a failure there - * leaves no token at all — a logged-out state — rather than the previous account's token sitting - * beside the new account's name, which authenticates as the wrong user. - * - * Adding a profile without disturbing the others is {@link https://github.com/apify/apify-cli/issues/1386 | Stage-2}. + * 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. */ export function replaceStoredAccount(userId: string, profile: AuthProfile, secretsBackend: CredentialsBackend) { assertSupportedAuthFileVersion(); @@ -291,9 +275,8 @@ export function replaceStoredAccount(userId: string, profile: AuthProfile, secre export function removeActiveProfile() { const file = readAuthFile(); - // No version guard. Logout exists to discard credentials, so refusing a file this CLI cannot - // read would leave the user no way out of that state. A shape we do not understand goes whole - // rather than edited, because editing it would leave something worse than either outcome. + // No version guard: refusing to discard a file this CLI cannot read leaves no way out. It goes + // whole rather than edited, which would leave something worse than either outcome. if (file.version !== AUTH_FILE_VERSION) { discardAuthFiles(); return; From fbe77448a05d9fb5794ae696c4481d68f49ba023 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Richard=20Sol=C3=A1r?= Date: Thu, 24 Sep 2026 15:51:21 +0200 Subject: [PATCH 13/33] feat: record loggedInAt, and drop the v1 snapshot on login MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit auth.json.v1.bak described the account it was taken from and is never refreshed, so only logout removed it. Log in as someone else and one user's details sat on disk under another user's login. A login now discards it. Profiles carry loggedInAt. Nothing reads it — `auth list` will order by it, and a logout will fall back to the most recent profile left — but neither can backfill a time nobody recorded, so it has to be written from the start. A profile migrated from the pre-profile file gets null, because that file never held one. Co-Authored-By: Claude Opus 5 --- src/lib/auth-file.ts | 11 +++++++++++ src/lib/auth.ts | 1 + test/local/commands/auth.test.ts | 1 + test/local/lib/auth-file.test.ts | 14 ++++++++++++++ test/local/lib/rental-sunset-notice.test.ts | 9 ++++++++- 5 files changed, 35 insertions(+), 1 deletion(-) diff --git a/src/lib/auth-file.ts b/src/lib/auth-file.ts index beabdbd36..27f8f31c4 100644 --- a/src/lib/auth-file.ts +++ b/src/lib/auth-file.ts @@ -28,6 +28,12 @@ export interface AuthProfile { hasRefreshToken: boolean; /** Reserved: a keyring failure on one profile must not redirect another profile's reads. */ secretsBackend?: CredentialsBackend; + /** + * When this account last logged in, or `null` for one migrated from the pre-profile file. + * Written but unread: `auth list` orders by it, and a logout falls back to the most recent + * profile left. Neither exists yet, and neither can backfill a time nobody recorded. + */ + loggedInAt: string | null; } /** @@ -114,6 +120,7 @@ function v1Profile(file: LegacyAuthFile): AuthProfile { authMethod: 'token', expiresAt: null, hasRefreshToken: false, + loggedInAt: null, }; } @@ -260,6 +267,10 @@ export function getActiveProfile(): (AuthProfile & { id: string }) | undefined { export function replaceStoredAccount(userId: string, profile: AuthProfile, secretsBackend: CredentialsBackend) { assertSupportedAuthFileVersion(); + // The snapshot described the account being replaced, and is never refreshed, so keeping it + // would leave one user's details on disk under another user's login. + rmSync(AUTH_BACKUP_FILE_PATH(), { force: true, maxRetries: 10, retryDelay: 100 }); + writeAuthFile({ version: AUTH_FILE_VERSION, activeProfile: userId, diff --git a/src/lib/auth.ts b/src/lib/auth.ts index 1ab32c7ae..636dfb1cd 100644 --- a/src/lib/auth.ts +++ b/src/lib/auth.ts @@ -187,6 +187,7 @@ export async function loginWithToken( authMethod: 'token', expiresAt: null, hasRefreshToken: false, + loggedInAt: new Date().toISOString(), }, await getBackend(), ); diff --git a/test/local/commands/auth.test.ts b/test/local/commands/auth.test.ts index 681f1ab89..bdcf3cc2f 100644 --- a/test/local/commands/auth.test.ts +++ b/test/local/commands/auth.test.ts @@ -52,6 +52,7 @@ describe('auth commands', () => { authMethod: 'token', expiresAt: null, hasRefreshToken: false, + loggedInAt: expect.stringMatching(/^\d{4}-\d{2}-\d{2}T/), }); expect(lastErrorMessage()).toContain('You are logged in to Apify as me'); }); diff --git a/test/local/lib/auth-file.test.ts b/test/local/lib/auth-file.test.ts index 8c2e8ed49..f3b17a90c 100644 --- a/test/local/lib/auth-file.test.ts +++ b/test/local/lib/auth-file.test.ts @@ -43,6 +43,7 @@ const V2_PROFILE: AuthProfile = { authMethod: 'token', expiresAt: null, hasRefreshToken: false, + loggedInAt: null, }; const V1_PROFILE = { id: 'uid', ...V2_PROFILE }; @@ -324,6 +325,19 @@ describe('auth.json v2', () => { }); }); + describe('replacing the stored account', () => { + it('removes the snapshot of the account it replaced', async () => { + write(v1AuthFile({ secretsBackend: 'file' })); + await ensureAuthFileCurrent(); + expect(existsSync(AUTH_BACKUP_FILE_PATH())).toBe(true); + + replaceStoredAccount('other', { ...V2_PROFILE, username: 'other' }, 'file'); + + // It described the previous account and is never refreshed, so it must not survive. + expect(existsSync(AUTH_BACKUP_FILE_PATH())).toBe(false); + }); + }); + describe('the command paths that trigger the migration', () => { it('getLocalUserInfo() migrates the file it reads', async () => { write(v1AuthFile({ secretsBackend: 'file' })); diff --git a/test/local/lib/rental-sunset-notice.test.ts b/test/local/lib/rental-sunset-notice.test.ts index 947503ac3..c6372647a 100644 --- a/test/local/lib/rental-sunset-notice.test.ts +++ b/test/local/lib/rental-sunset-notice.test.ts @@ -33,7 +33,14 @@ async function writeAuthFile(username: string | undefined) { version: 2, activeProfile: 'user-id', profiles: { - 'user-id': { username, name: null, authMethod: 'token', expiresAt: null, hasRefreshToken: false }, + 'user-id': { + username, + name: null, + authMethod: 'token', + expiresAt: null, + hasRefreshToken: false, + loggedInAt: null, + }, }, token: 'apify_api_token', }), From 9379ff5e0d6d06ce06acef129dd8841c95fbac40 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Richard=20Sol=C3=A1r?= Date: Thu, 24 Sep 2026 15:55:13 +0200 Subject: [PATCH 14/33] docs: drop the loggedInAt docblock Four lines for a field whose name and type say it. The reason it exists belongs in the commit that added it. Co-Authored-By: Claude Opus 5 --- src/lib/auth-file.ts | 5 ----- 1 file changed, 5 deletions(-) diff --git a/src/lib/auth-file.ts b/src/lib/auth-file.ts index 27f8f31c4..9e7149c2d 100644 --- a/src/lib/auth-file.ts +++ b/src/lib/auth-file.ts @@ -28,11 +28,6 @@ export interface AuthProfile { hasRefreshToken: boolean; /** Reserved: a keyring failure on one profile must not redirect another profile's reads. */ secretsBackend?: CredentialsBackend; - /** - * When this account last logged in, or `null` for one migrated from the pre-profile file. - * Written but unread: `auth list` orders by it, and a logout falls back to the most recent - * profile left. Neither exists yet, and neither can backfill a time nobody recorded. - */ loggedInAt: string | null; } 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 15/33] 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 16/33] 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 17/33] 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 18/33] 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 19/33] 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); From 100c39c57dd5b0d709805ce47d5c79a744c7f9ba Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Richard=20Sol=C3=A1r?= Date: Tue, 29 Sep 2026 10:34:16 +0200 Subject: [PATCH 20/33] refactor: drop the secrets backend markers from auth.json The keyring is used unless APIFY_DISABLE_KEYRING=1 is set or the module cannot load. A token stored in the profile is the only record that its secrets live in the file, so a past keyring failure no longer pins later logins to plaintext, and a successful keyring write clears the file copies. Co-Authored-By: Claude Opus 5.5 --- src/commands/auth/login.ts | 4 +- src/lib/auth-file.ts | 38 +++---- src/lib/auth.ts | 25 ++--- src/lib/credentials.ts | 109 ++++++++------------ test/local/commands/auth.test.ts | 6 +- test/local/lib/auth-file.test.ts | 16 ++- test/local/lib/credentials.test.ts | 157 ++++++++++++++--------------- 7 files changed, 156 insertions(+), 199 deletions(-) diff --git a/src/commands/auth/login.ts b/src/commands/auth/login.ts index 959c99c2c..a1bf2aea4 100644 --- a/src/commands/auth/login.ts +++ b/src/commands/auth/login.ts @@ -13,7 +13,7 @@ import { ApifyCommand } from '../../lib/command-framework/apify-command.js'; import { Flags } from '../../lib/command-framework/flags.js'; import { getConsoleIntegrationsUrl, getConsoleUrl } from '../../lib/console-url.js'; import { AUTH_FILE_PATH, CommandExitCodes } from '../../lib/consts.js'; -import { getBackend } from '../../lib/credentials.js'; +import { backendFor } from '../../lib/credentials.js'; import { updateUserId } from '../../lib/hooks/telemetry/useTelemetryState.js'; import { useMaskedInput } from '../../lib/hooks/user-confirmations/useMaskedInput.js'; import { useSelectFromList } from '../../lib/hooks/user-confirmations/useSelectFromList.js'; @@ -36,7 +36,7 @@ const tryToLogin = async (token: string) => { const { userInfo } = result; await updateUserId(userInfo.id!); - const backend = await getBackend(); + const backend = await backendFor(userInfo.id!); let tokenLocation: string; if (backend === 'keyring') { tokenLocation = 'your OS keyring'; diff --git a/src/lib/auth-file.ts b/src/lib/auth-file.ts index 88bc59d92..ef5425b03 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, SecretKind } from './credentials.js'; +import type { SecretKind } from './credentials.js'; import { ensureApifyDirectory } from './files.js'; import { warning } from './outputs.js'; import { cliDebugPrint } from './utils/cliDebugPrint.js'; @@ -26,14 +26,11 @@ export interface AuthProfile { authMethod: 'token'; expiresAt: string | null; hasRefreshToken: boolean; + loggedInAt: string | null; /** - * 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. + * Set only when the keyring is disabled, unavailable, or refused the write. A token here is + * the record of where this profile's secrets live: no marker says so separately. */ - secretsBackend?: CredentialsBackend; - loggedInAt: string | null; - /** File backend only. The keyring backend keeps these in the OS store instead. */ token?: string; proxy?: { password?: string }; } @@ -46,7 +43,6 @@ export interface AuthFile { version?: number; activeProfile?: string; profiles?: Record; - secretsBackend?: CredentialsBackend; token?: string; proxy?: { password?: string; [k: string]: unknown }; } @@ -141,7 +137,6 @@ function toV2(file: LegacyAuthFile): AuthFile { migrated.profiles![file.id] = v1Profile(file); } - if (file.secretsBackend) migrated.secretsBackend = file.secretsBackend; if (typeof file.token === 'string') migrated.token = file.token; if (typeof file.proxy?.password === 'string') migrated.proxy = { password: file.proxy.password }; @@ -283,11 +278,6 @@ 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. @@ -296,15 +286,14 @@ export function writeProfileSecret(userId: string, kind: SecretKind, value: stri 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) => { - setProfileSecret(profile, kind, value); - profile.secretsBackend = 'file'; +/** Forgets every file-backend secret of a profile, once its token is in the keyring. */ +export function clearProfileFileSecrets(userId: string) { + const profile = readAuthFile().profiles?.[userId]; + if (profile?.token === undefined && profile?.proxy === undefined) return; + + updateProfile(userId, (edited) => { + delete edited.token; + delete edited.proxy; }); } @@ -345,7 +334,7 @@ function updateProfile(userId: string, edit: (profile: AuthProfile) => void) { * 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) { +export function replaceStoredAccount(userId: string, profile: AuthProfile) { assertSupportedAuthFileVersion(); // The snapshot described the account being replaced, and is never refreshed, so keeping it @@ -356,7 +345,6 @@ export function replaceStoredAccount(userId: string, profile: AuthProfile, secre version: AUTH_FILE_VERSION, activeProfile: userId, profiles: { [userId]: profile }, - secretsBackend, }); } diff --git a/src/lib/auth.ts b/src/lib/auth.ts index 7085797e3..5ca240106 100644 --- a/src/lib/auth.ts +++ b/src/lib/auth.ts @@ -13,7 +13,6 @@ import { deleteSecret, ensureMigrated, ensureSecretsKeyed, - getBackend, getSecret, setSecret, } from './credentials.js'; @@ -70,7 +69,7 @@ export function __resetAuthForTests() { * The single token resolver. Order: `APIFY_TOKEN` -> stored login. Inside a platform run there is * no stored login, so `APIFY_TOKEN` wins without a special case for the `actor` entrypoint. * - * Single-flighted like {@link getBackend}, because several callers resolve per command and reading + * Single-flighted like `getBackend()`, because several callers resolve per command and reading * the stored token is an uncached OS keyring hit. * * Read-only by contract, apart from the one-shot migration of an existing plaintext auth.json. @@ -183,19 +182,15 @@ export async function loginWithToken( const previousUserId = getActiveProfileId(); const { organizationOwnerUserId } = userInfo as { organizationOwnerUserId?: string }; - replaceStoredAccount( - userInfo.id, - { - username: userInfo.username, - name: null, - ...(organizationOwnerUserId ? { organizationOwnerUserId } : {}), - authMethod: 'token', - expiresAt: null, - hasRefreshToken: false, - loggedInAt: new Date().toISOString(), - }, - await getBackend(), - ); + replaceStoredAccount(userInfo.id, { + username: userInfo.username, + name: null, + ...(organizationOwnerUserId ? { organizationOwnerUserId } : {}), + authMethod: 'token', + expiresAt: null, + hasRefreshToken: false, + loggedInAt: new Date().toISOString(), + }); // 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) { diff --git a/src/lib/credentials.ts b/src/lib/credentials.ts index 38d495f43..179d58a92 100644 --- a/src/lib/credentials.ts +++ b/src/lib/credentials.ts @@ -3,10 +3,9 @@ import process from 'node:process'; import type { AuthFile } from './auth-file.js'; import { AUTH_FILE_VERSION, + clearProfileFileSecrets, deleteProfileSecret, - moveProfileSecretToFile, readAuthFile, - readProfileBackend, readProfileSecret, writeAuthFile, writeProfileSecret, @@ -103,12 +102,9 @@ async function importKeyringModule(): Promise { } /** - * Picks a backend the first time it's called and caches the result for the rest of the process. - * 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. + * Where new secrets go: the keyring, unless APIFY_DISABLE_KEYRING=1 is set or the keyring module + * cannot load. Nothing on disk overrides it, so a past keyring failure never pins a later write to + * the file. Cached for the process; single-flight so concurrent callers share the lookup. * * 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, and a failure falls back to the file. @@ -118,23 +114,12 @@ export async function getBackend(): Promise { backendPromise = (async (): Promise => { if (process.env.APIFY_DISABLE_KEYRING === '1') return 'file'; - const marker = readAuthFile().secretsBackend; - if (marker === 'file') return 'file'; const mod = await loadKeyringModule(); return mod ? 'keyring' : 'file'; })(); return backendPromise; } -/** - * 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'); -} - /** * Remove the proxy password, keeping any sibling field like `groups` and dropping `proxy` * entirely when the secret was all it carried. @@ -182,22 +167,32 @@ 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. + * Where one account's secrets live. A token in `auth.json` means the file, and the account's other + * secrets follow it there; otherwise the keyring, unless it is disabled or unavailable. Decided by + * the token alone, so a keyring account never looks up a proxy password in the file first. */ -async function backendFor(userId: string): Promise { - return readProfileBackend(userId) ?? (await getBackend()); +export async function backendFor(userId: string): Promise { + if (readProfileSecret(userId, 'token') !== undefined) return 'file'; + return getBackend(); } -/** One account's secret of the given kind, from whichever backend holds it. */ +/** + * One account's secret of the given kind, from whichever backend holds it. A keyring miss falls + * back to the file, where a secret lands when its own keyring write failed after the token's + * succeeded. + */ export async function getSecret(userId: string, kind: SecretKind): Promise { - if ((await backendFor(userId)) === 'keyring') return readKeyring(keyringKey(userId, kind)); + if ((await backendFor(userId)) === 'keyring') { + return (await readKeyring(keyringKey(userId, kind))) ?? readProfileSecret(userId, kind); + } return readProfileSecret(userId, kind); } /** - * 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. + * Persist one account's secret. A token goes wherever {@link getBackend} says now, so a re-login + * moves an account back to the keyring once it works again; the other secrets follow the token. + * When `skipIfUnchanged` is true and the stored value already matches in that place, the write is + * skipped. This avoids macOS Keychain prompts on every command. */ export async function setSecret( userId: string, @@ -205,19 +200,18 @@ export async function setSecret( value: string, opts: { skipIfUnchanged?: boolean } = {}, ): Promise { - const backend = await backendFor(userId); - if (opts.skipIfUnchanged && (await getSecret(userId, kind)) === value) return; + const current = await backendFor(userId); + const target = kind === 'token' ? await getBackend() : current; + if (opts.skipIfUnchanged && current === target && (await getSecret(userId, kind)) === value) return; - if (backend === 'keyring') { + if (target === 'keyring') { try { await writeKeyring(keyringKey(userId, kind), value); + // The file copies are what would send reads there, so they go once the keyring holds the token. + if (kind === 'token') clearProfileFileSecrets(userId); 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); - moveProfileSecretToFile(userId, kind, value); - return; } } @@ -230,11 +224,7 @@ export async function setSecret( * that replaces everything else. */ export async function deleteSecret(userId: string, kind: SecretKind): Promise { - if ((await backendFor(userId)) === 'keyring') { - await deleteKeyring(keyringKey(userId, kind)); - return; - } - + if ((await backendFor(userId)) === 'keyring') await deleteKeyring(keyringKey(userId, kind)); deleteProfileSecret(userId, kind); } @@ -256,13 +246,12 @@ export async function clearKeyringSecrets(userId?: string): Promise { } /** - * One-shot, idempotent migration of legacy plaintext auth.json to the keyring. + * Moves plaintext secrets at the top level of a v1 auth.json into the keyring. * - * Both the API token and the proxy password are moved into the keyring on the keyring backend. - * - * - `secretsBackend` marker in auth.json makes re-entry a no-op. - * - On `file` backend the marker is written but secrets stay in auth.json. - * - On `keyring` backend the token and proxy password are moved out of auth.json. + * - No top-level secret means there is nothing to do, which makes re-entry a no-op. + * - On the `file` backend, or when the keyring write fails, the secrets stay where they are, and + * `ensureSecretsKeyed()` moves them into the profile, after which this has nothing to do. + * - A `secretsBackend` marker from an older CLI is ignored and dropped by the shape migration. * - Wrapped in try/catch so a migration failure never blocks the CLI. */ export async function ensureMigrated(): Promise { @@ -273,15 +262,8 @@ export async function ensureMigrated(): Promise { // A file a newer CLI wrote is not ours to rewrite, and this runs before the shape // migration reports it. if (typeof file.version === 'number' && file.version > AUTH_FILE_VERSION) return; - if (file.secretsBackend) return; if (!file.token && !file.proxy?.password) return; - - const backend = await getBackend(); - if (backend === 'file') { - file.secretsBackend = 'file'; - writeAuthFile(file); - return; - } + if ((await getBackend()) === 'file') return; try { if (file.token) await writeKeyring(legacyKeyringKey('token'), file.token); @@ -289,16 +271,12 @@ export async function ensureMigrated(): Promise { await writeKeyring(legacyKeyringKey('proxy-password'), file.proxy.password); } } catch (err) { - cliDebugPrint('credentials', 'keyring write failed during migration; falling back to file', err); - downgradeBackendToFile(); - file.secretsBackend = 'file'; - writeAuthFile(file); + cliDebugPrint('credentials', 'keyring write failed during migration; keeping secrets in the file', err); return; } delete file.token; stripProxyPassword(file); - file.secretsBackend = 'keyring'; writeAuthFile(file); } catch (err) { cliDebugPrint('credentials', 'migration failed', err); @@ -331,8 +309,8 @@ async function keyKeyringSecrets(userId: string): Promise { const value = await readKeyring(legacy); if (value === undefined) continue; - // 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. + // A failure earlier in this loop put the token in 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); @@ -345,7 +323,7 @@ async function keyKeyringSecrets(userId: string): Promise { } } - moveProfileSecretToFile(userId, kind, value); + writeProfileSecret(userId, kind, value); if (readProfileSecret(userId, kind) === value) await deleteKeyring(legacy); } } @@ -362,7 +340,6 @@ 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; writeAuthFile(file); @@ -390,12 +367,10 @@ export async function ensureSecretsKeyed(): Promise { return; } - if ((await backendFor(userId)) === 'keyring') { - await keyKeyringSecrets(userId); - return; - } - + // Top-level secrets are already in the file, whichever backend is current, so they move into + // the profile either way; that is also what stops a failed keyring move from being retried. keyFileSecrets(userId, file); + if ((await backendFor(userId)) === 'keyring') await keyKeyringSecrets(userId); } catch (err) { cliDebugPrint('credentials', 'keying secrets by user failed', err); } diff --git a/test/local/commands/auth.test.ts b/test/local/commands/auth.test.ts index e678ba951..358f1642d 100644 --- a/test/local/commands/auth.test.ts +++ b/test/local/commands/auth.test.ts @@ -47,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, secretsBackend: 'file' }); + expect(readAuthFile().version).toBe(2); + expect(readAuthFile()).not.toHaveProperty('secretsBackend'); expect(readAuthFile().token).toBeUndefined(); expect(readActiveProfile()).toEqual({ id: 'uid', @@ -195,7 +196,8 @@ describe('auth commands', () => { expect(keyringStore.get(PROXY_PASSWORD_KEY)).toBe('pw'); const authFile = readAuthFile(); - expect(authFile).toMatchObject({ version: 2, secretsBackend: 'keyring' }); + expect(authFile.version).toBe(2); + expect(authFile).not.toHaveProperty('secretsBackend'); expect(authFile.token).toBeUndefined(); // Proxy groups are not a secret, but nothing reads them either. expect(authFile).not.toHaveProperty('proxy'); diff --git a/test/local/lib/auth-file.test.ts b/test/local/lib/auth-file.test.ts index ec1029357..31444e134 100644 --- a/test/local/lib/auth-file.test.ts +++ b/test/local/lib/auth-file.test.ts @@ -65,7 +65,6 @@ describe('auth.json v2', () => { version: 2, activeProfile: 'uid', profiles: { uid: V2_PROFILE }, - secretsBackend: 'file', token: 'apify_api_v1_token', proxy: { password: 'pw' }, }); @@ -79,7 +78,8 @@ describe('auth.json v2', () => { await ensureSecretsKeyed(); - expect(readAuthFile()).toMatchObject({ version: 2, secretsBackend: 'file' }); + expect(readAuthFile()).toMatchObject({ version: 2 }); + expect(readAuthFile()).not.toHaveProperty('secretsBackend'); 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'); @@ -225,7 +225,7 @@ describe('auth.json v2', () => { await ensureSecretsKeyed(); // 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(readAuthFile()).toEqual({ version: 2, profiles: {} }); expect(readBackup()).toEqual({ secretsBackend: 'file' }); await expect(getLocalUserInfo()).resolves.toEqual({}); }); @@ -274,7 +274,7 @@ describe('auth.json v2', () => { const newer = { version: 3, activeProfile: 'uid', profiles: { uid: { username: 'me' } } }; write(newer); - expect(() => replaceStoredAccount('uid2', V2_PROFILE, 'file')).toThrow('written by a newer Apify CLI'); + expect(() => replaceStoredAccount('uid2', V2_PROFILE)).toThrow('written by a newer Apify CLI'); expect(readAuthFile()).toEqual(newer); }); @@ -305,7 +305,7 @@ describe('auth.json v2', () => { proxy: { password: 'old_pw' }, }); - replaceStoredAccount('new', { ...V2_PROFILE, username: 'new' }, 'file'); + replaceStoredAccount('new', { ...V2_PROFILE, username: 'new' }); const file = readAuthFile(); expect(Object.keys(file.profiles!)).toEqual(['new']); @@ -324,7 +324,7 @@ describe('auth.json v2', () => { token: 'apify_api_old', }); - replaceStoredAccount('new', { ...V2_PROFILE, username: 'new' }, 'file'); + replaceStoredAccount('new', { ...V2_PROFILE, username: 'new' }); // Logged out, rather than logged in as the account that just went away. await expect(getSecret('new', 'token')).resolves.toBeUndefined(); @@ -337,7 +337,7 @@ describe('auth.json v2', () => { await ensureAuthFileCurrent(); expect(existsSync(AUTH_BACKUP_FILE_PATH())).toBe(true); - replaceStoredAccount('other', { ...V2_PROFILE, username: 'other' }, 'file'); + replaceStoredAccount('other', { ...V2_PROFILE, username: 'other' }); // It described the previous account and is never refreshed, so it must not survive. expect(existsSync(AUTH_BACKUP_FILE_PATH())).toBe(false); @@ -393,7 +393,6 @@ describe('auth.json v2', () => { version: 2, activeProfile: 'uid', profiles: { uid: V2_PROFILE }, - secretsBackend: 'keyring', }); expect(await getLocalUserInfo()).toEqual({ id: 'uid', @@ -415,7 +414,6 @@ describe('auth.json v2', () => { version: 2, activeProfile: 'uid', profiles: { uid: V2_PROFILE }, - secretsBackend: 'keyring', }); }); }); diff --git a/test/local/lib/credentials.test.ts b/test/local/lib/credentials.test.ts index a620d2a92..8b2556ce5 100644 --- a/test/local/lib/credentials.test.ts +++ b/test/local/lib/credentials.test.ts @@ -5,7 +5,7 @@ import process from 'node:process'; import { cryptoRandomObjectId } from '@apify/utilities'; import { __resetAuthFileForTests } from '../../../src/lib/auth-file.js'; -import { resolveAuth } from '../../../src/lib/auth.js'; +import { __resetAuthForTests, resolveAuth } from '../../../src/lib/auth.js'; import { AUTH_FILE_PATH, GLOBAL_CONFIGS_FOLDER } from '../../../src/lib/consts.js'; import { __resetCredentialsForTests, @@ -86,10 +86,10 @@ describe('credentials', () => { expect(await getBackend()).toBe('keyring'); }); - it('returns "file" when auth.json carries the marker, even if the keyring loads', async () => { + it('ignores a file marker an older CLI left in auth.json', async () => { vitest.stubEnv('APIFY_DISABLE_KEYRING', ''); writeAuthFile({ token: 'tok', secretsBackend: 'file' }); - expect(await getBackend()).toBe('file'); + expect(await getBackend()).toBe('keyring'); }); it('caches the backend choice for the rest of the process', async () => { @@ -103,7 +103,7 @@ describe('credentials', () => { describe('file backend', () => { beforeEach(() => { vitest.stubEnv('APIFY_DISABLE_KEYRING', '1'); - writeV2AuthFile({}, { secretsBackend: 'file' }); + writeV2AuthFile(); writeFileSyncSpy.mockClear(); }); @@ -112,8 +112,6 @@ describe('credentials', () => { expect(await getSecret(TEST_USER_ID, 'token')).toBe('tok_123'); expect(readProfile().token).toBe('tok_123'); expect(readAuthFile().token).toBeUndefined(); - // 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 () => { @@ -234,22 +232,33 @@ describe('credentials', () => { }); it('falls back to the profile when the keyring token write fails', async () => { - writeV2AuthFile({}, { secretsBackend: 'keyring' }); + writeV2AuthFile(); keyringFailures.add(TOKEN_KEY); await setSecret(TEST_USER_ID, 'token', 'tok_123'); expect(keyringStore.get(TOKEN_KEY)).toBeUndefined(); expect(readProfile().token).toBe('tok_123'); - // 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'); + // The token in the file is the only record; no marker is written. + expect(readProfile()).not.toHaveProperty('secretsBackend'); + expect(readAuthFile()).not.toHaveProperty('secretsBackend'); expect(await getBackend()).toBe('keyring'); expect(await getSecret(TEST_USER_ID, 'token')).toBe('tok_123'); }); + it('moves the token back to the keyring once a write there succeeds', async () => { + writeV2AuthFile({ token: 'tok_file', proxy: { password: 'pw_file' } }); + + await setSecret(TEST_USER_ID, 'token', 'tok_file', { skipIfUnchanged: true }); + + // Unchanged in value, but in the wrong place, so the write is not skipped. + expect(keyringStore.get(TOKEN_KEY)).toBe('tok_file'); + expect(readProfile()).not.toHaveProperty('token'); + expect(readProfile()).not.toHaveProperty('proxy'); + expect(await getSecret(TEST_USER_ID, 'token')).toBe('tok_file'); + }); + it('keeps using auth.json for later writes after a keyring failure', async () => { - writeV2AuthFile({}, { secretsBackend: 'keyring' }); + writeV2AuthFile(); keyringFailures.add(TOKEN_KEY); await setSecret(TEST_USER_ID, 'token', 'tok_123'); @@ -259,7 +268,7 @@ describe('credentials', () => { }); it('leaves another profile on the keyring after one profile falls back', async () => { - const file = v2AuthFile({}, { secretsBackend: 'keyring' }); + const file = v2AuthFile(); file.profiles!.other = { ...file.profiles![TEST_USER_ID] }; writeAuthFile(file as Record); keyringFailures.add(TOKEN_KEY); @@ -267,20 +276,22 @@ describe('credentials', () => { await setSecret(TEST_USER_ID, 'token', 'tok_123'); await setSecret('other', 'token', 'tok_other'); - expect(readAuthFile().profiles.other.secretsBackend).toBeUndefined(); + expect(readAuthFile().profiles.other).not.toHaveProperty('token'); 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({}, { secretsBackend: 'keyring' }); + writeV2AuthFile(); + await setSecret(TEST_USER_ID, 'token', 'tok_123'); 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'); + // The token stays in the keyring; only the proxy password is read from the file. + expect(await getSecret(TEST_USER_ID, 'token')).toBe('tok_123'); expect(await getSecret(TEST_USER_ID, 'proxy-password')).toBe('pw_abc'); }); }); @@ -330,22 +341,15 @@ describe('credentials', () => { writeAuthFile({ username: 'me', id: 'uid', token: 'tok_legacy' }); expect((await resolveAuth())?.token).toBe('tok_legacy'); - expect(readAuthFile().secretsBackend).toBe('file'); + expect(readAuthFile()).not.toHaveProperty('secretsBackend'); }); - it('is a no-op when secretsBackend marker is already set', async () => { - vitest.stubEnv('APIFY_DISABLE_KEYRING', '1'); - writeAuthFile({ token: 'tok', secretsBackend: 'file' }); - await ensureMigrated(); - expect(readAuthFile().token).toBe('tok'); - }); - - it('is a no-op when the marker says keyring and secrets are still in auth.json', async () => { + it('ignores a marker an older CLI left and moves the secrets to the keyring', async () => { vitest.stubEnv('APIFY_DISABLE_KEYRING', ''); - writeAuthFile({ token: 'tok', proxy: { password: 'pw' }, secretsBackend: 'keyring' }); + writeAuthFile({ token: 'tok', proxy: { password: 'pw' }, secretsBackend: 'file' }); await ensureMigrated(); - expect(keyringStore.get(LEGACY_KEYRING_TOKEN_KEY)).toBeUndefined(); - expect(readAuthFile()).toEqual({ token: 'tok', proxy: { password: 'pw' }, secretsBackend: 'keyring' }); + expect(keyringStore.get(LEGACY_KEYRING_TOKEN_KEY)).toBe('tok'); + expect(readAuthFile().token).toBeUndefined(); }); it('is a no-op when there are no secrets to migrate', async () => { @@ -354,14 +358,13 @@ describe('credentials', () => { expect(existsSync(AUTH_FILE_PATH())).toBe(false); }); - it('on the file backend, stamps the marker without moving data', async () => { + it('on the file backend, leaves the file untouched', async () => { vitest.stubEnv('APIFY_DISABLE_KEYRING', '1'); writeAuthFile({ token: 'tok', username: 'u' }); + writeFileSyncSpy.mockClear(); await ensureMigrated(); - const file = readAuthFile(); - expect(file.token).toBe('tok'); - expect(file.username).toBe('u'); - expect(file.secretsBackend).toBe('file'); + expect(authFileWrites()).toHaveLength(0); + expect(readAuthFile()).toEqual({ token: 'tok', username: 'u' }); }); it('on the keyring backend, moves the token and proxy password out of auth.json', async () => { @@ -370,11 +373,7 @@ describe('credentials', () => { await ensureMigrated(); 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(); - expect(file.username).toBe('u'); - expect(file.secretsBackend).toBe('keyring'); + expect(readAuthFile()).toEqual({ username: 'u' }); }); it('on the keyring backend, strips only the proxy password and keeps other proxy fields', async () => { @@ -382,9 +381,7 @@ describe('credentials', () => { writeAuthFile({ token: 'tok', proxy: { password: 'pw', groups: [{ name: 'g' }] }, username: 'u' }); await ensureMigrated(); 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'); + expect(readAuthFile().proxy).toEqual({ groups: [{ name: 'g' }] }); }); it('migrates proxy password to the keyring when token is absent', async () => { @@ -392,50 +389,44 @@ describe('credentials', () => { writeAuthFile({ proxy: { password: 'pw' }, username: 'u' }); await ensureMigrated(); expect(keyringStore.get(LEGACY_KEYRING_PROXY_PASSWORD_KEY)).toBe('pw'); - const file = readAuthFile(); - expect(file.proxy).toBeUndefined(); - expect(file.username).toBe('u'); - expect(file.secretsBackend).toBe('keyring'); + expect(readAuthFile()).toEqual({ username: 'u' }); }); - it('stamps the file marker for a proxy-only state on the file backend', async () => { - vitest.stubEnv('APIFY_DISABLE_KEYRING', '1'); - writeAuthFile({ proxy: { password: 'pw' } }); - await ensureMigrated(); - const file = readAuthFile(); - expect(file.secretsBackend).toBe('file'); - expect(file.proxy?.password).toBe('pw'); - }); - - it('falls back to file backend when the proxy keyring write fails after token succeeds', async () => { + it('keeps the secrets in the file when a keyring write fails', async () => { vitest.stubEnv('APIFY_DISABLE_KEYRING', ''); keyringFailures.add(LEGACY_KEYRING_PROXY_PASSWORD_KEY); writeAuthFile({ token: 'tok', proxy: { password: 'pw' }, username: 'u' }); await ensureMigrated(); - const file = readAuthFile(); - expect(file.secretsBackend).toBe('file'); - expect(file.token).toBe('tok'); - expect(file.proxy?.password).toBe('pw'); - expect(file.username).toBe('u'); + expect(readAuthFile()).toEqual({ token: 'tok', proxy: { password: 'pw' }, username: 'u' }); + }); + + it('a failed keyring move ends up in the profile, so the next command does not retry it', async () => { + vitest.stubEnv('APIFY_DISABLE_KEYRING', ''); + keyringFailures.add(LEGACY_KEYRING_TOKEN_KEY); + writeAuthFile({ username: 'me', id: 'uid', token: 'tok' }); + __resetAuthForTests(); + + expect((await resolveAuth())?.token).toBe('tok'); + expect(readAuthFile()).not.toHaveProperty('token'); + expect(readProfile().token).toBe('tok'); }); it('is memoized within a process', async () => { - vitest.stubEnv('APIFY_DISABLE_KEYRING', '1'); + vitest.stubEnv('APIFY_DISABLE_KEYRING', ''); writeAuthFile({ token: 'tok' }); await ensureMigrated(); - expect(readAuthFile().secretsBackend).toBe('file'); + expect(keyringStore.get(LEGACY_KEYRING_TOKEN_KEY)).toBe('tok'); - // Overwrite the marker and call again — the memoized promise should short-circuit. writeAuthFile({ token: 'tok2' }); await ensureMigrated(); - expect(readAuthFile().secretsBackend).toBeUndefined(); + expect(readAuthFile().token).toBe('tok2'); }); }); describe('ensureSecretsKeyed()', () => { it('moves keyring entries off the fixed names onto the user ID', async () => { vitest.stubEnv('APIFY_DISABLE_KEYRING', ''); - writeV2AuthFile({}, { secretsBackend: 'keyring' }); + writeV2AuthFile(); keyringStore.set(LEGACY_KEYRING_TOKEN_KEY, 'tok'); keyringStore.set(LEGACY_KEYRING_PROXY_PASSWORD_KEY, 'pw'); @@ -449,7 +440,7 @@ describe('credentials', () => { it('moves top-level file secrets into the profile', async () => { vitest.stubEnv('APIFY_DISABLE_KEYRING', '1'); - writeV2AuthFile({}, { secretsBackend: 'file', token: 'tok', proxy: { password: 'pw' } }); + writeV2AuthFile({}, { token: 'tok', proxy: { password: 'pw' } }); await ensureSecretsKeyed(); @@ -457,12 +448,22 @@ describe('credentials', () => { const file = readAuthFile(); expect(file.token).toBeUndefined(); expect(file.proxy).toBeUndefined(); - expect(file.secretsBackend).toBe('file'); + }); + + it('moves top-level file secrets into the profile on the keyring backend too', async () => { + vitest.stubEnv('APIFY_DISABLE_KEYRING', ''); + writeV2AuthFile({}, { token: 'tok', proxy: { password: 'pw' } }); + + await ensureSecretsKeyed(); + + expect(readProfile()).toMatchObject({ token: 'tok', proxy: { password: 'pw' } }); + expect(readAuthFile().token).toBeUndefined(); + expect(await getSecret(TEST_USER_ID, 'token')).toBe('tok'); }); 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' }); + writeAuthFile({ version: 2, profiles: {}, token: 'tok' }); keyringStore.set(LEGACY_KEYRING_TOKEN_KEY, 'tok_kr'); await ensureSecretsKeyed(); @@ -473,7 +474,7 @@ describe('credentials', () => { it('drops the legacy entries even under APIFY_DISABLE_KEYRING=1', async () => { vitest.stubEnv('APIFY_DISABLE_KEYRING', '1'); - writeAuthFile({ version: 2, profiles: {}, secretsBackend: 'keyring' }); + writeAuthFile({ version: 2, profiles: {} }); keyringStore.set(LEGACY_KEYRING_TOKEN_KEY, 'tok_kr'); await ensureSecretsKeyed(); @@ -483,7 +484,7 @@ describe('credentials', () => { 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' }); + writeV2AuthFile({ token: 'tok' }); writeFileSyncSpy.mockClear(); await ensureSecretsKeyed(); @@ -505,7 +506,7 @@ describe('credentials', () => { it('moves the profile to the file when the keyring write fails mid-migration', async () => { vitest.stubEnv('APIFY_DISABLE_KEYRING', ''); - writeV2AuthFile({}, { secretsBackend: 'keyring' }); + writeV2AuthFile(); keyringStore.set(LEGACY_KEYRING_TOKEN_KEY, 'tok'); keyringStore.set(LEGACY_KEYRING_PROXY_PASSWORD_KEY, 'pw'); keyringFailures.add(TOKEN_KEY); @@ -513,18 +514,17 @@ describe('credentials', () => { await ensureSecretsKeyed(); // 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(readProfile()).toMatchObject({ token: 'tok', proxy: { password: 'pw' } }); expect(keyringStore.size).toBe(0); }); it('is memoized within a process', async () => { vitest.stubEnv('APIFY_DISABLE_KEYRING', '1'); - writeV2AuthFile({}, { secretsBackend: 'file', token: 'tok' }); + writeV2AuthFile({}, { token: 'tok' }); await ensureSecretsKeyed(); expect(readProfile().token).toBe('tok'); - writeV2AuthFile({}, { secretsBackend: 'file', token: 'tok2' }); + writeV2AuthFile({}, { token: 'tok2' }); await ensureSecretsKeyed(); expect(readAuthFile().token).toBe('tok2'); }); @@ -533,7 +533,7 @@ describe('credentials', () => { 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' }); + writeV2AuthFile({ token: 'tok', proxy: { password: 'pw' } }); const info = await getLocalUserInfo(); expect(info.token).toBe('tok'); @@ -547,7 +547,6 @@ describe('credentials', () => { id: 'uid', token: 'tok', proxy: { password: 'pw', groups: [{ name: 'g' }] }, - secretsBackend: 'file', }); const info = await getLocalUserInfo(); expect(info.proxy).toEqual({ password: 'pw' }); @@ -557,7 +556,7 @@ describe('credentials', () => { vitest.stubEnv('APIFY_DISABLE_KEYRING', ''); keyringStore.set(LEGACY_KEYRING_TOKEN_KEY, 'tok_kr'); keyringStore.set(LEGACY_KEYRING_PROXY_PASSWORD_KEY, 'pw_kr'); - writeAuthFile({ username: 'me', id: 'uid', secretsBackend: 'keyring' }); + writeAuthFile({ username: 'me', id: 'uid' }); const info = await getLocalUserInfo(); expect(info.token).toBe('tok_kr'); expect(info.proxy?.password).toBe('pw_kr'); @@ -570,7 +569,7 @@ describe('credentials', () => { 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' }); + writeAuthFile({ token: 'tok' }); expect(await getLocalUserInfo()).toEqual({}); // The secret is dropped rather than left unreachable, so the next command asks for a login. From 82eebf3e1a93ce76b33fd6ca8ee89bc770d91971 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Richard=20Sol=C3=A1r?= Date: Wed, 30 Sep 2026 11:52:38 +0200 Subject: [PATCH 21/33] fix: stop the secret migration overwriting a newer login `keyKeyringSecrets()` copied a legacy keyring entry onto the keyed name without checking what was already there. A login runs before the migration does, so an upgraded user who logged in first had the new token replaced by the old one on their next command. Skip the copy when the account already has a secret of that kind, and delete the legacy entry instead. Co-Authored-By: Claude Opus 5 --- src/lib/credentials.ts | 10 +++++++++- test/local/lib/credentials.test.ts | 23 +++++++++++++++++++++++ 2 files changed, 32 insertions(+), 1 deletion(-) diff --git a/src/lib/credentials.ts b/src/lib/credentials.ts index 179d58a92..c92b2f737 100644 --- a/src/lib/credentials.ts +++ b/src/lib/credentials.ts @@ -301,7 +301,8 @@ async function dropUnkeyedSecrets(file: AuthFile): Promise { /** * 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. + * secret when the delete succeeds and the write does not. A kind the account already has is left + * alone, so the migration never restores a value something newer replaced. */ async function keyKeyringSecrets(userId: string): Promise { for (const kind of SECRET_KINDS) { @@ -309,6 +310,13 @@ async function keyKeyringSecrets(userId: string): Promise { const value = await readKeyring(legacy); if (value === undefined) continue; + // A login between the upgrade and this migration already stored this kind, and the legacy + // entry it left behind is the older value. + if ((await getSecret(userId, kind)) !== undefined) { + await deleteKeyring(legacy); + continue; + } + // A failure earlier in this loop put the token in the file, so the secrets after it belong // there too rather than under a keyring name nothing will read. if ((await backendFor(userId)) === 'keyring') { diff --git a/test/local/lib/credentials.test.ts b/test/local/lib/credentials.test.ts index 8b2556ce5..cd5c030a3 100644 --- a/test/local/lib/credentials.test.ts +++ b/test/local/lib/credentials.test.ts @@ -518,6 +518,29 @@ describe('credentials', () => { expect(keyringStore.size).toBe(0); }); + it('keeps a keyed entry a legacy entry would overwrite', async () => { + vitest.stubEnv('APIFY_DISABLE_KEYRING', ''); + writeV2AuthFile(); + keyringStore.set(LEGACY_KEYRING_TOKEN_KEY, 'tok_old'); + keyringStore.set(TOKEN_KEY, 'tok_new'); + + await ensureSecretsKeyed(); + + expect(keyringStore.get(TOKEN_KEY)).toBe('tok_new'); + expect(keyringStore.get(LEGACY_KEYRING_TOKEN_KEY)).toBeUndefined(); + }); + + it('leaves a login that ran before it alone', async () => { + vitest.stubEnv('APIFY_DISABLE_KEYRING', ''); + writeV2AuthFile(); + keyringStore.set(LEGACY_KEYRING_TOKEN_KEY, 'tok_old'); + + await setSecret(TEST_USER_ID, 'token', 'tok_new'); + await ensureSecretsKeyed(); + + expect(await getSecret(TEST_USER_ID, 'token')).toBe('tok_new'); + }); + it('is memoized within a process', async () => { vitest.stubEnv('APIFY_DISABLE_KEYRING', '1'); writeV2AuthFile({}, { token: 'tok' }); From 201a95816130be06821710636e908c7a21384867 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Richard=20Sol=C3=A1r?= Date: Wed, 30 Sep 2026 17:03:49 +0200 Subject: [PATCH 22/33] refactor: name the credential migration order The three migrations run in a fixed order, because keying secrets by user needs the ID the shape migration produces. That order was typed out at both call sites and documented in a comment. Co-Authored-By: Claude Opus 5 --- src/lib/credentials.ts | 17 +++++++++++++++++ src/lib/utils.ts | 8 +++----- 2 files changed, 20 insertions(+), 5 deletions(-) diff --git a/src/lib/credentials.ts b/src/lib/credentials.ts index c92b2f737..c51189889 100644 --- a/src/lib/credentials.ts +++ b/src/lib/credentials.ts @@ -5,6 +5,7 @@ import { AUTH_FILE_VERSION, clearProfileFileSecrets, deleteProfileSecret, + ensureAuthFileCurrent, readAuthFile, readProfileSecret, writeAuthFile, @@ -386,3 +387,19 @@ export async function ensureSecretsKeyed(): Promise { return keyingPromise; } + +/** + * Brings the stored credentials to their current form: the plaintext secrets into the keyring, the + * file into its current shape, then the secrets onto keys that carry the user ID. The order is a + * dependency chain — keying by user needs the user ID the shape migration produces. + * + * Every reader calls this before it reads. `loginWithToken()` does not: it replaces the file + * wholesale, so there is nothing to bring forward, and it clears the old keyring names itself. + * + * Each step is single-flight and never throws, so repeat calls cost nothing. + */ +export async function ensureCredentialsCurrent(): Promise { + await ensureMigrated(); + await ensureAuthFileCurrent(); + await ensureSecretsKeyed(); +} diff --git a/src/lib/utils.ts b/src/lib/utils.ts index 08bcb94a1..a5bcade11 100644 --- a/src/lib/utils.ts +++ b/src/lib/utils.ts @@ -32,7 +32,7 @@ import { SOURCE_FILE_FORMATS, } from '@apify/consts'; -import { ensureAuthFileCurrent, lookUpActiveProfile } from './auth-file.js'; +import { lookUpActiveProfile } from './auth-file.js'; import { describeAuthFailure, getApifyClientOptionsForToken, resolveAuth, type ResolvedAuth } from './auth.js'; import { AUTH_FILE_PATH, @@ -42,7 +42,7 @@ import { MINIMUM_SUPPORTED_PYTHON_VERSION, SUPPORTED_NODEJS_VERSION, } from './consts.js'; -import { ensureMigrated, ensureSecretsKeyed, getSecret } from './credentials.js'; +import { ensureCredentialsCurrent, 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'; @@ -90,9 +90,7 @@ export const getLocalRequestQueuePath = (storeId?: string) => { * stored. Secrets come from whichever backend holds them; the metadata comes from auth.json. */ export const getLocalUserInfo = async (): Promise => { - await ensureMigrated(); - await ensureAuthFileCurrent(); - await ensureSecretsKeyed(); + await ensureCredentialsCurrent(); const { profile, missingProfile } = lookUpActiveProfile(); From ffb31817b613f506118b0ff13fca4490d9c6b7f6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Richard=20Sol=C3=A1r?= Date: Wed, 30 Sep 2026 17:03:56 +0200 Subject: [PATCH 23/33] fix: clear the old keyring names on every login A login writes its secrets under keyed names, so anything left under the fixed names is stale. It survived a repeat login of the same account, and the next migration cannot tell it from a current secret: a proxy password the account had dropped came back. Co-Authored-By: Claude Opus 5 --- src/lib/auth.ts | 16 ++++++++-------- test/local/lib/auth.test.ts | 28 ++++++++++++++++++++++++++-- 2 files changed, 34 insertions(+), 10 deletions(-) diff --git a/src/lib/auth.ts b/src/lib/auth.ts index 5ca240106..32973d7c9 100644 --- a/src/lib/auth.ts +++ b/src/lib/auth.ts @@ -6,13 +6,13 @@ import { AxiosHeaders } from 'axios'; import { APIFY_ENV_VARS } from '@apify/consts'; -import { ensureAuthFileCurrent, getActiveProfileId, replaceStoredAccount } from './auth-file.js'; +import { getActiveProfileId, replaceStoredAccount } from './auth-file.js'; import { APIFY_CLIENT_DEFAULT_HEADERS, AUTH_FILE_PATH, CommandExitCodes } from './consts.js'; import { clearKeyringSecrets, deleteSecret, + ensureCredentialsCurrent, ensureMigrated, - ensureSecretsKeyed, getSecret, setSecret, } from './credentials.js'; @@ -97,8 +97,7 @@ 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(); + await ensureCredentialsCurrent(); const userId = getActiveProfileId(); const storedToken = userId ? await getSecret(userId, 'token') : undefined; @@ -192,10 +191,11 @@ export async function loginWithToken( loggedInAt: new Date().toISOString(), }); - // 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); - } + // Only once the switch is on disk: a failed write leaves auth.json naming the previous account, + // whose entries nothing else can find. The fixed-name entries go on every login, even a repeat + // of the same account: the secrets below are written under keyed names, so whatever is left + // under the old names is stale, and the next migration cannot tell it from a current secret. + await clearKeyringSecrets(previousUserId === userInfo.id ? undefined : 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/lib/auth.test.ts b/test/local/lib/auth.test.ts index 0ce3d90f1..e365308c2 100644 --- a/test/local/lib/auth.test.ts +++ b/test/local/lib/auth.test.ts @@ -4,12 +4,15 @@ import { ApifyApiError } from 'apify-client'; import { loginWithToken, resolveAuth } from '../../../src/lib/auth.js'; import { AUTH_FILE_PATH, CommandExitCodes } from '../../../src/lib/consts.js'; -import { getSecret } from '../../../src/lib/credentials.js'; +import { __resetCredentialsForTests, ensureSecretsKeyed, 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'; -import { useAuthSetup } from '../../__setup__/hooks/useAuthSetup.js'; +import { useAuthSetup, useKeyringBackend } from '../../__setup__/hooks/useAuthSetup.js'; import { useConsoleSpy } from '../../__setup__/hooks/useConsoleSpy.js'; +import { LEGACY_KEYRING_PROXY_PASSWORD_KEY, keyringStore, resetKeyringMock } from '../../__setup__/keyring-mock.js'; + +vi.mock('@napi-rs/keyring', () => import('../../__setup__/keyring-mock.js')); vi.mock('apify-client', async (importOriginal) => ({ ...(await importOriginal()), @@ -153,6 +156,27 @@ describe('auth', () => { }); }); + describe('loginWithToken() on the keyring backend', () => { + useKeyringBackend(); + + beforeEach(() => { + resetKeyringMock(); + }); + + it('does not resurrect a proxy password the account no longer has', async () => { + resetApifyClientMock({ id: 'uid', username: 'me' }); + keyringStore.set(LEGACY_KEYRING_PROXY_PASSWORD_KEY, 'pw_old'); + + await loginWithToken(STORED); + + // The migration next runs in a fresh process, with nothing memoized. + __resetCredentialsForTests(); + await ensureSecretsKeyed(); + + expect(await getSecret('uid', 'proxy-password')).toBeUndefined(); + }); + }); + describe('getLoggedClientOrThrow()', () => { afterEach(() => { // The command framework reads this; leaving it set would fail the vitest run. From a6c17e11d9bb5e2191ccbab8cd0509c64878c174 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Richard=20Sol=C3=A1r?= Date: Wed, 30 Sep 2026 17:12:18 +0200 Subject: [PATCH 24/33] fix: report keyring deletes that logout could not make `deleteKeyring()` swallowed every error, so `clearKeyringSecrets()` could not reject and logout's failure branch was unreachable. On a locked keyring, logout said the secrets were removed and exited 0, while deleting the only index of what the keyring holds. Every key is still attempted, and login warns rather than failing when it cannot remove the previous account's entries. Co-Authored-By: Claude Opus 5 --- src/lib/auth.ts | 16 ++++++++++++++- src/lib/credentials.ts | 21 +++++++++++++++---- test/__setup__/keyring-mock.ts | 1 + test/local/commands/auth.test.ts | 35 +++++++++++++++++++++++++++++++- 4 files changed, 67 insertions(+), 6 deletions(-) diff --git a/src/lib/auth.ts b/src/lib/auth.ts index 32973d7c9..9699a848c 100644 --- a/src/lib/auth.ts +++ b/src/lib/auth.ts @@ -195,7 +195,21 @@ export async function loginWithToken( // whose entries nothing else can find. The fixed-name entries go on every login, even a repeat // of the same account: the secrets below are written under keyed names, so whatever is left // under the old names is stale, and the next migration cannot tell it from a current secret. - await clearKeyringSecrets(previousUserId === userInfo.id ? undefined : previousUserId); + const staleUserId = previousUserId === userInfo.id ? undefined : previousUserId; + try { + await clearKeyringSecrets(staleUserId); + } catch (err) { + // The login itself succeeded, so it goes through. The entries left behind are the previous + // account's, and nothing reads them under the new one. + cliDebugPrint('[loginWithToken] clearing the previous keyring entries failed', { error: err }); + if (staleUserId) { + warning({ + message: + `Your previous secrets are still in the OS keyring under the account ${staleUserId}; ` + + `delete them with your OS keyring app.`, + }); + } + } // After the account, which drops the previous secrets. `skipIfUnchanged` avoids a Keychain prompt. await setSecret(userInfo.id, 'token', token, { skipIfUnchanged: true }); diff --git a/src/lib/credentials.ts b/src/lib/credentials.ts index c51189889..668e37440 100644 --- a/src/lib/credentials.ts +++ b/src/lib/credentials.ts @@ -157,13 +157,16 @@ async function writeKeyring(key: KeyringKey, value: string): Promise { entry.setPassword(value); } -async function deleteKeyring(key: KeyringKey): Promise { +/** Returns what the delete failed with, or null. Callers that cannot act on it ignore it. */ +async function deleteKeyring(key: KeyringKey): Promise { try { const entry = await getKeyringEntry(key); - if (!entry) return; + if (!entry) return null; entry.deletePassword(); + return null; } catch (err) { cliDebugPrint('credentials', `failed to delete ${key.service}/${key.account} from keyring`, err); + return err; } } @@ -238,11 +241,21 @@ export async function deleteSecret(userId: string, kind: SecretKind): Promise { + const failures: unknown[] = []; + for (const kind of SECRET_KINDS) { - if (userId) await deleteKeyring(keyringKey(userId, kind)); - await deleteKeyring(legacyKeyringKey(kind)); + if (userId) failures.push(await deleteKeyring(keyringKey(userId, kind))); + failures.push(await deleteKeyring(legacyKeyringKey(kind))); + } + + // Every key is attempted before this: one entry the keyring refuses must not strand the rest. + const failed = failures.filter((err) => err !== null); + if (failed.length) { + throw new AggregateError(failed, failed.map((err) => (err instanceof Error ? err.message : String(err))).join(' ')); } } diff --git a/test/__setup__/keyring-mock.ts b/test/__setup__/keyring-mock.ts index 6bfdf53cb..a6cd2cbf9 100644 --- a/test/__setup__/keyring-mock.ts +++ b/test/__setup__/keyring-mock.ts @@ -38,6 +38,7 @@ export class Entry { } deletePassword(): boolean { + if (keyringFailures.has(this.key)) throw new Error('simulated keyring failure'); return keyringStore.delete(this.key); } } diff --git a/test/local/commands/auth.test.ts b/test/local/commands/auth.test.ts index 358f1642d..531d571ca 100644 --- a/test/local/commands/auth.test.ts +++ b/test/local/commands/auth.test.ts @@ -8,6 +8,7 @@ import { readActiveProfile, readAuthFile } from '../../__setup__/auth-file.js'; import { useAuthSetup, useKeyringBackend } from '../../__setup__/hooks/useAuthSetup.js'; import { useConsoleSpy } from '../../__setup__/hooks/useConsoleSpy.js'; import { + keyringFailures, keyringProxyPasswordKey, keyringSetKeys, keyringStore, @@ -26,7 +27,7 @@ vi.mock('apify-client', async (importOriginal) => ({ })); useAuthSetup(); -const { lastLogMessage, lastErrorMessage } = useConsoleSpy(); +const { lastLogMessage, lastErrorMessage, logMessages } = useConsoleSpy(); const { AuthLoginCommand } = await import('../../../src/commands/auth/login.js'); const { AuthLogoutCommand } = await import('../../../src/commands/auth/logout.js'); @@ -284,6 +285,38 @@ describe('auth commands', () => { }, ); + it('login warns when the previous account entries cannot be removed', async () => { + await login(); + clientState.user = { id: 'uid2', username: 'other' }; + keyringFailures.add(TOKEN_KEY); + + await login('apify_api_other_token'); + + expect(readActiveProfile()).toMatchObject({ id: 'uid2' }); + expect([...logMessages.log, ...logMessages.error].join('\n')).toContain( + 'Your previous secrets are still in the OS keyring under the account uid', + ); + }); + + // Exiting 0 with a success line told the user the keyring was clear while the token was + // still in it, and auth.json, the only index of what it holds, was gone. + it('logout says so when the keyring cannot be cleared', async () => { + await login(); + keyringFailures.add(TOKEN_KEY); + + try { + await testRunCommand(AuthLogoutCommand, {}); + + expect(keyringStore.get(TOKEN_KEY)).toBe(TOKEN); + expect(lastErrorMessage()).toContain('Logout did not finish'); + expect(lastErrorMessage()).toContain('Your secrets are still in the OS keyring under the account uid'); + expect(lastErrorMessage()).toContain(`Your account was removed from ${AUTH_FILE_PATH()}`); + expect(process.exitCode).toBe(CommandExitCodes.RunFailed); + } finally { + 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 () => { From c8e41d4dd632f57ff3e8354a416129231108f48c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Richard=20Sol=C3=A1r?= Date: Wed, 30 Sep 2026 17:17:25 +0200 Subject: [PATCH 25/33] fix: point the API tests at the per-user secret helpers `getToken()` and `getProxyPassword()` are gone. Vitest resolved the missing imports to undefined instead of failing, so the suite died at the call. Co-Authored-By: Claude Opus 5 --- test/api/commands/log_in_out.test.ts | 6 +++--- test/local/commands/run.test.ts | 16 +++++++++------- 2 files changed, 12 insertions(+), 10 deletions(-) diff --git a/test/api/commands/log_in_out.test.ts b/test/api/commands/log_in_out.test.ts index d86998f92..d19856822 100644 --- a/test/api/commands/log_in_out.test.ts +++ b/test/api/commands/log_in_out.test.ts @@ -4,7 +4,7 @@ import axios from 'axios'; import { testRunCommand } from '../../../src/lib/command-framework/apify-command.js'; import { AUTH_FILE_PATH } from '../../../src/lib/consts.js'; -import { getToken } from '../../../src/lib/credentials.js'; +import { getSecret } from '../../../src/lib/credentials.js'; import { readActiveProfile } from '../../__setup__/auth-file.js'; import { TEST_USER_BAD_TOKEN, TEST_USER_TOKEN, testUserClient } from '../../__setup__/config.js'; import { safeLogin, useAuthSetup } from '../../__setup__/hooks/useAuthSetup.js'; @@ -41,7 +41,7 @@ describe('[api] apify login and logout', () => { id: expectedUserInfo.id, username: expectedUserInfo.username, }); - expect(await getToken()).to.eql(TEST_USER_TOKEN); + expect(await getSecret(expectedUserInfo.id, 'token')).to.eql(TEST_USER_TOKEN); await testRunCommand(LogoutCommand, {}); const isGlobalConfig = existsSync(AUTH_FILE_PATH()); @@ -77,6 +77,6 @@ describe('[api] apify login and logout', () => { id: expectedUserInfo.id, username: expectedUserInfo.username, }); - expect(await getToken()).to.eql(TEST_USER_TOKEN); + expect(await getSecret(expectedUserInfo.id, 'token')).to.eql(TEST_USER_TOKEN); }); }); diff --git a/test/local/commands/run.test.ts b/test/local/commands/run.test.ts index 0811f67b4..765ec0af0 100644 --- a/test/local/commands/run.test.ts +++ b/test/local/commands/run.test.ts @@ -5,7 +5,7 @@ import { ACTOR_ENV_VARS, APIFY_ENV_VARS } from '@apify/consts'; import { testRunCommand } from '../../../src/lib/command-framework/apify-command.js'; import { EMPTY_LOCAL_CONFIG, LOCAL_CONFIG_PATH } from '../../../src/lib/consts.js'; -import { getProxyPassword, getToken } from '../../../src/lib/credentials.js'; +import { getSecret, type SecretKind } from '../../../src/lib/credentials.js'; import { rimrafPromised } from '../../../src/lib/files.js'; import { getLocalDatasetPath, @@ -18,6 +18,8 @@ import { TEST_TIMEOUT } from '../../__setup__/consts.js'; import { safeLogin, useAuthSetup } from '../../__setup__/hooks/useAuthSetup.js'; import { useConsoleSpy } from '../../__setup__/hooks/useConsoleSpy.js'; import { useTempPath } from '../../__setup__/hooks/useTempPath.js'; + +const storedSecret = (kind: SecretKind) => getSecret(readActiveProfile()!.id, kind); import { defaultsInputSchemaPath, missingRequiredPropertyInputSchemaPath, @@ -127,9 +129,9 @@ describe('apify run', () => { const localEnvVars = JSON.parse(readFileSync(actOutputPath, 'utf8')); // Read from disk, not through getLocalUserInfo: `run` sources these from that same // function, so asserting against it would only prove it agrees with itself. - expect(localEnvVars[APIFY_ENV_VARS.PROXY_PASSWORD]).toStrictEqual(await getProxyPassword()); + expect(localEnvVars[APIFY_ENV_VARS.PROXY_PASSWORD]).toStrictEqual(await storedSecret('proxy-password')); expect(localEnvVars[APIFY_ENV_VARS.USER_ID]).toStrictEqual(readActiveProfile()!.id); - expect(localEnvVars[APIFY_ENV_VARS.TOKEN]).toStrictEqual(await getToken()); + expect(localEnvVars[APIFY_ENV_VARS.TOKEN]).toStrictEqual(await storedSecret('token')); expect(localEnvVars.TEST_LOCAL).toStrictEqual(testEnvVars.TEST_LOCAL); }); @@ -166,9 +168,9 @@ describe('apify run', () => { const actOutputPath = joinPath(getLocalKeyValueStorePath(), 'OUTPUT.json'); const localEnvVars = JSON.parse(readFileSync(actOutputPath, 'utf8')); - expect(localEnvVars[APIFY_ENV_VARS.PROXY_PASSWORD]).toStrictEqual(await getProxyPassword()); + expect(localEnvVars[APIFY_ENV_VARS.PROXY_PASSWORD]).toStrictEqual(await storedSecret('proxy-password')); expect(localEnvVars[APIFY_ENV_VARS.USER_ID]).toStrictEqual(readActiveProfile()!.id); - expect(localEnvVars[APIFY_ENV_VARS.TOKEN]).toStrictEqual(await getToken()); + expect(localEnvVars[APIFY_ENV_VARS.TOKEN]).toStrictEqual(await storedSecret('token')); expect(localEnvVars.TEST_LOCAL).toStrictEqual(testEnvVars.TEST_LOCAL); const actOutputPath2 = joinPath(getLocalKeyValueStorePath(), 'owo.json'); @@ -204,9 +206,9 @@ describe('apify run', () => { const actOutputPath = joinPath(getLocalKeyValueStorePath(), 'OUTPUT.json'); const localEnvVars = JSON.parse(readFileSync(actOutputPath, 'utf8')); - expect(localEnvVars[APIFY_ENV_VARS.PROXY_PASSWORD]).toStrictEqual(await getProxyPassword()); + expect(localEnvVars[APIFY_ENV_VARS.PROXY_PASSWORD]).toStrictEqual(await storedSecret('proxy-password')); expect(localEnvVars[APIFY_ENV_VARS.USER_ID]).toStrictEqual(readActiveProfile()!.id); - expect(localEnvVars[APIFY_ENV_VARS.TOKEN]).toStrictEqual(await getToken()); + expect(localEnvVars[APIFY_ENV_VARS.TOKEN]).toStrictEqual(await storedSecret('token')); expect(localEnvVars.TEST_LOCAL).toStrictEqual(testEnvVars.TEST_LOCAL); const actOutputPath2 = joinPath(getLocalKeyValueStorePath(), 'two.json'); From ad2ced9c55b054096b771cd80422303702ac2204 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Richard=20Sol=C3=A1r?= Date: Wed, 30 Sep 2026 17:19:33 +0200 Subject: [PATCH 26/33] fix: move the other secrets with a token that falls back to the file `backendFor()` sends every read for an account to the file once its token is there, so a proxy password left in the keyring could not be read. A local run lost its proxy password until the next login. Co-Authored-By: Claude Opus 5 --- src/lib/credentials.ts | 21 +++++++++++++++++++-- test/local/lib/credentials.test.ts | 15 +++++++++++++++ 2 files changed, 34 insertions(+), 2 deletions(-) diff --git a/src/lib/credentials.ts b/src/lib/credentials.ts index 668e37440..6d8a93c1d 100644 --- a/src/lib/credentials.ts +++ b/src/lib/credentials.ts @@ -172,8 +172,9 @@ async function deleteKeyring(key: KeyringKey): Promise { /** * Where one account's secrets live. A token in `auth.json` means the file, and the account's other - * secrets follow it there; otherwise the keyring, unless it is disabled or unavailable. Decided by - * the token alone, so a keyring account never looks up a proxy password in the file first. + * secrets are moved there with it; otherwise the keyring, unless it is disabled or unavailable. + * Decided by the token alone, so a keyring account never looks up a proxy password in the file + * first. */ export async function backendFor(userId: string): Promise { if (readProfileSecret(userId, 'token') !== undefined) return 'file'; @@ -216,12 +217,28 @@ export async function setSecret( return; } catch (err) { cliDebugPrint('credentials', 'keyring write failed; falling back to file', err); + // The token is about to land in the file, which is where reads go from now on. The rest + // follow it, or the keyring copies become unreachable. + if (kind === 'token') await moveKeyringSecretsToFile(userId); } } writeProfileSecret(userId, kind, value); } +/** Every secret but the token, which the caller writes next. */ +async function moveKeyringSecretsToFile(userId: string): Promise { + for (const kind of SECRET_KINDS) { + if (kind === 'token') continue; + + const value = await readKeyring(keyringKey(userId, kind)); + if (value === undefined) continue; + + writeProfileSecret(userId, kind, value); + if (readProfileSecret(userId, kind) === value) await deleteKeyring(keyringKey(userId, kind)); + } +} + /** * 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 diff --git a/test/local/lib/credentials.test.ts b/test/local/lib/credentials.test.ts index cd5c030a3..806a35675 100644 --- a/test/local/lib/credentials.test.ts +++ b/test/local/lib/credentials.test.ts @@ -9,6 +9,7 @@ import { __resetAuthForTests, resolveAuth } from '../../../src/lib/auth.js'; import { AUTH_FILE_PATH, GLOBAL_CONFIGS_FOLDER } from '../../../src/lib/consts.js'; import { __resetCredentialsForTests, + backendFor, clearKeyringSecrets, deleteSecret, ensureMigrated, @@ -257,6 +258,20 @@ describe('credentials', () => { expect(await getSecret(TEST_USER_ID, 'token')).toBe('tok_file'); }); + it('brings the proxy password down when the token falls back to the file', async () => { + writeV2AuthFile(); + await setSecret(TEST_USER_ID, 'token', 'tok_1'); + await setSecret(TEST_USER_ID, 'proxy-password', 'pw_abc'); + + keyringFailures.add(TOKEN_KEY); + await setSecret(TEST_USER_ID, 'token', 'tok_2'); + + expect(await backendFor(TEST_USER_ID)).toBe('file'); + expect(await getSecret(TEST_USER_ID, 'token')).toBe('tok_2'); + expect(await getSecret(TEST_USER_ID, 'proxy-password')).toBe('pw_abc'); + expect(keyringStore.get(PROXY_PASSWORD_KEY)).toBeUndefined(); + }); + it('keeps using auth.json for later writes after a keyring failure', async () => { writeV2AuthFile(); keyringFailures.add(TOKEN_KEY); From 0ba785fb0f53a734b69458e388d3224b12327bc9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Richard=20Sol=C3=A1r?= Date: Wed, 30 Sep 2026 22:45:59 +0200 Subject: [PATCH 27/33] fix: report only the keyring secrets a delete left behind The keyring module loads on machines where the secret service does not answer, so every entry throws although nothing was ever stored. Logout took that for a refused delete and exited 1, telling the user to clean a keyring they do not have, after removing their account cleanly. Co-Authored-By: Claude Opus 5 --- src/lib/credentials.ts | 11 +++++++++-- test/local/commands/auth.test.ts | 25 ++++++++++++++++++++++++- 2 files changed, 33 insertions(+), 3 deletions(-) diff --git a/src/lib/credentials.ts b/src/lib/credentials.ts index 6d8a93c1d..748a6dfcf 100644 --- a/src/lib/credentials.ts +++ b/src/lib/credentials.ts @@ -157,7 +157,14 @@ async function writeKeyring(key: KeyringKey, value: string): Promise { entry.setPassword(value); } -/** Returns what the delete failed with, or null. Callers that cannot act on it ignore it. */ +/** + * Returns what the delete failed with, or null. Callers that cannot act on it ignore it. + * + * Only a secret that still reads back is reported. The module loads on machines with no secret + * service, where every entry throws although nothing was ever stored, and a caller acting on that + * would tell the user to clean a keyring they do not have. A keyring that can neither delete nor + * read is silent for the same reason, which is the cost of not crying wolf on every such machine. + */ async function deleteKeyring(key: KeyringKey): Promise { try { const entry = await getKeyringEntry(key); @@ -166,7 +173,7 @@ async function deleteKeyring(key: KeyringKey): Promise { return null; } catch (err) { cliDebugPrint('credentials', `failed to delete ${key.service}/${key.account} from keyring`, err); - return err; + return (await readKeyring(key)) === undefined ? null : err; } } diff --git a/test/local/commands/auth.test.ts b/test/local/commands/auth.test.ts index 531d571ca..7c7d1e062 100644 --- a/test/local/commands/auth.test.ts +++ b/test/local/commands/auth.test.ts @@ -1,13 +1,16 @@ import { chmodSync, existsSync, statSync } from 'node:fs'; import process from 'node:process'; +import { __resetAuthFileForTests } from '../../../src/lib/auth-file.js'; import { AUTH_FILE_PATH, CommandExitCodes, GLOBAL_CONFIGS_FOLDER } from '../../../src/lib/consts.js'; -import { getSecret } from '../../../src/lib/credentials.js'; +import { __resetCredentialsForTests, ensureSecretsKeyed, 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 { + LEGACY_KEYRING_PROXY_PASSWORD_KEY, + LEGACY_KEYRING_TOKEN_KEY, keyringFailures, keyringProxyPasswordKey, keyringSetKeys, @@ -298,6 +301,26 @@ describe('auth commands', () => { ); }); + // The keyring module loads on machines where the secret service does not answer, so a + // delete that throws there is not a secret left behind: nothing was ever stored. + it('logout succeeds when the keyring answers nothing', async () => { + for (const key of [TOKEN_KEY, PROXY_PASSWORD_KEY, LEGACY_KEYRING_TOKEN_KEY, LEGACY_KEYRING_PROXY_PASSWORD_KEY]) { + keyringFailures.add(key); + } + + try { + await login(); + expect(keyringStore.size).toBe(0); + + await testRunCommand(AuthLogoutCommand, {}); + + expect(existsSync(AUTH_FILE_PATH())).toBe(false); + expect(process.exitCode).not.toBe(CommandExitCodes.RunFailed); + } finally { + process.exitCode = 0; + } + }); + // Exiting 0 with a success line told the user the keyring was clear while the token was // still in it, and auth.json, the only index of what it holds, was gone. it('logout says so when the keyring cannot be cleared', async () => { From e5548673f8804b00d6e4ea6525d997c0172cc611 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Richard=20Sol=C3=A1r?= Date: Wed, 30 Sep 2026 22:47:02 +0200 Subject: [PATCH 28/33] fix: stop one account claiming another account keyring secret A keyed token means a login already stored this account's secrets under the new names, so the fixed names hold whatever a previous login left. The per-kind guard could not see that: a login clears the proxy password it has none of, and the migration then read the previous account's under the new one. Co-Authored-By: Claude Opus 5 --- src/lib/auth.ts | 5 +++-- src/lib/credentials.ts | 15 ++++++++++----- test/local/commands/auth.test.ts | 17 +++++++++++++++++ 3 files changed, 30 insertions(+), 7 deletions(-) diff --git a/src/lib/auth.ts b/src/lib/auth.ts index 9699a848c..50e291446 100644 --- a/src/lib/auth.ts +++ b/src/lib/auth.ts @@ -199,8 +199,9 @@ export async function loginWithToken( try { await clearKeyringSecrets(staleUserId); } catch (err) { - // The login itself succeeded, so it goes through. The entries left behind are the previous - // account's, and nothing reads them under the new one. + // The login itself succeeded, so it goes through. What is left behind stays unreadable: + // `keyKeyringSecrets` claims nothing under the fixed names for an account that already has + // a keyed token, which this login is about to write. cliDebugPrint('[loginWithToken] clearing the previous keyring entries failed', { error: err }); if (staleUserId) { warning({ diff --git a/src/lib/credentials.ts b/src/lib/credentials.ts index 748a6dfcf..a546d2df8 100644 --- a/src/lib/credentials.ts +++ b/src/lib/credentials.ts @@ -339,18 +339,23 @@ async function dropUnkeyedSecrets(file: AuthFile): Promise { /** * 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. A kind the account already has is left - * alone, so the migration never restores a value something newer replaced. + * secret when the delete succeeds and the write does not. Nothing the account already has is + * touched, so the migration never restores a value something newer replaced. */ async function keyKeyringSecrets(userId: string): Promise { + // A keyed token means a login already wrote this account's secrets under the new names. The + // fixed names are then whatever a previous login left, which may be another account's, so + // nothing under them is claimed for this one. + const claimable = (await readKeyring(keyringKey(userId, 'token'))) === undefined; + for (const kind of SECRET_KINDS) { const legacy = legacyKeyringKey(kind); const value = await readKeyring(legacy); if (value === undefined) continue; - // A login between the upgrade and this migration already stored this kind, and the legacy - // entry it left behind is the older value. - if ((await getSecret(userId, kind)) !== undefined) { + // The second test covers the kinds a login stores directly; the first covers the kinds it + // leaves empty, which nothing else would tell apart from never having been set. + if (!claimable || (await getSecret(userId, kind)) !== undefined) { await deleteKeyring(legacy); continue; } diff --git a/test/local/commands/auth.test.ts b/test/local/commands/auth.test.ts index 7c7d1e062..296e71da0 100644 --- a/test/local/commands/auth.test.ts +++ b/test/local/commands/auth.test.ts @@ -301,6 +301,23 @@ describe('auth commands', () => { ); }); + it('does not hand one account the previous account secret', async () => { + keyringStore.set(LEGACY_KEYRING_PROXY_PASSWORD_KEY, 'pw_of_uid'); + // The keyring refuses that delete, so login cannot clear it. + keyringFailures.add(LEGACY_KEYRING_PROXY_PASSWORD_KEY); + + await login(); + clientState.user = { id: 'uid2', username: 'other' }; + await login('apify_api_other_token'); + + // The migration next runs in a fresh process, with nothing memoized. + __resetCredentialsForTests(); + __resetAuthFileForTests(); + await ensureSecretsKeyed(); + + expect(await getSecret('uid2', 'proxy-password')).toBeUndefined(); + }); + // The keyring module loads on machines where the secret service does not answer, so a // delete that throws there is not a secret left behind: nothing was ever stored. it('logout succeeds when the keyring answers nothing', async () => { From b14fbd1681864e8fe292c63c36b31f617ee9ec22 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Richard=20Sol=C3=A1r?= Date: Wed, 30 Sep 2026 22:58:44 +0200 Subject: [PATCH 29/33] perf: read the keyed token only when a fixed name holds something The check for whether the fixed names are this account's ran on every command. On a keyed account they hold nothing, so it never decided anything. Co-Authored-By: Claude Opus 5 --- src/lib/credentials.ts | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/lib/credentials.ts b/src/lib/credentials.ts index a546d2df8..a2d7ba2ae 100644 --- a/src/lib/credentials.ts +++ b/src/lib/credentials.ts @@ -345,14 +345,17 @@ async function dropUnkeyedSecrets(file: AuthFile): Promise { async function keyKeyringSecrets(userId: string): Promise { // A keyed token means a login already wrote this account's secrets under the new names. The // fixed names are then whatever a previous login left, which may be another account's, so - // nothing under them is claimed for this one. - const claimable = (await readKeyring(keyringKey(userId, 'token'))) === undefined; + // nothing under them is claimed for this one. Read once, and only once a fixed name turns + // something up, which on a keyed account is never. + let claimable: boolean | undefined; for (const kind of SECRET_KINDS) { const legacy = legacyKeyringKey(kind); const value = await readKeyring(legacy); if (value === undefined) continue; + claimable ??= (await readKeyring(keyringKey(userId, 'token'))) === undefined; + // The second test covers the kinds a login stores directly; the first covers the kinds it // leaves empty, which nothing else would tell apart from never having been set. if (!claimable || (await getSecret(userId, kind)) !== undefined) { From 49d90737383ed67a378c095d78ae3566ce10d933 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Richard=20Sol=C3=A1r?= Date: Thu, 1 Oct 2026 01:23:35 +0200 Subject: [PATCH 30/33] fix: name the keyring entries a logout or login left behind `clearKeyringSecrets` threw an AggregateError of bare messages, so neither caller knew which entry survived. Logout named the account it had just cleared successfully and never named the entry that was still there. Login said nothing at all when the account had not changed, leaving a live token in the keyring with nothing to ever mention it. It now returns the surviving entries. Nothing wanted an exception: both callers turned it straight back into a value. Logout also keeps its report of APIFY_TOKEN and clears the telemetry ID when the profile is gone, which the early return used to skip. Co-Authored-By: Claude Opus 5 --- src/commands/auth/logout.ts | 42 +++++++++++++----------- src/lib/auth.ts | 25 +++++++-------- src/lib/credentials.ts | 46 ++++++++++++++++---------- test/local/commands/auth.test.ts | 55 ++++++++++++++++++++++++++++---- 4 files changed, 113 insertions(+), 55 deletions(-) diff --git a/src/commands/auth/logout.ts b/src/commands/auth/logout.ts index e7593068e..bb8320ec8 100644 --- a/src/commands/auth/logout.ts +++ b/src/commands/auth/logout.ts @@ -6,7 +6,12 @@ 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, CommandExitCodes } from '../../lib/consts.js'; -import { clearKeyringSecrets } from '../../lib/credentials.js'; +import { + clearKeyringSecrets, + describeLeftovers, + type KeyringLeftover, + leftoverReasons, +} from '../../lib/credentials.js'; import { updateUserId } from '../../lib/hooks/telemetry/useTelemetryState.js'; import { error, success, warning } from '../../lib/outputs.js'; import { tildify } from '../../lib/utils.js'; @@ -35,10 +40,7 @@ export class AuthLogoutCommand extends ApifyCommand { // 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, - ); + const leftovers = await clearKeyringSecrets(activeProfileId); let profileError: unknown = null; try { @@ -47,16 +49,18 @@ export class AuthLogoutCommand extends ApifyCommand { profileError = err; } - if (keyringError || profileError) { - error({ message: partialLogoutMessage(activeProfileId, keyringError, profileError) }); + // The account is off disk whenever the profile step succeeded, so the telemetry ID goes too. + if (!profileError) await updateUserId(null); + + if (leftovers.length || profileError) { + error({ message: partialLogoutMessage(leftovers, profileError) }); process.exitCode = CommandExitCodes.RunFailed; - return; + } else { + success({ message: 'You are logged out from your Apify account.' }); } - await updateUserId(null); - - success({ message: 'You are logged out from your Apify account.' }); - + // Said either way: a token in the environment still authenticates every later command, and + // a half-finished logout is when the user most needs to hear it. const envToken = readEnvToken(); if (envToken.kind === 'token') { warning({ @@ -72,16 +76,16 @@ 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.` +function partialLogoutMessage(leftovers: KeyringLeftover[], profileError: unknown) { + const keyringPart = leftovers.length + ? `Your secrets are still in the OS keyring at ${describeLeftovers(leftovers)}; 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()}.`; + ? `Your account is still in ${tildify(AUTH_FILE_PATH())}; delete that file to finish logging out.` + : `Your account was removed from ${tildify(AUTH_FILE_PATH())}.`; - const reasons = [keyringError, profileError].filter(Boolean).map(reasonOf).join(' '); + const reasons = [leftoverReasons(leftovers), profileError ? reasonOf(profileError) : ''].filter(Boolean).join(' '); - return `Logout did not finish. ${keyringPart} ${profilePart} ${reasons}`; + return `Logout did not finish. ${keyringPart} ${profilePart} The reason was: ${reasons}`; } diff --git a/src/lib/auth.ts b/src/lib/auth.ts index 50e291446..bf6023624 100644 --- a/src/lib/auth.ts +++ b/src/lib/auth.ts @@ -11,6 +11,7 @@ import { APIFY_CLIENT_DEFAULT_HEADERS, AUTH_FILE_PATH, CommandExitCodes } from ' import { clearKeyringSecrets, deleteSecret, + describeLeftovers, ensureCredentialsCurrent, ensureMigrated, getSecret, @@ -196,20 +197,16 @@ export async function loginWithToken( // of the same account: the secrets below are written under keyed names, so whatever is left // under the old names is stale, and the next migration cannot tell it from a current secret. const staleUserId = previousUserId === userInfo.id ? undefined : previousUserId; - try { - await clearKeyringSecrets(staleUserId); - } catch (err) { - // The login itself succeeded, so it goes through. What is left behind stays unreadable: - // `keyKeyringSecrets` claims nothing under the fixed names for an account that already has - // a keyed token, which this login is about to write. - cliDebugPrint('[loginWithToken] clearing the previous keyring entries failed', { error: err }); - if (staleUserId) { - warning({ - message: - `Your previous secrets are still in the OS keyring under the account ${staleUserId}; ` + - `delete them with your OS keyring app.`, - }); - } + const leftovers = await clearKeyringSecrets(staleUserId); + + // The login itself succeeded, so it goes through. Said whether or not the account changed: a + // repeat login leaves the same entries behind, and nothing later in the CLI reads or names them. + if (leftovers.length) { + warning({ + message: + `Your previous secrets are still in the OS keyring at ${describeLeftovers(leftovers)}; ` + + `delete them with your OS keyring app.`, + }); } // After the account, which drops the previous secrets. `skipIfUnchanged` avoids a Keychain prompt. diff --git a/src/lib/credentials.ts b/src/lib/credentials.ts index a2d7ba2ae..1d0a2e8e2 100644 --- a/src/lib/credentials.ts +++ b/src/lib/credentials.ts @@ -26,7 +26,7 @@ export type SecretKind = 'token' | 'proxy-password'; const SECRET_KINDS: readonly SecretKind[] = ['token', 'proxy-password']; -interface KeyringKey { +export interface KeyringKey { service: string; account: string; } @@ -45,6 +45,12 @@ function legacyKeyringKey(kind: SecretKind): KeyringKey { return { service: KEYRING_SERVICE, account: kind }; } +/** A secret a delete could not remove, named by where it still is. */ +export interface KeyringLeftover { + key: KeyringKey; + error: unknown; +} + interface KeyringEntry { getPassword(): string | null; setPassword(password: string): void; @@ -158,14 +164,14 @@ async function writeKeyring(key: KeyringKey, value: string): Promise { } /** - * Returns what the delete failed with, or null. Callers that cannot act on it ignore it. + * Returns the secret this left in the keyring, or null. Callers that cannot act on it ignore it. * * Only a secret that still reads back is reported. The module loads on machines with no secret * service, where every entry throws although nothing was ever stored, and a caller acting on that - * would tell the user to clean a keyring they do not have. A keyring that can neither delete nor - * read is silent for the same reason, which is the cost of not crying wolf on every such machine. + * would name a keyring the user does not have. A keyring that can neither delete nor read is + * silent for the same reason, which is the cost of not naming one that was never there. */ -async function deleteKeyring(key: KeyringKey): Promise { +async function deleteKeyring(key: KeyringKey): Promise { try { const entry = await getKeyringEntry(key); if (!entry) return null; @@ -173,7 +179,7 @@ async function deleteKeyring(key: KeyringKey): Promise { return null; } catch (err) { cliDebugPrint('credentials', `failed to delete ${key.service}/${key.account} from keyring`, err); - return (await readKeyring(key)) === undefined ? null : err; + return (await readKeyring(key)) === undefined ? null : { key, error: err }; } } @@ -266,21 +272,29 @@ export async function deleteSecret(userId: string, kind: SecretKind): Promise { - const failures: unknown[] = []; +export async function clearKeyringSecrets(userId?: string): Promise { + const leftovers: (KeyringLeftover | null)[] = []; for (const kind of SECRET_KINDS) { - if (userId) failures.push(await deleteKeyring(keyringKey(userId, kind))); - failures.push(await deleteKeyring(legacyKeyringKey(kind))); + if (userId) leftovers.push(await deleteKeyring(keyringKey(userId, kind))); + leftovers.push(await deleteKeyring(legacyKeyringKey(kind))); } - // Every key is attempted before this: one entry the keyring refuses must not strand the rest. - const failed = failures.filter((err) => err !== null); - if (failed.length) { - throw new AggregateError(failed, failed.map((err) => (err instanceof Error ? err.message : String(err))).join(' ')); - } + return leftovers.filter((leftover) => leftover !== null); +} + +/** Names the entries a failed logout or login left behind, in the words the keyring app shows. */ +export function describeLeftovers(leftovers: KeyringLeftover[]): string { + return leftovers.map(({ key }) => `${key.service}/${key.account}`).join(', '); +} + +/** The reasons behind {@link describeLeftovers}, each said once however many entries share it. */ +export function leftoverReasons(leftovers: KeyringLeftover[]): string { + const reasons = leftovers.map(({ error }) => (error instanceof Error ? error.message : String(error))); + return [...new Set(reasons)].join(' '); } /** diff --git a/test/local/commands/auth.test.ts b/test/local/commands/auth.test.ts index 296e71da0..be41ad1d4 100644 --- a/test/local/commands/auth.test.ts +++ b/test/local/commands/auth.test.ts @@ -1,9 +1,12 @@ import { chmodSync, existsSync, statSync } from 'node:fs'; import process from 'node:process'; +import { APIFY_ENV_VARS } from '@apify/consts'; + import { __resetAuthFileForTests } from '../../../src/lib/auth-file.js'; import { AUTH_FILE_PATH, CommandExitCodes, GLOBAL_CONFIGS_FOLDER } from '../../../src/lib/consts.js'; import { __resetCredentialsForTests, ensureSecretsKeyed, getSecret } from '../../../src/lib/credentials.js'; +import { tildify } from '../../../src/lib/utils.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'; @@ -296,9 +299,9 @@ describe('auth commands', () => { await login('apify_api_other_token'); expect(readActiveProfile()).toMatchObject({ id: 'uid2' }); - expect([...logMessages.log, ...logMessages.error].join('\n')).toContain( - 'Your previous secrets are still in the OS keyring under the account uid', - ); + const printed = [...logMessages.log, ...logMessages.error].join('\n'); + expect(printed).toContain('Your previous secrets are still in the OS keyring at com.apify.cli.token/uid;'); + expect(printed).not.toContain('uid2'); }); it('does not hand one account the previous account secret', async () => { @@ -318,6 +321,46 @@ describe('auth commands', () => { expect(await getSecret('uid2', 'proxy-password')).toBeUndefined(); }); + it('logout names the keyring entry that survived, not the account', async () => { + await login(); + keyringStore.set(LEGACY_KEYRING_TOKEN_KEY, 'tok_legacy'); + keyringFailures.add(LEGACY_KEYRING_TOKEN_KEY); + + try { + await testRunCommand(AuthLogoutCommand, {}); + + expect(lastErrorMessage()).toContain('com.apify.cli/token'); + expect(lastErrorMessage()).not.toContain('under the account uid'); + } finally { + process.exitCode = 0; + } + }); + + it('logout still reports APIFY_TOKEN when a step failed', async () => { + vitest.stubEnv(APIFY_ENV_VARS.TOKEN, 'apify_api_env'); + await login(); + keyringStore.set(LEGACY_KEYRING_TOKEN_KEY, 'tok_legacy'); + keyringFailures.add(LEGACY_KEYRING_TOKEN_KEY); + + try { + await testRunCommand(AuthLogoutCommand, {}); + + expect([...logMessages.log, ...logMessages.error].join('\n')).toContain(`${APIFY_ENV_VARS.TOKEN} is still set`); + } finally { + process.exitCode = 0; + } + }); + + it('login reports a leftover entry when the account did not change', async () => { + await login(); + keyringStore.set(LEGACY_KEYRING_TOKEN_KEY, 'tok_legacy'); + keyringFailures.add(LEGACY_KEYRING_TOKEN_KEY); + + await login(); + + expect([...logMessages.log, ...logMessages.error].join('\n')).toContain('com.apify.cli/token'); + }); + // The keyring module loads on machines where the secret service does not answer, so a // delete that throws there is not a secret left behind: nothing was ever stored. it('logout succeeds when the keyring answers nothing', async () => { @@ -349,8 +392,8 @@ describe('auth commands', () => { expect(keyringStore.get(TOKEN_KEY)).toBe(TOKEN); expect(lastErrorMessage()).toContain('Logout did not finish'); - expect(lastErrorMessage()).toContain('Your secrets are still in the OS keyring under the account uid'); - expect(lastErrorMessage()).toContain(`Your account was removed from ${AUTH_FILE_PATH()}`); + expect(lastErrorMessage()).toContain('Your secrets are still in the OS keyring at com.apify.cli.token/uid;'); + expect(lastErrorMessage()).toContain(`Your account was removed from ${tildify(AUTH_FILE_PATH())}`); expect(process.exitCode).toBe(CommandExitCodes.RunFailed); } finally { process.exitCode = 0; @@ -370,7 +413,7 @@ describe('auth commands', () => { 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(lastErrorMessage()).toContain(`Your account is still in ${tildify(AUTH_FILE_PATH())}`); expect(process.exitCode).toBe(CommandExitCodes.RunFailed); } finally { chmodSync(GLOBAL_CONFIGS_FOLDER(), 0o700); From 74bfe7fa8cf0ed8059281423130ab47cbea72b2a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Richard=20Sol=C3=A1r?= Date: Thu, 1 Oct 2026 11:07:04 +0200 Subject: [PATCH 31/33] fix: report a proxy password the keyring would not give up `deleteSecret()` is what stops a revoked proxy password surviving a re-login, and it was the one delete whose failure nothing reported. The stored value stays, and reads serve it before the file, so every later local run gets a password the account no longer has. It now returns what it left behind, and login folds that into the warning it already prints. Also corrects a claim repeated in four places: `@napi-rs/keyring` does export `findCredentials()`. The CLI choosing not to enumerate is the real reason auth.json is its only index. Co-Authored-By: Claude Opus 5 --- src/lib/auth.ts | 24 +++++++++++++----------- src/lib/credentials.ts | 17 ++++++++++++----- test/local/commands/auth.test.ts | 26 +++++++++++++++++++++++--- test/local/lib/credentials.test.ts | 2 +- 4 files changed, 49 insertions(+), 20 deletions(-) diff --git a/src/lib/auth.ts b/src/lib/auth.ts index bf6023624..e08528753 100644 --- a/src/lib/auth.ts +++ b/src/lib/auth.ts @@ -199,23 +199,25 @@ export async function loginWithToken( const staleUserId = previousUserId === userInfo.id ? undefined : previousUserId; const leftovers = await clearKeyringSecrets(staleUserId); - // The login itself succeeded, so it goes through. Said whether or not the account changed: a - // repeat login leaves the same entries behind, and nothing later in the CLI reads or names them. - if (leftovers.length) { - warning({ - message: - `Your previous secrets are still in the OS keyring at ${describeLeftovers(leftovers)}; ` + - `delete them with your OS keyring app.`, - }); - } - // After the account, which drops the previous secrets. `skipIfUnchanged` avoids a Keychain prompt. await setSecret(userInfo.id, 'token', token, { skipIfUnchanged: true }); if (proxyPassword) { await setSecret(userInfo.id, 'proxy-password', proxyPassword, { skipIfUnchanged: true }); } else { - await deleteSecret(userInfo.id, 'proxy-password'); + // A refused delete leaves the revoked password where every read looks first. + const leftover = await deleteSecret(userInfo.id, 'proxy-password'); + if (leftover) leftovers.push(leftover); + } + + // The login itself succeeded, so it goes through. Said whether or not the account changed: a + // repeat login leaves the same entries behind, and nothing later in the CLI reads or names them. + if (leftovers.length) { + warning({ + message: + `Secrets this login could not remove are still in the OS keyring at ` + + `${describeLeftovers(leftovers)}; delete them with your OS keyring app.`, + }); } return { client: apifyClient, userInfo }; diff --git a/src/lib/credentials.ts b/src/lib/credentials.ts index 1d0a2e8e2..f35b3231f 100644 --- a/src/lib/credentials.ts +++ b/src/lib/credentials.ts @@ -256,10 +256,14 @@ async function moveKeyringSecretsToFile(userId: string): Promise { * 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. + * + * Returns the secret this left behind, or null. A refused delete here is the one that matters + * most: the stored value stays, and reads keep serving it as the account's own. */ -export async function deleteSecret(userId: string, kind: SecretKind): Promise { - if ((await backendFor(userId)) === 'keyring') await deleteKeyring(keyringKey(userId, kind)); +export async function deleteSecret(userId: string, kind: SecretKind): Promise { + const leftover = (await backendFor(userId)) === 'keyring' ? await deleteKeyring(keyringKey(userId, kind)) : null; deleteProfileSecret(userId, kind); + return leftover; } /** @@ -268,9 +272,12 @@ export async function deleteSecret(userId: string, kind: SecretKind): Promise { 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 + // auth.json no longer names uid, and the CLI never enumerates the keyring, 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'); @@ -268,7 +268,7 @@ describe('auth commands', () => { }); // 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. + // CLI never enumerates the keyring, so auth.json is its only index of what it holds. it.skipIf(process.platform === 'win32')( 'a switch that cannot be written keeps the outgoing account entries', async () => { @@ -300,7 +300,9 @@ describe('auth commands', () => { expect(readActiveProfile()).toMatchObject({ id: 'uid2' }); const printed = [...logMessages.log, ...logMessages.error].join('\n'); - expect(printed).toContain('Your previous secrets are still in the OS keyring at com.apify.cli.token/uid;'); + expect(printed).toContain( + 'Secrets this login could not remove are still in the OS keyring at com.apify.cli.token/uid;', + ); expect(printed).not.toContain('uid2'); }); @@ -361,6 +363,24 @@ describe('auth commands', () => { expect([...logMessages.log, ...logMessages.error].join('\n')).toContain('com.apify.cli/token'); }); + // The delete that exists to stop a revoked proxy password surviving a re-login. Its failure + // was the one the login never mentioned. + it('login reports a proxy password it could not remove', async () => { + await login(); + expect(keyringStore.get(PROXY_PASSWORD_KEY)).toBe('pw'); + + // The account loses its proxy password, and the keyring refuses to drop the stored one. + clientState.user = { id: 'uid', username: 'me' }; + keyringFailures.add(PROXY_PASSWORD_KEY); + + await login(); + + expect(keyringStore.get(PROXY_PASSWORD_KEY)).toBe('pw'); + expect([...logMessages.log, ...logMessages.error].join('\n')).toContain( + `still in the OS keyring at ${PROXY_PASSWORD_KEY.replace(':', '/')}`, + ); + }); + // The keyring module loads on machines where the secret service does not answer, so a // delete that throws there is not a secret left behind: nothing was ever stored. it('logout succeeds when the keyring answers nothing', async () => { diff --git a/test/local/lib/credentials.test.ts b/test/local/lib/credentials.test.ts index 806a35675..fbe2be0da 100644 --- a/test/local/lib/credentials.test.ts +++ b/test/local/lib/credentials.test.ts @@ -619,7 +619,7 @@ describe('credentials', () => { 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. + // auth.json is the CLI's only index of the keyring, so a hand-deleted file orphans 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 7135a76e9293f0156c2e0b167215db4368434030 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Richard=20Sol=C3=A1r?= Date: Thu, 1 Oct 2026 11:51:54 +0200 Subject: [PATCH 32/33] docs: correct three comments and cut the rest back MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A comment moved with login's warning and kept describing the narrower set it used to cover: `getSecret` reads the keyring before the file, so the proxy password it now reports is read on every command. `describeLeftovers` does not print what a keyring app shows — Windows and Linux Secret Service both name an entry differently. `ensureCredentialsCurrent` can throw, through `ensureAuthFileCurrent`. And `findCredentials()` throws on a keyutils machine rather than returning nothing. Co-Authored-By: Claude Opus 5 --- src/commands/auth/logout.ts | 3 +-- src/lib/auth.ts | 6 +----- src/lib/credentials.ts | 37 ++++++++++++++----------------------- 3 files changed, 16 insertions(+), 30 deletions(-) diff --git a/src/commands/auth/logout.ts b/src/commands/auth/logout.ts index bb8320ec8..0b7fa8f61 100644 --- a/src/commands/auth/logout.ts +++ b/src/commands/auth/logout.ts @@ -59,8 +59,7 @@ export class AuthLogoutCommand extends ApifyCommand { success({ message: 'You are logged out from your Apify account.' }); } - // Said either way: a token in the environment still authenticates every later command, and - // a half-finished logout is when the user most needs to hear it. + // Said either way: a half-finished logout is when this matters most. const envToken = readEnvToken(); if (envToken.kind === 'token') { warning({ diff --git a/src/lib/auth.ts b/src/lib/auth.ts index e08528753..55e870080 100644 --- a/src/lib/auth.ts +++ b/src/lib/auth.ts @@ -193,9 +193,7 @@ export async function loginWithToken( }); // Only once the switch is on disk: a failed write leaves auth.json naming the previous account, - // whose entries nothing else can find. The fixed-name entries go on every login, even a repeat - // of the same account: the secrets below are written under keyed names, so whatever is left - // under the old names is stale, and the next migration cannot tell it from a current secret. + // whose entries nothing else can find. The fixed names go every time, stale by then either way. const staleUserId = previousUserId === userInfo.id ? undefined : previousUserId; const leftovers = await clearKeyringSecrets(staleUserId); @@ -210,8 +208,6 @@ export async function loginWithToken( if (leftover) leftovers.push(leftover); } - // The login itself succeeded, so it goes through. Said whether or not the account changed: a - // repeat login leaves the same entries behind, and nothing later in the CLI reads or names them. if (leftovers.length) { warning({ message: diff --git a/src/lib/credentials.ts b/src/lib/credentials.ts index f35b3231f..e97d4d3a1 100644 --- a/src/lib/credentials.ts +++ b/src/lib/credentials.ts @@ -45,7 +45,6 @@ function legacyKeyringKey(kind: SecretKind): KeyringKey { return { service: KEYRING_SERVICE, account: kind }; } -/** A secret a delete could not remove, named by where it still is. */ export interface KeyringLeftover { key: KeyringKey; error: unknown; @@ -164,12 +163,9 @@ async function writeKeyring(key: KeyringKey, value: string): Promise { } /** - * Returns the secret this left in the keyring, or null. Callers that cannot act on it ignore it. - * - * Only a secret that still reads back is reported. The module loads on machines with no secret - * service, where every entry throws although nothing was ever stored, and a caller acting on that - * would name a keyring the user does not have. A keyring that can neither delete nor read is - * silent for the same reason, which is the cost of not naming one that was never there. + * Returns the secret this left in the keyring, or null. Only a secret that still reads back is + * reported: the module loads on machines with no secret service, where every entry throws although + * nothing was ever stored, and naming a keyring the user does not have helps no one. */ async function deleteKeyring(key: KeyringKey): Promise { try { @@ -230,8 +226,7 @@ export async function setSecret( return; } catch (err) { cliDebugPrint('credentials', 'keyring write failed; falling back to file', err); - // The token is about to land in the file, which is where reads go from now on. The rest - // follow it, or the keyring copies become unreachable. + // Reads go to the file from here, so a copy left in the keyring is unreachable. if (kind === 'token') await moveKeyringSecretsToFile(userId); } } @@ -257,8 +252,8 @@ async function moveKeyringSecretsToFile(userId: string): Promise { * the previous account's does not survive a re-login — the keyring outlives the auth.json rewrite * that replaces everything else. * - * Returns the secret this left behind, or null. A refused delete here is the one that matters - * most: the stored value stays, and reads keep serving it as the account's own. + * Returns the secret this left behind, or null: reads hit the keyring first, so a refused delete + * keeps serving a password the account no longer has. */ export async function deleteSecret(userId: string, kind: SecretKind): Promise { const leftover = (await backendFor(userId)) === 'keyring' ? await deleteKeyring(keyringKey(userId, kind)) : null; @@ -277,7 +272,7 @@ export async function deleteSecret(userId: string, kind: SecretKind): Promise leftover !== null); } -/** Names the entries a failed logout or login left behind, in the words the keyring app shows. */ +/** Names the entries a failed logout or login left behind. */ export function describeLeftovers(leftovers: KeyringLeftover[]): string { return leftovers.map(({ key }) => `${key.service}/${key.account}`).join(', '); } -/** The reasons behind {@link describeLeftovers}, each said once however many entries share it. */ +/** The reasons behind {@link describeLeftovers}, each said once. */ export function leftoverReasons(leftovers: KeyringLeftover[]): string { const reasons = leftovers.map(({ error }) => (error instanceof Error ? error.message : String(error))); return [...new Set(reasons)].join(' '); @@ -364,10 +359,8 @@ async function dropUnkeyedSecrets(file: AuthFile): Promise { * touched, so the migration never restores a value something newer replaced. */ async function keyKeyringSecrets(userId: string): Promise { - // A keyed token means a login already wrote this account's secrets under the new names. The - // fixed names are then whatever a previous login left, which may be another account's, so - // nothing under them is claimed for this one. Read once, and only once a fixed name turns - // something up, which on a keyed account is never. + // A keyed token means a login already wrote this account's secrets under the new names, so the + // fixed names hold a previous login's, possibly another account's. Read lazily: usually never. let claimable: boolean | undefined; for (const kind of SECRET_KINDS) { @@ -377,8 +370,6 @@ async function keyKeyringSecrets(userId: string): Promise { claimable ??= (await readKeyring(keyringKey(userId, 'token'))) === undefined; - // The second test covers the kinds a login stores directly; the first covers the kinds it - // leaves empty, which nothing else would tell apart from never having been set. if (!claimable || (await getSecret(userId, kind)) !== undefined) { await deleteKeyring(legacy); continue; @@ -459,10 +450,10 @@ export async function ensureSecretsKeyed(): Promise { * file into its current shape, then the secrets onto keys that carry the user ID. The order is a * dependency chain — keying by user needs the user ID the shape migration produces. * - * Every reader calls this before it reads. `loginWithToken()` does not: it replaces the file - * wholesale, so there is nothing to bring forward, and it clears the old keyring names itself. + * `loginWithToken()` does not call it: it replaces the file wholesale, so there is nothing to + * bring forward, and it clears the old keyring names itself. * - * Each step is single-flight and never throws, so repeat calls cost nothing. + * Each step is single-flight, so repeat calls cost nothing. */ export async function ensureCredentialsCurrent(): Promise { await ensureMigrated(); From 6b95b055282d1a62223c832829c8d385e78f1449 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Richard=20Sol=C3=A1r?= Date: Thu, 1 Oct 2026 13:43:51 +0200 Subject: [PATCH 33/33] test: keep the suite off the real OS keyring `__APIFY_INTERNAL_TEST_AUTH_PATH__` moves auth.json somewhere scratch, but the OS keyring is per-user and `clearKeyringSecrets()` deletes the fixed names whatever the backend is. Two of the seven test files that reach credentials never mocked the module, and one of them runs `logout`, so `test:api` deleted the developer's own stored login. Mocked for every test file instead of per file, with `useAuthSetup` refusing to run if the real module ever resolves again. Co-Authored-By: Claude Opus 5 --- test/__setup__/global.ts | 10 ++++++++++ test/__setup__/hooks/useAuthSetup.ts | 18 +++++++++++++++++- vitest.config.ts | 1 + 3 files changed, 28 insertions(+), 1 deletion(-) create mode 100644 test/__setup__/global.ts diff --git a/test/__setup__/global.ts b/test/__setup__/global.ts new file mode 100644 index 000000000..a099d07a5 --- /dev/null +++ b/test/__setup__/global.ts @@ -0,0 +1,10 @@ +/** + * Runs before every test file. + * + * The keyring is the one store a test cannot sandbox. `__APIFY_INTERNAL_TEST_AUTH_PATH__` moves + * `auth.json` somewhere scratch, but the OS keyring is per-user, and `clearKeyringSecrets()` + * deletes the fixed names whatever the backend is — so one `logout` in a test reaches the + * developer's own stored login. Mocking it here rather than per file means a new test cannot + * forget. + */ +vi.mock('@napi-rs/keyring', () => import('./keyring-mock.js')); diff --git a/test/__setup__/hooks/useAuthSetup.ts b/test/__setup__/hooks/useAuthSetup.ts index 932e6d26b..bdb04a4bd 100644 --- a/test/__setup__/hooks/useAuthSetup.ts +++ b/test/__setup__/hooks/useAuthSetup.ts @@ -43,6 +43,21 @@ const envVariable = '__APIFY_INTERNAL_TEST_AUTH_PATH__'; /** * A hook that allows each test to have a unique auth setup. */ +/** + * The keyring is the one store a test cannot sandbox: `__APIFY_INTERNAL_TEST_AUTH_PATH__` moves + * `auth.json` somewhere scratch, but the OS keyring is per-user, and `clearKeyringSecrets()` + * deletes the fixed names whatever the backend is. One `logout` against the real module reaches + * the developer's own stored login. + */ +async function assertKeyringIsMocked() { + const keyring = await import('@napi-rs/keyring').catch(() => null); + if (keyring && !('resetKeyringMock' in keyring)) { + throw new Error( + 'Tests resolved the real @napi-rs/keyring, which would read and delete your own stored login. Restore setupFiles in vitest.config.ts.', + ); + } +} + export function useAuthSetup({ cleanup = true, perTest = true }: UseAuthSetupOptions = {}) { const random = cryptoRandomObjectId(12); @@ -51,7 +66,8 @@ export function useAuthSetup({ cleanup = true, perTest = true }: UseAuthSetupOpt const before = perTest ? beforeEach : beforeAll; const after = perTest ? afterEach : afterAll; - before(() => { + before(async () => { + await assertKeyringIsMocked(); vitest.stubEnv(envVariable, envValue()); // Tests pin to the file backend so they don't touch the real OS keyring. // Unit tests for credentials.ts override this explicitly. diff --git a/vitest.config.ts b/vitest.config.ts index 3e6eeb0ba..de6ac52b6 100644 --- a/vitest.config.ts +++ b/vitest.config.ts @@ -14,6 +14,7 @@ export default defineConfig({ testTimeout: 120_000 * multiplierFactor, hookTimeout: 120_000 * multiplierFactor, include: ['**/*.{test,spec}.?(c|m)[jt]s?(x)'], + setupFiles: ['./test/__setup__/global.ts'], passWithNoTests: true, silent: !process.env.NO_SILENT_TESTS, env: {