JS: sharpen js/incomplete-url-regexp by not matching .* or .+

Author: Esben Sparre Andreasen

javascript/ql/src/Security/CWE-020/IncompleteUrlRegExp.ql

@@ -43,11 +43,9 @@ bindingset[pattern]
 predicate isIncompleteHostNameRegExpPattern(string pattern, string hostPart) {
   hostPart = pattern.regexpCapture(
     "(?i).*" +
-    // Either:
-    // - an unescaped and repeated  `.`, followed by anything
-    // - a unescaped single `.`
-    "(?:(?<!\\\\)[.][+*].*?|(?<!\\\\)[.])" +
-    // a sequence of subdomains, perhaps with some regex characters mixed in, followed by a known TLD
+    // an unescaped single `.`
+    "(?<!\\\\)[.]" +
+    // immediately followed by a sequence of subdomains, perhaps with some regex characters mixed in, followed by a known TLD
     "([():|?a-z0-9-]+(\\\\)?[.](com|org|edu|gov|uk|net))" +
     ".*", 1)
 }

javascript/ql/test/query-tests/Security/CWE-020/IncompleteUrlRegExp.expected

@@ -1,8 +1,6 @@
 | tst-IncompleteUrlRegExp.js:3:2:3:28 | /http:\\ ... le.com/ | This regular expression has an unescaped '.', which means that 'example.com' might not match the intended host of a matched URL. |
 | tst-IncompleteUrlRegExp.js:5:2:5:28 | /http:\\ ... le.net/ | This regular expression has an unescaped '.', which means that 'example.net' might not match the intended host of a matched URL. |
 | tst-IncompleteUrlRegExp.js:6:2:6:42 | /http:\\ ... b).com/ | This regular expression has an unescaped '.', which means that '(example-a\|example-b).com' might not match the intended host of a matched URL. |
-| tst-IncompleteUrlRegExp.js:7:2:7:30 | /http:\\ ... le.com/ | This regular expression has an unescaped '.', which means that 'example.com' might not match the intended host of a matched URL. |
-| tst-IncompleteUrlRegExp.js:9:2:9:39 | /http:\\ ... le.com/ | This regular expression has an unescaped '.', which means that 'example.com' might not match the intended host of a matched URL. |
 | tst-IncompleteUrlRegExp.js:11:13:11:37 | "http:/ ... le.com" | This regular expression has an unescaped '.', which means that 'example.com' might not match the intended host of a matched URL. |
 | tst-IncompleteUrlRegExp.js:12:10:12:34 | "http:/ ... le.com" | This regular expression has an unescaped '.', which means that 'example.com' might not match the intended host of a matched URL. |
 | tst-IncompleteUrlRegExp.js:15:22:15:46 | "http:/ ... le.com" | This regular expression has an unescaped '.', which means that 'example.com' might not match the intended host of a matched URL. |
@@ -11,7 +9,6 @@
 | tst-IncompleteUrlRegExp.js:19:17:19:34 | 'test.example.com' | This regular expression has an unescaped '.', which means that 'example.com' might not match the intended host of a matched URL. |
 | tst-IncompleteUrlRegExp.js:22:27:22:44 | 'test.example.com' | This regular expression has an unescaped '.', which means that 'example.com' might not match the intended host of a matched URL. |
 | tst-IncompleteUrlRegExp.js:28:22:28:39 | 'test.example.com' | This regular expression has an unescaped '.', which means that 'example.com' might not match the intended host of a matched URL. |
-| tst-IncompleteUrlRegExp.js:36:2:36:37 | /(.+\\.( ... \\.com)/ | This regular expression has an unescaped '.', which means that '(?:example-a\|example-b)\\.com' might not match the intended host of a matched URL. |
 | tst-IncompleteUrlRegExp.js:37:2:37:54 | /^(http ... =$\|\\/)/ | This regular expression has an unescaped '.', which means that ')?example.com' might not match the intended host of a matched URL. |
 | tst-IncompleteUrlRegExp.js:38:2:38:44 | /^(http ... p\\/f\\// | This regular expression has an unescaped '.', which means that 'example.com' might not match the intended host of a matched URL. |
 | tst-IncompleteUrlRegExp.js:39:2:39:34 | /\\(http ... m\\/\\)/g | This regular expression has an unescaped '.', which means that 'example.com' might not match the intended host of a matched URL. |

javascript/ql/test/query-tests/Security/CWE-020/tst-IncompleteUrlRegExp.js

@@ -4,9 +4,9 @@
 	/http:\/\/test\\.example.com/; // OK
 	/http:\/\/test.example.net/; // NOT OK
 	/http:\/\/test.(example-a|example-b).com/; // NOT OK
-	/http:\/\/(.+)\\.example.com/; // NOT OK
+	/http:\/\/(.+)\\.example.com/; // NOT OK, but not yet supported with enough precision
 	/http:\/\/(\\.+)\\.example.com/; // OK
-	/http:\/\/(?:.+)\\.test\\.example.com/; // NOT OK
+	/http:\/\/(?:.+)\\.test\\.example.com/; // NOT OK, but not yet supported with enough precision
 	/http:\/\/test.example.com\/(?:.*)/; // OK
 	new RegExp("http://test.example.com"); // NOT OK
 	s.match("http://test.example.com"); // NOT OK
@@ -33,7 +33,7 @@
 	}
 	domains.map(d => convert(d));
 
-	/(.+\.(?:example-a|example-b)\.com)/; // NOT OK
+	/(.+\.(?:example-a|example-b)\.com)/; // NOT OK, but not yet supported with enough precision
 	/^(https?:)?\/\/((service|www).)?example.com(?=$|\/)/; // NOT OK
 	/^(http|https):\/\/www.example.com\/p\/f\//; // NOT OK
 	/\(http:\/\/sub.example.com\/\)/g; // NOT OK