JS: additional whitelisting for js/unbound-event-handler-receiver

change-notes/1.19/analysis-javascript.md

@@ -13,6 +13,7 @@
 | **Query**                      | **Expected impact**        | **Change**                                   |
 |--------------------------------|----------------------------|----------------------------------------------|
 | Regular expression injection | Fewer false-positive results | This rule now identifies calls to `String.prototype.search` with more precision. |
+| Unbound event handler receiver | Fewer false-positive results | This rule now recognizes additional ways class methods can be bound. |
 
 
 ## Changes to QL libraries

javascript/ql/src/Expressions/UnboundEventHandlerReceiver.ql

@@ -10,20 +10,48 @@
 import javascript
 
 /**
- * Holds if the receiver of `method` is bound in a method of its class.
+ * Holds if the receiver of `method` is bound.
  */
 private predicate isBoundInMethod(MethodDeclaration method) {
   exists (DataFlow::ThisNode thiz, MethodDeclaration bindingMethod |
     bindingMethod.getDeclaringClass() = method.getDeclaringClass() and
     not bindingMethod.isStatic() and
-    thiz.getBinder().getAstNode() = bindingMethod.getBody() and
-    exists (DataFlow::Node rhs, DataFlow::MethodCallNode bind |
-      // this.<methodName> = <expr>.bind(...)
-      thiz.hasPropertyWrite(method.getName(), rhs) and
-      bind.flowsTo(rhs) and
-      bind.getMethodName() = "bind"
+    thiz.getBinder().getAstNode() = bindingMethod.getBody() |
+    // require("auto-bind")(this)
+    thiz.flowsTo(DataFlow::moduleImport("auto-bind").getACall().getArgument(0))
+    or
+    exists (string name |
+      name = method.getName() |
+      exists (DataFlow::Node rhs, DataFlow::MethodCallNode bind |
+        // this.<methodName> = <expr>.bind(...)
+        thiz.hasPropertyWrite(name, rhs) and
+        bind.flowsTo(rhs) and
+        bind.getMethodName() = "bind"
+      )
+      or
+      exists (DataFlow::MethodCallNode bindAll |
+        bindAll.getMethodName() = "bindAll" and
+        thiz.flowsTo(bindAll.getArgument(0)) |
+        // _.bindAll(this, <name1>)
+        bindAll.getArgument(1).mayHaveStringValue(name)
+        or
+        // _.bindAll(this, [<name1>, <name2>])
+        exists (DataFlow::ArrayLiteralNode names |
+          names.flowsTo(bindAll.getArgument(1)) and
+          names.getAnElement().mayHaveStringValue(name)
+        )
+      )
     )
   )
+  or
+  exists (Expr decoration, string name |
+    decoration = method.getADecorator().getExpression() and
+    name.regexpMatch("(?i).*(bind|bound).*") |
+    // @autobind
+    decoration.(Identifier).getName() = name or
+    // @action.bound
+    decoration.(PropAccess).getPropertyName() = name
+  )
 }
 
 /**

javascript/ql/test/query-tests/Expressions/UnboundEventHandlerReceiver/UnboundEventHandlerReceiver.expected

@@ -1,3 +1,3 @@
-| tst.js:56:18:56:40 | onClick ... bound1} | The receiver of this event handler call is unbound, `$@` will be `undefined` in the call to $@ | tst.js:14:9:14:12 | this | this | tst.js:13:5:15:5 | unbound ... ;\\n    } | unbound1 |
-| tst.js:57:18:57:40 | onClick ... bound2} | The receiver of this event handler call is unbound, `$@` will be `undefined` in the call to $@ | tst.js:18:15:18:18 | this | this | tst.js:17:5:19:5 | unbound ... ;\\n    } | unbound2 |
-| tst.js:58:18:58:35 | onClick={unbound3} | The receiver of this event handler call is unbound, `$@` will be `undefined` in the call to $@ | tst.js:22:15:22:18 | this | this | tst.js:21:5:23:5 | unbound ... ;\\n    } | unbound3 |
+| tst.js:27:18:27:40 | onClick ... bound1} | The receiver of this event handler call is unbound, `$@` will be `undefined` in the call to $@ | tst.js:56:9:56:12 | this | this | tst.js:55:5:57:5 | unbound ... ;\\n    } | unbound1 |
+| tst.js:28:18:28:40 | onClick ... bound2} | The receiver of this event handler call is unbound, `$@` will be `undefined` in the call to $@ | tst.js:60:15:60:18 | this | this | tst.js:59:5:61:5 | unbound ... ;\\n    } | unbound2 |
+| tst.js:29:18:29:35 | onClick={unbound3} | The receiver of this event handler call is unbound, `$@` will be `undefined` in the call to $@ | tst.js:64:15:64:18 | this | this | tst.js:63:5:65:5 | unbound ... ;\\n    } | unbound3 |

javascript/ql/test/query-tests/Expressions/UnboundEventHandlerReceiver/tst.js

@@ -1,13 +1,55 @@
 import React from 'react';
+import autoBind from 'auto-bind';
+
+class Component0 extends React.Component {
+
+    render() {
+        return <div>
+            <div onClick={this.bound_throughAutoBind}/> // OK
+            </div>
+    }
+
+    constructor(props) {
+        super(props);
+        autoBind(this);
+    }
+
+    bound_throughAutoBind() {
+        this.setState({ });
+    }
+}
+
+class Component1 extends React.Component {
+
+    render() {
+        var unbound3 = this.unbound3;
+        return <div>
+            <div onClick={this.unbound1}/> // NOT OK
+            <div onClick={this.unbound2}/> // NOT OK
+            <div onClick={unbound3}/> // NOT OK
+            <div onClick={this.bound_throughBindInConstructor}/> // OK
+            <div onClick={this.bound_throughDeclaration}/> // OK
+            <div onClick={this.unbound_butNoThis}/> // OK
+            <div onClick={this.unbound_butNoThis2}/> // OK
+            <div onClick={(e) => this.unbound_butInvokedSafely(e)}/> // OK
+            <div onClick={this.bound_throughBindInMethod}/> // OK
+            <div onClick={this.bound_throughNonSyntacticBindInConstructor}/> // OK
+            <div onClick={this.bound_throughBindAllInConstructor1}/> // OK
+            <div onClick={this.bound_throughBindAllInConstructor2}/> // OK
+            <div onClick={this.bound_throughDecorator_autobind}/> // OK
+            <div onClick={this.bound_throughDecorator_actionBound}/> // OK
+            </div>
+    }
 
-class Component extends React.Component {
     constructor(props) {
         super(props);
         this.bound_throughBindInConstructor = this.bound_throughBindInConstructor.bind(this);
         this.bound_throughBizarreBind = foo.bar.bind(baz);
         var cmp = this;
         var bound = (cmp.bound_throughNonSyntacticBindInConstructor.bind(this));
         (cmp).bound_throughNonSyntacticBindInConstructor = bound;
+        _.bindAll(this, 'bound_throughBindAllInConstructor1');
+        _.bindAll(this, ['bound_throughBindAllInConstructor2']);
     }
 
     unbound1() {
@@ -50,22 +92,6 @@ class Component extends React.Component {
         this.setState({ });
     }
 
-    render() {
-        var unbound3 = this.unbound3;
-        return <div>
-            <div onClick={this.unbound1}/> // NOT OK
-            <div onClick={this.unbound2}/> // NOT OK
-            <div onClick={unbound3}/> // NOT OK
-            <div onClick={this.bound_throughBindInConstructor}/> // OK
-            <div onClick={this.bound_throughDeclaration}/> // OK
-            <div onClick={this.unbound_butNoThis}/> // OK
-            <div onClick={this.unbound_butNoThis2}/> // OK
-            <div onClick={(e) => this.unbound_butInvokedSafely(e)}/> // OK
-            <div onClick={this.bound_throughBindInMethod}/> // OK
-            <div onClick={this.bound_throughNonSyntacticBindInConstructor}/> // OK
-            </div>
-    }
-
     componentWillMount() {
         this.bound_throughBindInMethod = this.bound_throughBindInMethod.bind(this);
     }
@@ -74,6 +100,24 @@ class Component extends React.Component {
         this.setState({ });
     }
 
+    bound_throughBindAllInConstructor1() {
+        this.setState({ });
+    }
+
+    bound_throughBindAllInConstructor2() {
+        this.setState({ });
+    }
+
+    @autobind
+    bound_throughDecorator_autobind() {
+        this.setState({ });
+    }
+
+    @action.bound
+    bound_throughDecorator_actionBound() {
+        this.setState({ });
+    }
+
 }
 
 // semmle-extractor-options: --experimental