Merge pull request #1113 from esben-semmle/js/useless-property-assign-setter

JS: improve use of attributes from ~Object.defineProperty~

Author: Max Schaefer

change-notes/1.21/analysis-javascript.md

@@ -15,7 +15,9 @@
 
 ## Changes to existing queries
 
-| **Query**                                  | **Expected impact**          | **Change**                                                                   |
-|--------------------------------------------|------------------------------|------------------------------------------------------------------------------|
+| **Query**                      | **Expected impact**          | **Change**                                                                |
+|--------------------------------|------------------------------|---------------------------------------------------------------------------|
+| Expression has no effect       | Fewer false-positive results | This rule now treats uses of `Object.defineProperty` more conservatively. |
+| Useless assignment to property | Fewer false-positive results | This rule now ignore reads of additional getters. |
 
 ## Changes to QL libraries

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,3 +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: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: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/Declarations/DeadStoreOfProperty/tst.js

@@ -82,17 +82,47 @@
     }
 
     // DOM
-    o.clientTop = 42;
+    o.clientTop = 42; // OK
     o.clientTop = 42;
 
-    o.defaulted1 = null;
+    o.defaulted1 = null; // OK
     o.defaulted1 = 42;
 
-    o.defaulted2 = -1;
+    o.defaulted2 = -1; // OK
     o.defaulted2 = 42;
 
     var o = {};
     o.pure18 = 42; // NOT OK
     o.pure18 = 42; // NOT OK
     o.pure18 = 42;
+
+	var o = {};
+	Object.defineProperty(o, "setter", { // OK
+		set: function (value) { }
+	});
+	o.setter = "";
+
+	var o = { set setter(value) { } }; // OK
+	o.setter = "";
+
+	var o = {
+		set accessor(value) { }, // OK
+		get accessor() { }
+	};
+
+	var o = { set setter(value) { } };
+	o.setter = 42; // probably OK, but still flagged - it seems fishy
+	o.setter = 87;
+
+	var o = {};
+	Object.defineProperty(o, "prop", {writable:!0,configurable:!0,enumerable:!1, value: getInitialValue()}) // NOT OK
+	o.prop = 42;
+
+	var o = {};
+	Object.defineProperty(o, "prop", {writable:!0,configurable:!0,enumerable:!1, value: undefined}) // OK, default value
+	o.prop = 42;
+
+	var o = {};
+	Object.defineProperty(o, "prop", {writable:!0,configurable:!0,enumerable:!1}) // OK
+	o.prop = 42;
 });

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. |

javascript/ql/test/query-tests/Expressions/ExprHasNoEffect/tst.js

@@ -51,3 +51,25 @@ try {
   new Error(computeSnarkyMessage(e)); // NOT OK
   new UnknownError(); // OK
 }
+
+function g() {
+	var o = {};
+
+	Object.defineProperty(o, "trivialGetter1", { get: function(){} });
+	o.trivialGetter1; // OK
+
+	Object.defineProperty(o, "trivialNonGetter1", "foo");
+	o.trivialNonGetter1; // NOT OK
+
+	var getterDef1 = { get: function(){} };
+	Object.defineProperty(o, "nonTrivialGetter1", getterDef1);
+	o.nonTrivialGetter1; // OK
+
+	var getterDef2 = { };
+	unknownPrepareGetter(getterDef2);
+	Object.defineProperty(o, "nonTrivialNonGetter1", getterDef2);
+	o.nonTrivialNonGetter1; // OK
+
+	Object.defineProperty(o, "nonTrivialGetter2", unknownGetterDef());
+	o.nonTrivialGetter2; // OK
+};