JS: Improve support for getters and instance members in API graphs

javascript/ql/lib/semmle/javascript/ApiGraphs.qll

@@ -604,12 +604,36 @@ module API {
           or
           lbl = Label::promisedError() and
           PromiseFlow::storeStep(rhs, pred, Promises::errorProp())
+          or
+          // The return-value of a getter G counts as a definition of property G
+          // (Ordinary methods and properties are handled as PropWrite nodes)
+          exists(string name | lbl = Label::member(name) |
+            rhs = pred.(DataFlow::ObjectLiteralNode).getPropertyGetter(name).getAReturn()
+            or
+            rhs =
+              pred.(DataFlow::ClassNode)
+                  .getStaticMember(name, DataFlow::MemberKind::getter())
+                  .getAReturn()
+          )
+          or
+          // If `new C()` escapes, generate edges to its instance members
+          exists(DataFlow::ClassNode cls, string name |
+            pred = cls.getAClassReference().getAnInstantiation() and
+            lbl = Label::member(name)
+          |
+            rhs = cls.getInstanceMethod(name)
+            or
+            rhs = cls.getInstanceMember(name, DataFlow::MemberKind::getter()).getAReturn()
+          )
         )
         or
         exists(DataFlow::ClassNode cls, string name |
           base = MkClassInstance(cls) and
-          lbl = Label::member(name) and
+          lbl = Label::member(name)
+        |
           rhs = cls.getInstanceMethod(name)
+          or
+          rhs = cls.getInstanceMember(name, DataFlow::MemberKind::getter()).getAReturn()
         )
         or
         exists(DataFlow::FunctionNode f |

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

@@ -589,6 +589,13 @@ module DataFlow {
      * Gets the node where the property write happens in the control flow graph.
      */
     abstract ControlFlowNode getWriteNode();
+
+    /**
+     * If this installs an accessor on an object, as opposed to a regular property,
+     * gets the body of the accessor. `isSetter` is true if installing a setter, and
+     * false is installing a getter.
+     */
+    DataFlow::FunctionNode getInstalledAccessor(boolean isSetter) { none() }
   }
 
   /**
@@ -628,6 +635,17 @@ module DataFlow {
 
     override Node getRhs() { result = valueNode(prop.(ValueProperty).getInit()) }
 
+    override DataFlow::FunctionNode getInstalledAccessor(boolean isSetter) {
+      (
+        prop instanceof PropertySetter and
+        isSetter = true
+        or
+        prop instanceof PropertyGetter and
+        isSetter = false
+      ) and
+      result = valueNode(prop.getInit())
+    }
+
     override ControlFlowNode getWriteNode() { result = prop }
   }
 
@@ -688,6 +706,17 @@ module DataFlow {
       result = valueNode(prop.getInit())
     }
 
+    override DataFlow::FunctionNode getInstalledAccessor(boolean isSetter) {
+      (
+        prop instanceof SetterMethodDefinition and
+        isSetter = true
+        or
+        prop instanceof GetterMethodDefinition and
+        isSetter = false
+      ) and
+      result = valueNode(prop.getInit())
+    }
+
     override ControlFlowNode getWriteNode() { result = prop }
   }
 
@@ -711,6 +740,17 @@ module DataFlow {
       result = valueNode(prop.getInit())
     }
 
+    override DataFlow::FunctionNode getInstalledAccessor(boolean isSetter) {
+      (
+        prop instanceof SetterMethodDefinition and
+        isSetter = true
+        or
+        prop instanceof GetterMethodDefinition and
+        isSetter = false
+      ) and
+      result = valueNode(prop.getInit())
+    }
+
     override ControlFlowNode getWriteNode() { result = prop }
   }
 

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

@@ -896,17 +896,31 @@ class ClassNode extends DataFlow::SourceNode instanceof ClassNode::Range {
    */
   FunctionNode getAnInstanceMember() { result = super.getAnInstanceMember(_) }
 
+  /**
+   * Gets the static method, getter, or setter declared in this class with the given name and kind.
+   */
+  FunctionNode getStaticMember(string name, MemberKind kind) {
+    result = super.getStaticMember(name, kind)
+  }
+
   /**
    * Gets the static method declared in this class with the given name.
    */
-  FunctionNode getStaticMethod(string name) { result = super.getStaticMethod(name) }
+  FunctionNode getStaticMethod(string name) {
+    result = this.getStaticMember(name, MemberKind::method())
+  }
+
+  /**
+   * Gets a static method, getter, or setter declared in this class with the given kind.
+   */
+  FunctionNode getAStaticMember(MemberKind kind) { result = super.getAStaticMember(kind) }
 
   /**
    * Gets a static method declared in this class.
    *
    * The constructor is not considered a static method.
    */
-  FunctionNode getAStaticMethod() { result = super.getAStaticMethod() }
+  FunctionNode getAStaticMethod() { result = this.getAStaticMember(MemberKind::method()) }
 
   /**
    * Gets a dataflow node that refers to the superclass of this class.
@@ -1108,18 +1122,34 @@ module ClassNode {
     abstract FunctionNode getAnInstanceMember(MemberKind kind);
 
     /**
+     * Gets the static member of this class with the given name and kind.
+     */
+    cached
+    abstract FunctionNode getStaticMember(string name, MemberKind kind);
+
+    /**
+     * DEPRECATED. Override `getStaticMember` instead.
+     *
      * Gets the static method of this class with the given name.
      */
     cached
-    abstract FunctionNode getStaticMethod(string name);
+    deprecated FunctionNode getStaticMethod(string name) { none() }
+
+    /**
+     * Gets a static member of this class of the given kind.
+     */
+    cached
+    abstract FunctionNode getAStaticMember(MemberKind kind);
 
     /**
+     * DEPRECATED. Override `getAStaticMember` instead.
+     *
      * Gets a static method of this class.
      *
      * The constructor is not considered a static method.
      */
     cached
-    abstract FunctionNode getAStaticMethod();
+    deprecated FunctionNode getAStaticMethod() { none() }
 
     /**
      * Gets a dataflow node representing a class to be used as the super-class
@@ -1175,23 +1205,27 @@ module ClassNode {
       result = this.getConstructor().getReceiver().getAPropertySource()
     }
 
-    override FunctionNode getStaticMethod(string name) {
+    override FunctionNode getStaticMember(string name, MemberKind kind) {
       exists(MethodDeclaration method |
         method = astNode.getMethod(name) and
         method.isStatic() and
+        kind = MemberKind::of(method) and
         result = method.getBody().flow()
       )
       or
+      kind.isMethod() and
       result = this.getAPropertySource(name)
     }
 
-    override FunctionNode getAStaticMethod() {
+    override FunctionNode getAStaticMember(MemberKind kind) {
       exists(MethodDeclaration method |
         method = astNode.getAMethod() and
         method.isStatic() and
+        kind = MemberKind::of(method) and
         result = method.getBody().flow()
       )
       or
+      kind.isMethod() and
       result = this.getAPropertySource()
     }
 
@@ -1289,9 +1323,15 @@ module ClassNode {
       )
     }
 
-    override FunctionNode getStaticMethod(string name) { result = this.getAPropertySource(name) }
+    override FunctionNode getStaticMember(string name, MemberKind kind) {
+      kind.isMethod() and
+      result = this.getAPropertySource(name)
+    }
 
-    override FunctionNode getAStaticMethod() { result = this.getAPropertySource() }
+    override FunctionNode getAStaticMember(MemberKind kind) {
+      kind.isMethod() and
+      result = this.getAPropertySource()
+    }
 
     /**
      * Gets a reference to the prototype of this class.

javascript/ql/lib/semmle/javascript/dataflow/internal/CallGraphs.qll

@@ -189,25 +189,43 @@ module CallGraph {
     )
   }
 
+  /**
+   * Holds if `ref` installs an accessor on an object. Such property writes should not
+   * be considered calls to an accessor.
+   */
+  pragma[nomagic]
+  private predicate isAccessorInstallation(DataFlow::PropWrite write) {
+    exists(write.getInstalledAccessor(_))
+  }
+
   /**
    * Gets a getter or setter invoked as a result of the given property access.
    */
   cached
   DataFlow::FunctionNode getAnAccessorCallee(DataFlow::PropRef ref) {
-    exists(DataFlow::ClassNode cls, string name |
-      ref = cls.getAnInstanceMemberAccess(name) and
-      result = cls.getInstanceMember(name, DataFlow::MemberKind::getter())
-      or
-      ref = getAnInstanceMemberAssignment(cls, name) and
-      result = cls.getInstanceMember(name, DataFlow::MemberKind::setter())
-    )
-    or
-    exists(DataFlow::ObjectLiteralNode object, string name |
-      ref = getAnAllocationSiteRef(object).getAPropertyRead(name) and
-      result = object.getPropertyGetter(name)
+    not isAccessorInstallation(ref) and
+    (
+      exists(DataFlow::ClassNode cls, string name |
+        ref = cls.getAnInstanceMemberAccess(name) and
+        result = cls.getInstanceMember(name, DataFlow::MemberKind::getter())
+        or
+        ref = getAnInstanceMemberAssignment(cls, name) and
+        result = cls.getInstanceMember(name, DataFlow::MemberKind::setter())
+        or
+        ref = cls.getAClassReference().getAPropertyRead(name) and
+        result = cls.getStaticMember(name, DataFlow::MemberKind::getter())
+        or
+        ref = cls.getAClassReference().getAPropertyWrite(name) and
+        result = cls.getStaticMember(name, DataFlow::MemberKind::setter())
+      )
       or
-      ref = getAnAllocationSiteRef(object).getAPropertyWrite(name) and
-      result = object.getPropertySetter(name)
+      exists(DataFlow::ObjectLiteralNode object, string name |
+        ref = getAnAllocationSiteRef(object).getAPropertyRead(name) and
+        result = object.getPropertyGetter(name)
+        or
+        ref = getAnAllocationSiteRef(object).getAPropertyWrite(name) and
+        result = object.getPropertySetter(name)
+      )
     )
   }
 

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

@@ -139,7 +139,7 @@ private module CachedSteps {
    * Holds if `invk` may invoke `f`.
    */
   cached
-  predicate calls(DataFlow::SourceNode invk, Function f) {
+  predicate calls(DataFlow::Node invk, Function f) {
     f = invk.(DataFlow::InvokeNode).getACallee(0)
     or
     f = invk.(DataFlow::PropRef).getAnAccessorCallee().getFunction()

javascript/ql/test/ApiGraphs/accessors/VerifyAssertions.ql

@@ -0,0 +1 @@
+import ApiGraphs.VerifyAssertions

javascript/ql/test/ApiGraphs/accessors/index.js

@@ -0,0 +1,65 @@
+const foo = require('foo');
+
+foo({
+    myMethod(x) { /* use (parameter 0 (member myMethod (parameter 0 (member exports (module foo))))) */
+        console.log(x);
+    }
+});
+
+foo({
+    get myMethod() {
+        return function(x) { /* use (parameter 0 (member myMethod (parameter 0 (member exports (module foo))))) */
+            console.log(x)
+        }
+    }
+});
+
+class C {
+    static myMethod(x) { /* use (parameter 0 (member myMethod (parameter 0 (member exports (module foo))))) */
+        console.log(x);
+    }
+}
+foo(C);
+
+class D {
+    myMethod(x) { /* use (parameter 0 (member myMethod (parameter 0 (member exports (module foo))))) */
+        console.log(x);
+    }
+}
+foo(new D());
+
+class E {
+    get myMethod() {
+        return function(x) { /* use (parameter 0 (member myMethod (parameter 0 (member exports (module foo))))) */
+            console.log(x);
+        }
+    }
+}
+foo(new E());
+
+class F {
+    static get myMethod() {
+        return function(x) { /* use (parameter 0 (member myMethod (parameter 0 (member exports (module foo))))) */
+            console.log(x);
+        }
+    }
+}
+foo(F);
+
+// Cases where the class is instantiated in `foo`:
+
+class G {
+    myMethod2(x) { /* use (parameter 0 (member myMethod2 (instance (parameter 0 (member exports (module foo)))))) */
+        console.log(x);
+    }
+}
+foo(G);
+
+class H {
+    get myMethod2() {
+        return function (x) { /* use (parameter 0 (member myMethod2 (instance (parameter 0 (member exports (module foo)))))) */
+            console.log(x);
+        }
+    }
+}
+foo(H);

javascript/ql/test/library-tests/CallGraphs/AnnotatedTest/Test.expected

@@ -1,5 +1,15 @@
 spuriousCallee
 missingCallee
-| constructor-field.ts:40:5:40:14 | f3.build() | constructor-field.ts:13:3:13:12 | build() {} | -1 |
-| constructor-field.ts:71:1:71:11 | bf3.build() | constructor-field.ts:13:3:13:12 | build() {} | -1 |
+| constructor-field.ts:40:5:40:14 | f3.build() | constructor-field.ts:13:3:13:12 | build() {} | -1 | calls |
+| constructor-field.ts:71:1:71:11 | bf3.build() | constructor-field.ts:13:3:13:12 | build() {} | -1 | calls |
 badAnnotation
+accessorCall
+| accessors.js:12:1:12:5 | obj.f | accessors.js:5:8:5:12 | () {} |
+| accessors.js:15:1:15:5 | obj.f | accessors.js:8:8:8:13 | (x) {} |
+| accessors.js:26:1:26:3 | C.f | accessors.js:19:15:19:19 | () {} |
+| accessors.js:29:1:29:3 | C.f | accessors.js:22:15:22:20 | (x) {} |
+| accessors.js:41:1:41:9 | new D().f | accessors.js:34:8:34:12 | () {} |
+| accessors.js:44:1:44:9 | new D().f | accessors.js:37:8:37:13 | (x) {} |
+| accessors.js:48:1:48:5 | obj.f | accessors.js:5:8:5:12 | () {} |
+| accessors.js:51:1:51:3 | C.f | accessors.js:19:15:19:19 | () {} |
+| accessors.js:54:1:54:9 | new D().f | accessors.js:34:8:34:12 | () {} |