Python: Port taint tests to use inline expectations #5687
Conversation
The meat of this PR is described in the new python/ql/test/experimental/meta/InlineTaintTest.qll file: > Defines a InlineExpectationsTest for checking whether any arguments in > `ensure_tainted` and `ensure_not_tainted` calls are tainted. > > Also defines query predicates to ensure that: > - if any arguments to `ensure_not_tainted` are tainted, their annotation is marked with `SPURIOUS`. > - if any arguments to `ensure_tainted` are not tainted, their annotation is marked with `MISSING`. > > The functionality of this module is tested in `ql/test/experimental/meta/inline-taint-test-demo`.
| @@ -3,73 +3,73 @@ | |||
|
|
|||
| class TaintTest(tornado.web.RequestHandler): | |||
| def get(self, name = "World!", number="0", foo="foo"): # $ requestHandler routedParameter=name routedParameter=number | |||
| ensure_tainted(name, number) | |||
| ensure_tainted(name, number) # $ tainted | |||
There was a problem hiding this comment.
Here you now have to split into two lines.
There was a problem hiding this comment.
no, this just means that all arguments on that line are tainted 😉
I didn't add any explicit tests showing what happens on ensure_tainted(is_tainted, is_not_tainted_yet) # $ tainted, but it should complain that there is a missing results for is_not_tainted_yet, which means it should be split into multiple lines. I'll just double check.
There was a problem hiding this comment.
This is a great initiative 💪
In the longer run, I would prefer if we could generalise the interface to InlineExpectationTest so that we could recognise ensure_tainted and ensure_not_tainted as annotations directly. But given how you have coded around this very nicely (with your extra consistency predicates), it is more a nice-to-have at this point.
Only two minor points, which came piecemeal because I pressed the wrong button..
To better match the standard output from inline expectation tests
|
I added a few more examples of what is ok and what isn't (and how errors will look like). I also updated the format of my query predicates to better match what is usually given by inline test expectations 👍 |
|
Interesting part of merge is 63a2657#diff-b952effafd539a00838363ef1cc0369a465f5f3687d60c839ceb19d3cf970da6 |
Thanks 😅 |
|
|
The meat of this PR is described in the new python/ql/test/experimental/meta/InlineTaintTest.qll file:
One of the things I liked the best about the old
TestTaint.expectedfiles was that it was easy to see how many failures we had. However, it's annoying to review code that only changes the.expectedfile, since you need to manually look up the.pyfile to get the full picture of what is now handled (see here as an example).To ensure we can still easily get an overview of failures, I introduced the requirement to add
SPURIOUSandMISSINGannotations. So it should be easy to grep for(SPRURIOUS|MISSING): taintedand find all cases our taint-steps could be improved.In regards to reviewing, it will be a pain to go through all files individually to see that everything is the same as before (I know, since I did this myself 😢) -- so I'll suggest that you look at the demo, and if you are convinced that the new
InlineTaintTest.qllworks properly, this PR should be good to go if tests pass 🤞