-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Declare permissions #15493
Declare permissions #15493
Conversation
To make the Check change note workflow happy, please add |
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.
I looked at your fork of the repo, which has identical permissions and works 👍 thank you for the contribution!
b997f22
to
c5a047d
Compare
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.
Blocking merge until the other comments are addressed 😄
9da590e
to
301cfcc
Compare
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.
Thanks for contributing this. I like that this narrows the permissions of all of our tokens.
I think .github/workflows/csv-coverage-update.yml
is broken. I have a few other suggestions that will allow you to narrow the permissions. security-events
is only required if we are reading or writing SARIF to or from code scanning.
I do have a concern that these jobs haven't been run. Are you able to trigger them in your fork to make sure all permissions are correct?
What we do in other repos and can do here (but best to wait for a followup PR) is to add a chunk that ensures the workflow file is run whenever the workflow file itself is modified.
eg-
pull_request:
paths:
- '.github/workflows/csv-coverage-timeseries.yml'
(and similar for all other workflow files)
@aeisenberg: the design of these workflows is really painful.
|
301cfcc
to
0f2888b
Compare
uses: github/codeql-action/init@v2 | ||
uses: github/codeql-action/init@main |
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.
@aeisenberg says:
All of it really should be using
@main
since we want to test on the latest in case we break something.
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.
Partial review. I'm finding it hard to verify that this PR is correct.
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.
This is looking good to me, but since this change affects lots of files, I'd like someone else to approve as well.
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.
Some questions in comments. Also, I still see a bunch of security-events: read
permissions here: are those necessary? I see that they were able to be dropped from a few workflows because of the changes you'd made in the Action 😄
f93e918
to
62d530c
Compare
Repositories can be configured with Default access (restricted) https://docs.github.com/en/actions/security-guides/automatic-token-authentication#permissions-for-the-github_token Best practice says that workflows should declare the minimal permissions they require. Without declaring permissions, paranoid forks fail miserably.
62d530c
to
b58c856
Compare
Thank you for your patience and of course contributions @jsoref!! Merging now 💕 |
Repositories can be configured with Default access (restricted) https://docs.github.com/en/actions/security-guides/automatic-token-authentication#permissions-for-the-github_token
Best practice says that workflows should declare the minimal permissions they require. Without declaring permissions, paranoid forks fail miserably.
closes #15462