Declare permissions - #15493
Conversation
Sorry, something went wrong.
|
To make the Check change note workflow happy, please add label:no-change-note-required |
Sorry, something went wrong.
angelapwen
left a comment
There was a problem hiding this comment.
I looked at your fork of the repo, which has identical permissions and works 👍 thank you for the contribution!
Sorry, something went wrong.
angelapwen
left a comment
There was a problem hiding this comment.
Blocking merge until the other comments are addressed 😄
Sorry, something went wrong.
aeisenberg
left a comment
There was a problem hiding this comment.
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)
Sorry, something went wrong.
|
@aeisenberg: the design of these workflows is really painful.
|
Sorry, something went wrong.
| uses: github/codeql-action/init@v2 | ||
| uses: github/codeql-action/init@main |
There was a problem hiding this comment.
@aeisenberg says:
All of it really should be using @main since we want to test on the latest in case we break something.
Sorry, something went wrong.
aeisenberg
left a comment
There was a problem hiding this comment.
Partial review. I'm finding it hard to verify that this PR is correct.
Sorry, something went wrong.
aeisenberg
left a comment
There was a problem hiding this comment.
This is looking good to me, but since this change affects lots of files, I'd like someone else to approve as well.
Sorry, something went wrong.
angelapwen
left a comment
There was a problem hiding this comment.
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 😄
Sorry, something went wrong.
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.
|
Thank you for your patience and of course contributions @jsoref!! Merging now 💕 |
Sorry, something went wrong.
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