Skip to content

Conversation

@MathiasVP
Copy link
Collaborator

@MathiasVP MathiasVP commented May 27, 2025

This PR does three small improvements to the cs/path-injection query (each in their own commit):

  • b40a437 does a very very small optimization. When you write:
    predicate isSink(DataFlow::Node sink, FlowState state) {
      sink instanceof Sink and
      exists(state)
    }
    you're technically creating a cartesian product (CP) between all sinks and all states. This isn't a problem in this particular case since there are only 2 states (normalized and non-normalized) so the CP is very small. However, this can be a real problem if there are many states. Because of this we added a way to avoid this in DataFlow: Support stateless isSink in StateConfigSigs github/codeql#13851. The technical details aren't super important. However, it may be worth remembering that it's always safe to make the transformation in b40a437 for a very small performance boost 🥳
  • 03e671a adds a false negative test that is fixed by a2d4c20.
    When I added the normalization logic in C#: Make StartsWith and EndsWith sanitizers on normalized paths #106 to fix FPs due to missed good uses of StartsWith I accidentally introduced a false negative. Luckily, it was easy to fix.
  • db7119c adds a false positive that is fixed by 4dfa886.
    This is the meat of this PR (and the whole reason I started looking at the query). We already had logic for concluding when it's safe to use StartsWith as a sanitizer (as I explained above). However, we were missing some cases of "path normalization" which meant that the StartsWith call wasn't marked as a sanitizer.

Commit-by-commit review recommended 🙂

@MathiasVP MathiasVP merged commit 10a8863 into main May 30, 2025
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants