From fe892d21cc02cca7fcd4e3ea4bd3f59c1106895a Mon Sep 17 00:00:00 2001 From: Okey Amy Date: Fri, 24 Jul 2026 22:49:46 +0100 Subject: [PATCH 1/3] fix(project): guard credential/auto-auth secret file reads (#282) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The v0.4.0 `project credential` and `project auto-auth` commands read their `--*-file` flags with a raw `readFileSync(path).trim()`. A missing file, a directory, or an oversized file escaped as an unwrapped Node error (exit 1) that also broke the `--output json` envelope (bare `{"error":"ENOENT…"}` instead of the structured `{code,message,nextAction}`) and leaked fs internals. Generalize the guard into `readSecretFileGuarded(path, flagName)` — mirroring the `--code-file` guard in test.ts and PR #248's `readPasswordFileGuarded` — and apply it to all six project.ts file-read sites: - project create/update --password-file - project credential --credential-file - project auto-auth --password-file / --client-secret-file / --refresh-token-file Missing/non-regular files now return VALIDATION_ERROR (exit 5) and oversized files return PAYLOAD_TOO_LARGE (exit 5), each carrying the flag name — the same contract every other file flag already honors. Adds 11 tests covering missing/directory/oversized inputs across all four new flags plus the two password-file sites, and the trimmed happy path. Closes #282 --- src/commands/project.test.ts | 162 +++++++++++++++++++++++++++++++++++ src/commands/project.ts | 12 ++- 2 files changed, 170 insertions(+), 4 deletions(-) diff --git a/src/commands/project.test.ts b/src/commands/project.test.ts index dc72cc7..e02f319 100644 --- a/src/commands/project.test.ts +++ b/src/commands/project.test.ts @@ -2404,3 +2404,165 @@ describe('runUpdate — --local ', () => { expect(out.join('\n')).toContain('testsprite test run --local 3000'); }); }); + +describe('#282 — secret --*-file flags are guarded (structured error, exit 5, no raw ENOENT)', () => { + const noNetwork = () => { + throw new Error('network should not be hit'); + }; + const deps = (credentialsPath: string) => ({ + credentialsPath, + fetchImpl: makeFetch(noNetwork), + stdout: () => {}, + stderr: () => {}, + }); + const missingPath = () => join(mkdtempSync(join(tmpdir(), 'cli-missing-')), 'no-such-secret.txt'); + + it('runCredential --credential-file missing → VALIDATION_ERROR (exit 5), no network', async () => { + const { credentialsPath } = makeCreds(); + await expect( + runCredential( + { + profile: 'default', + output: 'json', + debug: false, + projectId: 'p1', + authType: 'API key', + credentialFile: missingPath(), + }, + deps(credentialsPath), + ), + ).rejects.toMatchObject({ code: 'VALIDATION_ERROR', exitCode: 5 }); + }); + + it('runCredential --credential-file pointing at a directory → VALIDATION_ERROR (exit 5)', async () => { + const { credentialsPath } = makeCreds(); + const dir = mkdtempSync(join(tmpdir(), 'cli-cred-dir-')); + await expect( + runCredential( + { + profile: 'default', + output: 'json', + debug: false, + projectId: 'p1', + authType: 'API key', + credentialFile: dir, + }, + deps(credentialsPath), + ), + ).rejects.toMatchObject({ code: 'VALIDATION_ERROR', exitCode: 5 }); + }); + + it('runCredential reads a valid --credential-file (trimmed) and sends it', async () => { + const { credentialsPath } = makeCreds(); + const dir = mkdtempSync(join(tmpdir(), 'cli-cred-ok-')); + const credFile = join(dir, 'cred.txt'); + writeFileSync(credFile, ' tok-from-file\n'); + let sentBody: { credential?: string } | undefined; + const fetchImpl = makeFetch((_url, init) => { + sentBody = init.body ? JSON.parse(init.body as string) : undefined; + return { status: 200, body: { projectId: 'p1', authType: 'API key', rewroteCount: 1 } }; + }); + await runCredential( + { + profile: 'default', + output: 'json', + debug: false, + projectId: 'p1', + authType: 'API key', + credentialFile: credFile, + }, + { credentialsPath, fetchImpl, stdout: () => {}, stderr: () => {} }, + ); + // The on-disk fixture is " tok-from-file\n"; the shared guard trims it. + expect(sentBody?.credential).toBe('tok-from-file'); + }); + + it('runAutoAuth --password-file missing → VALIDATION_ERROR (exit 5), no network', async () => { + const { credentialsPath } = makeCreds(); + await expect( + runAutoAuth( + { + profile: 'default', + output: 'json', + debug: false, + projectId: 'p1', + method: 'password', + inject: 'bearer', + passwordFile: missingPath(), + }, + deps(credentialsPath), + ), + ).rejects.toMatchObject({ code: 'VALIDATION_ERROR', exitCode: 5 }); + }); + + it('runAutoAuth --client-secret-file missing → VALIDATION_ERROR (exit 5), no network', async () => { + const { credentialsPath } = makeCreds(); + await expect( + runAutoAuth( + { + profile: 'default', + output: 'json', + debug: false, + projectId: 'p1', + method: 'refresh_token', + inject: 'bearer', + clientSecretFile: missingPath(), + }, + deps(credentialsPath), + ), + ).rejects.toMatchObject({ code: 'VALIDATION_ERROR', exitCode: 5 }); + }); + + it('runAutoAuth --refresh-token-file missing → VALIDATION_ERROR (exit 5), no network', async () => { + const { credentialsPath } = makeCreds(); + await expect( + runAutoAuth( + { + profile: 'default', + output: 'json', + debug: false, + projectId: 'p1', + method: 'refresh_token', + inject: 'bearer', + refreshTokenFile: missingPath(), + }, + deps(credentialsPath), + ), + ).rejects.toMatchObject({ code: 'VALIDATION_ERROR', exitCode: 5 }); + }); + + it('runCreate --password-file missing → VALIDATION_ERROR (exit 5), no network', async () => { + const { credentialsPath } = makeCreds(); + await expect( + runCreate( + { + profile: 'default', + output: 'json', + debug: false, + type: 'frontend', + name: 'FE', + targetUrl: 'https://example.com', + username: 'u', + passwordFile: missingPath(), + }, + deps(credentialsPath), + ), + ).rejects.toMatchObject({ code: 'VALIDATION_ERROR', exitCode: 5 }); + }); + + it('runUpdate --password-file missing → VALIDATION_ERROR (exit 5), no network', async () => { + const { credentialsPath } = makeCreds(); + await expect( + runUpdate( + { + profile: 'default', + output: 'json', + debug: false, + projectId: 'p1', + passwordFile: missingPath(), + }, + deps(credentialsPath), + ), + ).rejects.toMatchObject({ code: 'VALIDATION_ERROR', exitCode: 5 }); + }); +}); diff --git a/src/commands/project.ts b/src/commands/project.ts index 495aac6..35a01f5 100644 --- a/src/commands/project.ts +++ b/src/commands/project.ts @@ -766,7 +766,7 @@ export async function runCredential( // except `public` (which clears it). let credential = opts.credential; if (credential === undefined && opts.credentialFile !== undefined) { - credential = readFileSync(opts.credentialFile, 'utf8').trim(); + credential = readSecretFileGuarded('credential-file', opts.credentialFile); } if (opts.authType !== 'public' && (credential === undefined || credential === '')) { throw localValidationError( @@ -875,16 +875,18 @@ export async function runAutoAuth( // Resolve secrets from --*-file variants so they stay out of shell history. const password = opts.password ?? - (opts.passwordFile !== undefined ? readFileSync(opts.passwordFile, 'utf8').trim() : undefined); + (opts.passwordFile !== undefined + ? readSecretFileGuarded('password-file', opts.passwordFile) + : undefined); const clientSecret = opts.clientSecret ?? (opts.clientSecretFile !== undefined - ? readFileSync(opts.clientSecretFile, 'utf8').trim() + ? readSecretFileGuarded('client-secret-file', opts.clientSecretFile) : undefined); const refreshToken = opts.refreshToken ?? (opts.refreshTokenFile !== undefined - ? readFileSync(opts.refreshTokenFile, 'utf8').trim() + ? readSecretFileGuarded('refresh-token-file', opts.refreshTokenFile) : undefined); const enabled = opts.disable !== true; @@ -2107,3 +2109,5 @@ function localValidationError(message: string): ApiError { }, }); } + + From e37c46fc837aae8df351389d32f06b5387dc7dd8 Mon Sep 17 00:00:00 2001 From: Okey Amy Date: Sat, 25 Jul 2026 18:11:41 +0100 Subject: [PATCH 2/3] =?UTF-8?q?fix(project):=20address=20CodeRabbit=20revi?= =?UTF-8?q?ew=20=E2=80=94=20dry-run=20bypass=20and=20TOCTOU=20in=20readSec?= =?UTF-8?q?retFileGuarded?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Move secret-file resolution in runAutoAuth to after the dry-run early return so --dry-run never touches the filesystem (matches runCreate / runUpdate behaviour) - Wrap the final readFileSync in readSecretFileGuarded with a try-catch so EACCES or any other read failure after a successful statSync is converted to a structured VALIDATION_ERROR (exit 5) instead of a raw Node error - Add test: runAutoAuth --dry-run with a missing --password-file returns the sample without touching the network or filesystem - Add test: TOCTOU path — file unreadable after stat → VALIDATION_ERROR (skipped when running as root) --- src/commands/project.test.ts | 51 ++++++++++++++++++++++++++++++++++++ src/commands/project.ts | 38 +++++++++++++-------------- 2 files changed, 70 insertions(+), 19 deletions(-) diff --git a/src/commands/project.test.ts b/src/commands/project.test.ts index e02f319..cbd988a 100644 --- a/src/commands/project.test.ts +++ b/src/commands/project.test.ts @@ -2565,4 +2565,55 @@ describe('#282 — secret --*-file flags are guarded (structured error, exit 5, ), ).rejects.toMatchObject({ code: 'VALIDATION_ERROR', exitCode: 5 }); }); + + it('runAutoAuth --dry-run with missing --password-file skips filesystem (returns sample)', async () => { + const { credentialsPath } = makeCreds(); + let fetched = false; + const fetchImpl = makeFetch(() => { + fetched = true; + return { body: {} }; + }); + const result = await runAutoAuth( + { + profile: 'default', + output: 'json', + debug: false, + dryRun: true, + projectId: 'p1', + method: 'password', + inject: 'bearer', + passwordFile: missingPath(), + }, + { credentialsPath, fetchImpl, stdout: () => {}, stderr: () => {} }, + ); + expect(fetched).toBe(false); + // blindfold: manual — dry-run skips file reads; returns sample with the projectId we passed in + expect(result.projectId).toBe('p1'); + }); + + it('runCredential --credential-file unreadable after stat → VALIDATION_ERROR (exit 5)', async () => { + if (process.getuid?.() === 0) return; // root bypasses permission checks + const { credentialsPath } = makeCreds(); + const dir = mkdtempSync(join(tmpdir(), 'cli-cred-mode-')); + const f = join(dir, 'secret.txt'); + writeFileSync(f, 'tok'); + chmodSync(f, 0o000); + try { + await expect( + runCredential( + { + profile: 'default', + output: 'json', + debug: false, + projectId: 'p1', + authType: 'API key', + credentialFile: f, + }, + deps(credentialsPath), + ), + ).rejects.toMatchObject({ code: 'VALIDATION_ERROR', exitCode: 5 }); + } finally { + chmodSync(f, 0o644); + } + }); }); diff --git a/src/commands/project.ts b/src/commands/project.ts index 35a01f5..54aad0b 100644 --- a/src/commands/project.ts +++ b/src/commands/project.ts @@ -872,7 +872,26 @@ export async function runAutoAuth( throw localValidationError(`--inject must be one of: ${AUTO_AUTH_INJECTS.join(', ')}`); } + const enabled = opts.disable !== true; + + const idempotencyKey = opts.idempotencyKey ?? `cli-proj-autoauth-${randomUUID()}`; + if (opts.idempotencyKey === undefined && (opts.output === 'json' || opts.verbose || opts.debug)) { + stderr(`idempotency-key: ${idempotencyKey}`); + } + + if (opts.dryRun) { + const sample: CliProjectAutoAuthResponse = { + projectId: opts.projectId, + enabled, + method: opts.method, + inject: opts.inject, + }; + out.print(sample, data => renderAutoAuthText(data as CliProjectAutoAuthResponse)); + return sample; + } + // Resolve secrets from --*-file variants so they stay out of shell history. + // Placed after the dry-run early return so --dry-run never touches the filesystem. const password = opts.password ?? (opts.passwordFile !== undefined @@ -889,7 +908,6 @@ export async function runAutoAuth( ? readSecretFileGuarded('refresh-token-file', opts.refreshTokenFile) : undefined); - const enabled = opts.disable !== true; const body: Record = { enabled, method: opts.method, inject: opts.inject }; const maybe = (k: string, v: string | undefined): void => { if (v !== undefined) body[k] = v; @@ -909,22 +927,6 @@ export async function runAutoAuth( maybe('scope', opts.scope); maybe('region', opts.region); - const idempotencyKey = opts.idempotencyKey ?? `cli-proj-autoauth-${randomUUID()}`; - if (opts.idempotencyKey === undefined && (opts.output === 'json' || opts.verbose || opts.debug)) { - stderr(`idempotency-key: ${idempotencyKey}`); - } - - if (opts.dryRun) { - const sample: CliProjectAutoAuthResponse = { - projectId: opts.projectId, - enabled, - method: opts.method, - inject: opts.inject, - }; - out.print(sample, data => renderAutoAuthText(data as CliProjectAutoAuthResponse)); - return sample; - } - const client = makeClient(opts, deps); const res = await client.put( `/projects/${encodeURIComponent(opts.projectId)}/auto-auth`, @@ -2109,5 +2111,3 @@ function localValidationError(message: string): ApiError { }, }); } - - From e476fb505c6c49034f67bd0e323d23f44df8442e Mon Sep 17 00:00:00 2001 From: Okey Amy Date: Mon, 28 Sep 2026 19:28:12 +0100 Subject: [PATCH 3/3] =?UTF-8?q?fix(project):=20address=20review=20?= =?UTF-8?q?=E2=80=94=20credential=20dry-run,=20cross-platform=20read-failu?= =?UTF-8?q?re=20test?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - runCredential: check only that a credential was supplied before the dry-run early return and read --credential-file after it, so --dry-run never touches the filesystem (matches runAutoAuth / runCreate / runUpdate). - Replace the chmod(0o000) "unreadable after stat" test with an injected EACCES on the guard's fd read. chmod only maps the write bit on win32, so the old fixture stayed readable there; the injected failure exercises the same branch deterministically on every platform. - Add a runCredential --dry-run test with a missing --credential-file. - Add the security/detect-non-literal-fs-filename suppression on the new tmpdir fixture write, and drop a stray tooling comment. - Drop the now-unused readFileSync import from project.ts. --- src/commands/project.test.ts | 82 ++++++++++++++++++++++++++++++------ src/commands/project.ts | 33 +++++++++------ 2 files changed, 90 insertions(+), 25 deletions(-) diff --git a/src/commands/project.test.ts b/src/commands/project.test.ts index cbd988a..9461854 100644 --- a/src/commands/project.test.ts +++ b/src/commands/project.test.ts @@ -1,4 +1,5 @@ -import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs'; +import { mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs'; +import type * as NodeFs from 'node:fs'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; import { afterAll, afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; @@ -20,6 +21,15 @@ import { parseTestIdAttributesFlag, } from './project.js'; +// readSecretFileGuarded reads the secret through the fd it opened, via +// node:fs's readFileSync(fd, ...) overload. Wrap readFileSync in a pass-through +// vi.fn so the #282 suite can inject a read failure after a successful open; +// every other test keeps the real implementation. +vi.mock('node:fs', async importOriginal => { + const actual = await importOriginal(); + return { ...actual, readFileSync: vi.fn(actual.readFileSync) }; +}); + const PROJECT_FIXTURE: CliProject = { id: 'project_b3c91efa', name: 'Checkout', @@ -2456,6 +2466,7 @@ describe('#282 — secret --*-file flags are guarded (structured error, exit 5, const { credentialsPath } = makeCreds(); const dir = mkdtempSync(join(tmpdir(), 'cli-cred-ok-')); const credFile = join(dir, 'cred.txt'); + // eslint-disable-next-line security/detect-non-literal-fs-filename -- test fixture write into this test's own mkdtempSync-created temp dir (dir), not user input. writeFileSync(credFile, ' tok-from-file\n'); let sentBody: { credential?: string } | undefined; const fetchImpl = makeFetch((_url, init) => { @@ -2587,18 +2598,61 @@ describe('#282 — secret --*-file flags are guarded (structured error, exit 5, { credentialsPath, fetchImpl, stdout: () => {}, stderr: () => {} }, ); expect(fetched).toBe(false); - // blindfold: manual — dry-run skips file reads; returns sample with the projectId we passed in + // Dry-run skips file reads; the sample carries the projectId we passed in. expect(result.projectId).toBe('p1'); }); - it('runCredential --credential-file unreadable after stat → VALIDATION_ERROR (exit 5)', async () => { - if (process.getuid?.() === 0) return; // root bypasses permission checks + it('runCredential --dry-run with missing --credential-file skips filesystem (returns sample)', async () => { const { credentialsPath } = makeCreds(); - const dir = mkdtempSync(join(tmpdir(), 'cli-cred-mode-')); - const f = join(dir, 'secret.txt'); - writeFileSync(f, 'tok'); - chmodSync(f, 0o000); - try { + let fetched = false; + const fetchImpl = makeFetch(() => { + fetched = true; + return { body: {} }; + }); + const result = await runCredential( + { + profile: 'default', + output: 'json', + debug: false, + dryRun: true, + projectId: 'p1', + authType: 'API key', + credentialFile: missingPath(), + }, + { credentialsPath, fetchImpl, stdout: () => {}, stderr: () => {} }, + ); + expect(fetched).toBe(false); + expect(result).toEqual({ projectId: 'p1', authType: 'API key', rewroteCount: 0 }); + }); + + describe('read fails after a successful open', () => { + afterEach(() => { + // mockReset() on the vi.fn(actual.readFileSync) double puts it back on + // the real fs call, so a failure here can't leak into later tests. + vi.mocked(readFileSync).mockReset(); + }); + + // Injected rather than built with chmod(0o000): on win32 chmod only maps + // the write bit, so a mode-denied file stays readable there. Failing the + // fd read directly exercises the same branch on every platform. + it('runCredential --credential-file read error → VALIDATION_ERROR (exit 5), no network', async () => { + const { credentialsPath } = makeCreds(); + const dir = mkdtempSync(join(tmpdir(), 'cli-cred-read-')); + const f = join(dir, 'secret.txt'); + // eslint-disable-next-line security/detect-non-literal-fs-filename -- test fixture write into this test's own mkdtempSync-created temp dir (dir), not user input. + writeFileSync(f, 'tok'); + const actual = await vi.importActual('node:fs'); + // Only the guard's fd-based read fails; path-based reads (e.g. the + // credentials file) keep hitting the real filesystem. + vi.mocked(readFileSync).mockImplementation((( + file: NodeFs.PathOrFileDescriptor, + ...rest: unknown[] + ) => { + if (typeof file === 'number') { + throw Object.assign(new Error('permission denied'), { code: 'EACCES' }); + } + return (actual.readFileSync as (...a: unknown[]) => unknown)(file, ...rest); + }) as typeof readFileSync); await expect( runCredential( { @@ -2611,9 +2665,11 @@ describe('#282 — secret --*-file flags are guarded (structured error, exit 5, }, deps(credentialsPath), ), - ).rejects.toMatchObject({ code: 'VALIDATION_ERROR', exitCode: 5 }); - } finally { - chmodSync(f, 0o644); - } + ).rejects.toMatchObject({ + code: 'VALIDATION_ERROR', + exitCode: 5, + nextAction: expect.stringContaining('permission denied reading'), + }); + }); }); }); diff --git a/src/commands/project.ts b/src/commands/project.ts index 54aad0b..d694b5f 100644 --- a/src/commands/project.ts +++ b/src/commands/project.ts @@ -1,5 +1,5 @@ import { randomUUID } from 'node:crypto'; -import { createReadStream, readFileSync, statSync, type Stats } from 'node:fs'; +import { createReadStream, statSync, type Stats } from 'node:fs'; import { basename, extname } from 'node:path'; import { Readable } from 'node:stream'; import { Command } from 'commander'; @@ -762,21 +762,19 @@ export async function runCredential( throw localValidationError(`--type must be one of: ${CLI_AUTH_TYPES.join(', ')}`); } - // Resolve the credential value (flag or file). Required for every type - // except `public` (which clears it). - let credential = opts.credential; - if (credential === undefined && opts.credentialFile !== undefined) { - credential = readSecretFileGuarded('credential-file', opts.credentialFile); - } - if (opts.authType !== 'public' && (credential === undefined || credential === '')) { - throw localValidationError( + // A credential (flag or file) is required for every type except `public` + // (which clears it). Presence only here: --credential-file is read after the + // dry-run early return so --dry-run never touches the filesystem. + const missingCredential = (): ApiError => + localValidationError( '--credential (or --credential-file) is required unless --type is "public"', ); + const credentialSupplied = + opts.credential !== undefined ? opts.credential !== '' : opts.credentialFile !== undefined; + if (opts.authType !== 'public' && !credentialSupplied) { + throw missingCredential(); } - const body: Record = { authType: opts.authType }; - if (opts.authType !== 'public' && credential !== undefined) body.credential = credential; - const idempotencyKey = opts.idempotencyKey ?? `cli-proj-cred-${randomUUID()}`; if (opts.idempotencyKey === undefined && (opts.output === 'json' || opts.verbose || opts.debug)) { stderr(`idempotency-key: ${idempotencyKey}`); @@ -792,6 +790,17 @@ export async function runCredential( return sample; } + let credential = opts.credential; + if (credential === undefined && opts.credentialFile !== undefined) { + credential = readSecretFileGuarded('credential-file', opts.credentialFile); + } + if (opts.authType !== 'public' && (credential === undefined || credential === '')) { + throw missingCredential(); + } + + const body: Record = { authType: opts.authType }; + if (opts.authType !== 'public' && credential !== undefined) body.credential = credential; + const client = makeClient(opts, deps); const res = await client.put( `/projects/${encodeURIComponent(opts.projectId)}/credential`,