Skip to content

FlowSummaryImpl: Embed AST nodes in source/sink summary nodes for better flow paths - #22145

Open
hvitved wants to merge 15 commits into
github:mainfrom
hvitved:flow-summary-source-sink-locations
Open

FlowSummaryImpl: Embed AST nodes in source/sink summary nodes for better flow paths#22145
hvitved wants to merge 15 commits into
github:mainfrom
hvitved:flow-summary-source-sink-locations

Conversation

@hvitved

@hvitved hvitved commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

This PR addresses the issues identified by @MathiasVP on #22113.

Note that the issue (and fix) only applies to Rust, which is the only language that currently supports source/sink definitions with non-empty access paths.

The underlying issue was that we were unable to map source/sink summary nodes to the locations represented by the output/input models-as-data specs, so for example a source with an Argument[0] spec would be unable to be mapped to the corresponding argument, and we instead resorted to using the location of the call as the source location. The second issue identified by @MathiasVP, defining a source that is supposed to be a parameter, was not even supported in Rust.

The fix to both issues is to embed language-specific AST nodes into the source/sink summary nodes, and then use the locations of those as the locations of the source/sink nodes. This PR also shows how to add support for parameter sources, as well as more complex sinks like Argument[0].ReturnValue.Field[A].Field[B] (a value stored inside B stored inside A, which is returned from a callback at position 0).

For source/sink specs with complex access paths like the one above, we include a data flow node for each of the access path tokens, which means we can get much more helpful flow paths:

let a = |a| {
    let s = source(1);
//          ^^^^^^^^^ source (both before and after)
    return s;
//         ^ sink before, intermediate node after
};

// a sink with access path `Argument[0].ReturnValue`
complex_sink(a);
//           ^ sink after

Commit-by-commit review is strongly encouraged, and the second commit should be reviewed ignoring whitespaces.

The impact for Rust is that some alert locations have changed (to more precise locations, and in alignment with other languages), and I have added a change note for this.

Comment thread rust/ql/lib/codeql/rust/dataflow/internal/FlowSummaryImpl.qll Fixed
Comment thread shared/dataflow/codeql/dataflow/internal/FlowSummaryImpl.qll Fixed
@hvitved
hvitved force-pushed the flow-summary-source-sink-locations branch from b24377a to d2107ce Compare July 9, 2026 11:51
@github-actions github-actions Bot added Rust Pull requests that update Rust code DataFlow Library labels Jul 9, 2026
@hvitved
hvitved force-pushed the flow-summary-source-sink-locations branch from d2107ce to d397063 Compare July 9, 2026 12:42
@hvitved
hvitved force-pushed the flow-summary-source-sink-locations branch 3 times, most recently from 61ebe22 to fddb4f0 Compare July 10, 2026 13:40
@hvitved
hvitved force-pushed the flow-summary-source-sink-locations branch 5 times, most recently from bb0f3dc to 332303a Compare August 4, 2026 10:20
Comment thread shared/dataflow/codeql/dataflow/internal/FlowSummaryImpl.qll Fixed
Comment thread shared/dataflow/codeql/dataflow/internal/FlowSummaryImpl.qll Fixed
@hvitved
hvitved force-pushed the flow-summary-source-sink-locations branch 3 times, most recently from 8196e39 to bca818e Compare August 5, 2026 12:21
@hvitved
hvitved force-pushed the flow-summary-source-sink-locations branch from bca818e to 74ec8fd Compare August 5, 2026 12:47

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

Embeds source/sink reporting elements in flow-summary nodes, enabling precise Rust access-path locations and parameter sources while adapting all language implementations to the revised shared API.

Changes:

  • Adds reporting-element-aware summary nodes and flow steps.
  • Implements Rust callback, parameter, nested-field, and return-value access paths.
  • Updates inline annotations, expected results, and Rust change notes.
Show a summary per file
File Description
shared/dataflow/codeql/dataflow/internal/FlowSummaryImpl.qll Defines reporting-element summary infrastructure.
swift/ql/lib/codeql/swift/dataflow/internal/TaintTrackingPrivate.qll Adapts summary taint steps.
swift/ql/lib/codeql/swift/dataflow/internal/FlowSummaryImpl.qll Implements revised summary API.
swift/ql/lib/codeql/swift/dataflow/internal/DataFlowPrivate.qll Uses embedded node metadata.
rust/ql/test/query-tests/security/CWE-918/RequestForgery.expected Updates request-forgery paths.
rust/ql/test/query-tests/security/CWE-918/request_forgery_tests.rs Relocates source expectation.
rust/ql/test/query-tests/security/CWE-798/HardcodedCryptographicValue.expected Updates cryptographic sink locations.
rust/ql/test/query-tests/security/CWE-327/WeakSensitiveDataHashing/WeakSensitiveDataHashing.expected Updates hashing alert paths.
rust/ql/test/query-tests/security/CWE-327/WeakSensitiveDataHashing/CryptographicOperations.expected Updates operation locations.
rust/ql/test/query-tests/security/CWE-319/UseOfHttp.expected Updates HTTP sink locations.
rust/ql/test/query-tests/security/CWE-312/CleartextStorageDatabase.expected Updates database sink paths.
rust/ql/test/query-tests/security/CWE-311/CleartextTransmission.expected Updates transmission sink paths.
rust/ql/test/query-tests/security/CWE-295/DisabledCertificateCheck.expected Updates certificate-check paths.
rust/ql/test/query-tests/security/CWE-089/mysql.rs Relocates SQL sink annotations.
rust/ql/test/query-tests/security/CWE-079/warp/XSS.expected Updates Warp XSS path.
rust/ql/test/query-tests/security/CWE-079/warp/main.rs Relocates Warp source annotation.
rust/ql/test/query-tests/security/CWE-079/axum/XSS.expected Updates Axum XSS source.
rust/ql/test/query-tests/security/CWE-079/actix/XSS.expected Updates Actix XSS path.
rust/ql/test/query-tests/security/CWE-079/actix/main.rs Relocates Actix source annotation.
rust/ql/test/query-tests/security/CWE-022/TaintedPathSinks.ql Restricts results to source locations.
rust/ql/test/query-tests/security/CWE-020/RegexInjection.expected Updates regex source location.
rust/ql/test/library-tests/frameworks/rusqlite/main.rs Relocates SQL sink annotation.
rust/ql/test/library-tests/frameworks/postgres/main.rs Relocates SQL sink annotation.
rust/ql/test/library-tests/dataflow/sources/web_frameworks/test.rs Relocates callback source annotations.
rust/ql/test/library-tests/dataflow/sources/web_frameworks/TaintSources.expected Updates framework source locations.
rust/ql/test/library-tests/dataflow/sources/stdin/TaintSources.expected Updates stdin source locations.
rust/ql/test/library-tests/dataflow/sources/net/TaintSources.expected Updates network source locations.
rust/ql/test/library-tests/dataflow/sources/file/TaintSources.expected Updates file source locations.
rust/ql/test/library-tests/dataflow/sources/env/TaintSources.expected Updates environment source locations.
rust/ql/test/library-tests/dataflow/sources/database/test.rs Relocates database source annotations.
rust/ql/test/library-tests/dataflow/sources/database/TaintSources.expected Updates database source locations.
rust/ql/test/library-tests/dataflow/models/models.ext.yml Adds complex source/sink models.
rust/ql/test/library-tests/dataflow/models/main.rs Tests nested and parameter paths.
rust/ql/test/library-tests/dataflow/local/DataFlowStep.expected Updates local-step output.
rust/ql/lib/utils/test/InlineFlowTest.qll Resolves embedded reporting elements.
rust/ql/lib/codeql/rust/security/WeakSensitiveDataHashingExtensions.qll Adapts modeled hash operations.
rust/ql/lib/codeql/rust/security/HardcodedCryptographicValueExtensions.qll Adapts modeled-sink deduplication.
rust/ql/lib/codeql/rust/security/DisabledCertificateCheckExtensions.qll Adapts certificate-sink deduplication.
rust/ql/lib/codeql/rust/dataflow/internal/TaintTrackingImpl.qll Adapts summary taint steps.
rust/ql/lib/codeql/rust/dataflow/internal/Node.qll Uses reporting-element metadata.
rust/ql/lib/codeql/rust/dataflow/internal/ModelsAsData.qll Models callable definitions directly.
rust/ql/lib/codeql/rust/dataflow/internal/FlowSummaryImpl.qll Implements Rust reporting elements.
rust/ql/lib/codeql/rust/dataflow/internal/DataFlowImpl.qll Integrates revised summary steps.
rust/ql/lib/codeql/rust/dataflow/internal/DataFlowConsistency.qll Updates summary-node consistency.
rust/ql/lib/change-notes/2026-08-05-data-flow-alert-locations.md Documents alert-location changes.
ruby/ql/lib/codeql/ruby/dataflow/internal/TaintTrackingPrivate.qll Adapts summary taint steps.
ruby/ql/lib/codeql/ruby/dataflow/internal/FlowSummaryImpl.qll Implements revised summary API.
ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowPrivate.qll Uses embedded node metadata.
python/ql/lib/semmle/python/dataflow/new/internal/TaintTrackingPrivate.qll Adapts summary taint steps.
python/ql/lib/semmle/python/dataflow/new/internal/FlowSummaryImpl.qll Implements revised summary API.
python/ql/lib/semmle/python/dataflow/new/internal/DataFlowPrivate.qll Adapts summary flow steps.
python/ql/lib/semmle/python/dataflow/new/internal/DataFlowDispatch.qll Uses summary-node metadata.
javascript/ql/lib/semmle/javascript/dataflow/internal/TaintTrackingPrivate.qll Adapts summary taint steps.
javascript/ql/lib/semmle/javascript/dataflow/internal/sharedlib/FlowSummaryImpl.qll Exposes revised private API.
javascript/ql/lib/semmle/javascript/dataflow/internal/sharedlib/DataFlowArg.qll Removes obsolete call base.
javascript/ql/lib/semmle/javascript/dataflow/internal/FlowSummaryPrivate.qll Implements revised summary API.
javascript/ql/lib/semmle/javascript/dataflow/internal/DataFlowPrivate.qll Uses embedded node metadata.
java/ql/test/library-tests/dataflow/local-additional-taint/localAdditionalTaintStep.ql Adapts summary-step test.
java/ql/lib/semmle/code/java/dataflow/internal/TaintTrackingUtil.qll Adapts summary taint steps.
java/ql/lib/semmle/code/java/dataflow/internal/FlowSummaryImpl.qll Implements revised summary API.
java/ql/lib/semmle/code/java/dataflow/internal/DataFlowUtil.qll Adapts local summary steps.
java/ql/lib/semmle/code/java/dataflow/internal/DataFlowPrivate.qll Adapts summary jump steps.
java/ql/lib/semmle/code/java/dataflow/internal/DataFlowNodes.qll Uses summary-node metadata.
go/ql/lib/semmle/go/dataflow/internal/TaintTrackingUtil.qll Adapts summary taint steps.
go/ql/lib/semmle/go/dataflow/internal/FlowSummaryImpl.qll Implements revised summary API.
go/ql/lib/semmle/go/dataflow/internal/DataFlowUtil.qll Adapts local summary steps.
go/ql/lib/semmle/go/dataflow/internal/DataFlowPrivate.qll Adapts summary jump steps.
go/ql/lib/semmle/go/dataflow/internal/DataFlowNodes.qll Uses summary-node locations.
csharp/ql/lib/semmle/code/csharp/dataflow/internal/TaintTrackingPrivate.qll Adapts summary taint steps.
csharp/ql/lib/semmle/code/csharp/dataflow/internal/FlowSummaryImpl.qll Implements revised summary API.
csharp/ql/lib/semmle/code/csharp/dataflow/internal/DataFlowPrivate.qll Uses embedded node metadata.
cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/TaintTrackingUtil.qll Adapts summary taint steps.
cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowUtil.qll Adapts local summary steps.
cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowPrivate.qll Adapts summary jump steps.
cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowNodes.qll Uses summary-node metadata.
cpp/ql/lib/semmle/code/cpp/dataflow/internal/FlowSummaryImpl.qll Implements revised summary API.

Review details

  • Files reviewed: 89/90 changed files
  • Comments generated: 0
  • Review effort level: Balanced

@hvitved
hvitved force-pushed the flow-summary-source-sink-locations branch from db6ff1d to 9c03c21 Compare August 6, 2026 11:46
@hvitved
hvitved marked this pull request as ready for review August 6, 2026 12:28
@hvitved
hvitved requested review from a team as code owners August 6, 2026 12:28
Comment on lines +140 to +145
class SourceSinkReportingElement extends Void {
Location getLocation() { none() }

DataFlowCallable getEnclosingCallable() { none() }

SourceSinkReportingElement getASuccessor(Impl::Private::SummaryComponent sc) { none() }

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.

Having one class to model both sources and sink elements makes it impossible to give each kind their own toString, right?

For C/C++ I'm hoping that we can keep the source output argument output from toString for the source node, but keep the arg output for toString when it's a sink. For example, I really hope that we could keep the source node's toString as source output argument, but have the sink node's toString give arg in:

void test() {
  source(arg);
  sink(arg);
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can override the default toString implementations provided by SourceOutputNode and SinkInputNode, where sources and sinks are distinguished. I'm curious, however, why is source output argument a better string representation than simply arg (which I believe all other languages use)?

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.

I personally like it because the two nodes represent different things (i.e., the value before or after a call), and I like to have those things to have different string representations 🤷

On a less subjective note: It's super useful for debugging customer confusion about dataflow. Customers often confuse node.asDefiningArgument() (which selects the post-update node of an argument), and node.asExpr() - probably since they both give you the argument Expr. To make the confusion even worse, if you have code such as:

int* x = ...;
source(x);
sink(x);

then using source.asExpr() and sink.asExpr() will work and give you the expected flow. However, since asExpr won't give you the post-update node that solution won't work in cases such as:

struct S { int x; };
S s;
source(s.x);
sink(s.x);

So if a customer reports a flow path starting at an argument node and the node is not of the form ... output argument then I know they've done something wrong, and they will be missing flow.

It's not the end of the world to not have this, but it's a useful feature that I'd rather avoid losing.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In Rust, source(x) and source(s.x) should both use the post-update nodes of x and s.x, resp., so they will have the [post] ... prefix in the textual representation to distinguish them from the values before the call. This logic exists for MaD defined sources here in the getSourceDataFlowNode predicate.

@MathiasVP MathiasVP Aug 10, 2026

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.

Oh that part I agree with. What I'm saying is that, in C++, if a customer writes a query like:

module Config implements DataFlow::ConfigSig {
  predicate isSource(DataFlow::Node source) { ... source.asExpr() ... }
}

then this will give them the right flow for source(x), but not for source(s.x). I don't see how this also won't apply to Rust?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, that also applies to Rust, and perhaps all languages should have an asDefiningArgument predicate for this. But I still think the textual representation using [post] ... should be enough for debugging.

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.

Indeed, I would also be totally okay with [post] ... for the outgoing argument. As long as the toString is different from the toString of the expression node I'm happy 🙂

Comment thread shared/dataflow/codeql/dataflow/internal/FlowSummaryImpl.qll Outdated
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants