-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Fix: Set minimal permissions to github workflows #4665
Conversation
Signed-off-by: Joyce <joycebrum@google.com>
Signed-off-by: Joyce <joycebrum@google.com>
Signed-off-by: Joyce <joycebrum@google.com>
Signed-off-by: Joyce <joycebrum@google.com>
Signed-off-by: Joyce <joycebrum@google.com>
Signed-off-by: Joyce <joycebrum@google.com>
Signed-off-by: Joyce <joycebrum@google.com>
Signed-off-by: Joyce <joycebrum@google.com>
Signed-off-by: Joyce <joycebrum@google.com>
Everything is working, thanks! What if someone sends a PR that removes those settings again? Do we have to change some global settings? So that removing the workflow-specific settings means read-only? (Does that question make sense?) |
Yeah it does! It is possible to change the default permission of the workflows to read only so, even if at some moment this permission got omitted from the file or a new workflow is submitted without the minimal permission needed configured, it will only be granted the configured default permissions. See Setting the permissions of the GITHUB_TOKEN for your repository to know how to configure it. |
I've changed the setting. Could some of this be simplified now? I'm guessing the read-all could be removed? |
jobs: | ||
label: | ||
name: Labeler | ||
runs-on: ubuntu-latest | ||
permissions: | ||
contents: read | ||
pull-requests: write |
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.
Is this the only line that's needed now that we've changed the default to read only?
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.
It is also considered a good practice to keep the minimal permission set on the workflow:
- It provides transparency in the permissions used since the settings are not public
- It works as a double prevention by preventing from insider risks (since users with admin access can revert the default permissions)
I personally would recommend to keep the minimal permission set on the workflow files either, but if you rather remove them I'd say that the only ones that should stay would be labeler.yml
and ci.yml
.
PS: ci.yml does not work only with contents and package read.
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.
considered a good practice to keep the minimal permission set on the workflow
Sounds good to me, thanks a lot Joyce!
@joycebrum JIC http://github.com/google/pywrapcc is also on your radar: I just changed the setting there as well and imported this PR (google/pybind11clif#30039). |
Description
Closes #4567
Run tests:
Suggested changelog entry: