JavaScript: Be more conservative about templates in AmbiguousIdAttribute.

Max Schaefer · Max Schaefer · commit 25f95d9fb1e5 · 2019-02-12T16:31:01.000Z
Previously, we only excluded attributes where the value of the attribute itself suggests templating happening. Now we exclude all attributes in documents where _any_ attribute value suggests templating.
diff --git a/change-notes/1.20/analysis-javascript.md b/change-notes/1.20/analysis-javascript.md
@@ -33,6 +33,7 @@
 
 | **Query**                                  | **Expected impact**          | **Change**                                                                   |
 |--------------------------------------------|------------------------------|------------------------------------------------------------------------------|
+| Ambiguous HTML id attribute                | Fewer false-positive results | This rule now treats templates more conservatively. |
 | Client-side cross-site scripting           | More true-positive results, fewer false-positive results. | This rule now recognizes WinJS functions that are vulnerable to HTML injection. It no longer flags certain safe uses of jQuery, and recognizes custom sanitizers. |
 | Hard-coded credentials                     | Fewer false-positive results | This rule no longer flag the empty string as a hardcoded username. |
 | Insecure randomness | More results | This rule now flags insecure uses of `crypto.pseudoRandomBytes`. |
diff --git a/javascript/ql/src/DOM/AmbiguousIdAttribute.ql b/javascript/ql/src/DOM/AmbiguousIdAttribute.ql
@@ -26,22 +26,19 @@ predicate idAt(
     id = attr.getStringValue() and
     root = elt.getRoot() and
     elt.getLocation().hasLocationInfo(_, line, column, _, _) and
-    not (
-      // exclude invalid ids (reported by another query)
-      DOM::isInvalidHtmlIdAttributeValue(attr, _)
-      or
-      // exclude attribute values that look like they might be templated
-      attr.mayHaveTemplateValue()
-    )
+    // exclude invalid ids (reported by another query)
+    not DOM::isInvalidHtmlIdAttributeValue(attr, _)
   )
 }
 
 /**
  * Holds if attributes `earlier` and `later` are id attributes with the same value in
  * the same document, and `earlier` appears textually before `later`.
  */
-predicate sameId(DOM::AttributeDefinition earlier, DOM::AttributeDefinition later) {
-  exists(string id, DOM::ElementDefinition root, int l1, int c1, int l2, int c2 |
+predicate sameId(
+  DOM::ElementDefinition root, DOM::AttributeDefinition earlier, DOM::AttributeDefinition later
+) {
+  exists(string id, int l1, int c1, int l2, int c2 |
     idAt(earlier, id, root, l1, c1) and idAt(later, id, root, l2, c2)
   |
     l1 < l2
@@ -50,6 +47,21 @@ predicate sameId(DOM::AttributeDefinition earlier, DOM::AttributeDefinition late
   )
 }
 
-from DOM::AttributeDefinition earlier, DOM::AttributeDefinition later
-where sameId(earlier, later) and not sameId(_, earlier)
+/**
+ * Holds if any attribute value in `root` looks like it is templated.
+ */
+predicate mayContainTemplates(DOM::ElementDefinition root) {
+  exists(DOM::AttributeDefinition attr |
+    attr.mayHaveTemplateValue() and
+    root = attr.getElement().getRoot()
+  )
+}
+
+from DOM::ElementDefinition root, DOM::AttributeDefinition earlier, DOM::AttributeDefinition later
+where
+  sameId(root, earlier, later) and
+  // only flag the first ambiguity if there are many
+  not sameId(root, _, earlier) and
+  // exclude templates
+  not mayContainTemplates(root)
 select earlier, "This element has the same id as $@.", later, "another element"
diff --git a/javascript/ql/test/query-tests/DOM/HTML/jinja2.html b/javascript/ql/test/query-tests/DOM/HTML/jinja2.html
@@ -0,0 +1,9 @@
+<html>
+<head><title></title></head>
+<body>
+{% if foo %}
+<a href="{{url}}" id="unique">foo</a>
+{% else %}
+<a href="{{url}}" id="unique">!foo</div>
+{% endif %}
+</body>
