From be3d4c71712e6fb1c652f605927dc775214794c4 Mon Sep 17 00:00:00 2001
From: Garry Tan <garrytan@gmail.com>
Date: Sun, 7 Jun 2026 22:48:55 -0700
Subject: [PATCH] fix(redact): reject malformed --max-bytes instead of silently
 disabling the size guard (#1824)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The oversize check is designed to fail CLOSED, but a malformed --max-bytes turned
it fail-OPEN. bin/gstack-redact did parseInt(maxBytes,10) and passed it straight
through; parseInt("foo") is NaN. The engine guarded with `opts.maxBytes ?? DEFAULT`,
and ?? does not catch NaN, so `byteLen > NaN` was always false and the fail-closed
block never fired. A negative value made `byteLen > -5` always true, blocking
everything.

Two layers:
- bin/gstack-redact validates the RAW string (parseInt accepts "123abc"->123,
  "1.5"->1): require /^\d+$/ and > 0, else exit 1 with a clear message.
- lib/redact-engine.ts hardens the fallback to Number.isFinite && > 0 else the
  default cap — a guardrail so the engine never silently runs uncapped even if a
  bad value reaches it directly.

Tests: NaN and negative both fall back to the default cap (oversize still blocks);
CLI rejects garbage/negative with exit 1.

Reported by @jbetala7.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
---
 bin/gstack-redact          | 15 ++++++++++++++-
 lib/redact-engine.ts       | 11 ++++++++++-
 test/redact-engine.test.ts | 21 +++++++++++++++++++++
 3 files changed, 45 insertions(+), 2 deletions(-)

diff --git a/bin/gstack-redact b/bin/gstack-redact
index ccb6e48c5..41bd54c65 100755
--- a/bin/gstack-redact
+++ b/bin/gstack-redact
@@ -161,12 +161,25 @@ function readLines(path: string | undefined): string[] | undefined {
 function buildOpts(): ScanOptions {
   const vis = (arg("--repo-visibility") as RepoVisibility) || "unknown";
   const maxBytes = arg("--max-bytes");
+  // #1824: validate the RAW string, not the parse result. parseInt("123abc")
+  // is 123 and parseInt("foo") is NaN — both silently corrupt the fail-closed
+  // oversize guard. Require a clean positive integer or reject before scanning.
+  let maxBytesOpt: number | undefined;
+  if (maxBytes !== undefined) {
+    if (!/^\d+$/.test(maxBytes) || Number(maxBytes) <= 0) {
+      process.stderr.write(
+        `gstack-redact: --max-bytes must be a positive integer (got "${maxBytes}")\n`,
+      );
+      process.exit(1);
+    }
+    maxBytesOpt = Number(maxBytes);
+  }
   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) } : {}),
+    ...(maxBytesOpt !== undefined ? { maxBytes: maxBytesOpt } : {}),
   };
 }
 
diff --git a/lib/redact-engine.ts b/lib/redact-engine.ts
index 88149f5d9..02cf66829 100644
--- a/lib/redact-engine.ts
+++ b/lib/redact-engine.ts
@@ -253,7 +253,16 @@ 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;
+  // #1824: ?? only catches null/undefined, not NaN or <= 0. A bad value
+  // (NaN from a malformed --max-bytes, or a negative) would make `byteLen >
+  // maxBytes` always false and silently disable the fail-closed oversize guard.
+  // Guardrail: any non-finite or non-positive value falls back to the default
+  // cap. The CLI is the layer that rejects bad args; this is belt-and-suspenders
+  // so the engine never silently runs uncapped.
+  const maxBytes =
+    Number.isFinite(opts.maxBytes) && (opts.maxBytes as number) > 0
+      ? (opts.maxBytes as number)
+      : DEFAULT_MAX_BYTES;
 
   // Fail CLOSED on oversize input. Check byte length BEFORE heavy work.
   const byteLen = Buffer.byteLength(input, "utf8");
diff --git a/test/redact-engine.test.ts b/test/redact-engine.test.ts
index dbbfd8a3a..1300e94cb 100644
--- a/test/redact-engine.test.ts
+++ b/test/redact-engine.test.ts
@@ -269,6 +269,27 @@ describe("oversize fails CLOSED", () => {
     expect(r.findings[0].id).toBe("engine.input_too_large");
     expect(exitCodeFor(r)).toBe(3);
   });
+
+  // #1824: a malformed --max-bytes used to reach the engine as NaN. `byteLen >
+  // NaN` is always false, silently disabling the fail-closed guard. The engine
+  // guardrail must fall back to the default cap for any non-finite / <= 0 value.
+  test("NaN maxBytes falls back to the default cap (does NOT disable the guard)", () => {
+    const big = "a".repeat(2 * 1024 * 1024); // > 1 MiB default cap
+    const r = scan(big, { maxBytes: NaN });
+    expect(r.oversize).toBe(true);
+    expect(r.findings[0].id).toBe("engine.input_too_large");
+    expect(exitCodeFor(r)).toBe(3);
+  });
+
+  test("negative / zero maxBytes falls back to the default cap", () => {
+    // negative would make `byteLen > -5` always true (block everything);
+    // the guardrail normalizes it to the default instead.
+    const small = "ok";
+    expect(scan(small, { maxBytes: -5 }).oversize).toBeFalsy();
+    expect(scan(small, { maxBytes: 0 }).oversize).toBeFalsy();
+    const big = "a".repeat(2 * 1024 * 1024);
+    expect(scan(big, { maxBytes: -5 }).oversize).toBe(true);
+  });
 });
 
 describe("validators", () => {