fix: do not require a keychain token read for commands that do not need auth

__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 );
+		} );
+	} );
+} );

__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' );
+	} );
 } );

__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' );

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<Token | undefined>} 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 ||

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 <env> -- --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
+		)
+	);
+}