-
Notifications
You must be signed in to change notification settings - Fork 1.8k
actions: add MaD model for permissions needed by actions #19166
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
actions: add MaD model for permissions needed by actions #19166
Conversation
Use this to suggest minimal set of nedded permissions
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 pull request introduces a new MaD model to specify the minimal permissions required for actions, supporting the permission lookup for CodeQL queries.
- Added a new test workflow (perms6.yml) for a CWE-275 scenario.
- Created a configuration file (actions_permissions.yml) listing required permissions for various actions along with TODO comments for pending additions.
Reviewed Changes
Copilot reviewed 2 out of 8 changed files in this pull request and generated no comments.
File | Description |
---|---|
actions/ql/test/query-tests/Security/CWE-275/.github/workflows/perms6.yml | Introduces a new workflow to test permission configuration. |
actions/ql/lib/ext/config/actions_permissions.yml | Defines a new data model for action permissions with minimal required scopes. |
Files not reviewed (6)
- actions/ql/lib/codeql/actions/Ast.qll: Language not supported
- actions/ql/lib/codeql/actions/ast/internal/Ast.qll: Language not supported
- actions/ql/lib/codeql/actions/config/Config.qll: Language not supported
- actions/ql/lib/codeql/actions/config/ConfigExtensions.qll: Language not supported
- actions/ql/src/Security/CWE-275/MissingActionsPermissions.ql: Language not supported
- actions/ql/test/query-tests/Security/CWE-275/MissingActionsPermissions.expected: Language not supported
Tip: Copilot code review supports C#, Go, Java, JavaScript, Markdown, Python, Ruby and TypeScript, with more languages coming soon. Learn more
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.
Nice work. I like how it's come together and the simple strategy of including the recommendation in the alert message. Minor suggestions only.
Before merging, let's test internally how well autofix does with the new information in the alert message.
Co-authored-by: Aditya Sharad <6874315+adityasharad@users.noreply.github.com>
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.
Overall looks good. Comment suggestions only.
Ah forgot to mention - change note in the query pack please. Something like "Alerts produced by the query |
Co-authored-by: Aditya Sharad <6874315+adityasharad@users.noreply.github.com>
Use this to suggest minimal set of needed permissions.
For review: Do the permissions depend on the version of the actions? Currently, the version is stripped before the permissions are looked up.