Merge pull request github#2796 from asger-semmle/js/partial-invoke-receiver

Approved by esbena

Author: semmle-qlci

javascript/ql/src/semmle/javascript/Closure.qll

@@ -267,6 +267,9 @@ module Closure {
       result = this
     }
 
-    override DataFlow::Node getBoundReceiver() { result = getArgument(1) }
+    override DataFlow::Node getBoundReceiver(DataFlow::Node callback) {
+      callback = getArgument(0) and
+      result = getArgument(1)
+    }
   }
 }

javascript/ql/src/semmle/javascript/StandardLibrary.qll

@@ -46,33 +46,26 @@ class DirectEval extends CallExpr {
 }
 
 /**
- * Flow analysis for `this` expressions inside a function that is called with
- * `Array.prototype.map` or a similar Array function that binds `this`.
- *
- * However, since the function could be invoked in another way, we additionally
- * still infer the ordinary abstract value.
+ * Models `Array.prototype.map` and friends as partial invocations that pass their second
+ * argument as the receiver to the callback.
  */
-private class AnalyzedThisInArrayIterationFunction extends AnalyzedNode, DataFlow::ThisNode {
-  AnalyzedNode thisSource;
-
-  AnalyzedThisInArrayIterationFunction() {
-    exists(DataFlow::MethodCallNode bindingCall, string name |
+private class ArrayIterationCallbackAsPartialInvoke extends DataFlow::PartialInvokeNode::Range, DataFlow::MethodCallNode {
+  ArrayIterationCallbackAsPartialInvoke() {
+    getNumArgument() = 2 and
+    // Filter out library methods named 'forEach' etc
+    not DataFlow::moduleImport(_).flowsTo(getReceiver()) and
+    exists(string name | name = getMethodName() |
       name = "filter" or
       name = "forEach" or
       name = "map" or
       name = "some" or
       name = "every"
-    |
-      name = bindingCall.getMethodName() and
-      2 = bindingCall.getNumArgument() and
-      getBinder() = bindingCall.getCallback(0) and
-      thisSource = bindingCall.getArgument(1)
     )
   }
 
-  override AbstractValue getALocalValue() {
-    result = thisSource.getALocalValue() or
-    result = AnalyzedNode.super.getALocalValue()
+  override DataFlow::Node getBoundReceiver(DataFlow::Node callback) {
+    callback = getArgument(0) and
+    result = getArgument(1)
   }
 }
 

javascript/ql/src/semmle/javascript/dataflow/Configuration.qll

@@ -1311,6 +1311,9 @@ class MidPathNode extends PathNode, MkMidNode {
     or
     // Skip the exceptional return on functions, as this highlights the entire function.
     nd = any(DataFlow::FunctionNode f).getExceptionalReturn()
+    or
+    // Skip the synthetic 'this' node, as a ThisExpr will be the next node anyway
+    nd = DataFlow::thisNode(_)
   }
 }
 

javascript/ql/src/semmle/javascript/dataflow/Nodes.qll

@@ -1199,6 +1199,13 @@ class PartialInvokeNode extends DataFlow::Node {
 
   PartialInvokeNode() { this = range }
 
+  /** Gets a node holding a callback invoked by this partial invocation node. */
+  DataFlow::Node getACallbackNode() {
+    isPartialArgument(result, _, _)
+    or
+    exists(getBoundReceiver(result))
+  }
+
   /**
    * Holds if `argument` is passed as argument `index` to the function in `callback`.
    */
@@ -1216,7 +1223,12 @@ class PartialInvokeNode extends DataFlow::Node {
   /**
    * Gets the node holding the receiver to be passed to the bound function, if specified.
    */
-  DataFlow::Node getBoundReceiver() { result = range.getBoundReceiver() }
+  DataFlow::Node getBoundReceiver() { result = range.getBoundReceiver(_) }
+
+  /**
+   * Gets the node holding the receiver to be passed to the bound function, if specified.
+   */
+  DataFlow::Node getBoundReceiver(DataFlow::Node callback) { result = range.getBoundReceiver(callback) }
 }
 
 module PartialInvokeNode {
@@ -1235,9 +1247,17 @@ module PartialInvokeNode {
     DataFlow::SourceNode getBoundFunction(DataFlow::Node callback, int boundArgs) { none() }
 
     /**
+     * DEPRECATED. Use the one-argument version of `getBoundReceiver` instead.
+     *
      * Gets the node holding the receiver to be passed to the bound function, if specified.
      */
+    deprecated
     DataFlow::Node getBoundReceiver() { none() }
+
+    /**
+     * Gets the node holding the receiver to be passed to `callback`.
+     */
+    DataFlow::Node getBoundReceiver(DataFlow::Node callback) { none() }
   }
 
   /**
@@ -1264,7 +1284,8 @@ module PartialInvokeNode {
       result = this
     }
 
-    override DataFlow::Node getBoundReceiver() {
+    override DataFlow::Node getBoundReceiver(DataFlow::Node callback) {
+      callback = getReceiver() and
       result = getArgument(0)
     }
   }
@@ -1309,6 +1330,22 @@ module PartialInvokeNode {
       result = this
     }
   }
+
+  /**
+   * A call to `for-in` or `for-own`, passing the context parameter to the target function.
+   */
+  class ForOwnInPartialCall extends PartialInvokeNode::Range, DataFlow::CallNode {
+    ForOwnInPartialCall() {
+      exists(string name | name = "for-in" or name = "for-own" |
+        this = moduleImport(name).getACall()
+      )
+    }
+
+    override DataFlow::Node getBoundReceiver(DataFlow::Node callback) {
+      callback = getArgument(1) and
+      result = getArgument(2)
+    }
+  }
 }
 
 /**

javascript/ql/src/semmle/javascript/dataflow/internal/FlowSteps.qll

@@ -99,7 +99,7 @@ private module CachedSteps {
   private predicate partiallyCalls(
     DataFlow::PartialInvokeNode invk, DataFlow::AnalyzedNode callback, Function f
   ) {
-    invk.isPartialArgument(callback, _, _) and
+    callback = invk.getACallbackNode() and
     exists(AbstractFunction callee | callee = callback.getAValue() |
       if callback.getAValue().isIndefinite("global")
       then f = callee.getFunction() and f.getFile() = invk.getFile()
@@ -135,6 +135,12 @@ private module CachedSteps {
       not p.isRestParameter() and
       parm = DataFlow::parameterNode(p)
     )
+    or
+    exists(DataFlow::Node callback |
+      arg = invk.(DataFlow::PartialInvokeNode).getBoundReceiver(callback) and
+      partiallyCalls(invk, callback, f) and
+      parm = DataFlow::thisNode(f)
+    )
   }
 
   /**

javascript/ql/src/semmle/javascript/dataflow/internal/InterProceduralTypeInference.qll

@@ -274,3 +274,24 @@ private class TypeInferredMethodWithAnalyzedReturnFlow extends CallWithNonLocalA
 
   override AnalyzedFunction getACallee() { result = fun }
 }
+
+/**
+ * Propagates receivers into locally defined callbacks of partial invocations.
+ */
+private class AnalyzedThisInPartialInvokeCallback extends AnalyzedNode, DataFlow::ThisNode {
+  DataFlow::PartialInvokeNode call;
+  DataFlow::Node receiver;
+
+  AnalyzedThisInPartialInvokeCallback() {
+    exists(DataFlow::Node callbackArg |
+      receiver = call.getBoundReceiver(callbackArg) and
+      getBinder().flowsTo(callbackArg)
+    )
+  }
+
+  override AbstractValue getALocalValue() {
+    result = receiver.analyze().getALocalValue()
+    or
+    result = AnalyzedNode.super.getALocalValue()
+  }
+}

javascript/ql/src/semmle/javascript/frameworks/AngularJS/AngularJSCore.qll

@@ -1097,5 +1097,8 @@ private class BindCall extends DataFlow::PartialInvokeNode::Range, DataFlow::Cal
     result = this
   }
 
-  override DataFlow::Node getBoundReceiver() { result = getArgument(0) }
+  override DataFlow::Node getBoundReceiver(DataFlow::Node callback) {
+    callback = getArgument(1) and
+    result = getArgument(0)
+  }
 }

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

@@ -407,112 +407,109 @@ module LodashUnderscore {
  * However, since the function could be invoked in another way, we additionally
  * still infer the ordinary abstract value.
  */
-private class AnalyzedThisInBoundCallback extends AnalyzedNode, DataFlow::ThisNode {
-  AnalyzedNode thisSource;
+private class LodashCallbackAsPartialInvoke extends DataFlow::PartialInvokeNode::Range,
+  DataFlow::CallNode {
+  int callbackIndex;
+  int contextIndex;
 
-  AnalyzedThisInBoundCallback() {
-    exists(
-      DataFlow::CallNode bindingCall, string binderName, int callbackIndex, int contextIndex,
-      int argumentCount
-    |
-      bindingCall = LodashUnderscore::member(binderName).getACall() and
-      bindingCall.getNumArgument() = argumentCount and
-      getBinder() = bindingCall.getCallback(callbackIndex) and
-      thisSource = bindingCall.getArgument(contextIndex)
+  LodashCallbackAsPartialInvoke() {
+    exists(string name, int argumentCount |
+      this = LodashUnderscore::member(name).getACall() and
+      getNumArgument() = argumentCount
     |
       (
-        binderName = "bind" or
-        binderName = "callback" or
-        binderName = "iteratee"
+        name = "bind" or
+        name = "callback" or
+        name = "iteratee"
       ) and
       callbackIndex = 0 and
       contextIndex = 1 and
       argumentCount = 2
       or
       (
-        binderName = "all" or
-        binderName = "any" or
-        binderName = "collect" or
-        binderName = "countBy" or
-        binderName = "detect" or
-        binderName = "dropRightWhile" or
-        binderName = "dropWhile" or
-        binderName = "each" or
-        binderName = "eachRight" or
-        binderName = "every" or
-        binderName = "filter" or
-        binderName = "find" or
-        binderName = "findIndex" or
-        binderName = "findKey" or
-        binderName = "findLast" or
-        binderName = "findLastIndex" or
-        binderName = "findLastKey" or
-        binderName = "forEach" or
-        binderName = "forEachRight" or
-        binderName = "forIn" or
-        binderName = "forInRight" or
-        binderName = "groupBy" or
-        binderName = "indexBy" or
-        binderName = "map" or
-        binderName = "mapKeys" or
-        binderName = "mapValues" or
-        binderName = "max" or
-        binderName = "min" or
-        binderName = "omit" or
-        binderName = "partition" or
-        binderName = "pick" or
-        binderName = "reject" or
-        binderName = "remove" or
-        binderName = "select" or
-        binderName = "some" or
-        binderName = "sortBy" or
-        binderName = "sum" or
-        binderName = "takeRightWhile" or
-        binderName = "takeWhile" or
-        binderName = "tap" or
-        binderName = "thru" or
-        binderName = "times" or
-        binderName = "unzipWith" or
-        binderName = "zipWith"
+        name = "all" or
+        name = "any" or
+        name = "collect" or
+        name = "countBy" or
+        name = "detect" or
+        name = "dropRightWhile" or
+        name = "dropWhile" or
+        name = "each" or
+        name = "eachRight" or
+        name = "every" or
+        name = "filter" or
+        name = "find" or
+        name = "findIndex" or
+        name = "findKey" or
+        name = "findLast" or
+        name = "findLastIndex" or
+        name = "findLastKey" or
+        name = "forEach" or
+        name = "forEachRight" or
+        name = "forIn" or
+        name = "forInRight" or
+        name = "groupBy" or
+        name = "indexBy" or
+        name = "map" or
+        name = "mapKeys" or
+        name = "mapValues" or
+        name = "max" or
+        name = "min" or
+        name = "omit" or
+        name = "partition" or
+        name = "pick" or
+        name = "reject" or
+        name = "remove" or
+        name = "select" or
+        name = "some" or
+        name = "sortBy" or
+        name = "sum" or
+        name = "takeRightWhile" or
+        name = "takeWhile" or
+        name = "tap" or
+        name = "thru" or
+        name = "times" or
+        name = "unzipWith" or
+        name = "zipWith"
       ) and
       callbackIndex = 1 and
       contextIndex = 2 and
       argumentCount = 3
       or
       (
-        binderName = "foldl" or
-        binderName = "foldr" or
-        binderName = "inject" or
-        binderName = "reduce" or
-        binderName = "reduceRight" or
-        binderName = "transform"
+        name = "foldl" or
+        name = "foldr" or
+        name = "inject" or
+        name = "reduce" or
+        name = "reduceRight" or
+        name = "transform"
       ) and
       callbackIndex = 1 and
       contextIndex = 3 and
       argumentCount = 4
       or
       (
-        binderName = "sortedlastIndex"
+        name = "sortedlastIndex"
         or
-        binderName = "assign"
+        name = "assign"
         or
-        binderName = "eq"
+        name = "eq"
         or
-        binderName = "extend"
+        name = "extend"
         or
-        binderName = "merge"
+        name = "merge"
         or
-        binderName = "sortedIndex" and
-        binderName = "uniq"
+        name = "sortedIndex" and
+        name = "uniq"
       ) and
       callbackIndex = 2 and
       contextIndex = 3 and
       argumentCount = 4
     )
   }
 
-  override AbstractValue getALocalValue() {
-    result = thisSource.getALocalValue() or
-    result = AnalyzedNode.super.getALocalValue()
+  override DataFlow::Node getBoundReceiver(DataFlow::Node callback) {
+    callback = getArgument(callbackIndex) and
+    result = getArgument(contextIndex)
   }
 }