mirror of
https://github.com/garrytan/gstack.git
synced 2026-09-09 14:38:59 +02:00
fix(redact): a typo'd subcommand no longer exits 0 having done nothing
main() recognised exactly two subcommands and let everything else fall through
to the stdin scan. On empty stdin that prints "(no findings)" and exits 0, so:
$ gstack-redact install-prepush-hooks # plural typo
gstack-redact scan — repo UNKNOWN
(no findings)
$ echo $?
0
No hook was installed, and the operator has every reason to believe the
credential guard is armed. A guard that silently no-ops must never exit 0.
Two smaller faults in the same dispatch, both of which lead people here:
- There was no --help handler, so `gstack-redact --help` fell through to the
scanner. Piping a credential to it scanned the secret and exited 3.
- With no piped input and no --from-file, readInput() blocks on readSync(fd 0)
until an EOF that an interactive terminal never sends. That prints nothing
at all, so it reads as a hang rather than as "this is a filter, feed it".
Now: --help/-h/help prints usage and exits 0; an unrecognised positional
prints the offender and exits 1; a TTY with nothing piped in prints usage
instead of blocking. "scan" stays accepted, because the human output header
reads "gstack-redact scan — repo …" and that is what people type.
Usage errors exit 1, deliberately not 2 or 3. Those mean MEDIUM and HIGH
findings and callers gate dispatch on them, so a usage error exiting 2 would
be read as "medium findings — prompt the user". A test pins that.
Tests: 4 written failing first, then fixed. Full suite 7,722 pass / 0 fail.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
@@ -199,13 +199,67 @@ function humanTable(findings: Finding[]): string {
|
||||
return rows.join("\n");
|
||||
}
|
||||
|
||||
/**
|
||||
* Usage. Exits 0 when asked for (--help), 1 when the invocation was wrong.
|
||||
*
|
||||
* Deliberately NOT 2 or 3: those mean MEDIUM and HIGH findings, and callers
|
||||
* gate dispatch on them (see the exit-code table at the top). A usage error
|
||||
* that exited 2 would be read as "medium findings — prompt the user".
|
||||
*/
|
||||
function printUsage(code: number): never {
|
||||
const out = code === 0 ? process.stdout : process.stderr;
|
||||
out.write(
|
||||
"gstack-redact — scan text for secrets/PII/legal content.\n" +
|
||||
"\n" +
|
||||
"Reads the text to scan from STDIN, or from --from-file PATH. It is a\n" +
|
||||
"filter: with nothing piped in it has nothing to scan.\n" +
|
||||
"\n" +
|
||||
" git diff | gstack-redact --repo-visibility private\n" +
|
||||
" gstack-redact --from-file notes.md --json\n" +
|
||||
"\n" +
|
||||
"Subcommands:\n" +
|
||||
" install-prepush-hook install the managed git pre-push credential guard\n" +
|
||||
" uninstall-prepush-hook remove it\n" +
|
||||
"\n" +
|
||||
"Flags: --json --repo-visibility V --from-file PATH --allowlist PATH\n" +
|
||||
" --self-email EMAIL --repo-public-emails PATH --auto-redact IDS\n" +
|
||||
" --max-bytes N\n" +
|
||||
"\n" +
|
||||
"Exit: 0 clean · 1 usage error · 2 MEDIUM present · 3 HIGH present\n",
|
||||
);
|
||||
process.exit(code);
|
||||
}
|
||||
|
||||
function main() {
|
||||
// Subcommands (positional, not flags).
|
||||
const sub = process.argv[2];
|
||||
if (sub === "install-prepush-hook") return installPrepushHook();
|
||||
if (sub === "uninstall-prepush-hook") return uninstallPrepushHook();
|
||||
if (sub === "--help" || sub === "-h" || sub === "help") return printUsage(0);
|
||||
|
||||
// An unrecognized POSITIONAL is a typo, not input. This used to fall through
|
||||
// to the stdin scan, which on empty stdin prints "(no findings)" and exits 0
|
||||
// — so `install-prepush-hooks` (plural) installed nothing and still looked
|
||||
// like success, leaving the credential guard absent while the operator
|
||||
// believed it was armed. A guard that no-ops must never exit 0.
|
||||
//
|
||||
// "scan" is exempt: the human output header reads "gstack-redact scan —
|
||||
// repo …", so people reasonably type it. It stays an alias for the default.
|
||||
// Flags start with "-" and are parsed further down, so only bare words land
|
||||
// here.
|
||||
if (sub !== undefined && sub !== "scan" && !sub.startsWith("-")) {
|
||||
process.stderr.write(`gstack-redact: unknown subcommand "${sub}"\n\n`);
|
||||
return printUsage(1);
|
||||
}
|
||||
|
||||
const opts = buildOpts();
|
||||
|
||||
// Nothing piped in and no --from-file: readInput() below blocks on
|
||||
// readSync(fd 0) until EOF, which on an interactive terminal never comes.
|
||||
// That prints nothing at all and is indistinguishable from a crash or a
|
||||
// slow scan. Show usage instead of hanging silently.
|
||||
if (!arg("--from-file") && process.stdin.isTTY) return printUsage(1);
|
||||
|
||||
const input = readInput();
|
||||
|
||||
// Auto-redact mode: print redacted body to stdout, diff to stderr, exit 0.
|
||||
|
||||
@@ -95,3 +95,47 @@ describe("gstack-redact oversize fails closed", () => {
|
||||
expect(stdout).toContain("too large");
|
||||
});
|
||||
});
|
||||
|
||||
describe("gstack-redact argv dispatch", () => {
|
||||
// The bug: main() recognised exactly two subcommands and let everything else
|
||||
// fall through to the stdin scan, which reports "no findings" and exits 0.
|
||||
// So `install-prepush-hooks` (plural typo) installed no hook and still looked
|
||||
// like success — the credential guard silently absent while the operator
|
||||
// believes it is armed. A guard that no-ops must never exit 0.
|
||||
test("a typo'd install subcommand fails loudly instead of exiting 0", () => {
|
||||
const { code, stderr } = run(["install-prepush-hooks"], "");
|
||||
expect(code).not.toBe(0);
|
||||
expect(stderr).toContain("unknown subcommand");
|
||||
});
|
||||
|
||||
test("an unknown positional never reports a clean scan", () => {
|
||||
const { code, stdout } = run(["totally-bogus"], "");
|
||||
expect(code).not.toBe(0);
|
||||
expect(stdout).not.toContain("HIGH=0");
|
||||
});
|
||||
|
||||
// Usage errors must not collide with the findings codes (2 = MEDIUM,
|
||||
// 3 = HIGH); a caller gating on those would read a typo as "findings".
|
||||
test("usage errors exit 1, not a findings code", () => {
|
||||
expect(run(["totally-bogus"], "").code).toBe(1);
|
||||
});
|
||||
|
||||
test("--help prints usage and exits 0 without scanning", () => {
|
||||
const { code, stdout } = run(["--help"], "key AKIA1234567890ABCDEF");
|
||||
expect(code).toBe(0);
|
||||
expect(stdout).toContain("STDIN");
|
||||
expect(stdout).not.toContain("HIGH=1");
|
||||
});
|
||||
|
||||
// "scan" is what the human output header ("gstack-redact scan — repo …")
|
||||
// invites people to type, so it stays an accepted alias for the default
|
||||
// filter mode. Rejecting it would break that muscle memory for no gain.
|
||||
test("the 'scan' alias still scans normally", () => {
|
||||
expect(run(["scan"], "key AKIA1234567890ABCDEF").code).toBe(3);
|
||||
expect(run(["scan"], "just prose").code).toBe(0);
|
||||
});
|
||||
|
||||
test("flags are still parsed, not mistaken for subcommands", () => {
|
||||
expect(run(["--json"], "just prose").code).toBe(0);
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user