Python: Add modeling of coroutine routes in aiohttp.web

RasmusWL · RasmusWL · commit 3cbb909a3a4a · 2021-06-03T10:55:33.000+02:00
diff --git a/docs/codeql/support/reusables/frameworks.rst b/docs/codeql/support/reusables/frameworks.rst
@@ -152,6 +152,7 @@ Python built-in support
    :widths: auto
 
    Name, Category
+   aiohttp.web, Web framework
    Django, Web framework
    Flask, Web framework
    Tornado, Web framework
diff --git a/python/ql/src/semmle/python/Frameworks.qll b/python/ql/src/semmle/python/Frameworks.qll
@@ -4,6 +4,7 @@
 
 // If you add modeling of a new framework/library, remember to add it it to the docs in
 // `docs/codeql/support/reusables/frameworks.rst`
+private import semmle.python.frameworks.Aiohttp
 private import semmle.python.frameworks.Cryptodome
 private import semmle.python.frameworks.Cryptography
 private import semmle.python.frameworks.Dill
diff --git a/python/ql/src/semmle/python/frameworks/Aiohttp.qll b/python/ql/src/semmle/python/frameworks/Aiohttp.qll
@@ -0,0 +1,141 @@
+/**
+ * Provides classes modeling security-relevant aspects of the `aiohttp` PyPI package.
+ * See https://docs.aiohttp.org/en/stable/index.html
+ */
+
+private import python
+private import semmle.python.dataflow.new.DataFlow
+private import semmle.python.Concepts
+private import semmle.python.ApiGraphs
+private import semmle.python.frameworks.internal.PoorMansFunctionResolution
+
+/**
+ * INTERNAL: Do not use.
+ *
+ * Provides models for the web server part (`aiohttp.web`) of the `aiohttp` PyPI package.
+ * See https://docs.aiohttp.org/en/stable/web.html
+ */
+module AiohttpWebModel {
+  /** Gets a reference to a `aiohttp.web.Application` instance. */
+  API::Node applicationInstance() {
+    // Not sure whether you're allowed to add routes _after_ starting the app, for
+    // example in the middle of handling a http request... but I'm guessing that for 99%
+    // for all code, not modeling that `request.app` is a reference to an application
+    // should be good enough for the route-setup part of the modeling :+1:
+    result = API::moduleImport("aiohttp").getMember("web").getMember("Application").getReturn()
+  }
+
+  /** Gets a reference to a `aiohttp.web.UrlDispatcher` instance. */
+  API::Node urlDispathcerInstance() {
+    result = API::moduleImport("aiohttp").getMember("web").getMember("UrlDispatcher").getReturn()
+    or
+    result = applicationInstance().getMember("router")
+  }
+
+  // -- route modeling --
+
+  /** A route setup in `aiohttp.web` */
+  abstract class AiohttpRouteSetup extends HTTP::Server::RouteSetup::Range {
+    override Parameter getARoutedParameter() { none() }
+
+    override string getFramework() { result = "aiohttp.web" }
+  }
+
+  /** An aiohttp route setup that uses coroutines (async function) as request handler. */
+  abstract class AiohttpCoroutineRouteSetup extends AiohttpRouteSetup {
+    /** Gets the argument specifying the request handler (which is a coroutine/async function) */
+    abstract DataFlow::Node getRequestHandlerArg();
+
+    override Function getARequestHandler() {
+      this.getRequestHandlerArg() = poorMansFunctionTracker(result)
+    }
+  }
+
+  /**
+   * A route-setup from `add_route` or any of `add_get`, `add_post`, etc. on an
+   *    `aiohttp.web.UrlDispatcher`.
+   */
+  class AiohttpUrlDispatcherAddRouteCall extends AiohttpCoroutineRouteSetup, DataFlow::CallCfgNode {
+    /** At what index route arguments starts, so we can handle "route" version together with get/post/... */
+    int routeArgsStart;
+
+    AiohttpUrlDispatcherAddRouteCall() {
+      this = urlDispathcerInstance().getMember("add_" + HTTP::httpVerbLower()).getACall() and
+      routeArgsStart = 0
+      or
+      this = urlDispathcerInstance().getMember("add_route").getACall() and
+      routeArgsStart = 1
+    }
+
+    override DataFlow::Node getUrlPatternArg() {
+      result in [this.getArg(routeArgsStart + 0), this.getArgByName("path")]
+    }
+
+    override DataFlow::Node getRequestHandlerArg() {
+      result in [this.getArg(routeArgsStart + 1), this.getArgByName("handler")]
+    }
+  }
+
+  /**
+   * A route-setup from using `route` or any of `get`, `post`, etc. functions from `aiohttp.web`.
+   *
+   * Note that technically, this does not set up a route in itself (since it needs to be added to an application first).
+   * However, modeling this way makes it easier, and we don't expect it to lead to many problems.
+   */
+  class AiohttpWebRouteCall extends AiohttpCoroutineRouteSetup, DataFlow::CallCfgNode {
+    /** At what index route arguments starts, so we can handle "route" version together with get/post/... */
+    int routeArgsStart;
+
+    AiohttpWebRouteCall() {
+      exists(string funcName |
+        funcName = HTTP::httpVerbLower() and
+        routeArgsStart = 0
+        or
+        funcName = "route" and
+        routeArgsStart = 1
+      |
+        this = API::moduleImport("aiohttp").getMember("web").getMember(funcName).getACall()
+      )
+    }
+
+    override DataFlow::Node getUrlPatternArg() {
+      result in [this.getArg(routeArgsStart + 0), this.getArgByName("path")]
+    }
+
+    override DataFlow::Node getRequestHandlerArg() {
+      result in [this.getArg(routeArgsStart + 1), this.getArgByName("handler")]
+    }
+  }
+
+  /** A route-setup from using a `route` or any of `get`, `post`, etc. decorators from a `aiohttp.web.RouteTableDef`. */
+  class AiohttpRouteTableDefRouteCall extends AiohttpCoroutineRouteSetup, DataFlow::CallCfgNode {
+    /** At what index route arguments starts, so we can handle "route" version together with get/post/... */
+    int routeArgsStart;
+
+    AiohttpRouteTableDefRouteCall() {
+      exists(string decoratorName |
+        decoratorName = HTTP::httpVerbLower() and
+        routeArgsStart = 0
+        or
+        decoratorName = "route" and
+        routeArgsStart = 1
+      |
+        this =
+          API::moduleImport("aiohttp")
+              .getMember("web")
+              .getMember("RouteTableDef")
+              .getReturn()
+              .getMember(decoratorName)
+              .getACall()
+      )
+    }
+
+    override DataFlow::Node getUrlPatternArg() {
+      result in [this.getArg(routeArgsStart + 0), this.getArgByName("path")]
+    }
+
+    override DataFlow::Node getRequestHandlerArg() { none() }
+
+    override Function getARequestHandler() { result.getADecorator() = this.asExpr() }
+  }
+}
diff --git a/python/ql/test/library-tests/frameworks/aiohttp/routing_test.py b/python/ql/test/library-tests/frameworks/aiohttp/routing_test.py
@@ -15,55 +15,55 @@
 if True:
 
     # `app.add_routes` with list
-    async def foo(request):  # $ MISSING: requestHandler
+    async def foo(request):  # $ requestHandler
         return web.Response(text="foo")
 
-    async def foo2(request):  # $ MISSING: requestHandler
+    async def foo2(request):  # $ requestHandler
         return web.Response(text="foo2")
 
-    async def foo3(request):  # $ MISSING: requestHandler
+    async def foo3(request):  # $ requestHandler
         return web.Response(text="foo3")
 
     app.add_routes([
-        web.get("/foo", foo),  # $ MISSING: routeSetup
-        web.route("*", "/foo2", foo2), # $ MISSING: routeSetup
-        web.get(path="/foo3", handler=foo3), # $ MISSING: routeSetup
+        web.get("/foo", foo),  # $ routeSetup="/foo"
+        web.route("*", "/foo2", foo2), # $ routeSetup="/foo2"
+        web.get(path="/foo3", handler=foo3), # $ routeSetup="/foo3"
     ])
 
 
     # using decorator
     routes = web.RouteTableDef()
 
-    @routes.get("/bar")  # $ MISSING: routeSetup
-    async def bar(request):  # $ MISSING: requestHandler
+    @routes.get("/bar")  # $ routeSetup="/bar"
+    async def bar(request):  # $ requestHandler
         return web.Response(text="bar")
 
-    @routes.route("*", "/bar2")  # $ MISSING: routeSetup
-    async def bar2(request):  # $ MISSING: requestHandler
+    @routes.route("*", "/bar2")  # $ routeSetup="/bar2"
+    async def bar2(request):  # $ requestHandler
         return web.Response(text="bar2")
 
-    @routes.get(path="/bar3")  # $ MISSING: routeSetup
-    async def bar3(request):  # $ MISSING: requestHandler
+    @routes.get(path="/bar3")  # $ routeSetup="/bar3"
+    async def bar3(request):  # $ requestHandler
         return web.Response(text="bar3")
 
     app.add_routes(routes)
 
 
     # `app.router.add_get` / `app.router.add_route`
-    async def baz(request):  # $ MISSING: requestHandler
+    async def baz(request):  # $ requestHandler
         return web.Response(text="baz")
 
-    app.router.add_get("/baz", baz)  # $ MISSING: routeSetup
+    app.router.add_get("/baz", baz)  # $ routeSetup="/baz"
 
-    async def baz2(request):  # $ MISSING: requestHandler
+    async def baz2(request):  # $ requestHandler
         return web.Response(text="baz2")
 
-    app.router.add_route("*", "/baz2", baz2)  # $ MISSING: routeSetup
+    app.router.add_route("*", "/baz2", baz2)  # $ routeSetup="/baz2"
 
-    async def baz3(request):  # $ MISSING: requestHandler
+    async def baz3(request):  # $ requestHandler
         return web.Response(text="baz3")
 
-    app.router.add_get(path="/baz3", handler=baz3)  # $ MISSING: routeSetup
+    app.router.add_get(path="/baz3", handler=baz3)  # $ routeSetup="/baz3"
 
 
 ## Using classes / views
@@ -76,7 +76,7 @@ async def foo_handler(self, request):  # $ MISSING: requestHandler
             return web.Response(text="MyCustomHandlerClass.foo")
 
     my_custom_handler = MyCustomHandlerClass()
-    app.router.add_get("/MyCustomHandlerClass/foo", my_custom_handler.foo_handler)   # $ MISSING: routeSetup
+    app.router.add_get("/MyCustomHandlerClass/foo", my_custom_handler.foo_handler)   # $ routeSetup="/MyCustomHandlerClass/foo"
 
     # Using `web.View`
     # ---------------
@@ -116,12 +116,12 @@ async def get(self):  # $ MISSING: requestHandler
 if True:
     # see https://docs.aiohttp.org/en/stable/web_quickstart.html#variable-resources
 
-    async def matching(request: web.Request):  # $ MISSING: requestHandler
+    async def matching(request: web.Request):  # $ requestHandler
         name = request.match_info['name']
         number = request.match_info['number']
         return web.Response(text="matching name={} number={}".format(name, number))
 
-    app.router.add_get("/matching/{name}/{number:\d+}", matching)  # $ MISSING: routeSetup
+    app.router.add_get(r"/matching/{name}/{number:\d+}", matching)  # $ routeSetup="/matching/{name}/{number:\d+}"
 
 ## ======= ##
 ## subapps ##
@@ -130,10 +130,10 @@ async def matching(request: web.Request):  # $ MISSING: requestHandler
 if True:
     subapp = web.Application()
 
-    async def subapp_handler(request):  # $ MISSING: requestHandler
+    async def subapp_handler(request):  # $ requestHandler
         return web.Response(text="subapp_handler")
 
-    subapp.router.add_get("/subapp_handler", subapp_handler)  # $ MISSING: routeSetup
+    subapp.router.add_get("/subapp_handler", subapp_handler)  # $ routeSetup="/subapp_handler"
 
     app.add_subapp("/my_subapp", subapp)
 
@@ -146,11 +146,11 @@ async def subapp_handler(request):  # $ MISSING: requestHandler
 ## ================================ ##
 
 if True:
-    async def manual_dispatcher_instance(request):  # $ MISSING: requestHandler
+    async def manual_dispatcher_instance(request):  # $ requestHandler
         return web.Response(text="manual_dispatcher_instance")
 
     url_dispatcher = web.UrlDispatcher()
-    url_dispatcher.add_get("/manual_dispatcher_instance", manual_dispatcher_instance)  # $ MISSING: routeSetup
+    url_dispatcher.add_get("/manual_dispatcher_instance", manual_dispatcher_instance)  # $ routeSetup="/manual_dispatcher_instance"
 
     subapp2 = web.Application(router=url_dispatcher)
     app.add_subapp("/manual_dispatcher_instance_app", subapp2)
