From bfeb6462eeb3609a32199560d5100f1050d2ace1 Mon Sep 17 00:00:00 2001 From: Jayesh Betala Date: Mon, 1 Jun 2026 13:17:24 +0530 Subject: [PATCH] fix(redact): invalid --max-bytes must not disable the fail-closed guard MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A malformed `--max-bytes` value silently turned the redaction engine's oversize guard from fail-CLOSED into fail-OPEN. `bin/gstack-redact` parsed the flag with `parseInt(...)` and passed the result straight through, so `--max-bytes notanumber` became `maxBytes: NaN`. In `lib/redact-engine.ts`, `opts.maxBytes ?? DEFAULT_MAX_BYTES` only catches null/undefined — `NaN` slipped past, and `byteLen > NaN` is always false, so the "input too large to scan safely" block never fired. A negative value made the opposite happen: every input blocked with a nonsensical `> -5 bytes`. - engine: treat a non-finite or non-positive `maxBytes` as invalid and fall back to the known-good 1 MiB default, so the guard stays intact for every caller regardless of how the cap was computed. - CLI: reject a malformed `--max-bytes` with a clear stderr error and exit 1 (usage error, distinct from the 0/2/3 finding-tier codes) instead of silently passing NaN. - tests: engine regression (invalid cap still fails closed at the default) and CLI contract (malformed flag exits 1). Fixes #1824 Co-Authored-By: Claude Opus 4.8 (1M context) --- bin/gstack-redact | 18 ++++++++++++++++-- lib/redact-engine.ts | 10 +++++++++- test/gstack-redact-cli.test.ts | 11 +++++++++++ test/redact-engine.test.ts | 14 ++++++++++++++ 4 files changed, 50 insertions(+), 3 deletions(-) diff --git a/bin/gstack-redact b/bin/gstack-redact index ccb6e48c5..f4ee0583f 100755 --- a/bin/gstack-redact +++ b/bin/gstack-redact @@ -160,13 +160,27 @@ function readLines(path: string | undefined): string[] | undefined { function buildOpts(): ScanOptions { const vis = (arg("--repo-visibility") as RepoVisibility) || "unknown"; - const maxBytes = arg("--max-bytes"); + const maxBytesRaw = arg("--max-bytes"); + let maxBytes: number | undefined; + if (maxBytesRaw !== undefined) { + // Reject a malformed cap loudly. Silently passing NaN/0/negative would + // weaken the engine's fail-closed oversize guard (exit 1 = usage error, + // distinct from the 0/2/3 finding-tier codes). + const parsed = Number(maxBytesRaw); + if (!Number.isInteger(parsed) || parsed <= 0) { + process.stderr.write( + `gstack-redact: --max-bytes must be a positive integer, got: ${maxBytesRaw}\n`, + ); + process.exit(1); + } + maxBytes = parsed; + } return { repoVisibility: ["public", "private", "unknown"].includes(vis) ? vis : "unknown", allowlist: readLines(arg("--allowlist")), selfEmail: arg("--self-email"), repoPublicEmails: readLines(arg("--repo-public-emails")), - ...(maxBytes ? { maxBytes: parseInt(maxBytes, 10) } : {}), + ...(maxBytes !== undefined ? { maxBytes } : {}), }; } diff --git a/lib/redact-engine.ts b/lib/redact-engine.ts index 88149f5d9..c4c19882c 100644 --- a/lib/redact-engine.ts +++ b/lib/redact-engine.ts @@ -253,7 +253,15 @@ function emailAllowed(email: string, opts: ScanOptions): boolean { export function scan(input: string, opts: ScanOptions = {}): ScanResult { const repoVisibility: RepoVisibility = opts.repoVisibility ?? "unknown"; - const maxBytes = opts.maxBytes ?? DEFAULT_MAX_BYTES; + // `??` only catches null/undefined, so a NaN (e.g. from a malformed + // `--max-bytes` flag) or a non-positive value would slip through and make + // `byteLen > maxBytes` always false — silently turning this fail-CLOSED guard + // into fail-OPEN. Treat any invalid cap as "use the known-good default". + const requestedMax = opts.maxBytes; + const maxBytes = + typeof requestedMax === "number" && Number.isFinite(requestedMax) && requestedMax > 0 + ? requestedMax + : DEFAULT_MAX_BYTES; // Fail CLOSED on oversize input. Check byte length BEFORE heavy work. const byteLen = Buffer.byteLength(input, "utf8"); diff --git a/test/gstack-redact-cli.test.ts b/test/gstack-redact-cli.test.ts index 4808ba53b..d37b06051 100644 --- a/test/gstack-redact-cli.test.ts +++ b/test/gstack-redact-cli.test.ts @@ -94,4 +94,15 @@ describe("gstack-redact oversize fails closed", () => { expect(code).toBe(3); expect(stdout).toContain("too large"); }); + + // Regression: a malformed --max-bytes must error loudly (usage exit 1), not + // silently pass NaN into the engine where it would disable the fail-closed + // guard. Exit 1 is distinct from the 0/2/3 finding-tier codes. + for (const bad of ["notanumber", "-5", "0", "10.5"]) { + test(`malformed --max-bytes (${bad}) exits 1 with a clear error`, () => { + const { code, stderr } = run(["--max-bytes", bad], "a".repeat(500)); + expect(code).toBe(1); + expect(stderr).toContain("--max-bytes must be a positive integer"); + }); + } }); diff --git a/test/redact-engine.test.ts b/test/redact-engine.test.ts index 52c119a19..64f3b5a39 100644 --- a/test/redact-engine.test.ts +++ b/test/redact-engine.test.ts @@ -239,6 +239,20 @@ describe("oversize fails CLOSED", () => { expect(r.findings[0].id).toBe("engine.input_too_large"); expect(exitCodeFor(r)).toBe(3); }); + + // Regression: an invalid cap (NaN/0/negative) must NOT disable the guard. + // `?? DEFAULT` did not catch NaN, so `byteLen > NaN` was always false and the + // fail-CLOSED guard silently failed OPEN. Invalid caps fall back to the + // 1 MiB default, so a >1 MiB input still blocks. + for (const bad of [NaN, 0, -1]) { + test(`invalid maxBytes (${bad}) falls back to the default cap and still blocks >1 MiB`, () => { + const big = "a".repeat(1024 * 1024 + 1); + const r = scan(big, { maxBytes: bad }); + expect(r.oversize).toBe(true); + expect(r.findings[0].id).toBe("engine.input_too_large"); + expect(exitCodeFor(r)).toBe(3); + }); + } }); describe("validators", () => {