fix: pin 5 unpinned action(s), extract 1 step output to env var#35165
fix: pin 5 unpinned action(s), extract 1 step output to env var#35165dagecko wants to merge 3 commits intoiptv-org:masterfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR hardens CI workflows by pinning third‑party GitHub Actions to immutable commit SHAs and by moving a workflow expression used in a shell run: block into an environment variable to reduce injection risk.
Changes:
- Pin
tibdex/github-app-tokenandJamesIves/github-pages-deploy-actionusages to full commit SHAs (with version comments). - Refactor
check.ymlto pass changed-file list viaenv: ALL_CHANGED_FILESinstead of embedding${{ ... }}directly in therun:script.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| .github/workflows/update.yml | Pins tibdex/github-app-token and JamesIves/github-pages-deploy-action to commit SHAs. |
| .github/workflows/stale.yml | Pins tibdex/github-app-token to a commit SHA. |
| .github/workflows/format.yml | Pins tibdex/github-app-token to a commit SHA; modifies changed-file gathering script. |
| .github/workflows/check.yml | Moves step output reference into env and updates validation commands; modifies changed-file gathering script. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Good catch on the - Chris (dagecko) |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| - uses: actions/checkout@v6 | ||
| - uses: tibdex/github-app-token@v1.8.2 | ||
| - uses: tibdex/github-app-token@0d49dd721133f900ebd5e0dff2810704e8defbc6 # v1.8.2 | ||
| if: ${{ !env.ACT }} |
There was a problem hiding this comment.
PR description says it "pins all GitHub Actions to immutable commit SHAs", but this workflow (and others) still use mutable tags for first-party actions like actions/checkout@v6 and actions/setup-node@v6. Either adjust the PR description to clarify it only pins the third-party actions, or also pin the remaining actions to SHAs for consistency with the stated goal.
|
@freearhey you're right, the diff on check.yml is mostly a CRLF to LF line ending normalization that happened while we were working on the file. Apologies for the noise in the diff. There is one actual functional change at the bottom of the file. The step output reference was moved from inline in the run block to an env block for safer shell expansion: Before: This prevents the step output from being interpreted as shell code if it ever contains special characters. If you'd prefer I resubmit with just the functional changes and without the line ending normalization, happy to do that.
|
9c57365 to
86542f2
Compare
|
@freearhey cleaned up the line ending issue. The diff should now only show the actual functional changes. Thanks for flagging it.
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| run: | | ||
| npm run playlist:lint -- ${{ steps.files.outputs.all_changed_files }} | ||
| npm run playlist:validate -- ${{ steps.files.outputs.all_changed_files }} No newline at end of file | ||
| npm run playlist:lint -- ${ALL_CHANGED_FILES} | ||
| npm run playlist:validate -- ${ALL_CHANGED_FILES} | ||
| env: | ||
| ALL_CHANGED_FILES: ${{ steps.files.outputs.all_changed_files }} No newline at end of file |
There was a problem hiding this comment.
The step output steps.files.outputs.all_changed_files ultimately originates from writing ALL_CHANGED_FILES into $GITHUB_OUTPUT as a plain name=value line. If a changed filename ever contains a newline or other special characters, this can corrupt/forge step outputs (workflow command injection) and then propagate into ALL_CHANGED_FILES here. Consider generating the file list in a NUL-delimited form (e.g., git diff -z) and writing the output using the $GITHUB_OUTPUT delimiter (multiline) syntax so arbitrary filenames can’t break the output format.
Re-submission of #35030. Had a problem with my fork and had to delete it, which closed the original PR. Apologies for the noise.
Summary
This PR pins all GitHub Actions to immutable commit SHAs instead of mutable version tags and extracts a step output expression from a
run:block into anenv:mapping.Changes by file
${{ steps.files.outputs.all_changed_files }}→ALL_CHANGED_FILESenv varHow to verify
Review the diff — each change is mechanical and preserves workflow behavior:
action@v3becomesaction@abc123 # v3— original version preserved as comment${{ steps.files.outputs.all_changed_files }}inrun:moves toenv:block, referenced as${ALL_CHANGED_FILES}in the script — prevents shell injection if filenames contain metacharactersI wrote a scanner called Runner Guard and open sourced it here so you can scan yourself if you want to. More on the research behind this on Twitter.
If you have any questions, reach out. I'll be monitoring comms.
- Chris Nyhuis (dagecko)