-
Notifications
You must be signed in to change notification settings - Fork 127
External PR 1471: Add explicit permissions to CI workflow #1483
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
- Added workflow-level default permissions (contents: read) - Added job-level permissions for all jobs following least privilege: - build: contents:read, actions:write, security-events:write - upload-event-file: contents:read, actions:write - build-for-e2e-test: contents:read, actions:write - e2e-test: contents:read, actions:write, checks:write - publish: contents:write Fixes #1457
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 adds explicit permissions to the CI workflow following GitHub Actions security best practices and the principle of least privilege. It sets a baseline contents: read permission at the workflow level and then grants specific permissions to each job based on their actual needs. This addresses 403 permission errors that were occurring during CI runs.
Key Changes
- Workflow-level default permissions set to
contents: readfor baseline security - Job-specific permissions added for build, upload-event-file, build-for-e2e-test, e2e-test, and publish jobs
- Each job receives only the minimum permissions required for its operations (artifacts, CodeQL, test results, releases)
Comments suppressed due to low confidence (1)
.github/workflows/CI.yml:164
- The e2e-test job has duplicate
permissionsblocks. Lines 156-159 add new permissions, but lines 162-164 contain an existing permissions block. This will cause a YAML syntax error.
The permissions should be merged into a single block. Based on the PR description and the workflow's usage of publishing test results, the correct merged permissions should be:
permissions:
contents: read
actions: write
checks: writeRemove the duplicate block at lines 162-164.
permissions:
contents: read
actions: write
checks: write
if: github.event_name != 'pull_request' || github.event.pull_request.head.repo.owner.login == 'github'
needs: [build-for-e2e-test]
permissions:
checks: write
contents: read
mulana
left a comment
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.
lgtm
Summary
This PR adds explicit permissions to the CI workflow following security best practices and the principle of least privilege.
Context
This is based on external PR #1471 by @yugannkt. This fixes the 403 errors we've been seeing in CI when publishing test results and ensures all jobs have only the permissions they need.
Changes
contents: readas baselinecontents: read,actions: write,security-events: write(for checkout, artifacts, CodeQL)contents: read,actions: write(for uploading event file)contents: read,actions: write(for building and uploading binaries)contents: read,actions: write,checks: write(for downloading artifacts, uploading logs, publishing test results)contents: write(for creating releases and committing release notes)This follows the principle of least privilege by giving each job only the permissions it actually needs.
Fixes #1457