diff --git a/src/commands/project.test.ts b/src/commands/project.test.ts index ae98070..e2354fd 100644 --- a/src/commands/project.test.ts +++ b/src/commands/project.test.ts @@ -1044,6 +1044,122 @@ describe('runUpdate', () => { }); }); +describe('#79 — an unreadable --password-file is a validation error, not a crash', () => { + const missing = join(tmpdir(), 'testsprite-issue-79-absent-password-file'); + + it('runCreate rejects a missing file with VALIDATION_ERROR (exit 5) before the network', async () => { + const { credentialsPath } = makeCreds(); + const fetchImpl = vi.fn(async () => { + throw new Error('should not hit network'); + }); + + await expect( + runCreate( + { + profile: 'default', + output: 'json', + debug: false, + type: 'backend', + name: 'Guarded', + passwordFile: missing, + }, + { + credentialsPath, + fetchImpl: fetchImpl as unknown as typeof fetch, + stdout: () => {}, + stderr: () => {}, + }, + ), + ).rejects.toMatchObject({ code: 'VALIDATION_ERROR', exitCode: 5 }); + + expect(fetchImpl).not.toHaveBeenCalled(); + }); + + it('runUpdate rejects a missing file with VALIDATION_ERROR (exit 5) before the network', async () => { + const { credentialsPath } = makeCreds(); + const fetchImpl = vi.fn(async () => { + throw new Error('should not hit network'); + }); + + await expect( + runUpdate( + { + profile: 'default', + output: 'json', + debug: false, + projectId: 'proj_guarded', + passwordFile: missing, + }, + { + credentialsPath, + fetchImpl: fetchImpl as unknown as typeof fetch, + stdout: () => {}, + stderr: () => {}, + }, + ), + ).rejects.toMatchObject({ code: 'VALIDATION_ERROR', exitCode: 5 }); + + expect(fetchImpl).not.toHaveBeenCalled(); + }); + + it('names the flag in nextAction instead of leaking a raw ENOENT', async () => { + const { credentialsPath } = makeCreds(); + + await expect( + runCreate( + { + profile: 'default', + output: 'json', + debug: false, + type: 'backend', + name: 'Guarded', + passwordFile: missing, + }, + { + credentialsPath, + fetchImpl: (async () => { + throw new Error('should not hit network'); + }) as unknown as typeof fetch, + stdout: () => {}, + stderr: () => {}, + }, + ), + ).rejects.toMatchObject({ + nextAction: expect.stringContaining('--password-file') as unknown as string, + }); + }); + + it('still reads a password file that exists', async () => { + const { credentialsPath } = makeCreds(); + const dir = mkdtempSync(join(tmpdir(), 'cli-p79-')); + const passwordFile = join(dir, 'pw.txt'); + writeFileSync(passwordFile, 'from-file\n'); + + const sentBodies: unknown[] = []; + const fetchImpl = (async (_input: Parameters[0], init: RequestInit = {}) => { + if (init.body) sentBodies.push(JSON.parse(init.body as string) as unknown); + return new Response(JSON.stringify({ ...PROJECT_FIXTURE, id: 'proj_pw' }), { + status: 200, + headers: { 'content-type': 'application/json' }, + }); + }) as typeof fetch; + + await runCreate( + { + profile: 'default', + output: 'json', + debug: false, + type: 'backend', + name: 'Guarded', + passwordFile, + }, + { credentialsPath, fetchImpl, stdout: () => {}, stderr: () => {} }, + ); + + expect((sentBodies[0] as Record).password).toBe('from-file'); + }); +}); + describe('runDelete', () => { it('refuses without --confirm and never hits the network (exit 5)', async () => { const { credentialsPath } = makeCreds(); diff --git a/src/commands/project.ts b/src/commands/project.ts index 6a0b332..6d339f8 100644 --- a/src/commands/project.ts +++ b/src/commands/project.ts @@ -11,6 +11,7 @@ import { ApiError } from '../lib/errors.js'; import type { FetchImpl } from '../lib/http.js'; import type { HttpClient } from '../lib/http.js'; import { GLOBAL_OPTS_HINT, Output, resolveOutputMode, type OutputMode } from '../lib/output.js'; +import { readSecretFileGuarded } from '../lib/secret-file.js'; import { assertNotLocal } from '../lib/target-url.js'; import { renderTextTable, resolveTextColumns, type TextTableColumn } from '../lib/text-table.js'; import { assertIdempotencyKey } from '../lib/validate.js'; @@ -210,7 +211,7 @@ export async function runCreate( // Resolve password: flag > file > none let password = opts.password; if (password === undefined && opts.passwordFile !== undefined) { - password = readFileSync(opts.passwordFile, 'utf8').trim(); + password = readSecretFileGuarded('password-file', opts.passwordFile); } const idempotencyKey = opts.idempotencyKey ?? `cli-proj-create-${randomUUID()}`; @@ -326,7 +327,7 @@ export async function runUpdate( // filesystem, even when --password-file is present. let password = opts.password; if (password === undefined && opts.passwordFile !== undefined) { - password = readFileSync(opts.passwordFile, 'utf8').trim(); + password = readSecretFileGuarded('password-file', opts.passwordFile); } const idempotencyKey = opts.idempotencyKey ?? `cli-proj-update-${randomUUID()}`; diff --git a/src/lib/secret-file.test.ts b/src/lib/secret-file.test.ts new file mode 100644 index 0000000..f22a518 --- /dev/null +++ b/src/lib/secret-file.test.ts @@ -0,0 +1,118 @@ +import { mkdirSync, mkdtempSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { afterEach, beforeEach, describe, expect, it } from 'vitest'; +import { ApiError } from './errors.js'; +import { readSecretFileGuarded } from './secret-file.js'; + +let tmpRoot: string; +const originalCwd = process.cwd(); + +beforeEach(() => { + tmpRoot = mkdtempSync(join(tmpdir(), 'testsprite-secret-file-')); +}); + +afterEach(() => { + // mkdtempSync directory is small and short-lived; OS cleans it up. + process.chdir(originalCwd); +}); + +describe('readSecretFileGuarded', () => { + it('returns the file contents', () => { + const path = join(tmpRoot, 'pw.txt'); + writeFileSync(path, 'hunter2'); + expect(readSecretFileGuarded('password-file', path)).toBe('hunter2'); + }); + + it('trims surrounding whitespace and the trailing newline', () => { + const path = join(tmpRoot, 'pw-newline.txt'); + writeFileSync(path, ' hunter2 \n'); + expect(readSecretFileGuarded('password-file', path)).toBe('hunter2'); + }); + + it('drops a leading UTF-8 BOM so PowerShell-written files still work', () => { + const path = join(tmpRoot, 'pw-bom.txt'); + writeFileSync(path, 'hunter2\n'); + expect(readSecretFileGuarded('password-file', path)).toBe('hunter2'); + }); + + it('preserves interior whitespace', () => { + const path = join(tmpRoot, 'pw-spaces.txt'); + writeFileSync(path, 'two words\n'); + expect(readSecretFileGuarded('password-file', path)).toBe('two words'); + }); + + it('resolves a relative path against the working directory', () => { + writeFileSync(join(tmpRoot, 'relative.txt'), 'from-cwd'); + process.chdir(tmpRoot); + expect(readSecretFileGuarded('password-file', 'relative.txt')).toBe('from-cwd'); + }); + + it('returns an empty string for an empty file rather than throwing', () => { + const path = join(tmpRoot, 'empty.txt'); + writeFileSync(path, ''); + expect(readSecretFileGuarded('password-file', path)).toBe(''); + }); + + describe('missing file', () => { + it('throws VALIDATION_ERROR with exit code 5', () => { + const path = join(tmpRoot, 'nope.txt'); + expect(() => readSecretFileGuarded('password-file', path)).toThrow(ApiError); + try { + readSecretFileGuarded('password-file', path); + expect.unreachable('should have thrown'); + } catch (err) { + expect(err).toMatchObject({ code: 'VALIDATION_ERROR', exitCode: 5 }); + } + }); + + it('names the offending flag and path in nextAction', () => { + const path = join(tmpRoot, 'nope.txt'); + try { + readSecretFileGuarded('password-file', path); + expect.unreachable('should have thrown'); + } catch (err) { + const { nextAction } = err as ApiError; + expect(nextAction).toContain('--password-file'); + expect(nextAction).toContain('file does not exist'); + expect(nextAction).toContain(path); + } + }); + + it('attributes the error to whichever flag the caller names', () => { + const path = join(tmpRoot, 'nope.txt'); + try { + readSecretFileGuarded('client-secret-file', path); + expect.unreachable('should have thrown'); + } catch (err) { + expect((err as ApiError).nextAction).toContain('--client-secret-file'); + } + }); + + it('reports the path as typed, not the resolved absolute path', () => { + process.chdir(tmpRoot); + try { + readSecretFileGuarded('password-file', 'missing.txt'); + expect.unreachable('should have thrown'); + } catch (err) { + const { nextAction } = err as ApiError; + expect(nextAction).toContain('missing.txt'); + expect(nextAction).not.toContain(tmpRoot); + } + }); + }); + + describe('directory instead of a file', () => { + it('throws VALIDATION_ERROR instead of crashing with EISDIR', () => { + const path = join(tmpRoot, 'a-directory'); + mkdirSync(path); + try { + readSecretFileGuarded('password-file', path); + expect.unreachable('should have thrown'); + } catch (err) { + expect(err).toMatchObject({ code: 'VALIDATION_ERROR', exitCode: 5 }); + expect((err as ApiError).nextAction).toContain('not a regular file'); + } + }); + }); +}); diff --git a/src/lib/secret-file.ts b/src/lib/secret-file.ts new file mode 100644 index 0000000..b9619e6 --- /dev/null +++ b/src/lib/secret-file.ts @@ -0,0 +1,87 @@ +/** + * Guarded reader for the `--*-file` secret flags. + * + * Every one of these flags exists so a secret stays out of shell history, and + * every one of them is a path the user types by hand — so a typo is the + * expected failure, not an exceptional one. A bare + * `readFileSync(path, 'utf8').trim()` turns that typo into an unhandled Node + * exception: exit `1` instead of `5`, an `--output json` payload whose `error` + * is a bare string rather than the `{ code, message, nextAction }` envelope the + * rest of the CLI emits, and the absolute path plus errno leaked to stderr. + * + * This maps those failures onto the same typed `VALIDATION_ERROR` envelope the + * already-guarded file flags produce, mirroring `readCodeFileGuarded` in + * `src/commands/test.ts`. The payload cap is deliberately not carried over: + * secrets are small, and a size ceiling would be a behaviour change on a + * shipped flag rather than part of fixing the crash. + * + * Callers pass the flag name so the envelope names the flag the user actually + * typed — one helper serves `--password-file` today and the remaining + * credential/auto-auth file flags once they are migrated. + */ +import { readFileSync, statSync } from 'node:fs'; +import { isAbsolute, resolve } from 'node:path'; +import { localValidationError } from './errors.js'; + +/** + * Read a secret from `path`, surfacing every filesystem failure as a typed + * `VALIDATION_ERROR` (exit 5) attributed to `flag`. + * + * The returned value is trimmed, matching what the unguarded call sites did. + * Trimming also drops a leading UTF-8 BOM: `U+FEFF` is ECMAScript whitespace, + * so a file written by PowerShell 5.1's default `Set-Content -Encoding utf8` + * no longer smuggles an invisible character into the secret. + * + * @param flag - Flag name without the leading dashes, e.g. `'password-file'`. + * @param path - Path as supplied by the user; may be relative. + * @throws {ApiError} `VALIDATION_ERROR` when the path is missing, unreadable, + * or not a regular file. + */ +export function readSecretFileGuarded(flag: string, path: string): string { + const absolute = isAbsolute(path) ? path : resolve(process.cwd(), path); + + let stat; + try { + stat = statSync(absolute); + } catch (err) { + throw secretFileError(flag, path, err, 'stat'); + } + + // A directory would otherwise reach readFileSync and throw EISDIR on Linux + // while resolving to an empty read on some platforms — reject it up front so + // the contract is the same everywhere. + if (!stat.isFile()) { + throw localValidationError(flag, `not a regular file: ${path}`); + } + + try { + return readFileSync(absolute, 'utf8').trim(); + } catch (err) { + throw secretFileError(flag, path, err, 'read'); + } +} + +/** + * Translate a Node filesystem error into the CLI's validation envelope, + * reporting the path the user typed rather than the resolved absolute path so + * no directory layout leaks into output. + */ +function secretFileError( + flag: string, + path: string, + err: unknown, + verb: 'stat' | 'read', +): ReturnType { + const code = (err as NodeJS.ErrnoException).code; + if (code === 'ENOENT') { + return localValidationError(flag, `file does not exist: ${path}`); + } + if (code === 'EACCES' || code === 'EPERM') { + return localValidationError(flag, `permission denied reading ${path}`); + } + if (code === 'EISDIR') { + return localValidationError(flag, `not a regular file: ${path}`); + } + const reason = err instanceof Error ? err.message : 'unknown error'; + return localValidationError(flag, `cannot ${verb} ${path}: ${reason}`); +}