Actions: Fix dominates() false positive in reusable workflows#21986
Actions: Fix dominates() false positive in reusable workflows#21986JarLob wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Updates the CWE-829 “Untrusted Checkout (Critical)” query logic and fixtures to better recognize permission checks (including across reusable workflows) and to validate expected results.
Changes:
- Extend
ControlCheckdominance logic to account for checks that occur in the caller of a reusable workflow. - Add new GitHub Actions workflow fixtures covering permission-check patterns (and a “missing needs” negative case).
- Update the expected test results to include the new fixtures/edges.
Show a summary per file
| File | Description |
|---|---|
| actions/ql/lib/codeql/actions/security/ControlChecks.qll | Adds reusable-workflow caller coverage to control-check dominance logic. |
| actions/ql/test/query-tests/Security/CWE-829/UntrustedCheckoutCritical.expected | Updates expected edges/results for new workflow fixtures. |
| actions/ql/test/query-tests/Security/CWE-829/.github/workflows/untrusted_checkout_permissions_check.yml | Adds fixture where collaborator permission check precedes checkout. |
| actions/ql/test/query-tests/Security/CWE-829/.github/workflows/untrusted_checkout_permission_check_reusable.yml | Adds fixture where permission check precedes a reusable workflow call. |
| actions/ql/test/query-tests/Security/CWE-829/.github/workflows/untrusted_checkout_no_needs.yml | Adds negative fixture where the permission-check job exists but is not required by the checkout job. |
| actions/ql/test/query-tests/Security/CWE-829/.github/workflows/external/TestOrg/TestRepo/.github/workflows/build.yml | Adds external reusable workflow fixture used by the tests. |
| actions/ql/lib/change-notes/2026-06-15-permission_check.md | Adds changelog entry for the reusable-workflow support fix. |
Copilot's findings
- Files reviewed: 7/7 changed files
- Comments generated: 1
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
owen-mc
changed the title
redsun82
left a comment
redsun82
left a comment
There was a problem hiding this comment.
👋 thanks for this contribution, I think this is going in the right direction! Currently this is relaxing the check a little too widely though, if I got it right. It seems one caller having the correct permission check can hide other callers being unsafe. Could you double check that, and add it to the tests after fixing if I got that right?
| // check if the control check dominates any caller job in the chain. | ||
| exists(ExternalJob directCaller, ExternalJob caller | | ||
| directCaller = node.getEnclosingWorkflow().(ReusableWorkflow).getACaller() and | ||
| caller = getAnOuterCaller*(directCaller) and |
There was a problem hiding this comment.
I think this might be too broad when the same reusable workflow has multiple callers. Since dominates only takes the reusable-workflow node, this exists caller can make a check from one protected caller count as protection for the reusable workflow as a whole.
I've cooked up a scratch qltest with two callers to the same build.yml, one with needs: is-collaborator and one without. It turns out the unprotected caller does not produce an alert, because the protected caller is enough to satisfy this predicate. I guess we need to keep the protection tied to the specific caller/call path that produced the event, rather than any caller of the reusable workflow.
| exit 1 | ||
| build: | ||
| needs: is-collaborator | ||
| uses: TestOrg/TestRepo/.github/workflows/build.yml@main |
There was a problem hiding this comment.
Could we also add a regression for the mixed-caller case? Something like this protected caller plus another job in the same triggering workflow that calls the same reusable workflow without needs: is-collaborator.
That seems to be the tricky case here, because otherwise the reusable workflow node can look protected just because one of its callers is protected.
| --- | ||
| category: fix | ||
| --- | ||
| * GitHub Actions support for reusable workflows was improved. No newline at end of file |
There was a problem hiding this comment.
nit: maybe we could make this a little tighter? This sounds like a general reusable-workflow support improvement, while the change is more specifically about recognizing control checks around jobs that call reusable workflows.
Maybe something like:
| * GitHub Actions support for reusable workflows was improved. | |
| * GitHub Actions queries now better account for permission checks on jobs that call reusable workflows. |
No description provided.