Harden loop-action command execution against unquoted shell expansion - #395
Conversation
inputs.command was spliced as raw text into the run: script before bash parsed it. For multi-line command: | blocks this silently broke sandbox isolation: only the first line reached loop-sandbox run -- ...; every later line executed as a bare top-level statement on the runner, bypassing the sandbox and circuit-breaker entirely. Write command to a temp script (passed via env:, never re-templated by the workflow YAML) and invoke it as a single quoted bash "$SCRIPT" call from both the sandbox and direct paths. Document safe usage and warn against embedding untrusted input in command in the README.
cobusgreyling
left a comment
cobusgreyling
left a comment
There was a problem hiding this comment.
Summary
This PR correctly fixes the real multi-line/inputs.command splicing bug: moving command through env: into a temp script and invoking a single bash "$LOOP_SCRIPT_PATH" keeps the whole payload inside sandbox/direct execution and avoids unquoted expansion in the outer run: script. The approach matches GitHub’s recommended pattern, and the README’s untrusted-input warning is accurate. The main remaining correctness gap is that the nested bash invocation does not use Actions’ default -eo pipefail, so multi-line scripts can continue after failures and even report success if the last line succeeds.
Issue counts by severity
- bugs: 1
- suggestions: 1
- nits: 1
Decision: request changes. The temp-script approach is the right fix, but please invoke scripts with bash -eo pipefail (both sandbox and direct paths) so multi-line command matches Actions fail-fast behavior before merge.
| run: ${{ inputs.command }} | ||
| env: | ||
| LOOP_SCRIPT_PATH: ${{ steps.write_command.outputs.script-path }} | ||
| run: bash "$LOOP_SCRIPT_PATH" |
There was a problem hiding this comment.
[bug] The direct path runs bash "$LOOP_SCRIPT_PATH" (and the sandbox path does the same via bash "$LOOP_SCRIPT_PATH" on lines 57/59). GitHub Actions’ default bash shell is effectively bash --noprofile --norc -eo pipefail {0}, but a nested bash /path/to/script starts a fresh shell without -e or pipefail. For the multi-line command: | support this PR advertises, that means (1) a failing intermediate line does not stop later lines, and (2) the step’s exit code is only the last command’s status — so false / echo done succeeds overall. That is a behavior footgun relative to a normal run: step and can hide agent failures when a later line succeeds.
Suggestion: Invoke the temp script with the same fail-fast options Actions uses, on both paths, e.g. bash -eo pipefail "$LOOP_SCRIPT_PATH" (or bash --noprofile --norc -eo pipefail "$LOOP_SCRIPT_PATH"). Apply that in the direct run: line and in both sandbox loop-sandbox run ... -- bash ... invocations.
| if [ "${{ inputs.sandbox-shell }}" == "true" ]; then | ||
| npx --yes @cobusgreyling/loop-sandbox run --shell -- ${{ inputs.command }} | ||
| if [ "$LOOP_SANDBOX_SHELL" == "true" ]; then | ||
| npx --yes @cobusgreyling/loop-sandbox run --shell -- bash "$LOOP_SCRIPT_PATH" |
There was a problem hiding this comment.
[suggestion] After this change, command is always executed as a bash script (bash <script>), so sandbox-shell: 'true' only adds an extra Node shell: true wrap around an already-shell-driven bash invocation. The flag is now largely redundant for loop-action callers (including the multi-line examples that previously needed --shell for shell syntax). Keeping it for compatibility is fine, but the dual branch suggests a semantic difference that no longer really exists for this action.
Suggestion: Either always call loop-sandbox run -- bash -eo pipefail "$LOOP_SCRIPT_PATH" (drop the branch), or keep the branch but document in the README inputs table that command is always run via bash <temp-script>, so sandbox-shell is optional/compatibility-only when using loop-action.
| run: | | ||
| script_path="$(mktemp "${RUNNER_TEMP:-/tmp}/loop-action-command.XXXXXX")" | ||
| printf '%s\n' "$LOOP_COMMAND" > "$script_path" | ||
| chmod +x "$script_path" |
There was a problem hiding this comment.
[nit] chmod +x "$script_path" is unnecessary because every execution path invokes the file as bash "$LOOP_SCRIPT_PATH" rather than executing it directly. Harmless, but it implies the script is meant to be exec’d on its own (which would also want a shebang).
Suggestion: Remove the chmod +x line, or if you prefer direct execution later, add a #!/usr/bin/env bash header when writing the file and exec "$script_path" instead.
Run the temporary command script with bash -eo pipefail on the sandbox and direct paths so an intermediate failure in a multi-line command stops execution immediately and the step returns non-zero, instead of continuing on to later lines. Drop the chmod +x on the script since it's always invoked as an explicit argument to bash and never executed directly. Document sandbox-shell as a loop-sandbox compatibility option rather than something required for shell syntax in command.
|
Implemented the requested changes in ff8378b:
All relevant local tests pass. |
cobusgreyling
left a comment
cobusgreyling
left a comment
There was a problem hiding this comment.
Re-review (post-fix commit ff8378b)
All three review items addressed:
- bug —
bash -eo pipefailon both sandbox and direct paths restores Actions fail-fast / pipeline semantics for multi-linecommand: |blocks - suggestion —
sandbox-shelldocumented as a loop-sandbox compatibility knob, not a gate for shell syntax - nit — unnecessary
chmod +xremoved
Temp-script via env: + single quoted path remains the right fix for the original unquoted multi-line splice / sandbox-bypass bug. README warning on untrusted input in command is correct and important.
Decision: approve. Ready to merge.
cobusgreyling
left a comment
cobusgreyling
left a comment
There was a problem hiding this comment.
Re-review (triage)
Thanks @amarjaleelbanbhan — follow-up commit ff8378b addresses the prior change requests:
- Scripts now run with
bash -eo pipefailon both sandbox and direct paths - Unnecessary
chmod +xremoved - README documents
sandbox-shellas a loop-sandbox compatibility knob and documents fail-fast / multi-line behavior
Decision: approve + merge.
Maintainer note (PR triage 2026-07-27)Approved — review items from the prior pass are fixed on Fork workflows were stuck in action_required after reopen (required for If #404 merges first, this PR will be closed as superseded with full attribution to @amarjaleelbanbhan. |
|
Merged — thank you @amarjaleelbanbhan. Important sandbox-isolation fix for multi-line |
2 participants
inputs.command was spliced as raw text into the run: script before bash parsed it. For multi-line command: | blocks this silently broke sandbox isolation: only the first line reached loop-sandbox run -- ...; every later line executed as a bare top-level statement on the runner, bypassing the sandbox and circuit-breaker entirely.
Write command to a temp script (passed via env:, never re-templated by the workflow YAML) and invoke it as a single quoted bash "$SCRIPT" call from both the sandbox and direct paths. Document safe usage and warn against embedding untrusted input in command in the README.
Summary
Changes
templates/pattern-template.md+ updatedregistry.yaml)Checklist (from CONTRIBUTING)
STATE.md*examples use.examplesuffixdocs/safety.mdnode tools/loop-audit/dist/cli.js .(or on the starter) and addressed findingsTesting / Dogfood
loop-auditpasses on affected starters or this repoScreenshots / Examples (if UI or command output)
This template enforces the high bar this reference is known for.