Merge pull request #2801 from esbena/js/bulky-route-handler-registration

Approved by asgerf

javascript/ql/src/semmle/javascript/Collections.qll

@@ -31,7 +31,7 @@ private class PseudoProperty extends TypeTrackingPseudoProperty {
  * `load`/`store`/`loadStore` can be used in the `CollectionsTypeTracking` module.
  * (Thereby avoiding naming conflicts with a "cousin" `AdditionalFlowStep` implementation.)
  */
-abstract private class CollectionFlowStep extends DataFlow::AdditionalFlowStep {
+abstract class CollectionFlowStep extends DataFlow::AdditionalFlowStep {
   final override predicate step(DataFlow::Node pred, DataFlow::Node succ) { none() }
 
   final override predicate step(

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

@@ -119,7 +119,14 @@ module Express {
       t.start() and
       result = getARouteHandlerExpr().flow().getALocalSource()
       or
-      exists(DataFlow::TypeBackTracker t2 | result = getARouteHandler(t2).backtrack(t2, t))
+      exists(DataFlow::TypeBackTracker t2, DataFlow::SourceNode succ | succ = getARouteHandler(t2) |
+        result = succ.backtrack(t2, t)
+        or
+        exists(HTTP::RouteHandlerCandidateContainer container |
+          result = container.getRouteHandler(succ)
+        ) and
+        t = t2
+      )
     }
 
     override Expr getServer() { result.(Application).getARouteHandler() = getARouteHandler() }

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

@@ -3,6 +3,8 @@
  */
 
 import javascript
+private import semmle.javascript.DynamicPropertyAccess
+private import semmle.javascript.dataflow.internal.StepSummary
 
 module HTTP {
   /**
@@ -496,4 +498,111 @@ module HTTP {
   class CookieCryptographicKey extends CryptographicKey {
     CookieCryptographicKey() { this = any(CookieMiddlewareInstance instance).getASecretKey() }
   }
+
+  /**
+   * An object that contains one or more potential route handlers.
+   */
+  class RouteHandlerCandidateContainer extends DataFlow::Node {
+    RouteHandlerCandidateContainer::Range self;
+
+    RouteHandlerCandidateContainer() { this = self }
+
+    /**
+     * Gets the route handler in this container that is accessed at `access`.
+     */
+    DataFlow::SourceNode getRouteHandler(DataFlow::SourceNode access) {
+      result = self.getRouteHandler(access)
+    }
+  }
+
+  /**
+   * Provides classes for working with objects that may contain one or more route handlers.
+   */
+  module RouteHandlerCandidateContainer {
+    private DataFlow::SourceNode ref(DataFlow::TypeTracker t, RouteHandlerCandidateContainer c) {
+      t.start() and result = c
+      or
+      exists(DataFlow::TypeTracker t2 | result = ref(t2, c).track(t2, t))
+    }
+
+    private DataFlow::SourceNode ref(RouteHandlerCandidateContainer c) {
+      result = ref(DataFlow::TypeTracker::end(), c)
+    }
+
+    /**
+     * A container for one or more potential route handlers.
+     *
+     * Extend this class and implement its abstract member predicates to model additional
+     * containers.
+     */
+    abstract class Range extends DataFlow::SourceNode {
+      /**
+       * Gets the route handler in this container that is accessed at `access`.
+       */
+      abstract DataFlow::SourceNode getRouteHandler(DataFlow::SourceNode access);
+    }
+
+    /**
+     * An object that contains one or more potential route handlers.
+     */
+    private class ContainerObject extends Range {
+      ContainerObject() {
+        (
+          this instanceof DataFlow::ObjectLiteralNode
+          or
+          exists(DataFlow::CallNode create | this = create |
+            create = DataFlow::globalVarRef("Object").getAMemberCall("create") and
+            create.getArgument(0).asExpr() instanceof NullLiteral
+          )
+        ) and
+        exists(RouteHandlerCandidate candidate | candidate.flowsTo(getAPropertyWrite().getRhs()))
+      }
+
+      override DataFlow::SourceNode getRouteHandler(DataFlow::SourceNode access) {
+        result instanceof RouteHandlerCandidate and
+        exists(DataFlow::PropWrite write, DataFlow::PropRead read |
+          access = read and
+          ref(this).getAPropertyRead() = read and
+          result.flowsTo(write.getRhs()) and
+          write = this.getAPropertyWrite()
+        |
+          write.getPropertyName() = read.getPropertyName()
+          or
+          exists(EnumeratedPropName prop | access = prop.getASourceProp())
+          or
+          read = DataFlow::lvalueNode(any(ForOfStmt stmt).getLValue())
+        )
+      }
+    }
+
+    /**
+     * A collection that contains one or more route potential handlers.
+     */
+    private class ContainerCollection extends HTTP::RouteHandlerCandidateContainer::Range {
+      ContainerCollection() {
+        this = DataFlow::globalVarRef("Map").getAnInstantiation() and // restrict to Map for now
+        exists(
+          CollectionFlowStep store, DataFlow::Node storeTo, DataFlow::Node input,
+          RouteHandlerCandidate candidate
+        |
+          this.flowsTo(storeTo) and
+          store.store(input, storeTo, _) and
+          candidate.flowsTo(input)
+        )
+      }
+
+      override DataFlow::SourceNode getRouteHandler(DataFlow::SourceNode access) {
+        exists(
+          DataFlow::Node input, TypeTrackingPseudoProperty key, CollectionFlowStep store,
+          CollectionFlowStep load, DataFlow::Node storeTo, DataFlow::Node loadFrom
+        |
+          this.flowsTo(storeTo) and
+          store.store(input, storeTo, key) and
+          result.(RouteHandlerCandidate).flowsTo(input) and
+          ref(this).flowsTo(loadFrom) and
+          load.load(loadFrom, access, key)
+        )
+      }
+    }
+  }
 }

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

@@ -0,0 +1,8 @@
+import javascript
+
+query predicate getRouteHandlerContainerStep(
+  HTTP::RouteHandlerCandidateContainer container, DataFlow::SourceNode handler,
+  DataFlow::SourceNode access
+) {
+  handler = container.getRouteHandler(access)
+}

javascript/ql/test/library-tests/frameworks/Express/src/advanced-routehandler-registration.js

@@ -0,0 +1,163 @@
+var express = require("express");
+var app = express();
+
+// registration of route handlers in bulk
+let routes0 = {
+  a: (req, res) => console.log(req),
+  b: (req, res) => console.log(req)
+};
+for (const p in routes0) {
+  app.get(p, routes0[p]);
+}
+
+// registration of route handlers in bulk
+let routes1 = {
+  a: (req, res) => console.log(req),
+  b: (req, res) => console.log(req)
+};
+for (const handler of routes1) {
+  app.use(handler);
+}
+
+// registration of route handlers in bulk, with indirection
+let routes2 = {
+  a: (req, res) => console.log(req),
+  b: (req, res) => console.log(req)
+};
+for (const p of Object.keys(routes2)) {
+  app.get(p, routes2[p]);
+}
+
+// registration of route handlers in bulk, with indirection
+let routes3 = {
+  a: (req, res) => console.log(req),
+  b: (req, res) => console.log(req)
+};
+for (const h of Object.values(routes3)) {
+  app.use(h);
+}
+
+// custom router indirection for all requests
+let myRouter1 = {
+  handlers: {},
+  add: function(n, h) {
+    this.handlers[n] = h;
+  },
+  handle: function(req, res, target) {
+    this.handlers[target](req, res);
+  }
+};
+myRouter1.add("whatever", (req, res) => console.log(req));
+app.use((req, res) => myRouter1.handle(req, res, "whatever"));
+
+// simpler custom router indirection for all requests
+let mySimpleRouter = {
+  handler: undefined,
+  add: function(h) {
+    this.handler = h;
+  },
+  handle: function(req, res) {
+    this.handler(req, res);
+  }
+};
+mySimpleRouter.add((req, res) => console.log(req));
+app.use((req, res) => mySimpleRouter.handle(req, res));
+
+// simplest custom router indirection for all requests
+let mySimplestRouter = {
+  handler: (req, res) => console.log(req),
+  handle: function(req, res) {
+    this.handler(req, res);
+  }
+};
+app.use((req, res) => mySimplestRouter.handle(req, res));
+
+// a combination of bulk registration and indirection through a custom router
+let myRouter3 = {
+  handlers: {},
+  add: function(n, h) {
+    this.handlers[n] = h;
+  },
+  handle: function(req, res, target) {
+    this.handlers[target](req, res);
+  }
+};
+let routes3 = {
+  a: (req, res) => console.log(req),
+  b: (req, res) => console.log(req)
+};
+for (const p of Object.keys(routes3)) {
+  myRouter3.add(p, routes3[p]);
+}
+app.use((req, res) => myRouter3.handle(req, res, "whatever"));
+
+// a combination of bulk registration and indirection through a custom router. Using a map instead of an object.
+let myRouter4 = {
+  handlers: new Map(),
+  add: function(n, h) {
+    this.handlers.set(n, h);
+  },
+  handle: function(req, res, target) {
+    this.handlers.get(target)(req, res);
+  }
+};
+let routes4 = {
+  a: (req, res) => console.log(req),
+  b: (req, res) => console.log(req)
+};
+for (const p of Object.keys(routes4)) {
+  myRouter4.add(p, routes4[p]);
+}
+app.use((req, res) => myRouter4.handle(req, res, "whatever"));
+
+// registration of imported route handlers in bulk
+let importedRoutes = require("./route-collection").routes;
+for (const p in importedRoutes) {
+  app.get(p, importedRoutes[p]);
+}
+app.get("a", importedRoutes.a);
+app.get("b", importedRoutes.b);
+
+// registration of imported route handlers in a map
+let routesMap = new Map();
+routesMap.set("a", (req, res) => console.log(req));
+routesMap.set("b", (req, res) => console.log(req));
+routesMap.forEach((v, k) => app.get(k, v));
+app.get("a", routesMap.get("a"));
+app.get("b", routesMap.get("b"));
+
+let method = "GET";
+app[method.toLowerCase()](path, (req, res) => undefined);
+
+let names = ["handler-in-dynamic-require"];
+names.forEach(name => {
+	let dynamicRequire = require("./controllers/" + name);
+	app.get(dynamicRequire.path, dynamicRequire.handler);
+});
+
+let bulkRequire = require("./controllers");
+app.get(bulkRequire.bulky.path, bulkRequire.bulky.handler);
+
+let options = { app: app };
+let args = [];
+args.push((req, res) => undefined);
+app.use.apply(options.app, args);
+
+let handlers = { handlerA: (req, res) => undefined};
+app.use(handlers.handlerA.bind(data));
+
+for ([k, v] of routesMap) {
+	app.get(k, v) // not supported - requires one too many heap steps
+}
+
+app.get("b", routesMap.get("NOT_A_KEY!")); // unknown route handler
+
+let routesMap2 = new Map();
+routesMap2.set("c", (req, res) => console.log(req));
+routesMap2.set(unknown(), (req, res) => console.log(req));
+routesMap2.set("e", (req, res) => console.log(req));
+
+app.get("c", routesMap2.get("c"));
+app.get("d", routesMap2.get(unknown()));
+app.get("e", unknown());
+app.get("d", routesMap2.get("f"));

javascript/ql/test/library-tests/frameworks/Express/src/controllers/handler-in-bulk-require.js

@@ -0,0 +1 @@
+module.exports = { path: "bulky", handler: (req, res) => undefined };

javascript/ql/test/library-tests/frameworks/Express/src/controllers/handler-in-dynamic-require.js

@@ -0,0 +1 @@
+module.exports = { path: "/A", handler: (req, res) => undefined };

javascript/ql/test/library-tests/frameworks/Express/src/controllers/index.js

@@ -0,0 +1,4 @@
+let bulky = require("./handler-in-bulk-require");
+module.exports = {
+	bulky: bulky
+};

javascript/ql/test/library-tests/frameworks/Express/src/route-collection.js

@@ -0,0 +1,4 @@
+exports.routes = {
+  a: (req, res) => console.log(req),
+  b: (req, res) => console.log(req)
+};

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

@@ -46,3 +46,4 @@ import RequestExpr
 import RouteHandlerExpr_getAsSubRouter
 import Credentials
 import RouteHandler_getARequestExpr
+import RouteHandlerContainer