Merge pull request #8791 from asgerf/js/static-accessors

Approved by erik-krogh

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

@@ -898,17 +898,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.
@@ -1119,18 +1133,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
@@ -1186,23 +1216,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()
     }
 
@@ -1300,9 +1334,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())
+    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 = 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)
-      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/src/change-notes/2022-04-21-static-accessors.md

@@ -0,0 +1,6 @@
+---
+category: minorAnalysis
+---
+* The call graph now deals more precisely with calls to accessors (getters and setters).
+  Previously, calls to static accessors were not resolved, and some method calls were
+  incorrectly seen as calls to an accessor. Both issues have been fixed.

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 | () {} |

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

@@ -25,16 +25,28 @@ class AnnotatedFunction extends Function {
 }
 
 /** A function annotated with `calls:NAME` */
-class AnnotatedCall extends InvokeExpr {
+class AnnotatedCall extends DataFlow::Node {
   string calls;
+  string kind;
 
-  AnnotatedCall() { calls = getAnnotation(this, "calls") }
+  AnnotatedCall() {
+    this instanceof DataFlow::InvokeNode and
+    calls = getAnnotation(this.asExpr(), kind) and
+    kind = "calls"
+    or
+    this instanceof DataFlow::PropRef and
+    calls = getAnnotation(this.getAstNode(), kind) and
+    kind = "callsAccessor"
+  }
 
   string getCallTargetName() { result = calls }
 
-  AnnotatedFunction getAnExpectedCallee() { result.getCalleeName() = getCallTargetName() }
+  AnnotatedFunction getAnExpectedCallee(string kind_) {
+    result.getCalleeName() = getCallTargetName() and
+    kind = kind_
+  }
 
-  int getBoundArgs() { result = getAnnotation(this, "boundArgs").toInt() }
+  int getBoundArgs() { result = getAnnotation(this.getAstNode(), "boundArgs").toInt() }
 
   int getBoundArgsOrMinusOne() {
     result = getBoundArgs()
@@ -42,25 +54,33 @@ class AnnotatedCall extends InvokeExpr {
     not exists(getBoundArgs()) and
     result = -1
   }
+
+  string getKind() { result = kind }
 }
 
 predicate callEdge(AnnotatedCall call, AnnotatedFunction target, int boundArgs) {
-  FlowSteps::calls(call.flow(), target) and boundArgs = -1
+  FlowSteps::calls(call, target) and boundArgs = -1
   or
-  FlowSteps::callsBound(call.flow(), target, boundArgs)
+  FlowSteps::callsBound(call, target, boundArgs)
 }
 
-query predicate spuriousCallee(AnnotatedCall call, AnnotatedFunction target, int boundArgs) {
+query predicate spuriousCallee(
+  AnnotatedCall call, AnnotatedFunction target, int boundArgs, string kind
+) {
   callEdge(call, target, boundArgs) and
+  kind = call.getKind() and
   not (
-    target = call.getAnExpectedCallee() and
+    target = call.getAnExpectedCallee(kind) and
     boundArgs = call.getBoundArgsOrMinusOne()
   )
 }
 
-query predicate missingCallee(AnnotatedCall call, AnnotatedFunction target, int boundArgs) {
+query predicate missingCallee(
+  AnnotatedCall call, AnnotatedFunction target, int boundArgs, string kind
+) {
   not callEdge(call, target, boundArgs) and
-  target = call.getAnExpectedCallee() and
+  kind = call.getKind() and
+  target = call.getAnExpectedCallee(kind) and
   boundArgs = call.getBoundArgsOrMinusOne()
 }
 
@@ -72,3 +92,7 @@ query predicate badAnnotation(string name) {
   not name = any(AnnotatedCall cl).getCallTargetName() and
   name = any(AnnotatedFunction cl).getCalleeName()
 }
+
+query predicate accessorCall(DataFlow::PropRef ref, Function target) {
+  FlowSteps::calls(ref, target)
+}

javascript/ql/test/library-tests/CallGraphs/AnnotatedTest/accessors.js

@@ -0,0 +1,54 @@
+import 'dummy';
+
+let obj = {
+  /** name:obj.f.get */
+  get f() {},
+
+  /** name:obj.f.set */
+  set f(x) {}
+};
+
+/** callsAccessor:obj.f.get */
+obj.f;
+
+/** callsAccessor:obj.f.set */
+obj.f = 1;
+
+class C {
+  /** name:C.f.get */
+  static get f() {}
+
+  /** name:C.f.set */
+  static set f(x) {}
+}
+
+/** callsAccessor:C.f.get */
+C.f;
+
+/** callsAccessor:C.f.set */
+C.f = 1;
+
+
+class D {
+  /** name:D.f.get */
+  get f() {}
+
+  /** name:D.f.set */
+  set f(x) {}
+}
+
+/** callsAccessor:D.f.get */
+new D().f;
+
+/** callsAccessor:D.f.set */
+new D().f = 1;
+
+// Avoid regular calls being seen as calls to the accessor itself
+/** calls:NONE */
+obj.f();
+
+/** calls:NONE */
+C.f();
+
+/** calls:NONE */
+new D().f();

javascript/ql/test/library-tests/ClassNode/tests.expected

@@ -24,6 +24,7 @@ getAReceiverNode
 | tst.js:23:1:23:15 | function D() {} | tst.js:25:13:25:12 | this |
 | tst.js:23:1:23:15 | function D() {} | tst.js:26:13:26:12 | this |
 | tst.js:23:1:23:15 | function D() {} | tst.js:27:4:27:3 | this |
+| tst.js:30:1:34:1 | class S ... x) {}\\n} | tst.js:30:21:30:20 | this |
 getFieldTypeAnnotation
 | fields.ts:1:1:3:1 | class B ... mber;\\n} | baseField | fields.ts:2:16:2:21 | number |
 | fields.ts:5:1:13:1 | class F ... > {};\\n} | x | fields.ts:6:27:6:32 | number |
@@ -53,6 +54,11 @@ instanceMethod
 | tst.js:15:1:15:15 | function B() {} | foo | tst.js:17:19:17:31 | function() {} | B |
 | tst.js:19:1:19:15 | function C() {} | bar | tst.js:21:19:21:31 | function() {} | C |
 | tst.js:23:1:23:15 | function D() {} | m | tst.js:27:4:27:8 | () {} | D |
+staticMember
+| tst.js:3:1:10:1 | class A ... () {}\\n} | staticMethod | method | tst.js:5:22:5:26 | () {} | A |
+| tst.js:30:1:34:1 | class S ... x) {}\\n} | getter | getter | tst.js:32:20:32:24 | () {} | StaticMembers |
+| tst.js:30:1:34:1 | class S ... x) {}\\n} | method | method | tst.js:31:16:31:20 | () {} | StaticMembers |
+| tst.js:30:1:34:1 | class S ... x) {}\\n} | setter | setter | tst.js:33:20:33:25 | (x) {} | StaticMembers |
 superClass
 | fields.ts:5:1:13:1 | class F ... > {};\\n} | fields.ts:1:1:3:1 | class B ... mber;\\n} | Foo | Base |
 | tst.js:13:1:13:21 | class A ... ds A {} | tst.js:3:1:10:1 | class A ... () {}\\n} | A2 | A |

javascript/ql/test/library-tests/ClassNode/tests.ql

@@ -24,6 +24,12 @@ query predicate instanceMethod(
   cls.getInstanceMethod(name) = inst and clsName = cls.getName()
 }
 
+query predicate staticMember(
+  DataFlow::ClassNode cls, string name, string kind, DataFlow::FunctionNode inst, string clsName
+) {
+  cls.getStaticMember(name, kind) = inst and clsName = cls.getName()
+}
+
 query predicate superClass(
   DataFlow::ClassNode cls, DataFlow::ClassNode sup, string clsName, string supName
 ) {

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

@@ -26,3 +26,9 @@ D.prototype = {
   set setter(x) {},
   m() {}
 }
+
+class StaticMembers {
+  static method() {}
+  static get getter() {}
+  static set setter(x) {}
+}