Ruby: disable diff-informed mode on regex queries #19416
Conversation
These queries were failing in `codeql test run --check-diff-informed` because they can select locations inside the regex. Until that can be fixed, diff-informed mode is disabled for these queries.
There was a problem hiding this comment.
Pull Request Overview
This PR disables diff-informed mode for two regex queries to resolve failures during diff-informed testing. The changes remove the predicate observeDiffInformedIncrementalMode and associated location selection logic from both the PolynomialReDoSQuery and MissingFullAnchorQuery modules.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| ruby/ql/lib/codeql/ruby/security/regexp/PolynomialReDoSQuery.qll | Removed diff-informed predicates and location getter logic. |
| ruby/ql/lib/codeql/ruby/security/regexp/MissingFullAnchorQuery.qll | Removed diff-informed predicates and location getter logic. |
Comments suppressed due to low confidence (2)
ruby/ql/lib/codeql/ruby/security/regexp/PolynomialReDoSQuery.qll:19
- The removal of the diff-informed predicate appears intentional; please confirm that no downstream consumers depend on observeDiffInformedIncrementalMode or the removed getASelectedSinkLocation logic.
predicate observeDiffInformedIncrementalMode() { any() }
ruby/ql/lib/codeql/ruby/security/regexp/MissingFullAnchorQuery.qll:19
- Ensure that the removal of diff-informed mode components does not affect any components expecting the location selection logic that was provided by getASelectedSinkLocation.
predicate observeDiffInformedIncrementalMode() { any() }
There was a problem hiding this comment.
Seems to be the same issue: RegExpTerm has both getLocation() and hasLocationInfo but they're not the same.
getLocation() however does enclose the hasLocationInfo location (which is a subterm in the regexp), but alert filtering in testing mode expects an exact match, not an overlap. I think in production this would actually work because there we check for overlap (at line granularity) not exactness.
Approving to unblock a potentially failing CI but I believe the proper fix is to improve on the AlertFiltering to check for overlap.
|
We agreed offline that checking for (line-granularity) containment is not the answer. That's because diff-informed queries follows the behaviour of code scanning on PRs where only the start line matters, not the end line, so containment would just mean start line equality. In the case of these regex queries, we would lose results on multi-line regexes. More generally, we would lose results whenever the contained location doesn't start on the same line as the container. |
These queries were failing in
codeql test run --check-diff-informedbecause they can select locations inside the regex. Until that can be fixed, diff-informed mode is disabled for these queries.I didn't add a code comment with details about the problem because I didn't investigate it in full detail, but I presume it's overall the same story as in #19379.
Cc @d10c @cklin @asgerf