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 > {