Python: Add small api enhancements determined useful during documentation work by yoff · Pull Request #5398 · github/codeql

yoff · 2021-03-12T17:10:59Z

These are features we would like to use in the examples in the documentation. We cannot do that before they are implemented and live.
This PR implement them, we will have to remember to update the documentation once they are live.

tausbn

Looks good to me. I have made a few suggestions on some minor things I noticed.

tausbn · 2021-03-12T17:21:43Z

+* Make `ParameterNode` extend `LocalSourceNode`, thus making members like `flowsTo` available.
+* Add member predicate `taintFlowsTo` to `LocalSourceNode` facilitating smoother syntax for local taint tracking.
+* Add member `getALocalTaintSource` to `DataFlow::Node` facilitating smoother syntax for local taint tracking.
+* Add predicate `parameterNode` to map from a `Parameter` to a data-flow node.


I'm a bit unsure about the way in which these sentences are formulated. They read more like commit messages than change notes. Perhaps one could rewrite this as e.g.

The class ParameterNode now extends LocalSourceNode, thus making methods like flowsTo available.

Local taint tracking can now be performed using the taintFlowsTo method on the LocalSourceNode class.

and so on...

I like that direction..

tausbn

I'm still wondering if we should leave out the taint stuff until we have disentangled things a bit. I'll leave that decision up to you and rasmuswl.

RasmusWL

Nice, one small suggestion from me, otherwise looks good 👍

RasmusWL · 2021-03-16T13:38:12Z

+
+  /**
+   * Gets a local source node from which data may flow to this node in zero or more local taint-flow steps.
+   * WARNING: This will only have results if the taint tracking libraries have been imported into scope.


This warning seems annoying. Is there anything blocking us from just adding a private import TaintTrackingPublic? -- which would make things work out of the box

Indeed that imports all the concepts and actually changes test output (because all classes are also django helper classes).

I have opened the relevant can of worms on slack...

actually changes test output (because all classes are also django helper classes).

What output is changed? (what tests are we talking about)

We talked about this face-to-face. This approach had been taken previously, and the test changes can be seen in changes from b3ff3f7 (where suddenly, classes are also treated as DjangoViewClassHelper when using getAQlClass). This stems from the fact that as part of impoty python we also privately import the dataflow library

codeql/python/ql/src/python.qll

Lines 40 to 43 in 7e3dbb0

// Removing this import perturbs the compilation process enough that the points-to analysis gets

// compiled -- and cached -- differently depending on whether the data flow library is imported. By

// importing it privately here, we ensure that the points-to analysis is compiled the same way.

private import semmle.python.dataflow.new.DataFlow

, and with this change, we privately bring the taint tracking library into scope, which transitively brings all our framework modeling into scope.

@yoff and I talked a bit about the fact that importing taint-tracking would also import all external library models, so that all concrete subclasses of AdditionalTaintStep would also be in scope, following the advice in https://github.com/github/codeql/blob/main/docs/ql-design-patterns.md#importing-all-subclasses-of-a-class (that arguably, I wrote myself).

In the end, we opted for making getALocalTaintSource just works out of the box without having to remember to import something manually yourself.

determined useful during documentation work.

Co-authored-by: Taus <tausbn@github.com>

I suspect it has to do with ParameterNode being a LocalSourceNode, but I really have no idea...

yoff · 2021-03-17T14:13:52Z

Sorry for the force push, but I did not feel like dancing with git revert...

RasmusWL

Since this import question is hard to change our mind about later down the line, I think it would be wise to exercise a bit more caution before merging this PR. Possibly even wait until we have had a chance to talk with Taus about this? 😊

yoff · 2021-03-17T14:56:06Z

That is fine, there is no great rush here.

and `taintFlowsTo` for now..

RasmusWL

👍 as we agreed

…cements

RasmusWL

Still 👍

yoff requested a review from a team as a code owner March 12, 2021 17:11

github-actions Bot added documentation Python labels Mar 12, 2021

tausbn requested changes Mar 12, 2021

View reviewed changes

yoff requested a review from tausbn March 12, 2021 18:00

tausbn previously approved these changes Mar 12, 2021

View reviewed changes

Comment thread python/change-notes/2021-03-12-small-api-enhancements.md

yoff dismissed tausbn’s stale review via 4f5490d March 12, 2021 20:47

yoff mentioned this pull request Mar 12, 2021

Python: rework documentation #5357

Merged

tausbn previously approved these changes Mar 13, 2021

View reviewed changes

Comment thread python/ql/src/semmle/python/dataflow/new/internal/LocalSources.qll Outdated

Comment thread python/ql/src/semmle/python/dataflow/new/internal/DataFlowPublic.qll Outdated

yoff dismissed tausbn’s stale review via b74d640 March 15, 2021 16:48

RasmusWL requested changes Mar 16, 2021

View reviewed changes

yoff requested a review from RasmusWL March 16, 2021 23:13

RasmusWL removed their request for review March 17, 2021 10:45

yoff and others added 4 commits March 17, 2021 15:11

Python: Add small api enhancements

4d856d4

determined useful during documentation work.

Apply suggestions from code review

63b732c

Co-authored-by: Taus <tausbn@github.com>

Python: More review suggestions

8f46700

PythonÆ adjust test expectations

b3ff3f7

I suspect it has to do with ParameterNode being a LocalSourceNode, but I really have no idea...

yoff force-pushed the python-api-enhancements branch from b74d640 to b3ff3f7 Compare March 17, 2021 14:12

yoff requested a review from RasmusWL March 17, 2021 14:13

RasmusWL reviewed Mar 17, 2021

View reviewed changes

Python: Remove getALocalTaintSource

a9af135

and `taintFlowsTo` for now..

yoff requested review from RasmusWL and tausbn March 24, 2021 00:23

RasmusWL previously approved these changes Mar 24, 2021

View reviewed changes

Merge branch 'main' of github.com:github/codeql into python-api-enhan…

c777f1d

…cements

yoff dismissed RasmusWL’s stale review via c777f1d April 6, 2021 07:31

RasmusWL approved these changes Apr 6, 2021

View reviewed changes

RasmusWL merged commit 0ebb24e into github:main Apr 6, 2021

	// Removing this import perturbs the compilation process enough that the points-to analysis gets
	// compiled -- and cached -- differently depending on whether the data flow library is imported. By
	// importing it privately here, we ensure that the points-to analysis is compiled the same way.
	private import semmle.python.dataflow.new.DataFlow

Conversation

yoff commented Mar 12, 2021

Uh oh!

tausbn left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tausbn Mar 12, 2021

Choose a reason for hiding this comment

Uh oh!

yoff Mar 12, 2021

Choose a reason for hiding this comment

Uh oh!

tausbn left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

RasmusWL left a comment

Choose a reason for hiding this comment

Uh oh!

RasmusWL Mar 16, 2021

Choose a reason for hiding this comment

Uh oh!

yoff Mar 16, 2021

Choose a reason for hiding this comment

Uh oh!

yoff Mar 16, 2021

Choose a reason for hiding this comment

Uh oh!

RasmusWL Mar 17, 2021

Choose a reason for hiding this comment

Uh oh!

RasmusWL Mar 17, 2021

Choose a reason for hiding this comment

Uh oh!

yoff commented Mar 17, 2021

Uh oh!

RasmusWL left a comment

Choose a reason for hiding this comment

Uh oh!

yoff commented Mar 17, 2021

Uh oh!

RasmusWL left a comment

Choose a reason for hiding this comment

Uh oh!

RasmusWL left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants