Actions: Fix dominates() false positive in reusable workflows

actions/ql/lib/change-notes/2026-06-15-permission_check.md

@@ -0,0 +1,4 @@
+---
+category: fix
+---
+* GitHub Actions queries now better account for permission checks on jobs that call reusable workflows.

actions/ql/lib/codeql/actions/security/ControlChecks.qll

@@ -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() {
@@ -53,43 +62,170 @@ abstract class ControlCheck extends AstNode {
 
   predicate protects(AstNode node, Event event, string category) {
     // The check dominates the step it should protect
-    this.dominates(node) and
+    this.dominates(node, event) and
     // The check is effective against the event and category
     this.protectsCategoryAndEvent(category, event.getName()) and
     // The check can be triggered by the event
-    this.getATriggerEvent() = event
+    this.getATriggerEvent() = event and
+    // For reusable workflows, there must be no unprotected caller chain for this event.
+    (
+      not node.getEnclosingWorkflow() instanceof ReusableWorkflow
+      or
+      this.dominatesSameWorkflow(node, event)
+      or
+      not exists(ExternalJob directCaller |
+        directCaller = node.getEnclosingWorkflow().(ReusableWorkflow).getACaller() and
+        unprotectedCallerChain(directCaller, event, category)
+      )
+    )
   }
 
-  predicate dominates(AstNode node) {
+  /**
+   * Holds if this control check must execute and pass before `node` can run.
+   */
+  predicate dominates(AstNode node, Event event) {
+    this.dominatesSameWorkflow(node, event)
+    or
+    // When the node is inside a reusable workflow,
+    // this check dominates via at least one caller chain.
+    this.dominatesViaCaller(node, event, _)
+  }
+
+  /**
+   * Holds if this control check dominates `node` within the same workflow.
+   */
+  predicate dominatesSameWorkflow(AstNode node, Event event) {
+    this.getATriggerEvent() = event and
+    (
+      // 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
+        node.getEnclosingJob().getIf() = this or
+        node.getEnclosingJob().getANeededJob().(LocalJob).getAStep().getIf() = this or
+        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
+      )
+    )
+  }
+
+  /**
+   * Holds if this control check dominates `node` in a reusable workflow
+   * via the caller chain starting at `directCaller`.
+   */
+  predicate dominatesViaCaller(AstNode node, Event event, ExternalJob directCaller) {
+    directCaller = node.getEnclosingWorkflow().(ReusableWorkflow).getACaller() and
+    directCaller.getATriggerEvent() = event and
+    exists(ExternalJob caller |
+      caller = getAnOuterCaller*(directCaller) and
+      this.dominatesCaller(caller)
+    )
+  }
+
+  /**
+   * Holds if this control check directly dominates `caller`.
+   */
+  predicate dominatesCaller(ExternalJob caller) {
     this instanceof If and
     (
-      node.getEnclosingStep().getIf() = this or
-      node.getEnclosingJob().getIf() = this or
-      node.getEnclosingJob().getANeededJob().(LocalJob).getAStep().getIf() = this or
-      node.getEnclosingJob().getANeededJob().(LocalJob).getIf() = this
+      caller.getIf() = this or
+      caller.getANeededJob().(LocalJob).getIf() = this or
+      caller.getANeededJob().(LocalJob).getAStep().getIf() = this
     )
     or
     this instanceof Environment and
     (
-      node.getEnclosingJob().getEnvironment() = this
-      or
-      node.getEnclosingJob().getANeededJob().getEnvironment() = this
+      caller.getEnvironment() = this or
+      caller.getANeededJob().getEnvironment() = this
     )
     or
-    (
-      this instanceof Run or
-      this instanceof UsesStep
-    ) and
-    (
-      this.(Step).getAFollowingStep() = node.getEnclosingStep()
-      or
-      node.getEnclosingJob().getANeededJob().(LocalJob).getAStep() = this.(Step)
-    )
+    (this instanceof Run or this instanceof UsesStep) and
+    caller.getANeededJob().(LocalJob).getAStep() = this
   }
 
   abstract predicate protectsCategoryAndEvent(string category, string event);
 }
 
+/**
+ * Holds if this control check directly protects `caller`.
+ */
+bindingset[caller, event, category]
+private predicate protectedCaller(ExternalJob caller, Event event, string category) {
+  exists(ControlCheck check |
+    check.protectsCategoryAndEvent(category, event.getName()) and
+    check.getATriggerEvent() = event and
+    check.dominatesCaller(caller)
+  )
+}
+
+cached
+private newtype TCallerState =
+  MkCallerState(ExternalJob caller, Event event, string category) {
+    caller.getATriggerEvent() = event and
+    category = any_category()
+  }
+
+private class CallerState extends TCallerState, MkCallerState {
+  ExternalJob caller;
+  Event event;
+  string category;
+
+  CallerState() { this = MkCallerState(caller, event, category) }
+
+  ExternalJob getCaller() { result = caller }
+
+  Event getEvent() { result = event }
+
+  string getCategory() { result = category }
+
+  /**
+   * Gets an outer caller state if this caller is not protected.
+   */
+  CallerState getUnprotectedOuterState() {
+    not protectedCaller(this.getCaller(), this.getEvent(), this.getCategory()) and
+    result = MkCallerState(getAnOuterCaller(this.getCaller()), this.getEvent(), this.getCategory())
+  }
+
+  predicate isUnprotectedOutermost() {
+    not protectedCaller(this.getCaller(), this.getEvent(), this.getCategory()) and
+    not exists(getAnOuterCaller(this.getCaller()))
+  }
+
+  string toString() { result = caller + " / " + event + " / " + category }
+}
+
+/**
+ * Holds if there is a caller path from `caller` to an outer workflow that has no protection.
+ */
+bindingset[caller, event, category]
+private predicate unprotectedCallerChain(ExternalJob caller, Event event, string category) {
+  exists(CallerState start, CallerState outermost |
+    start = MkCallerState(caller, event, category) and
+    outermost = start.getUnprotectedOuterState*() and
+    outermost.isUnprotectedOutermost()
+  )
+}
+
 abstract class AssociationCheck extends ControlCheck {
   // Checks if the actor is a MEMBER/OWNER the repo
   // - they are effective against pull requests and workflow_run (since these are triggered by pull_requests) since they can control who is making the PR

actions/ql/src/Security/CWE-285/ImproperAccessControl.ql

@@ -18,7 +18,7 @@ from LocalJob job, LabelCheck check, MutableRefCheckoutStep checkout, Event even
 where
   job.isPrivileged() and
   job.getAStep() = checkout and
-  check.dominates(checkout) and
+  check.dominates(checkout, event) and
   (
     job.getATriggerEvent() = event and
     event.getName() = "pull_request_target" and

actions/ql/src/Security/CWE-829/UntrustedCheckoutHigh.ql

@@ -34,8 +34,8 @@ where
         check instanceof AssociationCheck or
         check instanceof PermissionCheck
       ) and
-      check.dominates(checkout) and
-      date_check.dominates(checkout)
+      check.dominates(checkout, event) and
+      date_check.dominates(checkout, event)
     )
     or
     // not issue_comment triggered workflows

actions/ql/test/query-tests/Security/CWE-829/.github/workflows/external/TestOrg/TestRepo/.github/workflows/build.yml

@@ -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
+

actions/ql/test/query-tests/Security/CWE-829/.github/workflows/external/TestOrg/TestRepo/.github/workflows/build_nested.yml

@@ -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 }}
+
+

actions/ql/test/query-tests/Security/CWE-829/.github/workflows/external/TestOrg/TestRepo/.github/workflows/build_nested_branching.yml

@@ -0,0 +1,33 @@
+on:
+  workflow_call:
+    inputs:
+      COMMIT_SHA:
+        type: string
+
+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_safe:
+    needs: is-collaborator
+    uses: TestOrg/TestRepo/.github/workflows/build_nested.yml@main
+    with:
+      COMMIT_SHA: ${{ inputs.COMMIT_SHA }}
+  build_unsafe:
+    uses: TestOrg/TestRepo/.github/workflows/build_nested.yml@main
+    with:
+      COMMIT_SHA: ${{ inputs.COMMIT_SHA }}

actions/ql/test/query-tests/Security/CWE-829/.github/workflows/untrusted_checkout_no_needs.yml

@@ -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

actions/ql/test/query-tests/Security/CWE-829/.github/workflows/untrusted_checkout_permission_check_reusable.yml

@@ -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
+    with:
+      COMMIT_SHA: ${{ github.event.pull_request.head.sha }} # shouldn't alert since permission check

actions/ql/test/query-tests/Security/CWE-829/.github/workflows/untrusted_checkout_permission_check_reusable2.yml

@@ -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_unsafe:
+    # needs: is-collaborator
+    uses: TestOrg/TestRepo/.github/workflows/build.yml@main
+    with:
+      COMMIT_SHA: ${{ github.event.pull_request.head.sha }} # should alert since no permission check
+  build_safe:
+    needs: is-collaborator
+    uses: TestOrg/TestRepo/.github/workflows/build.yml@main
+    with:
+      COMMIT_SHA: ${{ github.event.pull_request.head.sha }} # shouldn't alert since permission check

actions/ql/test/query-tests/Security/CWE-829/.github/workflows/untrusted_checkout_permission_check_reusable_branching_nested.yml

@@ -0,0 +1,8 @@
+on:
+  pull_request_target:
+
+jobs:
+  build:
+    uses: TestOrg/TestRepo/.github/workflows/build_nested_branching.yml@main
+    with:
+      COMMIT_SHA: ${{ github.event.pull_request.head.sha }}