support containers with decorated route handlers

Author: erik-krogh

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

@@ -555,26 +555,51 @@ module HTTP {
             create.getArgument(0).asExpr() instanceof NullLiteral
           )
         ) and
-        exists(RouteHandlerCandidate candidate | candidate.flowsTo(getAPropertyWrite().getRhs()))
+        exists(RouteHandlerCandidate candidate |
+          getAPossiblyDecoratedHandler(candidate).flowsTo(getAPropertyWrite().getRhs())
+        )
       }
 
-      override DataFlow::SourceNode getRouteHandler(DataFlow::SourceNode access) {
-        result instanceof RouteHandlerCandidate and
+      override RouteHandlerCandidate getRouteHandler(DataFlow::SourceNode access) {
         exists(DataFlow::PropWrite write, DataFlow::PropRead read |
           access = read and
           ref(this).getAPropertyRead() = read and
-          result.flowsTo(write.getRhs()) and
+          getAPossiblyDecoratedHandler(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())
+          or
+          // for forwarding calls to an element where the key is determined by the request.
+          getRequestParameterRead(read.getContainer().(Function).flow())
+              .flowsToExpr(read.getPropertyNameExpr())
         )
       }
     }
 
+    /**
+     * Gets a (chained) property-read/method-call on the request parameter of the route-handler `f`.
+     */
+    private DataFlow::SourceNode getRequestParameterRead(RouteHandlerCandidate f) {
+      result = f.getParameter(0)
+      or
+      result = getRequestParameterRead(f).getAPropertyRead()
+      or
+      result = getRequestParameterRead(f).getAMethodCall()
+    }
+
+    /**
+     * Gets a node that is either `candidate`, or a call that decorates `candidate`.
+     */
+    DataFlow::SourceNode getAPossiblyDecoratedHandler(RouteHandlerCandidate candidate) {
+      result = candidate
+      or
+      Express::decoratedRouteHandler(candidate, result)
+    }
+
     /**
      * A collection that contains one or more route potential handlers.
      */
@@ -591,14 +616,14 @@ module HTTP {
         )
       }
 
-      override DataFlow::SourceNode getRouteHandler(DataFlow::SourceNode access) {
+      override RouteHandlerCandidate 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
+          getAPossiblyDecoratedHandler(result).flowsTo(input) and
           ref(this).flowsTo(loadFrom) and
           load.load(loadFrom, access, key)
         )

javascript/ql/test/library-tests/frameworks/NodeJSLib/src/indirect2.js

@@ -0,0 +1,18 @@
+var http = require('http');
+
+// These are exceptions where we override the routes
+var handlers = {
+  someKey: myIndirectHandler
+};
+
+
+function get(req, res) { // route handler
+  handlers[req.params.key.toLowerCase()](req, res);
+}
+
+function myIndirectHandler(req, res) { // route handler
+  res.setHeader('Content-Type', 'application/json');
+  res.send("\"some result\"");
+}
+
+var server = http.createServer(get);

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

@@ -10,6 +10,7 @@ test_isCreateServer
 | src/http.js:70:1:70:36 | http.cr ... dler()) |
 | src/https.js:4:14:10:2 | https.c ... foo;\\n}) |
 | src/https.js:12:1:16:2 | https.c ... r");\\n}) |
+| src/indirect2.js:18:14:18:35 | http.cr ... er(get) |
 | src/indirect.js:34:14:34:58 | http.cr ... dler()) |
 test_RequestInputAccess
 | src/http.js:6:26:6:32 | req.url | url | src/http.js:4:32:10:1 | functio ... .foo;\\n} |
@@ -24,9 +25,11 @@ test_RouteHandler_getAResponseHeader
 | src/http.js:12:19:16:1 | functio ... ar");\\n} | content-type | src/http.js:13:3:13:44 | res.set ... /html') |
 | src/https.js:4:33:10:1 | functio ... .foo;\\n} | location | src/https.js:7:3:7:42 | res.wri ... rget }) |
 | src/https.js:12:20:16:1 | functio ... ar");\\n} | content-type | src/https.js:13:3:13:44 | res.set ... /html') |
+| src/indirect2.js:13:1:16:1 | functio ... \\"");\\n} | content-type | src/indirect2.js:14:3:14:51 | res.set ... /json') |
 test_HeaderDefinition_defines
 | src/http.js:13:3:13:44 | res.set ... /html') | content-type | text/html |
 | src/https.js:13:3:13:44 | res.set ... /html') | content-type | text/html |
+| src/indirect2.js:14:3:14:51 | res.set ... /json') | content-type | application/json |
 test_SystemCommandExecution
 | es6-imported-exec.js:3:1:3:11 | exec("cmd") | es6-imported-exec.js:3:6:3:10 | "cmd" |
 | exec.js:3:1:3:38 | cp.exec ... "], cb) | exec.js:3:13:3:18 | "node" |
@@ -56,6 +59,11 @@ test_ResponseExpr
 | src/https.js:13:3:13:5 | res | src/https.js:12:20:16:1 | functio ... ar");\\n} |
 | src/https.js:14:3:14:5 | res | src/https.js:12:20:16:1 | functio ... ar");\\n} |
 | src/https.js:15:3:15:5 | res | src/https.js:12:20:16:1 | functio ... ar");\\n} |
+| src/indirect2.js:9:19:9:21 | res | src/indirect2.js:9:1:11:1 | functio ... res);\\n} |
+| src/indirect2.js:10:47:10:49 | res | src/indirect2.js:9:1:11:1 | functio ... res);\\n} |
+| src/indirect2.js:13:33:13:35 | res | src/indirect2.js:13:1:16:1 | functio ... \\"");\\n} |
+| src/indirect2.js:14:3:14:5 | res | src/indirect2.js:13:1:16:1 | functio ... \\"");\\n} |
+| src/indirect2.js:15:3:15:5 | res | src/indirect2.js:13:1:16:1 | functio ... \\"");\\n} |
 | src/indirect.js:16:26:16:28 | res | src/indirect.js:16:12:20:5 | functio ... ;\\n    } |
 | src/indirect.js:19:38:19:40 | res | src/indirect.js:16:12:20:5 | functio ... ;\\n    } |
 test_HeaderDefinition
@@ -64,6 +72,7 @@ test_HeaderDefinition
 | src/http.js:63:3:63:40 | res.set ... , "23") | src/http.js:62:19:65:1 | functio ... r2");\\n} |
 | src/https.js:7:3:7:42 | res.wri ... rget }) | src/https.js:4:33:10:1 | functio ... .foo;\\n} |
 | src/https.js:13:3:13:44 | res.set ... /html') | src/https.js:12:20:16:1 | functio ... ar");\\n} |
+| src/indirect2.js:14:3:14:51 | res.set ... /json') | src/indirect2.js:13:1:16:1 | functio ... \\"");\\n} |
 test_RouteSetup_getServer
 | createServer.js:2:1:2:42 | https.c ... es) {}) | createServer.js:2:1:2:42 | https.c ... es) {}) |
 | createServer.js:3:1:3:45 | https.c ... es) {}) | createServer.js:3:1:3:45 | https.c ... es) {}) |
@@ -76,6 +85,7 @@ test_RouteSetup_getServer
 | src/http.js:70:1:70:36 | http.cr ... dler()) | src/http.js:70:1:70:36 | http.cr ... dler()) |
 | src/https.js:4:14:10:2 | https.c ... foo;\\n}) | src/https.js:4:14:10:2 | https.c ... foo;\\n}) |
 | src/https.js:12:1:16:2 | https.c ... r");\\n}) | src/https.js:12:1:16:2 | https.c ... r");\\n}) |
+| src/indirect2.js:18:14:18:35 | http.cr ... er(get) | src/indirect2.js:18:14:18:35 | http.cr ... er(get) |
 | src/indirect.js:34:14:34:58 | http.cr ... dler()) | src/indirect.js:34:14:34:58 | http.cr ... dler()) |
 test_ClientRequest
 | src/http.js:18:1:18:30 | http.re ... uth" }) |
@@ -87,6 +97,7 @@ test_HeaderDefinition_getAHeaderName
 | src/http.js:13:3:13:44 | res.set ... /html') | content-type |
 | src/https.js:7:3:7:42 | res.wri ... rget }) | location |
 | src/https.js:13:3:13:44 | res.set ... /html') | content-type |
+| src/indirect2.js:14:3:14:51 | res.set ... /json') | content-type |
 test_ServerDefinition
 | createServer.js:2:1:2:42 | https.c ... es) {}) |
 | createServer.js:3:1:3:45 | https.c ... es) {}) |
@@ -99,6 +110,7 @@ test_ServerDefinition
 | src/http.js:70:1:70:36 | http.cr ... dler()) |
 | src/https.js:4:14:10:2 | https.c ... foo;\\n}) |
 | src/https.js:12:1:16:2 | https.c ... r");\\n}) |
+| src/indirect2.js:18:14:18:35 | http.cr ... er(get) |
 | src/indirect.js:34:14:34:58 | http.cr ... dler()) |
 test_HeaderAccess
 | src/http.js:9:3:9:17 | req.headers.foo | foo |
@@ -109,6 +121,7 @@ test_HeaderDefinition_getNameExpr
 | src/http.js:63:3:63:40 | res.set ... , "23") | src/http.js:63:17:63:33 | req.query.myParam |
 | src/https.js:7:3:7:42 | res.wri ... rget }) | src/https.js:7:17:7:19 | 302 |
 | src/https.js:13:3:13:44 | res.set ... /html') | src/https.js:13:17:13:30 | 'Content-Type' |
+| src/indirect2.js:14:3:14:51 | res.set ... /json') | src/indirect2.js:14:17:14:30 | 'Content-Type' |
 test_RouteHandler_getAResponseExpr
 | createServer.js:2:20:2:41 | functio ... res) {} | createServer.js:2:35:2:37 | res |
 | createServer.js:3:23:3:44 | functio ... res) {} | createServer.js:3:38:3:40 | res |
@@ -131,6 +144,11 @@ test_RouteHandler_getAResponseExpr
 | src/https.js:12:20:16:1 | functio ... ar");\\n} | src/https.js:13:3:13:5 | res |
 | src/https.js:12:20:16:1 | functio ... ar");\\n} | src/https.js:14:3:14:5 | res |
 | src/https.js:12:20:16:1 | functio ... ar");\\n} | src/https.js:15:3:15:5 | res |
+| src/indirect2.js:9:1:11:1 | functio ... res);\\n} | src/indirect2.js:9:19:9:21 | res |
+| src/indirect2.js:9:1:11:1 | functio ... res);\\n} | src/indirect2.js:10:47:10:49 | res |
+| src/indirect2.js:13:1:16:1 | functio ... \\"");\\n} | src/indirect2.js:13:33:13:35 | res |
+| src/indirect2.js:13:1:16:1 | functio ... \\"");\\n} | src/indirect2.js:14:3:14:5 | res |
+| src/indirect2.js:13:1:16:1 | functio ... \\"");\\n} | src/indirect2.js:15:3:15:5 | res |
 | src/indirect.js:16:12:20:5 | functio ... ;\\n    } | src/indirect.js:16:26:16:28 | res |
 | src/indirect.js:16:12:20:5 | functio ... ;\\n    } | src/indirect.js:19:38:19:40 | res |
 test_ServerDefinition_getARouteHandler
@@ -145,6 +163,8 @@ test_ServerDefinition_getARouteHandler
 | src/http.js:70:1:70:36 | http.cr ... dler()) | src/http.js:68:12:68:27 | (req,res) => f() |
 | src/https.js:4:14:10:2 | https.c ... foo;\\n}) | src/https.js:4:33:10:1 | functio ... .foo;\\n} |
 | src/https.js:12:1:16:2 | https.c ... r");\\n}) | src/https.js:12:20:16:1 | functio ... ar");\\n} |
+| src/indirect2.js:18:14:18:35 | http.cr ... er(get) | src/indirect2.js:9:1:11:1 | functio ... res);\\n} |
+| src/indirect2.js:18:14:18:35 | http.cr ... er(get) | src/indirect2.js:13:1:16:1 | functio ... \\"");\\n} |
 | src/indirect.js:34:14:34:58 | http.cr ... dler()) | src/indirect.js:16:12:20:5 | functio ... ;\\n    } |
 test_ResponseSendArgument
 | src/http.js:14:13:14:17 | "foo" | src/http.js:12:19:16:1 | functio ... ar");\\n} |
@@ -168,6 +188,9 @@ test_RouteSetup_getARouteHandler
 | src/http.js:70:1:70:36 | http.cr ... dler()) | src/http.js:70:19:70:35 | getArrowHandler() |
 | src/https.js:4:14:10:2 | https.c ... foo;\\n}) | src/https.js:4:33:10:1 | functio ... .foo;\\n} |
 | src/https.js:12:1:16:2 | https.c ... r");\\n}) | src/https.js:12:20:16:1 | functio ... ar");\\n} |
+| src/indirect2.js:18:14:18:35 | http.cr ... er(get) | src/indirect2.js:9:1:11:1 | functio ... res);\\n} |
+| src/indirect2.js:18:14:18:35 | http.cr ... er(get) | src/indirect2.js:10:3:10:40 | handler ... Case()] |
+| src/indirect2.js:18:14:18:35 | http.cr ... er(get) | src/indirect2.js:13:1:16:1 | functio ... \\"");\\n} |
 | src/indirect.js:34:14:34:58 | http.cr ... dler()) | src/indirect.js:14:19:21:3 | return of method requestHandler |
 | src/indirect.js:34:14:34:58 | http.cr ... dler()) | src/indirect.js:16:12:20:5 | functio ... ;\\n    } |
 | src/indirect.js:34:14:34:58 | http.cr ... dler()) | src/indirect.js:16:12:20:16 | functio ... d(this) |
@@ -203,6 +226,8 @@ test_RouteHandler
 | src/http.js:68:12:68:27 | (req,res) => f() | src/http.js:70:1:70:36 | http.cr ... dler()) |
 | src/https.js:4:33:10:1 | functio ... .foo;\\n} | src/https.js:4:14:10:2 | https.c ... foo;\\n}) |
 | src/https.js:12:20:16:1 | functio ... ar");\\n} | src/https.js:12:1:16:2 | https.c ... r");\\n}) |
+| src/indirect2.js:9:1:11:1 | functio ... res);\\n} | src/indirect2.js:18:14:18:35 | http.cr ... er(get) |
+| src/indirect2.js:13:1:16:1 | functio ... \\"");\\n} | src/indirect2.js:18:14:18:35 | http.cr ... er(get) |
 | src/indirect.js:16:12:20:5 | functio ... ;\\n    } | src/indirect.js:34:14:34:58 | http.cr ... dler()) |
 test_RequestExpr
 | createServer.js:2:30:2:32 | req | createServer.js:2:20:2:41 | functio ... res) {} |
@@ -223,6 +248,10 @@ test_RequestExpr
 | src/https.js:8:3:8:5 | req | src/https.js:4:33:10:1 | functio ... .foo;\\n} |
 | src/https.js:9:3:9:5 | req | src/https.js:4:33:10:1 | functio ... .foo;\\n} |
 | src/https.js:12:29:12:31 | req | src/https.js:12:20:16:1 | functio ... ar");\\n} |
+| src/indirect2.js:9:14:9:16 | req | src/indirect2.js:9:1:11:1 | functio ... res);\\n} |
+| src/indirect2.js:10:12:10:14 | req | src/indirect2.js:9:1:11:1 | functio ... res);\\n} |
+| src/indirect2.js:10:42:10:44 | req | src/indirect2.js:9:1:11:1 | functio ... res);\\n} |
+| src/indirect2.js:13:28:13:30 | req | src/indirect2.js:13:1:16:1 | functio ... \\"");\\n} |
 | src/indirect.js:16:21:16:23 | req | src/indirect.js:16:12:20:5 | functio ... ;\\n    } |
 | src/indirect.js:17:28:17:30 | req | src/indirect.js:16:12:20:5 | functio ... ;\\n    } |
 | src/indirect.js:19:33:19:35 | req | src/indirect.js:16:12:20:5 | functio ... ;\\n    } |
@@ -254,6 +283,10 @@ test_RouteHandler_getARequestExpr
 | src/https.js:4:33:10:1 | functio ... .foo;\\n} | src/https.js:8:3:8:5 | req |
 | src/https.js:4:33:10:1 | functio ... .foo;\\n} | src/https.js:9:3:9:5 | req |
 | src/https.js:12:20:16:1 | functio ... ar");\\n} | src/https.js:12:29:12:31 | req |
+| src/indirect2.js:9:1:11:1 | functio ... res);\\n} | src/indirect2.js:9:14:9:16 | req |
+| src/indirect2.js:9:1:11:1 | functio ... res);\\n} | src/indirect2.js:10:12:10:14 | req |
+| src/indirect2.js:9:1:11:1 | functio ... res);\\n} | src/indirect2.js:10:42:10:44 | req |
+| src/indirect2.js:13:1:16:1 | functio ... \\"");\\n} | src/indirect2.js:13:28:13:30 | req |
 | src/indirect.js:16:12:20:5 | functio ... ;\\n    } | src/indirect.js:16:21:16:23 | req |
 | src/indirect.js:16:12:20:5 | functio ... ;\\n    } | src/indirect.js:17:28:17:30 | req |
 | src/indirect.js:16:12:20:5 | functio ... ;\\n    } | src/indirect.js:19:33:19:35 | req |