From 8c8e3b9e52d18ab0c2699fec3ce43efe0ce40a4f Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Thu, 11 Jun 2026 23:31:26 -0700 Subject: [PATCH] fix: pre-landing review fixes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review army findings (1 critical, auto-fixed with regression tests): - CRITICAL (security specialist, verified live): redactFindingSpans spliced only the regex capture span, and pem.private_key / gcp.service_account capture just the BEGIN-header — the key body survived "redaction" and shipped via telemetry. Marker-only patterns now drop the whole payload (null, fail closed). Overlapping spans (Bearer+JWT on the same bytes) are coalesced before splicing so stale offsets can't leave partial secret bytes behind. - gitStrict: drop the dead `|| r.status === null` disjunct (null !== 0 already covers it); add the signal-kill/null-status regression test the docstring promised. - security-dashboard human mode flags stale snapshots ("figures may be out of date") instead of presenting frozen counts as current. - community-dashboard marker check uses jq when available — the grep-only variant misclassified whitespaced/reserialized bodies as legacy. - telemetry fail-closed test now shadows bun with a failing stub (deterministic on any host layout); stale "five status cases" describe title renamed. Co-Authored-By: Claude Fable 5 --- bin/gstack-community-dashboard | 9 ++++- bin/gstack-redact-prepush | 4 ++- bin/gstack-security-dashboard | 4 +++ lib/redact-engine.ts | 46 +++++++++++++++++++----- test/gbrain-local-status.test.ts | 2 +- test/redact-engine.test.ts | 30 ++++++++++++++++ test/redact-prepush-hook.test.ts | 25 +++++++++++++ test/security-dashboard-fallback.test.ts | 16 +++++++++ test/telemetry.test.ts | 36 +++++++++++++++---- 9 files changed, 155 insertions(+), 17 deletions(-) diff --git a/bin/gstack-community-dashboard b/bin/gstack-community-dashboard index 10a1ffa1f..51ddad579 100755 --- a/bin/gstack-community-dashboard +++ b/bin/gstack-community-dashboard @@ -57,7 +57,14 @@ WEEKLY="$(echo "$DATA" | grep -o '"weekly_active":[0-9]*' | grep -o '[0-9]*' || CHANGE="$(echo "$DATA" | grep -o '"change_pct":[0-9-]*' | grep -o '[0-9-]*' || echo "0")" echo "Weekly active installs: ${WEEKLY}" -if ! printf '%s' "$DATA" | grep -q '"status":"ok"'; then +# Marker check: jq when available (whitespace/reserialization-proof); grep +# fallback only matches the compact form the edge function emits today. +if command -v jq >/dev/null 2>&1; then + _MARKER="$(printf '%s' "$DATA" | jq -r '.status // empty' 2>/dev/null)" +else + _MARKER="$(printf '%s' "$DATA" | grep -q '"status":"ok"' && echo ok || true)" +fi +if [ "$_MARKER" != "ok" ]; then echo " (unverified — legacy backend response; deploy the latest community-pulse for verified figures)" fi if [ "$CHANGE" -gt 0 ] 2>/dev/null; then diff --git a/bin/gstack-redact-prepush b/bin/gstack-redact-prepush index 57e04ce9e..61e242bdf 100755 --- a/bin/gstack-redact-prepush +++ b/bin/gstack-redact-prepush @@ -54,7 +54,9 @@ function git(args: string[]): string { */ function gitStrict(args: string[]): string { const r = spawnSync("git", args, { encoding: "utf8", maxBuffer: 64 * 1024 * 1024 }); - if (r.status !== 0 || r.status === null) { + // status !== 0 covers BOTH a non-zero exit AND null (process killed by a + // signal or maxBuffer overflow — null !== 0 is true). + if (r.status !== 0) { throw new Error( `git ${args[0]} failed (status=${r.status ?? "killed/overflow"}): ${(r.stderr ?? "").slice(0, 300)}`, ); diff --git a/bin/gstack-security-dashboard b/bin/gstack-security-dashboard index 01c803a36..17bb9199e 100755 --- a/bin/gstack-security-dashboard +++ b/bin/gstack-security-dashboard @@ -110,6 +110,10 @@ TOTAL="$(echo "$DATA" | jq -r '.security.attacks_last_7_days // 0' 2>/dev/null | echo "Attacks detected last 7 days: ${TOTAL}" if [ "$STATE" = "legacy" ]; then echo " (unverified — legacy backend response; deploy the latest community-pulse for verified figures)" +elif [ "$(echo "$DATA" | jq -r '.stale // false' 2>/dev/null)" = "true" ]; then + # The backend serves its last good snapshot when recompute fails — figures + # are real but frozen. Don't present them as current. + echo " (stale snapshot — backend recompute failing; figures may be out of date)" elif [ "$TOTAL" = "0" ]; then echo " (No attack attempts reported by the community yet. Good news.)" fi diff --git a/lib/redact-engine.ts b/lib/redact-engine.ts index 42b650f1a..c337e660e 100644 --- a/lib/redact-engine.ts +++ b/lib/redact-engine.ts @@ -427,24 +427,54 @@ export function applyRedactions( return { body, diff: diffLines.reverse().join("\n"), skipped }; } +/** + * Patterns whose regex captures only a MARKER, not the secret payload itself + * (the PEM header line; the GCP JSON key prefix). Span replacement on these + * would redact the header and forward the key body — so redactFindingSpans + * drops the whole payload instead. + */ +const MARKER_ONLY_PATTERN_IDS = new Set(["pem.private_key", "gcp.service_account"]); + /** * Replace EVERY finding's span with ``, regardless of tier or * autoRedactable. For machine egress surfaces (telemetry error_message, * #1947) where structure preservation doesn't matter and fail-closed beats - * fidelity. Returns null when any finding's span cannot be located — the - * caller must then drop the whole payload rather than risk leaking an - * unlocated secret. (Contrast applyRedactions, which is the interactive, - * autoRedactable-only, structure-preserving path.) + * fidelity. Returns null — caller must drop the whole payload — when: + * - any finding's span cannot be located, or + * - any finding matched a marker-only pattern (PEM / GCP service-account + * JSON): their regexes capture the header, not the key material, so a + * span splice would leak the body that follows the marker. + * Overlapping spans (e.g. a Bearer token that is also a JWT) are coalesced + * before splicing so stale offsets never leave partial secret bytes behind. + * (Contrast applyRedactions, which is the interactive, autoRedactable-only, + * structure-preserving path.) */ export function redactFindingSpans(input: string, opts: ScanOptions = {}): string | null { const { findings } = scan(input, opts); + if (findings.some((f) => MARKER_ONLY_PATTERN_IDS.has(f.id))) return null; const targets = findings.map((f) => ({ f, ...locateSpan(input, f) })); if (targets.some((t) => t.start < 0)) return null; - // Right-to-left so earlier offsets remain valid after splicing. - targets.sort((a, b) => b.start - a.start); - let body = input; + + // Coalesce overlapping/touching ranges — splicing two intersecting spans + // independently applies a stale end offset to already-modified text and + // can leave trailing secret bytes in place. + targets.sort((a, b) => a.start - b.start); + const merged: Array<{ start: number; end: number; ids: string[] }> = []; for (const t of targets) { - body = body.slice(0, t.start) + `` + body.slice(t.end); + const last = merged[merged.length - 1]; + if (last && t.start <= last.end) { + last.end = Math.max(last.end, t.end); + if (!last.ids.includes(t.f.id)) last.ids.push(t.f.id); + } else { + merged.push({ start: t.start, end: t.end, ids: [t.f.id] }); + } + } + + // Right-to-left so earlier offsets remain valid after splicing. + let body = input; + for (let i = merged.length - 1; i >= 0; i--) { + const m = merged[i]; + body = body.slice(0, m.start) + `` + body.slice(m.end); } return body; } diff --git a/test/gbrain-local-status.test.ts b/test/gbrain-local-status.test.ts index b3d3e398e..46ab9ad88 100644 --- a/test/gbrain-local-status.test.ts +++ b/test/gbrain-local-status.test.ts @@ -174,7 +174,7 @@ function applyEnv(env: FakeEnv): () => void { }; } -describe("lib/gbrain-local-status — five status cases", () => { +describe("lib/gbrain-local-status — status classification", () => { let env: FakeEnv | null = null; let restoreEnv: (() => void) | null = null; diff --git a/test/redact-engine.test.ts b/test/redact-engine.test.ts index 88b302db6..b52c630d5 100644 --- a/test/redact-engine.test.ts +++ b/test/redact-engine.test.ts @@ -409,6 +409,36 @@ describe("redactFindingSpans — machine-egress masking (#1947)", () => { }); expect(out).toBe("line one\nline two has \nline three"); }); + + // Pre-landing review CRITICAL: pem.private_key and gcp.service_account + // capture only the HEADER, not the key material — a span splice would + // redact the marker and forward the key body. Marker-only patterns must + // drop the whole payload. + test("PEM private key → null (header-only span must not forward the key body)", () => { + const msg = + "deploy failed: -----BEGIN PRIVATE KEY-----\nMIIEvQIBADANBgkqhkiG9w0BAQEFAASC\n-----END PRIVATE KEY-----"; + expect(redactFindingSpans(msg, { repoVisibility: "private" })).toBeNull(); + }); + + test("GCP service-account JSON → null (key body follows the captured marker)", () => { + const msg = + 'config dump: {"private_key_id": "abc123", "private_key": "-----BEGIN PRIVATE KEY-----\\nMIIEvQIBADANBg..."}'; + expect(redactFindingSpans(msg, { repoVisibility: "private" })).toBeNull(); + }); + + // Pre-landing review: overlapping spans (a Bearer token that is also a + // JWT) must coalesce — independent splices apply stale offsets and can + // leave trailing secret bytes or mangled markers. + test("overlapping spans (Bearer JWT fires auth.bearer + jwt) never leak and produce clean markers", () => { + const jwt = "eyJ" + "a".repeat(20) + ".eyJ" + "b".repeat(20) + "." + "c".repeat(20); + const out = redactFindingSpans(`Authorization: Bearer ${jwt}`, { repoVisibility: "private" }); + expect(out).not.toBeNull(); + expect(out!).not.toContain("eyJ"); + expect(out!).not.toContain("aaaa"); + expect(out!).not.toContain("cccc"); + // One coalesced, well-formed marker — no truncated fragments. + expect(out!).toMatch(/^Authorization: Bearer $/); + }); }); describe("taxonomy integrity", () => { diff --git a/test/redact-prepush-hook.test.ts b/test/redact-prepush-hook.test.ts index 589f679c6..fa38ff0db 100644 --- a/test/redact-prepush-hook.test.ts +++ b/test/redact-prepush-hook.test.ts @@ -127,6 +127,31 @@ describe("fail closed on unscannable diffs (#1946)", () => { const { code } = runHook(`refs/heads/main ${head} refs/heads/main ${head}\n`); expect(code).toBe(0); }); + + test("a diff killed by a signal (null status — the maxBuffer/kill class) BLOCKS", () => { + // Stub git: probes delegate to the real git; the diff invocation kills + // itself, producing spawnSync status === null. This is the exact branch + // gitStrict's docstring names (oversized-diff overflow is delivered the + // same way) — pre-landing review flagged it as untested. + const realGit = Bun.which("git") || "/usr/bin/git"; + const stubDir = fs.mkdtempSync(path.join(os.tmpdir(), "prepush-stubgit-")); + try { + const stub = `#!/bin/sh\nif [ "$1" = "diff" ]; then kill -KILL $$; fi\nexec "${realGit}" "$@"\n`; + fs.writeFileSync(path.join(stubDir, "git"), stub); + fs.chmodSync(path.join(stubDir, "git"), 0o755); + + const base = git(["rev-parse", "HEAD"]); + const head = commit("clean.txt", "clean content\n", "clean commit"); + const { code, stderr } = runHook(`refs/heads/main ${head} refs/heads/main ${base}\n`, { + PATH: `${stubDir}:${process.env.PATH}`, + }); + expect(code).toBe(1); + expect(stderr).toContain("could not compute the pushed diff"); + expect(stderr).toContain("GSTACK_REDACT_PREPUSH=skip"); + } finally { + fs.rmSync(stubDir, { recursive: true, force: true }); + } + }); }); describe("install UX surfaces (#1946 / eng review D3+D10)", () => { diff --git a/test/security-dashboard-fallback.test.ts b/test/security-dashboard-fallback.test.ts index f23a6800e..d9612725b 100644 --- a/test/security-dashboard-fallback.test.ts +++ b/test/security-dashboard-fallback.test.ts @@ -182,6 +182,13 @@ describe("gstack-security-dashboard — never reports fake zeros (#1947)", () => expect(parsed.stale).toBe(true); }); + it("stale snapshot is flagged in human mode too — frozen figures never read as current", () => { + const staleBody = JSON.stringify({ ...JSON.parse(GOOD_BODY_MARKER), stale: true }); + const r = run(SEC_BIN, { mode: "ok", body: staleBody }); + expect(r.stdout).toContain("Attacks detected last 7 days: 3"); + expect(r.stdout).toContain("stale snapshot"); + }); + it("200 without marker (legacy backend) → figures shown with unverified note", () => { const r = run(SEC_BIN, { mode: "ok", body: GOOD_BODY_LEGACY }); expect(r.stdout).toContain("Attacks detected last 7 days: 3"); @@ -238,4 +245,13 @@ describe("gstack-community-dashboard — never reports fake zeros (#1947)", () = expect(r.stdout).toContain("unknown — backend error (HTTP 200)"); expect(r.stdout).not.toContain("Weekly active installs:"); }); + + it("whitespaced marker ('\"status\": \"ok\"') still classified as verified when jq is present", () => { + // Pre-landing review: the grep-only marker check was whitespace-sensitive; + // a proxy-reserialized body must not be misclassified as legacy. + const spaced = GOOD_BODY_MARKER.replace('"status":"ok"', '"status": "ok"'); + const r = run(COMM_BIN, { mode: "ok", body: spaced }); + expect(r.stdout).toContain("Weekly active installs: 42"); + expect(r.stdout).not.toContain("unverified"); + }); }); diff --git a/test/telemetry.test.ts b/test/telemetry.test.ts index 33e1db241..668349481 100644 --- a/test/telemetry.test.ts +++ b/test/telemetry.test.ts @@ -222,12 +222,21 @@ describe('gstack-telemetry-log', () => { test('fails closed: error_message becomes null when the redactor is unavailable (#1947)', () => { setConfig('telemetry', 'anonymous'); const token = 'ghp_' + 'A1b2C3d4E5f6G7h8I9j0K1l2M3n4O5p6Q7r8'; - // PATH without bun: the redaction snippet cannot run, so the whole - // message must drop — never raw passthrough. - run( - `${BIN}/gstack-telemetry-log --skill qa --duration 10 --outcome error --error-message 'auth ${token} rejected' --session-id red-2`, - { PATH: '/usr/bin:/bin' }, - ); + // Shadow bun with a failing stub on a prepended PATH (deterministic on + // any host layout — pre-landing review flagged the bare '/usr/bin:/bin' + // variant as environment-dependent): the redaction snippet cannot run, + // so the whole message must drop — never raw passthrough. + const stubBin = fs.mkdtempSync(path.join(os.tmpdir(), 'gstack-tel-nobun-')); + try { + fs.writeFileSync(path.join(stubBin, 'bun'), '#!/bin/sh\nexit 127\n'); + fs.chmodSync(path.join(stubBin, 'bun'), 0o755); + run( + `${BIN}/gstack-telemetry-log --skill qa --duration 10 --outcome error --error-message 'auth ${token} rejected' --session-id red-2`, + { PATH: `${stubBin}:${process.env.PATH}` }, + ); + } finally { + fs.rmSync(stubBin, { recursive: true, force: true }); + } const lines = readJsonl(); expect(lines).toHaveLength(1); @@ -236,6 +245,21 @@ describe('gstack-telemetry-log', () => { expect(lines[0]).not.toContain(token); }); + test('fails closed: PEM key in error_message drops the whole message (#1947 review fix)', () => { + setConfig('telemetry', 'anonymous'); + // Header-only pattern: span replacement would forward the key body, so + // the engine returns null and the bin must store null. + run( + `${BIN}/gstack-telemetry-log --skill qa --duration 10 --outcome error --error-message 'deploy failed: -----BEGIN PRIVATE KEY----- MIIEvQIBADANBgkqhkiG9w0BAQEFAASC' --session-id red-5`, + ); + + const lines = readJsonl(); + expect(lines).toHaveLength(1); + const event = JSON.parse(lines[0]); + expect(event.error_message).toBeNull(); + expect(lines[0]).not.toContain('MIIEvQIBADAN'); + }); + test('truncates error_message to 200 chars after redaction (#1947)', () => { setConfig('telemetry', 'anonymous'); const long = 'x'.repeat(300);