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
redsun82
left a comment
There was a problem hiding this comment.
Thanks!! Overall looks good to me, with just one testing gap that could be filled
Reusable workflow jobs inherit their callers' trigger events and do not expose workflow_call through getATriggerEvent(). Remove the branch that assumed otherwise, and add coverage for caller resolution and inherited events.
Preserve both workflow_dispatch test scenarios and regenerate the CWE-349 expected results.
|
Implemented suggestions and added tests. Also noticed one incorrect assumption about |
https://github.blog/changelog/2026-06-26-read-only-actions-cache-for-untrusted-triggers/