JS: only select non-nullable terms in the broken sanitizer

Author: esbena

javascript/ql/src/Security/CWE-116/IncompleteMultiCharacterSanitization.ql

@@ -147,13 +147,16 @@ from
 where
   regexp = replace.getRegExp().getRoot() and
   dangerous.getRootTerm() = regexp and
+  // skip leading optional elements
+  not dangerous.isNullable() and
   // only warn about the longest match (presumably the most descriptive)
   prefix = max(string m | matchesDangerousPrefix(dangerous, m, kind) | m order by m.length()) and
   // only warn once per kind
   not exists(EmptyReplaceRegExpTerm other |
     other = dangerous.getAChild+() or other = dangerous.getPredecessor+()
   |
-    matchesDangerousPrefix(other, _, kind)
+    matchesDangerousPrefix(other, _, kind) and
+    not other.isNullable()
   ) and
   // don't flag replace operations in a loop
   not replace.getAMethodCall*().flowsTo(replace.getReceiver()) and

javascript/ql/test/query-tests/Security/CWE-116/IncompleteSanitization/IncompleteMultiCharacterSanitization.expected

@@ -7,6 +7,7 @@
 | tst-multi-character-sanitization.js:19:3:19:35 | respons ... pt, "") | This string may still contain $@, which may cause a HTML element injection vulnerability. | tst-multi-character-sanitization.js:18:18:18:24 | <script | <script |
 | tst-multi-character-sanitization.js:25:10:25:40 | text.re ... /g, "") | This string may still contain $@, which may cause a HTML element injection vulnerability. | tst-multi-character-sanitization.js:25:24:25:27 | <!-- | <!-- |
 | tst-multi-character-sanitization.js:49:13:49:43 | req.url ... EL, "") | This string may still contain $@, which may cause a path injection vulnerability. | tst-multi-character-sanitization.js:48:22:48:23 | \\/ | /.. |
+| tst-multi-character-sanitization.js:49:13:49:43 | req.url ... EL, "") | This string may still contain $@, which may cause a path injection vulnerability. | tst-multi-character-sanitization.js:48:26:48:27 | \\. | ../ |
 | tst-multi-character-sanitization.js:64:7:64:73 | x.repla ... /g, "") | This string may still contain $@, which may cause a HTML element injection vulnerability. | tst-multi-character-sanitization.js:64:18:64:24 | <script | <script |
 | tst-multi-character-sanitization.js:66:7:66:56 | x.repla ... /g, "") | This string may still contain $@, which may cause a HTML attribute injection vulnerability. | tst-multi-character-sanitization.js:66:18:66:49 | (\\/\|\\s)on\\w+=(\\'\|")?[^"]*(\\'\|")? | on |
 | tst-multi-character-sanitization.js:75:7:75:37 | x.repla ... gm, "") | This string may still contain $@, which may cause a HTML element injection vulnerability. | tst-multi-character-sanitization.js:75:18:75:21 | <!-- | <!-- |
@@ -26,8 +27,8 @@
 | tst-multi-character-sanitization.js:108:7:108:75 | x.repla ... gm, "") | This string may still contain $@, which may cause a HTML element injection vulnerability. | tst-multi-character-sanitization.js:108:18:108:18 | < | <script |
 | tst-multi-character-sanitization.js:109:7:109:58 | x.repla ... gi, "") | This string may still contain $@, which may cause a HTML element injection vulnerability. | tst-multi-character-sanitization.js:109:18:109:24 | <script | <script |
 | tst-multi-character-sanitization.js:110:7:110:50 | x.repla ... gi, "") | This string may still contain $@, which may cause a HTML element injection vulnerability. | tst-multi-character-sanitization.js:110:18:110:24 | <script | <script |
-| tst-multi-character-sanitization.js:111:7:111:32 | x.repla ... /g, "") | This string may still contain $@, which may cause a HTML element injection vulnerability. | tst-multi-character-sanitization.js:111:18:111:19 |  ? | <!-- |
-| tst-multi-character-sanitization.js:126:7:129:34 | x\\n    . ... //, "") | This string may still contain $@, which may cause a path injection vulnerability. | tst-multi-character-sanitization.js:129:15:129:20 | [^\\/]* | /.. |
+| tst-multi-character-sanitization.js:111:7:111:32 | x.repla ... /g, "") | This string may still contain $@, which may cause a HTML element injection vulnerability. | tst-multi-character-sanitization.js:111:20:111:23 | <!-- | <!-- |
+| tst-multi-character-sanitization.js:126:7:129:34 | x\\n    . ... //, "") | This string may still contain $@, which may cause a path injection vulnerability. | tst-multi-character-sanitization.js:129:21:129:22 | \\/ | /.. |
 | tst-multi-character-sanitization.js:135:2:135:44 | content ... gi, "") | This string may still contain $@, which may cause a HTML element injection vulnerability. | tst-multi-character-sanitization.js:135:19:135:25 | <script | <script |
 | tst-multi-character-sanitization.js:136:2:136:46 | content ... gi, "") | This string may still contain $@, which may cause a HTML element injection vulnerability. | tst-multi-character-sanitization.js:136:19:136:19 | < | <script |
-| tst-multi-character-sanitization.js:138:2:138:48 | content ... gi, "") | This string may still contain $@, which may cause a HTML element injection vulnerability. | tst-multi-character-sanitization.js:138:19:138:20 | .* | <script |
+| tst-multi-character-sanitization.js:138:2:138:48 | content ... gi, "") | This string may still contain $@, which may cause a HTML element injection vulnerability. | tst-multi-character-sanitization.js:138:21:138:21 | < | <script |