Merge pull request github#19132 from asgerf/js/guarded-route-handler-token

JS: Add GuardedRouteHandler access path component

Author: asgerf

docs/codeql/codeql-language-guides/customizing-library-models-for-javascript.rst

@@ -349,6 +349,48 @@ Note that this flow is already recognized by the CodeQL JS analysis, but for thi
 - The last column, **value**, indicates the kind of flow to add. The value **value** means the input value is unchanged as
   it flows to the output.
 
+
+Example: Modeling properties injected by a middleware function
+--------------------------------------------------------------
+
+In this example, we'll show how to model a hypothetical middleware function that adds a tainted value
+on the incoming request objects:
+
+.. code-block:: js
+
+  const express = require('express')
+  const app = express()
+
+  app.use(require('@example/middleware').injectData())
+
+  app.get('/foo', (req, res) => {
+    req.data; // <-- mark 'req.data' as a taint source
+  });
+
+This can be achieved with the following data extension:
+
+.. code-block:: yaml
+
+  extensions:
+    - addsTo:
+        pack: codeql/javascript-all
+        extensible: sourceModel
+      data:
+        - [
+            "@example/middleware",
+            "Member[injectData].ReturnValue.GuardedRouteHandler.Parameter[0].Member[data]",
+            "remote",
+          ]
+
+- Since we're adding a new taint source, we add a tuple to the **sourceModel** extensible predicate.
+- The first column, **"@example/middleware"**, begins the search at imports of the hypothetical NPM package **@example/middleware**.
+- **Member[injectData]** selects accesses to the **injectData** member.
+- **ReturnValue** selects the return value of the call to **injectData**.
+- **GuardedRouteHandler** interprets the current value as a middleware function and selects all route handlers guarded by that middleware. Since the current value is passd to **app.use()**, the callback subsequently passed to **app.get()** is seen as a guarded route handler.
+- **Parameter[0]** selects the first parameter of the callback (the parameter named **req**).
+- **Member[data]** selects accesses to the **data** property of the **req** object.
+- Finally, the kind **remote** indicates that this is considered a source of remote flow.
+
 Reference material
 ------------------
 
@@ -494,6 +536,12 @@ Components related to decorators:
 - **DecoratedParameter** selects a parameter that is decorated by the current value.
 - **DecoratedMember** selects a method, field, or accessor that is decorated by the current value.
 
+Additionally there is a component related to middleware functions:
+
+- **GuardedRouteHandler** interprets the current value as a middleware function, and selects any route handler function that comes after it in the routing hierarchy.
+  This can be used to model properties injected onto request and response objects, such as **req.db** after a middleware that injects a database connection.
+  Note that this currently over-approximates the set of route handlers but may be made more accurate in the future.
+
 Additional notes about the syntax of operands:
 
 - Multiple operands may be given to a single component, as a shorthand for the union of the operands. For example, **Member[foo,bar]** matches the union of **Member[foo]** and **Member[bar]**.

javascript/ql/lib/semmle/javascript/frameworks/data/internal/ApiGraphModelsSpecific.qll

@@ -184,6 +184,20 @@ API::Node getExtraSuccessorFromNode(API::Node node, AccessPathTokenBase token) {
   or
   token.getName() = "DecoratedParameter" and
   result = node.getADecoratedParameter()
+  or
+  token.getName() = "GuardedRouteHandler" and
+  result = getAGuardedRouteHandlerApprox(node)
+}
+
+bindingset[node]
+pragma[inline_late]
+private API::Node getAGuardedRouteHandlerApprox(API::Node node) {
+  // For now just get any routing node with the same root (i.e. the same web app), as
+  // there are some known performance issues when checking if it is actually guarded by the given node.
+  exists(JS::Routing::Node root |
+    root = JS::Routing::getNode(node.getAValueReachableFromSource()).getRootNode() and
+    root = JS::Routing::getNode(result.asSink()).getRootNode()
+  )
 }
 
 /**
@@ -317,7 +331,7 @@ predicate isExtraValidTokenNameInIdentifyingAccessPath(string name) {
     [
       "Member", "AnyMember", "Instance", "Awaited", "ArrayElement", "Element", "MapValue",
       "NewCall", "Call", "DecoratedClass", "DecoratedMember", "DecoratedParameter",
-      "WithStringArgument"
+      "WithStringArgument", "GuardedRouteHandler"
     ]
 }
 
@@ -329,7 +343,7 @@ predicate isExtraValidNoArgumentTokenInIdentifyingAccessPath(string name) {
   name =
     [
       "AnyMember", "Instance", "Awaited", "ArrayElement", "Element", "MapValue", "NewCall", "Call",
-      "DecoratedClass", "DecoratedMember", "DecoratedParameter"
+      "DecoratedClass", "DecoratedMember", "DecoratedParameter", "GuardedRouteHandler"
     ]
 }
 

javascript/ql/test/library-tests/frameworks/data/guardedRouteHandler.js

@@ -0,0 +1,21 @@
+const express = require('express');
+const app = express();
+const testlib = require('testlib');
+
+app.get('/before', (req, res) => {
+    sink(req.injectedReqData); // OK [INCONSISTENCY] - happens before middleware
+    sink(req.injectedResData); // OK - wrong parameter
+
+    sink(res.injectedReqData); // OK - wrong parameter
+    sink(res.injectedResData); // OK [INCONSISTENCY] - happens before middleware
+});
+
+app.use(testlib.middleware());
+
+app.get('/after', (req, res) => {
+    sink(req.injectedReqData); // NOT OK
+    sink(req.injectedResData); // OK - wrong parameter
+
+    sink(res.injectedReqData); // OK - wrong parameter
+    sink(res.injectedResData); // NOT OK
+});

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

@@ -1,6 +1,10 @@
 legacyDataFlowDifference
 consistencyIssue
 taintFlow
+| guardedRouteHandler.js:6:10:6:28 | req.injectedReqData | guardedRouteHandler.js:6:10:6:28 | req.injectedReqData |
+| guardedRouteHandler.js:10:10:10:28 | res.injectedResData | guardedRouteHandler.js:10:10:10:28 | res.injectedResData |
+| guardedRouteHandler.js:16:10:16:28 | req.injectedReqData | guardedRouteHandler.js:16:10:16:28 | req.injectedReqData |
+| guardedRouteHandler.js:20:10:20:28 | res.injectedResData | guardedRouteHandler.js:20:10:20:28 | res.injectedResData |
 | paramDecorator.ts:6:54:6:54 | x | paramDecorator.ts:7:10:7:10 | x |
 | test.js:5:30:5:37 | source() | test.js:5:8:5:38 | testlib ... urce()) |
 | test.js:6:22:6:29 | source() | test.js:6:8:6:30 | preserv ... urce()) |

javascript/ql/test/library-tests/frameworks/data/test.ext.yml

@@ -13,6 +13,8 @@ extensions:
       - ['testlib', 'Member[getSourceArray].ReturnValue.ArrayElement', 'test-source']
       - ['(testlib)', 'Member[parenthesizedPackageName].ReturnValue', 'test-source']
       - ['danger-constant', 'Member[danger]', 'test-source']
+      - ['testlib', 'Member[middleware].ReturnValue.GuardedRouteHandler.Parameter[0].Member[injectedReqData]', 'test-source']
+      - ['testlib', 'Member[middleware].ReturnValue.GuardedRouteHandler.Parameter[1].Member[injectedResData]', 'test-source']
 
   - addsTo:
       pack: codeql/javascript-all