From ccee34d6d3a251cb9a9a353902df035dc25d7a4f Mon Sep 17 00:00:00 2001
From: Napalys <napalys@github.com>
Date: Tue, 5 Nov 2024 08:51:24 +0100
Subject: [PATCH 01/13] Added support for matchAll in CWE-020 including new
 test cases

---
 .../ql/lib/semmle/javascript/Regexp.qll       |  4 ++--
 .../ql/lib/semmle/javascript/StringOps.qll    |  2 +-
 .../Security/CWE-020/MissingRegExpAnchor.ql   |  2 ++
 .../MissingRegExpAnchor.expected              |  9 ++++++++
 .../tst-UnanchoredUrlRegExp.js                | 23 +++++++++++++++++++
 5 files changed, 37 insertions(+), 3 deletions(-)

diff --git a/javascript/ql/lib/semmle/javascript/Regexp.qll b/javascript/ql/lib/semmle/javascript/Regexp.qll
index 27ad339c733e..df0f76e7cda6 100644
--- a/javascript/ql/lib/semmle/javascript/Regexp.qll
+++ b/javascript/ql/lib/semmle/javascript/Regexp.qll
@@ -938,7 +938,7 @@ private predicate isMatchObjectProperty(string name) {
 
 /** Holds if `call` is a call to `match` whose result is used in a way that is incompatible with Match objects. */
 private predicate isUsedAsNonMatchObject(DataFlow::MethodCallNode call) {
-  call.getMethodName() = "match" and
+  call.getMethodName() = ["match", "matchAll"] and
   call.getNumArgument() = 1 and
   (
     // Accessing a property that is absent on Match objects
@@ -996,7 +996,7 @@ predicate isInterpretedAsRegExp(DataFlow::Node source) {
         not isNativeStringMethod(func, methodName)
       )
     |
-      methodName = "match" and
+      methodName = ["match", "matchAll"] and
       source = mce.getArgument(0) and
       mce.getNumArgument() = 1 and
       not isUsedAsNonMatchObject(mce)
diff --git a/javascript/ql/lib/semmle/javascript/StringOps.qll b/javascript/ql/lib/semmle/javascript/StringOps.qll
index 6b7820e964dd..f2b7f3eb9aa7 100644
--- a/javascript/ql/lib/semmle/javascript/StringOps.qll
+++ b/javascript/ql/lib/semmle/javascript/StringOps.qll
@@ -722,7 +722,7 @@ module StringOps {
     }
 
     private class MatchCall extends DataFlow::MethodCallNode {
-      MatchCall() { this.getMethodName() = "match" }
+      MatchCall() { this.getMethodName() = ["match", "matchAll"] }
     }
 
     private class ExecCall extends DataFlow::MethodCallNode {
diff --git a/javascript/ql/src/Security/CWE-020/MissingRegExpAnchor.ql b/javascript/ql/src/Security/CWE-020/MissingRegExpAnchor.ql
index 858dedc603e5..c32a097cf2ad 100644
--- a/javascript/ql/src/Security/CWE-020/MissingRegExpAnchor.ql
+++ b/javascript/ql/src/Security/CWE-020/MissingRegExpAnchor.ql
@@ -36,6 +36,8 @@ private module Impl implements
         name = "replace"
         or
         name = "match" and exists(mcn.getAPropertyRead())
+        or
+        name = "matchAll" and exists(mcn.getAPropertyRead())
       )
     )
   }
diff --git a/javascript/ql/test/query-tests/Security/CWE-020/MissingRegExpAnchor/MissingRegExpAnchor.expected b/javascript/ql/test/query-tests/Security/CWE-020/MissingRegExpAnchor/MissingRegExpAnchor.expected
index a7933f2926ec..ebff0b583c02 100644
--- a/javascript/ql/test/query-tests/Security/CWE-020/MissingRegExpAnchor/MissingRegExpAnchor.expected
+++ b/javascript/ql/test/query-tests/Security/CWE-020/MissingRegExpAnchor/MissingRegExpAnchor.expected
@@ -59,3 +59,12 @@
 | tst-UnanchoredUrlRegExp.js:26:3:26:22 | "^https?://good.com" | This hostname pattern may match any domain name, as it is missing a '$' or '/' at the end. |
 | tst-UnanchoredUrlRegExp.js:35:2:35:32 | /https? ... 0-9]+)/ | When this is used as a regular expression on a URL, it may match anywhere, and arbitrary hosts may come before or after it. |
 | tst-UnanchoredUrlRegExp.js:77:11:77:32 | /vimeo\\ ... 0-9]+)/ | When this is used as a regular expression on a URL, it may match anywhere, and arbitrary hosts may come before or after it. |
+| tst-UnanchoredUrlRegExp.js:111:50:111:68 | "https?://good.com" | When this is used as a regular expression on a URL, it may match anywhere, and arbitrary hosts may come before or after it. |
+| tst-UnanchoredUrlRegExp.js:112:61:112:79 | "https?://good.com" | When this is used as a regular expression on a URL, it may match anywhere, and arbitrary hosts may come before or after it. |
+| tst-UnanchoredUrlRegExp.js:113:50:113:69 | "^https?://good.com" | This hostname pattern may match any domain name, as it is missing a '$' or '/' at the end. |
+| tst-UnanchoredUrlRegExp.js:114:50:114:72 | /^https ... d.com/g | This hostname pattern may match any domain name, as it is missing a '$' or '/' at the end. |
+| tst-UnanchoredUrlRegExp.js:115:50:115:94 | "(^http ... 2.com)" | This hostname pattern may match any domain name, as it is missing a '$' or '/' at the end. |
+| tst-UnanchoredUrlRegExp.js:116:50:116:93 | "(https ... e.com)" | This hostname pattern may match any domain name, as it is missing a '$' or '/' at the end. |
+| tst-UnanchoredUrlRegExp.js:117:50:117:59 | "good.com" | When this is used as a regular expression on a URL, it may match anywhere, and arbitrary hosts may come before or after it. |
+| tst-UnanchoredUrlRegExp.js:118:50:118:68 | "https?://good.com" | When this is used as a regular expression on a URL, it may match anywhere, and arbitrary hosts may come before or after it. |
+| tst-UnanchoredUrlRegExp.js:119:50:119:73 | "https? ... m:8080" | When this is used as a regular expression on a URL, it may match anywhere, and arbitrary hosts may come before or after it. |
diff --git a/javascript/ql/test/query-tests/Security/CWE-020/MissingRegExpAnchor/tst-UnanchoredUrlRegExp.js b/javascript/ql/test/query-tests/Security/CWE-020/MissingRegExpAnchor/tst-UnanchoredUrlRegExp.js
index 24815ebe59e7..c0c5ecb3e3e0 100644
--- a/javascript/ql/test/query-tests/Security/CWE-020/MissingRegExpAnchor/tst-UnanchoredUrlRegExp.js
+++ b/javascript/ql/test/query-tests/Security/CWE-020/MissingRegExpAnchor/tst-UnanchoredUrlRegExp.js
@@ -105,4 +105,27 @@
 
 	/\.com|\.org/; // OK, has no domain name
 	/example\.com|whatever/; // OK, the other disjunction doesn't match a hostname
+
+	// MatchAll test cases:
+	// Vulnerable patterns
+	if ("http://evil.com/?http://good.com".matchAll("https?://good.com")) {} // NOT OK
+	if ("http://evil.com/?http://good.com".matchAll(new RegExp("https?://good.com"))) {} // NOT OK
+	if ("http://evil.com/?http://good.com".matchAll("^https?://good.com")) {} // NOT OK - missing post-anchor
+	if ("http://evil.com/?http://good.com".matchAll(/^https?:\/\/good.com/g)) {} // NOT OK - missing post-anchor
+	if ("http://evil.com/?http://good.com".matchAll("(^https?://good1.com)|(^https?://good2.com)")) {} // NOT OK - missing post-anchor
+	if ("http://evil.com/?http://good.com".matchAll("(https?://good.com)|(^https?://goodie.com)")) {} // NOT OK - missing post-anchor
+	if ("http://evil.com/?http://good.com".matchAll("good.com")) {} // NOT OK - missing protocol
+	if ("http://evil.com/?http://good.com".matchAll("https?://good.com")) {} // NOT OK
+	if ("http://evil.com/?http://good.com".matchAll("https?://good.com:8080")) {} // NOT OK
+
+	// Non-vulnerable patterns
+	if ("something".matchAll("other")) {} // OK
+	if ("something".matchAll("x.commissary")) {} // OK
+	if ("http://evil.com/?http://good.com".matchAll("^https?://good.com$")) {} // OK
+	if ("http://evil.com/?http://good.com".matchAll(new RegExp("^https?://good.com$"))) {} // OK
+	if ("http://evil.com/?http://good.com".matchAll("^https?://good.com/$")) {} // OK
+	if ("http://evil.com/?http://good.com".matchAll(/^https?:\/\/good.com\/$/)) {} // OK
+	if ("http://evil.com/?http://good.com".matchAll("(^https?://good1.com$)|(^https?://good2.com$)")) {} // OK
+	if ("http://evil.com/?http://good.com".matchAll("(https?://good.com$)|(^https?://goodie.com$)")) {} // OK
+
 });

From b239bfabf1893bf12ff205514a64ea52ed4fd2f4 Mon Sep 17 00:00:00 2001
From: Napalys <napalys@github.com>
Date: Tue, 5 Nov 2024 09:22:26 +0100
Subject: [PATCH 02/13] Added tests forIncompleteHostnameRegExp and
 normalizedPaths using matchAll

---
 .../IncompleteHostnameRegExp.expected         |  1 +
 .../tst-IncompleteHostnameRegExp.js           |  2 ++
 .../CWE-022/TaintedPath/TaintedPath.expected  | 29 +++++++++++++++++++
 .../CWE-022/TaintedPath/normalizedPaths.js    | 22 ++++++++++++++
 4 files changed, 54 insertions(+)

diff --git a/javascript/ql/test/query-tests/Security/CWE-020/IncompleteHostnameRegExp/IncompleteHostnameRegExp.expected b/javascript/ql/test/query-tests/Security/CWE-020/IncompleteHostnameRegExp/IncompleteHostnameRegExp.expected
index d0fb91322fe1..8341636994e4 100644
--- a/javascript/ql/test/query-tests/Security/CWE-020/IncompleteHostnameRegExp/IncompleteHostnameRegExp.expected
+++ b/javascript/ql/test/query-tests/Security/CWE-020/IncompleteHostnameRegExp/IncompleteHostnameRegExp.expected
@@ -25,3 +25,4 @@
 | tst-IncompleteHostnameRegExp.js:53:14:53:35 | test.example.com$ | This regular expression has an unescaped '.' before 'example.com', so it might match more hosts than expected. | tst-IncompleteHostnameRegExp.js:53:13:53:36 | 'test.' ... e.com$' | here |
 | tst-IncompleteHostnameRegExp.js:55:14:55:38 | ^http://test.example.com | This regular expression has an unescaped '.' before 'example.com', so it might match more hosts than expected. | tst-IncompleteHostnameRegExp.js:55:13:55:39 | '^http: ... le.com' | here |
 | tst-IncompleteHostnameRegExp.js:59:5:59:20 | foo.example\\.com | This regular expression has an unescaped '.' before 'example\\.com', so it might match more hosts than expected. | tst-IncompleteHostnameRegExp.js:59:2:59:32 | /^(foo. ... ever)$/ | here |
+| tst-IncompleteHostnameRegExp.js:61:18:61:41 | ^http://test.example.com | This regular expression has an unescaped '.' before 'example.com', so it might match more hosts than expected. | tst-IncompleteHostnameRegExp.js:61:17:61:42 | "^http: ... le.com" | here |
diff --git a/javascript/ql/test/query-tests/Security/CWE-020/IncompleteHostnameRegExp/tst-IncompleteHostnameRegExp.js b/javascript/ql/test/query-tests/Security/CWE-020/IncompleteHostnameRegExp/tst-IncompleteHostnameRegExp.js
index ddc267ebdd71..51c2c9914209 100644
--- a/javascript/ql/test/query-tests/Security/CWE-020/IncompleteHostnameRegExp/tst-IncompleteHostnameRegExp.js
+++ b/javascript/ql/test/query-tests/Security/CWE-020/IncompleteHostnameRegExp/tst-IncompleteHostnameRegExp.js
@@ -57,4 +57,6 @@
 	/^http:\/\/(..|...)\.example\.com\/index\.html/; // OK, wildcards are intentional
 	/^http:\/\/.\.example\.com\/index\.html/; // OK, the wildcard is intentional
 	/^(foo.example\.com|whatever)$/; // kinda OK - one disjunction doesn't even look like a hostname
+
+	if (s.matchAll("^http://test.example.com")) {} // NOT OK
 });
diff --git a/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/TaintedPath.expected b/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/TaintedPath.expected
index 2a3784617e75..0022ca69c6ba 100644
--- a/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/TaintedPath.expected
+++ b/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/TaintedPath.expected
@@ -2237,6 +2237,19 @@ nodes
 | normalizedPaths.js:408:38:408:59 | req.que ... it('/') |
 | normalizedPaths.js:408:38:408:59 | req.que ... it('/') |
 | normalizedPaths.js:408:38:408:59 | req.que ... it('/') |
+| normalizedPaths.js:412:7:412:46 | path |
+| normalizedPaths.js:412:7:412:46 | path |
+| normalizedPaths.js:412:14:412:46 | pathMod ... uery.x) |
+| normalizedPaths.js:412:14:412:46 | pathMod ... uery.x) |
+| normalizedPaths.js:412:35:412:45 | req.query.x |
+| normalizedPaths.js:412:35:412:45 | req.query.x |
+| normalizedPaths.js:412:35:412:45 | req.query.x |
+| normalizedPaths.js:415:19:415:22 | path |
+| normalizedPaths.js:415:19:415:22 | path |
+| normalizedPaths.js:415:19:415:22 | path |
+| normalizedPaths.js:426:21:426:24 | path |
+| normalizedPaths.js:426:21:426:24 | path |
+| normalizedPaths.js:426:21:426:24 | path |
 | other-fs-libraries.js:9:7:9:48 | path |
 | other-fs-libraries.js:9:7:9:48 | path |
 | other-fs-libraries.js:9:7:9:48 | path |
@@ -7524,6 +7537,20 @@ edges
 | normalizedPaths.js:408:38:408:59 | req.que ... it('/') | normalizedPaths.js:408:19:408:60 | pathMod ... t('/')) |
 | normalizedPaths.js:408:38:408:59 | req.que ... it('/') | normalizedPaths.js:408:19:408:60 | pathMod ... t('/')) |
 | normalizedPaths.js:408:38:408:59 | req.que ... it('/') | normalizedPaths.js:408:19:408:60 | pathMod ... t('/')) |
+| normalizedPaths.js:412:7:412:46 | path | normalizedPaths.js:415:19:415:22 | path |
+| normalizedPaths.js:412:7:412:46 | path | normalizedPaths.js:415:19:415:22 | path |
+| normalizedPaths.js:412:7:412:46 | path | normalizedPaths.js:415:19:415:22 | path |
+| normalizedPaths.js:412:7:412:46 | path | normalizedPaths.js:415:19:415:22 | path |
+| normalizedPaths.js:412:7:412:46 | path | normalizedPaths.js:426:21:426:24 | path |
+| normalizedPaths.js:412:7:412:46 | path | normalizedPaths.js:426:21:426:24 | path |
+| normalizedPaths.js:412:7:412:46 | path | normalizedPaths.js:426:21:426:24 | path |
+| normalizedPaths.js:412:7:412:46 | path | normalizedPaths.js:426:21:426:24 | path |
+| normalizedPaths.js:412:14:412:46 | pathMod ... uery.x) | normalizedPaths.js:412:7:412:46 | path |
+| normalizedPaths.js:412:14:412:46 | pathMod ... uery.x) | normalizedPaths.js:412:7:412:46 | path |
+| normalizedPaths.js:412:35:412:45 | req.query.x | normalizedPaths.js:412:14:412:46 | pathMod ... uery.x) |
+| normalizedPaths.js:412:35:412:45 | req.query.x | normalizedPaths.js:412:14:412:46 | pathMod ... uery.x) |
+| normalizedPaths.js:412:35:412:45 | req.query.x | normalizedPaths.js:412:14:412:46 | pathMod ... uery.x) |
+| normalizedPaths.js:412:35:412:45 | req.query.x | normalizedPaths.js:412:14:412:46 | pathMod ... uery.x) |
 | other-fs-libraries.js:9:7:9:48 | path | other-fs-libraries.js:11:19:11:22 | path |
 | other-fs-libraries.js:9:7:9:48 | path | other-fs-libraries.js:11:19:11:22 | path |
 | other-fs-libraries.js:9:7:9:48 | path | other-fs-libraries.js:11:19:11:22 | path |
@@ -10539,6 +10566,8 @@ edges
 | normalizedPaths.js:399:21:399:24 | path | normalizedPaths.js:385:35:385:45 | req.query.x | normalizedPaths.js:399:21:399:24 | path | This path depends on a $@. | normalizedPaths.js:385:35:385:45 | req.query.x | user-provided value |
 | normalizedPaths.js:407:19:407:67 | pathMod ... t('/')) | normalizedPaths.js:407:45:407:55 | req.query.x | normalizedPaths.js:407:19:407:67 | pathMod ... t('/')) | This path depends on a $@. | normalizedPaths.js:407:45:407:55 | req.query.x | user-provided value |
 | normalizedPaths.js:408:19:408:60 | pathMod ... t('/')) | normalizedPaths.js:408:38:408:48 | req.query.x | normalizedPaths.js:408:19:408:60 | pathMod ... t('/')) | This path depends on a $@. | normalizedPaths.js:408:38:408:48 | req.query.x | user-provided value |
+| normalizedPaths.js:415:19:415:22 | path | normalizedPaths.js:412:35:412:45 | req.query.x | normalizedPaths.js:415:19:415:22 | path | This path depends on a $@. | normalizedPaths.js:412:35:412:45 | req.query.x | user-provided value |
+| normalizedPaths.js:426:21:426:24 | path | normalizedPaths.js:412:35:412:45 | req.query.x | normalizedPaths.js:426:21:426:24 | path | This path depends on a $@. | normalizedPaths.js:412:35:412:45 | req.query.x | user-provided value |
 | other-fs-libraries.js:11:19:11:22 | path | other-fs-libraries.js:9:24:9:30 | req.url | other-fs-libraries.js:11:19:11:22 | path | This path depends on a $@. | other-fs-libraries.js:9:24:9:30 | req.url | user-provided value |
 | other-fs-libraries.js:12:27:12:30 | path | other-fs-libraries.js:9:24:9:30 | req.url | other-fs-libraries.js:12:27:12:30 | path | This path depends on a $@. | other-fs-libraries.js:9:24:9:30 | req.url | user-provided value |
 | other-fs-libraries.js:13:24:13:27 | path | other-fs-libraries.js:9:24:9:30 | req.url | other-fs-libraries.js:13:24:13:27 | path | This path depends on a $@. | other-fs-libraries.js:9:24:9:30 | req.url | user-provided value |
diff --git a/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/normalizedPaths.js b/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/normalizedPaths.js
index a5453a728d3f..4fa6b3f50d54 100644
--- a/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/normalizedPaths.js
+++ b/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/normalizedPaths.js
@@ -407,3 +407,25 @@ app.get('/join-spread', (req, res) => {
   fs.readFileSync(pathModule.join('foo', ...req.query.x.split('/'))); // NOT OK
   fs.readFileSync(pathModule.join(...req.query.x.split('/'))); // NOT OK
 });
+
+app.get('/dotdot-matchAll-regexp', (req, res) => {
+  let path = pathModule.normalize(req.query.x);
+  if (pathModule.isAbsolute(path))
+    return;
+  fs.readFileSync(path); // NOT OK
+  if (!path.matchAll(/\./)) {
+    fs.readFileSync(path); // OK
+  }
+  if (!path.matchAll(/\.\./)) {
+    fs.readFileSync(path); // OK
+  }
+  if (!path.matchAll(/\.\.\//)) {
+    fs.readFileSync(path); // OK
+  }
+  if (!path.matchAll(/\.\.\/foo/)) {
+    fs.readFileSync(path); // NOT OK
+  }
+  if (!path.matchAll(/(\.\.\/|\.\.\\)/)) {
+    fs.readFileSync(path); // OK
+  }
+});

From 5e8b1b061fdd3947d285ff4fab4568004ea0069d Mon Sep 17 00:00:00 2001
From: Napalys Klicius <napalys@github.com>
Date: Tue, 5 Nov 2024 10:29:22 +0100
Subject: [PATCH 03/13] Update
 javascript/ql/src/Security/CWE-020/MissingRegExpAnchor.ql

Co-authored-by: Erik Krogh Kristensen <erik-krogh@github.com>
---
 javascript/ql/src/Security/CWE-020/MissingRegExpAnchor.ql | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/javascript/ql/src/Security/CWE-020/MissingRegExpAnchor.ql b/javascript/ql/src/Security/CWE-020/MissingRegExpAnchor.ql
index c32a097cf2ad..1057f9ccca50 100644
--- a/javascript/ql/src/Security/CWE-020/MissingRegExpAnchor.ql
+++ b/javascript/ql/src/Security/CWE-020/MissingRegExpAnchor.ql
@@ -35,9 +35,7 @@ private module Impl implements
       |
         name = "replace"
         or
-        name = "match" and exists(mcn.getAPropertyRead())
-        or
-        name = "matchAll" and exists(mcn.getAPropertyRead())
+        name = ["match", "matchAll"] and exists(mcn.getAPropertyRead())
       )
     )
   }

From 4106663d89b3710b713bcf7c35fa37b9187a4ab2 Mon Sep 17 00:00:00 2001
From: Napalys <napalys@github.com>
Date: Thu, 7 Nov 2024 09:43:44 +0100
Subject: [PATCH 04/13] Added tests for regex sanitization to identify false
 positives matchAll

---
 .../Security/CWE-918/SSRF.expected             | 18 ++++++++++++++++++
 .../Security/CWE-918/check-regex.js            |  9 ++++++++-
 2 files changed, 26 insertions(+), 1 deletion(-)

diff --git a/javascript/ql/test/experimental/Security/CWE-918/SSRF.expected b/javascript/ql/test/experimental/Security/CWE-918/SSRF.expected
index 848264b661b1..b947a535fd9f 100644
--- a/javascript/ql/test/experimental/Security/CWE-918/SSRF.expected
+++ b/javascript/ql/test/experimental/Security/CWE-918/SSRF.expected
@@ -51,6 +51,14 @@ nodes
 | check-regex.js:41:13:41:43 | "test.c ... tainted |
 | check-regex.js:41:27:41:43 | req.query.tainted |
 | check-regex.js:41:27:41:43 | req.query.tainted |
+| check-regex.js:44:15:44:45 | "test.c ... tainted |
+| check-regex.js:44:15:44:45 | "test.c ... tainted |
+| check-regex.js:44:29:44:45 | req.query.tainted |
+| check-regex.js:44:29:44:45 | req.query.tainted |
+| check-regex.js:47:15:47:45 | "test.c ... tainted |
+| check-regex.js:47:15:47:45 | "test.c ... tainted |
+| check-regex.js:47:29:47:45 | req.query.tainted |
+| check-regex.js:47:29:47:45 | req.query.tainted |
 | check-validator.js:15:15:15:45 | "test.c ... tainted |
 | check-validator.js:15:15:15:45 | "test.c ... tainted |
 | check-validator.js:15:29:15:45 | req.query.tainted |
@@ -127,6 +135,14 @@ edges
 | check-regex.js:41:27:41:43 | req.query.tainted | check-regex.js:41:13:41:43 | "test.c ... tainted |
 | check-regex.js:41:27:41:43 | req.query.tainted | check-regex.js:41:13:41:43 | "test.c ... tainted |
 | check-regex.js:41:27:41:43 | req.query.tainted | check-regex.js:41:13:41:43 | "test.c ... tainted |
+| check-regex.js:44:29:44:45 | req.query.tainted | check-regex.js:44:15:44:45 | "test.c ... tainted |
+| check-regex.js:44:29:44:45 | req.query.tainted | check-regex.js:44:15:44:45 | "test.c ... tainted |
+| check-regex.js:44:29:44:45 | req.query.tainted | check-regex.js:44:15:44:45 | "test.c ... tainted |
+| check-regex.js:44:29:44:45 | req.query.tainted | check-regex.js:44:15:44:45 | "test.c ... tainted |
+| check-regex.js:47:29:47:45 | req.query.tainted | check-regex.js:47:15:47:45 | "test.c ... tainted |
+| check-regex.js:47:29:47:45 | req.query.tainted | check-regex.js:47:15:47:45 | "test.c ... tainted |
+| check-regex.js:47:29:47:45 | req.query.tainted | check-regex.js:47:15:47:45 | "test.c ... tainted |
+| check-regex.js:47:29:47:45 | req.query.tainted | check-regex.js:47:15:47:45 | "test.c ... tainted |
 | check-validator.js:15:29:15:45 | req.query.tainted | check-validator.js:15:15:15:45 | "test.c ... tainted |
 | check-validator.js:15:29:15:45 | req.query.tainted | check-validator.js:15:15:15:45 | "test.c ... tainted |
 | check-validator.js:15:29:15:45 | req.query.tainted | check-validator.js:15:15:15:45 | "test.c ... tainted |
@@ -166,6 +182,8 @@ edges
 | check-regex.js:31:15:31:45 | "test.c ... tainted | check-regex.js:31:29:31:45 | req.query.tainted | check-regex.js:31:15:31:45 | "test.c ... tainted | The URL of this request depends on a user-provided value. |
 | check-regex.js:34:15:34:42 | baseURL ... tainted | check-regex.js:34:25:34:42 | req.params.tainted | check-regex.js:34:15:34:42 | baseURL ... tainted | The URL of this request depends on a user-provided value. |
 | check-regex.js:41:13:41:43 | "test.c ... tainted | check-regex.js:41:27:41:43 | req.query.tainted | check-regex.js:41:13:41:43 | "test.c ... tainted | The URL of this request depends on a user-provided value. |
+| check-regex.js:44:15:44:45 | "test.c ... tainted | check-regex.js:44:29:44:45 | req.query.tainted | check-regex.js:44:15:44:45 | "test.c ... tainted | The URL of this request depends on a user-provided value. |
+| check-regex.js:47:15:47:45 | "test.c ... tainted | check-regex.js:47:29:47:45 | req.query.tainted | check-regex.js:47:15:47:45 | "test.c ... tainted | The URL of this request depends on a user-provided value. |
 | check-validator.js:15:15:15:45 | "test.c ... tainted | check-validator.js:15:29:15:45 | req.query.tainted | check-validator.js:15:15:15:45 | "test.c ... tainted | The URL of this request depends on a user-provided value. |
 | check-validator.js:27:15:27:45 | "test.c ... tainted | check-validator.js:27:29:27:45 | req.query.tainted | check-validator.js:27:15:27:45 | "test.c ... tainted | The URL of this request depends on a user-provided value. |
 | check-validator.js:50:15:50:45 | "test.c ... tainted | check-validator.js:50:29:50:45 | req.query.tainted | check-validator.js:50:15:50:45 | "test.c ... tainted | The URL of this request depends on a user-provided value. |
diff --git a/javascript/ql/test/experimental/Security/CWE-918/check-regex.js b/javascript/ql/test/experimental/Security/CWE-918/check-regex.js
index a05c1ae7ddfa..323a9a2c8db4 100644
--- a/javascript/ql/test/experimental/Security/CWE-918/check-regex.js
+++ b/javascript/ql/test/experimental/Security/CWE-918/check-regex.js
@@ -25,7 +25,7 @@ app.get('/check-with-axios', req => {
   } else {
     axios.get(baseURL + req.params.tainted); // OK
   }
-  
+
   // Blacklists are not safe
   if (!req.query.tainted.match(/^[/\.%]+$/)) {
     axios.get("test.com/" + req.query.tainted); // SSRF
@@ -39,6 +39,13 @@ app.get('/check-with-axios', req => {
   }
 
   axios.get("test.com/" + req.query.tainted); // OK - False Positive
+
+  if (req.query.tainted.matchAll(/^[0-9a-z]+$/g)) { // letters and numbers
+    axios.get("test.com/" + req.query.tainted); // OK
+  }
+  if (req.query.tainted.matchAll(/^[0-9a-z\-_]+$/g)) { // letters, numbers, - and _
+    axios.get("test.com/" + req.query.tainted); // OK
+  }
 });
 
 const isValidPath = path =>  path.match(/^[0-9a-z]+$/);

From 449cee91c86da4d9806331aa561e0be9e7ef7616 Mon Sep 17 00:00:00 2001
From: Napalys <napalys@github.com>
Date: Thu, 7 Nov 2024 10:03:54 +0100
Subject: [PATCH 05/13] Fixes false positives from commit
 445552d3b53ec9592e8e3892cb337d1004b6a432

---
 .../semmle/javascript/MembershipCandidates.qll |  2 +-
 .../Security/CWE-918/SSRF.expected             | 18 ------------------
 2 files changed, 1 insertion(+), 19 deletions(-)

diff --git a/javascript/ql/lib/semmle/javascript/MembershipCandidates.qll b/javascript/ql/lib/semmle/javascript/MembershipCandidates.qll
index 21f4cc1b1c5c..da9e90744ef0 100644
--- a/javascript/ql/lib/semmle/javascript/MembershipCandidates.qll
+++ b/javascript/ql/lib/semmle/javascript/MembershipCandidates.qll
@@ -193,7 +193,7 @@ module MembershipCandidate {
         or
         // u.match(/re/) or u.match("re")
         base = this and
-        m = "match" and
+        m = ["match", "matchAll"] and
         enumeration = RegExp::getRegExpFromNode(firstArg)
       )
     }
diff --git a/javascript/ql/test/experimental/Security/CWE-918/SSRF.expected b/javascript/ql/test/experimental/Security/CWE-918/SSRF.expected
index b947a535fd9f..848264b661b1 100644
--- a/javascript/ql/test/experimental/Security/CWE-918/SSRF.expected
+++ b/javascript/ql/test/experimental/Security/CWE-918/SSRF.expected
@@ -51,14 +51,6 @@ nodes
 | check-regex.js:41:13:41:43 | "test.c ... tainted |
 | check-regex.js:41:27:41:43 | req.query.tainted |
 | check-regex.js:41:27:41:43 | req.query.tainted |
-| check-regex.js:44:15:44:45 | "test.c ... tainted |
-| check-regex.js:44:15:44:45 | "test.c ... tainted |
-| check-regex.js:44:29:44:45 | req.query.tainted |
-| check-regex.js:44:29:44:45 | req.query.tainted |
-| check-regex.js:47:15:47:45 | "test.c ... tainted |
-| check-regex.js:47:15:47:45 | "test.c ... tainted |
-| check-regex.js:47:29:47:45 | req.query.tainted |
-| check-regex.js:47:29:47:45 | req.query.tainted |
 | check-validator.js:15:15:15:45 | "test.c ... tainted |
 | check-validator.js:15:15:15:45 | "test.c ... tainted |
 | check-validator.js:15:29:15:45 | req.query.tainted |
@@ -135,14 +127,6 @@ edges
 | check-regex.js:41:27:41:43 | req.query.tainted | check-regex.js:41:13:41:43 | "test.c ... tainted |
 | check-regex.js:41:27:41:43 | req.query.tainted | check-regex.js:41:13:41:43 | "test.c ... tainted |
 | check-regex.js:41:27:41:43 | req.query.tainted | check-regex.js:41:13:41:43 | "test.c ... tainted |
-| check-regex.js:44:29:44:45 | req.query.tainted | check-regex.js:44:15:44:45 | "test.c ... tainted |
-| check-regex.js:44:29:44:45 | req.query.tainted | check-regex.js:44:15:44:45 | "test.c ... tainted |
-| check-regex.js:44:29:44:45 | req.query.tainted | check-regex.js:44:15:44:45 | "test.c ... tainted |
-| check-regex.js:44:29:44:45 | req.query.tainted | check-regex.js:44:15:44:45 | "test.c ... tainted |
-| check-regex.js:47:29:47:45 | req.query.tainted | check-regex.js:47:15:47:45 | "test.c ... tainted |
-| check-regex.js:47:29:47:45 | req.query.tainted | check-regex.js:47:15:47:45 | "test.c ... tainted |
-| check-regex.js:47:29:47:45 | req.query.tainted | check-regex.js:47:15:47:45 | "test.c ... tainted |
-| check-regex.js:47:29:47:45 | req.query.tainted | check-regex.js:47:15:47:45 | "test.c ... tainted |
 | check-validator.js:15:29:15:45 | req.query.tainted | check-validator.js:15:15:15:45 | "test.c ... tainted |
 | check-validator.js:15:29:15:45 | req.query.tainted | check-validator.js:15:15:15:45 | "test.c ... tainted |
 | check-validator.js:15:29:15:45 | req.query.tainted | check-validator.js:15:15:15:45 | "test.c ... tainted |
@@ -182,8 +166,6 @@ edges
 | check-regex.js:31:15:31:45 | "test.c ... tainted | check-regex.js:31:29:31:45 | req.query.tainted | check-regex.js:31:15:31:45 | "test.c ... tainted | The URL of this request depends on a user-provided value. |
 | check-regex.js:34:15:34:42 | baseURL ... tainted | check-regex.js:34:25:34:42 | req.params.tainted | check-regex.js:34:15:34:42 | baseURL ... tainted | The URL of this request depends on a user-provided value. |
 | check-regex.js:41:13:41:43 | "test.c ... tainted | check-regex.js:41:27:41:43 | req.query.tainted | check-regex.js:41:13:41:43 | "test.c ... tainted | The URL of this request depends on a user-provided value. |
-| check-regex.js:44:15:44:45 | "test.c ... tainted | check-regex.js:44:29:44:45 | req.query.tainted | check-regex.js:44:15:44:45 | "test.c ... tainted | The URL of this request depends on a user-provided value. |
-| check-regex.js:47:15:47:45 | "test.c ... tainted | check-regex.js:47:29:47:45 | req.query.tainted | check-regex.js:47:15:47:45 | "test.c ... tainted | The URL of this request depends on a user-provided value. |
 | check-validator.js:15:15:15:45 | "test.c ... tainted | check-validator.js:15:29:15:45 | req.query.tainted | check-validator.js:15:15:15:45 | "test.c ... tainted | The URL of this request depends on a user-provided value. |
 | check-validator.js:27:15:27:45 | "test.c ... tainted | check-validator.js:27:29:27:45 | req.query.tainted | check-validator.js:27:15:27:45 | "test.c ... tainted | The URL of this request depends on a user-provided value. |
 | check-validator.js:50:15:50:45 | "test.c ... tainted | check-validator.js:50:29:50:45 | req.query.tainted | check-validator.js:50:15:50:45 | "test.c ... tainted | The URL of this request depends on a user-provided value. |

From 42600c93ffad33fce4975542e11f32d45fd25747 Mon Sep 17 00:00:00 2001
From: Napalys <napalys@github.com>
Date: Thu, 7 Nov 2024 11:40:20 +0100
Subject: [PATCH 06/13] Added tests which shows false positive SSRF via
 matchAll

---
 .../Security/CWE-918/SSRF.expected            | 27 +++++++++++++++++++
 .../Security/CWE-918/check-regex.js           | 14 ++++++++++
 2 files changed, 41 insertions(+)

diff --git a/javascript/ql/test/experimental/Security/CWE-918/SSRF.expected b/javascript/ql/test/experimental/Security/CWE-918/SSRF.expected
index 848264b661b1..0dbb00780702 100644
--- a/javascript/ql/test/experimental/Security/CWE-918/SSRF.expected
+++ b/javascript/ql/test/experimental/Security/CWE-918/SSRF.expected
@@ -51,6 +51,18 @@ nodes
 | check-regex.js:41:13:41:43 | "test.c ... tainted |
 | check-regex.js:41:27:41:43 | req.query.tainted |
 | check-regex.js:41:27:41:43 | req.query.tainted |
+| check-regex.js:58:15:58:42 | baseURL ... tainted |
+| check-regex.js:58:15:58:42 | baseURL ... tainted |
+| check-regex.js:58:25:58:42 | req.params.tainted |
+| check-regex.js:58:25:58:42 | req.params.tainted |
+| check-regex.js:61:15:61:42 | baseURL ... tainted |
+| check-regex.js:61:15:61:42 | baseURL ... tainted |
+| check-regex.js:61:25:61:42 | req.params.tainted |
+| check-regex.js:61:25:61:42 | req.params.tainted |
+| check-regex.js:63:15:63:42 | baseURL ... tainted |
+| check-regex.js:63:15:63:42 | baseURL ... tainted |
+| check-regex.js:63:25:63:42 | req.params.tainted |
+| check-regex.js:63:25:63:42 | req.params.tainted |
 | check-validator.js:15:15:15:45 | "test.c ... tainted |
 | check-validator.js:15:15:15:45 | "test.c ... tainted |
 | check-validator.js:15:29:15:45 | req.query.tainted |
@@ -127,6 +139,18 @@ edges
 | check-regex.js:41:27:41:43 | req.query.tainted | check-regex.js:41:13:41:43 | "test.c ... tainted |
 | check-regex.js:41:27:41:43 | req.query.tainted | check-regex.js:41:13:41:43 | "test.c ... tainted |
 | check-regex.js:41:27:41:43 | req.query.tainted | check-regex.js:41:13:41:43 | "test.c ... tainted |
+| check-regex.js:58:25:58:42 | req.params.tainted | check-regex.js:58:15:58:42 | baseURL ... tainted |
+| check-regex.js:58:25:58:42 | req.params.tainted | check-regex.js:58:15:58:42 | baseURL ... tainted |
+| check-regex.js:58:25:58:42 | req.params.tainted | check-regex.js:58:15:58:42 | baseURL ... tainted |
+| check-regex.js:58:25:58:42 | req.params.tainted | check-regex.js:58:15:58:42 | baseURL ... tainted |
+| check-regex.js:61:25:61:42 | req.params.tainted | check-regex.js:61:15:61:42 | baseURL ... tainted |
+| check-regex.js:61:25:61:42 | req.params.tainted | check-regex.js:61:15:61:42 | baseURL ... tainted |
+| check-regex.js:61:25:61:42 | req.params.tainted | check-regex.js:61:15:61:42 | baseURL ... tainted |
+| check-regex.js:61:25:61:42 | req.params.tainted | check-regex.js:61:15:61:42 | baseURL ... tainted |
+| check-regex.js:63:25:63:42 | req.params.tainted | check-regex.js:63:15:63:42 | baseURL ... tainted |
+| check-regex.js:63:25:63:42 | req.params.tainted | check-regex.js:63:15:63:42 | baseURL ... tainted |
+| check-regex.js:63:25:63:42 | req.params.tainted | check-regex.js:63:15:63:42 | baseURL ... tainted |
+| check-regex.js:63:25:63:42 | req.params.tainted | check-regex.js:63:15:63:42 | baseURL ... tainted |
 | check-validator.js:15:29:15:45 | req.query.tainted | check-validator.js:15:15:15:45 | "test.c ... tainted |
 | check-validator.js:15:29:15:45 | req.query.tainted | check-validator.js:15:15:15:45 | "test.c ... tainted |
 | check-validator.js:15:29:15:45 | req.query.tainted | check-validator.js:15:15:15:45 | "test.c ... tainted |
@@ -166,6 +190,9 @@ edges
 | check-regex.js:31:15:31:45 | "test.c ... tainted | check-regex.js:31:29:31:45 | req.query.tainted | check-regex.js:31:15:31:45 | "test.c ... tainted | The URL of this request depends on a user-provided value. |
 | check-regex.js:34:15:34:42 | baseURL ... tainted | check-regex.js:34:25:34:42 | req.params.tainted | check-regex.js:34:15:34:42 | baseURL ... tainted | The URL of this request depends on a user-provided value. |
 | check-regex.js:41:13:41:43 | "test.c ... tainted | check-regex.js:41:27:41:43 | req.query.tainted | check-regex.js:41:13:41:43 | "test.c ... tainted | The URL of this request depends on a user-provided value. |
+| check-regex.js:58:15:58:42 | baseURL ... tainted | check-regex.js:58:25:58:42 | req.params.tainted | check-regex.js:58:15:58:42 | baseURL ... tainted | The URL of this request depends on a user-provided value. |
+| check-regex.js:61:15:61:42 | baseURL ... tainted | check-regex.js:61:25:61:42 | req.params.tainted | check-regex.js:61:15:61:42 | baseURL ... tainted | The URL of this request depends on a user-provided value. |
+| check-regex.js:63:15:63:42 | baseURL ... tainted | check-regex.js:63:25:63:42 | req.params.tainted | check-regex.js:63:15:63:42 | baseURL ... tainted | The URL of this request depends on a user-provided value. |
 | check-validator.js:15:15:15:45 | "test.c ... tainted | check-validator.js:15:29:15:45 | req.query.tainted | check-validator.js:15:15:15:45 | "test.c ... tainted | The URL of this request depends on a user-provided value. |
 | check-validator.js:27:15:27:45 | "test.c ... tainted | check-validator.js:27:29:27:45 | req.query.tainted | check-validator.js:27:15:27:45 | "test.c ... tainted | The URL of this request depends on a user-provided value. |
 | check-validator.js:50:15:50:45 | "test.c ... tainted | check-validator.js:50:29:50:45 | req.query.tainted | check-validator.js:50:15:50:45 | "test.c ... tainted | The URL of this request depends on a user-provided value. |
diff --git a/javascript/ql/test/experimental/Security/CWE-918/check-regex.js b/javascript/ql/test/experimental/Security/CWE-918/check-regex.js
index 323a9a2c8db4..055992fd22ce 100644
--- a/javascript/ql/test/experimental/Security/CWE-918/check-regex.js
+++ b/javascript/ql/test/experimental/Security/CWE-918/check-regex.js
@@ -51,3 +51,17 @@ app.get('/check-with-axios', req => {
 const isValidPath = path =>  path.match(/^[0-9a-z]+$/);
 
 const isInBlackList = path =>  path.match(/^[/\.%]+$/);
+
+app.get('/check-with-axios', req => {
+  const baseURL = "test.com/"
+  if (isValidPathMatchAll(req.params.tainted) ) {
+    axios.get(baseURL + req.params.tainted); // OK
+  }
+  if (!isValidPathMatchAll(req.params.tainted) ) {
+    axios.get(baseURL + req.params.tainted); // SSRF
+  } else {
+    axios.get(baseURL + req.params.tainted); // OK
+  }
+});
+
+const isValidPathMatchAll = path =>  path.matchAll(/^[0-9a-z]+$/g);

From 514375dbf9f1e09add18c3d3a952d0b65d66e5c4 Mon Sep 17 00:00:00 2001
From: Napalys <napalys@github.com>
Date: Thu, 7 Nov 2024 11:47:36 +0100
Subject: [PATCH 07/13] Fixes false positives from commit
 42600c93ffad33fce4975542e11f32d45fd25747

---
 .../javascript/dataflow/TaintTracking.qll      |  2 +-
 .../Security/CWE-918/SSRF.expected             | 18 ------------------
 2 files changed, 1 insertion(+), 19 deletions(-)

diff --git a/javascript/ql/lib/semmle/javascript/dataflow/TaintTracking.qll b/javascript/ql/lib/semmle/javascript/dataflow/TaintTracking.qll
index 11ce802ac720..d10d53b3c49b 100644
--- a/javascript/ql/lib/semmle/javascript/dataflow/TaintTracking.qll
+++ b/javascript/ql/lib/semmle/javascript/dataflow/TaintTracking.qll
@@ -998,7 +998,7 @@ module TaintTracking {
         or
         // u.match(/re/) or u.match("re")
         base = expr and
-        m = "match" and
+        m = ["match", "matchAll"] and
         RegExp::isGenericRegExpSanitizer(RegExp::getRegExpFromNode(firstArg.flow()),
           sanitizedOutcome)
       )
diff --git a/javascript/ql/test/experimental/Security/CWE-918/SSRF.expected b/javascript/ql/test/experimental/Security/CWE-918/SSRF.expected
index 0dbb00780702..b8f58cb4c785 100644
--- a/javascript/ql/test/experimental/Security/CWE-918/SSRF.expected
+++ b/javascript/ql/test/experimental/Security/CWE-918/SSRF.expected
@@ -51,18 +51,10 @@ nodes
 | check-regex.js:41:13:41:43 | "test.c ... tainted |
 | check-regex.js:41:27:41:43 | req.query.tainted |
 | check-regex.js:41:27:41:43 | req.query.tainted |
-| check-regex.js:58:15:58:42 | baseURL ... tainted |
-| check-regex.js:58:15:58:42 | baseURL ... tainted |
-| check-regex.js:58:25:58:42 | req.params.tainted |
-| check-regex.js:58:25:58:42 | req.params.tainted |
 | check-regex.js:61:15:61:42 | baseURL ... tainted |
 | check-regex.js:61:15:61:42 | baseURL ... tainted |
 | check-regex.js:61:25:61:42 | req.params.tainted |
 | check-regex.js:61:25:61:42 | req.params.tainted |
-| check-regex.js:63:15:63:42 | baseURL ... tainted |
-| check-regex.js:63:15:63:42 | baseURL ... tainted |
-| check-regex.js:63:25:63:42 | req.params.tainted |
-| check-regex.js:63:25:63:42 | req.params.tainted |
 | check-validator.js:15:15:15:45 | "test.c ... tainted |
 | check-validator.js:15:15:15:45 | "test.c ... tainted |
 | check-validator.js:15:29:15:45 | req.query.tainted |
@@ -139,18 +131,10 @@ edges
 | check-regex.js:41:27:41:43 | req.query.tainted | check-regex.js:41:13:41:43 | "test.c ... tainted |
 | check-regex.js:41:27:41:43 | req.query.tainted | check-regex.js:41:13:41:43 | "test.c ... tainted |
 | check-regex.js:41:27:41:43 | req.query.tainted | check-regex.js:41:13:41:43 | "test.c ... tainted |
-| check-regex.js:58:25:58:42 | req.params.tainted | check-regex.js:58:15:58:42 | baseURL ... tainted |
-| check-regex.js:58:25:58:42 | req.params.tainted | check-regex.js:58:15:58:42 | baseURL ... tainted |
-| check-regex.js:58:25:58:42 | req.params.tainted | check-regex.js:58:15:58:42 | baseURL ... tainted |
-| check-regex.js:58:25:58:42 | req.params.tainted | check-regex.js:58:15:58:42 | baseURL ... tainted |
 | check-regex.js:61:25:61:42 | req.params.tainted | check-regex.js:61:15:61:42 | baseURL ... tainted |
 | check-regex.js:61:25:61:42 | req.params.tainted | check-regex.js:61:15:61:42 | baseURL ... tainted |
 | check-regex.js:61:25:61:42 | req.params.tainted | check-regex.js:61:15:61:42 | baseURL ... tainted |
 | check-regex.js:61:25:61:42 | req.params.tainted | check-regex.js:61:15:61:42 | baseURL ... tainted |
-| check-regex.js:63:25:63:42 | req.params.tainted | check-regex.js:63:15:63:42 | baseURL ... tainted |
-| check-regex.js:63:25:63:42 | req.params.tainted | check-regex.js:63:15:63:42 | baseURL ... tainted |
-| check-regex.js:63:25:63:42 | req.params.tainted | check-regex.js:63:15:63:42 | baseURL ... tainted |
-| check-regex.js:63:25:63:42 | req.params.tainted | check-regex.js:63:15:63:42 | baseURL ... tainted |
 | check-validator.js:15:29:15:45 | req.query.tainted | check-validator.js:15:15:15:45 | "test.c ... tainted |
 | check-validator.js:15:29:15:45 | req.query.tainted | check-validator.js:15:15:15:45 | "test.c ... tainted |
 | check-validator.js:15:29:15:45 | req.query.tainted | check-validator.js:15:15:15:45 | "test.c ... tainted |
@@ -190,9 +174,7 @@ edges
 | check-regex.js:31:15:31:45 | "test.c ... tainted | check-regex.js:31:29:31:45 | req.query.tainted | check-regex.js:31:15:31:45 | "test.c ... tainted | The URL of this request depends on a user-provided value. |
 | check-regex.js:34:15:34:42 | baseURL ... tainted | check-regex.js:34:25:34:42 | req.params.tainted | check-regex.js:34:15:34:42 | baseURL ... tainted | The URL of this request depends on a user-provided value. |
 | check-regex.js:41:13:41:43 | "test.c ... tainted | check-regex.js:41:27:41:43 | req.query.tainted | check-regex.js:41:13:41:43 | "test.c ... tainted | The URL of this request depends on a user-provided value. |
-| check-regex.js:58:15:58:42 | baseURL ... tainted | check-regex.js:58:25:58:42 | req.params.tainted | check-regex.js:58:15:58:42 | baseURL ... tainted | The URL of this request depends on a user-provided value. |
 | check-regex.js:61:15:61:42 | baseURL ... tainted | check-regex.js:61:25:61:42 | req.params.tainted | check-regex.js:61:15:61:42 | baseURL ... tainted | The URL of this request depends on a user-provided value. |
-| check-regex.js:63:15:63:42 | baseURL ... tainted | check-regex.js:63:25:63:42 | req.params.tainted | check-regex.js:63:15:63:42 | baseURL ... tainted | The URL of this request depends on a user-provided value. |
 | check-validator.js:15:15:15:45 | "test.c ... tainted | check-validator.js:15:29:15:45 | req.query.tainted | check-validator.js:15:15:15:45 | "test.c ... tainted | The URL of this request depends on a user-provided value. |
 | check-validator.js:27:15:27:45 | "test.c ... tainted | check-validator.js:27:29:27:45 | req.query.tainted | check-validator.js:27:15:27:45 | "test.c ... tainted | The URL of this request depends on a user-provided value. |
 | check-validator.js:50:15:50:45 | "test.c ... tainted | check-validator.js:50:29:50:45 | req.query.tainted | check-validator.js:50:15:50:45 | "test.c ... tainted | The URL of this request depends on a user-provided value. |

From a4fe728af2def2d79559dbbba12bc0bfac5fd9a1 Mon Sep 17 00:00:00 2001
From: Napalys <napalys@github.com>
Date: Thu, 7 Nov 2024 13:09:15 +0100
Subject: [PATCH 08/13] Added matchAll test which is not marked as
 vulnurability by CodeQL

---
 .../test/query-tests/Security/CWE-117/logInjectionBad.js  | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/javascript/ql/test/query-tests/Security/CWE-117/logInjectionBad.js b/javascript/ql/test/query-tests/Security/CWE-117/logInjectionBad.js
index c6c86363164e..2c6a727224e6 100644
--- a/javascript/ql/test/query-tests/Security/CWE-117/logInjectionBad.js
+++ b/javascript/ql/test/query-tests/Security/CWE-117/logInjectionBad.js
@@ -116,4 +116,10 @@ const server4 = http.createServer((req, res) => {
         });
         server.start();
     }
-});
\ No newline at end of file
+});
+
+const serverMatchAll = http.createServer((req, res) => {
+    let username = url.parse(req.url, true).query.username;
+    let otherStr = username.matchAll(/.*/g)[0]; // BAD - this is suppose to be cought by Taint Tracking, works for match but not matchAll
+    console.log(otherStr);
+});

From dbd57e3870c5911324942ad6196270155a002dcf Mon Sep 17 00:00:00 2001
From: Napalys <napalys@github.com>
Date: Thu, 7 Nov 2024 13:40:10 +0100
Subject: [PATCH 09/13] Fixed issue where TaintTracking was not catching
 matchAll vulnerability

---
 .../javascript/dataflow/TaintTracking.qll     |  2 +-
 .../Security/CWE-117/LogInjection.expected    | 24 +++++++++++++++++++
 .../Security/CWE-117/logInjectionBad.js       |  2 +-
 3 files changed, 26 insertions(+), 2 deletions(-)

diff --git a/javascript/ql/lib/semmle/javascript/dataflow/TaintTracking.qll b/javascript/ql/lib/semmle/javascript/dataflow/TaintTracking.qll
index d10d53b3c49b..8a1f5b6f9875 100644
--- a/javascript/ql/lib/semmle/javascript/dataflow/TaintTracking.qll
+++ b/javascript/ql/lib/semmle/javascript/dataflow/TaintTracking.qll
@@ -716,7 +716,7 @@ module TaintTracking {
 
   pragma[nomagic]
   private DataFlow::MethodCallNode matchMethodCall() {
-    result.getMethodName() = "match" and
+    result.getMethodName() = ["match", "matchAll"] and
     exists(DataFlow::AnalyzedNode analyzed |
       pragma[only_bind_into](analyzed) = result.getArgument(0).analyze() and
       analyzed.getAType() = TTRegExp()
diff --git a/javascript/ql/test/query-tests/Security/CWE-117/LogInjection.expected b/javascript/ql/test/query-tests/Security/CWE-117/LogInjection.expected
index db473a17d2c3..b87a6e9bbd4f 100644
--- a/javascript/ql/test/query-tests/Security/CWE-117/LogInjection.expected
+++ b/javascript/ql/test/query-tests/Security/CWE-117/LogInjection.expected
@@ -94,6 +94,18 @@ nodes
 | logInjectionBad.js:99:26:99:33 | username |
 | logInjectionBad.js:113:37:113:44 | username |
 | logInjectionBad.js:113:37:113:44 | username |
+| logInjectionBad.js:122:9:122:58 | username |
+| logInjectionBad.js:122:20:122:43 | url.par ... , true) |
+| logInjectionBad.js:122:20:122:49 | url.par ... ).query |
+| logInjectionBad.js:122:20:122:58 | url.par ... sername |
+| logInjectionBad.js:122:30:122:36 | req.url |
+| logInjectionBad.js:122:30:122:36 | req.url |
+| logInjectionBad.js:123:9:123:46 | otherStr |
+| logInjectionBad.js:123:20:123:27 | username |
+| logInjectionBad.js:123:20:123:43 | usernam ... (/.*/g) |
+| logInjectionBad.js:123:20:123:46 | usernam ... */g)[0] |
+| logInjectionBad.js:124:17:124:24 | otherStr |
+| logInjectionBad.js:124:17:124:24 | otherStr |
 edges
 | logInjectionBad.js:19:9:19:36 | q | logInjectionBad.js:20:20:20:20 | q |
 | logInjectionBad.js:19:13:19:36 | url.par ... , true) | logInjectionBad.js:19:9:19:36 | q |
@@ -186,6 +198,17 @@ edges
 | logInjectionBad.js:73:20:73:20 | q | logInjectionBad.js:73:20:73:26 | q.query |
 | logInjectionBad.js:73:20:73:26 | q.query | logInjectionBad.js:73:20:73:35 | q.query.username |
 | logInjectionBad.js:73:20:73:35 | q.query.username | logInjectionBad.js:73:9:73:35 | username |
+| logInjectionBad.js:122:9:122:58 | username | logInjectionBad.js:123:20:123:27 | username |
+| logInjectionBad.js:122:20:122:43 | url.par ... , true) | logInjectionBad.js:122:20:122:49 | url.par ... ).query |
+| logInjectionBad.js:122:20:122:49 | url.par ... ).query | logInjectionBad.js:122:20:122:58 | url.par ... sername |
+| logInjectionBad.js:122:20:122:58 | url.par ... sername | logInjectionBad.js:122:9:122:58 | username |
+| logInjectionBad.js:122:30:122:36 | req.url | logInjectionBad.js:122:20:122:43 | url.par ... , true) |
+| logInjectionBad.js:122:30:122:36 | req.url | logInjectionBad.js:122:20:122:43 | url.par ... , true) |
+| logInjectionBad.js:123:9:123:46 | otherStr | logInjectionBad.js:124:17:124:24 | otherStr |
+| logInjectionBad.js:123:9:123:46 | otherStr | logInjectionBad.js:124:17:124:24 | otherStr |
+| logInjectionBad.js:123:20:123:27 | username | logInjectionBad.js:123:20:123:43 | usernam ... (/.*/g) |
+| logInjectionBad.js:123:20:123:43 | usernam ... (/.*/g) | logInjectionBad.js:123:20:123:46 | usernam ... */g)[0] |
+| logInjectionBad.js:123:20:123:46 | usernam ... */g)[0] | logInjectionBad.js:123:9:123:46 | otherStr |
 #select
 | logInjectionBad.js:22:18:22:43 | `[INFO] ... rname}` | logInjectionBad.js:19:23:19:29 | req.url | logInjectionBad.js:22:18:22:43 | `[INFO] ... rname}` | Log entry depends on a $@. | logInjectionBad.js:19:23:19:29 | req.url | user-provided value |
 | logInjectionBad.js:23:37:23:44 | username | logInjectionBad.js:19:23:19:29 | req.url | logInjectionBad.js:23:37:23:44 | username | Log entry depends on a $@. | logInjectionBad.js:19:23:19:29 | req.url | user-provided value |
@@ -208,3 +231,4 @@ edges
 | logInjectionBad.js:91:26:91:33 | username | logInjectionBad.js:72:23:72:29 | req.url | logInjectionBad.js:91:26:91:33 | username | Log entry depends on a $@. | logInjectionBad.js:72:23:72:29 | req.url | user-provided value |
 | logInjectionBad.js:99:26:99:33 | username | logInjectionBad.js:72:23:72:29 | req.url | logInjectionBad.js:99:26:99:33 | username | Log entry depends on a $@. | logInjectionBad.js:72:23:72:29 | req.url | user-provided value |
 | logInjectionBad.js:113:37:113:44 | username | logInjectionBad.js:72:23:72:29 | req.url | logInjectionBad.js:113:37:113:44 | username | Log entry depends on a $@. | logInjectionBad.js:72:23:72:29 | req.url | user-provided value |
+| logInjectionBad.js:124:17:124:24 | otherStr | logInjectionBad.js:122:30:122:36 | req.url | logInjectionBad.js:124:17:124:24 | otherStr | Log entry depends on a $@. | logInjectionBad.js:122:30:122:36 | req.url | user-provided value |
diff --git a/javascript/ql/test/query-tests/Security/CWE-117/logInjectionBad.js b/javascript/ql/test/query-tests/Security/CWE-117/logInjectionBad.js
index 2c6a727224e6..35acc1e3aa30 100644
--- a/javascript/ql/test/query-tests/Security/CWE-117/logInjectionBad.js
+++ b/javascript/ql/test/query-tests/Security/CWE-117/logInjectionBad.js
@@ -120,6 +120,6 @@ const server4 = http.createServer((req, res) => {
 
 const serverMatchAll = http.createServer((req, res) => {
     let username = url.parse(req.url, true).query.username;
-    let otherStr = username.matchAll(/.*/g)[0]; // BAD - this is suppose to be cought by Taint Tracking, works for match but not matchAll
+    let otherStr = username.matchAll(/.*/g)[0]; // BAD
     console.log(otherStr);
 });

From c2baf0bd6df3eb8f28b13b63a3ef38fb8dff8282 Mon Sep 17 00:00:00 2001
From: Napalys <napalys@github.com>
Date: Fri, 8 Nov 2024 08:56:12 +0100
Subject: [PATCH 10/13] Added test where RegExp. is used after matchAll but it
 not flagged as potential issue

---
 .../ql/test/query-tests/Security/CWE-117/logInjectionBad.js  | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/javascript/ql/test/query-tests/Security/CWE-117/logInjectionBad.js b/javascript/ql/test/query-tests/Security/CWE-117/logInjectionBad.js
index 35acc1e3aa30..21913f09a4a0 100644
--- a/javascript/ql/test/query-tests/Security/CWE-117/logInjectionBad.js
+++ b/javascript/ql/test/query-tests/Security/CWE-117/logInjectionBad.js
@@ -123,3 +123,8 @@ const serverMatchAll = http.createServer((req, res) => {
     let otherStr = username.matchAll(/.*/g)[0]; // BAD
     console.log(otherStr);
 });
+
+const serverMatchAl2l = http.createServer((req, res) => {
+    const result = url.parse(req.url, true).query.username.matchAll(/(\d+)/g); // BAD - match is marked as vulnerable, while matchAll is not.
+    console.log("First captured group:", RegExp.$1);
+});

From 70cf1a57bcae0dd50d89ca5bec935de401e93af6 Mon Sep 17 00:00:00 2001
From: Napalys <napalys@github.com>
Date: Fri, 8 Nov 2024 08:59:31 +0100
Subject: [PATCH 11/13] Now catches usage of RegExp. after matchAll usage.

---
 .../semmle/javascript/dataflow/TaintTracking.qll   |  2 +-
 .../Security/CWE-117/LogInjection.expected         | 14 ++++++++++++++
 .../Security/CWE-117/logInjectionBad.js            |  2 +-
 3 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/javascript/ql/lib/semmle/javascript/dataflow/TaintTracking.qll b/javascript/ql/lib/semmle/javascript/dataflow/TaintTracking.qll
index 8a1f5b6f9875..989c551ab2fa 100644
--- a/javascript/ql/lib/semmle/javascript/dataflow/TaintTracking.qll
+++ b/javascript/ql/lib/semmle/javascript/dataflow/TaintTracking.qll
@@ -917,7 +917,7 @@ module TaintTracking {
      */
     private ControlFlowNode getACaptureSetter(DataFlow::Node input) {
       exists(DataFlow::MethodCallNode call | result = call.asExpr() |
-        call.getMethodName() = ["search", "replace", "replaceAll", "match"] and
+        call.getMethodName() = ["search", "replace", "replaceAll", "match", "matchAll"] and
         input = call.getReceiver()
         or
         call.getMethodName() = ["test", "exec"] and input = call.getArgument(0)
diff --git a/javascript/ql/test/query-tests/Security/CWE-117/LogInjection.expected b/javascript/ql/test/query-tests/Security/CWE-117/LogInjection.expected
index b87a6e9bbd4f..1f810f1beea8 100644
--- a/javascript/ql/test/query-tests/Security/CWE-117/LogInjection.expected
+++ b/javascript/ql/test/query-tests/Security/CWE-117/LogInjection.expected
@@ -106,6 +106,13 @@ nodes
 | logInjectionBad.js:123:20:123:46 | usernam ... */g)[0] |
 | logInjectionBad.js:124:17:124:24 | otherStr |
 | logInjectionBad.js:124:17:124:24 | otherStr |
+| logInjectionBad.js:128:20:128:43 | url.par ... , true) |
+| logInjectionBad.js:128:20:128:49 | url.par ... ).query |
+| logInjectionBad.js:128:20:128:58 | url.par ... sername |
+| logInjectionBad.js:128:30:128:36 | req.url |
+| logInjectionBad.js:128:30:128:36 | req.url |
+| logInjectionBad.js:129:42:129:50 | RegExp.$1 |
+| logInjectionBad.js:129:42:129:50 | RegExp.$1 |
 edges
 | logInjectionBad.js:19:9:19:36 | q | logInjectionBad.js:20:20:20:20 | q |
 | logInjectionBad.js:19:13:19:36 | url.par ... , true) | logInjectionBad.js:19:9:19:36 | q |
@@ -209,6 +216,12 @@ edges
 | logInjectionBad.js:123:20:123:27 | username | logInjectionBad.js:123:20:123:43 | usernam ... (/.*/g) |
 | logInjectionBad.js:123:20:123:43 | usernam ... (/.*/g) | logInjectionBad.js:123:20:123:46 | usernam ... */g)[0] |
 | logInjectionBad.js:123:20:123:46 | usernam ... */g)[0] | logInjectionBad.js:123:9:123:46 | otherStr |
+| logInjectionBad.js:128:20:128:43 | url.par ... , true) | logInjectionBad.js:128:20:128:49 | url.par ... ).query |
+| logInjectionBad.js:128:20:128:49 | url.par ... ).query | logInjectionBad.js:128:20:128:58 | url.par ... sername |
+| logInjectionBad.js:128:20:128:58 | url.par ... sername | logInjectionBad.js:129:42:129:50 | RegExp.$1 |
+| logInjectionBad.js:128:20:128:58 | url.par ... sername | logInjectionBad.js:129:42:129:50 | RegExp.$1 |
+| logInjectionBad.js:128:30:128:36 | req.url | logInjectionBad.js:128:20:128:43 | url.par ... , true) |
+| logInjectionBad.js:128:30:128:36 | req.url | logInjectionBad.js:128:20:128:43 | url.par ... , true) |
 #select
 | logInjectionBad.js:22:18:22:43 | `[INFO] ... rname}` | logInjectionBad.js:19:23:19:29 | req.url | logInjectionBad.js:22:18:22:43 | `[INFO] ... rname}` | Log entry depends on a $@. | logInjectionBad.js:19:23:19:29 | req.url | user-provided value |
 | logInjectionBad.js:23:37:23:44 | username | logInjectionBad.js:19:23:19:29 | req.url | logInjectionBad.js:23:37:23:44 | username | Log entry depends on a $@. | logInjectionBad.js:19:23:19:29 | req.url | user-provided value |
@@ -232,3 +245,4 @@ edges
 | logInjectionBad.js:99:26:99:33 | username | logInjectionBad.js:72:23:72:29 | req.url | logInjectionBad.js:99:26:99:33 | username | Log entry depends on a $@. | logInjectionBad.js:72:23:72:29 | req.url | user-provided value |
 | logInjectionBad.js:113:37:113:44 | username | logInjectionBad.js:72:23:72:29 | req.url | logInjectionBad.js:113:37:113:44 | username | Log entry depends on a $@. | logInjectionBad.js:72:23:72:29 | req.url | user-provided value |
 | logInjectionBad.js:124:17:124:24 | otherStr | logInjectionBad.js:122:30:122:36 | req.url | logInjectionBad.js:124:17:124:24 | otherStr | Log entry depends on a $@. | logInjectionBad.js:122:30:122:36 | req.url | user-provided value |
+| logInjectionBad.js:129:42:129:50 | RegExp.$1 | logInjectionBad.js:128:30:128:36 | req.url | logInjectionBad.js:129:42:129:50 | RegExp.$1 | Log entry depends on a $@. | logInjectionBad.js:128:30:128:36 | req.url | user-provided value |
diff --git a/javascript/ql/test/query-tests/Security/CWE-117/logInjectionBad.js b/javascript/ql/test/query-tests/Security/CWE-117/logInjectionBad.js
index 21913f09a4a0..028412c346b0 100644
--- a/javascript/ql/test/query-tests/Security/CWE-117/logInjectionBad.js
+++ b/javascript/ql/test/query-tests/Security/CWE-117/logInjectionBad.js
@@ -125,6 +125,6 @@ const serverMatchAll = http.createServer((req, res) => {
 });
 
 const serverMatchAl2l = http.createServer((req, res) => {
-    const result = url.parse(req.url, true).query.username.matchAll(/(\d+)/g); // BAD - match is marked as vulnerable, while matchAll is not.
+    const result = url.parse(req.url, true).query.username.matchAll(/(\d+)/g); // BAD
     console.log("First captured group:", RegExp.$1);
 });

From 1eabb6cbdd20e6e78d15d21cfd3b7d4c6c1b1e42 Mon Sep 17 00:00:00 2001
From: Napalys Klicius <napalys@github.com>
Date: Mon, 11 Nov 2024 15:40:22 +0100
Subject: [PATCH 12/13] Update
 javascript/ql/test/experimental/Security/CWE-918/check-regex.js

Co-authored-by: Erik Krogh Kristensen <erik-krogh@github.com>
---
 javascript/ql/test/experimental/Security/CWE-918/check-regex.js | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/javascript/ql/test/experimental/Security/CWE-918/check-regex.js b/javascript/ql/test/experimental/Security/CWE-918/check-regex.js
index 055992fd22ce..238aa9068432 100644
--- a/javascript/ql/test/experimental/Security/CWE-918/check-regex.js
+++ b/javascript/ql/test/experimental/Security/CWE-918/check-regex.js
@@ -58,7 +58,7 @@ app.get('/check-with-axios', req => {
     axios.get(baseURL + req.params.tainted); // OK
   }
   if (!isValidPathMatchAll(req.params.tainted) ) {
-    axios.get(baseURL + req.params.tainted); // SSRF
+    axios.get(baseURL + req.params.tainted); // NOT OK - SSRF
   } else {
     axios.get(baseURL + req.params.tainted); // OK
   }

From 00790bf3f42828b100a6c0cc8561851141f583ee Mon Sep 17 00:00:00 2001
From: Napalys <napalys@github.com>
Date: Mon, 11 Nov 2024 15:43:54 +0100
Subject: [PATCH 13/13] Added change notes

---
 javascript/ql/lib/change-notes/2024-11-11-matchAll-support.md | 4 ++++
 1 file changed, 4 insertions(+)
 create mode 100644 javascript/ql/lib/change-notes/2024-11-11-matchAll-support.md

diff --git a/javascript/ql/lib/change-notes/2024-11-11-matchAll-support.md b/javascript/ql/lib/change-notes/2024-11-11-matchAll-support.md
new file mode 100644
index 000000000000..74d7c3f34c2d
--- /dev/null
+++ b/javascript/ql/lib/change-notes/2024-11-11-matchAll-support.md
@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+Added support for `String.prototype.matchAll`.