Skip to content

Set explicit contents: read permissions in CI workflow #36

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

Merged
merged 1 commit into from
Apr 17, 2025

Conversation

EliahKagan
Copy link
Member

@EliahKagan EliahKagan commented Apr 17, 2025

This specifies contents: read permissions only for ci.yml, since no other permissions are currently needed in it. In so doing, it avoids the ambiguous and sometimes greater than intended workflow permissions assigned if no permissions key is used.

This is similar in motivation to, though much simpler than, GitoxideLabs/gitoxide@f41a58c (GitoxideLabs/gitoxide#1668).


Although I worked on the workflow in #34, I did not notice that this was missing at that time. But I enabled experimental CodeQL queries for GitHub Actions workflows in my fork, which conveniently found this.

Although CodeQL can be enabled by writing a workflow that runs it (an "advanced" setup), I suggest enabling the default setup, which requires no workflow file, in this repository's security settings. CodeQL doesn't yet have Rust queries, so this would only be for GitHub Actions, for which I think it should not be necessary to go beyond the default setup. (You may also want to enable it in cargo-smart-release.)

By the way, I've found that, at least as things stand now, for GitHub Actions queries, the default setup is in practice more versatile than advanced setups in one important way. Like advanced setups, the default setup allows configuring which suite of queries to run. Since, unlike advanced setups, the default setup requires no workflow file, this makes it easy for an upstream repository and its forks to run different suites, without diverging. For example, I enabled the default setup with the default suite in gitoxide about 10 days ago, while also running the default setup with the security-extended suite in my fork, which includes queries that are more subjective in nature or that may carry more false positives.

Since no other permissions are currently needed in it.

This avoids the ambiguous and sometimes greater than intended
workflow permissions assigned if no `permissions` key is used. See:
https://github.com/github/codeql/blob/main/actions/ql/src/Security/CWE-275/MissingActionsPermissions.md

This is similar in motivation to, though much simpler than:
GitoxideLabs/gitoxide@f41a58c
Copy link
Member

@Byron Byron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for continuously improving the project 🙏!

@Byron Byron merged commit cff2029 into GitoxideLabs:main Apr 17, 2025
2 checks passed
@EliahKagan EliahKagan deleted the run-ci/workflow-permissions branch April 17, 2025 16:40
@EliahKagan
Copy link
Member Author

Thanks, and no problem! Since GitoxideLabs/cargo-smart-release#48 (comment), I've enabled CodeQL in cargo-smart-release with the default setup using the default query suite. (I am able to do that there even though I don't currently have write access because I have the security manager role in the GitoxideLabs organization and the default setup doesn't require a workflow file.) But I cannot do that here, so if you do want CodeQL for GitHub Actions here in the prodash repository then I think you would have to enable that.

@Byron
Copy link
Member

Byron commented Apr 18, 2025

As prodash is mainly used by gitoxide, even though it was created for criner (crates-io-miner), I decided to move it to the organisation, too. Now you have write permissions, and should be able to turn CodeQL on as well.

EliahKagan added a commit to EliahKagan/algorithms-python that referenced this pull request Apr 25, 2025
This resolves a CodeQL check and is also just a good idea.

See GitoxideLabs/prodash#36.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants