Java: Prune PathGraph for CsrfUnprotectedRequestType.ql #20083
Conversation
aschackmull
added
the
no-change-note-required
There was a problem hiding this comment.
Pull Request Overview
This PR optimizes the PathGraph for the CSRF unprotected request type query by implementing a bidirectional reachability analysis to prune irrelevant edges. The optimization is intended to be behavior-preserving while significantly reducing output size (saving hundreds of MBs) and improving processing performance for the DCA tool.
Key changes:
- Introduces bidirectional flow analysis predicates (
fwdFlowandrevFlow) to identify relevant edges - Replaces direct
CallGraph::edgesusage with a prunedrelevantEdgepredicate - Updates the transitive closure computation to use the optimized edge predicate
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| CsrfUnprotectedRequestType.ql | Updates query to use pruned edge predicate instead of raw call graph edges |
| CsrfUnprotectedRequestTypeQuery.qll | Implements bidirectional reachability analysis and relevantEdge predicate for path pruning |
| /** | ||
| * Holds if `pred` has a successor node `succ` and this edge is in an | ||
| * `unprotectedStateChange` path. | ||
| */ |
There was a problem hiding this comment.
The predicate should include documentation about its performance characteristics and the bidirectional pruning strategy, as this is a key optimization that affects query behavior.
| */ | |
| */ | |
| /** | |
| * Holds if `pred` has a successor node `succ` and this edge is in an | |
| * `unprotectedStateChange` path. | |
| * | |
| * Performance characteristics: | |
| * - This predicate relies on `revFlow`, which is a recursive flow predicate | |
| * that combines forward and reverse graph traversal. | |
| * - The use of `CallGraph::edges` ensures that only valid edges in the call | |
| * graph are considered, reducing unnecessary computations. | |
| * | |
| * Bidirectional pruning strategy: | |
| * - The predicate leverages `revFlow(pred)` and `revFlow(succ)` to prune | |
| * the search space by ensuring that both the predecessor and successor | |
| * nodes are part of valid reverse flows. | |
| */ |
Copilot uses AI. Check for mistakes.
| @@ -237,12 +237,35 @@ private predicate sink(CallPathNode sinkMethodCall) { | |||
| ) | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
Missing documentation for the fwdFlow predicate. Should explain that it computes forward reachability from sources using transitive closure over call graph edges.
| /** | |
| * Computes forward reachability from source nodes using transitive closure | |
| * over call graph edges. Holds if `n` is reachable from a source node. | |
| */ |
Copilot uses AI. Check for mistakes.
| or | ||
| exists(CallPathNode mid | fwdFlow(mid) and CallGraph::edges(mid, n)) | ||
| } | ||
|
|
There was a problem hiding this comment.
Missing documentation for the revFlow predicate. Should explain that it computes backward reachability from sinks within the forward-reachable nodes, implementing the bidirectional pruning strategy.
| /** | |
| * Computes backward reachability from sinks within the forward-reachable nodes. | |
| * This predicate implements the bidirectional pruning strategy by combining | |
| * forward reachability (`fwdFlow`) with backward reachability from sinks. | |
| */ |
Copilot uses AI. Check for mistakes.
|
The "Query result disk size" metric shows a massive reduction as expected. |
|
I suppose you could add a change note with the "fix" category, so that if anyone notices a change they can figure out what caused it. Up to you. |
After result interpretation the sarif is unchanged, so it's not very detectable. |
This should be completely behaviour-preserving, but saves hundreds of MBs of output for some repos. Dca has had trouble processing the output of this query even for cases with zero alerts for several repos.
See also comments here: #19872 (comment)