fix(brainstorm): launch BRAINSTORM_OPEN_CMD without a shell

skills/brainstorming/scripts/server.cjs

@@ -301,6 +301,33 @@ function browserLauncherForPlatform(url, {
   return null;
 }
 
+// Quote-aware tokenizer for the trusted BRAINSTORM_OPEN_CMD operator input.
+// Splits a single- or double-quoted command string into an argv (no shell, no
+// metacharacter expansion), so execFile can launch it without a `sh -c` layer.
+// Returns null on unbalanced quotes so a malformed command is never handed to
+// a shell as a fallback.
+function shellArgv(input) {
+  const argv = [];
+  let cur = '';
+  let quote = null;
+  for (let i = 0; i < input.length; i++) {
+    const ch = input[i];
+    if (quote) {
+      if (ch === quote) { quote = null; continue; }
+      cur += ch;
+    } else if (ch === '"' || ch === "'") {
+      quote = ch;
+    } else if (ch === ' ' || ch === '\t' || ch === '\n') {
+      if (cur.length) { argv.push(cur); cur = ''; }
+    } else {
+      cur += ch;
+    }
+  }
+  if (quote) return null; // unbalanced quotes — refuse to guess
+  if (cur.length) argv.push(cur);
+  return argv.length ? argv : null;
+}
+
 function isRegularFileInsideContentDir(filePath) {
   let stat, realContentDir, realFilePath;
   try {
@@ -535,9 +562,17 @@ function maybeOpenBrowser() {
   if (clients.size > 0) return; // the user already opened it
   const url = companionUrl(); // must carry the key or the gate 403s it
   const cp = require('child_process');
-  // Operator-provided launcher: run as given (this env var is trusted operator input).
+  // Operator-provided launcher: this env var is trusted operator input (opt-in,
+  // loopback-only), but we still avoid the shell entirely. Split the command into
+  // an argv with a quote-aware tokenizer and pass the URL as a single argv element
+  // via execFile, mirroring the platform launchers below — so a URL containing
+  // shell metacharacters can never reach a shell. A command that fails to tokenize
+  // (unbalanced quotes) is rejected rather than handed to a shell.
   if (process.env.BRAINSTORM_OPEN_CMD) {
-    try { cp.exec(process.env.BRAINSTORM_OPEN_CMD + ' ' + JSON.stringify(url), () => {}); } catch (e) { /* best effort */ }
+    const argv = shellArgv(process.env.BRAINSTORM_OPEN_CMD);
+    if (argv) {
+      try { cp.execFile(argv[0], argv.slice(1).concat(url), () => {}); } catch (e) { /* best effort */ }
+    }
     return;
   }
   // Platform launchers: pass the URL as an argv element via execFile (no shell),

skills/brainstorming/visual-companion.md

@@ -102,6 +102,22 @@ scripts/start-server.sh \
 
 Use `--url-host` to control what hostname is printed in the returned URL JSON.
 
+### Custom browser launcher: `BRAINSTORM_OPEN_CMD`
+
+The `--open` flag auto-launches the user's default browser using the platform's native launcher. For environments where that doesn't apply — a container with no desktop session, a headless dev box reached via a tunnel, or a non-standard browser binary — set `BRAINSTORM_OPEN_CMD` to the command that should open a URL:
+
+```bash
+BRAINSTORM_OPEN_CMD='firefox' scripts/start-server.sh --project-dir /path --open
+BRAINSTORM_OPEN_CMD='flatpak run org.mozilla.firefox' scripts/start-server.sh --project-dir /path --open
+```
+
+The server passes the companion URL as the final argument. Accepted shapes:
+- A bare command: `firefox` → `firefox <url>`
+- A command with arguments: `flatpak run org.mozilla.firefox` → `flatpak run org.mozilla.firefox <url>`
+- Quoted argv segments: `node "/path/to/capture.js" "marker"` → that exact argv with `<url>` appended
+
+**Trust posture:** `BRAINSTORM_OPEN_CMD` is an environment variable, so it runs with the invoking user's authority — only set it from a trusted environment, and treat any value the way you'd treat a shell command you typed yourself. The launcher deliberately invokes the command **without a shell** (it tokenizes the value and passes `<url>` as a single argv element), so shell metacharacters in the value are not interpreted — the URL's `&`/`?` stay intact, and the value can't splice in extra commands. This means a value like `firefox ; curl evil` will *not* execute the `curl`; it just fails to find a binary named `firefox ; curl evil`. The no-shell path is the safe default; you don't need to do anything to get it.
+
 ## The Loop
 
 1. **Check server is alive**, then **write HTML** to a new file in `screen_dir`:

tests/brainstorm-server/lifecycle.test.js

@@ -490,6 +490,63 @@ async function runTests() {
     assert(!opened, 'must not open the browser without explicit approval');
   });
 
+  await test('BRAINSTORM_OPEN_CMD launches without a shell and keeps the URL as one argv element', async () => {
+    // Regression for #1957: BRAINSTORM_OPEN_CMD must not go through `sh -c`.
+    // Capture the launched argv with a node script that writes each argv element
+    // on its own line; the server URL carries a `?key=` query (shell metachar `&`),
+    // which a shell would split. execFile passes it intact as the final element.
+    const dir = fs.mkdtempSync('/tmp/bs-argv-');
+    const marker = path.join(dir, 'argv.log');
+    const scriptPath = path.resolve(dir, 'capture-argv.cjs');
+    fs.writeFileSync(scriptPath,
+      "const fs = require('fs');\n" +
+      "fs.appendFileSync(" + JSON.stringify(marker) + ", process.argv.slice(1).join('\\n') + '\\n');\n");
+    const openCmd = `node ${JSON.stringify(scriptPath)}`;
+    const srv = spawn('node', [SERVER], { env: { ...process.env, BRAINSTORM_PORT: 3420, BRAINSTORM_DIR: dir, BRAINSTORM_OPEN: '1', BRAINSTORM_OPEN_CMD: openCmd, BRAINSTORM_LIFECYCLE_CHECK_MS: 100000 } });
+    let out = ''; srv.stdout.on('data', d => out += d.toString());
+    for (let i = 0; i < 60 && !out.includes('server-started'); i++) await sleep(50);
+    fs.writeFileSync(path.join(dir, 'content', 'first.html'), '<h2>First</h2>');
+    await waitForFile(marker);
+    await killAndWait(srv);
+    const lines = fs.readFileSync(marker, 'utf8').trim().split('\n').filter(Boolean);
+    fs.rmSync(dir, { recursive: true, force: true });
+    // argv = [scriptPath, url]; the URL must be a single element carrying the key.
+    assert.strictEqual(lines.length, 2, `expected 2 argv elements (script + url), got ${lines.length}`);
+    assert(/key=/.test(lines[1]), `final argv element should be the URL carrying the key, got ${lines[1]}`);
+    assert(!/[ ;|]/.test(lines[1]), `URL must not be shell-split; found shell metacharacters in ${lines[1]}`);
+  });
+
+  await test('BRAINSTORM_OPEN_CMD metacharacters do not splice a second command (no shell)', async () => {
+    // Companion to the URL-integrity test above: prove shell metacharacters in the
+    // OPEN_CMD value itself can't inject a second command. A value containing a
+    // command separator (`; touch <pwned>`) must NOT result in <pwned> being
+    // created — the launcher tokenizes and runs without a shell, so `;` is literal.
+    // Without the no-shell fix (#1957), this value reached `sh -c`, the `;`
+    // split it, and `touch <pwned>` executed as a sibling command.
+    const dir = fs.mkdtempSync('/tmp/bs-inj-');
+    const marker = path.join(dir, 'argv.log');
+    const pwned = path.join(dir, 'pwned.flag');
+    const scriptPath = path.resolve(dir, 'capture-argv.cjs');
+    fs.writeFileSync(scriptPath,
+      "const fs = require('fs');\n" +
+      "fs.appendFileSync(" + JSON.stringify(marker) + ", process.argv.slice(1).join('\\n') + '\\n');\n");
+    // A shell would split on `;` and run `touch <pwned>` as a second command.
+    // The no-shell path treats the whole value as one tokenized argv, so the
+    // `;`-bearing command simply fails to launch and <pwned> never appears.
+    const openCmd = `node ${JSON.stringify(scriptPath)} ; touch ${JSON.stringify(pwned)}`;
+    const srv = spawn('node', [SERVER], { env: { ...process.env, BRAINSTORM_PORT: 3424, BRAINSTORM_DIR: dir, BRAINSTORM_OPEN: '1', BRAINSTORM_OPEN_CMD: openCmd, BRAINSTORM_LIFECYCLE_CHECK_MS: 100000 } });
+    let out = ''; srv.stdout.on('data', d => out += d.toString());
+    for (let i = 0; i < 60 && !out.includes('server-started'); i++) await sleep(50);
+    fs.writeFileSync(path.join(dir, 'content', 'first.html'), '<h2>Inject</h2>');
+    // Give the launcher (which fails to spawn the `;`-bearing command) time to
+    // have done nothing — wait long enough that a `sh -c` would have run `touch`.
+    await sleep(500);
+    await killAndWait(srv);
+    const pwnedCreated = fs.existsSync(pwned);
+    fs.rmSync(dir, { recursive: true, force: true });
+    assert(!pwnedCreated, `shell metacharacter ';' must not splice a command: <pwned> was created, meaning BRAINSTORM_OPEN_CMD went through a shell`);
+  });
+
   await test('unauthenticated requests do not defeat the idle timeout', async () => {
     const dir = fs.mkdtempSync('/tmp/bs-life-');
     const srv = spawn('node', [SERVER], { env: { ...process.env, BRAINSTORM_PORT: 3419, BRAINSTORM_DIR: dir, BRAINSTORM_TOKEN: 'authtok', BRAINSTORM_IDLE_TIMEOUT_MS: 400, BRAINSTORM_LIFECYCLE_CHECK_MS: 100 } });