Actions: fix filtering of code injection results between medium and critical version of query #20937
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes a filtering gap in code injection queries where alerts could be missed when neither the medium nor critical severity query would report them.
- Introduces a new helper predicate
getRelevantCriticalEventForSourceSinkthat checks both sink context and source event matching - Updates the medium query to explicitly exclude critical query results rather than using complementary context checks
- Adds test case demonstrating the fix with a workflow triggered by both push and workflow_dispatch events
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
actions/ql/lib/codeql/actions/security/CodeInjectionQuery.qll |
Adds new predicate combining sink context and source event checks |
actions/ql/src/Security/CWE-094/CodeInjectionCritical.ql |
Simplifies query to use new combined predicate |
actions/ql/src/Security/CWE-094/CodeInjectionMedium.ql |
Changes to explicitly exclude critical results using new predicate |
actions/ql/test/query-tests/Security/CWE-094/.github/workflows/push_and_workflow_dispatch.yml |
New test case demonstrating the filtering fix |
actions/ql/test/query-tests/Security/CWE-094/CodeInjectionMedium.expected |
Updated expected test results |
actions/ql/test/query-tests/Security/CWE-094/CodeInjectionCritical.expected |
Updated expected test results |
actions/ql/lib/change-notes/2025-11-28-fix-code-injection-alert-filtering.md |
Documents the majorAnalysis change |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
owen-mc
force-pushed
the
actions/fix/code-injection-privileged-context
branch
from
6f75eb7 to
6d3ee98
Compare
adityasharad
left a comment
adityasharad
left a comment
There was a problem hiding this comment.
Nice work. I think this looks right, and appreciate the tests and structure. Some questions to make sure I fully understand, or that may let us make it even cleaner.
| pragma[inline] | ||
| predicate criticalSeverity(DataFlow::Node source, DataFlow::Node sink, Event event) { | ||
| event = getRelevantCriticalEventForSink(sink) and | ||
| source.(RemoteFlowSource).getEventName() = event.getName() |
There was a problem hiding this comment.
Minor: maybe just declare sink with this type.
There was a problem hiding this comment.
Other refactors meant that this isn't possible.
| predicate mediumSeverity(DataFlow::Node source, DataFlow::Node sink) { | ||
| not criticalSeverity(source, sink, _) and | ||
| // exclude cases where the sink is a JS script and the expression uses toJson | ||
| not exists(UsesStep script | |
There was a problem hiding this comment.
Minor: put Expression expr as a variable here, and equate that to sink.asExpr() for reuse.
There was a problem hiding this comment.
This logic is actually used in two places, so I've put the logic into a separate predicate which takes an argument Expression expr.
|
|
||
| /** Holds if the flow from `source` to `sink` has medium severity. */ | ||
| pragma[inline] | ||
| predicate mediumSeverity(DataFlow::Node source, DataFlow::Node sink) { |
There was a problem hiding this comment.
Don't we need a positive statement in this logic? Or are we assuming we're in a context where we've checked flowPath(source, sink), and this predicate gets inlined?
I wonder if we should expose flowPathCriticalSeverity and flowPathMediumSeverity predicates directly on CodeInjectionFlow.
There was a problem hiding this comment.
It's probably more robust and easier to understand to have CodeInjectionFlow::flowPath(source, sink) in the same predicate. I've made that change. I have a slight preference for not putting the predicates in the flow config module, because I want this to be a standard pattern that all the other queries can use, and it allows more consistency for the non-flow queries.
|
Code changes LGTM, once you have tests passing happy to approve. What does a DCA run look like? |
|
DCA looks uneventful. Merging in |
This is based on push.yml, and it should still be found by actions/code-injection/medium, but it isn't.
owen-mc
force-pushed
the
actions/fix/code-injection-privileged-context
branch
from
849c5ff to
4a16de2
Compare
|
@adityasharad This is ready to merge but needs re-approval because I had to rebase and force-push. |
2 participants
Fixes https://github.com/github/codeql-team/issues/4370 . Includes a test demonstrating this.
In general I think the filtering between different severity levels for many queries should probably be modified to be more like this, so that it's clearer that we aren't letting some alerts fall between the cracks.