Python: rework documentation by yoff · Pull Request #5357 · github/codeql

yoff · 2021-03-08T10:06:35Z

This PR is a step towards the structure suggested in https://github.com/github/codeql-python-team/issues/319. It

rearranges sections
removes the unused sections
adds a new section on data flow
removes all references to the old data-flow implementation from surrounding documents

Remaining sections can then be added later. The section on data flow is an adaptation from the one for C# with exercises removed. It has running examples and should be accurate if not comprehensive. For example, it only introduces LocalSourceNode very briefly (but does illustrate the concept with an example).

I am not sure if we are ready to merge this PR as is or if we would rather build a tower of PRs on top, adding the remaining planned sections. But I open it up for review now, so people (especially from the doc team) can get involved.

Initilally a copy of the one from C#

python queries.

github-actions · 2021-03-10T15:49:10Z

Hello @github/docs-content-codeql - this PR is ready for docs review.

yoff · 2021-03-10T15:51:16Z

Does this need a change-note? It is not really tied to the code, right?

github-actions · 2021-03-10T20:48:01Z

Hello @github/docs-content-codeql - this PR is ready for docs review.

shati-patel

Thanks for this update! A couple of small suggestions, but otherwise LGTM. It's great to see more consistency across the languages 😃

Does this need a change-note? It is not really tied to the code, right?

You're right, the changes you've made here don't need a change note! I'm assuming there was a change note when you added the new libraries themselves?

I am not sure if we are ready to merge this PR as is or if we would rather build a tower of PRs on top, adding the remaining planned sections. But I open it up for review now, so people (especially from the doc team) can get involved.

From my perspective, it's fine to merge as-is. Assuming the information/examples are accurate (I defer to the Python team for that 😅), this looks like a good change in itself. We can always expand on the details and add more sections later!

shati-patel · 2021-03-11T14:24:19Z

+.. code-block:: ql
+
+   import csharp
+   import semmle.code.csharp.dataflow.flowsources.PublicCallableParameter


Suggested change

import semmle.code.csharp.dataflow.flowsources.PublicCallableParameter

import semmle.code.python.dataflow.flowsources.PublicCallableParameter

I'm assuming the rest of the import path is correct? Or should it be semmle.python.dataflow.new.RemoteFlowSources?

Yes, it should be the latter, thank you for catching all these non-runnable examples!

Co-authored-by: Shati Patel <42641846+shati-patel@users.noreply.github.com>

shati-patel

Looks good!

tausbn

Generally this looks good to me, but I feel like we should be emphasising LocalSourceNode more as the right abstraction to use. If we can get people started on using these early, then type trackers can be presented as using LocalSourceNode everywhere (and hence reap the corresponding performance benefits).

tausbn · 2021-03-11T16:16:28Z

+    from DataFlow::CallCfgNode call, DataFlow::ParameterNode p
+    where
+      call = API::moduleImport("os").getMember("open").getACall() and
+      DataFlow::localFlow(p, call.getArg(0))


Hot take: we should be able to use flowsTo here, and hence ParameterNode should in fact extend LocalSourceNode.

That would make a lot of sense, morally all parameters are local sources. I am not sure if some technical SSA/CfgNode discrepancy has to be overcome first..

Judging by this, all ParameterNodes are in fact LocalSourceNodes already.

tausbn · 2021-03-11T16:17:54Z

+    from DataFlow::CallCfgNode call, DataFlow::ParameterNode p
+    where
+      call = API::moduleImport("os").getMember("open").getACall() and
+      TaintTracking::localTaint(p, call.getArg(0))


In light of my comment in the data-flow version of this example, I wonder if we should have taintFlowsTo as a method on LocalSourceNode and perhaps discourage the use of localFlow/localTaint entirely.

That might be good.

yoff · 2021-03-12T20:49:52Z

The desirable api enhancements identified here are in #5398. We will have to wait until that is merged and live until we update the documentation to refer to them. I have added a section calling attention to local source nodes. I hope this addresses the concern for now.

also remove references to `parameterNode` which is not available yet.

shati-patel

A few more small edits, but the docs look good overall 🥇

Co-authored-by: Shati Patel <42641846+shati-patel@users.noreply.github.com>

RasmusWL

Looks really good 💪 A few minor things, then it's ready to be merged from my point of view 🥇

Personally, I don't feel strongly about flowsTo vs DataFlow::localFlow (or the taint version of this). I suggest we don't postpone getting the PR merged because of this, and can solve this in a follow up discussion next week.

RasmusWL · 2021-03-16T10:59:59Z

 Many of the built-in path queries included in CodeQL follow a simple structure, which depends on how the language you are analyzing is modeled with CodeQL.

-For C/C++, C#, Java, and JavaScript you should use the following template:
+You should use the following template:


Really nice that we're able to do this now 💪

Only problem for python is that you import python is not enough, and you need to do import semmle.python.dataflow.new.DataFlow as well. I looked through other languages:

for C#/JS/Go: Works by only import <language>

Python/C++/Java: Need to import DataFlow library manually

So I don't think it's breaking, and I'll make a follow up PR to try and improve on this situation 👍

yoff · 2021-03-16T11:36:31Z

Sorry to keep updating this, but I was reminded by https://github.com/github/codeql-python-team/issues/319 that we needed to describe attributes and saw a natural place to do it. I think now that the remaining description of type trackers are best left to a separate PR, so this is the final tweak. 🤞

RasmusWL · 2021-03-16T11:42:18Z

+   -  ``Concepts::SystemCommandExecution`` - a data-flow node that executes an operating system command, for instance by spawning a new process.
+   -  ``Concepts::FileSystemAccess`` - a data flow node that performs a file system access, including reading and writing data, creating and deleting files and folders, checking and updating permissions, and so on.
+   -  ``Concepts::Path::PathNormalization`` - a data-flow node that performs path normalization. This is often needed in order to safely access paths.
+   -  ``Concepts::Decoding`` - a data-flow node that decodes data from a binary or textual format. A decoding (automatically) preserves taint from input to output. However, it can also be a problem in itself, for example if it allows code execution or could result in denial-of-service.
+   -  ``Concepts::Encoding`` - a data-flow node that encodes data to a binary or textual format. An encoding (automatically) preserves taint from input to output.
+   -  ``Concepts::CodeExecution`` - a data-flow node that dynamically executes Python code.
+   -  ``Concepts::SqlExecution`` - a data-flow node that executes SQL statements.
+   -  ``Concepts::HTTP::Server::RouteSetup`` - a data-flow node that sets up a route on a server.
+   -  ``Concepts::HTTP::Server::HttpResponse`` - a data-flow node that creates a HTTP response on a server.


I think it's great to mention Concepts 👍

Do you want to build an exhaustive list here? (meaning we need to update it every time we add a new concept) otherwise we could give a few examples, and say that more can be found in the file 🤷

(I'm leaning towards it being a pain point to list them all manually, but interested to see how you feel about it)

I am unsure, actually. When I saw that the list was not very long, I just took them all, as I felt it could be useful to have such an index somewhere, and this is probably one of the first places people would look.

…on.rst Co-authored-by: Rasmus Wriedt Larsen <rasmuswriedtlarsen@gmail.com>

calumgrant · 2021-03-17T09:55:28Z

@xcorail would really like to open up Python to the bug bounty programme this week so could this be merged asap. Am I right in thinking that once this has been merged, the documentation is in a good enough state again?

RasmusWL

juuuust a few last things 😊

yoff · 2021-03-17T11:56:40Z

Am I right in thinking that once this has been merged, the documentation is in a good enough state again?

This and #5389, I would say.

…on.rst Co-authored-by: Rasmus Wriedt Larsen <rasmuswriedtlarsen@gmail.com>

We're going to postpone flowsTo vs DataFlow::localFlow discussion, so we can get this merged now 👍

yoff added 2 commits March 7, 2021 14:02

Python: File for dataflow section

aaaf909

Initilally a copy of the one from C#

Python, doc: Make first batch of examples runnable

2bccb67

python queries.

github-actions Bot added the documentation label Mar 8, 2021

Python: Make the documentation not lie

91c0066

github-actions Bot added the Python label Mar 9, 2021

yoff added 6 commits March 10, 2021 10:52

Python, doc: Add links to runs on LGTM.com

76e936c

Python, doc: Remove section on taint-tracking

e4422fc

Python: remove unused file

5c3d0c8

Python, doc: Remove unused file

b865e88

Python: Fix old reference

0ab4e3e

Python: purge old references

ad35c01

yoff marked this pull request as ready for review March 10, 2021 15:48

yoff requested a review from a team as a code owner March 10, 2021 15:48

yoff added the ready-for-doc-review This PR requires and is ready for review from the GitHub docs team. label Mar 10, 2021

yoff added the no-change-note-required This PR does not need a change note label Mar 10, 2021

adityasharad added ready-for-doc-review This PR requires and is ready for review from the GitHub docs team. and removed ready-for-doc-review This PR requires and is ready for review from the GitHub docs team. labels Mar 10, 2021

shati-patel reviewed Mar 11, 2021

View reviewed changes

yoff and others added 2 commits March 11, 2021 15:57

Apply suggestions from code review

4d1b49a

Co-authored-by: Shati Patel <42641846+shati-patel@users.noreply.github.com>

Python: One more change from code review

f561c45

yoff requested a review from shati-patel March 11, 2021 14:59

shati-patel previously approved these changes Mar 11, 2021

View reviewed changes

tausbn previously requested changes Mar 11, 2021

View reviewed changes

tausbn mentioned this pull request Mar 11, 2021

Python: Document API graphs #5389

Merged

Python, doc: Describe smoother syntax

748749c

yoff dismissed shati-patel’s stale review via 748749c March 12, 2021 15:02

Python, doc: subsection on local sources

0d8f8d2

also remove references to `parameterNode` which is not available yet.

yoff requested review from shati-patel and tausbn March 13, 2021 07:16

shati-patel reviewed Mar 15, 2021

View reviewed changes

Comment thread docs/codeql/codeql-language-guides/analyzing-data-flow-in-python.rst Outdated

Comment thread docs/codeql/codeql-language-guides/analyzing-data-flow-in-python.rst Outdated

Comment thread docs/codeql/codeql-language-guides/analyzing-data-flow-in-python.rst Outdated

yoff and others added 2 commits March 15, 2021 17:56

Apply suggestions from code review

14dd708

Co-authored-by: Shati Patel <42641846+shati-patel@users.noreply.github.com>

Python: Describe Concepts and Attributes

cf791e8

RasmusWL requested changes Mar 16, 2021

View reviewed changes

RasmusWL reviewed Mar 16, 2021

View reviewed changes

yoff and others added 2 commits March 16, 2021 19:58

Update docs/codeql/codeql-language-guides/analyzing-data-flow-in-pyth…

0ee7ccf

…on.rst Co-authored-by: Rasmus Wriedt Larsen <rasmuswriedtlarsen@gmail.com>

Python: Address review suggestions

acac519

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

RasmusWL requested changes Mar 17, 2021

View reviewed changes

Comment thread docs/codeql/codeql-language-guides/analyzing-data-flow-in-python.rst

Comment thread docs/codeql/codeql-language-guides/analyzing-data-flow-in-python.rst Outdated

Comment thread docs/codeql/codeql-language-guides/codeql-library-for-python.rst

Update docs/codeql/codeql-language-guides/analyzing-data-flow-in-pyth…

0fc3099

…on.rst Co-authored-by: Rasmus Wriedt Larsen <rasmuswriedtlarsen@gmail.com>

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

RasmusWL approved these changes Mar 17, 2021

View reviewed changes

RasmusWL merged commit 1ecee2d into github:main Mar 17, 2021

	import semmle.code.csharp.dataflow.flowsources.PublicCallableParameter
	import semmle.code.python.dataflow.flowsources.PublicCallableParameter

Conversation

yoff commented Mar 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Mar 10, 2021

Uh oh!

yoff commented Mar 10, 2021

Uh oh!

github-actions Bot commented Mar 10, 2021

Uh oh!

shati-patel left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

shati-patel left a comment

Choose a reason for hiding this comment

Uh oh!

tausbn left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yoff commented Mar 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shati-patel 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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

yoff commented Mar 16, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

calumgrant commented Mar 17, 2021

Uh oh!

RasmusWL left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

yoff commented Mar 17, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

yoff commented Mar 8, 2021 •

edited

Loading

yoff commented Mar 12, 2021 •

edited

Loading