diff --git a/src/commands/test.run.spec.ts b/src/commands/test.run.spec.ts index f41e5e93..90e029b8 100644 --- a/src/commands/test.run.spec.ts +++ b/src/commands/test.run.spec.ts @@ -710,6 +710,34 @@ describe('runTestRun — with --wait', () => { expect(printed.status).toBe('passed'); }); + it('a --summary-file in a missing directory is refused before the run is triggered', async () => { + const { credentialsPath } = makeCreds(); + const dir = mkdtempSync(join(tmpdir(), 'cli-single-preflight-')); + const calls: Array<{ url: string; method: string }> = []; + const fetchImpl = makeFetch((url, init) => { + calls.push({ url, method: init?.method ?? 'GET' }); + return { body: TRIGGER_RESP }; + }); + await expect( + runTestRun( + { + profile: 'default', + output: 'json', + debug: false, + dryRun: false, + testId: 'test_xyz', + wait: true, + timeoutSeconds: 60, + summaryFile: join(dir, 'no-such-subdir', 'summary.json'), + }, + { credentialsPath, fetchImpl, stdout: () => {}, stderr: () => {}, sleep: instantSleep }, + ), + ).rejects.toMatchObject({ exitCode: 5, details: { field: 'summary-file' } }); + // Read-only target resolution may run first; the run itself is never triggered. + expect(calls.filter(c => c.method !== 'GET')).toEqual([]); + rmSync(dir, { recursive: true, force: true }); + }); + it('wait path — failed status → throws CLIError with exitCode 1', async () => { const { credentialsPath } = makeCreds(); const fetchImpl = makeFetch(url => { @@ -5627,6 +5655,35 @@ describe('runTestRunAll — zero-dispatch fails the CI gate', () => { expect(JSON.parse(readFileSync(summaryFile, 'utf8')).passed).toBe(0); }); + it.each([ + ['--report-file', ['--report', 'junit', '--report-file'], 'junit.xml', 'report-file'], + ['--summary-file', ['--summary-file'], 'summary.json', 'summary-file'], + ])( + 'test run --all: a %s in a missing directory is refused before anything is dispatched', + async (_flag, flags, file, field) => { + vi.resetModules(); + const { createTestCommand } = await import('./test.js'); + const { credentialsPath } = makeCreds(); + const dir = mkdtempSync(join(tmpdir(), 'cli-preflight-')); + const fetchImpl = vi.fn(makeFetch(() => ({ body: EMPTY_BATCH }))); + const command = createTestCommand({ + credentialsPath, + fetchImpl, + stdout: () => {}, + stderr: () => {}, + sleep: () => Promise.resolve(), + }); + await expect( + command.parseAsync( + ['run', '--all', '--project', 'P', '--wait', ...flags, join(dir, 'no-such-subdir', file)], + { from: 'user' }, + ), + ).rejects.toMatchObject({ exitCode: 5, details: { field } }); + expect(fetchImpl).not.toHaveBeenCalled(); + rmSync(dir, { recursive: true, force: true }); + }, + ); + it('--allow-empty does NOT green a dispatched-but-failed run', async () => { const { credentialsPath } = makeCreds(); const acceptedBatch: BatchRunFreshResponse = { diff --git a/src/commands/test.ts b/src/commands/test.ts index ffab7a82..1a484d1d 100644 --- a/src/commands/test.ts +++ b/src/commands/test.ts @@ -57,6 +57,7 @@ import { resolveBatchReportProjectId, skippedJUnitResultsFromSummary, writeJUnitReportFile, + assertArtifactPath, type JUnitReportFormat, parseJUnitReportFormat, type JUnitTestResult, @@ -8094,6 +8095,17 @@ function assertFreshRunWaitOptions( ); } +/** Pre-dispatch: refuse unwritable --report-file / --summary-file before anything is triggered. */ +function assertRunArtifactPaths(opts: { + report?: JUnitReportFormat; + reportFile?: string; + summaryFile?: string; +}): void { + if (opts.report === 'junit' && opts.reportFile !== undefined) + assertArtifactPath('report-file', opts.reportFile); + if (opts.summaryFile !== undefined) assertArtifactPath('summary-file', opts.summaryFile); +} + function isSameLoopbackPort(url: string, port: number): boolean { try { return parseLoopbackTargetUrl(url)?.port === port; @@ -8330,6 +8342,7 @@ export async function runTestRun( client = makeClient(clientOpts, deps); } assertFreshRunWaitOptions(opts, false); + assertRunArtifactPaths(opts); const tunnelRequestTimeoutMs = isTunnelRun ? resolveRequestTimeoutMs(clientOpts, deps.env ?? process.env) : undefined; @@ -15710,6 +15723,11 @@ export function createTestCommand(deps: TestDeps = {}): Command { '--summary-file requires --wait (it reduces the terminal run result). Add --wait.', ); } + assertRunArtifactPaths({ + report, + reportFile: cmdOpts.reportFile, + summaryFile: cmdOpts.summaryFile, + }); if (isAll && !usingLocal && cmdOpts.tunnelClient === undefined) { // --all path: wave-ordered fresh batch run. diff --git a/src/commands/testlist.spec.ts b/src/commands/testlist.spec.ts index fc1b698c..0f9a124c 100644 --- a/src/commands/testlist.spec.ts +++ b/src/commands/testlist.spec.ts @@ -1,7 +1,7 @@ import { mkdtempSync, readFileSync, rmSync } from 'node:fs'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; -import { afterEach, beforeEach, describe, expect, it } from 'vitest'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import { runTestlistAdd, runTestlistCreate, @@ -668,6 +668,43 @@ describe('testlist run', () => { expect(err.exitCode).toBe(6); }); + it.each([ + ['--report-file', { report: 'junit', reportFile: 'missing/report.xml' }, 'report-file'], + ['--summary-file', { summaryFile: 'missing/summary.json' }, 'summary-file'], + ])( + 'a %s in a missing directory is refused before the list is dispatched', + async (_flag, over, field) => { + const { env } = makeCreds(); + const dir = mkdtempSync(join(tmpdir(), 'cli-testlist-preflight-')); + const fetchImpl = vi.fn(makeFetch(() => ({ body: { accepted: [] } }))); + const paths = Object.fromEntries( + Object.entries(over).map(([k, v]) => [k, k === 'report' ? v : join(dir, v as string)]), + ); + const err = (await runTestlistRun( + { ...runBase, listId: 'list-1', wait: true, ...paths } as never, + { env, fetchImpl, stdout: () => {}, stderr: () => {}, sleep: instantSleep }, + ).catch(e => e)) as { exitCode: number; details?: { field?: string } }; + expect(err.exitCode).toBe(5); + expect(err.details?.field).toBe(field); + expect(fetchImpl).not.toHaveBeenCalled(); + }, + ); + + it('refuses --report-file reports/ before dispatch', async () => { + const { env } = makeCreds(); + const fetchImpl = vi.fn(makeFetch(() => ({ body: { accepted: [] } }))); + const err = (await runTestlistRun( + { ...runBase, listId: 'list-1', wait: true, report: 'junit', reportFile: 'reports/' }, + { env, fetchImpl, stdout: () => {}, stderr: () => {}, sleep: instantSleep }, + ).catch(e => e)) as ApiError; + expect(err.exitCode).toBe(5); + expect(err.details).toMatchObject({ + field: 'report-file', + reason: 'must point to a file, not a directory', + }); + expect(fetchImpl).not.toHaveBeenCalled(); + }); + it('gh-output: an all-conflict --wait run emits annotations + summary before exiting 6', async () => { const { env } = makeCreds(); const dir = mkdtempSync(join(tmpdir(), 'cli-testlist-conflict-')); diff --git a/src/commands/testlist.ts b/src/commands/testlist.ts index fe65ac7f..ec0160ad 100644 --- a/src/commands/testlist.ts +++ b/src/commands/testlist.ts @@ -55,6 +55,7 @@ import { parseJUnitReportFormat, skippedJUnitResultsFromSummary, writeJUnitReportFile, + assertArtifactPath, type JUnitReportFormat, type JUnitTestResult, } from '../lib/junit-report.js'; @@ -536,6 +537,10 @@ function validateTestlistRunOptions(opts: RunTestlistRunOptions): void { '--summary-file requires --wait (it reduces the terminal run result). Add --wait.', ); } + if (opts.report === 'junit' && opts.reportFile !== undefined) { + assertArtifactPath('report-file', opts.reportFile); + } + if (opts.summaryFile !== undefined) assertArtifactPath('summary-file', opts.summaryFile); } export async function runTestlistRun( diff --git a/src/lib/junit-report.test.ts b/src/lib/junit-report.test.ts index 5e88eaad..2c4c7029 100644 --- a/src/lib/junit-report.test.ts +++ b/src/lib/junit-report.test.ts @@ -1,9 +1,18 @@ -import { mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs'; +import { + chmodSync, + mkdirSync, + mkdtempSync, + readFileSync, + rmSync, + symlinkSync, + writeFileSync, +} from 'node:fs'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; import { describe, expect, it } from 'vitest'; import { ApiError } from './errors.js'; import { + assertArtifactPath, assertJUnitReportOptions, buildJUnitReport, durationSecondsBetween, @@ -690,3 +699,93 @@ describe('per-row classname for batches spanning projects', () => { ); }); }); + +describe('assertArtifactPath', () => { + it('returns the resolved path when the parent directory exists', () => { + const dir = mkdtempSync(join(tmpdir(), 'cli-artifact-')); + expect(assertArtifactPath('summary-file', join(dir, 'summary.json'))).toBe( + join(dir, 'summary.json'), + ); + rmSync(dir, { recursive: true, force: true }); + }); + + it('refuses a missing parent directory with the flag name', () => { + const dir = mkdtempSync(join(tmpdir(), 'cli-artifact-')); + expect(() => assertArtifactPath('summary-file', join(dir, 'nope', 's.json'))).toThrow(ApiError); + expect(() => assertArtifactPath('report-file', join(dir, 'nope', 'r.xml'))).toThrow( + expect.objectContaining({ details: expect.objectContaining({ field: 'report-file' }) }), + ); + rmSync(dir, { recursive: true, force: true }); + }); + + it('reports an unreadable target instead of treating it as absent', () => { + const dir = mkdtempSync(join(tmpdir(), 'cli-artifact-')); + // a <-> b symlink loop: stat fails with ELOOP, which must not pass as "file not created yet" + // eslint-disable-next-line security/detect-non-literal-fs-filename -- create a symlink only inside this test's fresh temporary directory. + symlinkSync(join(dir, 'b'), join(dir, 'a')); + // eslint-disable-next-line security/detect-non-literal-fs-filename -- complete the loop inside the same isolated temporary directory. + symlinkSync(join(dir, 'a'), join(dir, 'b')); + expect(() => assertArtifactPath('report-file', join(dir, 'a'))).toThrow( + expect.objectContaining({ + details: expect.objectContaining({ + field: 'report-file', + reason: expect.stringMatching(/^cannot access .*ELOOP$/), + }), + }), + ); + rmSync(dir, { recursive: true, force: true }); + }); + + it.skipIf(process.platform === 'win32' || process.getuid?.() === 0)( + 'reports a permission error on the parent instead of "does not exist"', + () => { + const dir = mkdtempSync(join(tmpdir(), 'cli-artifact-')); + // eslint-disable-next-line security/detect-non-literal-fs-filename -- create the permission fixture inside this test's fresh temporary directory. + mkdirSync(join(dir, 'locked', 'out'), { recursive: true }); + // eslint-disable-next-line security/detect-non-literal-fs-filename -- restrict only the temporary fixture directory and restore it in finally. + chmodSync(join(dir, 'locked'), 0o000); + try { + expect(() => + assertArtifactPath('summary-file', join(dir, 'locked', 'out', 's.json')), + ).toThrow( + expect.objectContaining({ + details: expect.objectContaining({ + field: 'summary-file', + reason: expect.stringMatching(/^cannot access parent directory .*EACCES$/), + }), + }), + ); + } finally { + // eslint-disable-next-line security/detect-non-literal-fs-filename -- restore permissions on the temporary fixture before cleanup. + chmodSync(join(dir, 'locked'), 0o755); + rmSync(dir, { recursive: true, force: true }); + } + }, + ); + + it('refuses an existing directory as the target', () => { + const dir = mkdtempSync(join(tmpdir(), 'cli-artifact-')); + // eslint-disable-next-line security/detect-non-literal-fs-filename -- make a target-directory fixture inside this test's fresh temporary directory. + mkdirSync(join(dir, 'sub')); + expect(() => assertArtifactPath('report-file', join(dir, 'sub'))).toThrow(ApiError); + rmSync(dir, { recursive: true, force: true }); + }); + + it('refuses an empty path', () => { + expect(() => assertArtifactPath('summary-file', '')).toThrow(ApiError); + }); + + it.each(['reports/', 'reports\\'])( + 'refuses a raw trailing separator before resolving %s', + path => { + expect(() => assertArtifactPath('report-file', path)).toThrow( + expect.objectContaining({ + details: expect.objectContaining({ + field: 'report-file', + reason: 'must point to a file, not a directory', + }), + }), + ); + }, + ); +}); diff --git a/src/lib/junit-report.ts b/src/lib/junit-report.ts index 08f61211..dc89636a 100644 --- a/src/lib/junit-report.ts +++ b/src/lib/junit-report.ts @@ -1,5 +1,5 @@ -import { createWriteStream } from 'node:fs'; -import { rename, stat, unlink } from 'node:fs/promises'; +import { createWriteStream, statSync } from 'node:fs'; +import { rename, unlink } from 'node:fs/promises'; import { basename, dirname, isAbsolute, join, resolve } from 'node:path'; import { randomUUID } from 'node:crypto'; import { localValidationError, TransportError } from './errors.js'; @@ -350,34 +350,63 @@ export function skippedJUnitResultsFromSummary( .map(row => ({ testId: row.testId, status: 'skipped', skipReason: row.error ?? row.status })); } -async function assertReportFileParent(rawPath: string): Promise { +function errorCode(err: unknown): string { + return (err as NodeJS.ErrnoException | undefined)?.code ?? 'unknown error'; +} + +function isAbsentPathError(err: unknown): boolean { + const code = errorCode(err); + return code === 'ENOENT' || code === 'ENOTDIR'; +} + +/** + * Check that an output artifact path (`--report-file`, `--summary-file`) can be + * written: non-empty, not a directory, and inside an existing directory. + * Returns the resolved absolute path. + * + * Called in each command's pre-flight, before anything is dispatched or billed, + * so a mistyped directory fails fast with exit 5 instead of surfacing only after + * a full batch has run. The write itself re-checks (the directory can vanish in + * between). + */ +export function assertArtifactPath(field: 'report-file' | 'summary-file', rawPath: string): string { if (typeof rawPath !== 'string' || rawPath.length === 0) { - throw localValidationError('report-file', 'must be a non-empty file path'); + throw localValidationError(field, 'must be a non-empty file path'); } - const resolved = isAbsolute(rawPath) ? rawPath : resolve(process.cwd(), rawPath); - if (resolved.endsWith('/') || resolved.endsWith('\\')) { - throw localValidationError('report-file', 'must point to a file, not a directory'); + if (/[\\/]$/.test(rawPath)) { + throw localValidationError(field, 'must point to a file, not a directory'); } + const resolved = isAbsolute(rawPath) ? rawPath : resolve(process.cwd(), rawPath); const parent = dirname(resolved); let parentStat; try { - parentStat = await stat(parent); - } catch { - throw localValidationError('report-file', `parent directory does not exist: ${parent}`); + // eslint-disable-next-line security/detect-non-literal-fs-filename -- inspect the user-selected artifact parent before dispatch; no file content is read. + parentStat = statSync(parent); + } catch (err) { + if (isAbsentPathError(err)) + throw localValidationError(field, `parent directory does not exist: ${parent}`); + throw localValidationError( + field, + `cannot access parent directory ${parent}: ${errorCode(err)}`, + ); } if (!parentStat.isDirectory()) { - throw localValidationError('report-file', `parent path is not a directory: ${parent}`); + throw localValidationError(field, `parent path is not a directory: ${parent}`); } let targetStat; try { - targetStat = await stat(resolved); - } catch { - return resolved; + // eslint-disable-next-line security/detect-non-literal-fs-filename -- inspect the user-selected artifact target to reject directories and inaccessible paths before dispatch. + targetStat = statSync(resolved); + } catch (err) { + // Only a missing target is fine (it will be created); anything else (EACCES, ELOOP...) would fail the + // write after the run was dispatched, which is exactly what this pre-flight exists to prevent. + if (isAbsentPathError(err)) return resolved; + throw localValidationError(field, `cannot access ${resolved}: ${errorCode(err)}`); } if (targetStat.isDirectory()) { - throw localValidationError('report-file', `must point to a file, not a directory: ${resolved}`); + throw localValidationError(field, `must point to a file, not a directory: ${resolved}`); } return resolved; } @@ -386,7 +415,7 @@ async function assertReportFileParent(rawPath: string): Promise { * Atomically write JUnit XML to `--report-file` (temp sibling + rename). */ export async function writeJUnitReportFile(rawPath: string, xml: string): Promise { - const resolved = await assertReportFileParent(rawPath); + const resolved = assertArtifactPath('report-file', rawPath); const parent = dirname(resolved); const tmpPath = join(parent, `.${basename(resolved)}.tmp-${randomUUID()}`);