JS: Reduce reliance on RouteHandler in Express model

Author: asgerf

javascript/ql/src/semmle/javascript/frameworks/Express.qll

@@ -390,7 +390,7 @@ module Express {
   }
 
   /** An Express response source. */
-  abstract private class ResponseSource extends HTTP::Servers::ResponseSource { }
+  abstract class ResponseSource extends HTTP::Servers::ResponseSource { }
 
   /**
    * An Express response source, that is, the response parameter of a
@@ -421,7 +421,7 @@ module Express {
   }
 
   /** An Express request source. */
-  abstract private class RequestSource extends HTTP::Servers::RequestSource { }
+  abstract class RequestSource extends HTTP::Servers::RequestSource { }
 
   /**
    * An Express request source, that is, the request parameter of a
@@ -465,73 +465,98 @@ module Express {
    * Gets a reference to the "query" object from a request-object originating from route-handler `rh`.
    */
   DataFlow::SourceNode getAQueryObjectReference(DataFlow::TypeTracker t, RouteHandler rh) {
-    t.startInProp("query") and
-    result = rh.getARequestSource()
-    or
-    exists(DataFlow::TypeTracker t2 | result = getAQueryObjectReference(t2, rh).track(t2, t))
+    result = queryRef(rh.getARequestSource(), t)
   }
 
   /**
    * Gets a reference to the "params" object from a request-object originating from route-handler `rh`.
    */
   DataFlow::SourceNode getAParamsObjectReference(DataFlow::TypeTracker t, RouteHandler rh) {
-    t.startInProp("params") and
-    result = rh.getARequestSource()
+    result = paramsRef(rh.getARequestSource(), t)
+  }
+
+  /** The input parameter to an `app.param()` route handler. */
+  private class ParamHandlerInputAccess extends HTTP::RequestInputAccess {
+    RouteHandler rh;
+
+    ParamHandlerInputAccess() {
+      exists(RouteSetup setup | rh = setup.getARouteHandler() |
+        this = DataFlow::parameterNode(rh.getRouteHandlerParameter("parameter"))
+      )
+    }
+
+    override HTTP::RouteHandler getRouteHandler() { result = rh }
+
+    override string getKind() { result = "parameter" }
+  }
+
+  /** Gets a data flow node referring to `req.query`. */
+  private DataFlow::SourceNode queryRef(RequestSource req, DataFlow::TypeTracker t) {
+    t.start() and
+    result = req.ref().getAPropertyRead("query")
+    or
+    exists(DataFlow::TypeTracker t2 | result = queryRef(req, t2).track(t2, t))
+  }
+
+  /** Gets a data flow node referring to `req.query`. */
+  private DataFlow::SourceNode queryRef(RequestSource req) {
+    result = queryRef(req, DataFlow::TypeTracker::end())
+  }
+
+  /** Gets a data flow node referring to `req.params`. */
+  private DataFlow::SourceNode paramsRef(RequestSource req, DataFlow::TypeTracker t) {
+    t.start() and
+    result = req.ref().getAPropertyRead("params")
     or
-    exists(DataFlow::TypeTracker t2 | result = getAParamsObjectReference(t2, rh).track(t2, t))
+    exists(DataFlow::TypeTracker t2 | result = paramsRef(req, t2).track(t2, t))
+  }
+
+  /** Gets a data flow node referring to `req.params`. */
+  private DataFlow::SourceNode paramsRef(RequestSource req) {
+    result = paramsRef(req, DataFlow::TypeTracker::end())
   }
 
   /**
    * An access to a user-controlled Express request input.
    */
   class RequestInputAccess extends HTTP::RequestInputAccess {
-    RouteHandler rh;
+    RequestSource request;
     string kind;
 
     RequestInputAccess() {
       kind = "parameter" and
-      this =
-        [
-          getAQueryObjectReference(DataFlow::TypeTracker::end(), rh),
-          getAParamsObjectReference(DataFlow::TypeTracker::end(), rh)
-        ].getAPropertyRead()
+      this = [queryRef(request), paramsRef(request)].getAPropertyRead()
       or
-      exists(DataFlow::SourceNode request | request = rh.getARequestSource().ref() |
+      exists(DataFlow::SourceNode ref | ref = request.ref() |
         kind = "parameter" and
-        this = request.getAMethodCall("param")
+        this = ref.getAMethodCall("param")
         or
         // `req.originalUrl`
         kind = "url" and
-        this = request.getAPropertyRead("originalUrl")
+        this = ref.getAPropertyRead("originalUrl")
         or
         // `req.cookies`
         kind = "cookie" and
-        this = request.getAPropertyRead("cookies")
+        this = ref.getAPropertyRead("cookies")
         or
         // `req.files`, treated the same as `req.body`.
         // `express-fileupload` uses .files, and `multer` uses .files or .file
         kind = "body" and
-        this = request.getAPropertyRead(["files", "file"])
-      )
-      or
-      kind = "body" and
-      this.asExpr() = rh.getARequestBodyAccess()
-      or
-      // `value` in `router.param('foo', (req, res, next, value) => { ... })`
-      kind = "parameter" and
-      exists(RouteSetup setup | rh = setup.getARouteHandler() |
-        this = DataFlow::parameterNode(rh.getRouteHandlerParameter("parameter"))
+        this = ref.getAPropertyRead(["files", "file"])
+        or
+        kind = "body" and
+        this = ref.getAPropertyRead("body")
       )
     }
 
-    override RouteHandler getRouteHandler() { result = rh }
+    override RouteHandler getRouteHandler() { result = request.getRouteHandler() }
 
     override string getKind() { result = kind }
 
     override predicate isUserControlledObject() {
       kind = "body" and
       exists(ExpressLibraries::BodyParser bodyParser, RouteHandlerExpr expr |
-        expr.getBody() = rh and
+        expr.getBody() = request.getRouteHandler() and
         bodyParser.producesUserControlledObjects() and
         bodyParser.flowsToExpr(expr.getAMatchingAncestor())
       )
@@ -542,43 +567,26 @@ module Express {
       forall(ExpressLibraries::BodyParser bodyParser | bodyParser.producesUserControlledObjects())
       or
       kind = "parameter" and
-      exists(DataFlow::Node request | request = DataFlow::valueNode(rh.getARequestExpr()) |
-        this.(DataFlow::MethodCallNode).calls(request, "param")
-      )
+      this = request.ref().getAMethodCall("param")
       or
       // `req.query.name`
       kind = "parameter" and
-      this = getAQueryObjectReference(DataFlow::TypeTracker::end(), rh).getAPropertyRead()
+      this = queryRef(request).getAPropertyRead()
     }
   }
 
   /**
    * An access to a header on an Express request.
    */
   private class RequestHeaderAccess extends HTTP::RequestHeaderAccess {
-    RouteHandler rh;
+    RequestSource request;
 
     RequestHeaderAccess() {
-      exists(DataFlow::Node request | request = DataFlow::valueNode(rh.getARequestExpr()) |
-        exists(string methodName |
-          // `req.get(...)` or `req.header(...)`
-          this.(DataFlow::MethodCallNode).calls(request, methodName)
-        |
-          methodName = "get" or
-          methodName = "header"
-        )
-        or
-        exists(DataFlow::PropRead headers |
-          // `req.headers.name`
-          headers.accesses(request, "headers") and
-          this = headers.getAPropertyRead()
-        )
-        or
-        exists(string propName | propName = "host" or propName = "hostname" |
-          // `req.host` and `req.hostname` are derived from headers
-          this.(DataFlow::PropRead).accesses(request, propName)
-        )
-      )
+      this = request.ref().getAMethodCall(["get", "header"])
+      or
+      this = request.ref().getAPropertyRead("headers").getAPropertyRead()
+      or
+      this = request.ref().getAPropertyRead(["host", "hostname"])
     }
 
     override string getAHeaderName() {
@@ -591,22 +599,15 @@ module Express {
       )
     }
 
-    override RouteHandler getRouteHandler() { result = rh }
+    override RouteHandler getRouteHandler() { result = request.getRouteHandler() }
 
     override string getKind() { result = "header" }
   }
 
   /**
    * HTTP headers created by Express calls
    */
-  abstract private class ExplicitHeader extends HTTP::ExplicitHeaderDefinition {
-    Expr response;
-
-    /**
-     * Gets the response expression that this header is set on.
-     */
-    Expr getResponse() { result = response }
-  }
+  abstract private class ExplicitHeader extends HTTP::ExplicitHeaderDefinition { }
 
   /**
    * Holds if `e` is an HTTP request object.
@@ -635,16 +636,13 @@ module Express {
    * An invocation of the `redirect` method of an HTTP response object.
    */
   private class RedirectInvocation extends HTTP::RedirectInvocation, MethodCallExpr {
-    RouteHandler rh;
+    ResponseSource response;
 
-    RedirectInvocation() {
-      getReceiver() = rh.getAResponseExpr() and
-      getMethodName() = "redirect"
-    }
+    RedirectInvocation() { this = response.ref().getAMethodCall("redirect").asExpr() }
 
     override Expr getUrlArgument() { result = getLastArgument() }
 
-    override RouteHandler getRouteHandler() { result = rh }
+    override RouteHandler getRouteHandler() { result = response.getRouteHandler() }
   }
 
   /**
@@ -662,21 +660,18 @@ module Express {
    * An invocation of the `set` or `header` method on an HTTP response object that
    * sets multiple headers.
    */
-  class SetMultipleHeaders extends ExplicitHeader, DataFlow::ValueNode {
-    override MethodCallExpr astNode;
-    RouteHandler rh;
+  class SetMultipleHeaders extends ExplicitHeader, DataFlow::MethodCallNode {
+    ResponseSource response;
 
     SetMultipleHeaders() {
-      astNode.getReceiver() = rh.getAResponseExpr() and
-      response = astNode.getReceiver() and
-      astNode.getMethodName() = any(string n | n = "set" or n = "header") and
-      astNode.getNumArgument() = 1
+      this = response.ref().getAMethodCall(["set", "header"]) and
+      getNumArgument() = 1
     }
 
     /**
      * Gets a reference to the multiple headers object that is to be set.
      */
-    private DataFlow::SourceNode getAHeaderSource() { result.flowsToExpr(astNode.getArgument(0)) }
+    private DataFlow::SourceNode getAHeaderSource() { result.flowsTo(getArgument(0)) }
 
     override predicate definesExplicitly(string headerName, Expr headerValue) {
       exists(string header |
@@ -685,7 +680,7 @@ module Express {
       )
     }
 
-    override RouteHandler getRouteHandler() { result = rh }
+    override RouteHandler getRouteHandler() { result = response.getRouteHandler() }
 
     override Expr getNameExpr() {
       exists(DataFlow::PropWrite write | getAHeaderSource().getAPropertyWrite() = write |
@@ -705,31 +700,26 @@ module Express {
    * An argument passed to the `send` or `end` method of an HTTP response object.
    */
   private class ResponseSendArgument extends HTTP::ResponseSendArgument {
-    RouteHandler rh;
+    ResponseSource response;
 
-    ResponseSendArgument() {
-      exists(MethodCallExpr mce |
-        mce.calls(rh.getAResponseExpr(), "send") and
-        this = mce.getArgument(0)
-      )
-    }
+    ResponseSendArgument() { this = response.ref().getAMethodCall("send").getArgument(0).asExpr() }
 
-    override RouteHandler getRouteHandler() { result = rh }
+    override RouteHandler getRouteHandler() { result = response.getRouteHandler() }
   }
 
   /**
    * An invocation of the `cookie` method on an HTTP response object.
    */
   class SetCookie extends HTTP::CookieDefinition, MethodCallExpr {
-    RouteHandler rh;
+    ResponseSource response;
 
-    SetCookie() { calls(rh.getAResponseExpr(), "cookie") }
+    SetCookie() { this = response.ref().getAMethodCall("cookie").asExpr() }
 
     override Expr getNameArgument() { result = getArgument(0) }
 
     override Expr getValueArgument() { result = getArgument(1) }
 
-    override RouteHandler getRouteHandler() { result = rh }
+    override RouteHandler getRouteHandler() { result = response.getRouteHandler() }
   }
 
   /**
@@ -750,19 +740,19 @@ module Express {
    * An object passed to the `render` method of an HTTP response object.
    */
   class TemplateObjectInput extends DataFlow::Node {
-    RouteHandler rh;
+    ResponseSource response;
 
     TemplateObjectInput() {
       exists(DataFlow::MethodCallNode render |
-        render.calls(rh.getAResponseExpr().flow(), "render") and
+        render = response.ref().getAMethodCall("render") and
         this = render.getArgument(1)
       )
     }
 
     /**
      * Gets the route handler that uses this object.
      */
-    RouteHandler getRouteHandler() { result = rh }
+    RouteHandler getRouteHandler() { result = response.getRouteHandler() }
   }
 
   /**