Merge pull request #1874 from asger-semmle/express-types

Approved by esben-semmle, xiemaisi

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

@@ -37,27 +37,25 @@ module Express {
    */
   private predicate isRouter(Expr e, RouterDefinition router) {
     router.flowsTo(e)
+  }
+
+  /**
+   * Holds if `e` may refer to a router object.
+   */
+  private predicate isRouter(Expr e) {
+    isRouter(e, _)
     or
-    exists(DataFlow::MethodCallNode chain, DataFlow::Node base, string name |
-      name = "route" or
-      name = routeSetupMethodName()
-    |
-      chain.calls(base, name) and
-      isRouter(base.asExpr(), router) and
-      chain.flowsToExpr(e)
-    )
+    e.getType().hasUnderlyingType("express", "Router")
   }
 
   /**
    * An expression that refers to a route.
    */
   class RouteExpr extends MethodCallExpr {
-    RouterDefinition router;
+    RouteExpr() { isRouter(this) }
 
-    RouteExpr() { isRouter(this, router) }
-
-    /** Gets the router from which this route was created. */
-    RouterDefinition getRouter() { result = router }
+    /** Gets the router from which this route was created, if it is known. */
+    RouterDefinition getRouter() { isRouter(this, result) }
   }
 
   /**
@@ -77,10 +75,8 @@ module Express {
    * A call to an Express router method that sets up a route.
    */
   class RouteSetup extends HTTP::Servers::StandardRouteSetup, MethodCallExpr {
-    RouterDefinition router;
-
     RouteSetup() {
-      isRouter(getReceiver(), router) and
+      isRouter(getReceiver()) and
       getMethodName() = routeSetupMethodName()
     }
 
@@ -88,7 +84,7 @@ module Express {
     string getPath() { getArgument(0).mayHaveStringValue(result) }
 
     /** Gets the router on which handlers are being registered. */
-    RouterDefinition getRouter() { result = router }
+    RouterDefinition getRouter() { isRouter(getReceiver(), result) }
 
     /** Holds if this is a call `use`, such as `app.use(handler)`. */
     predicate isUseCall() { getMethodName() = "use" }
@@ -340,14 +336,18 @@ module Express {
     )
   }
 
+  /** An Express response source. */
+  abstract private class ResponseSource extends HTTP::Servers::ResponseSource {
+  }
+
   /**
    * An Express response source, that is, the response parameter of a
    * route handler, or a chained method call on a response.
    */
-  private class ResponseSource extends HTTP::Servers::ResponseSource {
+  private class ExplicitResponseSource extends ResponseSource {
     RouteHandler rh;
 
-    ResponseSource() {
+    ExplicitResponseSource() {
       this = DataFlow::parameterNode(rh.getResponseParameter())
       or
       isChainableResponseMethodCall(rh, this.asExpr())
@@ -359,14 +359,29 @@ module Express {
     override RouteHandler getRouteHandler() { result = rh }
   }
 
+  /**
+   * An Express response source, based on static type information.
+   */
+  private class TypedResponseSource extends ResponseSource {
+    TypedResponseSource() {
+      hasUnderlyingType("express", "Response")
+    }
+
+    override RouteHandler getRouteHandler() { none() } // Not known.
+  }
+
+  /** An Express request source. */
+  abstract private class RequestSource extends HTTP::Servers::RequestSource {
+  }
+
   /**
    * An Express request source, that is, the request parameter of a
    * route handler.
    */
-  private class RequestSource extends HTTP::Servers::RequestSource {
+  private class ExplicitRequestSource extends RequestSource {
     RouteHandler rh;
 
-    RequestSource() { this = DataFlow::parameterNode(rh.getRequestParameter()) }
+    ExplicitRequestSource() { this = DataFlow::parameterNode(rh.getRequestParameter()) }
 
     /**
      * Gets the route handler that handles this request.
@@ -374,6 +389,17 @@ module Express {
     override RouteHandler getRouteHandler() { result = rh }
   }
 
+  /**
+   * An Express request source, based on static type information.
+   */
+  private class TypedRequestSource extends RequestSource {
+    TypedRequestSource() {
+      hasUnderlyingType("express", "Request")
+    }
+
+    override RouteHandler getRouteHandler() { none() } // Not known.
+  }
+
   /**
    * An Express response expression.
    */
@@ -678,6 +704,13 @@ module Express {
       t.start() and
       result = DataFlow::exprNode(this)
       or
+      exists(string name |
+        result = ref(t.continue()).getAMethodCall(name)
+      |
+        name = "route" or
+        name = routeSetupMethodName()
+      )
+      or
       exists(DataFlow::TypeTracker t2 | result = ref(t2).track(t2, t))
     }
 

javascript/ql/test/library-tests/frameworks/Express/RequestExpr.qll

@@ -3,3 +3,7 @@ import javascript
 query predicate test_RequestExpr(Express::RequestExpr e, HTTP::RouteHandler res) {
   res = e.getRouteHandler()
 }
+
+query predicate test_RequestExprStandalone(Express::RequestExpr e) {
+  not exists(e.getRouteHandler())
+}

javascript/ql/test/library-tests/frameworks/Express/tests.expected

@@ -26,6 +26,7 @@ test_RouteSetup
 | src/csurf-example.js:39:1:39:48 | app.get ... es) {}) | src/csurf-example.js:7:11:7:19 | express() | false |
 | src/csurf-example.js:40:1:40:49 | app.pos ... es) {}) | src/csurf-example.js:7:11:7:19 | express() | false |
 | src/express2.js:3:1:3:56 | router. ...  res }) | src/express2.js:5:11:5:13 | e() | false |
+| src/express2.js:3:1:4:77 | router. ... sult }) | src/express2.js:5:11:5:13 | e() | false |
 | src/express3.js:4:1:7:2 | app.get ... l");\\n}) | src/express3.js:2:11:2:19 | express() | false |
 | src/express4.js:4:1:6:2 | app.get ... ery;\\n}) | src/express4.js:2:11:2:19 | express() | false |
 | src/express.js:4:1:9:2 | app.get ... es);\\n}) | src/express.js:2:11:2:19 | express() | false |
@@ -245,6 +246,7 @@ test_StandardRouteHandler
 | src/csurf-example.js:39:26:39:47 | functio ... res) {} | src/csurf-example.js:7:11:7:19 | express() | src/csurf-example.js:39:36:39:38 | req | src/csurf-example.js:39:41:39:43 | res |
 | src/csurf-example.js:40:27:40:48 | functio ... res) {} | src/csurf-example.js:7:11:7:19 | express() | src/csurf-example.js:40:37:40:39 | req | src/csurf-example.js:40:42:40:44 | res |
 | src/express2.js:3:25:3:55 | functio ... , res } | src/express2.js:5:11:5:13 | e() | src/express2.js:3:34:3:36 | req | src/express2.js:3:39:3:41 | res |
+| src/express2.js:4:32:4:76 | functio ... esult } | src/express2.js:5:11:5:13 | e() | src/express2.js:4:41:4:47 | request | src/express2.js:4:50:4:55 | result |
 | src/express3.js:4:23:7:1 | functio ... al");\\n} | src/express3.js:2:11:2:19 | express() | src/express3.js:4:32:4:34 | req | src/express3.js:4:37:4:39 | res |
 | src/express4.js:4:23:6:1 | functio ... uery;\\n} | src/express4.js:2:11:2:19 | express() | src/express4.js:4:32:4:34 | req | src/express4.js:4:37:4:39 | res |
 | src/express.js:4:23:9:1 | functio ... res);\\n} | src/express.js:2:11:2:19 | express() | src/express.js:4:32:4:34 | req | src/express.js:4:37:4:39 | res |
@@ -395,6 +397,7 @@ test_RouterDefinition_getARouteHandler
 | src/csurf-example.js:7:11:7:19 | express() | src/csurf-example.js:40:27:40:48 | functio ... res) {} |
 | src/csurf-example.js:30:16:30:35 | new express.Router() | src/csurf-example.js:32:30:34:3 | functio ... e')\\n  } |
 | src/express2.js:2:14:2:23 | e.Router() | src/express2.js:3:25:3:55 | functio ... , res } |
+| src/express2.js:2:14:2:23 | e.Router() | src/express2.js:4:32:4:76 | functio ... esult } |
 | src/express3.js:2:11:2:19 | express() | src/express3.js:4:23:7:1 | functio ... al");\\n} |
 | src/express4.js:2:11:2:19 | express() | src/express4.js:4:23:6:1 | functio ... uery;\\n} |
 | src/express.js:2:11:2:19 | express() | src/express.js:4:23:9:1 | functio ... res);\\n} |
@@ -407,6 +410,7 @@ test_RouterDefinition_getARouteHandler
 | src/responseExprs.js:2:11:2:19 | express() | src/responseExprs.js:10:23:12:1 | functio ... res3;\\n} |
 | src/responseExprs.js:2:11:2:19 | express() | src/responseExprs.js:13:23:15:1 | functio ... res4;\\n} |
 | src/responseExprs.js:2:11:2:19 | express() | src/responseExprs.js:16:30:42:1 | functio ...     }\\n} |
+| src/route.js:2:14:2:29 | express.Router() | src/route.js:5:12:5:38 | functio ... ext) {} |
 test_CookieMiddlewareInstance
 | src/cookie-parser.js:3:1:3:23 | session ... key-1") | src/cookie-parser.js:3:9:3:22 | "secret-key-1" |
 | src/cookie-parser.js:5:1:5:41 | session ... ey-3"]) | src/cookie-parser.js:5:10:5:23 | "secret-key-2" |
@@ -451,6 +455,7 @@ test_RouteSetup_getServer
 | src/csurf-example.js:39:1:39:48 | app.get ... es) {}) | src/csurf-example.js:7:11:7:19 | express() |
 | src/csurf-example.js:40:1:40:49 | app.pos ... es) {}) | src/csurf-example.js:7:11:7:19 | express() |
 | src/express2.js:3:1:3:56 | router. ...  res }) | src/express2.js:5:11:5:13 | e() |
+| src/express2.js:3:1:4:77 | router. ... sult }) | src/express2.js:5:11:5:13 | e() |
 | src/express3.js:4:1:7:2 | app.get ... l");\\n}) | src/express3.js:2:11:2:19 | express() |
 | src/express4.js:4:1:6:2 | app.get ... ery;\\n}) | src/express4.js:2:11:2:19 | express() |
 | src/express.js:4:1:9:2 | app.get ... es);\\n}) | src/express.js:2:11:2:19 | express() |
@@ -1019,6 +1024,8 @@ test_RequestExpr
 | src/express.js:50:3:50:5 | req | src/express.js:46:22:51:1 | functio ... ame];\\n} |
 | src/inheritedFromNode.js:7:2:7:4 | req | src/inheritedFromNode.js:4:15:8:1 | functio ... .url;\\n} |
 | src/responseExprs.js:17:5:17:7 | req | src/responseExprs.js:16:30:42:1 | functio ...     }\\n} |
+test_RequestExprStandalone
+| typed_src/tst.ts:6:3:6:3 | x |
 test_RouteHandlerExpr_getAsSubRouter
 | src/csurf-example.js:13:17:13:19 | api | src/csurf-example.js:30:16:30:35 | new express.Router() |
 | src/express2.js:6:9:6:14 | router | src/express2.js:2:14:2:23 | e.Router() |

javascript/ql/test/library-tests/frameworks/Express/tsconfig.json

@@ -0,0 +1,4 @@
+{
+  "include": ["typed_src"]
+}
+

javascript/ql/test/library-tests/frameworks/Express/typed_src/shim.d.ts

@@ -0,0 +1,13 @@
+declare namespace ServeStaticCore {
+  interface Request {
+    body: any;
+  }
+}
+
+declare module 'express' {
+  interface Request extends ServeStaticCore.Request {}
+}
+
+declare module 'express-serve-static-core' {
+  export = ServeStaticCore;
+}

javascript/ql/test/library-tests/frameworks/Express/typed_src/tst.ts

@@ -0,0 +1,7 @@
+/// <reference path="./shim.d.ts"/>
+
+import * as express from 'express';
+
+function test(x: express.Request) {
+  x.body;
+}