Skip to content

Python: Use Query.qll suffix for dataflow configuration definitions #8511

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Apr 6, 2022

Conversation

RasmusWL
Copy link
Member

@RasmusWL RasmusWL commented Mar 21, 2022

Adopting the standard setup of defining the data flow configurations for our path-problem queries in a file called ...Query.qll, so it's very obvious that this file should only be used by the query. (there is a CI check for this on our own codebase).

Like JS did in #6450

When adopting this, I wanted us to do the same as other languages in terms of setup. For simple queries that only require a single data-flow configuration, all langauges define that in the ...Query.qll file. JS/Ruby always use the name Configuration for this configuration, which is also the most straightforward thing for us to do.

However, it's not quite as obvious how we should handle cases where we need multiple configurations, currently ServerSideRequestForgeryQuery.qll, WeakSensitiveDataHashingQuery.qll, and LdapInjectionQuery.qll. I noted that LdapInjectionQuery.qll actually used a very different pattern than the two others. Instead of defining separate modules for each configuration, they just had different names within the same module 🤔

Honestly, the approach in LdapInjectionQuery.qll seems simpler and requires less boiler-plate code, so I will suggest adopting that approach for ServerSideRequestForgeryQuery.qll. For WeakSensitiveDataHashingQuery.qll I really do think that having the two separate ql-modules makes sense, since they both use different sinks/sources, and could have different sanitizers (exposing them as top-level configurations means that we have to rewrite source instanceof Source to source instanceof WeakSensitiveDataHashingCustomizations::NormalHashFunction::Source 😠)

According to git grep "extends TaintTracking::Configuration" **/*Query.qll | cut -d ':' -f 1 | uniq -c | grep -v "1 " the only file that has more than one extends TaintTracking::Configuration is csharp/ql/lib/semmle/code/csharp/security/dataflow/UnsafeDeserializationQuery.qll -- all 3 used directly in the query. So not too much inspiration to match against.

This commit in itself makes everything break, but should make it easy to
follow the overall changes being made.
and move all the old deprecated aliases to that file. We now have a
situation where all queries should work as they did before, and we just
have these new Query.qll files that contain the implementation.

(deprecation comes later)
So we stick to the naming conventions.

This rename is OK, since the new file was only just introduced in this
PR.
AHA! This change happened because we are no longer importing all the old
deprecated implementation.
@RasmusWL RasmusWL marked this pull request as ready for review March 21, 2022 21:33
@RasmusWL RasmusWL requested a review from a team as a code owner March 21, 2022 21:33
Copy link
Contributor

@yoff yoff left a comment

Choose a reason for hiding this comment

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

LGTM
It took me a few iterations and jumping between the single-commit-view and the all-commits-view, but I think the commit structure actually made it quite easy to understand the transformation (I just had to check that nothing was missed) 👍

@RasmusWL
Copy link
Member Author

RasmusWL commented Apr 6, 2022

It took me a few iterations and jumping between the single-commit-view and the all-commits-view, but I think the commit structure actually made it quite easy to understand the transformation (I just had to check that nothing was missed)

After doing this I wasn't quite sure if I had made the right choice, so that makes me happy ☺️

@RasmusWL RasmusWL merged commit 4d2a3b3 into github:main Apr 6, 2022
@RasmusWL RasmusWL deleted the use-query-suffix branch April 6, 2022 09:59
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.

2 participants