From 0a2df6c00d53224ef78b97d7bb655467c18ee7f4 Mon Sep 17 00:00:00 2001
From: Max Schaefer <max@semmle.com>
Date: Wed, 2 Jan 2019 11:44:02 +0000
Subject: [PATCH] JavaScript: Highlight id attribute (not entire element) in
 `AmbiguousIdAttribute`.

---
 javascript/ql/src/DOM/AmbiguousIdAttribute.ql    | 16 ++++++++--------
 .../DOM/HTML/AmbiguousIdAttribute.expected       |  6 +++---
 .../DOM/HTML/DuplicateAttributes.expected        |  1 +
 javascript/ql/test/query-tests/DOM/HTML/tst.js   |  3 +++
 4 files changed, 15 insertions(+), 11 deletions(-)

diff --git a/javascript/ql/src/DOM/AmbiguousIdAttribute.ql b/javascript/ql/src/DOM/AmbiguousIdAttribute.ql
index f0304515731c..502f2ffc0417 100644
--- a/javascript/ql/src/DOM/AmbiguousIdAttribute.ql
+++ b/javascript/ql/src/DOM/AmbiguousIdAttribute.ql
@@ -14,13 +14,13 @@
 import javascript
 
 /**
- * Holds if `elt` defines a DOM element with the given `id`
+ * Holds if `attr` is an id attribute with value `id` of a DOM element
  * under document `root` at the given `line` and `column`.
  *
  * Furthermore, the id is required to be valid, and not look like a template.
  */
-predicate elementAt(DOM::ElementDefinition elt, string id, DOM::ElementDefinition root, int line, int column) {
-  exists (DOM::AttributeDefinition attr |
+predicate idAt(DOM::AttributeDefinition attr, string id, DOM::ElementDefinition root, int line, int column) {
+  exists (DOM::ElementDefinition elt |
     attr = elt.getAttributeByName("id") |
     id = attr.getStringValue() and
     root = elt.getRoot() and
@@ -35,17 +35,17 @@ predicate elementAt(DOM::ElementDefinition elt, string id, DOM::ElementDefinitio
 }
 
 /**
- * Holds if elements `earlier` and `later` have the same id and belong
- * to the same document, and `earlier` appears textually before `later`.
+ * 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::ElementDefinition earlier, DOM::ElementDefinition later) {
+predicate sameId(DOM::AttributeDefinition earlier, DOM::AttributeDefinition later) {
   exists (string id, DOM::ElementDefinition root, int l1, int c1, int l2, int c2 |
-    elementAt(earlier, id, root, l1, c1) and elementAt(later, id, root, l2, c2) |
+    idAt(earlier, id, root, l1, c1) and idAt(later, id, root, l2, c2) |
     l1 < l2 or
     l1 = l2 and c1 < c2
   )
 }
 
-from DOM::ElementDefinition earlier, DOM::ElementDefinition later
+from DOM::AttributeDefinition earlier, DOM::AttributeDefinition later
 where sameId(earlier, later) and not sameId(_, earlier)
 select earlier, "This element has the same id as $@.", later, "another element"
diff --git a/javascript/ql/test/query-tests/DOM/HTML/AmbiguousIdAttribute.expected b/javascript/ql/test/query-tests/DOM/HTML/AmbiguousIdAttribute.expected
index a5e6ba74bca1..c013ea098297 100644
--- a/javascript/ql/test/query-tests/DOM/HTML/AmbiguousIdAttribute.expected
+++ b/javascript/ql/test/query-tests/DOM/HTML/AmbiguousIdAttribute.expected
@@ -1,3 +1,3 @@
-| AmbiguousIdAttribute.html:4:1:4:29 | <li>...</> | This element has the same id as $@. | AmbiguousIdAttribute.html:5:1:5:30 | <li>...</> | another element |
-| AmbiguousIdAttribute_fragment.html:2:3:3:2 | <li>...</> | This element has the same id as $@. | AmbiguousIdAttribute_fragment.html:3:3:3:32 | <li>...</> | another element |
-| tst.js:22:17:22:40 | <div id ... ></div> | This element has the same id as $@. | tst.js:22:41:22:64 | <div id ... ></div> | another element |
+| AmbiguousIdAttribute.html:4:5:4:14 | id=first | This element has the same id as $@. | AmbiguousIdAttribute.html:5:5:5:14 | id=first | another element |
+| AmbiguousIdAttribute_fragment.html:2:7:2:16 | id=first | This element has the same id as $@. | AmbiguousIdAttribute_fragment.html:3:7:3:16 | id=first | another element |
+| tst.js:22:22:22:33 | id="theDiff" | This element has the same id as $@. | tst.js:22:46:22:57 | id="theDiff" | another element |
diff --git a/javascript/ql/test/query-tests/DOM/HTML/DuplicateAttributes.expected b/javascript/ql/test/query-tests/DOM/HTML/DuplicateAttributes.expected
index 0f55ca89869e..1590c20f477f 100644
--- a/javascript/ql/test/query-tests/DOM/HTML/DuplicateAttributes.expected
+++ b/javascript/ql/test/query-tests/DOM/HTML/DuplicateAttributes.expected
@@ -1,2 +1,3 @@
 | DuplicateAttributes.html:1:4:1:28 | href=https://semmle.com | This attribute is duplicated $@. | DuplicateAttributes.html:1:30:1:54 | href=https://semmle.com | here |
 | tst.js:9:4:9:28 | href="h ... le.com" | This attribute is duplicated $@. | tst.js:9:30:9:54 | href="h ... le.com" | here |
+| tst.js:25:17:25:28 | id="theDiff" | This attribute is duplicated $@. | tst.js:25:30:25:41 | id="theDiff" | here |
diff --git a/javascript/ql/test/query-tests/DOM/HTML/tst.js b/javascript/ql/test/query-tests/DOM/HTML/tst.js
index a021cd9ce492..df60053b8b93 100644
--- a/javascript/ql/test/query-tests/DOM/HTML/tst.js
+++ b/javascript/ql/test/query-tests/DOM/HTML/tst.js
@@ -20,3 +20,6 @@ var div2 = <div id="theDiff"></div>;
 
 // not OK
 var div3 = <div><div id="theDiff"></div><div id="theDiff"></div></div>;
+
+// not OK
+var div4 = <div id="theDiff" id="theDiff"></div>;