JS: Modernize jQuery attribute defs

Author: asger-semmle

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

@@ -125,7 +125,7 @@ private class JQueryDomElementDefinition extends DOM::ElementDefinition, @callex
 /**
  * An attribute defined using jQuery APIs.
  */
-abstract private class JQueryAttributeDefinition extends DOM::AttributeDefinition { }
+abstract private class JQueryAttributeDefinition extends DOM::AttributeDefinition {}
 
 /**
  * An attribute definition supplied when constructing a DOM element using `$(...)`.
@@ -149,18 +149,20 @@ private class JQueryAttributeDefinitionInElement extends JQueryAttributeDefiniti
   override DOM::ElementDefinition getElement() { result = elt }
 }
 
+/** Gets the `attr` or `prop` string. */
+private string attrOrProp() {
+  result = "attr" or result = "prop"
+}
+
 /**
  * An attribute definition using `elt.attr(name, value)` or `elt.prop(name, value)`
  * where `elt` is a wrapped set.
  */
 private class JQueryAttr2Call extends JQueryAttributeDefinition, @callexpr {
-  JQueryDomElementDefinition elt;
-
   JQueryAttr2Call() {
-    exists(MethodCallExpr mce | this = mce |
-      mce.getReceiver().(DOM::Element).getDefinition() = elt and
-      (mce.getMethodName() = "attr" or mce.getMethodName() = "prop") and
-      mce.getNumArgument() = 2
+    exists(DataFlow::MethodCallNode call | this = call.asExpr() |
+      call = JQuery::objectRef().getAMethodCall(attrOrProp()) and
+      call.getNumArgument() = 2
     )
   }
 
@@ -170,57 +172,63 @@ private class JQueryAttr2Call extends JQueryAttributeDefinition, @callexpr {
     result = DataFlow::valueNode(this.(CallExpr).getArgument(1))
   }
 
-  override DOM::ElementDefinition getElement() { result = elt }
+  override DOM::ElementDefinition getElement() {
+    exists(DataFlow::MethodCallNode call | this = call.asExpr() |
+      result = call.getReceiver().getALocalSource().asExpr().(DOM::Element).getDefinition()
+    )
+  }
 }
 
 /**
  * Holds if `mce` is a call to `elt.attr(attributes)` or `elt.prop(attributes)`.
  */
-private predicate bulkAttributeInit(
-  MethodCallExpr mce, JQueryDomElementDefinition elt, DataFlow::SourceNode attributes
-) {
-  mce.getReceiver().(DOM::Element).getDefinition() = elt and
-  (mce.getMethodName() = "attr" or mce.getMethodName() = "prop") and
+private predicate bulkAttributeInit(DataFlow::MethodCallNode mce, DataFlow::SourceNode attributes) {
+  mce = JQuery::objectRef().getAMethodCall(attrOrProp()) and
   mce.getNumArgument() = 1 and
-  attributes.flowsToExpr(mce.getArgument(0))
+  attributes.flowsTo(mce.getArgument(0))
 }
 
 /**
- * An attribute definition using `elt.attr(attributes)` or `elt.prop(attributes)`
- * where `elt` is a wrapped set and `attributes` is an object of attribute-value pairs
- * to set.
+ * A property stored on an object flowing to `elt.attr(attributes)` or `elt.prop(attributes)`
+ * where `elt` is a wrapped set.
+ *
+ * To avoid spurious combinations of `getName()` and `getValueNode()`,
+ * this class is tied to an individual property write, as opposed to the call itself.
  */
-private class JQueryAttrCall extends JQueryAttributeDefinition, @callexpr {
-  JQueryDomElementDefinition elt;
+private class JQueryBulkAttributeProp extends JQueryAttributeDefinition {
   DataFlow::PropWrite pwn;
 
-  JQueryAttrCall() {
+  JQueryBulkAttributeProp() {
     exists(DataFlow::SourceNode attributes |
-      bulkAttributeInit(this, elt, attributes) and
-      attributes.flowsTo(pwn.getBase())
+      bulkAttributeInit(_, attributes) and
+      pwn = attributes.getAPropertyWrite() and
+      this = pwn.getAstNode()
     )
   }
 
   override string getName() { result = pwn.getPropertyName() }
 
   override DataFlow::Node getValueNode() { result = pwn.getRhs() }
 
-  override DOM::ElementDefinition getElement() { result = elt }
+  override DOM::ElementDefinition getElement() {
+    exists(DataFlow::MethodCallNode mce |
+      bulkAttributeInit(mce, pwn.getBase().getALocalSource()) and
+      result = mce.getReceiver().asExpr().(DOM::Element).getDefinition()
+    )
+  }
 }
 
 /**
  * An attribute definition using `jQuery.attr(elt, name, value)` or `jQuery.prop(elt, name, value)`
  * where `elt` is a wrapped set or a plain DOM element.
  */
 private class JQueryAttr3Call extends JQueryAttributeDefinition, @callexpr {
-  DOM::ElementDefinition elt;
+  MethodCallExpr mce;
 
   JQueryAttr3Call() {
-    exists(MethodCallExpr mce | this = mce |
-      mce = jquery().getAMemberCall(any(string m | m = "attr" or m = "prop")).asExpr() and
-      mce.getArgument(0).(DOM::Element).getDefinition() = elt and
-      mce.getNumArgument() = 3
-    )
+    this = mce and
+    mce = jquery().getAMemberCall(attrOrProp()).asExpr() and
+    mce.getNumArgument() = 3
   }
 
   override string getName() { result = this.(CallExpr).getArgument(1).getStringValue() }
@@ -229,7 +237,9 @@ private class JQueryAttr3Call extends JQueryAttributeDefinition, @callexpr {
     result = DataFlow::valueNode(this.(CallExpr).getArgument(2))
   }
 
-  override DOM::ElementDefinition getElement() { result = elt }
+  override DOM::ElementDefinition getElement() {
+    result = mce.getArgument(0).(DOM::Element).getDefinition()
+  }
 }
 
 /**

javascript/ql/test/library-tests/frameworks/jQuery/AttributeDefinitions.expected

@@ -0,0 +1,4 @@
+| tracking.js:15:5:15:26 | e.attr( ...  'foo') | class | tracking.js:15:21:15:25 | 'foo' |
+| tracking.js:17:7:17:18 | color: 'red' | color | tracking.js:17:14:17:18 | 'red' |
+| tracking.js:18:7:18:18 | size: '12pt' | size | tracking.js:18:13:18:18 | '12pt' |
+| tst.js:3:1:3:44 | $("<a/> ... e.com") | href | tst.js:3:24:3:43 | "https://semmle.com" |

javascript/ql/test/library-tests/frameworks/jQuery/AttributeDefinitions.ql

@@ -0,0 +1,4 @@
+import javascript
+
+from DOM::AttributeDefinition def
+select def, def.getName(), def.getValueNode()

javascript/ql/test/library-tests/frameworks/jQuery/JQueryMethodCall.expected

@@ -1,7 +1,9 @@
 WARNING: Type JQueryMethodCall has been deprecated and may be removed in future (JQueryMethodCall.ql:3,6-22)
 | tracking.js:7:5:7:24 | this.elm.html('foo') | html |
 | tracking.js:14:5:14:17 | e.html('foo') | html |
-| tracking.js:18:7:18:15 | $('#foo') | $ |
+| tracking.js:15:5:15:26 | e.attr( ...  'foo') | attr |
+| tracking.js:16:5:19:6 | e.attr( ... \\n    }) | attr |
+| tracking.js:23:7:23:15 | $('#foo') | $ |
 | ts_global_types.ts:3:5:3:17 | e.html('foo') | html |
 | ts_global_types.ts:7:1:7:9 | $('#foo') | $ |
 | ts_import.ts:5:5:5:17 | e.html('foo') | html |

javascript/ql/test/library-tests/frameworks/jQuery/interpretsArgumentAsHtml.expected

@@ -1,7 +1,7 @@
 WARNING: Type JQueryMethodCall has been deprecated and may be removed in future (interpretsArgumentAsHtml.ql:3,6-22)
 | tracking.js:7:5:7:24 | this.elm.html('foo') | tracking.js:7:19:7:23 | 'foo' |
 | tracking.js:14:5:14:17 | e.html('foo') | tracking.js:14:12:14:16 | 'foo' |
-| tracking.js:18:7:18:15 | $('#foo') | tracking.js:18:9:18:14 | '#foo' |
+| tracking.js:23:7:23:15 | $('#foo') | tracking.js:23:9:23:14 | '#foo' |
 | ts_global_types.ts:3:5:3:17 | e.html('foo') | ts_global_types.ts:3:12:3:16 | 'foo' |
 | ts_global_types.ts:7:1:7:9 | $('#foo') | ts_global_types.ts:7:3:7:8 | '#foo' |
 | ts_import.ts:5:5:5:17 | e.html('foo') | ts_import.ts:5:12:5:16 | 'foo' |

javascript/ql/test/library-tests/frameworks/jQuery/tracking.js

@@ -12,6 +12,11 @@ class C {
    */
   doSomethingWithTypes(e) {
     e.html('foo');
+    e.attr('class', 'foo');
+    e.attr({
+      color: 'red',
+      size: '12pt',
+    });
   }
 }