Pin third-party actions to SHAs and scope workflow tokens - #413
Conversation
Closes the rest of #384. Every third-party action was referenced by mutable tag, which SonarCloud has been flagging on every PR (githubactions:S7637): a tag can be repointed by the action owner, or by anyone who compromises the action repo, and these workflows would silently run different code. Eight actions are now pinned to full commit SHAs with the tag in a trailing comment, which Dependabot understands and can bump. The ninth cannot be pinned: devbotsxyz/xcode-select, used by pages.yml, returns 404 — the repository no longer exists on GitHub. That workflow therefore cannot run at all, which is a third independent breakage on top of the two already recorded in #385. Left in place rather than guessing at a replacement, since whether Pages publishing is still wanted is an open question there. The repository default workflow permission is write, so every workflow was inheriting a write-scoped GITHUB_TOKEN regardless of need. Each now declares the minimum it actually uses. release.yml differs per job: the build only reads, publishing needs contents: write, and the version bump additionally needs pull-requests: write. persist-credentials: false on every checkout that does not push, so the token is not left in .git/config while builds and third-party tooling run. Two checkouts deliberately keep it: bump_version, whose working copy create-pull-request pushes from, and pages.yml, which is untouched pending the decision in #385. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe workflows now restrict repository token permissions, prevent checkout credential persistence, and pin third-party actions to immutable commit references. Release jobs retain the write permissions required for publishing and version updates. ChangesWorkflow hardening
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
.github/workflows/release.yml (1)
149-149: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winConsider using the runner-provided
ghCLI.
zizmorreportssoftprops/action-gh-releaseas superfluous. The GitHub CLI supports release creation with asset patterns,--generate-notes, and--prerelease, so this step can avoid an additional third-party action while preserving the current behavior. (cli.github.com)If you remove the action, keep the current tag, artifact path, and conditional prerelease behavior. Verify the replacement on
ubuntu-latestbefore changing the release path.Possible first-party replacement
- - name: Release - uses: softprops/action-gh-release@3d0d9888cb7fd7b750713d6e236d1fcb99157228 # v3 - with: - prerelease: ${{ needs.build.outputs.prerelease == 'true' }} - generate_release_notes: true - files: | - application/xchtmlreport-* - env: - GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} + - name: Release + env: + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + GH_REPO: ${{ github.repository }} + run: | + args=(release create "$GITHUB_REF_NAME" application/xchtmlreport-* --generate-notes --verify-tag) + if [[ "${{ needs.build.outputs.prerelease }}" == "true" ]]; then + args+=(--prerelease) + fi + gh "${args[@]}"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/release.yml at line 149, Replace the softprops/action-gh-release step with a runner-provided gh release create invocation, preserving the existing tag, artifact path or asset pattern, generated notes, and conditional prerelease behavior. Keep the release step’s current conditions and validate the command on ubuntu-latest before finalizing the workflow change.Source: Linters/SAST tools
.github/workflows/pages.yml (1)
6-10: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winKeep
contents: readat job scope.Both workflows use workflow-level
contents: readeven though the required repository access belongs tobuild. The current elevated jobs override the default, but a future job could inherit it. GitHub sets unspecified permissions tononewhen explicit permissions are declared, so job-level blocks provide the clearer boundary. (docs.github.com)
.github/workflows/pages.yml#L6-L10: movecontents: readunderjobs.build.permissions; keep the deploy permissions limited to Pages and OIDC..github/workflows/release.yml#L17-L21: movecontents: readunderjobs.build.permissions; keep thereleaseandbump_versionpermission blocks explicit.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/pages.yml around lines 6 - 10, Move contents: read from workflow scope into jobs.build.permissions in .github/workflows/pages.yml lines 6-10, while keeping the deploy permissions limited to Pages and OIDC. Apply the same change in .github/workflows/release.yml lines 17-21, preserving explicit permissions for the release and bump_version jobs.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In @.github/workflows/pages.yml:
- Around line 6-10: Move contents: read from workflow scope into
jobs.build.permissions in .github/workflows/pages.yml lines 6-10, while keeping
the deploy permissions limited to Pages and OIDC. Apply the same change in
.github/workflows/release.yml lines 17-21, preserving explicit permissions for
the release and bump_version jobs.
In @.github/workflows/release.yml:
- Line 149: Replace the softprops/action-gh-release step with a runner-provided
gh release create invocation, preserving the existing tag, artifact path or
asset pattern, generated notes, and conditional prerelease behavior. Keep the
release step’s current conditions and validate the command on ubuntu-latest
before finalizing the workflow change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 862f587d-9b7e-4b78-9765-6410a7e2b86d
📒 Files selected for processing (7)
.github/workflows/ci.yml.github/workflows/codecov.yml.github/workflows/homebrew-bump.yml.github/workflows/pages.yml.github/workflows/release.yml.github/workflows/test-artifacts.yml.github/workflows/test.yml
SonarCloud githubactions:S8264 on release.yml and pages.yml. Both had a workflow-level contents: read that most of their jobs then overrode, so the grant effectively served one job while the rest inherited something they did not need. Every job in both files now declares its own minimum. Verified no workflow-level permissions block remains in either, and that no job is left implicit — without a workflow-level default, an undeclared job would fall back to the repository default, which is write. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
|
SonarCloud Both files had a workflow-level
Worth stating the sharp edge, since it is the opposite of the usual advice: removing the workflow-level block means any job without its own declaration falls back to the repository default, which is write. So this is only safe because every job is now explicit. Verified by walking the parsed YAML rather than reading the diff. CodeRabbit nitpick — use It would genuinely remove a third-party action from the release path, which is squarely in the spirit of this PR. But the release job is guarded on Filed separately so it can be done deliberately, alongside a release where it can actually be exercised. |
upload-artifact and download-artifact are a matched pair in release.yml: the build job uploads the signed, notarized binary and the release job downloads exactly that artifact to attach it to the release. Dependabot raised only the upload half, because open-pull-requests-limit was already saturated by the other actions updates. Merging that alone would have left upload at v7 and download at v4 — a mismatch the release dry run cannot catch, since the release job only runs on a tag push. Both halves now move together. Verified the merge preserves the SHA pinning from #413: SonarCloud reported the third-party actions as unpinned, but that was an artifact of analysing a branch nine commits behind main. A test merge confirmed all five third-party pins survive, and that setup-xcode's SHA was correctly re-pinned by Dependabot when it bumped to 1.7.0.
Closes #385. The workflow could not run, in three independent ways: - devbotsxyz/[email protected] no longer exists — the repository returns 404. Found while SHA-pinning actions in #413; it was the one action that could not be pinned. - `swift run xchtmlreport -- -j ...` exits 64: SwiftPM forwards the --, so -j parses as a result-bundle path rather than the JUnit flag. - It downloaded sample-test-results-macos-12-14.2, an artifact name test-artifacts.yml no longer produces. Nothing is lost: the Pages site returns HTTP 404 and has never served content. The only deployment attempts were three, all on 2023-05-15. Closes #405 and #406 as moot — both only touched this file. The repository still has Pages enabled in settings, now with no workflow behind it. That can be switched off in repository settings if wanted; a pull request should not change repository settings silently.
…tion (#427) * Publish releases with the runner's gh CLI instead of an action The release job is the most sensitive path in the repository: it holds the Developer ID signing identity, the notarization credentials and a token with contents: write. Until now it also handed all of that to a third-party action to do something the runner can already do. zizmor flags it as superfluous and CodeRabbit raised it on #413. `gh release create` covers every input the action was given, and the resulting release is identical in shape: --generate-notes sets both the body and the title, and GitHub's generate-notes API returns the tag as the name, which is what the action produced for 3.0.0rc1 and 3.0.0. Neither sends make_latest, so GitHub's default still applies. Neither drafts. Two things are deliberately stricter than what they replace. --verify-tag stops gh from inventing a tag off the default branch if the one it is handed does not exist; the tag is what triggered the run, so that could only mean something had gone wrong. And a glob that matches nothing now fails the step, where the action would happily have published a release with no binary attached to it. Everything reaches the script through the environment rather than being interpolated into it, and GH_REPO is set because this job has no checkout for gh to infer the repository from. Refs #414 * Record why a re-run over an existing release is meant to fail Raised by CodeRabbit on #427. The old action would have replaced the assets of an already-published release; gh refuses. On a path that publishes signed, notarized binaries that is the behaviour we want, so it is worth writing down before someone reads the difference as a regression and adds a fallback.
Closes the rest of #384. Third of the maintenance-automation batch.
SHA pinning
Every third-party action was referenced by mutable tag — the finding SonarCloud has been raising on every PR in this batch (
githubactions:S7637). A tag can be repointed by the action owner, or by anyone who compromises the action's repository, and these workflows would silently execute different code.Eight actions pinned to full commit SHAs with the tag preserved in a trailing comment, which is the form Dependabot reads and bumps:
Apple-Actions/import-codesign-certs5142e029…(v7)codecov/codecov-actionb9fd7d16…(v4)dawidd6/action-download-artifactbf251b5a…(v6)dawidd6/action-homebrew-bump-formulaa0e064e0…(v8)maxim-lobanov/setup-xcode60606e26…(v1.6.0)peter-evans/create-pull-request5f6978fa…(v8)reecetech/version-incrementa29aa752…(2024.10.1)softprops/action-gh-release3d0d9888…(v3)actions/*are left on tags — first-party, and not what the rule targets.One action could not be pinned, because it no longer exists
devbotsxyz/[email protected]is used atpages.yml:13. The repository has been deleted from GitHub, so that workflow cannot run at all — a third independent breakage on top of the two already recorded in #385 (--swallowing-j, and downloading an artifact name that is no longer produced).Left in place rather than guessing at a replacement, since #385 asks whether Pages publishing is still wanted at all. If it is, that action needs replacing with
maxim-lobanov/setup-xcodeas part of fixing it.Token scoping
The repository default workflow permission is write, so every workflow has been inheriting a write-scoped
GITHUB_TOKENregardless of what it does. Each now declares its minimum.release.ymldiffers per job rather than taking a blanket value:buildcontents: readreleasecontents: writebump_versioncontents: write,pull-requests: writepersist-credentialsactions/checkoutleaves the token in.git/configby default. Set tofalseon every checkout that does not push, so it is not sitting on disk while builds and third-party tooling (brew install,xcodebuild) run.Two checkouts deliberately keep credentials:
bump_version—peter-evans/create-pull-requestpushes a branch from that working copy.pages.yml— untouched pending pages.yml is broken two ways #385.Verification
All nine workflow files parse, and each checkout's setting was confirmed by walking the parsed YAML per step rather than by reading the diff —
test-artifacts.ymluses 6-space step indentation where the others use 4, and the first attempt putwith:at the wrong level. YAML validation caught it.A release dry run follows, to confirm the
release.ymlpermission and checkout changes do not disturb a pipeline that was verified end to end earlier today.🤖 Generated with Claude Code
Summary by CodeRabbit