From ff5b85067a888d3a69edc532e4cc7e1c2ca5a5ed Mon Sep 17 00:00:00 2001 From: Esben Sparre Andreasen Date: Mon, 11 Mar 2019 09:36:26 +0100 Subject: [PATCH 1/3] JS: add tests --- .../DeadStoreOfProperty.expected | 3 ++ .../Declarations/DeadStoreOfProperty/tst.js | 36 +++++++++++++++++-- .../Expressions/ExprHasNoEffect/tst.js | 22 ++++++++++++ 3 files changed, 58 insertions(+), 3 deletions(-) diff --git a/javascript/ql/test/query-tests/Declarations/DeadStoreOfProperty/DeadStoreOfProperty.expected b/javascript/ql/test/query-tests/Declarations/DeadStoreOfProperty/DeadStoreOfProperty.expected index e11706d9e77d..221314a382e6 100644 --- a/javascript/ql/test/query-tests/Declarations/DeadStoreOfProperty/DeadStoreOfProperty.expected +++ b/javascript/ql/test/query-tests/Declarations/DeadStoreOfProperty/DeadStoreOfProperty.expected @@ -12,3 +12,6 @@ | 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 | diff --git a/javascript/ql/test/query-tests/Declarations/DeadStoreOfProperty/tst.js b/javascript/ql/test/query-tests/Declarations/DeadStoreOfProperty/tst.js index 81cf4fcd2107..2fd07a91d17b 100644 --- a/javascript/ql/test/query-tests/Declarations/DeadStoreOfProperty/tst.js +++ b/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; }); diff --git a/javascript/ql/test/query-tests/Expressions/ExprHasNoEffect/tst.js b/javascript/ql/test/query-tests/Expressions/ExprHasNoEffect/tst.js index f90209436f25..fec30c89fa5e 100644 --- a/javascript/ql/test/query-tests/Expressions/ExprHasNoEffect/tst.js +++ b/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 +}; From bd7eef08e86445b541d68e3bdbf7269efad691e1 Mon Sep 17 00:00:00 2001 From: Esben Sparre Andreasen Date: Mon, 11 Mar 2019 09:35:35 +0100 Subject: [PATCH 2/3] JS: introduce CallToObjectDefineProperty::getAPropertyAttribute --- .../src/Declarations/DeadStoreOfProperty.ql | 5 +++++ .../ql/src/Expressions/ExprHasNoEffect.ql | 19 +++++++++++++------ .../src/semmle/javascript/StandardLibrary.qll | 9 +++++++++ .../semmle/javascript/dataflow/DataFlow.qll | 5 +---- .../DeadStoreOfProperty.expected | 3 +-- .../ExprHasNoEffect/ExprHasNoEffect.expected | 1 + 6 files changed, 30 insertions(+), 12 deletions(-) diff --git a/javascript/ql/src/Declarations/DeadStoreOfProperty.ql b/javascript/ql/src/Declarations/DeadStoreOfProperty.ql index e2bbb7843eeb..a6ab02d79e71 100644 --- a/javascript/ql/src/Declarations/DeadStoreOfProperty.ql +++ b/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.", diff --git a/javascript/ql/src/Expressions/ExprHasNoEffect.ql b/javascript/ql/src/Expressions/ExprHasNoEffect.ql index 3ceda6fa29bc..300ce3dba385 100644 --- a/javascript/ql/src/Expressions/ExprHasNoEffect.ql +++ b/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, )` + // 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 diff --git a/javascript/ql/src/semmle/javascript/StandardLibrary.qll b/javascript/ql/src/semmle/javascript/StandardLibrary.qll index f6ea5bd76b32..68ad116f7f91 100644 --- a/javascript/ql/src/semmle/javascript/StandardLibrary.qll +++ b/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() + ) + } + } /** diff --git a/javascript/ql/src/semmle/javascript/dataflow/DataFlow.qll b/javascript/ql/src/semmle/javascript/dataflow/DataFlow.qll index 6d9551fbb8cb..fc3c41fe9340 100644 --- a/javascript/ql/src/semmle/javascript/dataflow/DataFlow.qll +++ b/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() } diff --git a/javascript/ql/test/query-tests/Declarations/DeadStoreOfProperty/DeadStoreOfProperty.expected b/javascript/ql/test/query-tests/Declarations/DeadStoreOfProperty/DeadStoreOfProperty.expected index 221314a382e6..68890cc34da7 100644 --- a/javascript/ql/test/query-tests/Declarations/DeadStoreOfProperty/DeadStoreOfProperty.expected +++ b/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 | diff --git a/javascript/ql/test/query-tests/Expressions/ExprHasNoEffect/ExprHasNoEffect.expected b/javascript/ql/test/query-tests/Expressions/ExprHasNoEffect/ExprHasNoEffect.expected index 0f74edd1835c..108e0de6a9e1 100644 --- a/javascript/ql/test/query-tests/Expressions/ExprHasNoEffect/ExprHasNoEffect.expected +++ b/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. | From bfc1c6ec8e0ed3766c40c96b28a1fcc498948648 Mon Sep 17 00:00:00 2001 From: Esben Sparre Andreasen Date: Thu, 14 Mar 2019 14:53:26 +0100 Subject: [PATCH 3/3] JS: change notes --- change-notes/1.21/analysis-javascript.md | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/change-notes/1.21/analysis-javascript.md b/change-notes/1.21/analysis-javascript.md index 33f60a1fc9c5..5faff6fc84dd 100644 --- a/change-notes/1.21/analysis-javascript.md +++ b/change-notes/1.21/analysis-javascript.md @@ -12,7 +12,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