diff --git a/change-notes/1.20/analysis-javascript.md b/change-notes/1.20/analysis-javascript.md
index d12801d41a4..824e4f09b29 100644
--- 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
index 583ebddb672..368c5cc6d61 100644
--- a/javascript/ql/src/DOM/AmbiguousIdAttribute.ql
+++ b/javascript/ql/src/DOM/AmbiguousIdAttribute.ql
@@ -26,13 +26,8 @@ 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, _)
)
}
@@ -40,8 +35,10 @@ predicate idAt(
* 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
new file mode 100644
index 00000000000..50a88054493
--- /dev/null
+++ b/javascript/ql/test/query-tests/DOM/HTML/jinja2.html
@@ -0,0 +1,9 @@
+
+
+
+{% if foo %}
+foo
+{% else %}
+!foo
+{% endif %}
+