-
-
Notifications
You must be signed in to change notification settings - Fork 8
fix(updater): Prevent script injection vulnerabilities #98
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
2fa8e16
fix(updater): Prevent script injection vulnerabilities
vaind 3841d24
refactor: Split input validation into separate steps
vaind 71bc194
fix: Use [:space:] character class for spaces in regex
vaind 4526fc8
fix: Correct regex character class syntax for hyphens
vaind 15af118
refactor: Use PowerShell for input validation steps
vaind fff8b7b
docs: Add changelog entry for script injection security fix
vaind 9af03f8
Apply suggestion from @jpnurmi
vaind 7bb1cd7
Apply suggestion from @vaind
vaind b67288f
Apply suggestion from @vaind
vaind 2feccc3
Apply suggestion from @vaind
vaind 97fbb71
Apply suggestion from @vaind
vaind File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -72,187 +72,216 @@ | |
| with: | ||
| access_token: ${{ github.token }} | ||
|
|
||
| validate-inputs: | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - name: Validate dependency name | ||
| shell: pwsh | ||
| run: | | ||
| # Validate that inputs.name contains only safe characters | ||
| if ('${{ inputs.name }}' -notmatch '^[a-zA-Z0-9_\./@\s-]+$') { | ||
| Write-Output "::error::Invalid dependency name: '${{ inputs.name }}'. Only alphanumeric characters, spaces, and _-./@ are allowed." | ||
| exit 1 | ||
| } | ||
| Write-Output "✓ Dependency name '${{ inputs.name }}' is valid" | ||
| - name: Validate dependency path | ||
| shell: pwsh | ||
| run: | | ||
| # Validate that inputs.path contains only safe characters | ||
| if ('${{ inputs.path }}' -notmatch '^[a-zA-Z0-9_\./-]+$') { | ||
| Write-Output "::error::Invalid dependency path: '${{ inputs.path }}'. Only alphanumeric characters and _-./ are allowed." | ||
| exit 1 | ||
| } | ||
| Write-Output "✓ Dependency path '${{ inputs.path }}' is valid" | ||
|
|
||
| # What we need to accomplish: | ||
| # * update to the latest tag | ||
| # * create a PR | ||
| # * update changelog (including the link to the just created PR) | ||
| # | ||
| # What we actually do is based on whether a PR exists already: | ||
| # * YES it does: | ||
| # * make the update | ||
| # * update changelog (with the ID of an existing PR) | ||
| # * push to the PR | ||
| # * NO it doesn't: | ||
| # * make the update | ||
| # * push to a new PR | ||
| # * update changelog (with the ID of the just created PR) | ||
| # * push to the PR | ||
| # We do different approach on subsequent runs because otherwise we would spam users' mailboxes | ||
| # with notifications about pushes to existing PRs. This way there is actually no push if not needed. | ||
| update: | ||
Check warningCode scanning / CodeQL Workflow does not contain permissions Medium
Actions job or workflow does not limit the permissions of the GITHUB_TOKEN. Consider setting an explicit permissions block, using the following as a minimal starting point: {}
|
||
| needs: validate-inputs | ||
| runs-on: ${{ inputs.runs-on }} | ||
| # Map the job outputs to step outputs | ||
| outputs: | ||
| prUrl: ${{ steps.pr.outputs.url }} | ||
| baseBranch: ${{ steps.root.outputs.baseBranch }} | ||
| prBranch: ${{ steps.root.outputs.prBranch }} | ||
| originalTag: ${{ steps.target.outputs.originalTag }} | ||
| latestTag: ${{ steps.target.outputs.latestTag }} | ||
| timeout-minutes: 30 | ||
| defaults: | ||
| run: | ||
| shell: pwsh | ||
| env: | ||
| DEPENDENCY_NAME: ${{ inputs.name }} | ||
| DEPENDENCY_PATH: ${{ inputs.path }} | ||
| DEPENDENCY_PATTERN: ${{ inputs.pattern }} | ||
| CHANGELOG_SECTION: ${{ inputs.changelog-section }} | ||
| PR_STRATEGY: ${{ inputs.pr-strategy }} | ||
| steps: | ||
| - uses: actions/checkout@v4 | ||
| with: | ||
| ssh-key: ${{ secrets.api-token }} | ||
|
|
||
| # In order to run scripts from this repo, we need to check it out manually, doesn't seem available locally. | ||
| - name: Check out workflow scripts | ||
| # Note: cannot use `actions/checkout` at the moment because you can't clone outside of the repo root. | ||
| # Follow https://github.com/actions/checkout/issues/197 | ||
| run: | | ||
| mkdir -p ${{ runner.temp }}/ghwf | ||
| cd ${{ runner.temp }}/ghwf | ||
| git init | ||
| git remote add origin https://github.com/getsentry/github-workflows.git | ||
| git fetch --depth 1 origin ${{ inputs._workflow_version }} | ||
| git checkout FETCH_HEAD | ||
|
|
||
| - name: Update to the latest version | ||
| id: target | ||
| run: ${{ runner.temp }}/ghwf/updater/scripts/update-dependency.ps1 -Path '${{ inputs.path }}' -Pattern '${{ inputs.pattern }}' | ||
| run: ${{ runner.temp }}/ghwf/updater/scripts/update-dependency.ps1 -Path $env:DEPENDENCY_PATH -Pattern $env:DEPENDENCY_PATTERN | ||
|
|
||
| - name: Get the base repo info | ||
| if: steps.target.outputs.latestTag != steps.target.outputs.originalTag | ||
| id: root | ||
| run: | | ||
| $mainBranch = $(git remote show origin | Select-String "HEAD branch: (.*)").Matches[0].Groups[1].Value | ||
| $prBranch = switch ('${{ inputs.pr-strategy }}') | ||
| $prBranch = switch ($env:PR_STRATEGY) | ||
| { | ||
| 'create' { 'deps/${{ inputs.path }}/${{ steps.target.outputs.latestTag }}' } | ||
| 'update' { 'deps/${{ inputs.path }}' } | ||
| default { throw "Unkown PR strategy '${{ inputs.pr-strategy }}'." } | ||
| 'create' { "deps/$env:DEPENDENCY_PATH/${{ steps.target.outputs.latestTag }}" } | ||
| 'update' { "deps/$env:DEPENDENCY_PATH" } | ||
| default { throw "Unkown PR strategy '$env:PR_STRATEGY'." } | ||
| } | ||
| "baseBranch=$mainBranch" | Tee-Object $env:GITHUB_OUTPUT -Append | ||
| "prBranch=$prBranch" | Tee-Object $env:GITHUB_OUTPUT -Append | ||
| $nonBotCommits = ${{ runner.temp }}/ghwf/updater/scripts/nonbot-commits.ps1 ` | ||
| -RepoUrl "$(git config --get remote.origin.url)" -PrBranch $prBranch -MainBranch $mainBranch | ||
| $changed = $nonBotCommits.Length -gt 0 ? 'true' : 'false' | ||
| "changed=$changed" | Tee-Object $env:GITHUB_OUTPUT -Append | ||
| if ("$changed" -eq "true") | ||
| { | ||
| Write-Output "::warning::Target branch '$prBranch' has been changed manually - skipping updater to avoid overwriting these changes." | ||
| } | ||
|
|
||
| - name: Parse the existing PR URL | ||
| if: ${{ ( steps.target.outputs.latestTag != steps.target.outputs.originalTag ) && ( steps.root.outputs.changed == 'false') }} | ||
| id: existing-pr | ||
| env: | ||
| GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} | ||
| run: | | ||
| $urls = @(gh api 'repos/${{ github.repository }}/pulls?base=${{ steps.root.outputs.baseBranch }}&head=${{ github.repository_owner }}:${{ steps.root.outputs.prBranch }}' --jq '.[].html_url') | ||
| if ($urls.Length -eq 0) | ||
| { | ||
| "url=" | Tee-Object $env:GITHUB_OUTPUT -Append | ||
| } | ||
| elseif ($urls.Length -eq 1) | ||
| { | ||
| "url=$($urls[0])" | Tee-Object $env:GITHUB_OUTPUT -Append | ||
| } | ||
| else | ||
| { | ||
| throw "Unexpected number of PRs matched ($($urls.Length)): $urls" | ||
| } | ||
|
|
||
| - run: git --no-pager diff | ||
| if: ${{ ( steps.target.outputs.latestTag != steps.target.outputs.originalTag ) && ( steps.existing-pr.outputs.url == '') && ( steps.root.outputs.changed == 'false') }} | ||
|
|
||
| - name: Get target changelog | ||
| if: ${{ ( steps.target.outputs.latestTag != steps.target.outputs.originalTag ) && ( steps.root.outputs.changed == 'false') }} | ||
| run: | | ||
| $changelog = ${{ runner.temp }}/ghwf/updater/scripts/get-changelog.ps1 ` | ||
| -RepoUrl '${{ steps.target.outputs.url }}' ` | ||
| -OldTag '${{ steps.target.outputs.originalTag }}' ` | ||
| -NewTag '${{ steps.target.outputs.latestTag }}' | ||
| ${{ runner.temp }}/ghwf/updater/scripts/set-github-env.ps1 TARGET_CHANGELOG $changelog | ||
|
|
||
| # First we create a PR only if it doesn't exist. We will later overwrite the content with the same action. | ||
| - name: Create a PR | ||
| if: ${{ ( steps.target.outputs.latestTag != steps.target.outputs.originalTag ) && ( steps.existing-pr.outputs.url == '') && ( steps.root.outputs.changed == 'false') }} | ||
| uses: peter-evans/create-pull-request@a4f52f8033a6168103c2538976c07b467e8163bc # pin#v6.0.1 | ||
| id: create-pr | ||
| with: | ||
| base: ${{ steps.root.outputs.baseBranch }} | ||
| branch: ${{ steps.root.outputs.prBranch }} | ||
| commit-message: 'chore: update ${{ inputs.path }} to ${{ steps.target.outputs.latestTag }}' | ||
| commit-message: 'chore: update ${{ env.DEPENDENCY_PATH }} to ${{ steps.target.outputs.latestTag }}' | ||
| author: 'GitHub <[email protected]>' | ||
| title: 'chore(deps): update ${{ inputs.name }} to ${{ steps.target.outputs.latestTagNice }}' | ||
| title: 'chore(deps): update ${{ env.DEPENDENCY_NAME }} to ${{ steps.target.outputs.latestTagNice }}' | ||
| body: | | ||
| Bumps ${{ inputs.path }} from ${{ steps.target.outputs.originalTag }} to ${{ steps.target.outputs.latestTag }}. | ||
| Bumps ${{ env.DEPENDENCY_PATH }} from ${{ steps.target.outputs.originalTag }} to ${{ steps.target.outputs.latestTag }}. | ||
|
|
||
| Auto-generated by a [dependency updater](https://github.com/getsentry/github-workflows/blob/main/.github/workflows/updater.yml). | ||
| ${{ env.TARGET_CHANGELOG }} | ||
| labels: dependencies | ||
| # draft: true | ||
|
|
||
| - name: Verify we have a PR | ||
| if: ${{ ( steps.target.outputs.latestTag != steps.target.outputs.originalTag ) && ( steps.root.outputs.changed == 'false') }} | ||
| id: pr | ||
| run: | | ||
| if ('${{ steps.create-pr.outputs.pull-request-url }}' -ne '') | ||
| { | ||
| "url=${{ steps.create-pr.outputs.pull-request-url }}" | Tee-Object $env:GITHUB_OUTPUT -Append | ||
| } | ||
| elseif ('${{ steps.existing-pr.outputs.url }}' -ne '') | ||
| { | ||
| "url=${{ steps.existing-pr.outputs.url }}" | Tee-Object $env:GITHUB_OUTPUT -Append | ||
| } | ||
| else | ||
| { | ||
| throw "PR hasn't been created" | ||
| } | ||
|
|
||
| # If we had to create a new PR, we must do a clean checkout & update the submodule again. | ||
| # If we didn't do this, the new PR would only have a changelog... | ||
| - name: 'After new PR: restore repo' | ||
| if: ${{ ( steps.target.outputs.latestTag != steps.target.outputs.originalTag ) && ( steps.existing-pr.outputs.url == '') && ( steps.root.outputs.changed == 'false') }} | ||
| uses: actions/checkout@v4 | ||
| with: | ||
| ssh-key: ${{ secrets.api-token }} | ||
|
|
||
| - name: 'After new PR: redo the update' | ||
| if: ${{ ( steps.target.outputs.latestTag != steps.target.outputs.originalTag ) && ( steps.existing-pr.outputs.url == '') && ( steps.root.outputs.changed == 'false') }} | ||
| run: ${{ runner.temp }}/ghwf/updater/scripts/update-dependency.ps1 -Path '${{ inputs.path }}' -Tag '${{ steps.target.outputs.latestTag }}' | ||
| run: ${{ runner.temp }}/ghwf/updater/scripts/update-dependency.ps1 -Path $env:DEPENDENCY_PATH -Tag '${{ steps.target.outputs.latestTag }}' | ||
|
|
||
| - name: Update Changelog | ||
| if: ${{ inputs.changelog-entry && ( steps.target.outputs.latestTag != steps.target.outputs.originalTag ) && ( steps.root.outputs.changed == 'false') }} | ||
| run: | | ||
| ${{ runner.temp }}/ghwf/updater/scripts/update-changelog.ps1 ` | ||
| -Name '${{ inputs.name }}' ` | ||
| -Name $env:DEPENDENCY_NAME ` | ||
| -PR '${{ steps.pr.outputs.url }}' ` | ||
| -RepoUrl '${{ steps.target.outputs.url }}' ` | ||
| -MainBranch '${{ steps.target.outputs.mainBranch }}' ` | ||
| -OldTag '${{ steps.target.outputs.originalTag }}' ` | ||
| -NewTag '${{ steps.target.outputs.latestTag }}' ` | ||
| -Section '${{ inputs.changelog-section }}' | ||
| -Section $env:CHANGELOG_SECTION | ||
|
|
||
| - run: git --no-pager diff | ||
| if: ${{ ( steps.target.outputs.latestTag != steps.target.outputs.originalTag ) && ( steps.root.outputs.changed == 'false') }} | ||
|
|
||
| # Now make the PR in its final state. This way we only have one commit and no updates if there are no changes between runs. | ||
| - name: Update the PR | ||
| if: ${{ ( steps.target.outputs.latestTag != steps.target.outputs.originalTag ) && ( steps.root.outputs.changed == 'false') }} | ||
| uses: peter-evans/create-pull-request@a4f52f8033a6168103c2538976c07b467e8163bc # pin#v6.0.1 | ||
| with: | ||
| base: ${{ steps.root.outputs.baseBranch }} | ||
| branch: ${{ steps.root.outputs.prBranch }} | ||
| commit-message: 'chore: update ${{ inputs.path }} to ${{ steps.target.outputs.latestTag }}' | ||
| commit-message: 'chore: update ${{ env.DEPENDENCY_PATH }} to ${{ steps.target.outputs.latestTag }}' | ||
| author: 'GitHub <[email protected]>' | ||
| title: 'chore(deps): update ${{ inputs.name }} to ${{ steps.target.outputs.latestTagNice }}' | ||
| title: 'chore(deps): update ${{ env.DEPENDENCY_NAME }} to ${{ steps.target.outputs.latestTagNice }}' | ||
| body: | | ||
| Bumps ${{ inputs.path }} from ${{ steps.target.outputs.originalTag }} to ${{ steps.target.outputs.latestTag }}. | ||
| Bumps ${{ env.DEPENDENCY_PATH }} from ${{ steps.target.outputs.originalTag }} to ${{ steps.target.outputs.latestTag }}. | ||
|
|
||
| Auto-generated by a [dependency updater](https://github.com/getsentry/github-workflows/blob/main/.github/workflows/updater.yml). | ||
| ${{ env.TARGET_CHANGELOG }} | ||
| labels: dependencies | ||
Check warningCode scanning / CodeQL Workflow does not contain permissions Medium
Actions job or workflow does not limit the permissions of the GITHUB_TOKEN. Consider setting an explicit permissions block, using the following as a minimal starting point: {contents: read}
|
||
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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this also have double-quotes around?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no because we can just switch directly on the variable value instead of converting it to string (which it is anyway), and actually I don't think we should be quoting in the step above, let me check