From 0de1bcdb6d4eec609bf60338c2b182b2c48413cf Mon Sep 17 00:00:00 2001 From: Spenser Hale Date: Thu, 9 Jul 2026 19:41:55 -0700 Subject: [PATCH] fix: do not require a keychain token read for commands that do not need auth On macOS over SSH (and other headless sessions), reading the stored token from the OS keychain fails because the keychain ACL authorization prompt requires a GUI. On trunk, rootCmd() in src/bin/vip.js called Token.get() unconditionally before checking whether the invocation even needs auth, so `vip -h`, `vip -v`, `vip logout`, and local `vip dev-env ...` crashed with an opaque "Unexpected error" even though none of them require a token. keytar's native rejection carries no stack frames, and the crash happened before argument parsing so --debug could not help. Changes: - vip.js: compute the argv classification flags before fetching the token, and add resolveToken() which skips Token.get() entirely for commands that do not consult the token (help, version, logout, tokenless dev-env, custom-deploy with key). When the token is actually needed and the keychain read fails, surface an actionable error explaining the SSH/headless keychain limitation instead of crashing. - vip.js: stop doesArgvHaveAtLeastOneParam() from scanning past the `--` terminator so `vip wp -- --help` is not misclassified as top-level help. - token.ts: Token.uuid() now tolerates an unreachable keychain by returning a stable per-process UUID, so analytics cannot crash an otherwise-working command. - logout.ts: tolerate an unavailable keychain by skipping the server-side logout call when the stored token cannot be read or is invalid, and by wrapping Token.purge() so local cleanup always completes. - startup-args.ts: extract doesArgvHaveAtLeastOneParam() and isTokenReadRequired() into a pure, unit-tested module so the startup classification logic is covered outside the bin script. --- __tests__/lib/cli/startup-args.js | 102 ++++++++++++++++++++++++++++++ __tests__/lib/logout.test.ts | 25 +++++++- __tests__/lib/token.js | 23 +++++++ src/bin/vip.js | 56 ++++++++++++---- src/lib/cli/startup-args.ts | 81 ++++++++++++++++++++++++ src/lib/logout.ts | 25 +++++++- src/lib/token.ts | 29 +++++++-- 7 files changed, 320 insertions(+), 21 deletions(-) create mode 100644 __tests__/lib/cli/startup-args.js create mode 100644 src/lib/cli/startup-args.ts diff --git a/__tests__/lib/cli/startup-args.js b/__tests__/lib/cli/startup-args.js new file mode 100644 index 000000000..70ac59dcc --- /dev/null +++ b/__tests__/lib/cli/startup-args.js @@ -0,0 +1,102 @@ +import { describe, expect, it } from '@jest/globals'; + +import { + doesArgvHaveAtLeastOneParam, + isTokenReadRequired, +} from '../../../src/lib/cli/startup-args'; + +describe( 'lib/cli/startup-args', () => { + describe( 'doesArgvHaveAtLeastOneParam', () => { + it( 'matches a param that appears anywhere before `--`', () => { + expect( + doesArgvHaveAtLeastOneParam( [ 'node', 'vip', '--help' ], [ '-h', '--help', 'help' ] ) + ).toBe( true ); + expect( + doesArgvHaveAtLeastOneParam( + [ 'node', 'vip', 'wp', '--help', 'list' ], + [ '-h', '--help', 'help' ] + ) + ).toBe( true ); + } ); + + it( 'returns false when no param is present', () => { + expect( + doesArgvHaveAtLeastOneParam( [ 'node', 'vip', 'wp', 'list' ], [ '-h', '--help', 'help' ] ) + ).toBe( false ); + } ); + + it( 'ignores params that appear only after `--`', () => { + expect( + doesArgvHaveAtLeastOneParam( + [ 'node', 'vip', 'wp', '@app.env', '--', '--help' ], + [ '-h', '--help', 'help' ] + ) + ).toBe( false ); + } ); + + it( 'still matches params before `--` even when the same param also follows `--`', () => { + expect( + doesArgvHaveAtLeastOneParam( + [ 'node', 'vip', '--help', '--', '--help' ], + [ '-h', '--help', 'help' ] + ) + ).toBe( true ); + } ); + + it( 'handles argv with no `--` terminator', () => { + expect( doesArgvHaveAtLeastOneParam( [ 'node', 'vip', 'logout' ], [ 'logout' ] ) ).toBe( + true + ); + expect( doesArgvHaveAtLeastOneParam( [ 'node', 'vip', 'whoami' ], [ 'logout' ] ) ).toBe( + false + ); + } ); + + it( 'treats every argument as after the terminator when `--` is the first arg', () => { + expect( + doesArgvHaveAtLeastOneParam( [ '--', '--help', 'help' ], [ '-h', '--help', 'help' ] ) + ).toBe( false ); + } ); + } ); + + describe( 'isTokenReadRequired', () => { + const allFalse = { + isLoginCommand: false, + isLogoutCommand: false, + isHelpCommand: false, + isVersionCommand: false, + isDevEnvCommandWithoutEnv: false, + isCustomDeployCmdWithKey: false, + }; + + it( 'requires the token when no flags are set (normal commands need auth)', () => { + expect( isTokenReadRequired( allFalse ) ).toBe( true ); + } ); + + it.each( [ + 'isLogoutCommand', + 'isHelpCommand', + 'isVersionCommand', + 'isDevEnvCommandWithoutEnv', + 'isCustomDeployCmdWithKey', + ] )( 'skips the token read when %s alone is set', bypassFlag => { + expect( isTokenReadRequired( { ...allFalse, [ bypassFlag ]: true } ) ).toBe( false ); + } ); + + it( 'requires the token when isLoginCommand alone is set', () => { + expect( isTokenReadRequired( { ...allFalse, isLoginCommand: true } ) ).toBe( true ); + } ); + + it.each( [ + 'isLogoutCommand', + 'isHelpCommand', + 'isVersionCommand', + 'isDevEnvCommandWithoutEnv', + 'isCustomDeployCmdWithKey', + ] )( 'requires the token when isLoginCommand is set even alongside %s', bypassFlag => { + expect( + isTokenReadRequired( { ...allFalse, isLoginCommand: true, [ bypassFlag ]: true } ) + ).toBe( true ); + } ); + } ); +} ); diff --git a/__tests__/lib/logout.test.ts b/__tests__/lib/logout.test.ts index 6532786b6..9e3f0f0c2 100644 --- a/__tests__/lib/logout.test.ts +++ b/__tests__/lib/logout.test.ts @@ -16,6 +16,7 @@ jest.mock( '../../src/lib/api/http', () => ( { jest.mock( '../../src/lib/token', () => ( { __esModule: true, default: { + get: jest.fn( () => Promise.resolve( { valid: () => true } ) ), purge: jest.fn( () => Promise.resolve( true ) ), }, } ) ); @@ -35,17 +36,22 @@ jest.mock( '../../src/lib/tracker', () => ( { } ) ); const mockHttpApiFn = jest.mocked( http ); -// eslint-disable-next-line @typescript-eslint/unbound-method +/* eslint-disable @typescript-eslint/unbound-method */ +const mockTokenGetFn = jest.mocked( Token.get ); const mockTokenPurgeFn = jest.mocked( Token.purge ); +/* eslint-enable @typescript-eslint/unbound-method */ describe( 'logout', () => { beforeEach( () => { jest.clearAllMocks(); + mockTokenGetFn.mockResolvedValue( { valid: () => true } as InstanceType< typeof Token > ); + mockTokenPurgeFn.mockResolvedValue( true ); } ); it( 'purges primary token, clears elevated-token cache, and emits telemetry', async () => { mockHttpApiFn.mockResolvedValueOnce( { ok: true } as unknown as Response ); await logout(); + expect( mockHttpApiFn ).toHaveBeenCalledTimes( 1 ); expect( mockTokenPurgeFn ).toHaveBeenCalledTimes( 1 ); expect( tokenCache.clearAll ).toHaveBeenCalledTimes( 1 ); expect( trackEvent ).toHaveBeenCalledWith( 'logout_command_execute' ); @@ -57,4 +63,21 @@ describe( 'logout', () => { expect( mockTokenPurgeFn ).toHaveBeenCalledTimes( 1 ); expect( tokenCache.clearAll ).toHaveBeenCalledTimes( 1 ); } ); + + it( 'skips the server-side logout when the stored token cannot be read', async () => { + mockTokenGetFn.mockRejectedValueOnce( new Error( 'An unknown error occurred.' ) ); + await logout(); + expect( mockHttpApiFn ).not.toHaveBeenCalled(); + expect( mockTokenPurgeFn ).toHaveBeenCalledTimes( 1 ); + expect( tokenCache.clearAll ).toHaveBeenCalledTimes( 1 ); + expect( trackEvent ).toHaveBeenCalledWith( 'logout_command_execute' ); + } ); + + it( 'completes even when purging the stored token fails', async () => { + mockHttpApiFn.mockResolvedValueOnce( { ok: true } as unknown as Response ); + mockTokenPurgeFn.mockRejectedValueOnce( new Error( 'An unknown error occurred.' ) ); + await logout(); + expect( tokenCache.clearAll ).toHaveBeenCalledTimes( 1 ); + expect( trackEvent ).toHaveBeenCalledWith( 'logout_command_execute' ); + } ); } ); diff --git a/__tests__/lib/token.js b/__tests__/lib/token.js index 3fef26c5a..31e8f35a6 100644 --- a/__tests__/lib/token.js +++ b/__tests__/lib/token.js @@ -1,3 +1,6 @@ +import { jest } from '@jest/globals'; + +import * as keychain from '../../src/lib/keychain'; import Token, { SERVICE } from '../../src/lib/token'; describe( 'token tests', () => { @@ -59,6 +62,26 @@ describe( 'token tests', () => { } ); } ); + describe( 'uuid() keychain resilience', () => { + let getKeychainSpy; + + afterEach( () => { + getKeychainSpy?.mockRestore(); + } ); + + it( 'degrades to a stable per-process UUID when the keychain is unavailable', async () => { + getKeychainSpy = jest + .spyOn( keychain, 'getKeychain' ) + .mockRejectedValue( new Error( 'keychain locked' ) ); + + const uuid1 = await Token.uuid(); + const uuid2 = await Token.uuid(); + + expect( uuid1 ).toMatch( /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/ ); + expect( uuid2 ).toBe( uuid1 ); + } ); + } ); + describe( 'getServiceName()', () => { // TODO how do we test this when it comes from env var, which we've already overridden? it.todo( 'should return default service name for default API_HOST' ); diff --git a/src/bin/vip.js b/src/bin/vip.js index 713f7eaa8..5ef4d53c1 100755 --- a/src/bin/vip.js +++ b/src/bin/vip.js @@ -8,12 +8,14 @@ import { prompt } from 'enquirer'; import command, { containsAppEnvArgument } from '../lib/cli/command'; import config from '../lib/cli/config'; +import * as exit from '../lib/cli/exit'; import { loadInternalBin } from '../lib/cli/internal-bin-loader'; import { rewriteArgvForInternalBin, resolveInternalBinFromArgv, isSeaRuntime, } from '../lib/cli/sea-dispatch'; +import { doesArgvHaveAtLeastOneParam, isTokenReadRequired } from '../lib/cli/startup-args'; import tokenCache from '../lib/rechallenge/token-cache'; import Token from '../lib/token'; import { aliasUser, trackEvent } from '../lib/tracker'; @@ -82,15 +84,6 @@ const runCmd = async function () { await cmd.argv( process.argv ); }; -/** - * @param {any[]} argv - * @param {any[]} params - * @returns {boolean} - */ -function doesArgvHaveAtLeastOneParam( argv, params ) { - return argv.some( arg => params.includes( arg ) ); -} - async function runLoginFlow() { console.log(); console.log( ' _ __ ________ ________ ____' ); @@ -183,6 +176,40 @@ async function runLoginFlow() { return token; } +/** + * Reads the stored auth token, but only for commands whose branch decision + * actually depends on it. + * + * Commands like `vip -h` / `vip -v` / `logout` never consult the token, so we + * skip the keychain entirely for them. This matters over SSH / non-graphical + * sessions where the OS keychain may be locked or unable to show an + * authorization prompt: reading the stored credential there throws a bare + * native error that would otherwise crash the CLI before argument parsing. + * + * @param {Object} flags Precomputed argv classification flags. + * @returns {Promise} The stored token, or undefined when not needed. + */ +async function resolveToken( flags ) { + if ( ! isTokenReadRequired( flags ) ) { + return undefined; + } + + try { + return await Token.get(); + } catch ( err ) { + debug( 'Failed to read token from keychain:', err ); + + exit.withError( + 'Unable to read VIP-CLI credentials from the system keychain: ' + + `${ err instanceof Error ? err.message : String( err ) }\n` + + 'If you are connected over SSH or a non-graphical session, the OS keychain may be ' + + 'locked or unable to show an authorization prompt. Try running VIP-CLI from a local, ' + + 'graphical session, or unlock the keychain first.\n' + + 'Re-run with the environment variable DEBUG=@automattic/vip:* for full details.' + ); + } +} + const rootCmd = async function () { if ( isSeaRuntime() && ! process.env.VIP_CLI_TARGET_BIN ) { const resolution = resolveInternalBinFromArgv( process.argv ); @@ -193,8 +220,6 @@ const rootCmd = async function () { } } - let token = await Token.get(); - const isHelpCommand = doesArgvHaveAtLeastOneParam( process.argv, [ 'help', '-h', '--help' ] ); const isVersionCommand = doesArgvHaveAtLeastOneParam( process.argv, [ '-v', '--version' ] ); const isLogoutCommand = doesArgvHaveAtLeastOneParam( process.argv, [ 'logout' ] ); @@ -207,6 +232,15 @@ const rootCmd = async function () { debug( 'Argv:', process.argv ); + let token = await resolveToken( { + isLoginCommand, + isLogoutCommand, + isHelpCommand, + isVersionCommand, + isDevEnvCommandWithoutEnv, + isCustomDeployCmdWithKey, + } ); + if ( ! isLoginCommand && ( isLogoutCommand || diff --git a/src/lib/cli/startup-args.ts b/src/lib/cli/startup-args.ts new file mode 100644 index 000000000..f7be4663b --- /dev/null +++ b/src/lib/cli/startup-args.ts @@ -0,0 +1,81 @@ +/** + * Pure helpers used by the CLI entrypoint (`src/bin/vip.js`) to classify the + * incoming argv at startup and decide whether authentication is required before + * dispatching a command. Extracted from the bin script so the branching logic + * can be unit-tested in isolation. + */ + +/** + * Reports whether `argv` contains at least one of the given `params`. + * + * Only arguments _before_ a `--` terminator are considered: everything after + * `--` is meant to be forwarded verbatim to a downstream command (e.g. + * `vip wp @app.env -- --help`), so a matching token there must not be treated + * as one of our own flags. + * + * @param argv The process argv (or any argument list) to inspect. + * @param params The parameters to look for (e.g. `[ '-h', '--help', 'help' ]`). + * @return `true` if any param appears before `--`, otherwise `false`. + */ +export function doesArgvHaveAtLeastOneParam( argv: string[], params: string[] ): boolean { + // Arguments after `--` belong to the wrapped command (e.g. `vip wp -- --help`) + // and must not classify the invocation itself as help/version/etc. + const terminatorIndex = argv.indexOf( '--' ); + const ownArgs = terminatorIndex === -1 ? argv : argv.slice( 0, terminatorIndex ); + return ownArgs.some( arg => params.includes( arg ) ); +} + +/** + * The set of startup classification flags derived from the argv. Each flag is a + * boolean describing whether the current invocation matches a particular + * special-case command. + */ +export interface StartupFlags { + /** The user is running `login`. */ + isLoginCommand: boolean; + /** The user is running `logout`. */ + isLogoutCommand: boolean; + /** The user is requesting help (`help`, `-h`, `--help`). */ + isHelpCommand: boolean; + /** The user is requesting the version (`-v`, `--version`). */ + isVersionCommand: boolean; + /** The user is running `dev-env` without targeting an app/env. */ + isDevEnvCommandWithoutEnv: boolean; + /** The user is running `deploy` with a `WPVIP_DEPLOY_TOKEN` set. */ + isCustomDeployCmdWithKey: boolean; +} + +/** + * Decides whether the stored authentication token needs to be read from the + * keychain before dispatching the command. + * + * The token read is skipped for commands that never rely on the stored token: + * `logout`, help, version, `dev-env` without an app/env, and a custom-deploy + * invocation carrying its own token. `login` always requires the read path to + * be taken so it proceeds through the login branch, even when combined with one + * of the bypass flags above. + * + * @param flags The startup classification flags derived from the argv. + * @return `true` when the token must be read, otherwise `false`. + */ +export function isTokenReadRequired( flags: StartupFlags ): boolean { + const { + isLoginCommand, + isLogoutCommand, + isHelpCommand, + isVersionCommand, + isDevEnvCommandWithoutEnv, + isCustomDeployCmdWithKey, + } = flags; + + return ( + isLoginCommand || + ! ( + isLogoutCommand || + isHelpCommand || + isVersionCommand || + isDevEnvCommandWithoutEnv || + isCustomDeployCmdWithKey + ) + ); +} diff --git a/src/lib/logout.ts b/src/lib/logout.ts index 3011fe422..320c7d2d0 100644 --- a/src/lib/logout.ts +++ b/src/lib/logout.ts @@ -1,13 +1,34 @@ +import debugLib from 'debug'; + import http from '../lib/api/http'; import tokenCache from '../lib/rechallenge/token-cache'; import Token from '../lib/token'; import { trackEvent } from '../lib/tracker'; +const debug = debugLib( '@automattic/vip:logout' ); + export default async (): Promise< void > => { try { - await http( '/logout', { method: 'post' } ); + // Server-side logout is skipped when the stored token cannot be read + // (e.g. a locked OS keychain over SSH) or is not valid — there is + // nothing usable to send. + let storedToken; + try { + storedToken = await Token.get(); + } catch ( err ) { + debug( 'Skipping server-side logout; could not read the stored token:', err ); + } + + if ( storedToken?.valid() ) { + await http( '/logout', { method: 'post' } ); + } } finally { - await Token.purge(); + try { + await Token.purge(); + } catch ( err ) { + debug( 'Could not purge the stored token from the keychain:', err ); + } + await tokenCache.clearAll(); } diff --git a/src/lib/token.ts b/src/lib/token.ts index 512c1239e..bec213b6e 100644 --- a/src/lib/token.ts +++ b/src/lib/token.ts @@ -1,9 +1,12 @@ +import debugLib from 'debug'; import { jwtDecode } from 'jwt-decode'; import { randomUUID } from 'node:crypto'; import { API_HOST, PRODUCTION_API_HOST } from './api/constants'; import { getKeychain } from './keychain'; +const debug = debugLib( '@automattic/vip:token' ); + interface Payload { id?: number; iat?: number; @@ -78,17 +81,29 @@ export default class Token { return this._raw ?? ''; } + // Per-process fallback used when the keychain cannot be reached (e.g. a + // locked OS keychain over SSH). Keeps analytics working anonymously for the + // lifetime of the process instead of crashing a command that would + // otherwise succeed. + private static ephemeralUuid?: string; + public static async uuid(): Promise< string > { const service = Token.getServiceName( '-uuid' ); - const keychain = await getKeychain(); - let _uuid = await keychain.getPassword( service ); - if ( ! _uuid ) { - _uuid = randomUUID(); - await keychain.setPassword( service, _uuid ); + try { + const keychain = await getKeychain(); + let _uuid = await keychain.getPassword( service ); + if ( ! _uuid ) { + _uuid = randomUUID(); + await keychain.setPassword( service, _uuid ); + } + + return _uuid; + } catch ( err ) { + debug( 'Failed to read/write analytics UUID from keychain; using an ephemeral UUID', err ); + Token.ephemeralUuid ??= randomUUID(); + return Token.ephemeralUuid; } - - return _uuid; } public static async setUuid( _uuid: string ): Promise< void > {