-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Run zizmor workflow scanner and fix issues identified. #51500
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
Conversation
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.
Pull Request Overview
This PR enhances security in GitHub Actions workflows by preventing credential persistence and standardizing environment variable usage. The changes address security best practices for workflows that interact with PRs and external repositories.
- Adds
persist-credentials: falseto checkout actions across multiple workflows to prevent credential exposure - Refactors GitHub Actions expression interpolation to use environment variables instead of direct inline expressions in shell scripts
- Changes workflow triggers from
pull_request_targettopull_requestfor safer execution context
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
.github/workflows/update-static-web-assets-baselines.yml |
Adds persist-credentials: false and refactors all GitHub Actions expressions to use environment variables for safer shell script execution |
.github/workflows/update-man-pages.yml |
Adds persist-credentials: false to checkout action |
.github/workflows/remove-lockdown-label.yml |
Changes trigger from pull_request_target to pull_request for safer execution |
.github/workflows/copilot-setup-steps.yml |
Adds persist-credentials: false to checkout action |
.github/workflows/add-lockdown-label.yml |
Changes trigger from pull_request_target to pull_request and adds persist-credentials: false to checkout action |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
|
||
| on: | ||
| pull_request_target: | ||
| pull_request: |
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.
I don't think this is going to work, nor do I think the change is correct.
As noted in GitHub's docs:
- https://docs.github.com/en/actions/writing-workflows/choosing-when-your-workflow-runs/events-that-trigger-workflows#pull_request_target
- "This event allows your workflow to do things like label or comment on pull requests from forks. Avoid using this event if you need to build or run code from the pull request."
And noted in Zizmor's docs:
- https://github.com/zizmorcore/zizmor/blob/main/docs/audits.md#dangerous-triggers
- "Replace pull_request_target with pull_request, unless you absolutely need repository write permissions (e.g. to leave a comment or make other changes to the upstream repo)."
This workflow is precisely doing what the pull_request_target workflow is situated for: performing privileged actions on the upstream repo (labeling) without running code from the pull request. The pull_request event, for pull requests from forks, will not have the necessary permission, which would result in the PR showing the action as needing to be authorized. If authorized, that would permit a pull request to run an action with escalated permissions.
This is precisely why the dotnet/issue-labeler uses pull_request_target.
However, the other remediation notes on zizmor apply:
- Never run PR-controlled code in the context of a pull_request_target-triggered workflow.
- Consider adding a branch filter to only run the workflow for matching target branches.
- consider adding a github.repository == ... check to only run for your repository but not in forks
Where we've set up issue-labeler in each of our repos, we do apply all three of those remediations as well.
/cc @blowdart
|
|
||
| on: | ||
| pull_request_target: | ||
| pull_request: |
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.
Same comment as above; I don't think this change is right.
| branches: | ||
| - 'release/8.*' | ||
| - 'release/9.*' | ||
| - 'release/10.*' |
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.
I think it's reasonable to just use 'release/*' here as that probably matches the branch protection rule, then it wouldn't need to be updated with each release.
| # Only run on the main repository, not forks | ||
| if: github.repository == 'dotnet/sdk' |
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.
This works. Alternate approach we used for the issue-labeler is to only check the org and also to allow a manual dispatch to bypass this check. I doubt this repo would be renamed or that you care to let forks manually dispatch the action though.
Tool was recommended by @blowdart I used the --fix switch.