From 8abf726f043994c23b78d81c5733ebd459fd90cc Mon Sep 17 00:00:00 2001 From: RagavRida Date: Fri, 24 Apr 2026 00:04:30 +0530 Subject: [PATCH 01/13] fix(build-app): escape sed replacement metachars in Chromium rebrand MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit build-app.sh injects \$APP_NAME directly into the replacement half of sed's s/// when patching Chromium's localized InfoPlist.strings. If \$APP_NAME ever carries '/', '&', or '\\' — the command either breaks or starts interpreting input as sed syntax. The trailing '|| true' would then silently hide the failure and ship a DMG that still says 'Google Chrome for Testing' in the menu bar. Escape replacement metachars before substitution. No change for the default name 'GStack Browser'. --- scripts/build-app.sh | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/scripts/build-app.sh b/scripts/build-app.sh index 1c7b0c3039..90ba8a7484 100755 --- a/scripts/build-app.sh +++ b/scripts/build-app.sh @@ -94,7 +94,9 @@ if [ -f "$CHROMIUM_PLIST" ]; then if [ -f "$CHROMIUM_STRINGS" ]; then # InfoPlist.strings may be binary plist, convert to xml first plutil -convert xml1 "$CHROMIUM_STRINGS" 2>/dev/null || true - sed -i '' "s/Google Chrome for Testing/$APP_NAME/g" "$CHROMIUM_STRINGS" 2>/dev/null || true + # Escape sed replacement metachars (& / \) in $APP_NAME so unusual names can't break or inject into the s/// command. + APP_NAME_SED_ESCAPED=$(printf '%s' "$APP_NAME" | sed 's/[&/\]/\\&/g') + sed -i '' "s/Google Chrome for Testing/${APP_NAME_SED_ESCAPED}/g" "$CHROMIUM_STRINGS" 2>/dev/null || true fi # Replace Chromium's icon with ours so the Dock shows the GStack icon # (Chromium's process owns the Dock icon, not our launcher) From b7d690bb34320f29a21707473b886a9a8f566ca1 Mon Sep 17 00:00:00 2001 From: RagavRida Date: Fri, 24 Apr 2026 00:05:30 +0530 Subject: [PATCH 02/13] fix(build-app): bail out if 'mktemp -d' fails instead of cp-ing into '/' MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The DMG creation step sets DMG_TMP from 'mktemp -d' with no error check. If mktemp fails (tmpfs full, permissions, TMPDIR misconfigured), DMG_TMP is empty and the very next line — 'cp -a "\$APP_DIR" "\$DMG_TMP/"' — expands to 'cp -a "" "/"', which copies the bundle into the root of the filesystem. Refuse to continue unless mktemp produced a real directory. Defensive second check catches the (rare) case where mktemp succeeds but returns something that isn't a directory we can cp into. --- scripts/build-app.sh | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/scripts/build-app.sh b/scripts/build-app.sh index 90ba8a7484..8869212ab9 100755 --- a/scripts/build-app.sh +++ b/scripts/build-app.sh @@ -179,7 +179,11 @@ echo " Creating DMG..." rm -f "$DMG_PATH" # Create a temporary directory for DMG contents -DMG_TMP=$(mktemp -d) +DMG_TMP=$(mktemp -d) || { echo "ERROR: mktemp -d failed — refusing to continue so we don't cp into the filesystem root." >&2; exit 1; } +if [ -z "$DMG_TMP" ] || [ ! -d "$DMG_TMP" ]; then + echo "ERROR: mktemp -d returned an invalid path ('$DMG_TMP')." >&2 + exit 1 +fi cp -a "$APP_DIR" "$DMG_TMP/" ln -s /Applications "$DMG_TMP/Applications" From 46821fe6d86a1f1a338a9fa4da87ab698cb4fb1b Mon Sep 17 00:00:00 2001 From: RagavRida Date: Fri, 24 Apr 2026 00:06:39 +0530 Subject: [PATCH 03/13] fix(telemetry-sync): drop predictable $$ tmp-file fallback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit gstack-telemetry-sync tried 'mktemp /tmp/gstack-sync-XXXXXX' and on failure fell back to '/tmp/gstack-sync-$$'. $$ is the PID — predictable and reusable, so on shared hosts another user can pre-create or symlink the path and either steal the response body or clobber an unrelated file when curl writes through it. Drop the fallback. If mktemp cannot produce a unique file we just skip this sync cycle — the events stay on disk and the next run picks them up. Also install an EXIT trap so the response file is cleaned up on unexpected exit, not just on the happy path. --- bin/gstack-telemetry-sync | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/bin/gstack-telemetry-sync b/bin/gstack-telemetry-sync index 93cf2707af..20f322043e 100755 --- a/bin/gstack-telemetry-sync +++ b/bin/gstack-telemetry-sync @@ -107,7 +107,13 @@ BATCH="$BATCH]" [ "$COUNT" -eq 0 ] && exit 0 # ─── POST to edge function ─────────────────────────────────── -RESP_FILE="$(mktemp /tmp/gstack-sync-XXXXXX 2>/dev/null || echo "/tmp/gstack-sync-$$")" +# Create response file atomically. If mktemp fails, refuse to continue rather +# than fall back to a predictable $$-based path (race + overwrite footgun). +RESP_FILE="$(mktemp "${TMPDIR:-/tmp}/gstack-sync-XXXXXX")" || { + echo "gstack-telemetry-sync: mktemp failed — skipping this run" >&2 + exit 0 +} +trap 'rm -f "$RESP_FILE"' EXIT HTTP_CODE="$(curl -s -w '%{http_code}' --max-time 10 \ -X POST "${SUPABASE_URL}/functions/v1/telemetry-ingest" \ -H "Content-Type: application/json" \ From 3bba467289aacd16d4376faeff253404b80255b0 Mon Sep 17 00:00:00 2001 From: RagavRida Date: Fri, 24 Apr 2026 00:06:58 +0530 Subject: [PATCH 04/13] fix(verify-rls): drop predictable $$-based tmp file fallback Same shape as gstack-telemetry-sync: on mktemp failure the script fell back to '/tmp/verify-rls-$$-$TOTAL', which is fully predictable from the PID and a per-check counter. On a shared box another user can pre-create or symlink the path and either capture the HTTP response body (which may leak what the RLS tests revealed) or corrupt an unrelated file that curl writes through. Make mktemp strict. On failure return from the check function; the caller tallies a FAIL and the run moves on. --- supabase/verify-rls.sh | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/supabase/verify-rls.sh b/supabase/verify-rls.sh index 4ed92bc671..3657776a1b 100755 --- a/supabase/verify-rls.sh +++ b/supabase/verify-rls.sh @@ -30,7 +30,12 @@ check() { TOTAL=$(( TOTAL + 1 )) local resp_file - resp_file="$(mktemp 2>/dev/null || echo "/tmp/verify-rls-$$-$TOTAL")" + # Use mktemp strictly. Don't fall back to a predictable $$-based path — + # that's a race/overwrite footgun on shared machines. + resp_file="$(mktemp "${TMPDIR:-/tmp}/verify-rls-XXXXXX")" || { + echo "verify-rls: mktemp failed, aborting" >&2 + return 1 + } local http_code if [ "$method" = "GET" ]; then From 321a4e75b5b7267f91507b1c06eedfe402b6462e Mon Sep 17 00:00:00 2001 From: RagavRida Date: Fri, 24 Apr 2026 00:07:24 +0530 Subject: [PATCH 05/13] fix(security-classifier): close writer + delete tmp on download error MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit downloadFile() opens an fs.WriteStream to '.tmp.' and drives it from a fetch body reader, but if reader.read() or writer.write() throws mid-download the writer is never closed. That leaks an FD per failed attempt and leaves the half-written tmp on disk. A later retry can land in renameSync(tmp, dest) with a truncated TestSavantAI / DeBERTa ONNX file — which then loads but produces garbage classifier verdicts until the user manually nukes the models cache. Wrap the download loop in try/catch. On failure, destroy() the writer and unlink the tmp before rethrowing, so the next attempt starts from a clean slate. --- browse/src/security-classifier.ts | 26 +++++++++++++++++--------- 1 file changed, 17 insertions(+), 9 deletions(-) diff --git a/browse/src/security-classifier.ts b/browse/src/security-classifier.ts index 68a41ba265..7d8029b3a3 100644 --- a/browse/src/security-classifier.ts +++ b/browse/src/security-classifier.ts @@ -144,16 +144,24 @@ async function downloadFile(url: string, dest: string): Promise { const writer = fs.createWriteStream(tmp); // @ts-ignore — Node stream compat const reader = res.body.getReader(); - let done = false; - while (!done) { - const chunk = await reader.read(); - if (chunk.done) { done = true; break; } - writer.write(chunk.value); + try { + let done = false; + while (!done) { + const chunk = await reader.read(); + if (chunk.done) { done = true; break; } + writer.write(chunk.value); + } + await new Promise((resolve, reject) => { + writer.end((err?: Error | null) => (err ? reject(err) : resolve())); + }); + fs.renameSync(tmp, dest); + } catch (err) { + // Close the writer and drop the half-written tmp so we don't leak an FD + // or ship a truncated model file to the renameSync path on retry. + try { writer.destroy(); } catch { /* already destroyed */ } + try { fs.unlinkSync(tmp); } catch { /* nothing to clean */ } + throw err; } - await new Promise((resolve, reject) => { - writer.end((err?: Error | null) => (err ? reject(err) : resolve())); - }); - fs.renameSync(tmp, dest); } async function ensureTestsavantStaged(onProgress?: (msg: string) => void): Promise { From ac89d8847e3ddeaa20c3399bf594aa2ab2be30e2 Mon Sep 17 00:00:00 2001 From: RagavRida Date: Fri, 24 Apr 2026 00:07:52 +0530 Subject: [PATCH 06/13] fix(meta-commands): guard JSON.parse in pdf --from-file parser MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit parsePdfFromFile() runs JSON.parse on user-supplied file contents with no try/catch. A malformed payload surfaces as an uncaught SyntaxError from the 'pdf' command handler and the user sees an opaque stack trace instead of "this file isn't valid JSON". Worse, the same call path is used by make-pdf when header/footer HTML would overflow Windows' CreateProcess argv cap, so a corrupt payload file there can take down the make-pdf run. Wrap JSON.parse. Re-throw with a message that names the offending file and echoes the parser's own explanation. Also reject top-level non- objects (null, array, primitive) since the rest of the function treats json as an object — catching that here produces a clear error instead of a TypeError further down. --- browse/src/meta-commands.ts | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/browse/src/meta-commands.ts b/browse/src/meta-commands.ts index c505d4cf41..bbd90f62d0 100644 --- a/browse/src/meta-commands.ts +++ b/browse/src/meta-commands.ts @@ -149,7 +149,16 @@ function parsePdfFromFile(payloadPath: string): ParsedPdfArgs { ); } const raw = fs.readFileSync(payloadPath, 'utf8'); - const json = JSON.parse(raw); + let json: any; + try { + json = JSON.parse(raw); + } catch (err) { + const msg = err instanceof Error ? err.message : String(err); + throw new Error(`pdf: --from-file ${payloadPath} is not valid JSON (${msg}).`); + } + if (json === null || typeof json !== 'object' || Array.isArray(json)) { + throw new Error(`pdf: --from-file ${payloadPath} must be a JSON object, got ${Array.isArray(json) ? 'array' : typeof json}.`); + } const out: ParsedPdfArgs = { output: json.output || `${TEMP_DIR}/browse-page.pdf`, format: json.format, From 7dc60076ec9052b598752f621736fb5734354175 Mon Sep 17 00:00:00 2001 From: RagavRida Date: Fri, 24 Apr 2026 00:08:28 +0530 Subject: [PATCH 07/13] fix(global-discover): stop dropping sessions when header >8KB extractCwdFromJsonl() reads the first 8KB of each JSONL session file and runs JSON.parse on every newline-split line. When a session record happens to straddle the 8KB cap, the last line ends in a truncated JSON fragment, JSON.parse throws, the catch block 'continue's silently, and if that was the only line carrying 'cwd' the whole project gets dropped from the discovery output without a warning. Two independent hardening steps: 1. Raise the read cap to 64KB. Session headers observed in Claude Code / Codex / Gemini transcripts fit comfortably; this just moves the cliff out of the normal range. 2. Drop the final segment after splitting on '\\n'. If the read hit the cap mid-line, that segment is guaranteed incomplete; if the file ended inside the buffer, the split produces an empty final segment and dropping it is a no-op. Together these make the parser robust regardless of how verbose the leading records are. --- bin/gstack-global-discover.ts | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/bin/gstack-global-discover.ts b/bin/gstack-global-discover.ts index 4e1445b37a..7f07141008 100644 --- a/bin/gstack-global-discover.ts +++ b/bin/gstack-global-discover.ts @@ -274,15 +274,22 @@ function resolveClaudeCodeCwd( } function extractCwdFromJsonl(filePath: string): string | null { + // Read a capped prefix so huge JSONL files don't blow up memory. 64KB + // comfortably fits the largest observed session headers; the old 8KB cap + // would sometimes fall inside a single long line and silently drop the + // project (JSON.parse failure on the truncated tail). + const MAX_BYTES = 64 * 1024; + const MAX_LINES = 30; try { - // Read only the first 8KB to avoid loading huge JSONL files into memory const fd = openSync(filePath, "r"); - const buf = Buffer.alloc(8192); - const bytesRead = readSync(fd, buf, 0, 8192, 0); + const buf = Buffer.alloc(MAX_BYTES); + const bytesRead = readSync(fd, buf, 0, MAX_BYTES, 0); closeSync(fd); const text = buf.toString("utf-8", 0, bytesRead); - const lines = text.split("\n").slice(0, 15); - for (const line of lines) { + // Drop the final segment — it may be an incomplete line at the cap boundary. + const parts = text.split("\n"); + const completeLines = parts.length > 1 ? parts.slice(0, -1) : parts; + for (const line of completeLines.slice(0, MAX_LINES)) { if (!line.trim()) continue; try { const obj = JSON.parse(line); From f75f891512af39bf354fe0e5bb86187822a864fb Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Sun, 17 May 2026 19:53:08 -0700 Subject: [PATCH 08/13] test: export downloadFile, parsePdfFromFile, extractCwdFromJsonl These three internal helpers are now imported by regression tests landing in the next commits (PR #1169 follow-up). Pattern matches the existing normalizeRemoteUrl export in gstack-global-discover.ts which test/global-discover.test.ts already imports side-effect-free. No change to runtime behavior; gstack has no public package entrypoint that would re-export these, so the in-repo surface is unchanged for callers. Co-Authored-By: Claude Opus 4.7 (1M context) --- bin/gstack-global-discover.ts | 2 +- browse/src/meta-commands.ts | 2 +- browse/src/security-classifier.ts | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/bin/gstack-global-discover.ts b/bin/gstack-global-discover.ts index 7f07141008..79189e42a2 100644 --- a/bin/gstack-global-discover.ts +++ b/bin/gstack-global-discover.ts @@ -273,7 +273,7 @@ function resolveClaudeCodeCwd( return null; } -function extractCwdFromJsonl(filePath: string): string | null { +export function extractCwdFromJsonl(filePath: string): string | null { // Read a capped prefix so huge JSONL files don't blow up memory. 64KB // comfortably fits the largest observed session headers; the old 8KB cap // would sometimes fall inside a single long line and silently drop the diff --git a/browse/src/meta-commands.ts b/browse/src/meta-commands.ts index bbd90f62d0..32bc1344fc 100644 --- a/browse/src/meta-commands.ts +++ b/browse/src/meta-commands.ts @@ -136,7 +136,7 @@ function parsePdfArgs(args: string[]): ParsedPdfArgs { return result; } -function parsePdfFromFile(payloadPath: string): ParsedPdfArgs { +export function parsePdfFromFile(payloadPath: string): ParsedPdfArgs { // Parity with load-html --from-file (browse/src/write-commands.ts) and // the direct load-html path: every caller-supplied file path // must pass validateReadPath so the safe-dirs policy can't be skirted diff --git a/browse/src/security-classifier.ts b/browse/src/security-classifier.ts index 7d8029b3a3..54f770c792 100644 --- a/browse/src/security-classifier.ts +++ b/browse/src/security-classifier.ts @@ -135,7 +135,7 @@ export function getClassifierStatus(): ClassifierStatus { // ─── Model download + staging ──────────────────────────────── -async function downloadFile(url: string, dest: string): Promise { +export async function downloadFile(url: string, dest: string): Promise { const res = await fetch(url); if (!res.ok || !res.body) { throw new Error(`Failed to fetch ${url}: ${res.status} ${res.statusText}`); From 206e11438795164ec2b5bdc5e7b49b3285a09dc7 Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Sun, 17 May 2026 20:28:56 -0700 Subject: [PATCH 09/13] fix(security-classifier): await writer close before unlinking tmp on error MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The earlier downloadFile() error-path cleanup hit a race: Node's createWriteStream lazily opens the FD and flushes buffered writes during destroy(), so a naive `fs.unlinkSync(tmp)` immediately after `writer.destroy()` hits ENOENT (file not yet on disk), then the writer's destroy finishes on the next tick and creates the file fresh — leaving the half-written tmp behind exactly as the original fix tried to prevent. The new sequence awaits the writer's 'close' event before unlinking, so the FD is fully torn down and no subsequent flush can re-create the path. Caught by browse/test/security-classifier-download-cleanup.test.ts in the next commit. Co-Authored-By: Claude Opus 4.7 (1M context) --- browse/src/security-classifier.ts | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/browse/src/security-classifier.ts b/browse/src/security-classifier.ts index 54f770c792..0c8304b669 100644 --- a/browse/src/security-classifier.ts +++ b/browse/src/security-classifier.ts @@ -156,9 +156,15 @@ export async function downloadFile(url: string, dest: string): Promise { }); fs.renameSync(tmp, dest); } catch (err) { - // Close the writer and drop the half-written tmp so we don't leak an FD - // or ship a truncated model file to the renameSync path on retry. - try { writer.destroy(); } catch { /* already destroyed */ } + // Drop the half-written tmp so we don't ship a truncated model file to + // a retry's renameSync. Wait for the writer to close fully before + // unlinking: Node's createWriteStream lazily opens the FD and flushes + // buffered writes during destroy(), so a naive unlinkSync hits ENOENT + // first and the writer re-creates the file on the next tick. + await new Promise((resolve) => { + writer.once('close', () => resolve()); + writer.destroy(); + }); try { fs.unlinkSync(tmp); } catch { /* nothing to clean */ } throw err; } From d3bc54526828d01a861d74ce4c0cd9a8a22de56d Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Sun, 17 May 2026 20:29:08 -0700 Subject: [PATCH 10/13] test(browse): regression tests for downloadFile cleanup + parsePdfFromFile guard MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Covers PR #1169 bugs #6 and #7: - security-classifier-download-cleanup.test.ts pins downloadFile error-path cleanup against three failure shapes: reader rejects mid-stream, non-2xx response, missing body. Asserts the dest file is not created and no .tmp.* siblings remain (glob-matched, not exact path — codex push: if the fix later switches to mkdtempSync, the assertion still holds). Includes a happy-path case so the cleanup isn't fighting a correct download. - regression-pr1169-pdf-from-file-invalid-json.test.ts pins parsePdfFromFile to throw a helpful error for: invalid JSON, empty file, top-level array, top-level number, top-level string, top-level null, top-level boolean. Codex push: JSON.parse accepts primitives too, so Array.isArray + typeof guard must be tested separately from the JSON.parse try/catch. Both files use mkdtempSync(process.cwd()/...) for fixture isolation since SAFE_DIRECTORIES allows TEMP_DIR or cwd; cwd is universal across CI hosts. Co-Authored-By: Claude Opus 4.7 (1M context) --- ...-pr1169-pdf-from-file-invalid-json.test.ts | 83 +++++++++++ ...curity-classifier-download-cleanup.test.ts | 138 ++++++++++++++++++ 2 files changed, 221 insertions(+) create mode 100644 browse/test/regression-pr1169-pdf-from-file-invalid-json.test.ts create mode 100644 browse/test/security-classifier-download-cleanup.test.ts diff --git a/browse/test/regression-pr1169-pdf-from-file-invalid-json.test.ts b/browse/test/regression-pr1169-pdf-from-file-invalid-json.test.ts new file mode 100644 index 0000000000..834bce078c --- /dev/null +++ b/browse/test/regression-pr1169-pdf-from-file-invalid-json.test.ts @@ -0,0 +1,83 @@ +/** + * Regression test for PR #1169 bug #7 — `pdf --from-file` ran JSON.parse on + * user-supplied file contents with no try/catch. A malformed payload crashed + * the pdf handler with a raw SyntaxError. Codex flagged that JSON.parse + * accepts primitives too (numbers, strings, null) and Array.isArray must be + * checked separately, so the fix added an explicit object-shape gate. + * + * Test surface: parsePdfFromFile, exported for tests at meta-commands.ts:139. + * All fixtures land in process.cwd() (SAFE_DIRECTORIES allows TEMP_DIR or cwd; + * cwd is universally safe on every platform our CI runs on). + */ +import { describe, expect, test, beforeAll, afterAll } from "bun:test"; +import * as fs from "node:fs"; +import * as path from "node:path"; + +import { parsePdfFromFile } from "../src/meta-commands"; + +const FIXTURE_DIR = fs.mkdtempSync(path.join(process.cwd(), "pr1169-pdf-")); + +beforeAll(() => { + // mkdtempSync already created the dir +}); + +afterAll(() => { + fs.rmSync(FIXTURE_DIR, { recursive: true, force: true }); +}); + +function writeFixture(name: string, body: string): string { + const p = path.join(FIXTURE_DIR, name); + fs.writeFileSync(p, body); + return p; +} + +describe("parsePdfFromFile — invalid JSON regression (PR #1169 bug #7)", () => { + test("invalid JSON: throws with file path AND parser detail", () => { + const p = writeFixture("invalid.json", "{ not-json"); + expect(() => parsePdfFromFile(p)).toThrow(/not valid JSON/); + expect(() => parsePdfFromFile(p)).toThrow(p); + }); + + test("empty file: throws JSON-parse style error", () => { + const p = writeFixture("empty.json", ""); + // Empty string is invalid JSON per ECMA-404. + expect(() => parsePdfFromFile(p)).toThrow(/not valid JSON/); + }); + + test("top-level array: throws 'must be a JSON object' with type", () => { + const p = writeFixture("array.json", JSON.stringify(["a", "b"])); + expect(() => parsePdfFromFile(p)).toThrow(/must be a JSON object/); + expect(() => parsePdfFromFile(p)).toThrow(/array/); + }); + + test("top-level number: throws with 'number' type label", () => { + const p = writeFixture("number.json", "42"); + expect(() => parsePdfFromFile(p)).toThrow(/must be a JSON object/); + expect(() => parsePdfFromFile(p)).toThrow(/number/); + }); + + test("top-level string: throws with 'string' type label", () => { + const p = writeFixture("string.json", JSON.stringify("hello")); + expect(() => parsePdfFromFile(p)).toThrow(/must be a JSON object/); + expect(() => parsePdfFromFile(p)).toThrow(/string/); + }); + + test("top-level null: throws with 'object' type label (JS null typeof === object)", () => { + const p = writeFixture("null.json", "null"); + // null passes typeof === 'object' but the fix's `=== null` branch catches it. + expect(() => parsePdfFromFile(p)).toThrow(/must be a JSON object/); + }); + + test("top-level boolean: throws with 'boolean' type label", () => { + const p = writeFixture("bool.json", "true"); + expect(() => parsePdfFromFile(p)).toThrow(/must be a JSON object/); + expect(() => parsePdfFromFile(p)).toThrow(/boolean/); + }); + + test("valid object: parses successfully (happy-path regression)", () => { + const p = writeFixture("valid.json", JSON.stringify({ format: "A4", pageNumbers: true })); + const result = parsePdfFromFile(p); + expect(result.format).toBe("A4"); + expect(result.pageNumbers).toBe(true); + }); +}); diff --git a/browse/test/security-classifier-download-cleanup.test.ts b/browse/test/security-classifier-download-cleanup.test.ts new file mode 100644 index 0000000000..af82961f18 --- /dev/null +++ b/browse/test/security-classifier-download-cleanup.test.ts @@ -0,0 +1,138 @@ +/** + * Regression test for PR #1169 bug #6 — downloadFile opened a WriteStream to + * `.tmp.` but never closed it on error paths. If the reader or + * writer threw mid-download, the FD leaked and the half-written tmp could + * be promoted by a retry's renameSync. + * + * The fix wraps the read loop in try/catch and runs `writer.destroy()` + + * `fs.unlinkSync(tmp)` before rethrowing. + * + * Per codex's pushback, this test must exercise BOTH the reader-throws path + * and the non-2xx-response path, and it must NOT assume the specific tmp + * filename — only that no `.tmp.*` sibling remains. + */ +import { describe, expect, test, beforeAll, afterAll, beforeEach, afterEach } from "bun:test"; +import * as fs from "node:fs"; +import * as path from "node:path"; + +import { downloadFile } from "../src/security-classifier"; + +function tmpSiblings(destDir: string, destBase: string): string[] { + if (!fs.existsSync(destDir)) return []; + return fs.readdirSync(destDir).filter((f) => + f.startsWith(destBase + ".tmp.") + ); +} + +let FIXTURE_DIR = ""; +let originalFetch: typeof fetch; + +beforeAll(() => { + FIXTURE_DIR = fs.mkdtempSync(path.join(process.cwd(), "pr1169-dl-")); +}); + +afterAll(() => { + if (FIXTURE_DIR) { + fs.rmSync(FIXTURE_DIR, { recursive: true, force: true }); + } +}); + +beforeEach(() => { + originalFetch = globalThis.fetch; +}); + +afterEach(() => { + globalThis.fetch = originalFetch; +}); + +describe("downloadFile error-path cleanup (PR #1169 bug #6)", () => { + test("reader rejects mid-stream: throws, no dest, no tmp sibling left", async () => { + const dest = path.join(FIXTURE_DIR, "reader-fail-model.bin"); + const destDir = path.dirname(dest); + const destBase = path.basename(dest); + + // Build a ReadableStream that emits one chunk then errors on second pull. + const body = new ReadableStream({ + start(controller) { + controller.enqueue(new Uint8Array([1, 2, 3, 4])); + }, + pull(controller) { + // Second pull triggers the failure path the fix protects against. + controller.error(new Error("simulated mid-stream read failure")); + }, + }); + + // @ts-expect-error — overwrite global fetch for the test + globalThis.fetch = async () => + new Response(body, { status: 200, statusText: "OK" }); + + await expect(downloadFile("https://example.com/model.bin", dest)).rejects.toThrow( + /simulated mid-stream read failure/ + ); + + expect(fs.existsSync(dest)).toBe(false); + expect(tmpSiblings(destDir, destBase)).toEqual([]); + }); + + test("non-2xx response: throws with status, no tmp file created", async () => { + const dest = path.join(FIXTURE_DIR, "http500-model.bin"); + const destDir = path.dirname(dest); + const destBase = path.basename(dest); + + // @ts-expect-error — overwrite global fetch for the test + globalThis.fetch = async () => + new Response("server boom", { status: 500, statusText: "Server Error" }); + + await expect(downloadFile("https://example.com/model.bin", dest)).rejects.toThrow( + /Failed to fetch.*500/ + ); + + expect(fs.existsSync(dest)).toBe(false); + expect(tmpSiblings(destDir, destBase)).toEqual([]); + }); + + test("missing body: throws, no tmp file created", async () => { + const dest = path.join(FIXTURE_DIR, "nobody-model.bin"); + const destDir = path.dirname(dest); + const destBase = path.basename(dest); + + // Response with null body (some upstreams send this on edge errors). + // @ts-expect-error — overwrite global fetch for the test + globalThis.fetch = async () => + new Response(null, { status: 200, statusText: "OK" }); + + await expect(downloadFile("https://example.com/model.bin", dest)).rejects.toThrow( + /Failed to fetch/ + ); + + expect(fs.existsSync(dest)).toBe(false); + expect(tmpSiblings(destDir, destBase)).toEqual([]); + }); + + test("happy path: 2xx body completes, dest exists, no tmp sibling remains", async () => { + const dest = path.join(FIXTURE_DIR, "ok-model.bin"); + const destDir = path.dirname(dest); + const destBase = path.basename(dest); + + const body = new ReadableStream({ + start(controller) { + controller.enqueue(new Uint8Array([9, 9, 9, 9])); + controller.close(); + }, + }); + + // @ts-expect-error — overwrite global fetch for the test + globalThis.fetch = async () => + new Response(body, { status: 200, statusText: "OK" }); + + await downloadFile("https://example.com/model.bin", dest); + + expect(fs.existsSync(dest)).toBe(true); + expect(tmpSiblings(destDir, destBase)).toEqual([]); + const written = fs.readFileSync(dest); + expect(Array.from(written)).toEqual([9, 9, 9, 9]); + + fs.unlinkSync(dest); + }); +}); + From acd485ff11af3c08eb742c260a7e18144b3546a6 Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Sun, 17 May 2026 20:29:23 -0700 Subject: [PATCH 11/13] test(global-discover): regression for extractCwdFromJsonl 64KB cap MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR #1169 bug #8: the 8KB read cap landed mid-line on Claude Code session headers, JSON.parse threw on the truncated tail, the catch silently continued, and the project disappeared from /gstack discovery output. Six new cases under describe("extractCwdFromJsonl 64KB cap"): - happy path: small JSONL with obj.cwd returns it - 12KB first line with obj.cwd: returns cwd (the bug case) - 80KB single line overflowing 64KB: returns null without crashing - complete line followed by partial second line: trailing-partial-drop must not poison the result; returns first line's cwd - missing file: returns null (file read error swallowed) - malformed first line + valid second line within cap: skips bad, returns second's cwd Tests use the exported extractCwdFromJsonl (added in earlier export commit) and live in a separate describe block from the existing "4KB / 128KB buffer" tests, which exercise the unrelated scanCodex meta.payload.cwd path at L338 — different function, different bug. Co-Authored-By: Claude Opus 4.7 (1M context) --- test/global-discover.test.ts | 88 ++++++++++++++++++++++++++++++++++++ 1 file changed, 88 insertions(+) diff --git a/test/global-discover.test.ts b/test/global-discover.test.ts index e541644c2e..f433da8c4a 100644 --- a/test/global-discover.test.ts +++ b/test/global-discover.test.ts @@ -343,4 +343,92 @@ describe("gstack-global-discover", () => { expect(remotes.length).toBe(uniqueRemotes.size); }); }); + + describe("extractCwdFromJsonl 64KB cap (PR #1169 bug #8)", () => { + // Regression: the old 8KB cap landed mid-line on Claude Code sessions with + // long headers, JSON.parse threw on the truncated tail, the catch + // `continue`d silently, and the project disappeared from discovery. + // The fix raised the cap to 64KB AND drops the trailing partial segment + // before parsing. + let extractCwdFromJsonl: (filePath: string) => string | null; + let tmpDir: string; + + beforeEach(async () => { + const mod = await import("../bin/gstack-global-discover.ts"); + extractCwdFromJsonl = mod.extractCwdFromJsonl; + tmpDir = mkdtempSync(join(tmpdir(), "pr1169-cwd-")); + }); + + afterEach(() => { + rmSync(tmpDir, { recursive: true, force: true }); + }); + + test("happy path: small JSONL with obj.cwd returns it (sanity)", () => { + const filePath = join(tmpDir, "small.jsonl"); + const line = JSON.stringify({ cwd: "/tmp/repo-small", type: "header" }); + writeFileSync(filePath, line + "\n"); + expect(extractCwdFromJsonl(filePath)).toBe("/tmp/repo-small"); + }); + + test("12KB first line with obj.cwd: returns cwd (old 8KB cap returned null)", () => { + // Pad a JSONL header so the whole line is ~12KB ending in `}\n`. + // Old 8KB read would slice mid-line; JSON.parse on the truncated tail + // would throw, the catch would `continue`, and we'd return null. + const padding = "x".repeat(12 * 1024); + const line = JSON.stringify({ + cwd: "/tmp/repo-12k", + type: "header", + notes: padding, + }); + expect(line.length).toBeGreaterThan(8 * 1024); + expect(line.length).toBeLessThan(64 * 1024); + + const filePath = join(tmpDir, "header-12k.jsonl"); + writeFileSync(filePath, line + "\n"); + expect(extractCwdFromJsonl(filePath)).toBe("/tmp/repo-12k"); + }); + + test("80KB single line (overflows 64KB cap): returns null without crashing", () => { + // One line >64KB with no newline inside the read window. The 64KB read + // captures a truncated prefix, parts.length === 1, no trailing drop + // applies, JSON.parse throws, catch returns null. The fix's + // trailing-partial-drop must not crash on this shape. + const padding = "y".repeat(80 * 1024); + const line = JSON.stringify({ cwd: "/tmp/repo-80k", type: "header", notes: padding }); + expect(line.length).toBeGreaterThan(64 * 1024); + + const filePath = join(tmpDir, "header-80k.jsonl"); + writeFileSync(filePath, line + "\n"); + // Don't throw, just return null. + expect(extractCwdFromJsonl(filePath)).toBeNull(); + }); + + test("complete line followed by partial second line: returns first line's cwd", () => { + // Line 1 ends cleanly with `\n` well within the cap. + // Line 2 is long enough that the 64KB read captures only its incomplete + // beginning. The trailing-partial drop must skip the truncated line 2 + // and not poison the result. + const line1 = JSON.stringify({ cwd: "/tmp/repo-line-1", type: "header" }); + const line2Padding = "z".repeat(80 * 1024); + const line2 = JSON.stringify({ cwd: "/tmp/repo-line-2", notes: line2Padding }); + + const filePath = join(tmpDir, "header-partial-2.jsonl"); + writeFileSync(filePath, line1 + "\n" + line2 + "\n"); + expect(extractCwdFromJsonl(filePath)).toBe("/tmp/repo-line-1"); + }); + + test("missing file: returns null (file read error is swallowed)", () => { + const filePath = join(tmpDir, "nonexistent.jsonl"); + expect(extractCwdFromJsonl(filePath)).toBeNull(); + }); + + test("malformed first line then valid second line within cap: returns second", () => { + // Both lines fully within 64KB. First line is not valid JSON; second + // is. The function must skip first and return second's cwd. + const filePath = join(tmpDir, "bad-then-good.jsonl"); + const good = JSON.stringify({ cwd: "/tmp/repo-skip-bad" }); + writeFileSync(filePath, "{ not valid json\n" + good + "\n"); + expect(extractCwdFromJsonl(filePath)).toBe("/tmp/repo-skip-bad"); + }); + }); }); From 81645b29e527f2905c42c689f7ad5096ffbefb02 Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Sun, 17 May 2026 20:29:42 -0700 Subject: [PATCH 12/13] test: regression tests for shell-script bugs in PR #1169 (#2-#5) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two new test files pinning the four shell-script invariants from the external audit: regression-pr1169-build-app-sed.test.ts — bugs #2 + #3 - Runtime isolation: extracts the sed-escape sequence from build-app.sh and runs it against hostile $APP_NAME values ("Foo/Bar&Baz", "Cool\App", "A/B\C&D"). Asserts the literal hostile name round-trips through a real `sed s///` invocation, locking the metachar safety end-to-end. - Static check: the rebrand block must contain both the escape line AND the sed line referencing $APP_NAME_SED_ESCAPED; bare $APP_NAME interpolation directly into the s/// replacement is rejected. - Static check: DMG_TMP=$(mktemp -d) is followed by an explicit `|| { ... exit }` failure handler AND a `[ -z "$DMG_TMP" ] || [ ! -d "$DMG_TMP" ]` validation AND the cp -a appears AFTER both guards. - Runtime fake-bin: extracts the guard shape, runs with a fake mktemp that exits 1, asserts the script exits non-zero before any cp block can reach. regression-pr1169-mktemp-fallbacks.test.ts — bugs #4 + #5 - Per codex pushback, the invariant is "no `mktemp ... || echo ` fallback shape" — not just "no $$ token." That's a stronger invariant that catches future swaps to $RANDOM or hardcoded paths. - For each of bin/gstack-telemetry-sync and supabase/verify-rls.sh: - no echo-based fallback after mktemp - no $$ inside any /tmp path literal - mktemp failure path explicitly exits / returns non-zero - telemetry-sync also pins the `trap rm -f $RESP_FILE EXIT` cleanup so success paths don't leak the tmp on normal exit. All seven new test files are gate-tier (deterministic, sub-second, no LLM, no network). Runtime shell tests use fake-bin PATH stubs in temp dirs; no $HOME mutation. Co-Authored-By: Claude Opus 4.7 (1M context) --- test/regression-pr1169-build-app-sed.test.ts | 161 ++++++++++++++++++ ...regression-pr1169-mktemp-fallbacks.test.ts | 82 +++++++++ 2 files changed, 243 insertions(+) create mode 100644 test/regression-pr1169-build-app-sed.test.ts create mode 100644 test/regression-pr1169-mktemp-fallbacks.test.ts diff --git a/test/regression-pr1169-build-app-sed.test.ts b/test/regression-pr1169-build-app-sed.test.ts new file mode 100644 index 0000000000..8d25961121 --- /dev/null +++ b/test/regression-pr1169-build-app-sed.test.ts @@ -0,0 +1,161 @@ +/** + * Regression tests for PR #1169 bugs #2 + #3 — scripts/build-app.sh. + * + * Bug #2: sed replacement for Chromium rebrand interpolated $APP_NAME without + * escaping sed replacement metachars (`&`, `/`, `\`). A name with `/` either + * broke the s/// command or got interpreted as sed syntax. + * + * Bug #3: `DMG_TMP=$(mktemp -d)` was unchecked. On mktemp failure $DMG_TMP + * was empty and the next `cp -a "$APP_DIR" "$DMG_TMP/"` would copy the .app + * bundle into the filesystem root. + * + * Bug #2 is verified via a runtime isolation test of the sed-escape sequence + * (codex pushback: static-grep for "uses escape helper" is too narrow; the + * real invariant is metachar safety end-to-end). Bug #3 is verified via + * static check — the entire build flow needs xcrun/hdiutil and can't be + * spawned in CI, but the failure-guard shape is what we want to lock. + */ +import { describe, expect, test } from "bun:test"; +import * as fs from "node:fs"; +import * as path from "node:path"; +import { spawnSync } from "node:child_process"; + +const ROOT = path.resolve(import.meta.dir, ".."); +const SCRIPT = path.join(ROOT, "scripts/build-app.sh"); + +describe("PR #1169 bug #2: build-app.sh sed escape for $APP_NAME", () => { + test("escape sequence produces sed-safe output for `&`, `/`, `\\` in APP_NAME", () => { + // Mirror the script's escape sequence and run it in isolation against a + // hostile name. The escape sequence at line ~98 is: + // APP_NAME_SED_ESCAPED=$(printf '%s' "$APP_NAME" | sed 's/[&/\]/\\&/g') + // We assert the resulting string can then be used as a sed replacement + // safely — round-trip via a real `sed s///` against a stub strings file. + + const inputs: string[] = [ + "Foo/Bar&Baz", // slash + ampersand + "Cool\\App", // backslash + "Plain Name", // no metachars (baseline) + "A/B\\C&D", // all three at once + "End/", // trailing slash + "&Start", // leading ampersand + ]; + + for (const appName of inputs) { + // Bug #2 invariant: the escaped string, used as the replacement half + // of `sed s///g`, results in the literal appName + // appearing in the output. + const result = spawnSync( + "bash", + ["-c", + `set -eu + APP_NAME="$1" + APP_NAME_SED_ESCAPED=$(printf '%s' "$APP_NAME" | sed 's/[&/\\]/\\\\&/g') + printf 'Google Chrome for Testing' | sed "s/Google Chrome for Testing/\${APP_NAME_SED_ESCAPED}/g" + `, + "_", + appName, + ], + { encoding: "utf-8" } + ); + + expect(result.status).toBe(0); + expect(result.stdout).toBe(appName); + expect(result.stderr).toBe(""); + } + }); + + test("script body still routes APP_NAME through the escape helper before sed", () => { + // Belt-and-braces static check: the rebrand block must contain BOTH the + // escape line and the sed line referencing the escaped variable. + const body = fs.readFileSync(SCRIPT, "utf-8"); + expect(body).toMatch(/APP_NAME_SED_ESCAPED=\$\(printf '%s' "\$APP_NAME" \| sed/); + expect(body).toMatch(/sed -i ''\s*"s\/Google Chrome for Testing\/\$\{APP_NAME_SED_ESCAPED\}\/g"/); + }); + + test("no bare `$APP_NAME` interpolation directly into the rebrand sed", () => { + // Ensure no future refactor reintroduces the bug by interpolating + // $APP_NAME straight into the s/// replacement. + const body = fs.readFileSync(SCRIPT, "utf-8"); + expect(body).not.toMatch(/sed -i ''\s*"s\/Google Chrome for Testing\/\$APP_NAME\//); + expect(body).not.toMatch(/sed -i ''\s*"s\/Google Chrome for Testing\/\$\{APP_NAME\}\//); + }); +}); + +describe("PR #1169 bug #3: build-app.sh DMG_TMP mktemp failure guard", () => { + test("mktemp -d for DMG_TMP is followed by an explicit failure handler", () => { + const body = fs.readFileSync(SCRIPT, "utf-8"); + // The script must assign DMG_TMP and immediately check for failure on + // the SAME line via `||`, then validate the path is non-empty and a real + // directory before cp. + const guard = body.match( + /DMG_TMP=\$\(mktemp -d\)\s*\|\|\s*\{[^}]*exit\s+\d/ + ); + expect(guard).not.toBeNull(); + }); + + test("DMG_TMP is also validated as non-empty AND a directory before cp", () => { + const body = fs.readFileSync(SCRIPT, "utf-8"); + // After mktemp, a defensive check should reject empty or non-directory + // paths (covers cases where mktemp succeeds but returns garbage). + expect(body).toMatch( + /\[\s*-z\s+"\$DMG_TMP"\s*\][^\n]*\|\|\s*\[\s*!\s+-d\s+"\$DMG_TMP"\s*\]/ + ); + }); + + test("no `cp -a ... \"$DMG_TMP/\"` before the validation block", () => { + const body = fs.readFileSync(SCRIPT, "utf-8"); + // The cp must come AFTER the validation. Find the line offsets. + const mktempIdx = body.search(/DMG_TMP=\$\(mktemp -d\)/); + const validationIdx = body.search( + /\[\s*-z\s+"\$DMG_TMP"\s*\]/ + ); + const cpIdx = body.search(/cp -a "\$APP_DIR" "\$DMG_TMP\//); + expect(mktempIdx).toBeGreaterThan(-1); + expect(validationIdx).toBeGreaterThan(mktempIdx); + expect(cpIdx).toBeGreaterThan(validationIdx); + }); + + test("runtime: escape function refuses to leave DMG_TMP empty (fake-mktemp PATH stub)", () => { + // Codex strongly preferred runtime testing here. The full build-app.sh + // depends on xcrun/hdiutil/PlistBuddy — too heavy for CI. Instead, we + // extract just the failure-guard shape and run it with a fake mktemp + // that always exits 1. Asserts the script exits non-zero before cp. + + const fakeBin = fs.mkdtempSync(path.join("/tmp", "pr1169-fakebin-")); + fs.writeFileSync( + path.join(fakeBin, "mktemp"), + "#!/bin/sh\nexit 1\n", + { mode: 0o755 } + ); + + // The guard, isolated. Mirrors the actual script's logic. Use a regular + // string + array of lines so the embedded bash backticks/dollars don't + // get interpreted by the JS template-literal parser. + const guardScript = [ + 'set -u', + 'DMG_TMP=$(mktemp -d) || { echo "ERROR: mktemp -d failed — refusing to continue so we don\'t cp into the filesystem root." >&2; exit 1; }', + 'if [ -z "$DMG_TMP" ] || [ ! -d "$DMG_TMP" ]; then', + ' echo "ERROR: mktemp -d returned an invalid path (\'$DMG_TMP\')." >&2', + ' exit 1', + 'fi', + '# If we got here, we would run the cp block, which is the bug.', + 'echo "REACHED_CP_BLOCK_WHICH_IS_THE_BUG" >&2', + 'exit 0', + ].join('\n'); + + const result = spawnSync( + "bash", + ["-c", guardScript], + { + encoding: "utf-8", + env: { ...process.env, PATH: `${fakeBin}:${process.env.PATH}` }, + } + ); + + fs.rmSync(fakeBin, { recursive: true, force: true }); + + expect(result.status).not.toBe(0); + expect(result.stderr).toMatch(/mktemp -d failed|invalid path/); + expect(result.stderr).not.toMatch(/REACHED_CP_BLOCK_WHICH_IS_THE_BUG/); + }); +}); diff --git a/test/regression-pr1169-mktemp-fallbacks.test.ts b/test/regression-pr1169-mktemp-fallbacks.test.ts new file mode 100644 index 0000000000..0ed0d3cb2c --- /dev/null +++ b/test/regression-pr1169-mktemp-fallbacks.test.ts @@ -0,0 +1,82 @@ +/** + * Regression tests for PR #1169 bugs #4 + #5 — predictable `$$`-based tmp + * file fallbacks on mktemp failure. + * + * Per codex's pushback, the real invariant is not just "no `$$` token" — it's + * "no `mktemp ... || echo ` shape at all, AND mktemp failure + * exits cleanly." A future cleanup could swap `$$` for `$RANDOM` or a + * hardcoded path and silently keep the foot-gun. The static checks below + * lock the broader invariant. + * + * Runtime fake-bin tests for these two scripts would require setting up + * SUPABASE_URL, JSONL fixtures, rate files, and config state — disproportionate + * for the invariant. The static checks pin the actual shape of the bug. + */ +import { describe, expect, test } from "bun:test"; +import * as fs from "node:fs"; +import * as path from "node:path"; + +const ROOT = path.resolve(import.meta.dir, ".."); + +function readScript(rel: string): string { + return fs.readFileSync(path.join(ROOT, rel), "utf-8"); +} + +describe("PR #1169 bug #4: gstack-telemetry-sync mktemp fallback", () => { + const SCRIPT = "bin/gstack-telemetry-sync"; + + test("no `mktemp ... || echo ` fallback shape anywhere in the script", () => { + const body = readScript(SCRIPT); + // Match: mktemp call, optional pipe, then `|| echo ` + // The fallback shape regardless of what the fallback path looks like + // ($$, $RANDOM, hardcoded — all predictable). + const fallback = body.match(/mktemp[^|\n]*\|\|\s*echo\s+["']?[^"'\n]*/); + expect(fallback).toBeNull(); + }); + + test("no `$$` PID interpolation appears anywhere in a /tmp path literal", () => { + const body = readScript(SCRIPT); + // Catches any /tmp-style path that uses the PID as part of the name. + expect(body).not.toMatch(/\/tmp\/[^"'\s]*\$\$/); + }); + + test("mktemp failure path exits or skips this run", () => { + const body = readScript(SCRIPT); + // The mktemp invocation must be guarded by `|| { ... exit 0; }` or + // equivalent. Match the multi-line guard immediately after `mktemp`. + const guard = body.match( + /mktemp\s+[^\n]+\)["']\s*\|\|\s*\{[^}]*exit\s+\d/ + ); + expect(guard).not.toBeNull(); + }); + + test("trap cleans up the response file on EXIT (no leftover tmp on success)", () => { + const body = readScript(SCRIPT); + expect(body).toMatch(/trap\s+['"]rm\s+-f\s+"?\$RESP_FILE/); + }); +}); + +describe("PR #1169 bug #5: supabase/verify-rls.sh mktemp fallback", () => { + const SCRIPT = "supabase/verify-rls.sh"; + + test("no `mktemp ... || echo ` fallback shape", () => { + const body = readScript(SCRIPT); + const fallback = body.match(/mktemp[^|\n]*\|\|\s*echo\s+["']?[^"'\n]*/); + expect(fallback).toBeNull(); + }); + + test("no `$$` PID interpolation in /tmp path literals", () => { + const body = readScript(SCRIPT); + expect(body).not.toMatch(/\/tmp\/[^"'\s]*\$\$/); + }); + + test("mktemp failure path returns non-zero from check()", () => { + const body = readScript(SCRIPT); + // The check function must fail loudly — `return 1` (or `exit`) inside + // the mktemp error handler. Same multi-line guard shape. + const guard = body.match( + /mktemp\s+[^\n]+\)["']\s*\|\|\s*\{[^}]*(?:return|exit)\s+\d/ + ); + expect(guard).not.toBeNull(); + }); +}); From 4761d67182480d11069ebd82d157e788783d0d2c Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Mon, 18 May 2026 16:56:31 -0700 Subject: [PATCH 13/13] chore: bump version and changelog (v1.41.1.0) Co-Authored-By: Claude Opus 4.7 --- CHANGELOG.md | 42 ++++++++++++++++++++++++++++++++++++++++++ VERSION | 2 +- package.json | 2 +- 3 files changed, 44 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a8320798db..e9f0a71435 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,47 @@ # Changelog +## [1.41.1.0] - 2026-05-18 + +## **Seven HIGH-severity audit bugs land with regression tests pinning every fix.** +## **A new test suite caught a real race in the contributor's cleanup path — fixed before the wave shipped.** + +The external audit wave originally filed in #1169 lands as one consolidated release after rebasing onto v1.40.0.0 and adding regression coverage. The original commit for the disconnect-handler crash was dropped because that bug was independently fixed since v1.6.4.0; the remaining seven HIGH-severity bugs all reproduce on current main and ship with tests. The contributor's `downloadFile` cleanup path turned out to race with Node's `createWriteStream` lazy FD open — the new test caught it and the wave includes a follow-up fix that awaits the writer's `'close'` event before unlinking. + +### The numbers that matter + +Source: `bun test test/regression-pr1169-*.test.ts test/global-discover.test.ts browse/test/regression-pr1169-pdf-from-file-invalid-json.test.ts browse/test/security-classifier-download-cleanup.test.ts` — 51 assertions across 5 files, all green. Full `bun test` suite exits 0. + +| Surface | Before | After | +|---|---|---| +| `scripts/build-app.sh` rebrand with a `$APP_NAME` containing `/`, `&`, or `\` | sed `s///` either broke or interpreted the literal as syntax; trailing `\|\| true` hid the failure | `$APP_NAME` is escaped (`& / \`) before interpolation; runtime regression test round-trips hostile names through real `sed` | +| `scripts/build-app.sh` DMG step when `mktemp -d` fails | `$DMG_TMP` was empty; next line `cp -a "$APP_DIR" "$DMG_TMP/"` copied the bundle into the filesystem root | Explicit guard exits non-zero before `cp`; fake-mktemp PATH stub asserts the guard fires | +| `bin/gstack-telemetry-sync` and `supabase/verify-rls.sh` when mktemp fails | Fallback to `/tmp/...-$$` — predictable PID path lets an attacker pre-create or symlink the response file | mktemp failure skips/aborts cleanly; static invariants forbid any `mktemp \|\| echo` fallback shape | +| `browse/src/security-classifier.ts` `downloadFile` on reader rejection mid-stream | FD leaked; half-written `.tmp.` survived to be promoted by the next retry's `renameSync` | Writer is awaited via `'close'` event before unlinking, so the lazy FD open can't race the cleanup. Three failure paths covered: reader rejects, non-2xx response, missing body | +| `browse/src/meta-commands.ts` `pdf --from-file` with malformed payload | `JSON.parse` threw a raw `SyntaxError` to the user; arrays/null/primitives silently passed shape check | Wrapped `JSON.parse`; rejects array, number, string, boolean, null with a useful error referencing the file path | +| `bin/gstack-global-discover.ts` `extractCwdFromJsonl` on session headers >8KB | Read cap landed mid-line; `JSON.parse` threw on the truncated tail and the project disappeared from `/gstack` discovery | 64KB read cap; trailing partial segment is dropped so it can't poison earlier complete lines | + +### What this means for builders + +If you build the GStack Browser DMG from a workstation where `/tmp` is constrained, the build fails cleanly instead of cp'ing your app bundle into `/`. If you run `gstack-telemetry-sync` or `verify-rls.sh` on a shared host, mktemp failure aborts the run instead of writing through a predictable PID path. If the security classifier's model download hits a transient mid-stream error, the next retry sees a clean slate instead of inheriting a truncated ONNX file. If you run `/gstack` discovery across long-headered Claude Code sessions, the project shows up. Run `/gstack-upgrade` to pick up the fixes; no migration needed. + +### Itemized changes + +### Added +- Regression tests for every audit bug shipping in this wave: `test/regression-pr1169-build-app-sed.test.ts`, `test/regression-pr1169-mktemp-fallbacks.test.ts`, `test/global-discover.test.ts` (new `extractCwdFromJsonl 64KB cap` describe block), `browse/test/regression-pr1169-pdf-from-file-invalid-json.test.ts`, `browse/test/security-classifier-download-cleanup.test.ts`. 51 assertions across 5 files. + +### Fixed +- `scripts/build-app.sh`: escape sed replacement metachars (`&`, `/`, `\`) in `$APP_NAME` before the Chromium rebrand `s///` runs. Contributed by @RagavRida. +- `scripts/build-app.sh`: bail out cleanly when `mktemp -d` for the DMG staging dir returns empty or a non-directory, so a failure can't trick `cp -a` into copying into `/`. Contributed by @RagavRida. +- `bin/gstack-telemetry-sync`: drop the predictable `/tmp/gstack-sync-$$` fallback when `mktemp` fails; skip the run with a stderr note and clean the response file via an EXIT trap on the happy path. Contributed by @RagavRida. +- `supabase/verify-rls.sh`: drop the predictable `/tmp/verify-rls-$$-$TOTAL` fallback when `mktemp` fails; return non-zero from the check. Contributed by @RagavRida. +- `browse/src/security-classifier.ts`: `downloadFile` now awaits the writer's `'close'` event before unlinking the tmp file. The original cleanup path raced with Node's lazy FD open — naive `unlinkSync` hit ENOENT, then `writer.destroy()` finished asynchronously and re-created the file. Caught by the new test suite. +- `browse/src/security-classifier.ts`: `downloadFile` wraps the read loop in try/catch; on reader rejection, writer error, or non-2xx response the half-written tmp is unlinked and the FD is closed. Contributed by @RagavRida. +- `browse/src/meta-commands.ts`: `parsePdfFromFile` wraps `JSON.parse` and rejects top-level primitives (array, number, string, boolean, null) with a useful error pointing at the offending file. Contributed by @RagavRida. +- `bin/gstack-global-discover.ts`: `extractCwdFromJsonl` reads 64KB (up from 8KB) and drops the trailing partial segment before parsing, so Claude Code sessions with long headers stop disappearing from discovery output. Contributed by @RagavRida. + +### For contributors +- `downloadFile`, `parsePdfFromFile`, and `extractCwdFromJsonl` are now exported from their respective modules for test access. Pattern matches the existing `normalizeRemoteUrl` export in `bin/gstack-global-discover.ts`. + ## [1.40.0.0] - 2026-05-16 ## **gbrain sync stops biting users across the install path, slug algorithm, federation queue, and `.env.local` footgun.** diff --git a/VERSION b/VERSION index 895062404a..166ee9c396 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -1.40.0.0 +1.41.1.0 diff --git a/package.json b/package.json index 3851a78bd7..07ef3db95c 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "gstack", - "version": "1.40.0.0", + "version": "1.41.1.0", "description": "Garry's Stack — Claude Code skills + fast headless browser. One repo, one install, entire AI engineering workflow.", "license": "MIT", "type": "module",