v1.41.1.0 fix wave: 7 HIGH bugs from external audit + regression tests (PR #1169 follow-up)

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 `<dest>.tmp.<pid>` 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.**

VERSION

@@ -1 +1 @@
-1.40.0.0
+1.41.1.0

bin/gstack-global-discover.ts

@@ -273,16 +273,23 @@ 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
+  // 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);

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" \

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 <file> path: every caller-supplied file path
   // must pass validateReadPath so the safe-dirs policy can't be skirted
@@ -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,

browse/src/security-classifier.ts

@@ -135,7 +135,7 @@ export function getClassifierStatus(): ClassifierStatus {
 
 // ─── Model download + staging ────────────────────────────────
 
-async function downloadFile(url: string, dest: string): Promise<void> {
+export async function downloadFile(url: string, dest: string): Promise<void> {
   const res = await fetch(url);
   if (!res.ok || !res.body) {
     throw new Error(`Failed to fetch ${url}: ${res.status} ${res.statusText}`);
@@ -144,16 +144,30 @@ async function downloadFile(url: string, dest: string): Promise<void> {
   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<void>((resolve, reject) => {
+      writer.end((err?: Error | null) => (err ? reject(err) : resolve()));
+    });
+    fs.renameSync(tmp, dest);
+  } catch (err) {
+    // 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<void>((resolve) => {
+      writer.once('close', () => resolve());
+      writer.destroy();
+    });
+    try { fs.unlinkSync(tmp); } catch { /* nothing to clean */ }
+    throw err;
   }
-  await new Promise<void>((resolve, reject) => {
-    writer.end((err?: Error | null) => (err ? reject(err) : resolve()));
-  });
-  fs.renameSync(tmp, dest);
 }
 
 async function ensureTestsavantStaged(onProgress?: (msg: string) => void): Promise<void> {

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);
+  });
+});

browse/test/security-classifier-download-cleanup.test.ts

@@ -0,0 +1,138 @@
+/**
+ * Regression test for PR #1169 bug #6 — downloadFile opened a WriteStream to
+ * `<dest>.tmp.<pid>` 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 `<dest>.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<Uint8Array>({
+      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<Uint8Array>({
+      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);
+  });
+});
+