JS: introduce CallToObjectDefineProperty::getAPropertyAttribute

Author: Esben Sparre Andreasen

javascript/ql/src/Declarations/DeadStoreOfProperty.ql

@@ -157,6 +157,11 @@ where
     or
     // exclude result from accessor declarations
     assign1.getWriteNode() instanceof AccessorMethodDeclaration
+  ) and
+  // exclude results from non-value definitions from `Object.defineProperty`
+  (
+    assign1 instanceof CallToObjectDefineProperty implies
+    assign1.(CallToObjectDefineProperty).getAPropertyAttribute().getPropertyName() = "value"
   )
 select assign1.getWriteNode(),
   "This write to property '" + name + "' is useless, since $@ always overrides it.",

javascript/ql/src/Expressions/ExprHasNoEffect.ql

@@ -35,13 +35,20 @@ predicate isDeclaration(Expr e) {
  * Holds if there exists a getter for a property called `name` anywhere in the program.
  */
 predicate isGetterProperty(string name) {
-  // there is a call of the form `Object.defineProperty(..., name, { get: ..., ... })`
-  // or `Object.defineProperty(..., name, <something that's not an object literal>)`
+  // there is a call of the form `Object.defineProperty(..., name, descriptor)` ...
   exists(CallToObjectDefineProperty defProp |
-    name = defProp.getPropertyName() and
-    exists(Expr descriptor | descriptor = defProp.getPropertyDescriptor().asExpr() |
-      exists(descriptor.(ObjectExpr).getPropertyByName("get")) or
-      not descriptor instanceof ObjectExpr
+    name = defProp.getPropertyName() |
+    // ... where `descriptor` defines a getter
+    defProp.getAPropertyAttribute().getPropertyName() = "get" or
+    // ... where `descriptor` may define a getter
+    exists (DataFlow::SourceNode descriptor |
+      descriptor.flowsTo(defProp.getPropertyDescriptor()) |
+      descriptor.isIncomplete(_) or
+      // minimal escape analysis for the descriptor
+      exists (DataFlow::InvokeNode invk |
+        not invk = defProp and
+        descriptor.flowsTo(invk.getAnArgument())
+      )
     )
   )
   or

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

@@ -21,6 +21,15 @@ class CallToObjectDefineProperty extends DataFlow::MethodCallNode {
 
   /** Gets the data flow node denoting the descriptor of the property being defined. */
   DataFlow::Node getPropertyDescriptor() { result = getArgument(2) }
+
+  /** Gets a data flow node defining a descriptor attribute of the property being defined. */
+  DataFlow::PropWrite getAPropertyAttribute() {
+    exists (DataFlow::SourceNode descriptor |
+      descriptor.flowsTo(getPropertyDescriptor()) and
+      result = descriptor.getAPropertyWrite()
+    )
+  }
+
 }
 
 /**

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

@@ -496,10 +496,7 @@ module DataFlow {
     override string getPropertyName() { result = odp.getPropertyName() }
 
     override Node getRhs() {
-      exists(ObjectLiteralNode propdesc |
-        propdesc.flowsTo(odp.getPropertyDescriptor()) and
-        propdesc.hasPropertyWrite("value", result)
-      )
+      odp.getAPropertyAttribute().writes(_, "value", result)
     }
 
     override ControlFlowNode getWriteNode() { result = odp.getAstNode() }

javascript/ql/test/query-tests/Declarations/DeadStoreOfProperty/DeadStoreOfProperty.expected

@@ -12,6 +12,5 @@
 | tst.js:76:5:76:34 | o.pure1 ... te = 42 | This write to property 'pure16_simpleAliasWrite' is useless, since $@ always overrides it. | tst.js:77:5:77:36 | o16.pur ... te = 42 | another property write |
 | tst.js:95:5:95:17 | o.pure18 = 42 | This write to property 'pure18' is useless, since $@ always overrides it. | tst.js:96:5:96:17 | o.pure18 = 42 | another property write |
 | tst.js:96:5:96:17 | o.pure18 = 42 | This write to property 'pure18' is useless, since $@ always overrides it. | tst.js:97:5:97:17 | o.pure18 = 42 | another property write |
-| tst.js:100:2:102:3 | Object. ... { }\\n\\t}) | This write to property 'setter' is useless, since $@ always overrides it. | tst.js:103:2:103:14 | o.setter = "" | another property write |
 | tst.js:114:2:114:14 | o.setter = 42 | This write to property 'setter' is useless, since $@ always overrides it. | tst.js:115:2:115:14 | o.setter = 87 | another property write |
-| tst.js:122:2:122:78 | Object. ... le:!1}) | This write to property 'prop' is useless, since $@ always overrides it. | tst.js:123:2:123:12 | o.prop = 42 | another property write |
+| tst.js:118:2:118:104 | Object. ... lue()}) | This write to property 'prop' is useless, since $@ always overrides it. | tst.js:119:2:119:12 | o.prop = 42 | another property write |

javascript/ql/test/query-tests/Expressions/ExprHasNoEffect/ExprHasNoEffect.expected

@@ -8,4 +8,5 @@
 | tst.js:49:3:49:26 | new Err ... ou so") | This expression has no effect. |
 | tst.js:50:3:50:49 | new Syn ... o me?") | This expression has no effect. |
 | tst.js:51:3:51:36 | new Err ... age(e)) | This expression has no effect. |
+| tst.js:62:2:62:20 | o.trivialNonGetter1 | This expression has no effect. |
 | uselessfn.js:1:1:1:15 | (functi ... .");\\n}) | This expression has no effect. |