fix: complete script injection hardening across all actions#152
Conversation
PR #150 moved user inputs to env vars but left step outputs (`steps.*.outputs.*`) directly interpolated in `run:` blocks — an attacker controlling e.g. git tags in a dependency repo could still inject arbitrary commands. Additionally, switch all PowerShell run blocks from double-quote string interpolation (`"$env:VAR"`) to string concatenation (`'prefix' + $env:VAR`) to eliminate any possibility of subexpression evaluation. Changes: - updater/action.yml: move all remaining step outputs (tags, URLs, branch names) to env vars; replace double-quote interpolation with concatenation throughout - sentry-cli/integration-test/action.yml: same concatenation fix - danger/action.yml: move docker image version from direct interpolation to env var with semver validation Refs: VULN-1100 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Hardens composite GitHub Actions against script injection by removing direct interpolation of step outputs into run: blocks and by avoiding PowerShell double-quoted interpolation patterns.
Changes:
updater/action.yml: Move step outputs intoenv:and switch to PowerShell string concatenation for output/log construction.sentry-cli/integration-test/action.yml: Replace PowerShell double-quoted interpolation with concatenation.danger/action.yml: Pass Docker image version viaenvwith a semver format validation before use.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
updater/action.yml |
Reworks multiple pwsh steps to avoid ${{ steps.* }} in run: and removes double-quoted interpolation in favor of concatenation. |
sentry-cli/integration-test/action.yml |
Updates module import and Pester invocation to avoid double-quoted interpolation. |
danger/action.yml |
Uses DANGER_VERSION env var for the Docker image tag and validates it as semver to prevent injection. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
PR branches derived from CMake dependency paths can contain '#', which the previous query-string concatenation would treat as a URL fragment delimiter and truncate. Switch to `gh api -X GET -f` so gh URL-encodes the values, ensuring existing PRs are still matched when the branch name contains special characters. Also add the changelog entry for this PR so the advisory danger check passes.
| # Validate that inputs.name contains only safe characters | ||
| if ("$env:DEPENDENCY_NAME" -notmatch '^[a-zA-Z0-9_\./@\s-]+$') { | ||
| Write-Output "::error::Invalid dependency name: '$env:DEPENDENCY_NAME'. Only alphanumeric characters, spaces, and _-./@ are allowed." | ||
| if ($env:DEPENDENCY_NAME -notmatch '^[a-zA-Z0-9_\./@\s-]+$') { |
There was a problem hiding this comment.
Out of scope for this PR — regex was added in #150, and \s here does not create an injection vector since the value is consumed via env vars / script parameters, not shell interpolation. The message/regex mismatch is worth a follow-up.
| # Validate that inputs.post-update-script contains only safe characters | ||
| if ("$env:POST_UPDATE_SCRIPT" -notmatch '^[a-zA-Z0-9_\./#\s-]+$') { | ||
| Write-Output "::error::Invalid post-update-script path: '$env:POST_UPDATE_SCRIPT'. Only alphanumeric characters, spaces, and _-./# are allowed." | ||
| if ($env:POST_UPDATE_SCRIPT -notmatch '^[a-zA-Z0-9_\./#\s-]+$') { |
There was a problem hiding this comment.
Same as the DEPENDENCY_NAME case above — pre-existing from #150, not a security concern for this PR (value flows through env vars to PowerShell, no shell interpolation). Candidate for a small follow-up.
| else | ||
| { | ||
| throw "Unexpected number of PRs matched ($($urls.Length)): $urls" | ||
| throw ('Unexpected number of PRs matched (' + $urls.Length + '): ' + $urls) |
There was a problem hiding this comment.
False positive — PowerShell coerces array → string via $OFS (default space). "msg: " + @("a","b") yields msg: a b, not System.Object[]. Verified locally. This is the second time this exact comment has been raised on the PR.
Summary
Follow-up to #150 which moved user inputs to env vars but left step outputs (
steps.*.outputs.*) directly interpolated inrun:blocks. An attacker controlling e.g. git tags in a dependency repo could still inject arbitrary commands.env:blocks; replace PowerShell double-quote string interpolation with concatenation throughout to eliminate any possibility of subexpression evaluation${{ }}interpolation to env var with semver validationAfter this change, the only
${{ }}expressions remaining inrun:blocks are GitHub-controlled values (github.action_path,github.repository,github.repository_owner).Refs: VULN-1100
Test plan
🤖 Generated with Claude Code