Merge pull request #4173 from erik-krogh/targetBlankFP

Approved by esbena

Author: codeql-ci

change-notes/1.26/analysis-javascript.md

@@ -26,6 +26,7 @@
 
 | **Query**                      | **Expected impact**          | **Change**                                                                |
 |--------------------------------|------------------------------|---------------------------------------------------------------------------|
+| Potentially unsafe external link (`js/unsafe-external-link`) | Fewer results | This query no longer flags URLs constructed using a template system where only the hash or query part of the URL is dynamic. |
 | Incomplete URL substring sanitization (`js/incomplete-url-substring-sanitization`) | More results | This query now recognizes additional URLs when the substring check is an inclusion check. |
 | Ambiguous HTML id attribute (`js/duplicate-html-id`) | Results no longer shown | Precision tag reduced to "low". The query is no longer run by default. |
 | Unused loop iteration variable (`js/unused-loop-variable`) | Fewer results | This query no longer flags variables in a destructuring array assignment that are not the last variable in the destructed array. |

javascript/ql/src/DOM/TargetBlank.ql

@@ -29,7 +29,7 @@ predicate hasDynamicHrefHostAttributeValue(DOM::ElementDefinition elem) {
     or
     exists(string url | url = attr.getStringValue() |
       // fixed string with templating
-      url.regexpMatch(Templating::getDelimiterMatchingRegexp()) and
+      url.regexpMatch(Templating::getDelimiterMatchingRegexpWithPrefix("[^?#]*")) and
       // ... that does not start with a fixed host or a relative path (common formats)
       not url.regexpMatch("(?i)((https?:)?//)?[-a-z0-9.]*/.*")
     )

javascript/ql/src/semmle/javascript/frameworks/Templating.qll

@@ -36,7 +36,16 @@ module Templating {
    * of the known template delimiters identified by `getADelimiter()`,
    * storing it in its first (and only) capture group.
    */
-  string getDelimiterMatchingRegexp() {
-    result = "(?s).*(" + concat("\\Q" + getADelimiter() + "\\E", "|") + ").*"
+  string getDelimiterMatchingRegexp() { result = getDelimiterMatchingRegexpWithPrefix(".*") }
+
+  /**
+   * Gets a regular expression that matches a string containing one
+   * of the known template delimiters identified by `getADelimiter()`,
+   * storing it in its first (and only) capture group.
+   * Where the string prior to the template delimiter matches the regexp `prefix`.
+   */
+  bindingset[prefix]
+  string getDelimiterMatchingRegexpWithPrefix(string prefix) {
+    result = "(?s)" + prefix + "(" + concat("\\Q" + getADelimiter() + "\\E", "|") + ").*"
   }
 }

javascript/ql/test/query-tests/DOM/TargetBlank/TargetBlank.expected

@@ -1,6 +1,7 @@
 | tst.html:23:1:23:61 | <a>...</> | External links without noopener/noreferrer are a potential security risk. |
 | tst.html:24:1:24:48 | <a>...</> | External links without noopener/noreferrer are a potential security risk. |
 | tst.html:25:1:25:36 | <a>...</> | External links without noopener/noreferrer are a potential security risk. |
+| tst.html:30:1:30:61 | <a>...</> | External links without noopener/noreferrer are a potential security risk. |
 | tst.js:18:1:18:43 | <a href ... ple</a> | External links without noopener/noreferrer are a potential security risk. |
 | tst.js:19:1:19:58 | <a href ... ple</a> | External links without noopener/noreferrer are a potential security risk. |
 | tst.js:20:1:20:51 | <a data ... ple</a> | External links without noopener/noreferrer are a potential security risk. |

javascript/ql/test/query-tests/DOM/TargetBlank/tst.html

@@ -26,5 +26,13 @@ <h1>NOT OK, because of dynamic URL</h1>
   Example
 </a>
 
+<h1>NOT OK: mailto is not fine.</h1>
+<a target="_blank" href="mailto:{{var:mail}}">mail somone</a>
+
+<h1>OK: template elements after # or ? are fine.</h1>
+<a href="file.extension?#[% row.href %]" target="_blank">Example</a>
+<a href="file.extension?[% row.href %]" target="_blank">Example</a>
+<a href="file.extension#[% row.href %]" target="_blank">Example</a>
+
 </body>
 </html>