JS: imcomplete sanization now handles properly maybe global

github · Nov 27, 2024 · 57d542f · 57d542f
1 parent 577085d
commit 57d542f
Show file tree

Hide file tree

Showing 3 changed files with 5 additions and 8 deletions.
diff --git a/javascript/ql/src/Security/CWE-116/IncompleteSanitization.ql b/javascript/ql/src/Security/CWE-116/IncompleteSanitization.ql
@@ -53,7 +53,7 @@ predicate isSimpleAlt(RegExpAlt t) { forall(RegExpTerm ch | ch = t.getAChild() |
  * regular expression and `new` prefixes the matched string with a backslash.
  */
 predicate isBackslashEscape(StringReplaceCall mce, DataFlow::RegExpCreationNode re) {
-  mce.isGlobal() and
+  mce.maybeGlobal() and
   re = mce.getRegExp() and
   (
     // replacement with `\$&`, `\$1` or similar
@@ -147,7 +147,7 @@ from StringReplaceCall repl, DataFlow::Node old, string msg
 where
   (old = repl.getArgument(0) or old = repl.getRegExp()) and
   (
-    not repl.isGlobal() and
+    not repl.maybeGlobal() and
     msg = "This replaces only the first occurrence of " + old + "." and
     // only flag if this is likely to be a sanitizer or URL encoder or decoder
     exists(string m | m = getAMatchedString(old) |

diff --git a/.../test/query-tests/Security/CWE-116/IncompleteSanitization/IncompleteSanitization.expected b/.../test/query-tests/Security/CWE-116/IncompleteSanitization/IncompleteSanitization.expected
@@ -29,12 +29,9 @@
 | tst.js:149:2:149:24 | x.repla ... replace | This replaces only the first occurrence of "\\n". |
 | tst.js:193:9:193:17 | s.replace | This replaces only the first occurrence of /'/. |
 | tst.js:202:10:202:18 | p.replace | This replaces only the first occurrence of "/../". |
-| tst.js:337:2:337:12 | s().replace | This replaces only the first occurrence of new Reg ... nown()). |
 | tst.js:341:9:341:17 | p.replace | This replaces only the first occurrence of new Reg ... .\\\\./"). |
 | tst.js:345:9:345:17 | s.replace | This does not escape backslash characters in the input. |
 | tst.js:349:9:349:17 | s.replace | This replaces only the first occurrence of new RegExp("\\'"). |
-| tst.js:353:9:353:17 | s.replace | This replaces only the first occurrence of new Reg ... lags()). |
+| tst.js:353:9:353:17 | s.replace | This does not escape backslash characters in the input. |
 | tst.js:362:2:362:10 | x.replace | This replaces only the first occurrence of new RegExp("\\n"). |
 | tst.js:363:2:363:24 | x.repla ... replace | This replaces only the first occurrence of new RegExp("\\n"). |
-| tst.js:365:2:365:10 | x.replace | This replaces only the first occurrence of new Reg ... lags()). |
-| tst.js:366:2:366:24 | x.repla ... replace | This replaces only the first occurrence of new Reg ... lags()). |
diff --git a/javascript/ql/test/query-tests/Security/CWE-116/IncompleteSanitization/tst.js b/javascript/ql/test/query-tests/Security/CWE-116/IncompleteSanitization/tst.js
@@ -362,6 +362,6 @@ function newlinesNewReGexp(s) {
 	x.replace(new RegExp("\n"), "").replace(x, y); // NOT OK
 	x.replace(x, y).replace(new RegExp("\n"), ""); // NOT OK
 
-	x.replace(new RegExp("\n", unknownFlags()), "").replace(x, y); // OK -- Should not be flagged but now it is 
-	x.replace(x, y).replace(new RegExp("\n", unknownFlags()), ""); // OK -- Should not be flagged but now it is 
+	x.replace(new RegExp("\n", unknownFlags()), "").replace(x, y); // OK
+	x.replace(x, y).replace(new RegExp("\n", unknownFlags()), ""); // OK
 }