Python: Model Django REST framework #7016

RasmusWL · 2021-11-01T14:30:27Z

No description provided.

RasmusWL · 2021-11-02T08:54:46Z

QLDocs check complains about these:

Error: The following CodeQL elements are lacking documentation:
semmle/python/frameworks/Django.qll  Django::PrivateDjango::DjangoRouteHandler                                    module
semmle/python/frameworks/Django.qll  Django::PrivateDjango::DjangoRouteHandler::Range                             class
semmle/python/frameworks/Django.qll  Django::PrivateDjango::DjangoRouteHandler::StandardDjangoRouteHandlers       class
semmle/python/frameworks/Django.qll  Django::PrivateDjango::django::conf::conf_urls                               module
semmle/python/frameworks/Django.qll  Django::PrivateDjango::django::db::DjangoDbConnection                        class
semmle/python/frameworks/Django.qll  Django::PrivateDjango::django::http::response::HttpResponse::baseClassRef/0  classless-predicate

I guess I should just go ahead and write QLDocs for them ...

this is just pure Django project for now, (and very much a copy of the one in `django-v2-v3`), to make it easier to see the changes needed to set up Django REST framework.

I had to make the Django and PrivateDjango modeling non-private :O

Had to expose even more things, and had to make the `DjangoRouteHandler` modeling more flexible so I could extend the char-pred in a different file.

Added internal helper `.qll` file as well

yoff

Generally looks good.

I caught this in the doc:

Any APIException exceptions will be caught and mediated into appropriate responses.

Could that lead to unintended exposure of information?

The rest framework also seems to provide some rich routing capabilities, but I am happy to postpone any modelling of these until it has been established as impactful.

python/ql/lib/semmle/python/frameworks/internal/SubclassFinder.qll

yoff · 2021-11-11T11:17:56Z

python/ql/lib/semmle/python/frameworks/RestFramework.qll

+   */
+  private class ModeledApiViewClasses extends Django::Views::View::ModeledSubclass {
+    ModeledApiViewClasses() {
+      this = API::moduleImport("rest_framework").getMember("views").getMember("APIView") or


Does this pass the autoformatter? I would have thought that the or should be on its own line.

Yes, I was a bit surprised the first time I saw this as well, but apparently there are 2 accepts way of using or:

foo() or bar() or baz()

or

foo() or bar() or baz()

Slightly embarrassingly I had problems getting the quick-eval SubclassFinder query to output \n without tripping up the extension, so that why I ended up with this way of doing this (which is certainly non-standard for the python CodeQL code)

Also note that the autoformatter runs as part of our tests, so it would complain if the formatting was wrong

I would prefer that it generated code that looked more familiar, but if it is legal, we can fix the aesthetics later. (Indeed I was confused about the autoformat check not firing..)

Ask and you shall receive 😊

yoff · 2021-11-11T11:32:06Z

python/ql/lib/semmle/python/frameworks/internal/SubclassFinder.qll

+  /**
+   * Holds if `newModelFullyQualified` describes either a new subclass, or a new alias, belonging to `spec` that we should include in our automated modeling.
+   * This new element is defined by `ast`, which is defined at `loc` in the module `mod`.
+   */
+  query predicate newModel(
+    FindSubclassesSpec spec, string newModelFullyQualified, AstNode ast, Module mod, Location loc
+  ) {
+    (
+      newSubclass(spec, newModelFullyQualified, ast, mod, loc)
+      or
+      newDirectAlias(spec, newModelFullyQualified, ast, mod, loc)
+      or
+      newImportStar(spec, newModelFullyQualified, ast, mod, _, _, loc)
+    )
+  }


I would like this predicate to be moved up. I found myself reding this first and its constituents later (even though they all interdepend given the recursion). I would say just below FindSubclassesSpecz (so currently to line 50).

yoff · 2021-11-11T11:33:08Z

python/ql/lib/semmle/python/frameworks/internal/SubclassFinder.qll

+        concat(string newModelFullyQualified |
+          newModel(any(MySpec spec), newModelFullyQualified, _, _, _)
+        |
+          fullyQualifiedToAPIGraphPath(newModelFullyQualified), " or this = API::"


If only newModelFullyQualified were an API ::Node, so you could just use toString...

yoff · 2021-11-11T11:34:12Z

python/ql/lib/semmle/python/frameworks/internal/SubclassFinder.qll

+  // inherint problem with API graphs is that there doesn't need to exist a result for all
+  // the stuff we have already modeled... as an example, the following query has no
+  // results when evaluated against Django
+  //
+  // select API::moduleImport("django")
+  //       .getMember("contrib")
+  //       .getMember("admin")
+  //       .getMember("views")
+  //       .getMember("main")
+  //       .getMember("ChangeListSearchForm")


Is this the explanation for why you need to build strings? Could it be moved up and elaborated into such an argument?

yes. I've tried to highlight that now. Please add a suggestion if what I did was not sufficient :blush

RasmusWL · 2021-11-12T09:51:41Z

Any APIException exceptions will be caught and mediated into appropriate responses.

Good find 👍 I'll take a look

Co-authored-by: yoff <lerchedahl@gmail.com>

yoff

One small suggestion, otherwise looks good. Nice work, including the exceptions 👍

yoff · 2021-11-12T10:55:42Z

python/ql/lib/semmle/python/frameworks/RestFramework.qll

+        result = this.getArgByName("detail")
+      }
+
+      // How to support the `headers` argument here?


Is that even honoured when an exception is raised?

That was a bad copy-paste, I have fixed that up

yoff · 2021-11-12T11:09:11Z

python/ql/lib/semmle/python/frameworks/internal/SubclassFinder.qll

+  // ---------------------------------------------------------------------------
+  // Implementation below
+  // ---------------------------------------------------------------------------
+  //
+  // inherent problem with API graphs is that there doesn't need to exist a result for
+  // all the stuff we have already modeled... as an example, the following query has no
+  // results when evaluated against a django/django DB
+  //
+  // select API::moduleImport("django")
+  //       .getMember("contrib")
+  //       .getMember("admin")
+  //       .getMember("views")
+  //       .getMember("main")
+  //       .getMember("ChangeListSearchForm")
+  //
+  // therefore we use fully qualified names to capture new classes/new aliases.
+  //
+  // note that this implementation was originally created to help with automatically
+  // modeling packages in mind, and was just copied for this purpose. See
+  // https://github.com/github/codeql/pull/5632 for more discussion. I wanted to get
+  // this into the codeql-repo, so it could be of use when modeling 3rd party libraries,
+  // and save some manual effort.


I would rewrite it slightly:

Suggested change

// ---------------------------------------------------------------------------

// Implementation below

// ---------------------------------------------------------------------------

//

// inherent problem with API graphs is that there doesn't need to exist a result for

// all the stuff we have already modeled... as an example, the following query has no

// results when evaluated against a django/django DB

//

// select API::moduleImport("django")

// .getMember("contrib")

// .getMember("admin")

// .getMember("views")

// .getMember("main")

// .getMember("ChangeListSearchForm")

//

// therefore we use fully qualified names to capture new classes/new aliases.

//

// note that this implementation was originally created to help with automatically

// modeling packages in mind, and was just copied for this purpose. See

// https://github.com/github/codeql/pull/5632 for more discussion. I wanted to get

// this into the codeql-repo, so it could be of use when modeling 3rd party libraries,

// and save some manual effort.

// ---------------------------------------------------------------------------

// Implementation below

// ---------------------------------------------------------------------------

// We are looking to find all subclassed of the already modelled classes, and

// ideally we would identify an `API::Node` for each (then `toString` would give

// the API path).

//

// However, an inherent problem with API graphs is that there doesn't need to exist a result for

// all the stuff we have already modeled... as an example, the following query has no

// results when evaluated against a django/django DB

//

// select API::moduleImport("django")

// .getMember("contrib")

// .getMember("admin")

// .getMember("views")

// .getMember("main")

// .getMember("ChangeListSearchForm")

//

// Therefore we use fully qualified names to capture new classes/new aliases, and transform these

// into API paths, see `fullyQualifiedToAPIGraphPath`.

//

// Further automation is planned her, see

// https://github.com/github/codeql/pull/5632 for more discussion.

I didn't fully agree, but have rewritten based on your suggestion 👍

yoff

LGTM

github-actions bot added documentation Python labels Nov 1, 2021

RasmusWL and others added 15 commits November 2, 2021 10:55

Python: Add RequestHandler meta query

f1307b7

Python: Set up test for Django REST framework

9d84315

this is just pure Django project for now, (and very much a copy of the one in `django-v2-v3`), to make it easier to see the changes needed to set up Django REST framework.

Python: Add simple Django REST framework code

9bbf08d

Python: Add examples of class/function based views

095f896

Python: Add rest_framework taint tests

75e2555

Python: Add note about .method

a64e939

Python: Add initial rest_framework modeling

222db37

I had to make the Django and PrivateDjango modeling non-private :O

Python: rest_framework.decorators.api_view handling

57e13c6

Had to expose even more things, and had to make the `DjangoRouteHandler` modeling more flexible so I could extend the char-pred in a different file.

Python: Add basic rest_framework Request modeling

5d77e62

Python: Add rest_framework Request taint modeling

62d3063

Python: Model known APIView subclasses

13815fe

Added internal helper `.qll` file as well

Python: Add rest_framework Response modeling

a7e4e5e

Python: Add change-note

fd12b14

Python: Fix experimental Django.qll

5c2734c

Python: Add some missing QLDocs

83389be

RasmusWL force-pushed the django-rest-framework branch from 78c748b to 83389be Compare November 2, 2021 10:03

RasmusWL marked this pull request as ready for review November 2, 2021 10:26

RasmusWL requested a review from a team as a code owner November 2, 2021 10:26

Merge branch 'main' into django-rest-framework

cb6bcad

yoff requested changes Nov 11, 2021

View reviewed changes

RasmusWL and others added 3 commits November 12, 2021 10:57

Python: Apply suggestions from code review

f48ecb1

Co-authored-by: yoff <lerchedahl@gmail.com>

Python: SubclassFinder: reorder + comment

62e58b5

Python: Model rest_framework.exceptions.APIException

5e4b866

RasmusWL requested a review from yoff November 12, 2021 10:38

yoff requested changes Nov 12, 2021

View reviewed changes

Python: Remove copy-pasted comment

f7b5332

RasmusWL added 2 commits November 12, 2021 13:29

Python: Expand on SubclassFinder implementation note

de69e4c

Python: Adjust generated code to be more familiar

491f72b

RasmusWL requested a review from yoff November 12, 2021 12:31

yoff approved these changes Nov 12, 2021

View reviewed changes

yoff merged commit 9f614b1 into github:main Nov 12, 2021

RasmusWL deleted the django-rest-framework branch November 12, 2021 13:29

RasmusWL mentioned this pull request Jun 19, 2023

Python: Prototype for automated modeling #5632

Closed

Python: Model Django REST framework #7016

Python: Model Django REST framework #7016

Uh oh!

Conversation

RasmusWL commented Nov 1, 2021

Uh oh!

RasmusWL commented Nov 2, 2021

Uh oh!

yoff 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!

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!

RasmusWL commented Nov 12, 2021

Uh oh!

yoff left a comment

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 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!