fix: sanitize expression injection in workflow_dispatch inputs#74
fix: sanitize expression injection in workflow_dispatch inputs#74brandon-fox wants to merge 2 commits intomainfrom
Conversation
Replace direct ${{ inputs.feature_dir }} interpolation in shell run
blocks with environment variable references to prevent script injection.
Affects: spec-analyze-gate, speckit-pipeline, speckit-clarify-on-merge
workflows. spec-to-issue-sync is addressed in a separate PR.
Ref: https://securitylab.github.com/resources/github-actions-preventing-pwn-requests/
|
Note Gemini is unable to generate a summary for this pull request due to the file types involved not being currently supported. |
There was a problem hiding this comment.
1 issue found across 3 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name=".github/workflows/speckit-pipeline.yml">
<violation number="1" location=".github/workflows/speckit-pipeline.yml:59">
P1: Incomplete injection fix: `${{ steps.detect.outputs.feature_dir }}` is still expression-interpolated in downstream `run:` blocks (Run Clarify, Run Plan, Run Tasks). The user-controlled value flows from `inputs.feature_dir` → `FEATURE_INPUT` → `FEATURE_DIR` → step output → expression injection again. Each downstream step should also pass `steps.detect.outputs.feature_dir` through an `env:` block rather than inlining it in `run:`.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| - name: Detect changed spec | ||
| id: detect | ||
| env: | ||
| FEATURE_INPUT: ${{ inputs.feature_dir }} |
There was a problem hiding this comment.
P1: Incomplete injection fix: ${{ steps.detect.outputs.feature_dir }} is still expression-interpolated in downstream run: blocks (Run Clarify, Run Plan, Run Tasks). The user-controlled value flows from inputs.feature_dir → FEATURE_INPUT → FEATURE_DIR → step output → expression injection again. Each downstream step should also pass steps.detect.outputs.feature_dir through an env: block rather than inlining it in run:.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .github/workflows/speckit-pipeline.yml, line 59:
<comment>Incomplete injection fix: `${{ steps.detect.outputs.feature_dir }}` is still expression-interpolated in downstream `run:` blocks (Run Clarify, Run Plan, Run Tasks). The user-controlled value flows from `inputs.feature_dir` → `FEATURE_INPUT` → `FEATURE_DIR` → step output → expression injection again. Each downstream step should also pass `steps.detect.outputs.feature_dir` through an `env:` block rather than inlining it in `run:`.</comment>
<file context>
@@ -55,9 +55,11 @@ jobs:
- name: Detect changed spec
id: detect
+ env:
+ FEATURE_INPUT: ${{ inputs.feature_dir }}
run: |
- if [ -n "${{ inputs.feature_dir }}" ]; then
</file context>
|
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name=".github/workflows/speckit-pipeline.yml">
<violation number="1" location=".github/workflows/speckit-pipeline.yml:78">
P0: Expression injection is not fully mitigated. `${{ steps.detect.outputs.feature_dir }}` in `run:` blocks is still expanded before the shell runs, and it carries the original user-controlled `inputs.feature_dir` value via the step output. This re-introduces the same injection the PR intends to fix.
Each of the three run steps (Clarify, Plan, Tasks) should pass the step output through `env:` and reference a shell variable instead. For example, add `FEATURE_DIR: ${{ steps.detect.outputs.feature_dir }}` to each step's `env:` block and replace the expression with `$FEATURE_DIR` in the shell script.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| if: steps.detect.outputs.skip == 'false' | ||
| run: | | ||
| echo "🔍 Running clarify for ${{ steps.detect.outputs.feature_dir }}..." >> $GITHUB_STEP_SUMMARY | ||
| echo "Running clarify for ${{ steps.detect.outputs.feature_dir }}..." >> $GITHUB_STEP_SUMMARY |
There was a problem hiding this comment.
P0: Expression injection is not fully mitigated. ${{ steps.detect.outputs.feature_dir }} in run: blocks is still expanded before the shell runs, and it carries the original user-controlled inputs.feature_dir value via the step output. This re-introduces the same injection the PR intends to fix.
Each of the three run steps (Clarify, Plan, Tasks) should pass the step output through env: and reference a shell variable instead. For example, add FEATURE_DIR: ${{ steps.detect.outputs.feature_dir }} to each step's env: block and replace the expression with $FEATURE_DIR in the shell script.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .github/workflows/speckit-pipeline.yml, line 78:
<comment>Expression injection is not fully mitigated. `${{ steps.detect.outputs.feature_dir }}` in `run:` blocks is still expanded before the shell runs, and it carries the original user-controlled `inputs.feature_dir` value via the step output. This re-introduces the same injection the PR intends to fix.
Each of the three run steps (Clarify, Plan, Tasks) should pass the step output through `env:` and reference a shell variable instead. For example, add `FEATURE_DIR: ${{ steps.detect.outputs.feature_dir }}` to each step's `env:` block and replace the expression with `$FEATURE_DIR` in the shell script.</comment>
<file context>
@@ -69,18 +69,18 @@ jobs:
if: steps.detect.outputs.skip == 'false'
run: |
- echo "🔍 Running clarify for ${{ steps.detect.outputs.feature_dir }}..." >> $GITHUB_STEP_SUMMARY
+ echo "Running clarify for ${{ steps.detect.outputs.feature_dir }}..." >> $GITHUB_STEP_SUMMARY
if [ -f "scripts/speckit_clarify.py" ]; then
uv run python scripts/speckit_clarify.py --feature "${{ steps.detect.outputs.feature_dir }}" 2>&1 | tail -20 || true
</file context>
\ud83d\udd12 Security: Expression Injection Sanitization\n\nIssue: Direct
${{ inputs.feature_dir }}interpolation in shellrun:blocks creates a script injection vulnerability. A maliciousworkflow_dispatchinput could execute arbitrary shell commands.\n\nFix: All 3 affected workflows now passinputs.feature_dirthrough anenv:block as$FEATURE_INPUTand reference it as a shell variable instead of a GitHub expression.\n\n### Files Changed\n\n| Workflow | Change |\n|---|---|\n|spec-analyze-gate.yml|${{ inputs.feature_dir }}→$FEATURE_INPUTviaenv:|\n|speckit-pipeline.yml|${{ inputs.feature_dir }}→$FEATURE_INPUTviaenv:|\n|speckit-clarify-on-merge.yml|${{ inputs.feature_dir }}→$FEATURE_INPUTviaenv:|\n\n>spec-to-issue-sync.ymlis addressed in PR #73.\n\n### Review Checklist\n- [x] No${{ inputs.* }}in anyrun:block\n- [x] All inputs flow throughenv:→ shell variable\n- [x] Functional behavior unchanged\n- [x] Run Clarify step preserved in pipeline