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/bin/gstack-global-discover.ts b/bin/gstack-global-discover.ts index 4e1445b37a..79189e42a2 100644 --- a/bin/gstack-global-discover.ts +++ b/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); 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" \ diff --git a/browse/src/meta-commands.ts b/browse/src/meta-commands.ts index c505d4cf41..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 @@ -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, diff --git a/browse/src/security-classifier.ts b/browse/src/security-classifier.ts index 68a41ba265..0c8304b669 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}`); @@ -144,16 +144,30 @@ 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) { + // 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; } - 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 { 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); + }); +}); + 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", diff --git a/scripts/build-app.sh b/scripts/build-app.sh index 1c7b0c3039..8869212ab9 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) @@ -177,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" 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 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"); + }); + }); }); 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(); + }); +});