Ask two data-flow tools the question a matcher cannot ask - #41
Conversation
ADR 0003 ends where a pattern over one syntax tree stops: this repository can enforce "no command is built outside the helper" and would rather enforce "every command this module runs came FROM the helper". ADR 0004 is what Semgrep and CodeQL said when they were asked the second one. Semgrep's taint mode takes one real step past the structural tier -- it joins a construction and a use that are separate statements linked by a variable, which no single pattern does -- and stops one step short of the property. Measured on this repository's own `src/probe.rs`, with the defect planted behind a wrapper one function away: `Findings: 0`. Cross-function taint is the Pro engine and wants a login. Two other things it taught: the helper's own body reads as a violation without a carve-out, because the sanitizer runs after the sink in source order. CodeQL answers it. It resolves calls rather than matching their spelling, which is what lets the query state the POSITIVE form -- `not Flow::flowTo(sink)`, no value out of the helper reaches this execution -- which is exactly the required-shape direction ADR 0003 records a matcher cannot express at all. On the same planted defect it reports the one finding and nothing else. Three measurements went into not adopting it. The first run produced three findings, all false, all `detached(..).args([..]).output()`: value flow does not cross `Command::arg` because nothing models it, so the query needs a hand-written step before it says anything true -- and an unmodelled API produces confident wrong answers in both directions. The toolchain is 600 MB, the database 174 MB and two minutes. And the third is the one that matters here. A clean answer still does not mean the file was read, at any height on the cost hierarchy. With one unparseable function above the planted defect, `codeql database create` prints "Successfully created" and the query returns nothing -- the same output as a compliant module. The state is recoverable from `parse_error` diagnostics, and the recovery is worse than it sounds: the clean tree already carries 1,689 diagnostics of that severity from macro expansion, so the signal is four more rows against that background, at severity Warning, in a channel the query's own result never mentions. Neither is adopted. The property is already enforced here by a structural test at a cost of milliseconds, and the case CodeQL proves in addition is one this module does not contain. The query is kept in the record, where it is reproducible, rather than in a directory with no runner. Third tier to produce the same requirement for the provider contract: a clean run is evidence only when something separately establishes that the analyzer could read what it was pointed at.
|
Warning Review limit reached
Next review available in: 25 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #41 +/- ##
=======================================
Coverage 89.44% 89.44%
=======================================
Files 31 31
Lines 9732 9732
=======================================
Hits 8705 8705
Misses 1027 1027 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
2 participants
Answers the Semgrep and CodeQL deliverables of #13. ADR 0003 ended where a pattern over one syntax tree stops: this repository can enforce "no command is built outside the helper" and would rather enforce "every command this module runs came from the helper". ADR 0004 is what two data-flow tools said when they were asked the second one.
Semgrep: one real step past the structural tier, one step short of the property
Taint mode joins a construction and a use that are separate statements linked by a variable, which no single AST pattern does. Thirteen lines, one finding on the fixture, and it is the right one.
Then, measured on this repository's own
src/probe.rswith the defect planted behind a wrapper one function away:Cross-function taint is the Pro engine. Also learned: the helper's own body reads as a violation without a carve-out, because
detachedsetscurrent_dirbefore it callsenv_removeand the sanitizer is therefore after the sink in source order.CodeQL: the property, answered
It resolves calls rather than matching their spelling --
Command::newin the database is<std::process::Command>::new, from type inference -- which is what lets the query state the positive form:A negation over a flow relation, which is exactly the required-shape direction ADR 0003 records a matcher cannot express at all. On the same planted defect Semgrep missed, it reports the one finding and nothing else.
Three measurements, none of them free
detached(..).args([..]).output()-- value flow does not crossCommand::arg, because nothing in the standard model says those return the same command. The query needs a hand-writtenisAdditionalFlowStepbefore it says anything true. An unmodelled API produces confident wrong answers in both directions, and the silent direction is the one nobody investigates.codeql database createprintsSuccessfully created databaseand the query returns nothing -- the same output as a compliant module. Recoverable fromparse_errordiagnostics, and the recovery is worse than it sounds: the clean tree already carries 1,689 diagnostics of that severity, mostlymacro expansion failedfromassert!andformat!. The signal is four more rows against that, at severity Warning, in a channel the query result never mentions.Decisions
Neither is adopted. The property is already enforced here by a structural test at a cost of milliseconds, and the extra case CodeQL proves -- a wrapper bypassing the helper -- is one
src/probe.rsdoes not contain. A second checker over one answer is two answers free to disagree. The query is kept in the record, where it is reproducible, rather than in a directory with no runner.Semgrep OSS is not the semantic tier; it is a better structural tier, and a repository that needs the cross-function property should be told that rather than discover it from a green run.
This is the third tier to produce the same requirement for the provider contract: a clean run is evidence only when something separately establishes that the analyzer could read what it was pointed at.
Documentation only -- no code changes.