python: enable summaries from model

[yoff]

This requires a change to the shared interface:
Making getNodeFromPath public.

This because Python is doing its own thing and identifying call-backs.

I am unsure if this constitutes a feature yet, or if we should add a CSV parser first?

[RasmusWL]

Nice 💪

Although a bit of a nitpick, can we please change Foo in the package name to foo? -- it just stands out as very non-standard.

I think it's a shame that the data-flow tests and taint-tracking tests are almost identical.

For taint-tracking we've mostly used InlineTaintTest.qll, so maybe we can use that alongside the data-flow test, and only have a single TestSummaries.qll file?

[RasmusWL]

I looked a bit at where the existing NormalTaintTest was used, and the only case is https://github.com/github/codeql/blob/11c89adbe3b238e3a142256175f0003e19d7972b/python/ql/test/experimental/dataflow/summaries/summaries.py -- I think that could also benefit from being rewritten to have BOTH dataflow and taint-tracking tests, instead of only having taint tests.

[yoff]

I tried putting it in one file now, I agree we should consolidate all our summary tests at some point..

[RasmusWL]

Besides the two code comments, I also have a stylistic recommendation for your Python code.

1. put trailing commas after last argument (means diff for adding a new argument in the future is a bit more clean)
2. do not indent closing parentheses (

Both follows black formatter (although it would try to put both arguments to ensure_tainted on one line 😮‍💨)

specifically it would change your code like this:

 ensure_tainted(
     tainted_list_el,  # $ tainted
-    tainted_list_el[0]  # $ tainted
-    )
+    tainted_list_el[0],  # $ tainted
+)

[RasmusWL]

Since you have just shown that we have data-flow, and we know all dataflow steps are also taint-flow steps, I think it's fine to only check taint-flow for the cases where there is NOT dataflow.

I think that will make the test-file a bit easier to read as well.

[yoff]

Good point. I tried to make it more consistent now by not checking taint when dataflow is already established and when we do check taint, check both the collection and the expected element.

[RasmusWL]

Thinking some more about this, I think these tests should be about whether you can write flow summaries in CSV files that do the right thing.

Having all these extra taint steps are closer to what our current taint-tracking does in the end, but from my point of view, doesn't help us achieve the goal of the tests.

I removed the TAINTED_LIST parts locally, but thought it was a bit too controversial to just commit directly to your PR -- instead I've put the commits here: yoff#79 (if you agree, we can add these to main later on 👍)

[asgerf]

Minor comment otherwise LGTM

[asgerf]

LGTM once the tests pass.

There seem to be some test expectations that need updating, probably due to some of the recent changes to inline test expectations.

[RasmusWL]

Approved for now, tests can be adjusted later 👍