JS: Ensure accessors do not appear to be calls

Author: asgerf

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/internal/CallGraphs.qll

@@ -189,31 +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
-      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
-    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/test/library-tests/CallGraphs/AnnotatedTest/Test.expected

@@ -4,12 +4,8 @@ missingCallee
 | constructor-field.ts:71:1:71:11 | bf3.build() | constructor-field.ts:13:3:13:12 | build() {} | -1 | calls |
 badAnnotation
 accessorCall
-| accessors.js:5:3:5:12 | get f() {} | accessors.js:8:8:8:13 | (x) {} |
-| accessors.js:8:3:8:13 | set f(x) {} | accessors.js:8:8:8:13 | (x) {} |
 | 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:19:3:19:19 | static get f() {} | accessors.js:22:15:22:20 | (x) {} |
-| accessors.js:22:3:22:20 | static set f(x) {} | accessors.js:22:15:22:20 | (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 | () {} |