Merge pull request #718 from xiemaisi/js/ambiguous-id-attr-alert-loc

Approved by asger-semmle

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) {
+predicate idAt(DOM::AttributeDefinition attr, string id, DOM::ElementDefinition root, int line, int column) {
-  exists (DOM::AttributeDefinition attr |
+  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
+ * Holds if attributes `earlier` and `later` are id attributes with the same value in
- * to the same document, and `earlier` appears textually before `later`.
+ * 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"

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.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:3:3:2 | <li>...</> | This element has the same id as $@. | AmbiguousIdAttribute_fragment.html:3:3:3:32 | <li>...</> | 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:17:22:40 | <div id ... ></div> | This element has the same id as $@. | tst.js:22:41:22:64 | <div id ... ></div> | 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 |

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 |

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>;