Skip to content

Actions: Fix actions cache poisoning queries to account for the latest changes in cache write access - #22260

Open
JarLob wants to merge 3 commits into
github:mainfrom
JarLob:fix/actions-cache-poisoning-cache-write-access
Open

Actions: Fix actions cache poisoning queries to account for the latest changes in cache write access#22260
JarLob wants to merge 3 commits into
github:mainfrom
JarLob:fix/actions-cache-poisoning-cache-write-access

Conversation

@JarLob

@JarLob JarLob commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

JarLob and others added 2 commits July 30, 2026 10:50
Bind event-property and event-context matching to source expressions so code-injection queries avoid materializing global source/event relations.
Copilot AI review requested due to automatic review settings July 30, 2026 16:13
@JarLob
JarLob requested a review from a team as a code owner July 30, 2026 16:13
@github-actions github-actions Bot added documentation Actions Analysis of GitHub Actions labels Jul 30, 2026
Comment thread actions/ql/lib/codeql/actions/dataflow/FlowSources.qll Fixed
Comment thread actions/ql/lib/codeql/actions/dataflow/FlowSources.qll Fixed
Comment thread actions/ql/lib/codeql/actions/dataflow/FlowSources.qll Fixed
Comment thread actions/ql/lib/codeql/actions/dataflow/FlowSources.qll Fixed

Copilot AI left a comment

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.

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Updates the GitHub Actions cache-poisoning CodeQL queries to align results with GitHub’s default-branch cache write-access rules, reducing findings for low-trust triggers that are now read-only.

Changes:

  • Replace “runs on default branch” conditions with hasDefaultBranchCacheWriteAccess(...) across cache-poisoning queries.
  • Update/add test workflows and expected outputs to reflect new trigger/write-access behavior.
  • Refine dataflow source detection and update query documentation + change note.
Show a summary per file
File Description
actions/ql/test/query-tests/Security/CWE-349/CachePoisoningViaPoisonableStep.expected Updates expected results/edges to match new cache write-access filtering.
actions/ql/test/query-tests/Security/CWE-349/CachePoisoningViaDirectCache.expected Updates expected results/edges to match new cache write-access filtering.
actions/ql/test/query-tests/Security/CWE-349/CachePoisoningViaCodeInjection.expected Updates expected results to match event-aligned source/sink behavior.
actions/ql/test/query-tests/Security/CWE-349/.github/workflows/cache_write_capable_workflow_dispatch.yml Adds a workflow_dispatch scenario intended to be cache-write capable in default-branch scope.
actions/ql/test/query-tests/Security/CWE-349/.github/workflows/cache_write_capable_push.yml Adds a push-to-main scenario intended to exercise code-injection → cache-poisoning logic.
actions/ql/src/change-notes/2026-07-18-read-only-default-branch-cache.md Documents the analysis behavior change for the three cache-poisoning queries.
actions/ql/src/Security/CWE-349/CachePoisoningViaPoisonableStep.ql Switches to hasDefaultBranchCacheWriteAccess gating.
actions/ql/src/Security/CWE-349/CachePoisoningViaPoisonableStep.md Updates explanation/examples to reflect read-only cache access on low-trust triggers.
actions/ql/src/Security/CWE-349/CachePoisoningViaDirectCache.ql Switches to hasDefaultBranchCacheWriteAccess gating.
actions/ql/src/Security/CWE-349/CachePoisoningViaDirectCache.md Updates explanation/examples to reflect new write-access rules and provides new unsafe example.
actions/ql/src/Security/CWE-349/CachePoisoningViaCodeInjection.ql Adds an event-name alignment constraint between flow source and relevant cache-poisoning event.
actions/ql/src/Security/CWE-349/CachePoisoningViaCodeInjection.md Updates explanation/examples to reflect read-only cache access on low-trust triggers.
actions/ql/lib/codeql/actions/security/CodeInjectionQuery.qll Updates “relevant cache poisoning event” logic to use cache write-access predicate.
actions/ql/lib/codeql/actions/security/CachePoisoningQuery.qll Introduces default-branch cache write-event modeling + hasDefaultBranchCacheWriteAccess.
actions/ql/lib/codeql/actions/dataflow/FlowSources.qll Refactors event-context/json source detection into helper predicates.

Review details

  • Files reviewed: 15/15 changed files
  • Comments generated: 4
  • Review effort level: Low

Comment thread actions/ql/src/Security/CWE-349/CachePoisoningViaCodeInjection.ql

Copilot AI left a comment

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.

Review details

Comments suppressed due to low confidence (6)

actions/ql/src/Security/CWE-349/CachePoisoningViaPoisonableStep.md:65

  • The YAML example runs git fetch origin ... / git checkout ... without first checking out or otherwise initializing the repository (no actions/checkout and no git init + git remote add origin ...). As written, the example won’t work on a fresh runner. Add an initial repository checkout/clone step (or explicit git initialization) before the fetch/checkout so the example is runnable and matches the intended scenario.
on:
  workflow_dispatch:
    inputs:
      head_sha:
        required: true
jobs:
  test:
    permissions: {}
    runs-on: ubuntu-latest
    steps:
      - env:
          HEAD_SHA: ${{ github.event.inputs.head_sha }}
        run: |
          git fetch origin "$HEAD_SHA"
          git checkout "$HEAD_SHA"
      - name: Run tests
        run: npm install

actions/ql/src/Security/CWE-349/CachePoisoningViaDirectCache.md:69

  • Same issue as the other doc example: this snippet fetches/checks out a SHA without establishing a local git repository or an origin remote first. Add an explicit checkout/clone (or git init + remote setup) step before git fetch to make the example executable and unambiguous.
on:
  workflow_dispatch:
    inputs:
      head_sha:
        required: true

jobs:
  cache:
    permissions: read-all
    runs-on: ubuntu-latest
    steps:
      - env:
          HEAD_SHA: ${{ github.event.inputs.head_sha }}
        run: |
          git fetch origin "$HEAD_SHA"
          git checkout "$HEAD_SHA"
      - name: Cache fetched files
        uses: actions/cache@v4
        with:
          path: .
          key: dispatched-${{ github.event.inputs.head_sha }}

actions/ql/test/query-tests/Security/CWE-349/.github/workflows/cache_write_capable_workflow_dispatch.yml:14

  • Setting permissions: {} at the job level removes all GITHUB_TOKEN permissions, which can break actions/checkout (typically requires at least contents: read). Even though this is a test fixture, it’s helpful for fixtures to be runnable/representative—consider specifying minimal required permissions (for example contents: read) instead of an empty map.
  cache:
    permissions: {}
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v4

actions/ql/lib/codeql/actions/security/CachePoisoningQuery.qll:51

  • The helper returns multiple event names (a set via multiple result bindings), but the name defaultBranchCacheWriteEvent reads like a single value. Renaming to something plural (for example defaultBranchCacheWriteEvents) would make the intent clearer and reduce confusion with defaultBranchTriggerEvent().
private string defaultBranchCacheWriteEvent() {
  result =
    [
      "push", "workflow_dispatch", "repository_dispatch", "delete", "registry_package",
      "page_build", "schedule"
    ]
}

actions/ql/src/Security/CWE-349/CachePoisoningViaCodeInjection.ql:28

  • This uses a type-cast source.getNode().(RemoteFlowSource) as a filter. If the CodeInjection flow config ever admits non-RemoteFlowSource nodes as source, this will silently drop those paths. Consider rewriting this constraint using an explicit exists(RemoteFlowSource rfs | rfs = source.getNode() and ...) (or equivalent) to make the filtering intent explicit and more robust to future changes.
where
  CodeInjectionFlow::flowPath(source, sink) and
  event = getRelevantCachePoisoningEventForSink(sink.getNode()) and
  source.getNode().(RemoteFlowSource).getEventName() = event.getName() and
  // the checkout is not controlled by an access check
  not exists(ControlCheck check |
    check.protects(source.getNode().asExpr(), event, "code-injection")

actions/ql/src/Security/CWE-349/CachePoisoningViaPoisonableStep.ql:43

  • hasDefaultBranchCacheWriteAccess(job, event) already includes job.getATriggerEvent() = event. Keeping the separate job.getATriggerEvent() = event constraint here is redundant and makes the condition harder to read/maintain. Consider removing the duplicate constraint (or changing hasDefaultBranchCacheWriteAccess to only take job and derive the event internally).
  job.getATriggerEvent() = event and
  // job can be triggered by an external user
  event.isExternallyTriggerable() and
  hasDefaultBranchCacheWriteAccess(job, event) and
  • Files reviewed: 15/15 changed files
  • Comments generated: 0 new
  • Review effort level: Low

@JarLob JarLob changed the title Actions: Fix/actions cache poisoning queries to account for the latest changes in cache write access Actions: Fix actions cache poisoning queries to account for the latest changes in cache write access Jul 30, 2026
@JarLob
JarLob requested a review from Copilot July 30, 2026 16:56

Copilot AI left a comment

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.

Review details

Comments suppressed due to low confidence (4)

actions/ql/src/Security/CWE-349/CachePoisoningViaCodeInjection.ql:25

  • This hard cast to RemoteFlowSource will silently drop any paths where the source node is not a RemoteFlowSource, potentially introducing false negatives. Consider rewriting this as an exists(RemoteFlowSource rfs | rfs = source.getNode() and rfs.getEventName() = event.getName()) constraint (or guarding with an instanceof check) so the query remains robust to additional source kinds.
  source.getNode().(RemoteFlowSource).getEventName() = event.getName() and

actions/ql/test/query-tests/Security/CWE-349/.github/workflows/cache_write_capable_workflow_dispatch.yml:14

  • permissions: {} removes contents: read, which actions/checkout requires to fetch repository contents; as written, this fixture workflow would fail if executed. Even for a test fixture, it’s helpful to keep examples runnable—consider setting minimal permissions (for example contents: read) or removing the explicit empty permissions block.
  cache:
    permissions: {}
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v4

actions/ql/src/Security/CWE-349/CachePoisoningViaPoisonableStep.md:63

  • The ‘Incorrect Usage’ example runs git fetch/checkout without first checking out (or initializing) a repository, so it won’t work as a standalone workflow snippet for readers. To keep the example runnable while still demonstrating the vulnerability, add an initial actions/checkout step (or git init + git remote add origin ...) before the fetch.
    steps:
      - env:
          HEAD_SHA: ${{ github.event.inputs.head_sha }}
        run: |
          git fetch origin "$HEAD_SHA"
          git checkout "$HEAD_SHA"

actions/ql/src/Security/CWE-349/CachePoisoningViaDirectCache.md:64

  • Similarly here, the example uses git fetch/checkout without first checking out or initializing a repository, which makes the snippet non-functional for readers. Add an actions/checkout step (or equivalent repo initialization) before the git commands so the example can be followed as-is.
    steps:
      - env:
          HEAD_SHA: ${{ github.event.inputs.head_sha }}
        run: |
          git fetch origin "$HEAD_SHA"
          git checkout "$HEAD_SHA"
  • Files reviewed: 15/15 changed files
  • Comments generated: 0 new
  • Review effort level: Low

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Actions Analysis of GitHub Actions documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants