Repository navigation
Rust: Add neutral models for spurious actix-web connection_info sink models - #22884
Open
johncarmack1984 wants to merge 3 commits into
Open
johncarmack1984 wants to merge 3 commits into
johncarmack1984 wants to merge 3 commits into
Conversation
github-actions
Bot
added
documentation
Rust
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
The security-query sink suppressions and new dependency pins need human confirmation of the reported test results and broader analysis impact.
No open issues 🎉
What changed in this PR
This PR refines CodeQL’s Rust actix-web models to reduce false positives when handlers read connection information or build URLs.
Changes:
- Adds neutral sink models for seven APIs to suppress spurious allocation-size and log-injection results.
- Adds regression cases for both affected queries, with actix-web dependencies and pinned lockfiles.
- Documents the analysis improvement in a change note.
| File | Description |
|---|---|
rust/ql/test/query-tests/security/CWE-770/options.yml |
Adds the actix-web test dependency. |
rust/ql/test/query-tests/security/CWE-770/main.rs |
Exercises six APIs as allocation-size regression cases. |
rust/ql/test/query-tests/security/CWE-770/Cargo.lock |
Locks the added dependencies, including actix-rt 2.11.0. |
rust/ql/test/query-tests/security/CWE-117/options.yml |
Adds the actix-web test dependency. |
rust/ql/test/query-tests/security/CWE-117/main.rs |
Exercises six APIs as log-injection regression cases. |
rust/ql/test/query-tests/security/CWE-117/Cargo.lock |
Locks the added dependencies, including actix-rt 2.11.0. |
rust/ql/lib/codeql/rust/frameworks/actix-web.model.yml |
Adds seven neutral sink overrides. |
rust/ql/lib/change-notes/2026-10-25-neutral-models-actix-connection-info.md |
Documents the reduction in false positives. |
Review effort: Max
CommanderStorm
pushed a commit
to maplibre/martin
that referenced
this pull request
Oct 25, 2026
CodeQL's request-taint queries see nothing inside the 12 route handlers that `#[hotpath::measure]` instruments, the tile, TileJSON, catalog, font, sprite and style handlers among them. Its Rust extractor turns on every Cargo feature unless told otherwise ([docs](https://github.com/github/codeql/blob/codeql-cli/v2.27.3/rust/codeql-extractor.yml#L48-L54)), so hotpath's profiling is on, and the attribute then hands each async body to `hotpath::functions::measure_async` as an `async` block ([macro](https://github.com/hotpath-rs/hotpath/blob/4eb2e398d4a3bab902faa1809e6cfca48c1e919e/crates/hotpath-macros/src/lib_on.rs#L861-L864)) that CodeQL can't follow into. The CodeQL job now gives the extractor every workspace feature except the hotpath ones, read from `cargo metadata`, the way `just check` already leaves them out of its feature matrix. Seeing into those handlers, CodeQL flags their `req.connection_info()` calls, as it did [the OGC one on #3763](#3763 (comment)): its generated actix-web model makes `connection_info` an allocation and log sink, though it only reads the host, scheme and client address off the request. A neutral model in `.github/codeql/extensions`, which [code scanning loads on its own](https://docs.github.com/en/code-security/how-tos/find-and-fix-code-vulnerabilities/manage-your-configuration/edit-default-setup#extending-coverage-for-a-repository), cancels those generated sinks the way github/codeql#22871 does for `Vec`, until github/codeql#22884 ships the same rows upstream. With CI's suite and CodeQL 2.27.3, main goes from 7 results to 6, the OGC one included.
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The generated actix-web models make
HttpRequest::connection_infoanalloc-layoutand alog-injectionsink, and so do the methods built on it:full_url,url_for,url_for_static,ResourceMap::url_for,ServiceRequest::connection_infoand theConnectionInfoextractor.connection_infoonly reads the host, scheme and client address off the request and caches them there. It logs nothing, and the strings it allocates are copies of header values, which the HTTP layer already bounds. The rows look like they come fromConnectionInfo::new, which carries the same pair and is crate-private, so it gets no row here. Sorust/uncontrolled-allocation-sizeandrust/log-injectionflag every handler that reads the connection info. I hit this on maplibre/martin, where the handlers that build TileJSON and style URLs callreq.connection_info().This adds neutral
sinkmodels for those seven methods next to the existing actix-web models. Each mirrors a pair of generated rows, and none of the seven has any other sink kind, so nothing else is suppressed.The first commit adds an actix-web handler to the CWE-117 and CWE-770 query tests that calls six of the methods, with today's results tagged
SPURIOUS(the other.expectedchanges in that commit are model renumbering). The second adds the models, and both.expectedfiles come back to what they are onmain.ServiceRequest::connection_infoisn't in the test, since no source reaches aServiceRequestthere.codeql test runpasses on both directories and on the other three actix-web test directories with CLI 2.27.3. The test lockfiles pin actix-rt 2.11.0 like the other actix-web tests, because a fresh resolution picks actix-server 2.8.0 with actix-rt 2.15.0, which doesn't compile. DCA should show fewer results from both queries on actix-web projects that read the connection info.