From d7098a31d9ed67d9a6a4bd617c963254c960dc02 Mon Sep 17 00:00:00 2001
From: Samuel Carson <samuel.carson@gmail.com>
Date: Tue, 21 Apr 2026 01:48:53 -0500
Subject: [PATCH] fix(browse): wrap telemetry bash script with bash.exe on
 Windows
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

`reportAttemptTelemetry()` in browse/src/security.ts spawns
`gstack-telemetry-log` directly via Node's `child_process.spawn(bin, args)`.
On Windows, that fails with `ENOENT` / `-4058` (UV_ENOENT) even when the
script exists on disk, because `gstack-telemetry-log` is a shebang-scripted
bash file (`#!/usr/bin/env bash`) and Windows `CreateProcess` cannot launch
shebangs — it treats the file as a PE image and bails when the header
doesn't match. The `child.on('error', ...)` handler swallows the error per
the fire-and-forget semantics, so the failure is invisible: attack-attempt
telemetry drops on the floor on every Windows install.

Empirical reproduction on Windows 11, Node v25, bun 1.3.11:

    spawn('/path/to/gstack-telemetry-log', [...])
    → spawn error: ENOENT - spawn /path/to/gstack-telemetry-log ENOENT
    → exit: -4058

    spawn('bash', ['/path/to/gstack-telemetry-log', ...])
    → exit: 0 | stdout: telemetry received: ...

Fix: extract `buildTelemetrySpawnCommand(bin, args)` as a pure function of
(platform, bin, args) that returns `{ cmd: 'bash', cmdArgs: [bin, ...args] }`
on win32 and `{ cmd: bin, cmdArgs: args }` on POSIX. `reportAttemptTelemetry`
consumes the tuple and hands it to `spawn` unchanged. The POSIX path is
bit-identical to the old code.

Windows dev boxes running gstack invariably ship Git Bash (the installer
puts `bash.exe` on PATH), so in practice the wrapped invocation works
without further setup. If `bash` is missing, `spawn` fires an 'error' event
and the same existing swallow path kicks in — local `attempts.jsonl` keeps
the audit trail intact either way. No new failure modes introduced.

Why this shipped unnoticed: no gstack workflow runs on Windows — the
matrix in every CI file is `ubuntu-latest` and/or `macos-latest` only.
On both POSIX platforms the shebang is honored by `execve` and the
spawn succeeds, so the bug never triggered in CI.

Why `shell: true` is NOT an acceptable alternative fix: `record.urlDomain`
is extracted from user-browsed URLs, and `record.payloadHash`/`record.layer`/
`record.verdict` also flow through this call. Passing `shell: true` would
route those through `cmd.exe` arg parsing and open a shell-injection path
the moment a URL contains a quote or backslash. Explicit `spawn('bash',
[...])` keeps every arg in argv, so escaping stays honest.

Tests: three new assertions covering the POSIX pass-through path, the
win32 bash wrap, and arg-array immutability. Guarded by
`if (process.platform === 'win32') return;` / its inverse, so POSIX CI
exercises the platform-relevant branch automatically and the Windows
branch is covered whenever a contributor runs `bun test` on Windows.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
---
 browse/src/security.ts       | 32 ++++++++++++++++++++++++++++++--
 browse/test/security.test.ts | 30 ++++++++++++++++++++++++++++++
 2 files changed, 60 insertions(+), 2 deletions(-)

diff --git a/browse/src/security.ts b/browse/src/security.ts
index a5d27ff2ad..2770096e6a 100644
--- a/browse/src/security.ts
+++ b/browse/src/security.ts
@@ -330,6 +330,33 @@ function findTelemetryBinary(): string | null {
   return null;
 }
 
+/**
+ * Build the [cmd, args] tuple for invoking a bash-script telemetry binary
+ * in a way that works on both POSIX and Windows.
+ *
+ * POSIX: returns [bin, args] unchanged — shebang gets honored by execve.
+ * Win32: wraps in bash explicitly. `gstack-telemetry-log` is a shell script
+ * (`#!/usr/bin/env bash`) and Windows `CreateProcess` can't dispatch on a
+ * shebang — it tries to load the file as a PE image, fails with ENOEXEC,
+ * and our 'error' handler silently swallows it. Most Windows dev boxes
+ * running gstack ship Git Bash, which puts bash.exe on PATH; if bash is
+ * missing, spawn() will still fire 'error' and the same swallow path kicks
+ * in. Either way, the local `attempts.jsonl` write in logAttempt() keeps
+ * the audit trail intact.
+ *
+ * Exported for testability — resolution is a pure function of (platform,
+ * bin, args) so we can assert on it without actually spawning.
+ */
+export function buildTelemetrySpawnCommand(
+  bin: string,
+  args: string[],
+): { cmd: string; cmdArgs: string[] } {
+  if (process.platform === 'win32') {
+    return { cmd: 'bash', cmdArgs: [bin, ...args] };
+  }
+  return { cmd: bin, cmdArgs: args };
+}
+
 /**
  * Fire-and-forget subprocess invocation of gstack-telemetry-log with the
  * attack_attempt event type. The binary handles tier gating internally
@@ -343,14 +370,15 @@ function reportAttemptTelemetry(record: AttemptRecord): void {
   const bin = findTelemetryBinary();
   if (!bin) return;
   try {
-    const child = spawn(bin, [
+    const { cmd, cmdArgs } = buildTelemetrySpawnCommand(bin, [
       '--event-type', 'attack_attempt',
       '--url-domain', record.urlDomain || '',
       '--payload-hash', record.payloadHash,
       '--confidence', String(record.confidence),
       '--layer', record.layer,
       '--verdict', record.verdict,
-    ], {
+    ]);
+    const child = spawn(cmd, cmdArgs, {
       stdio: 'ignore',
       detached: true,
     });
diff --git a/browse/test/security.test.ts b/browse/test/security.test.ts
index bf8064c039..d524147ee9 100644
--- a/browse/test/security.test.ts
+++ b/browse/test/security.test.ts
@@ -20,6 +20,7 @@ import {
   readSessionState,
   getStatus,
   extractDomain,
+  buildTelemetrySpawnCommand,
   type LayerSignal,
 } from '../src/security';
 
@@ -320,3 +321,32 @@ describe('extractDomain', () => {
     expect(extractDomain('')).toBe('');
   });
 });
+
+// ─── Telemetry spawn command (Windows bash wrapper) ──────────
+
+describe('buildTelemetrySpawnCommand', () => {
+  const bin = '/home/user/.claude/skills/gstack/bin/gstack-telemetry-log';
+  const args = ['--event-type', 'attack_attempt', '--confidence', '0.95'];
+
+  test('on POSIX, returns the binary path and args unchanged', () => {
+    if (process.platform === 'win32') return;
+    const out = buildTelemetrySpawnCommand(bin, args);
+    expect(out.cmd).toBe(bin);
+    expect(out.cmdArgs).toEqual(args);
+  });
+
+  test('on win32, wraps the call in bash with the script as first arg', () => {
+    if (process.platform !== 'win32') return;
+    const out = buildTelemetrySpawnCommand(bin, args);
+    expect(out.cmd).toBe('bash');
+    // Script path must come first so bash treats it as the file to execute,
+    // followed by the original telemetry flags as bash's positional args.
+    expect(out.cmdArgs).toEqual([bin, ...args]);
+  });
+
+  test('does not mutate the caller-supplied args array', () => {
+    const originalArgs = [...args];
+    buildTelemetrySpawnCommand(bin, args);
+    expect(args).toEqual(originalArgs);
+  });
+});