Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions actions/ql/lib/change-notes/2026-06-15-permission_check.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
---
category: fix
---
* GitHub Actions support for reusable workflows was improved.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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:

Suggested change
* GitHub Actions support for reusable workflows was improved.
* GitHub Actions queries now better account for permission checks on jobs that call reusable workflows.

43 changes: 42 additions & 1 deletion actions/ql/lib/codeql/actions/security/ControlChecks.qll
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,15 @@ string actor_not_attacker_event() {
]
}

/**
* Gets the outer caller of `ej`, i.e. the `ExternalJob` that calls the
* reusable workflow containing `ej`. Used with transitive closure to
* walk up nested reusable workflow chains.
*/
private ExternalJob getAnOuterCaller(ExternalJob ej) {
result = ej.getEnclosingWorkflow().(ReusableWorkflow).getACaller()
}

/** An If node that contains an actor, user or label check */
abstract class ControlCheck extends AstNode {
ControlCheck() {
Expand All @@ -60,7 +69,12 @@ abstract class ControlCheck extends AstNode {
this.getATriggerEvent() = event
}

/**
* Holds if this control check must execute and pass before `node` can run.
*/
predicate dominates(AstNode node) {
// Step-level: the check is an `if:` on the step containing `node`,
// or on the enclosing job, or on a needed job/step.
this instanceof If and
(
node.getEnclosingStep().getIf() = this or
Expand All @@ -69,21 +83,48 @@ abstract class ControlCheck extends AstNode {
node.getEnclosingJob().getANeededJob().(LocalJob).getIf() = this
)
or
// Job-level: the check is an environment on the enclosing job or a needed job.
this instanceof Environment and
(
node.getEnclosingJob().getEnvironment() = this
or
node.getEnclosingJob().getANeededJob().getEnvironment() = this
)
or
// Step-level: the check is a Run/UsesStep that precedes `node`'s step
// in the same job, or is a step in a needed job.
(
this instanceof Run or
this instanceof UsesStep
) and
(
this.(Step).getAFollowingStep() = node.getEnclosingStep()
or
node.getEnclosingJob().getANeededJob().(LocalJob).getAStep() = this.(Step)
node.getEnclosingJob().getANeededJob().(LocalJob).getAStep() = this
)
or
// When the node is inside a (possibly nested) reusable workflow,
// 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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

(
this instanceof If and
(
caller.getIf() = this or
caller.getANeededJob().(LocalJob).getIf() = this or
caller.getANeededJob().(LocalJob).getAStep().getIf() = this
)
or
this instanceof Environment and
(
caller.getEnvironment() = this or
caller.getANeededJob().getEnvironment() = this
)
or
(this instanceof Run or this instanceof UsesStep) and
caller.getANeededJob().(LocalJob).getAStep() = this
)
)
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
on:
workflow_call:
inputs:
COMMIT_SHA:
type: string

jobs:
build:
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v6
with:
ref: ${{ inputs.COMMIT_SHA }}
- run: |
npm install
npm run lint
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
on:
workflow_call:
inputs:
COMMIT_SHA:
type: string

jobs:
build:
uses: TestOrg/TestRepo/.github/workflows/build.yml@main
with:
COMMIT_SHA: ${{ inputs.COMMIT_SHA }}


Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
on:
pull_request_target:

jobs:
is-collaborator:
runs-on: ubuntu-latest
steps:
- name: Get User Permission
id: checkAccess
uses: actions-cool/check-user-permission@cd622002ff25c2311d2e7fb82107c0d24be83f9b
with:
require: write
username: ${{ github.actor }}
env:
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
- name: Check User Permission
if: steps.checkAccess.outputs.require-result == 'false'
run: |
echo "${{ github.actor }} does not have permissions on this repo."
echo "Current permission level is ${{ steps.checkAccess.outputs.user-permission }}"
exit 1
build:
runs-on: ubuntu-latest
#needs: is-collaborator Mistake, doesn't wait for the collaborator - no security check
steps:
- name: Checkout repo
uses: actions/checkout@4
with:
ref: ${{ github.event.pull_request.head.sha }} # should alert
fetch-depth: 2
- run: yarn test
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
on:
pull_request_target:

jobs:
is-collaborator:
runs-on: ubuntu-latest
steps:
- name: Get User Permission
id: checkAccess
uses: actions-cool/check-user-permission@cd622002ff25c2311d2e7fb82107c0d24be83f9b
with:
require: write
username: ${{ github.actor }}
env:
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
- name: Check User Permission
if: steps.checkAccess.outputs.require-result == 'false'
run: |
echo "${{ github.actor }} does not have permissions on this repo."
echo "Current permission level is ${{ steps.checkAccess.outputs.user-permission }}"
exit 1
build:
needs: is-collaborator
uses: TestOrg/TestRepo/.github/workflows/build.yml@main

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

with:
COMMIT_SHA: ${{ github.event.pull_request.head.sha }} # shouldn't alert since permission check
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
on:
pull_request_target:

jobs:
is-collaborator:
runs-on: ubuntu-latest
steps:
- name: Get User Permission
id: checkAccess
uses: actions-cool/check-user-permission@cd622002ff25c2311d2e7fb82107c0d24be83f9b
with:
require: write
username: ${{ github.actor }}
env:
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
- name: Check User Permission
if: steps.checkAccess.outputs.require-result == 'false'
run: |
echo "${{ github.actor }} does not have permissions on this repo."
echo "Current permission level is ${{ steps.checkAccess.outputs.user-permission }}"
exit 1
build:
needs: is-collaborator
uses: TestOrg/TestRepo/.github/workflows/build_nested.yml@main
with:
COMMIT_SHA: ${{ github.event.pull_request.head.sha }} # shouldn't alert since permission check
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
on:
pull_request_target:

jobs:
is-collaborator:
runs-on: ubuntu-latest
steps:
- name: Get User Permission
id: checkAccess
uses: actions-cool/check-user-permission@cd622002ff25c2311d2e7fb82107c0d24be83f9b
with:
require: write
username: ${{ github.actor }}
env:
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
- name: Check User Permission
if: steps.checkAccess.outputs.require-result == 'false'
run: |
echo "${{ github.actor }} does not have permissions on this repo."
echo "Current permission level is ${{ steps.checkAccess.outputs.user-permission }}"
exit 1
build:
# needs: is-collaborator
uses: TestOrg/TestRepo/.github/workflows/build_nested.yml@main
with:
COMMIT_SHA: ${{ github.event.pull_request.head.sha }}
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
on:
pull_request_target:

jobs:
is-collaborator:
runs-on: ubuntu-latest
steps:
- name: Get User Permission
id: checkAccess
uses: actions-cool/check-user-permission@cd622002ff25c2311d2e7fb82107c0d24be83f9b
with:
require: write
username: ${{ github.actor }}
env:
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
- name: Check User Permission
if: steps.checkAccess.outputs.require-result == 'false'
run: |
echo "${{ github.actor }} does not have permissions on this repo."
echo "Current permission level is ${{ steps.checkAccess.outputs.user-permission }}"
exit 1
build:
runs-on: ubuntu-latest
needs: is-collaborator
steps:
- name: Checkout repo
uses: actions/checkout@4
with:
ref: ${{ github.event.pull_request.head.sha }} # shouldn't alert since permission check
fetch-depth: 2
- run: yarn test
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,7 @@ edges
| .github/workflows/dependabot3.yml:15:9:20:6 | Uses Step | .github/workflows/dependabot3.yml:20:9:25:6 | Uses Step |
| .github/workflows/dependabot3.yml:20:9:25:6 | Uses Step | .github/workflows/dependabot3.yml:25:9:48:6 | Run Step: set-milestone |
| .github/workflows/dependabot3.yml:25:9:48:6 | Run Step: set-milestone | .github/workflows/dependabot3.yml:48:9:52:57 | Run Step |
| .github/workflows/external/TestOrg/TestRepo/.github/workflows/build.yml:11:9:14:6 | Uses Step | .github/workflows/external/TestOrg/TestRepo/.github/workflows/build.yml:14:9:17:7 | Run Step |
| .github/workflows/external/TestOrg/TestRepo/.github/workflows/formal.yml:14:9:19:6 | Uses Step | .github/workflows/external/TestOrg/TestRepo/.github/workflows/formal.yml:19:9:25:6 | Run Step |
| .github/workflows/external/TestOrg/TestRepo/.github/workflows/formal.yml:19:9:25:6 | Run Step | .github/workflows/external/TestOrg/TestRepo/.github/workflows/formal.yml:25:9:70:20 | Run Step |
| .github/workflows/external/TestOrg/TestRepo/.github/workflows/reusable.yml:23:9:26:6 | Uses Step | .github/workflows/external/TestOrg/TestRepo/.github/workflows/reusable.yml:26:9:29:7 | Run Step |
Expand Down Expand Up @@ -334,6 +335,13 @@ edges
| .github/workflows/untrusted_checkout_6.yml:11:9:14:6 | Uses Step | .github/workflows/untrusted_checkout_6.yml:14:9:17:6 | Uses Step |
| .github/workflows/untrusted_checkout_6.yml:14:9:17:6 | Uses Step | .github/workflows/untrusted_checkout_6.yml:17:9:21:6 | Uses Step |
| .github/workflows/untrusted_checkout_6.yml:17:9:21:6 | Uses Step | .github/workflows/untrusted_checkout_6.yml:21:9:23:23 | Run Step |
| .github/workflows/untrusted_checkout_no_needs.yml:8:9:16:6 | Uses Step: checkAccess | .github/workflows/untrusted_checkout_no_needs.yml:16:9:22:2 | Run Step |
| .github/workflows/untrusted_checkout_no_needs.yml:26:9:31:6 | Uses Step | .github/workflows/untrusted_checkout_no_needs.yml:31:9:31:23 | Run Step |
| .github/workflows/untrusted_checkout_permission_check_reusable.yml:8:9:16:6 | Uses Step: checkAccess | .github/workflows/untrusted_checkout_permission_check_reusable.yml:16:9:22:2 | Run Step |
| .github/workflows/untrusted_checkout_permission_check_reusable_level2.yml:8:9:16:6 | Uses Step: checkAccess | .github/workflows/untrusted_checkout_permission_check_reusable_level2.yml:16:9:22:2 | Run Step |
| .github/workflows/untrusted_checkout_permission_check_reusable_no_needs.yml:8:9:16:6 | Uses Step: checkAccess | .github/workflows/untrusted_checkout_permission_check_reusable_no_needs.yml:16:9:22:2 | Run Step |
| .github/workflows/untrusted_checkout_permissions_check.yml:8:9:16:6 | Uses Step: checkAccess | .github/workflows/untrusted_checkout_permissions_check.yml:16:9:22:2 | Run Step |
| .github/workflows/untrusted_checkout_permissions_check.yml:26:9:31:6 | Uses Step | .github/workflows/untrusted_checkout_permissions_check.yml:31:9:31:23 | Run Step |
| .github/workflows/workflow_run_untrusted_checkout.yml:13:9:16:6 | Uses Step | .github/workflows/workflow_run_untrusted_checkout.yml:16:9:18:31 | Uses Step |
| .github/workflows/workflow_run_untrusted_checkout_2.yml:13:9:16:6 | Uses Step | .github/workflows/workflow_run_untrusted_checkout_2.yml:16:9:18:31 | Uses Step |
| .github/workflows/workflow_run_untrusted_checkout_3.yml:13:9:16:6 | Uses Step | .github/workflows/workflow_run_untrusted_checkout_3.yml:16:9:18:31 | Uses Step |
Expand All @@ -344,6 +352,7 @@ edges
| .github/workflows/auto_ci.yml:67:9:74:6 | Uses Step | .github/workflows/auto_ci.yml:67:9:74:6 | Uses Step | .github/workflows/auto_ci.yml:79:9:84:6 | Run Step | Checkout of untrusted code in a privileged workflow with later potential execution (event trigger: $@). | .github/workflows/auto_ci.yml:6:3:6:21 | pull_request_target | pull_request_target |
| .github/workflows/auto_ci.yml:67:9:74:6 | Uses Step | .github/workflows/auto_ci.yml:67:9:74:6 | Uses Step | .github/workflows/auto_ci.yml:84:9:93:6 | Run Step | Checkout of untrusted code in a privileged workflow with later potential execution (event trigger: $@). | .github/workflows/auto_ci.yml:6:3:6:21 | pull_request_target | pull_request_target |
| .github/workflows/dependabot3.yml:15:9:20:6 | Uses Step | .github/workflows/dependabot3.yml:15:9:20:6 | Uses Step | .github/workflows/dependabot3.yml:25:9:48:6 | Run Step: set-milestone | Checkout of untrusted code in a privileged workflow with later potential execution (event trigger: $@). | .github/workflows/dependabot3.yml:3:5:3:23 | pull_request_target | pull_request_target |
| .github/workflows/external/TestOrg/TestRepo/.github/workflows/build.yml:11:9:14:6 | Uses Step | .github/workflows/external/TestOrg/TestRepo/.github/workflows/build.yml:11:9:14:6 | Uses Step | .github/workflows/external/TestOrg/TestRepo/.github/workflows/build.yml:14:9:17:7 | Run Step | Checkout of untrusted code in a privileged workflow with later potential execution (event trigger: $@). | .github/workflows/untrusted_checkout_permission_check_reusable_no_needs.yml:2:3:2:21 | pull_request_target | pull_request_target |
| .github/workflows/external/TestOrg/TestRepo/.github/workflows/reusable.yml:23:9:26:6 | Uses Step | .github/workflows/external/TestOrg/TestRepo/.github/workflows/reusable.yml:23:9:26:6 | Uses Step | .github/workflows/external/TestOrg/TestRepo/.github/workflows/reusable.yml:26:9:29:7 | Run Step | Checkout of untrusted code in a privileged workflow with later potential execution (event trigger: $@). | .github/workflows/reusable_caller1.yaml:4:3:4:21 | pull_request_target | pull_request_target |
| .github/workflows/gitcheckout.yml:10:11:18:8 | Run Step | .github/workflows/gitcheckout.yml:10:11:18:8 | Run Step | .github/workflows/gitcheckout.yml:21:11:23:22 | Run Step | Checkout of untrusted code in a privileged workflow with later potential execution (event trigger: $@). | .github/workflows/gitcheckout.yml:2:3:2:21 | pull_request_target | pull_request_target |
| .github/workflows/label_trusted_checkout2.yml:12:7:16:4 | Uses Step | .github/workflows/label_trusted_checkout2.yml:12:7:16:4 | Uses Step | .github/workflows/label_trusted_checkout2.yml:17:7:21:4 | Run Step | Checkout of untrusted code in a privileged workflow with later potential execution (event trigger: $@). | .github/workflows/label_trusted_checkout2.yml:2:3:2:21 | pull_request_target | pull_request_target |
Expand Down Expand Up @@ -377,3 +386,4 @@ edges
| .github/workflows/untrusted_checkout4.yml:29:7:35:4 | Uses Step | .github/workflows/untrusted_checkout4.yml:29:7:35:4 | Uses Step | .github/workflows/untrusted_checkout4.yml:47:7:51:46 | Run Step | Checkout of untrusted code in a privileged workflow with later potential execution (event trigger: $@). | .github/workflows/untrusted_checkout4.yml:2:3:2:15 | issue_comment | issue_comment |
| .github/workflows/untrusted_checkout.yml:8:9:11:6 | Uses Step | .github/workflows/untrusted_checkout.yml:8:9:11:6 | Uses Step | .github/workflows/untrusted_checkout.yml:15:9:18:2 | Run Step | Checkout of untrusted code in a privileged workflow with later potential execution (event trigger: $@). | .github/workflows/untrusted_checkout.yml:2:3:2:21 | pull_request_target | pull_request_target |
| .github/workflows/untrusted_checkout.yml:23:9:26:6 | Uses Step | .github/workflows/untrusted_checkout.yml:23:9:26:6 | Uses Step | .github/workflows/untrusted_checkout.yml:30:9:32:23 | Run Step | Checkout of untrusted code in a privileged workflow with later potential execution (event trigger: $@). | .github/workflows/untrusted_checkout.yml:2:3:2:21 | pull_request_target | pull_request_target |
| .github/workflows/untrusted_checkout_no_needs.yml:26:9:31:6 | Uses Step | .github/workflows/untrusted_checkout_no_needs.yml:26:9:31:6 | Uses Step | .github/workflows/untrusted_checkout_no_needs.yml:31:9:31:23 | Run Step | Checkout of untrusted code in a privileged workflow with later potential execution (event trigger: $@). | .github/workflows/untrusted_checkout_no_needs.yml:2:3:2:21 | pull_request_target | pull_request_target |
Loading