JavaScript: ensure prefix sanitizers work for array.join()

Author: asger-semmle

javascript/ql/src/semmle/javascript/StringConcatenation.qll

@@ -28,10 +28,19 @@ module StringConcatenation {
       or
       n = 1 and result = assign.getRhs().flow())
     or
-    exists (DataFlow::ArrayCreationNode array |
-      node = array.getAMethodCall("join") and
-      node.(DataFlow::MethodCallNode).getArgument(0).mayHaveStringValue("") and
-      result = array.getElement(n))
+    exists (DataFlow::ArrayCreationNode array, DataFlow::MethodCallNode call |
+      call = array.getAMethodCall("join") and
+      call.getArgument(0).mayHaveStringValue("") and
+      (
+        // step from array element to array
+        result = array.getElement(n) and
+        node = array
+        or
+        // step from array to join call
+        node = call and
+        result = array and
+        n = 0
+      ))
   }
 
   /** Gets an operand to the string concatenation defining `node`. */

javascript/ql/test/library-tests/StringConcatenation/ContainsTwo.expected

@@ -14,10 +14,13 @@
 | tst.js:20:3:20:25 | x += (" ... "four") |
 | tst.js:21:10:21:10 | x |
 | tst.js:21:10:21:19 | x + "five" |
+| tst.js:25:10:25:32 | ["one", ... three"] |
 | tst.js:25:10:25:41 | ["one", ... oin("") |
 | tst.js:25:18:25:22 | "two" |
+| tst.js:29:10:29:37 | Array(" ... three") |
 | tst.js:29:10:29:46 | Array(" ... oin("") |
 | tst.js:29:23:29:27 | "two" |
+| tst.js:33:10:33:41 | new Arr ... three") |
 | tst.js:33:10:33:50 | new Arr ... oin("") |
 | tst.js:33:27:33:31 | "two" |
 | tst.js:38:11:38:15 | "two" |

javascript/ql/test/query-tests/Security/CWE-601/ServerSideUrlRedirect/ServerSideUrlRedirect.expected

@@ -6,6 +6,7 @@
 | express.js:78:16:78:43 | `${req. ... )}/foo` | Untrusted URL redirection due to $@. | express.js:78:19:78:37 | req.param("target") | user-provided value |
 | express.js:94:18:94:23 | target | Untrusted URL redirection due to $@. | express.js:87:16:87:34 | req.param("target") | user-provided value |
 | express.js:101:16:101:21 | target | Untrusted URL redirection due to $@. | express.js:87:16:87:34 | req.param("target") | user-provided value |
+| express.js:119:16:119:72 | [req.qu ... oin('') | Untrusted URL redirection due to $@. | express.js:119:17:119:30 | req.query.page | user-provided value |
 | node.js:7:34:7:39 | target | Untrusted URL redirection due to $@. | node.js:6:26:6:32 | req.url | user-provided value |
 | node.js:15:34:15:45 | '/' + target | Untrusted URL redirection due to $@. | node.js:11:26:11:32 | req.url | user-provided value |
 | node.js:32:34:32:55 | target  ... =" + me | Untrusted URL redirection due to $@. | node.js:29:26:29:32 | req.url | user-provided value |

javascript/ql/test/query-tests/Security/CWE-601/ServerSideUrlRedirect/express.js

@@ -110,3 +110,14 @@ app.get('/some/path', function(req, res) {
   else
     res.redirect(target);
 });
+
+app.get('/array/join', function(req, res) {
+  // GOOD: request input embedded in query string
+  res.redirect(['index.html?section=', req.query.section].join(''));
+
+  // GOOD: request input still embedded in query string
+  res.redirect(['index.html?section=', '34'].join('') + '&subsection=' + req.query.subsection);
+
+  // BAD: request input becomes before query string
+  res.redirect([req.query.page, '?section=', req.query.section].join(''));
+});