Merge pull request github#3196 from asger-semmle/js/unnecessary-source-node-range

Approved by esbena

Author: semmle-qlci

javascript/ql/src/semmle/javascript/AMD.qll

@@ -56,10 +56,16 @@ class AmdModuleDefinition extends CallExpr {
    */
   pragma[nomagic]
   DataFlow::SourceNode getFactoryNode() {
-    result.flowsToExpr(getLastArgument()) and
+    result = getFactoryNodeInternal() and
     result instanceof DataFlow::ValueNode
   }
 
+  private DataFlow::Node getFactoryNodeInternal() {
+    // To avoid recursion, this should not depend on `SourceNode`.
+    result = DataFlow::valueNode(getLastArgument()) or
+    result = getFactoryNodeInternal().getAPredecessor()
+  }
+
   /** Gets the expression defining this module. */
   Expr getModuleExpr() {
     exists(DataFlow::Node f | f = getFactoryNode() |
@@ -108,7 +114,7 @@ class AmdModuleDefinition extends CallExpr {
    * Gets the `i`th parameter of the factory function of this module.
    */
   private SimpleParameter getFactoryParameter(int i) {
-    getFactoryNode().(DataFlow::FunctionNode).getParameter(i) = DataFlow::parameterNode(result)
+    getFactoryNodeInternal().asExpr().(Function).getParameter(i) = result
   }
 
   /**

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

@@ -1369,18 +1369,18 @@ module DataFlow {
    */
   private predicate lvalueDefaultFlowStep(Node pred, Node succ) {
     exists(PropertyPattern pattern |
-      pred = valueNode(pattern.getDefault()) and
+      pred = TValueNode(pattern.getDefault()) and
       succ = lvalueNode(pattern.getValuePattern())
     )
     or
     exists(ArrayPattern array, int i |
-      pred = valueNode(array.getDefault(i)) and
+      pred = TValueNode(array.getDefault(i)) and
       succ = lvalueNode(array.getElement(i))
     )
     or
     exists(Parameter param |
-      pred = valueNode(param.getDefault()) and
-      succ = parameterNode(param)
+      pred = TValueNode(param.getDefault()) and
+      parameterNode(succ, param)
     )
   }
 

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

@@ -23,14 +23,33 @@ DataFlow::SourceNode angular() {
   result = DataFlow::moduleImport("angular")
 }
 
-pragma[noopt]
+/**
+ * Holds if `tl` appears to be a top-level using the AngularJS library.
+ *
+ * Should not depend on the `SourceNode` class.
+ */
+pragma[nomagic]
+private predicate isAngularTopLevel(TopLevel tl) {
+  exists(Import imprt |
+    imprt.getTopLevel() = tl and
+    imprt.getImportedPath().getValue() = "angular"
+  )
+  or
+  exists(GlobalVarAccess global |
+    global.getName() = "angular" and
+    global.getTopLevel() = tl
+  )
+}
+
+/**
+ * Holds if `s` is a string in a top-level using the AngularJS library.
+ *
+ * Should not depend on the `SourceNode` class.
+ */
+pragma[nomagic]
 private predicate isAngularString(Expr s) {
-  exists(DataFlow::SourceNode angular, StmtContainer sc, TopLevel tl |
-    angular = angular() and
-    sc = angular.getContainer() and
-    tl = sc.getTopLevel() and
-    tl = s.getTopLevel()
-  |
+  isAngularTopLevel(s.getTopLevel()) and
+  (
     s instanceof StringLiteral or
     s instanceof TemplateLiteral
   )

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

@@ -467,10 +467,6 @@ module HTTP {
     abstract DataFlow::Node getASecretKey();
   }
 
-  private class CookieMiddlewareInstanceAsSourceNode extends DataFlow::SourceNode::Range {
-    CookieMiddlewareInstanceAsSourceNode() { this instanceof CookieMiddlewareInstance }
-  }
-
   /**
    * A key used for signed cookies, viewed as a `CryptographicKey`.
    */

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

@@ -6,9 +6,11 @@ import javascript
 
 module LazyCache {
   /**
+   * DEPRECATED. DO NOT USE.
+   *
    * A lazy-cache object, usually created through an expression of form `require('lazy-cache')(require)`.
    */
-  class LazyCacheObject extends DataFlow::SourceNode {
+  deprecated class LazyCacheObject extends DataFlow::SourceNode {
     LazyCacheObject() {
       // Use `require` directly instead of `moduleImport` to avoid recursion.
       // For the same reason, avoid `Import.getImportedPath`.
@@ -19,13 +21,26 @@ module LazyCache {
     }
   }
 
+  /**
+   * A variable containing a lazy-cache object.
+   */
+  class LazyCacheVariable extends LocalVariable {
+    LazyCacheVariable() {
+      // To avoid recursion, this should not depend on `SourceNode`.
+      exists(Require req |
+        req.getArgument(0).getStringValue() = "lazy-cache" and
+        getAnAssignedExpr().(CallExpr).getCallee() = req
+      )
+    }
+  }
+
   /**
    * An import through `lazy-cache`.
    */
   class LazyCacheImport extends CallExpr, Import {
-    LazyCacheObject cache;
+    LazyCacheVariable cache;
 
-    LazyCacheImport() { this = cache.getACall().asExpr() }
+    LazyCacheImport() { getCallee() = cache.getAnAccess() }
 
     /** Gets the name of the package as it's exposed on the lazy-cache object. */
     string getLocalAlias() {
@@ -39,10 +54,22 @@ module LazyCache {
 
     override PathExpr getImportedPath() { result = getArgument(0) }
 
+    private LazyCacheVariable getVariable() { result = cache }
+
+    pragma[noopt]
     override DataFlow::Node getImportedModuleNode() {
+      this instanceof LazyCacheImport and
       result = this.flow()
       or
-      result = cache.getAPropertyRead(getLocalAlias())
+      exists(LazyCacheVariable variable, Expr base, PropAccess access, string localName |
+        // To avoid recursion, this should not depend on `SourceNode`.
+        variable = getVariable() and
+        base = variable.getAnAccess() and
+        access.getBase() = base and
+        localName = getLocalAlias() and
+        access.getPropertyName() = localName and
+        result = access.flow()
+      )
     }
   }
 

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

@@ -14,10 +14,6 @@ module LodashUnderscore {
     abstract string getName();
   }
 
-  private class MemberAsSourceNode extends DataFlow::SourceNode::Range {
-    MemberAsSourceNode() { this instanceof Member }
-  }
-
   /**
    * An import of `lodash` or `underscore` accessing a given member of that package.
    */

javascript/ql/test/library-tests/RecursionPrevention/SourceNodeFlowsTo.expected

@@ -0,0 +1 @@
+| Success |

javascript/ql/test/library-tests/RecursionPrevention/SourceNodeFlowsTo.ql

@@ -0,0 +1,16 @@
+/**
+ * Test that fails to compile if the ___domain of `SourceNode` depends on `SourceNode.flowsTo` (recursively).
+ *
+ * This tests adds a negative dependency `flowsTo --!--> SourceNode`
+ * so that the undesired edge `SourceNode --> flowsTo` completes a negative cycle.
+ */
+
+import javascript
+
+class BadSourceNode extends DataFlow::SourceNode {
+  BadSourceNode() { this.(DataFlow::PropRead).getPropertyName() = "foo" }
+
+  override predicate flowsTo(DataFlow::Node node) { not node instanceof DataFlow::SourceNode }
+}
+
+select "Success"

javascript/ql/test/library-tests/RecursionPrevention/tst.js

@@ -0,0 +1,2 @@
+// The contents of this file don't matter
+let x = 1;