github · semmle-qlci · May 18, 2020 · May 6, 2020 · May 7, 2020 · May 7, 2020
diff --git a/change-notes/1.25/analysis-javascript.md b/change-notes/1.25/analysis-javascript.md
@@ -3,6 +3,7 @@
 ## General improvements
 
 * Support for the following frameworks and libraries has been improved:
+  - [express](https://www.npmjs.com/package/express)
   - [fstream](https://www.npmjs.com/package/fstream)
   - [jGrowl](https://github.com/stanlemon/jGrowl)
   - [jQuery](https://jquery.com/)

diff --git a/javascript/ql/src/semmle/javascript/frameworks/ConnectExpressShared.qll b/javascript/ql/src/semmle/javascript/frameworks/ConnectExpressShared.qll
@@ -7,24 +7,104 @@
 import javascript
 
 module ConnectExpressShared {
+  /**
+   * String representing the signature of a route handler, that is,
+   * the list of parameters taken by the route handler.
+   *
+   * Concretely this is a comma-separated list of parameter kinds, which can be either
+   * `request`, `response`, `next`, `error`, or `parameter`, but this is considered an
+   * implementation detail.
+   */
+  private class RouteHandlerSignature extends string {
+    RouteHandlerSignature() {
+      this = "request,response" or
+      this = "request,response,next" or
+      this = "request,response,next,parameter" or
+      this = "error,request,response,next"
+    }
+
+    /** Gets the index of the parameter corresonding to the given `kind`, if any. */
+    pragma[noinline]
+    int getParameterIndex(string kind) { this.splitAt(",", result) = kind }
+
+    /** Gets the number of parameters taken by this signature. */
+    pragma[noinline]
+    int getArity() { result = count(getParameterIndex(_)) }
+
+    /** Holds if this signature takes a parameter of the given kind. */
+    predicate has(string kind) { exists(getParameterIndex(kind)) }
+  }
+
+  private module RouteHandlerSignature {
+    /** Gets the signature corresonding to `(req, res, next, param) => {...}`. */
+    RouteHandlerSignature requestResponseNextParameter() {
+      result = "request,response,next,parameter"
+    }
+
+    /** Gets the signature corresonding to `(req, res, next) => {...}`. */
+    RouteHandlerSignature requestResponseNext() { result = "request,response,next" }
+
+    /** Gets the signature corresonding to `(err, req, res, next) => {...}`. */
+    RouteHandlerSignature errorRequestResponseNext() { result = "error,request,response,next" }
+  }
+
+  /**
+   * Holds if `fun` appears to match the given signature based on parameter naming.
+   */
+  private predicate matchesSignature(Function function, RouteHandlerSignature sig) {
+    function.getNumParameter() = sig.getArity() and
+    function.getParameter(sig.getParameterIndex("request")).getName() = ["req", "request"] and
+    function.getParameter(sig.getParameterIndex("response")).getName() = ["res", "response"] and
+    (
+      sig.has("next")
+      implies
+      function.getParameter(sig.getParameterIndex("next")).getName() = ["next", "cb"]
+    )
+  }
+
+  /**
+   * Gets the parameter corresonding to the given `kind`, where `routeHandler` is interpreted as a
+   * route handler with the signature `sig`.
+   *
+   * This does not check if the function is actually a route handler or matches the signature in any way,
+   * so the caller should restrict the function accordingly.
+   */
+  pragma[inline]
+  private Parameter getRouteHandlerParameter(
+    Function routeHandler, RouteHandlerSignature sig, string kind
+  ) {
+    result = routeHandler.getParameter(sig.getParameterIndex(kind))
+  }
+
+  /**
+   * Gets the parameter of kind `kind` of a Connect/Express route parameter handler function.
+   *
+   * `kind` is one of: "error", "request", "response", "next".
+   */
+  pragma[inline]
+  Parameter getRouteParameterHandlerParameter(Function routeHandler, string kind) {
+    result =
+      getRouteHandlerParameter(routeHandler, RouteHandlerSignature::requestResponseNextParameter(),
+        kind)
+  }
+
   /**
    * Gets the parameter of kind `kind` of a Connect/Express route handler function.
    *
    * `kind` is one of: "error", "request", "response", "next".
    */
-  SimpleParameter getRouteHandlerParameter(Function routeHandler, string kind) {
-    exists(int index, int offset |
-      result = routeHandler.getParameter(index + offset) and
-      (if routeHandler.getNumParameter() = 4 then offset = 0 else offset = -1)
-    |
-      kind = "error" and index = 0
-      or
-      kind = "request" and index = 1
-      or
-      kind = "response" and index = 2
-      or
-      kind = "next" and index = 3
-    )
+  pragma[inline]
+  Parameter getRouteHandlerParameter(Function routeHandler, string kind) {
+    if routeHandler.getNumParameter() = 4
+    then
+      // For arity 4 there is ambiguity between (err, req, res, next) and (req, res, next, param)
+      // This predicate favors the 'err' signature whereas getRouteParameterHandlerParameter favors the other.
+      result =
+        getRouteHandlerParameter(routeHandler, RouteHandlerSignature::errorRequestResponseNext(),
+          kind)
+    else
+      result =
+        getRouteHandlerParameter(routeHandler, RouteHandlerSignature::requestResponseNext(), kind)
   }
 
   /**
@@ -34,39 +114,16 @@ module ConnectExpressShared {
    */
   class RouteHandlerCandidate extends HTTP::RouteHandlerCandidate {
     RouteHandlerCandidate() {
-      exists(string request, string response, string next, string error |
-        (request = "request" or request = "req") and
-        (response = "response" or response = "res") and
-        next = "next" and
-        (error = "error" or error = "err")
-      |
-        // heuristic: parameter names match the documentation
-        astNode.getNumParameter() >= 2 and
-        getRouteHandlerParameter(astNode, "request").getName() = request and
-        getRouteHandlerParameter(astNode, "response").getName() = response and
-        (
-          astNode.getNumParameter() >= 3
-          implies
-          getRouteHandlerParameter(astNode, "next").getName() = next
-        ) and
-        (
-          astNode.getNumParameter() = 4
-          implies
-          getRouteHandlerParameter(astNode, "error").getName() = error
-        ) and
-        not (
-          // heuristic: max four parameters (the server will only supply four arguments)
-          astNode.getNumParameter() > 4
-          or
-          // heuristic: not a class method (the server invokes this with a function call)
-          astNode = any(MethodDefinition def).getBody()
-          or
-          // heuristic: does not return anything (the server will not use the return value)
-          exists(astNode.getAReturnStmt().getExpr())
-          or
-          // heuristic: is not invoked (the server invokes this at a call site we cannot reason precisely about)
-          exists(DataFlow::InvokeNode cs | cs.getACallee() = astNode)
-        )
+      matchesSignature(astNode, _) and
+      not (
+        // heuristic: not a class method (the server invokes this with a function call)
+        astNode = any(MethodDefinition def).getBody()
+        or
+        // heuristic: does not return anything (the server will not use the return value)
+        exists(astNode.getAReturnStmt().getExpr())
+        or
+        // heuristic: is not invoked (the server invokes this at a call site we cannot reason precisely about)
+        exists(DataFlow::InvokeNode cs | cs.getACallee() = astNode)
       )
     }
   }

diff --git a/javascript/ql/src/semmle/javascript/frameworks/Express.qll b/javascript/ql/src/semmle/javascript/frameworks/Express.qll
@@ -127,14 +127,14 @@ module Express {
     /**
      * Gets the HTTP request type this is registered for, if any.
      *
-     * Has no result for `use` and `all` calls.
+     * Has no result for `use`, `all`, or `param` calls.
      */
     HTTP::RequestMethodName getRequestMethod() { result.toLowerCase() = getMethodName() }
 
     /**
      * Holds if this registers a route for all request methods.
      */
-    predicate handlesAllRequestMethods() { getMethodName() = "use" or getMethodName() = "all" }
+    predicate handlesAllRequestMethods() { getMethodName() = ["use", "all", "param"] }
 
     /**
      * Holds if this route setup sets up a route for the same
@@ -146,6 +146,11 @@ module Express {
       that.handlesAllRequestMethods() or
       this.getRequestMethod() = that.getRequestMethod()
     }
+
+    /**
+     * Holds if this route setup is a parameter handler, such as `app.param("foo", ...)`.
+     */
+    predicate isParameterHandler() { getMethodName() = "param" }
   }
 
   /**
@@ -314,7 +319,7 @@ module Express {
     /**
      * Gets the parameter of kind `kind` of this route handler.
      *
-     * `kind` is one of: "error", "request", "response", "next".
+     * `kind` is one of: "error", "request", "response", "next", or "parameter".
      */
     abstract SimpleParameter getRouteHandlerParameter(string kind);
 
@@ -340,11 +345,14 @@ module Express {
   class StandardRouteHandler extends RouteHandler, HTTP::Servers::StandardRouteHandler,
     DataFlow::ValueNode {
     override Function astNode;
+    RouteSetup routeSetup;
 
-    StandardRouteHandler() { this = any(RouteSetup setup).getARouteHandler() }
+    StandardRouteHandler() { this = routeSetup.getARouteHandler() }
 
     override SimpleParameter getRouteHandlerParameter(string kind) {
-      result = getRouteHandlerParameter(astNode, kind)
+      if routeSetup.isParameterHandler()
+      then result = getRouteParameterHandlerParameter(astNode, kind)
+      else result = getRouteHandlerParameter(astNode, kind)
     }
   }
 
@@ -453,32 +461,31 @@ module Express {
     string kind;
 
     RequestInputAccess() {
-      exists(DataFlow::Node request | request = DataFlow::valueNode(rh.getARequestExpr()) |
+      exists(DataFlow::SourceNode request | request = rh.getARequestSource().ref() |
         kind = "parameter" and
         (
-          this.(DataFlow::MethodCallNode).calls(request, "param")
+          this = request.getAMethodCall("param")
           or
-          exists(DataFlow::PropRead base, string propName |
-            // `req.params.name` or `req.query.name`
-            base.accesses(request, propName) and
-            this = base.getAPropertyReference(_)
-          |
-            propName = "params" or
-            propName = "query"
-          )
+          this = request.getAPropertyRead(["params", "query"]).getAPropertyRead()
         )
         or
         // `req.originalUrl`
         kind = "url" and
-        this.(DataFlow::PropRef).accesses(request, "originalUrl")
+        this = request.getAPropertyRead("originalUrl")
         or
         // `req.cookies`
         kind = "cookie" and
-        this.(DataFlow::PropRef).accesses(request, "cookies")
+        this = request.getAPropertyRead("cookies")
       )
       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"))
+      )
     }
 
     override RouteHandler getRouteHandler() { result = rh }
@@ -848,10 +855,14 @@ module Express {
    */
   private class TrackedRouteHandlerCandidateWithSetup extends RouteHandler,
     HTTP::Servers::StandardRouteHandler, DataFlow::FunctionNode {
-    TrackedRouteHandlerCandidateWithSetup() { this = any(RouteSetup s).getARouteHandler() }
+    RouteSetup routeSetup;
+
+    TrackedRouteHandlerCandidateWithSetup() { this = routeSetup.getARouteHandler() }
 
     override SimpleParameter getRouteHandlerParameter(string kind) {
-      result = getRouteHandlerParameter(astNode, kind)
+      if routeSetup.isParameterHandler()
+      then result = getRouteParameterHandlerParameter(astNode, kind)
+      else result = getRouteHandlerParameter(astNode, kind)
     }
   }
 

diff --git a/javascript/ql/src/semmle/javascript/frameworks/Firebase.qll b/javascript/ql/src/semmle/javascript/frameworks/Firebase.qll
@@ -219,12 +219,14 @@ module Firebase {
      */
     private class RouteHandler extends Express::RouteHandler, HTTP::Servers::StandardRouteHandler,
       DataFlow::ValueNode {
+      override Function astNode;
+
       RouteHandler() { this = any(RouteSetup setup).getARouteHandler() }
 
       override SimpleParameter getRouteHandlerParameter(string kind) {
-        kind = "request" and result = this.(DataFlow::FunctionNode).getParameter(0).getParameter()
+        kind = "request" and result = astNode.getParameter(0)
         or
-        kind = "response" and result = this.(DataFlow::FunctionNode).getParameter(1).getParameter()
+        kind = "response" and result = astNode.getParameter(1)
       }
     }
   }

diff --git a/javascript/ql/src/semmle/javascript/frameworks/HTTP.qll b/javascript/ql/src/semmle/javascript/frameworks/HTTP.qll
@@ -204,6 +204,20 @@ module HTTP {
      */
     abstract HeaderDefinition getAResponseHeader(string name);
 
+    /**
+     * Gets a request object originating from this route handler.
+     *
+     * Use `RequestSource.ref()` to get reference to this request object.
+     */
+    final Servers::RequestSource getARequestSource() { result.getRouteHandler() = this }
+
+    /**
+     * Gets a response object originating from this route handler.
+     *
+     * Use `ResponseSource.ref()` to get reference to this response object.
+     */
+    final Servers::ResponseSource getAResponseSource() { result.getRouteHandler() = this }
+
     /**
      * Gets an expression that contains a request object handled
      * by this handler.
@@ -296,14 +310,18 @@ module HTTP {
        */
       abstract RouteHandler getRouteHandler();
 
-      predicate flowsTo(DataFlow::Node nd) { ref(DataFlow::TypeTracker::end()).flowsTo(nd) }
+      /** DEPRECATED. Use `ref().flowsTo()` instead. */
+      deprecated predicate flowsTo(DataFlow::Node nd) { ref().flowsTo(nd) }
 
       private DataFlow::SourceNode ref(DataFlow::TypeTracker t) {
         t.start() and
         result = this
         or
         exists(DataFlow::TypeTracker t2 | result = ref(t2).track(t2, t))
       }
+
+      /** Gets a `SourceNode` that refers to this request object. */
+      DataFlow::SourceNode ref() { result = ref(DataFlow::TypeTracker::end()) }
     }
 
     /**
@@ -317,14 +335,18 @@ module HTTP {
        */
       abstract RouteHandler getRouteHandler();
 
-      predicate flowsTo(DataFlow::Node nd) { ref(DataFlow::TypeTracker::end()).flowsTo(nd) }
+      /** DEPRECATED. Use `ref().flowsTo()` instead. */
+      deprecated predicate flowsTo(DataFlow::Node nd) { ref().flowsTo(nd) }
 
       private DataFlow::SourceNode ref(DataFlow::TypeTracker t) {
         t.start() and
         result = this
         or
         exists(DataFlow::TypeTracker t2 | result = ref(t2).track(t2, t))
       }
+
+      /** Gets a `SourceNode` that refers to this response object. */
+      DataFlow::SourceNode ref() { result = ref(DataFlow::TypeTracker::end()) }
     }
 
     /**
@@ -333,7 +355,7 @@ module HTTP {
     class StandardRequestExpr extends RequestExpr {
       RequestSource src;
 
-      StandardRequestExpr() { src.flowsTo(DataFlow::valueNode(this)) }
+      StandardRequestExpr() { src.ref().flowsTo(DataFlow::valueNode(this)) }
 
       override RouteHandler getRouteHandler() { result = src.getRouteHandler() }
     }
@@ -344,7 +366,7 @@ module HTTP {
     class StandardResponseExpr extends ResponseExpr {
       ResponseSource src;
 
-      StandardResponseExpr() { src.flowsTo(DataFlow::valueNode(this)) }
+      StandardResponseExpr() { src.ref().flowsTo(DataFlow::valueNode(this)) }
 
       override RouteHandler getRouteHandler() { result = src.getRouteHandler() }
     }
@@ -370,6 +392,7 @@ module HTTP {
       /**
        * Gets a route handler that is defined by this setup.
        */
+      pragma[nomagic]
       abstract DataFlow::SourceNode getARouteHandler();
 
       /**