Python: Support positional arguments in Django routes

change-notes/1.23/analysis-python.md

@@ -20,3 +20,8 @@
 |----------------------------|------------------------|------------|
 | Unreachable code | Fewer false positives | Analysis now accounts for uses of `contextlib.suppress` to suppress exceptions. |
 | `__iter__` method returns a non-iterator | Better alert message | Alert now highlights which class is expected to be an iterator. |
+
+
+## Changes to QL libraries
+
+* Django library now recognizes positional arguments from a `django.conf.urls.url` regex (Django version 1.x)

python/ql/src/semmle/python/web/django/General.qll

@@ -25,4 +25,16 @@ class DjangoRoute extends CallNode {
             regex.getGroupName(_, _) = result
         )
     }
+
+    /**
+      * Get the number of positional arguments that will be passed to the view.
+      * Will only return a result if there are no named arguments.
+      */
+    int getNumPositionalArguments() {
+        exists(DjangoRouteRegex regex |
+            django_route(this, regex.getAFlowNode(), _) and
+            not exists(string s | s = regex.getGroupName(_, _)) and
+            result = count(regex.getGroupNumber(_, _))
+        )
+    }
 }

python/ql/src/semmle/python/web/django/Request.qll

@@ -79,11 +79,15 @@ class DjangoClassBasedViewRequestArgument extends DjangoRequestSource {
 /** An argument specified in a url routing table */
 class DjangoRequestParameter extends HttpRequestTaintSource {
     DjangoRequestParameter() {
-        exists(DjangoRoute route |
-            this.(ControlFlowNode).getNode() = route
-                        .getViewFunction()
-                        .getScope()
-                        .getArgByName(route.getNamedArgument())
+        exists(DjangoRoute route, Function f |
+            f = route.getViewFunction().getScope() |
+            this.(ControlFlowNode).getNode() = f.getArgByName(route.getNamedArgument())
+            or
+            exists(int i | i >= 0 |
+                i < route.getNumPositionalArguments() and
+                // +1 because first argument is always the request
+                this.(ControlFlowNode).getNode() = f.getArg(i+1)
+            )
         )
     }
 

python/ql/test/library-tests/web/django/Sinks.expected

@@ -4,3 +4,7 @@
 | test.py:25 | BinaryExpr | externally controlled string |
 | test.py:26 | BinaryExpr | externally controlled string |
 | test.py:34 | BinaryExpr | externally controlled string |
+| test.py:46 | Attribute() | externally controlled string |
+| test.py:57 | Attribute() | externally controlled string |
+| test.py:60 | Attribute() | externally controlled string |
+| test.py:63 | Attribute() | externally controlled string |

python/ql/test/library-tests/web/django/Sources.expected

@@ -1,2 +1,11 @@
 | test.py:11 | request | django.request.HttpRequest |
 | test.py:31 | request | django.request.HttpRequest |
+| test.py:56 | arg0 | externally controlled string |
+| test.py:56 | request | django.request.HttpRequest |
+| test.py:59 | arg0 | externally controlled string |
+| test.py:59 | arg1 | externally controlled string |
+| test.py:59 | arg2 | externally controlled string |
+| test.py:59 | request | django.request.HttpRequest |
+| test.py:62 | arg0 | externally controlled string |
+| test.py:62 | arg1 | externally controlled string |
+| test.py:62 | request | django.request.HttpRequest |

python/ql/test/library-tests/web/django/test.py

@@ -34,7 +34,38 @@ def maybe_xss(request):
     resp.write("first name is " + first_name)
     return resp
 
-urlpatterns2 = [
+urlpatterns = [
     # Route to code_execution
     url(r'^maybe_xss$', maybe_xss, name='maybe_xss')
 ]
+
+
+# Non capturing group (we correctly identify page_number as a request parameter)
+
+def show_articles(request, page_number=1):
+    return HttpResponse('articles page: {}'.format(page_number))
+
+urlpatterns = [
+    # one pattern to support `articles/page-<n>` and ensuring that articles/ goes to page-1
+    url(r'articles/^(?:page-(?P<page_number>\d+)/)?$', show_articles),
+]
+
+
+# Positional arguments
+
+def xxs_positional_arg1(request, arg0):
+    return HttpResponse('xxs_positional_arg1: {}'.format(arg0))
+
+def xxs_positional_arg2(request, arg0, arg1, arg2):
+    return HttpResponse('xxs_positional_arg2: {} {} {}'.format(arg0, arg1, arg2))
+
+def xxs_positional_arg3(request, arg0, arg1):
+    return HttpResponse('xxs_positional_arg3: {} {}'.format(arg0, arg1))
+
+urlpatterns = [
+    # passing as positional argument is not the recommended way of doing things,
+    # but it is certainly possible
+    url(r'^(.+)$', xxs_positional_arg1, name='xxs_positional_arg1'),
+    url(r'^([^/]+)/([^/]+)/([^/]+)$', xxs_positional_arg2, name='xxs_positional_arg2'),
+    url(r'^([^/]+)/(?:foo|bar)/([^/]+)$', xxs_positional_arg3, name='xxs_positional_arg3'),
+]