From 0134f29ec53f839d5fb8ed94954701795a6df2a6 Mon Sep 17 00:00:00 2001 From: Steve Date: Tue, 28 Jul 2026 10:39:32 -0600 Subject: [PATCH] fix(paths): shell-quote gstack-paths output so eval round-trips values MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `bin/gstack-paths` is consumed as `eval "$(gstack-paths)"`, but emitted bare `KEY=VALUE` lines with an unquoted RHS. eval re-parses those, so backslashes become escape sequences and spaces become word separators. On Windows this breaks every skill. $TMP is a backslash path, so: TMP=C:\Users\me\AppData\Local\Temp eval "$(gstack-paths)" echo "$TMP_ROOT" # C:UsersmeAppDataLocalTemp The following `mktemp "$TMP_ROOT/codex-err-XXXXXX.txt"` fails, TMPERR is empty, and `2>"$TMPERR"` is a redirect to an empty filename — so the bash block exits 1 before Codex ever runs and the user sees no review and no obvious cause. A value containing a space (C:\Program Files\Temp) is worse: eval word-splits it, prints `FilesTemp: command not found`, and leaves the variable empty. This is the same user-visible cascade as #2091 / #2370, which are filed as macOS/BSD-mktemp bugs. The root cause here is different, so those fixes don't help — a Windows user finds them, applies them, and is still broken. Not Windows-only: any POSIX TMPDIR containing a space or backslash corrupts the same way. Fix: emit with `printf %q` so eval reproduces values byte-for-byte. The script is already `#!/usr/bin/env bash`, so %q is available. Plain POSIX paths are unchanged by %q, so existing callers and tests see identical output. The header's existing "callers should quote expansions" note did not cover this: the corruption happens during eval, before the caller has a variable to quote. Reworded to say what the emitter now guarantees. Tests: three eval-round-trip cases (backslash, space, embedded quote) added to test/gstack-paths.test.ts. They fail against the old `echo` and pass with %q. The backslash case is skipped on win32 — MSYS rewrites backslashes to forward slashes in env values, so it cannot be injected through the environment on a Git Bash runner; the escape-eating it guards is pure eval semantics and is exercised on Linux/macOS CI. Same reasoning as the existing HOME-unset skips in that file. Fixes #2374 Co-Authored-By: Claude Opus 5 --- bin/gstack-paths | 18 +++++++++----- test/gstack-paths.test.ts | 51 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 63 insertions(+), 6 deletions(-) diff --git a/bin/gstack-paths b/bin/gstack-paths index 1a7e073065..2d86207ba3 100755 --- a/bin/gstack-paths +++ b/bin/gstack-paths @@ -13,9 +13,15 @@ # PLAN_ROOT: GSTACK_PLAN_DIR -> CLAUDE_PLANS_DIR -> $HOME/.claude/plans -> .claude/plans # TMP_ROOT: TMPDIR -> TMP -> .gstack/tmp (and mkdir -p, best-effort) # -# Security: output values are not sanitized — callers may receive paths with -# shell-special characters if env vars contain them. Skills should always quote -# expansions ("$GSTACK_STATE_ROOT", not $GSTACK_STATE_ROOT). +# Output: values are emitted shell-quoted (printf %q) so `eval` round-trips them +# byte-for-byte. This matters on Windows, where $TMP is a backslash path like +# C:\Users\me\AppData\Local\Temp — with a bare `echo`, eval consumes the +# backslashes as escapes and the caller gets C:UsersmeAppDataLocalTemp. A value +# containing a space (C:\Program Files\Temp) is worse: eval word-splits it and +# the variable ends up empty. Quoting here is the only fix that works, because +# the corruption happens during eval, before the caller has anything to quote. +# Callers should still quote expansions ("$GSTACK_STATE_ROOT") for the same +# reason any path variable needs quoting. set -u # State root: where gstack writes projects/, sessions/, analytics/. @@ -60,6 +66,6 @@ fi # will discover that on their own write attempt. Don't fail the eval here. mkdir -p "$_tmp_root" 2>/dev/null || true -echo "GSTACK_STATE_ROOT=$_state_root" -echo "PLAN_ROOT=$_plan_root" -echo "TMP_ROOT=$_tmp_root" +printf 'GSTACK_STATE_ROOT=%q\n' "$_state_root" +printf 'PLAN_ROOT=%q\n' "$_plan_root" +printf 'TMP_ROOT=%q\n' "$_tmp_root" diff --git a/test/gstack-paths.test.ts b/test/gstack-paths.test.ts index 42c13c3acd..628f599194 100644 --- a/test/gstack-paths.test.ts +++ b/test/gstack-paths.test.ts @@ -104,6 +104,57 @@ describe('gstack-paths', () => { expect(got).toHaveProperty('TMP_ROOT'); }); + // Regression: values must survive `eval "$(gstack-paths)"`, which is the + // documented calling convention. A bare `echo` emits an unquoted RHS, so eval + // re-parses it: backslashes become escapes and spaces become word separators. + // On Windows $TMP is always a backslash path, so every skill that then runs + // mktemp "$TMP_ROOT/..." fails and the bash block dies before doing any work. + // These run identically on POSIX — the values are just strings. + function evalRoundTrip(env: Record, varName: string): string { + const result = spawnSync( + 'bash', + ['-c', `eval "$(bash "$1")"; printf '%s' "\${${varName}}"`, 'sh', BIN], + { + env: { PATH: process.env.PATH, USERPROFILE: '', ...env } as Record, + encoding: 'utf-8', + }, + ); + if (result.status !== 0) { + throw new Error(`eval round-trip failed (status ${result.status}): ${result.stderr}`); + } + return result.stdout; + } + + // Values are POSIX-shaped on purpose: MSYS/Git Bash rewrites `C:\...` env + // values to `/c/...` before bash sees them, so a literal Windows path would + // assert the translation layer rather than the quoting. A backslash is a + // backslash to eval either way, which is the behavior under test. + test('eval round-trip preserves backslashes (#2374)', () => { + // Skip on Windows: MSYS also rewrites backslashes to forward slashes in + // env values, so a literal backslash cannot be injected through the + // environment on a Git Bash runner. The escape-eating this guards against + // is pure eval semantics, so exercising it on Linux/macOS CI is sufficient + // — same reasoning as the HOME-unset skips above. + if (process.platform === 'win32') return; + const backslashed = '/tmp/back\\slash/dir'; + expect(evalRoundTrip({ TMPDIR: backslashed, HOME: '/h' }, 'TMP_ROOT')).toBe(backslashed); + }); + + test('eval round-trip preserves spaces (#2374)', () => { + // Bare echo made eval word-split this, leaving the variable empty and + // emitting `: command not found`. + const spaced = '/tmp/two words/dir'; + expect(evalRoundTrip({ TMPDIR: spaced, HOME: '/h' }, 'TMP_ROOT')).toBe(spaced); + }); + + test('eval round-trip preserves quotes, and leaves plain paths alone (#2374)', () => { + expect(evalRoundTrip({ TMPDIR: "/tmp/o'brien", HOME: '/h' }, 'TMP_ROOT')).toBe("/tmp/o'brien"); + expect(evalRoundTrip({ GSTACK_HOME: '/tmp/state root' }, 'GSTACK_STATE_ROOT')).toBe( + '/tmp/state root', + ); + expect(evalRoundTrip({ HOME: '/tmp/myhome' }, 'PLAN_ROOT')).toBe('/tmp/myhome/.claude/plans'); + }); + test('output is shell-evalable: only KEY=VALUE lines, no extra prose', () => { const result = spawnSync('bash', [BIN], { env: { PATH: process.env.PATH, USERPROFILE: '', HOME: '/tmp/h' } as Record,