ci: declare workflow-level contents: read on 4 workflows#712
ci: declare workflow-level contents: read on 4 workflows#712arpitjain099 wants to merge 1 commit into
contents: read on 4 workflows#712Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (3)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughFour GitHub Actions workflows are updated to add explicit ChangesGitHub Actions Permission Hardening
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Hi Arpit — thanks for the focused supply-chain hardening. The CVE citation and Scorecard rationale make this easy to follow, and after this PR every workflow in .github/workflows/ carries an explicit permissions cap — a clean Scorecard win.
I verified both upstream reusable workflows (lfx-public-workflows/license-header-check.yml and lfx-ui/_pr-title-lint.yml) to check the actual API surface. Three of the four are clean: license-header-check, markdown-lint, and quality-check need nothing beyond contents: read. The fourth (pr-title-lint) is where the description is slightly off — see the inline comment above.
One process note: the commit shows as unsigned in GitHub's verification (reason: "unsigned"). Repo policy requires both --signoff (present ✅) and GPG signing (-S). A quick git commit --amend --no-edit -S && git push --force-with-lease before merge would bring it in line.
Summary:
- Blocking: 0
- Minor: 1 (pr-title-lint description accuracy / optional UX fix)
- Nits: 2 (missing EOF newline, unsigned commit)
Decision: Fix the code signing - this is a requirement. Otherwise, minor comments — happy to approve once issue 1 is acknowledged either way (description update or add pull-requests: write on that one file).
| # Possible values: https://help.github.com/en/actions/reference/events-that-trigger-workflows#pull-request-event-pull_request | ||
| types: [opened, edited, reopened, synchronize] | ||
|
|
||
| permissions: |
There was a problem hiding this comment.
[minor] The PR description says this workflow "doesn't call a GitHub API beyond the initial checkout," but the upstream reusable workflow (linuxfoundation/lfx-ui/.github/workflows/_pr-title-lint.yml@main) runs morrisoncole/pr-lint-action@v1.7.1 with:
on-failed-regex-comment— posts a PR comment on failure → needspull-requests: writeon-succeeded-regex-dismiss-review-comment— dismisses a review on success → needspull-requests: write
With the caller restricted to contents: read, those two API calls will return 403. The lint check itself remains gating (on-failed-regex-fail-action: true still fails the action), but contributors with a bad PR title will see a red ❌ with no explanatory comment, and the success-side review dismissal becomes a no-op.
Two options:
- Keep
contents: readand update the PR description to note this trade-off (and optionally raise a follow-up in the upstream workflow). - Add
pull-requests: writeon this file — that still caps the token and satisfies OpenSSF Scorecard, while preserving the comment UX.
|
|
||
| jobs: | ||
| pr-title-lint: | ||
| uses: linuxfoundation/lfx-ui/.github/workflows/_pr-title-lint.yml@main No newline at end of file |
There was a problem hiding this comment.
[nit] Missing trailing newline (\ No newline at end of file in the diff). Pre-existing condition, but easy to fix in the same commit since you're already touching this file.
Hi @dealako thanks for the comment. I will fix this after I get back home tonight. |
Pins the default GITHUB_TOKEN to contents: read on the workflows in .github/workflows/ that don't call a GitHub API beyond the initial checkout. The other workflows in this directory are left implicit because they need write scopes that a maintainer is better placed to declare. Motivation: CVE-2025-30066 (March 2025 tj-actions/changed-files compromise) exfiltrated GITHUB_TOKEN from workflow logs. Per-workflow caps bound runtime authority irrespective of repo or org default, give drift protection if the default ever widens, and are credited per-file by the OpenSSF Scorecard Token-Permissions check. YAML validated locally with yaml.safe_load. Signed-off-by: Arpit Jain <arpitjain099@gmail.com>
90fe0ba to
5444b07
Compare
|
@dealako pushed a force-with-lease with the commit SSH-signed (now showing as Verified on GitHub). Rebased onto current main while I was at it. Let me know if you'd like me to handle the |
|
@arpitjain099 - the one change will break the current behavior. For this one entry, I think the correct solution is to assign write permissions per this discussion. |
@dealako noted. let me review this tomorrow. |
Pins the default
GITHUB_TOKENtocontents: readon 4 workflows in.github/workflows/that don't call a GitHub API beyond the initial checkout.Why
CVE-2025-30066 (March 2025
tj-actions/changed-filessupply-chain compromise) exfiltratedGITHUB_TOKENfrom workflow logs. Pinning per workflow caps runtime authority irrespective of the repo or org default, gives drift protection if the default ever widens, and is credited per-file by the OpenSSF ScorecardToken-Permissionscheck.YAML validated locally with
yaml.safe_loadon each touched file.