JS: Fix handling of fastify-plugin

Author: asgerf

javascript/ql/lib/semmle/javascript/frameworks/Fastify.qll

@@ -50,9 +50,16 @@ module Fastify {
     t.start() and
     result = server(creation).getAMethodCall("register").getArgument(0).getALocalSource()
     or
+    // Track through require('fastify-plugin')
+    result = pluginCallback(creation, t).(FastifyPluginCall).getArgument(0).getALocalSource()
+    or
     exists(DataFlow::TypeBackTracker t2 | result = pluginCallback(creation, t2).backtrack(t2, t))
   }
 
+  private class FastifyPluginCall extends DataFlow::CallNode {
+    FastifyPluginCall() { this = DataFlow::moduleImport("fastify-plugin").getACall() }
+  }
+
   /** Gets a data flow node being used as a Fastify plugin. */
   private DataFlow::SourceNode pluginCallback(DataFlow::SourceNode creation) {
     result = pluginCallback(creation, DataFlow::TypeBackTracker::end())
@@ -198,18 +205,40 @@ module Fastify {
   }
 
   private class PluginRegistration extends Routing::RouteSetup::MethodCall {
-    ServerDefinition server;
+    PluginRegistration() { this = server().getAMethodCall("register") }
 
-    PluginRegistration() {
-      server.flowsTo(this.getReceiver().asExpr()) and
-      getMethodName() = "register"
+    private DataFlow::SourceNode pluginBody(DataFlow::TypeBackTracker t) {
+      t.start() and
+      result = getArgument(0).getALocalSource()
+      or
+      // step through calls to require('fastify-plugin')
+      result = pluginBody(t).(FastifyPluginCall).getArgument(0).getALocalSource()
+      or
+      exists(DataFlow::TypeBackTracker t2 | result = pluginBody(t2).backtrack(t2, t))
     }
 
+    /** Gets a functino flowing into the first argument. */
+    DataFlow::FunctionNode pluginBody() { result = pluginBody(DataFlow::TypeBackTracker::end()) }
+
     override HTTP::RequestMethodName getHttpMethod() {
       result = getOptionArgument(1, "method").getStringValue().toUpperCase()
     }
 
     override string getRelativePath() { result = getOptionArgument(1, "prefix").getStringValue() }
+
+    override DataFlow::Node getChildNode(int n) {
+      n = 0 and
+      (
+        // If we can see the plugin body, use its server parameter as the child to ensure
+        // plugins or routes installed in the plugin are ordered
+        result = pluginBody().getParameter(0)
+        or
+        // If we can't see the plugin body, just use the plugin expression so we can
+        // check if something is guarded by that plugin.
+        not exists(pluginBody()) and
+        result = getArgument(0)
+      )
+    }
   }
 
   /**
@@ -403,14 +432,4 @@ module Fastify {
       )
     }
   }
-
-  private class RouteHandlerTracking extends Routing::RouteHandlerTrackingStep {
-    override predicate step(DataFlow::Node pred, DataFlow::Node succ) {
-      exists(DataFlow::CallNode call |
-        call = DataFlow::moduleImport("fastify-plugin") and
-        pred = call.getArgument(0) and
-        succ = call
-      )
-    }
-  }
 }

javascript/ql/test/query-tests/Security/CWE-352/MissingCsrfMiddleware.expected

@@ -4,6 +4,7 @@
 | MissingCsrfMiddlewareBad.js:33:13:33:26 | cookieParser() | This cookie middleware is serving a request handler $@ without CSRF protection. | MissingCsrfMiddlewareBad.js:45:31:47:6 | errorCa ... \\n    }) | here |
 | csurf_api_example.js:42:37:42:50 | cookieParser() | This cookie middleware is serving a request handler $@ without CSRF protection. | csurf_api_example.js:42:53:45:3 | functio ... e')\\n  } | here |
 | csurf_example.js:18:9:18:22 | cookieParser() | This cookie middleware is serving a request handler $@ without CSRF protection. | csurf_example.js:31:40:34:1 | functio ... sed')\\n} | here |
+| fastify2.js:7:16:7:40 | require ... ookie') | This cookie middleware is serving a request handler $@ without CSRF protection. | fastify2.js:24:12:27:3 | async ( ... ody\\n  } | here |
 | fastify.js:5:14:5:38 | require ... ookie') | This cookie middleware is serving a request handler $@ without CSRF protection. | fastify.js:20:12:23:3 | async ( ... ody\\n  } | here |
 | lusca_example.js:9:9:9:22 | cookieParser() | This cookie middleware is serving a request handler $@ without CSRF protection. | lusca_example.js:26:42:29:1 | functio ... sed')\\n} | here |
 | lusca_example.js:9:9:9:22 | cookieParser() | This cookie middleware is serving a request handler $@ without CSRF protection. | lusca_example.js:31:40:34:1 | functio ... sed')\\n} | here |

javascript/ql/test/query-tests/Security/CWE-352/fastify2.js

@@ -0,0 +1,38 @@
+const fastify = require('fastify')
+const fp = require('fastify-plugin');
+
+const app = fastify();
+
+function plugin(app) {
+  app.register(require('fastify-cookie'));
+  app.register(require('fastify-csrf'));
+}
+app.register(fp(plugin));
+
+app.route({
+  method: 'GET',
+  path: '/getter',
+  handler: async (req, reply) => { // OK
+    return 'hello';
+  }
+})
+
+// unprotected route
+app.route({
+  method: 'POST',
+  path: '/',
+  handler: async (req, reply) => { // NOT OK - lacks CSRF protection
+    req.session.blah;
+    return req.body
+  }
+})
+
+
+app.route({
+  method: 'POST',
+  path: '/',
+  onRequest: app.csrfProtection,
+  handler: async (req, reply) => { // OK - has CSRF protection
+    return req.body
+  }
+})