Actions: improve improper access control query #20904
Conversation
There was a problem hiding this comment.
Pull request overview
This PR improves the actions/improper-access-control query to detect either unsafe condition independently (synchronize trigger OR mutable reference checkout) rather than requiring both conditions to be present simultaneously.
Key Changes:
- Modified query logic to use broader
PRHeadCheckoutSteptype and add explicit check for mutable refs as a separate OR condition - Updated test files to demonstrate both scenarios: unsafe trigger with safe checkout (
bad_triggers.yml) and safe trigger with unsafe checkout (bad_checkout.yml) - Added a safe test case (
good.yml) demonstrating proper usage with both safe trigger and immutable reference
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
actions/ql/src/Security/CWE-285/ImproperAccessControl.ql |
Updated query to detect either unsafe triggers or mutable refs independently by changing checkout type to PRHeadCheckoutStep and adding mutable ref check as separate OR condition |
actions/ql/test/query-tests/Security/CWE-285/ImproperAccessControl.expected |
Updated expected test results to include both new test cases (bad_checkout.yml and bad_triggers.yml) |
actions/ql/test/query-tests/Security/CWE-285/.github/workflows/good.yml |
Added new test case demonstrating safe usage with labeled trigger and immutable SHA reference |
actions/ql/test/query-tests/Security/CWE-285/.github/workflows/bad_triggers.yml |
Updated comment to clarify this tests the unsafe synchronize trigger scenario |
actions/ql/test/query-tests/Security/CWE-285/.github/workflows/bad_checkout.yml |
Updated comment to clarify this tests the mutable reference scenario |
actions/ql/src/change-notes/2025-11-25-improve-improper-access-control.md |
Added release notes documenting the query improvement |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| category: minorAnalysis | ||
| --- | ||
| * The `actions/improper-access-control` query has been improved to correctly detect cases where either the check | ||
| triggers or the checkout reference are unsafe, rather than only when both applied as was done previously. |
There was a problem hiding this comment.
Grammar issue: "when both applied" should be "when both apply" or "when both were applied".
| triggers or the checkout reference are unsafe, rather than only when both applied as was done previously. | |
| triggers or the checkout reference are unsafe, rather than only when both apply as was done previously. |
| if: contains(github.event.pull_request.labels.*.name, 'safe to test') | ||
| with: | ||
| ref: ${{ github.event.pull_request.head.ref }} | ||
| ref: ${{ github.event.pull_request.head.ref }} # BAD (mutable ref) |
There was a problem hiding this comment.
Technically head.ref without a repo name isn’t exploitable (as it will just check out the branch from the base repo a fail if not present).
There was a problem hiding this comment.
I guess it still could be a privesc in very narrow scenarios where the repo enforces strict feature/main branch boundaries and a higher privilege group needs to approve the run (even though both have write access to the repo).
The query was firing an alert only when both unsafe conditions were met:
synchronizetriggerHowever, both these can cause problems alone. The query has thus been changed to detect either of the two, rather than both conditions at the same time.
Closes: #20706