Skip to content

Commit 24be696

Browse files
vaindclaudeCopilot
authored
fix: complete script injection hardening across all actions (#152)
* fix: complete script injection hardening across all actions 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) <[email protected]> * Update updater/action.yml Co-authored-by: Copilot <[email protected]> * fix(updater): URL-encode existing-PR query params; add changelog entry 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. --------- Co-authored-by: Claude Opus 4.6 (1M context) <[email protected]> Co-authored-by: Copilot <[email protected]>
1 parent a940f77 commit 24be696

4 files changed

Lines changed: 75 additions & 45 deletions

File tree

‎CHANGELOG.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010

1111
### Fixes
1212

13+
- Complete script injection hardening across all actions: move remaining step outputs to env vars, validate Danger version against semver ([#152](https://github.com/getsentry/github-workflows/pull/152))
1314
- Updater - Trigger CI for new PRs without changelog updates ([#166](https://github.com/getsentry/github-workflows/pull/166))
1415
- Updater - Select the first branch when multiple branches point at `HEAD` ([#165](https://github.com/getsentry/github-workflows/pull/165))
1516

‎danger/action.yml‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -59,7 +59,13 @@ runs:
5959
env:
6060
GITHUB_TOKEN: ${{ inputs.api-token }}
6161
EXTRA_DANGERFILE_INPUT: ${{ inputs.extra-dangerfile }}
62+
DANGER_VERSION: ${{ steps.config.outputs.version }}
6263
run: |
64+
# Validate version looks like a semver tag (defense in depth)
65+
if ! [[ "$DANGER_VERSION" =~ ^[0-9]+\.[0-9]+\.[0-9]+$ ]]; then
66+
echo "::error::Invalid Danger version '$DANGER_VERSION'. Expected semver format (e.g., 13.0.4)."
67+
exit 1
68+
fi
6369
# Start a detached container with all necessary volumes and environment variables
6470
docker run -td --name danger \
6571
--entrypoint /bin/bash \
@@ -72,7 +78,7 @@ runs:
7278
-e "GITHUB_TOKEN" \
7379
-e DANGER_DISABLE_TRANSPILATION="true" \
7480
-e "EXTRA_DANGERFILE_INPUT" \
75-
ghcr.io/danger/danger-js:${{ steps.config.outputs.version }} \
81+
"ghcr.io/danger/danger-js:${DANGER_VERSION}" \
7682
-c "sleep infinity"
7783
7884
- name: Setup additional packages

‎sentry-cli/integration-test/action.yml‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -20,5 +20,5 @@ runs:
2020
ACTION_PATH: ${{ github.action_path }}
2121
TEST_PATH: ${{ inputs.path }}
2222
run: |
23-
Import-Module -Name "$env:ACTION_PATH/action.psm1" -Force
24-
Invoke-Pester -Output Detailed "$env:TEST_PATH"
23+
Import-Module -Name ($env:ACTION_PATH + '/action.psm1') -Force
24+
Invoke-Pester -Output Detailed $env:TEST_PATH

‎updater/action.yml‎

Lines changed: 65 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -77,47 +77,47 @@ runs:
7777
DEPENDENCY_NAME: ${{ inputs.name }}
7878
run: |
7979
# Validate that inputs.name contains only safe characters
80-
if ("$env:DEPENDENCY_NAME" -notmatch '^[a-zA-Z0-9_\./@\s-]+$') {
81-
Write-Output "::error::Invalid dependency name: '$env:DEPENDENCY_NAME'. Only alphanumeric characters, spaces, and _-./@ are allowed."
80+
if ($env:DEPENDENCY_NAME -notmatch '^[a-zA-Z0-9_\./@\s-]+$') {
81+
Write-Output ('::error::Invalid dependency name: "' + $env:DEPENDENCY_NAME + '". Only alphanumeric characters, spaces, and _-./@ are allowed.')
8282
exit 1
8383
}
84-
Write-Output "✓ Dependency name '$env:DEPENDENCY_NAME' is valid"
84+
Write-Output ('Dependency name "' + $env:DEPENDENCY_NAME + '" is valid')
8585
8686
- name: Validate dependency path
8787
shell: pwsh
8888
env:
8989
DEPENDENCY_PATH: ${{ inputs.path }}
9090
run: |
9191
# Validate that inputs.path contains only safe characters (including # for CMake dependencies)
92-
if ("$env:DEPENDENCY_PATH" -notmatch '^[a-zA-Z0-9_\./#-]+$') {
93-
Write-Output "::error::Invalid dependency path: '$env:DEPENDENCY_PATH'. Only alphanumeric characters and _-./# are allowed."
92+
if ($env:DEPENDENCY_PATH -notmatch '^[a-zA-Z0-9_\./#-]+$') {
93+
Write-Output ('::error::Invalid dependency path: "' + $env:DEPENDENCY_PATH + '". Only alphanumeric characters and _-./# are allowed.')
9494
exit 1
9595
}
96-
Write-Output "✓ Dependency path '$env:DEPENDENCY_PATH' is valid"
96+
Write-Output ('Dependency path "' + $env:DEPENDENCY_PATH + '" is valid')
9797
9898
- name: Validate changelog-entry
9999
shell: pwsh
100100
env:
101101
CHANGELOG_ENTRY: ${{ inputs.changelog-entry }}
102102
run: |
103103
# Validate that inputs.changelog-entry is either 'true' or 'false'
104-
if ("$env:CHANGELOG_ENTRY" -notin @('true', 'false')) {
105-
Write-Output "::error::Invalid changelog-entry value: '$env:CHANGELOG_ENTRY'. Only 'true' or 'false' are allowed."
104+
if ($env:CHANGELOG_ENTRY -notin @('true', 'false')) {
105+
Write-Output ('::error::Invalid changelog-entry value: "' + $env:CHANGELOG_ENTRY + '". Only "true" or "false" are allowed.')
106106
exit 1
107107
}
108-
Write-Output "✓ Changelog-entry value '$env:CHANGELOG_ENTRY' is valid"
108+
Write-Output ('Changelog-entry value "' + $env:CHANGELOG_ENTRY + '" is valid')
109109
110110
- name: Validate pr-strategy
111111
shell: pwsh
112112
env:
113113
PR_STRATEGY: ${{ inputs.pr-strategy }}
114114
run: |
115115
# Validate that inputs.pr-strategy is either 'create' or 'update'
116-
if ("$env:PR_STRATEGY" -notin @('create', 'update')) {
117-
Write-Output "::error::Invalid pr-strategy value: '$env:PR_STRATEGY'. Only 'create' or 'update' are allowed."
116+
if ($env:PR_STRATEGY -notin @('create', 'update')) {
117+
Write-Output ('::error::Invalid pr-strategy value: "' + $env:PR_STRATEGY + '". Only "create" or "update" are allowed.')
118118
exit 1
119119
}
120-
Write-Output "✓ PR strategy value '$env:PR_STRATEGY' is valid"
120+
Write-Output ('PR strategy value "' + $env:PR_STRATEGY + '" is valid')
121121
122122
- name: Validate post-update-script
123123
if: ${{ inputs.post-update-script != '' }}
@@ -126,11 +126,11 @@ runs:
126126
POST_UPDATE_SCRIPT: ${{ inputs.post-update-script }}
127127
run: |
128128
# Validate that inputs.post-update-script contains only safe characters
129-
if ("$env:POST_UPDATE_SCRIPT" -notmatch '^[a-zA-Z0-9_\./#\s-]+$') {
130-
Write-Output "::error::Invalid post-update-script path: '$env:POST_UPDATE_SCRIPT'. Only alphanumeric characters, spaces, and _-./# are allowed."
129+
if ($env:POST_UPDATE_SCRIPT -notmatch '^[a-zA-Z0-9_\./#\s-]+$') {
130+
Write-Output ('::error::Invalid post-update-script path: "' + $env:POST_UPDATE_SCRIPT + '". Only alphanumeric characters, spaces, and _-./# are allowed.')
131131
exit 1
132132
}
133-
Write-Output "✓ Post-update script path '$env:POST_UPDATE_SCRIPT' is valid"
133+
Write-Output ('Post-update script path "' + $env:POST_UPDATE_SCRIPT + '" is valid')
134134
135135
- name: Validate authentication inputs
136136
shell: pwsh
@@ -288,30 +288,31 @@ runs:
288288
PR_STRATEGY: ${{ inputs.pr-strategy }}
289289
DEPENDENCY_PATH: ${{ inputs.path }}
290290
TARGET_BRANCH: ${{ inputs.target-branch }}
291+
LATEST_TAG: ${{ steps.target.outputs.latestTag }}
291292
run: |
292293
if ([string]::IsNullOrEmpty($env:TARGET_BRANCH)) {
293294
$mainBranch = $(git remote show origin | Select-String "HEAD branch: (.*)").Matches[0].Groups[1].Value
294295
$prBranchPrefix = ''
295296
} else {
296297
$mainBranch = $env:TARGET_BRANCH
297-
$prBranchPrefix = "$mainBranch-"
298+
$prBranchPrefix = $mainBranch + '-'
298299
}
299300
$prBranch = switch ($env:PR_STRATEGY)
300301
{
301-
'create' { "deps/$env:DEPENDENCY_PATH/${{ steps.target.outputs.latestTag }}" }
302-
'update' { "deps/$env:DEPENDENCY_PATH" }
303-
default { throw "Unkown PR strategy '$env:PR_STRATEGY'." }
302+
'create' { 'deps/' + $env:DEPENDENCY_PATH + '/' + $env:LATEST_TAG }
303+
'update' { 'deps/' + $env:DEPENDENCY_PATH }
304+
default { throw ('Unknown PR strategy "' + $env:PR_STRATEGY + '".') }
304305
}
305306
$prBranch = $prBranchPrefix + $prBranch
306-
"baseBranch=$mainBranch" | Tee-Object $env:GITHUB_OUTPUT -Append
307-
"prBranch=$prBranch" | Tee-Object $env:GITHUB_OUTPUT -Append
307+
('baseBranch=' + $mainBranch) | Tee-Object $env:GITHUB_OUTPUT -Append
308+
('prBranch=' + $prBranch) | Tee-Object $env:GITHUB_OUTPUT -Append
308309
$nonBotCommits = ${{ github.action_path }}/scripts/nonbot-commits.ps1 `
309-
-RepoUrl "$(git config --get remote.origin.url)" -PrBranch $prBranch -MainBranch $mainBranch
310+
-RepoUrl $(git config --get remote.origin.url) -PrBranch $prBranch -MainBranch $mainBranch
310311
$changed = $nonBotCommits.Length -gt 0 ? 'true' : 'false'
311-
"changed=$changed" | Tee-Object $env:GITHUB_OUTPUT -Append
312-
if ("$changed" -eq "true")
312+
('changed=' + $changed) | Tee-Object $env:GITHUB_OUTPUT -Append
313+
if ($changed -eq 'true')
313314
{
314-
Write-Output "::warning::Target branch '$prBranch' has been changed manually - skipping updater to avoid overwriting these changes."
315+
Write-Output ('::warning::Target branch "' + $prBranch + '" has been changed manually - skipping updater to avoid overwriting these changes.')
315316
}
316317
317318
- name: Parse the existing PR URL
@@ -321,19 +322,28 @@ runs:
321322
working-directory: caller-repo
322323
env:
323324
GH_TOKEN: ${{ inputs.api-token || github.token }}
325+
BASE_BRANCH: ${{ steps.root.outputs.baseBranch }}
326+
PR_BRANCH: ${{ steps.root.outputs.prBranch }}
324327
run: |
325-
$urls = @(gh api 'repos/${{ github.repository }}/pulls?base=${{ steps.root.outputs.baseBranch }}&head=${{ github.repository_owner }}:${{ steps.root.outputs.prBranch }}' --jq '.[].html_url')
328+
# Use -f to let gh URL-encode query params; PR branch can contain '#' from CMake dependency paths.
329+
$head = '${{ github.repository_owner }}:' + $env:PR_BRANCH
330+
$base = $env:BASE_BRANCH
331+
$urls = @(gh api 'repos/${{ github.repository }}/pulls' `
332+
-X GET `
333+
-f ('base=' + $base) `
334+
-f ('head=' + $head) `
335+
--jq '.[].html_url')
326336
if ($urls.Length -eq 0)
327337
{
328-
"url=" | Tee-Object $env:GITHUB_OUTPUT -Append
338+
'url=' | Tee-Object $env:GITHUB_OUTPUT -Append
329339
}
330340
elseif ($urls.Length -eq 1)
331341
{
332-
"url=$($urls[0])" | Tee-Object $env:GITHUB_OUTPUT -Append
342+
('url=' + $urls[0]) | Tee-Object $env:GITHUB_OUTPUT -Append
333343
}
334344
else
335345
{
336-
throw "Unexpected number of PRs matched ($($urls.Length)): $urls"
346+
throw ('Unexpected number of PRs matched (' + $urls.Length + '): ' + $urls)
337347
}
338348
339349
- name: Show git diff
@@ -348,11 +358,14 @@ runs:
348358
working-directory: caller-repo
349359
env:
350360
GH_TOKEN: ${{ inputs.api-token || github.token }}
361+
TARGET_REPO_URL: ${{ steps.target.outputs.url }}
362+
ORIGINAL_TAG: ${{ steps.target.outputs.originalTag }}
363+
LATEST_TAG: ${{ steps.target.outputs.latestTag }}
351364
run: |
352365
$changelog = ${{ github.action_path }}/scripts/get-changelog.ps1 `
353-
-RepoUrl '${{ steps.target.outputs.url }}' `
354-
-OldTag '${{ steps.target.outputs.originalTag }}' `
355-
-NewTag '${{ steps.target.outputs.latestTag }}'
366+
-RepoUrl $env:TARGET_REPO_URL `
367+
-OldTag $env:ORIGINAL_TAG `
368+
-NewTag $env:LATEST_TAG
356369
${{ github.action_path }}/scripts/set-github-env.ps1 TARGET_CHANGELOG $changelog
357370
358371
# First we create a PR only if it doesn't exist. We will later overwrite the content with the same action.
@@ -382,14 +395,17 @@ runs:
382395
id: pr
383396
shell: pwsh
384397
working-directory: caller-repo
398+
env:
399+
CREATED_PR_URL: ${{ steps.create-pr.outputs.pull-request-url }}
400+
EXISTING_PR_URL: ${{ steps.existing-pr.outputs.url }}
385401
run: |
386-
if ('${{ steps.create-pr.outputs.pull-request-url }}' -ne '')
402+
if (-not [string]::IsNullOrEmpty($env:CREATED_PR_URL))
387403
{
388-
"url=${{ steps.create-pr.outputs.pull-request-url }}" | Tee-Object $env:GITHUB_OUTPUT -Append
404+
("url=" + $env:CREATED_PR_URL) | Tee-Object $env:GITHUB_OUTPUT -Append
389405
}
390-
elseif ('${{ steps.existing-pr.outputs.url }}' -ne '')
406+
elseif (-not [string]::IsNullOrEmpty($env:EXISTING_PR_URL))
391407
{
392-
"url=${{ steps.existing-pr.outputs.url }}" | Tee-Object $env:GITHUB_OUTPUT -Append
408+
("url=" + $env:EXISTING_PR_URL) | Tee-Object $env:GITHUB_OUTPUT -Append
393409
}
394410
else
395411
{
@@ -415,7 +431,9 @@ runs:
415431
DEPENDENCY_PATH: ${{ inputs.path }}
416432
POST_UPDATE_SCRIPT: ${{ inputs.post-update-script }}
417433
GH_TOKEN: ${{ inputs.api-token || github.token }}
418-
run: ${{ github.action_path }}/scripts/update-dependency.ps1 -Path $env:DEPENDENCY_PATH -Tag '${{ steps.target.outputs.latestTag }}' -OriginalTag '${{ steps.target.outputs.originalTag }}' -PostUpdateScript $env:POST_UPDATE_SCRIPT
434+
LATEST_TAG: ${{ steps.target.outputs.latestTag }}
435+
ORIGINAL_TAG: ${{ steps.target.outputs.originalTag }}
436+
run: ${{ github.action_path }}/scripts/update-dependency.ps1 -Path $env:DEPENDENCY_PATH -Tag $env:LATEST_TAG -OriginalTag $env:ORIGINAL_TAG -PostUpdateScript $env:POST_UPDATE_SCRIPT
419437

420438
- name: Update Changelog
421439
if: ${{ inputs.changelog-entry == 'true' && ( steps.target.outputs.latestTag != steps.target.outputs.originalTag ) && ( steps.root.outputs.changed == 'false') }}
@@ -425,14 +443,19 @@ runs:
425443
DEPENDENCY_NAME: ${{ inputs.name }}
426444
CHANGELOG_SECTION: ${{ inputs.changelog-section }}
427445
GH_TOKEN: ${{ inputs.api-token || github.token }}
446+
PR_URL: ${{ steps.pr.outputs.url }}
447+
TARGET_REPO_URL: ${{ steps.target.outputs.url }}
448+
TARGET_MAIN_BRANCH: ${{ steps.target.outputs.mainBranch }}
449+
ORIGINAL_TAG: ${{ steps.target.outputs.originalTag }}
450+
LATEST_TAG: ${{ steps.target.outputs.latestTag }}
428451
run: |
429452
${{ github.action_path }}/scripts/update-changelog.ps1 `
430453
-Name $env:DEPENDENCY_NAME `
431-
-PR '${{ steps.pr.outputs.url }}' `
432-
-RepoUrl '${{ steps.target.outputs.url }}' `
433-
-MainBranch '${{ steps.target.outputs.mainBranch }}' `
434-
-OldTag '${{ steps.target.outputs.originalTag }}' `
435-
-NewTag '${{ steps.target.outputs.latestTag }}' `
454+
-PR $env:PR_URL `
455+
-RepoUrl $env:TARGET_REPO_URL `
456+
-MainBranch $env:TARGET_MAIN_BRANCH `
457+
-OldTag $env:ORIGINAL_TAG `
458+
-NewTag $env:LATEST_TAG `
436459
-Section $env:CHANGELOG_SECTION
437460
438461
- name: Show final git diff

0 commit comments

Comments
 (0)