Skip to content

fix(gstack-paths): shell-quote output so eval is safe for paths with spaces - #1580

Closed
yannickspiess wants to merge 1 commit into
garrytan:mainfrom
yannickspiess:fix/gstack-paths-shell-quote-spaces
Closed

yannickspiess wants to merge 1 commit into
garrytan:mainfrom
yannickspiess:fix/gstack-paths-shell-quote-spaces

Conversation

@yannickspiess

Copy link
Copy Markdown

Problem

GSTACK_HOME (and the other path roots) may contain spaces or shell metacharacters — for example an iCloud-synced directory under Library/Mobile Documents/.

The current bare echo emits unquoted output:

GSTACK_STATE_ROOT=/Users/me/Library/Mobile Documents/Obsidian/GStack

When skills do eval "$(gstack-paths)", bash/zsh split on the first space:

  • GSTACK_STATE_ROOT gets set to /Users/me/Library/Mobile
  • The shell then tries to execute Documents/Obsidian/GStack as a command

Result: command not found and GSTACK_STATE_ROOT left truncated/wrong.

Fix

Add a POSIX-compatible _shell_quote helper that wraps each value in single quotes and properly escapes any embedded single quotes (the standard '\''\'' form). Replace the three echo lines with printf.

Output now looks like:

GSTACK_STATE_ROOT='/Users/me/Library/Mobile Documents/Obsidian/GStack'
PLAN_ROOT='/Users/me/.claude/plans'
TMP_ROOT='/var/folders/...'

Safe for paths containing: spaces, $, ;, backticks, double quotes, single quotes.

Works in sh / bash / zsh / dash (no printf %q which is bash/zsh-only).

Testing

Verified with:

  • Normal path (no spaces) — behaviour unchanged
  • Path with spaces — now works
  • Actual iCloud vault path (Library/Mobile Documents/iCloud~md~obsidian/...) — now works

Notes

The existing comment said "output values are not sanitized" — that warning is now removed since the output is sanitized. No other call sites need changing; the output contract (eval "$(gstack-paths)") is preserved.

…spaces

GSTACK_HOME (and the other path roots) may contain spaces or shell
metacharacters — for example an iCloud-synced directory under
`Library/Mobile Documents/`.  The previous bare echo emitted:

  GSTACK_STATE_ROOT=/path/with spaces

which eval split at the first space, leaving GSTACK_STATE_ROOT truncated
and attempting to execute the remainder as a command.

Fix: add a POSIX-compatible _shell_quote helper that wraps values in
single quotes and escapes any embedded single quotes. Works in
sh / bash / zsh / dash.

Tested with:
- normal path (no spaces)    — unchanged behaviour
- path with spaces           — now works
- path with single quote     — now works
- actual iCloud vault path   — now works

@NikhileshNanduri NikhileshNanduri left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: Correct fix, one edge case to verify

The core change is right — eval "$(gstack-paths)" with unquoted paths containing spaces, $, or backticks is a real injection risk, and single-quoting the output is the right mitigation.

The _shell_quote implementation is correct:

_shell_quote() {
  printf "'%s'" "$(printf '%s' "$1" | sed "s/'/'\\\\''/g")"
}

This properly escapes embedded single quotes using the '\'' technique (close-quote, escaped-quote, reopen-quote), which is POSIX-portable.

One concern: callers that don't use eval

The output format changes from:

GSTACK_STATE_ROOT=/some/path

to:

GSTACK_STATE_ROOT='/some/path'

Any caller that parses the raw output without eval — e.g. grep -o 'GSTACK_STATE_ROOT=.*' or source <(gstack-paths) where the shell strips quotes differently — may receive '/some/path' (with literal single quotes) as the value. Worth grepping callers in bin/ and skill templates to confirm none do raw string extraction on the output:

grep -r "gstack-paths" bin/ --include="*.sh" --include="gstack-*" | grep -v "eval"

If all callsites use eval "$(gstack-paths)", this is clean. If any do string parsing, they'll need updating alongside this PR.

Minor: The previous comment "callers may receive paths with shell-special characters" was accurate but left the problem to callers. This PR moves the safety guarantee into the binary itself, which is the right layering.

@yannickspiess

Copy link
Copy Markdown
Author

Thanks for the careful review, @NikhileshNanduri. Good catch on the raw-parse risk — went and verified it before flagging this ready.

Grepped every gstack-paths reference across the skill tree:

grep -rn "gstack-paths" --include="*.sh" --include="*.md" \
  --include="*.tmpl" --include="*.ts" --include="SKILL.md" \
  --include="gstack-*" .

Every actual callsite uses eval "$(...)". 35+ matches across plan-tune, learn, freeze, investigate, context-restore, codex, office-hours, guard, unfreeze, and context-save (both .tmpl and rendered SKILL.md). No grep -o, no source <(...), no string-extraction patterns anywhere.

The non-eval matches are all non-callers: the script's own header comments, CHANGELOG.md, AGENTS.md prose mentions, the test file, and one descriptive reference in codex/SKILL.md.

Agree with your framing on the layering — pushing the safety guarantee into the binary itself means future callers can't reintroduce the footgun by forgetting to quote at the callsite. Leaving the merge call to the maintainers.

@time-attack

Copy link
Copy Markdown
Contributor

@16francej because of what yannickspiess said

@capy-ai

capy-ai Bot commented Oct 3, 2026

Copy link
Copy Markdown
Contributor

Thanks for this, @yannickspiess. The fix is now on main: bin/gstack-paths emits shell-quoted values (printf %q), and skills now read single keys with --get (already on main as of v1.91.16.0). Closing as superseded; if you still see the problem on the latest gstack, please open an issue with ./setup --status output.

@capy-ai capy-ai Bot closed this Oct 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants