JS: address review comments

Author: Esben Sparre Andreasen

javascript/ql/src/Security/CWE-020/IncompleteHostnameRegExp.qhelp

@@ -8,7 +8,7 @@
 
 			Sanitizing untrusted URLs is an important technique for
 			preventing attacks such as request forgeries and malicious
-			redirections. Usually, this is done by checking that the host of a URL
+			redirections. Often, this is done by checking that the host of a URL
 			is in a set of allowed hosts.
 
 		</p>
@@ -56,7 +56,7 @@
 			an attacker-controlled domain such as <code>wwwXexample.com</code>.
 
 			Address this vulnerability by escaping <code>.</code>
-			appropriately: <code>let regex =/(www|beta|)\.example\.com/</code>.
+			appropriately: <code>let regex = /(www|beta|)\.example\.com/</code>.
 
 		</p>
 

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

@@ -12,28 +12,25 @@
 
 import javascript
 
-module IncompleteHostnameRegExpTracking {
-
-  /**
-   * A taint tracking configuration for incomplete hostname regular expressions sources.
-   */
-  class Configuration extends TaintTracking::Configuration {
-    Configuration() { this = "IncompleteHostnameRegExpTracking" }
-
-    override
-    predicate isSource(DataFlow::Node source) {
-      isIncompleteHostNameRegExpPattern(source.asExpr().getStringValue(), _)
-    }
+/**
+ * A taint tracking configuration for incomplete hostname regular expressions sources.
+ */
+class Configuration extends TaintTracking::Configuration {
+  Configuration() { this = "IncompleteHostnameRegExpTracking" }
 
-    override
-    predicate isSink(DataFlow::Node sink) {
-      isInterpretedAsRegExp(sink)
-    }
+  override
+  predicate isSource(DataFlow::Node source) {
+    isIncompleteHostNameRegExpPattern(source.asExpr().getStringValue(), _)
+  }
 
+  override
+  predicate isSink(DataFlow::Node sink) {
+    isInterpretedAsRegExp(sink)
   }
 
 }
 
+
 /**
  * Holds if `pattern` is a regular expression pattern for URLs with a host matched by `hostPart`,
  * and `pattern` contains a subtle mistake that allows it to match unexpected hosts.
@@ -45,23 +42,23 @@ predicate isIncompleteHostNameRegExpPattern(string pattern, string hostPart) {
     // 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))" +
+    "([():|?a-z0-9-]+(\\\\)?[.](" + RegExpPatterns::commonTLD() + "))" +
     ".*", 1)
 }
 
 from Expr e, string pattern, string hostPart
 where
       (
         e.(RegExpLiteral).getValue() = pattern or
-        exists (IncompleteHostnameRegExpTracking::Configuration cfg |
+        exists (Configuration cfg |
           cfg.hasFlow(e.flow(), _) and
           e.mayHaveStringValue(pattern)
         )
       ) and
       isIncompleteHostNameRegExpPattern(pattern, hostPart)
       and
       // ignore patterns with capture groups after the TLD
-      not pattern.regexpMatch("(?i).*[.](com|org|edu|gov|uk|net).*[(][?]:.*[)].*")
+      not pattern.regexpMatch("(?i).*[.](" + RegExpPatterns::commonTLD() + ").*[(][?]:.*[)].*")
 
 
 select e, "This regular expression has an unescaped '.' before '" + hostPart + "', so it might match more hosts than expected."

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

@@ -21,7 +21,7 @@ where
       substring.mayHaveStringValue(target) and
       (
         // target contains a domain on a common TLD, and perhaps some other URL components
-        target.regexpMatch("(?i)([a-z]*:?//)?\\.?([a-z0-9-]+\\.)+(com|org|edu|gov|uk|net)(:[0-9]+)?/?") or
+        target.regexpMatch("(?i)([a-z]*:?//)?\\.?([a-z0-9-]+\\.)+(" + RegExpPatterns::commonTLD() + ")(:[0-9]+)?/?") or
         // target is a HTTP URL to a domain on any TLD
         target.regexpMatch("(?i)https?://([a-z0-9-]+\\.)+([a-z]+)(:[0-9]+)?/?")
       ) and

javascript/ql/src/semmle/javascript/Regexp.qll

@@ -504,8 +504,22 @@ predicate isInterpretedAsRegExp(DataFlow::Node source) {
       methodName = "search" and
       source.asExpr() = mce.getArgument(0) and
       mce.getNumArgument() = 1 and
-      // `String.prototype.search` returns a number, so exclude chained accesses
+      // "search" is a common method name, and so we exclude chained accesses
+      // because `String.prototype.search` returns a number
       not exists(PropAccess p | p.getBase() = mce)
     )
   )
 }
+
+/**
+ * Provides regular expression patterns.
+ */
+module RegExpPatterns {
+  /**
+   * Gets a pattern that matches common top-level domain names.
+   */
+  string commonTLD() {
+    // according to ranking by http://google.com/search?q=site:.<<TLD>>
+    result = "com|org|edu|gov|uk|net|io"
+  }
+}