security(skillgen): stop substituting INPUT_PATH into an interpreted shell command in Step 1 - #3742
Closed
ayushcodes10 wants to merge 4 commits into
Closed
ayushcodes10 wants to merge 4 commits into
ayushcodes10 wants to merge 4 commits into
Conversation
Step 1's own bash and PowerShell fragments both built a command whose literal text was the resolved scan path, substituted verbatim by the agent following the instructions before that line ever ran. The bash form, echo dollar paren cd INPUT_PATH and pwd close paren, still expands a command substitution even inside the outer double quotes, and the cd itself was entirely unquoted; the PowerShell form passed INPUT_PATH as a bare unquoted argument to Resolve Path, exposed to the same class of problem through a stray semicolon or pipe. A path drawn from an untrusted source, a malicious readme's suggested command, a copied value from an untrusted page, executed as code the moment this line ran, before any Python was ever reached. Both now capture INPUT_PATH through a construct their own interpreter guarantees performs no expansion at all: a quoted bash heredoc for the posix form, a single quoted PowerShell here string for the windows form. The raw text reaches Python (respectively Resolve Path) only as inert data on standard input, never as source the shell itself parses. Verified for the posix form by actually executing the extracted Step 1 block with a malicious substituted value and confirming no injected command runs; the PowerShell form could not be executed in this environment (no interpreter available to install without an interactive credential prompt), so it was written using well established, unambiguous here string syntax and reviewed carefully rather than run. As a side effect of no longer building the path through an unquoted cd, a legitimate path containing a space is now handled correctly too, and a nonexistent path now fails the whole step loudly instead of silently continuing past a swallowed cd error. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017qfdzgbA5KedGEjD1AayNh
Ran the generator, then the bless command, with no manual edits. Confirmed clean with the check, audit coverage, monolith roundtrip, and schema singleton verification commands. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017qfdzgbA5KedGEjD1AayNh
Extracts the real Step 1 bash block from the committed skill.md artifact, substitutes INPUT_PATH the way an agent following the instructions would, and actually runs it, so the fix is checked against what users receive rather than the fragment source alone. One test confirms a command substitution embedded in the substituted path never runs, another confirms a semicolon separated command never runs, a third confirms a legitimate path with a space in it still resolves correctly, and a fourth confirms a nonexistent path still fails the whole step loudly. All four were checked against the pre fix block first, three failed as expected. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017qfdzgbA5KedGEjD1AayNh
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017qfdzgbA5KedGEjD1AayNh
safishamsi
added a commit
that referenced
this pull request
Sep 25, 2026
…ch reference Completes #3742: the Step 1 fix closed the .graphify_root shell line, but the progressive-disclosure add-watch reference still passed the raw, agent-substituted INPUT_PATH placeholder unquoted into a bash command ($(cat .graphify_python) -m graphify.watch INPUT_PATH), the same injection class - a scan root like $(...) / ; rm ... would execute. The watcher targets the folder graphify already extracted, so read the trusted graphify-out/.graphify_root that Step 1 resolves (the same sidecar pattern this reference already uses for .graphify_python) instead of re-substituting the untrusted placeholder. Regenerated all 14 host add-watch artifacts + expected/ snapshots; monolith-roundtrip and always-on-roundtrip stay green (cores untouched). The identical sink in the aider/devin monolith cores is tracked separately (editing a core requires a gen.py sanctioned-diff). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
safishamsi
added a commit
that referenced
this pull request
Sep 25, 2026
Complete the skillgen --watch INPUT_PATH injection fix in the add-watch reference (#3742), preserve cross-file edges on incremental update (#3812), stop C# type references resolving to same-named non-type nodes (#3815), prefer a real source location over richness in dedup (#3786), and resolve Maven inherited groupId + ${...} properties in pom.xml (#3823). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Collaborator
|
Shipped in v0.9.68 (now on PyPI) via an authorship-preserving cherry-pick, so your commit keeps contributor-graph credit. Thanks @ayushcodes10! Your Step 1 fix (quoted heredoc / literal here-string) landed as-is and the monolith-roundtrip stays clean. On top of it I closed the same injection class in the progressive-disclosure Release: https://github.com/Graphify-Labs/graphify/releases/tag/v0.9.68 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #3642.
Every generated skill's Step 1 substitutes
INPUT_PATH(the user-supplied corpus path) into an interpreted command line:echo "$(cd INPUT_PATH && pwd)" > graphify-out/.graphify_rootfor bash,(Resolve-Path INPUT_PATH).Pathfor PowerShell.INPUT_PATHis a placeholder the agent substitutes with free text before running the block — the outer double quotes in the bash form do not stop$()/backtick expansion, andcd INPUT_PATHitself is entirely unquoted. A path drawn from an untrusted source (a malicious README's suggested command, a copy-pasted value from an untrusted page) executes as shell code the moment Step 1 runs, before any Python is reached. I filed this issue myself in a previous session and deliberately did not attempt an inline patch at the time, given the scope (touches the fragment rendered into every non-monolith platform, plus the PowerShell equivalent).Fix: capture
INPUT_PATHthrough a construct each interpreter guarantees performs no expansion at all — a quoted bash heredoc (<<'EOF'), a single-quoted PowerShell here-string (@'...'@) — piping the raw text to a small Python one-liner (bash) orResolve-Path(PowerShell) as inert data, never re-parsed as script source.I verified empirically (actual subprocess execution) that an unquoted heredoc still allows
$()expansion in its body — only the quoted form fully neutralizes this — and confirmed the fix closes the hole while a legitimate path (including one with a space) still resolves correctly and a nonexistent path still fails loudly.tests/test_skillgen_input_path_injection.pyextracts the real Step 1 block from the committedskill.mdand actually executes it with hostile substituted values (a$()payload, a;-chained payload), so the fix is checked against what users actually receive.One caveat, disclosed rather than silently glossed over: I could not verify the PowerShell fragment via actual execution — no
pwsh/powershellwas available in my environment, and installing it required an interactivesudopassword I couldn't provide non-interactively. That fix was written using well-established, unambiguous PowerShell here-string syntax and reviewed carefully, but it has not been run. Flagging this explicitly in case a reviewer with Windows/PowerShell access can double check it before merge.All 14 derived
skill-*.mdartifacts regenerated viatools.skillgen+ bless, verified clean against check/audit-coverage/monolith-roundtrip/schema-singleton. Full suite (5718 passed) is green.🤖 Generated with Claude Code
https://claude.ai/code/session_017qfdzgbA5KedGEjD1AayNh