Actions: Fix actions cache poisoning queries to account for the latest changes in cache write access - #22260
Conversation
Bind event-property and event-context matching to source expressions so code-injection queries avoid materializing global source/event relations.
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 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
There was a problem hiding this comment.
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 (noactions/checkoutand nogit 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
originremote first. Add an explicit checkout/clone (orgit init+ remote setup) step beforegit fetchto 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 allGITHUB_TOKENpermissions, which can breakactions/checkout(typically requires at leastcontents: read). Even though this is a test fixture, it’s helpful for fixtures to be runnable/representative—consider specifying minimal required permissions (for examplecontents: 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
resultbindings), but the namedefaultBranchCacheWriteEventreads like a single value. Renaming to something plural (for exampledefaultBranchCacheWriteEvents) would make the intent clearer and reduce confusion withdefaultBranchTriggerEvent().
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-RemoteFlowSourcenodes assource, this will silently drop those paths. Consider rewriting this constraint using an explicitexists(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 includesjob.getATriggerEvent() = event. Keeping the separatejob.getATriggerEvent() = eventconstraint here is redundant and makes the condition harder to read/maintain. Consider removing the duplicate constraint (or changinghasDefaultBranchCacheWriteAccessto only takejoband 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
There was a problem hiding this comment.
Review details
Comments suppressed due to low confidence (4)
actions/ql/src/Security/CWE-349/CachePoisoningViaCodeInjection.ql:25
- This hard cast to
RemoteFlowSourcewill silently drop any paths where the source node is not aRemoteFlowSource, potentially introducing false negatives. Consider rewriting this as anexists(RemoteFlowSource rfs | rfs = source.getNode() and rfs.getEventName() = event.getName())constraint (or guarding with aninstanceofcheck) 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: {}removescontents: read, whichactions/checkoutrequires 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 examplecontents: 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/checkoutwithout 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 initialactions/checkoutstep (orgit 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/checkoutwithout first checking out or initializing a repository, which makes the snippet non-functional for readers. Add anactions/checkoutstep (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
https://github.blog/changelog/2026-06-26-read-only-actions-cache-for-untrusted-triggers/