Skip to content

Commit fa3422c

Browse files
committed
fix(project): address CodeRabbit review — dry-run bypass and TOCTOU in readSecretFileGuarded
- 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)
1 parent 080a929 commit fa3422c

2 files changed

Lines changed: 82 additions & 19 deletions

File tree

src/commands/project.test.ts

Lines changed: 52 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { mkdirSync, mkdtempSync, writeFileSync } from 'node:fs';
1+
import { chmodSync, mkdirSync, mkdtempSync, writeFileSync } from 'node:fs';
22
import { tmpdir } from 'node:os';
33
import { join } from 'node:path';
44
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest';
@@ -1605,4 +1605,55 @@ describe('#282 — secret --*-file flags are guarded (structured error, exit 5,
16051605
),
16061606
).rejects.toMatchObject({ code: 'VALIDATION_ERROR', exitCode: 5 });
16071607
});
1608+
1609+
it('runAutoAuth --dry-run with missing --password-file skips filesystem (returns sample)', async () => {
1610+
const { credentialsPath } = makeCreds();
1611+
let fetched = false;
1612+
const fetchImpl = makeFetch(() => {
1613+
fetched = true;
1614+
return { body: {} };
1615+
});
1616+
const result = await runAutoAuth(
1617+
{
1618+
profile: 'default',
1619+
output: 'json',
1620+
debug: false,
1621+
dryRun: true,
1622+
projectId: 'p1',
1623+
method: 'password',
1624+
inject: 'bearer',
1625+
passwordFile: missingPath(),
1626+
},
1627+
{ credentialsPath, fetchImpl, stdout: () => {}, stderr: () => {} },
1628+
);
1629+
expect(fetched).toBe(false);
1630+
// blindfold: manual — dry-run skips file reads; returns sample with the projectId we passed in
1631+
expect(result.projectId).toBe('p1');
1632+
});
1633+
1634+
it('runCredential --credential-file unreadable after stat → VALIDATION_ERROR (exit 5)', async () => {
1635+
if (process.getuid?.() === 0) return; // root bypasses permission checks
1636+
const { credentialsPath } = makeCreds();
1637+
const dir = mkdtempSync(join(tmpdir(), 'cli-cred-mode-'));
1638+
const f = join(dir, 'secret.txt');
1639+
writeFileSync(f, 'tok');
1640+
chmodSync(f, 0o000);
1641+
try {
1642+
await expect(
1643+
runCredential(
1644+
{
1645+
profile: 'default',
1646+
output: 'json',
1647+
debug: false,
1648+
projectId: 'p1',
1649+
authType: 'API key',
1650+
credentialFile: f,
1651+
},
1652+
deps(credentialsPath),
1653+
),
1654+
).rejects.toMatchObject({ code: 'VALIDATION_ERROR', exitCode: 5 });
1655+
} finally {
1656+
chmodSync(f, 0o644);
1657+
}
1658+
});
16081659
});

src/commands/project.ts

Lines changed: 30 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -574,7 +574,26 @@ export async function runAutoAuth(
574574
throw localValidationError(`--inject must be one of: ${AUTO_AUTH_INJECTS.join(', ')}`);
575575
}
576576

577+
const enabled = opts.disable !== true;
578+
579+
const idempotencyKey = opts.idempotencyKey ?? `cli-proj-autoauth-${randomUUID()}`;
580+
if (opts.idempotencyKey === undefined && (opts.output === 'json' || opts.verbose || opts.debug)) {
581+
stderr(`idempotency-key: ${idempotencyKey}`);
582+
}
583+
584+
if (opts.dryRun) {
585+
const sample: CliProjectAutoAuthResponse = {
586+
projectId: opts.projectId,
587+
enabled,
588+
method: opts.method,
589+
inject: opts.inject,
590+
};
591+
out.print(sample, data => renderAutoAuthText(data as CliProjectAutoAuthResponse));
592+
return sample;
593+
}
594+
577595
// Resolve secrets from --*-file variants so they stay out of shell history.
596+
// Placed after the dry-run early return so --dry-run never touches the filesystem.
578597
const password =
579598
opts.password ??
580599
(opts.passwordFile !== undefined
@@ -591,7 +610,6 @@ export async function runAutoAuth(
591610
? readSecretFileGuarded(opts.refreshTokenFile, 'refresh-token-file')
592611
: undefined);
593612

594-
const enabled = opts.disable !== true;
595613
const body: Record<string, unknown> = { enabled, method: opts.method, inject: opts.inject };
596614
const maybe = (k: string, v: string | undefined): void => {
597615
if (v !== undefined) body[k] = v;
@@ -611,22 +629,6 @@ export async function runAutoAuth(
611629
maybe('scope', opts.scope);
612630
maybe('region', opts.region);
613631

614-
const idempotencyKey = opts.idempotencyKey ?? `cli-proj-autoauth-${randomUUID()}`;
615-
if (opts.idempotencyKey === undefined && (opts.output === 'json' || opts.verbose || opts.debug)) {
616-
stderr(`idempotency-key: ${idempotencyKey}`);
617-
}
618-
619-
if (opts.dryRun) {
620-
const sample: CliProjectAutoAuthResponse = {
621-
projectId: opts.projectId,
622-
enabled,
623-
method: opts.method,
624-
inject: opts.inject,
625-
};
626-
out.print(sample, data => renderAutoAuthText(data as CliProjectAutoAuthResponse));
627-
return sample;
628-
}
629-
630632
const client = makeClient(opts, deps);
631633
const res = await client.put<CliProjectAutoAuthResponse>(
632634
`/projects/${encodeURIComponent(opts.projectId)}/auto-auth`,
@@ -1127,7 +1129,17 @@ function readSecretFileGuarded(path: string, flagName: string): string {
11271129
});
11281130
}
11291131

1130-
return readFileSync(absolute, 'utf8').trim();
1132+
let content: string;
1133+
try {
1134+
content = readFileSync(absolute, 'utf8');
1135+
} catch (err) {
1136+
const message = err instanceof Error ? err.message : String(err);
1137+
throw secretFileError(flagName, 'must point to a readable file', {
1138+
path: absolute,
1139+
error: message,
1140+
});
1141+
}
1142+
return content.trim();
11311143
}
11321144

11331145
function secretFileError(

0 commit comments

Comments
 (0)