From 1220b50737119b08356ecccdf91ed50ac356cd59 Mon Sep 17 00:00:00 2001
From: Esben Sparre Andreasen <esben@semmle.com>
Date: Tue, 11 Sep 2018 21:37:02 +0200
Subject: [PATCH] JS: whitelist _.bindAll-methods in
 js/unbound-event-handler-receiver

---
 .../UnboundEventHandlerReceiver.ql            | 20 +++++++--
 .../UnboundEventHandlerReceiver.expected      |  6 +--
 .../UnboundEventHandlerReceiver/tst.js        | 45 ++++++++++++-------
 3 files changed, 49 insertions(+), 22 deletions(-)

diff --git a/javascript/ql/src/Expressions/UnboundEventHandlerReceiver.ql b/javascript/ql/src/Expressions/UnboundEventHandlerReceiver.ql
index 63212b69bf1..8875a376c16 100644
--- a/javascript/ql/src/Expressions/UnboundEventHandlerReceiver.ql
+++ b/javascript/ql/src/Expressions/UnboundEventHandlerReceiver.ql
@@ -13,16 +13,30 @@ import javascript
  * Holds if the receiver of `method` is bound in a method of its class.
  */
 private predicate isBoundInMethod(MethodDeclaration method) {
-  exists (DataFlow::ThisNode thiz, MethodDeclaration bindingMethod |
+  exists (DataFlow::ThisNode thiz, MethodDeclaration bindingMethod, string name |
+    name = method.getName() and
     bindingMethod.getDeclaringClass() = method.getDeclaringClass() and
     not bindingMethod.isStatic() and
-    thiz.getBinder().getAstNode() = bindingMethod.getBody() and
+    thiz.getBinder().getAstNode() = bindingMethod.getBody() |
     exists (DataFlow::Node rhs, DataFlow::MethodCallNode bind |
       // this.<methodName> = <expr>.bind(...)
-      thiz.hasPropertyWrite(method.getName(), rhs) and
+      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)
+      )
+    )
   )
 }
 
diff --git a/javascript/ql/test/query-tests/Expressions/UnboundEventHandlerReceiver/UnboundEventHandlerReceiver.expected b/javascript/ql/test/query-tests/Expressions/UnboundEventHandlerReceiver/UnboundEventHandlerReceiver.expected
index d0cba5116dc..d0c16fa6c0e 100644
--- a/javascript/ql/test/query-tests/Expressions/UnboundEventHandlerReceiver/UnboundEventHandlerReceiver.expected
+++ b/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:8:18:8:40 | onClick ... bound1} | The receiver of this event handler call is unbound, `$@` will be `undefined` in the call to $@ | tst.js:35:9:35:12 | this | this | tst.js:34:5:36:5 | unbound ... ;\\n    } | unbound1 |
+| tst.js:9:18:9:40 | onClick ... bound2} | The receiver of this event handler call is unbound, `$@` will be `undefined` in the call to $@ | tst.js:39:15:39:18 | this | this | tst.js:38:5:40:5 | unbound ... ;\\n    } | unbound2 |
+| tst.js:10:18:10:35 | onClick={unbound3} | The receiver of this event handler call is unbound, `$@` will be `undefined` in the call to $@ | tst.js:43:15:43:18 | this | this | tst.js:42:5:44:5 | unbound ... ;\\n    } | unbound3 |
diff --git a/javascript/ql/test/query-tests/Expressions/UnboundEventHandlerReceiver/tst.js b/javascript/ql/test/query-tests/Expressions/UnboundEventHandlerReceiver/tst.js
index 196fcea7820..0d9334a9fd8 100644
--- a/javascript/ql/test/query-tests/Expressions/UnboundEventHandlerReceiver/tst.js
+++ b/javascript/ql/test/query-tests/Expressions/UnboundEventHandlerReceiver/tst.js
@@ -1,6 +1,25 @@
 import React from 'react';
 
 class Component 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>
+    }
+
     constructor(props) {
         super(props);
         this.bound_throughBindInConstructor = this.bound_throughBindInConstructor.bind(this);
@@ -8,6 +27,8 @@ class Component extends React.Component {
         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 +71,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 +79,14 @@ class Component extends React.Component {
         this.setState({ });
     }
 
+    bound_throughBindAllInConstructor1() {
+        this.setState({ });
+    }
+
+    bound_throughBindAllInConstructor2() {
+        this.setState({ });
+    }
+
 }
 
 // semmle-extractor-options: --experimental