From 3cabc12be36a214234f8dd56eb09df2f045b1d65 Mon Sep 17 00:00:00 2001 From: Max Schaefer Date: Mon, 4 Mar 2019 09:50:02 +0000 Subject: [PATCH] JavaScript: Teach InvalidExport to never flag `module.exports = exports = ...` and similar. This was previously flagged if `exports` wasn't used any further. While it's true that the assignment to `exports` is redundant in this case, the assignment is also flagged by DeadStorOfLocal, so there is no point in InvalidExport flagging it as well. --- change-notes/1.20/analysis-javascript.md | 1 + javascript/ql/src/NodeJS/InvalidExport.ql | 8 ++------ .../DeadStoreOfLocal/DeadStoreOfLocal.expected | 2 ++ .../query-tests/Declarations/DeadStoreOfLocal/tst3.js | 2 ++ .../query-tests/Declarations/DeadStoreOfLocal/tst3b.js | 2 ++ .../NodeJS/InvalidExport/InvalidExport.expected | 1 - .../ql/test/query-tests/NodeJS/InvalidExport/tst3.js | 2 +- .../ql/test/query-tests/NodeJS/InvalidExport/tst3b.js | 2 ++ 8 files changed, 12 insertions(+), 8 deletions(-) create mode 100644 javascript/ql/test/query-tests/Declarations/DeadStoreOfLocal/tst3.js create mode 100644 javascript/ql/test/query-tests/Declarations/DeadStoreOfLocal/tst3b.js create mode 100644 javascript/ql/test/query-tests/NodeJS/InvalidExport/tst3b.js diff --git a/change-notes/1.20/analysis-javascript.md b/change-notes/1.20/analysis-javascript.md index 0814284c5f92..271138df74bc 100644 --- a/change-notes/1.20/analysis-javascript.md +++ b/change-notes/1.20/analysis-javascript.md @@ -38,6 +38,7 @@ | **Query** | **Expected impact** | **Change** | |--------------------------------------------|------------------------------|------------------------------------------------------------------------------| | Ambiguous HTML id attribute | Fewer false-positive results | This rule now treats templates more conservatively. Its precision has been revised to 'high'. | +| Assignment to exports variable | Fewer results | This rule no longer flags code that is also flagged by the rule "Useless assignment to local variable". | | Client-side cross-site scripting | More true-positive results, fewer false-positive results. | This rule now recognizes WinJS functions that are vulnerable to HTML injection. It no longer flags certain safe uses of jQuery, and recognizes custom sanitizers. | | Hard-coded credentials | Fewer false-positive results | This rule no longer flag the empty string as a hardcoded username. | | Insecure randomness | More results | This rule now flags insecure uses of `crypto.pseudoRandomBytes`. | diff --git a/javascript/ql/src/NodeJS/InvalidExport.ql b/javascript/ql/src/NodeJS/InvalidExport.ql index 08925ab07d25..9daa363e8887 100644 --- a/javascript/ql/src/NodeJS/InvalidExport.ql +++ b/javascript/ql/src/NodeJS/InvalidExport.ql @@ -41,12 +41,8 @@ from Assignment assgn, Variable exportsVar, DataFlow::Node exportsVal where exportsAssign(assgn, exportsVar, exportsVal) and not exists(exportsVal.getAPredecessor()) and - not ( - // this is OK if `exportsVal` flows into `module.exports` - moduleExportsAssign(_, exportsVal) and - // however, if there are no further uses of `exports` the assignment is useless anyway - strictcount(exportsVar.getAnAccess()) > 1 - ) and + // this is OK if `exportsVal` flows into `module.exports` + not moduleExportsAssign(_, exportsVal) and // export assignments do work in closure modules not assgn.getTopLevel() instanceof Closure::ClosureModule select assgn, "Assigning to 'exports' does not export anything." diff --git a/javascript/ql/test/query-tests/Declarations/DeadStoreOfLocal/DeadStoreOfLocal.expected b/javascript/ql/test/query-tests/Declarations/DeadStoreOfLocal/DeadStoreOfLocal.expected index 5d4aadc784ed..fab74fd6bf80 100644 --- a/javascript/ql/test/query-tests/Declarations/DeadStoreOfLocal/DeadStoreOfLocal.expected +++ b/javascript/ql/test/query-tests/Declarations/DeadStoreOfLocal/DeadStoreOfLocal.expected @@ -1,6 +1,8 @@ | overload.ts:10:12:10:14 | baz | This definition of baz is useless, since its value is never read. | | tst2.js:26:9:26:14 | x = 23 | This definition of x is useless, since its value is never read. | | tst2.js:28:9:28:14 | x = 42 | This definition of x is useless, since its value is never read. | +| tst3.js:2:1:2:36 | exports ... a: 23 } | This definition of exports is useless, since its value is never read. | +| tst3b.js:2:18:2:36 | exports = { a: 23 } | This definition of exports is useless, since its value is never read. | | tst.js:6:2:6:7 | y = 23 | This definition of y is useless, since its value is never read. | | tst.js:13:6:13:11 | a = 23 | This definition of a is useless, since its value is never read. | | tst.js:13:14:13:19 | a = 42 | This definition of a is useless, since its value is never read. | diff --git a/javascript/ql/test/query-tests/Declarations/DeadStoreOfLocal/tst3.js b/javascript/ql/test/query-tests/Declarations/DeadStoreOfLocal/tst3.js new file mode 100644 index 000000000000..91a09ed03d7d --- /dev/null +++ b/javascript/ql/test/query-tests/Declarations/DeadStoreOfLocal/tst3.js @@ -0,0 +1,2 @@ +// NOT OK +exports = module.exports = { a: 23 }; diff --git a/javascript/ql/test/query-tests/Declarations/DeadStoreOfLocal/tst3b.js b/javascript/ql/test/query-tests/Declarations/DeadStoreOfLocal/tst3b.js new file mode 100644 index 000000000000..ca9ae499600f --- /dev/null +++ b/javascript/ql/test/query-tests/Declarations/DeadStoreOfLocal/tst3b.js @@ -0,0 +1,2 @@ +// NOT OK +module.exports = exports = { a: 23 }; diff --git a/javascript/ql/test/query-tests/NodeJS/InvalidExport/InvalidExport.expected b/javascript/ql/test/query-tests/NodeJS/InvalidExport/InvalidExport.expected index 6f9d65ff33ba..1ccacc33b62c 100644 --- a/javascript/ql/test/query-tests/NodeJS/InvalidExport/InvalidExport.expected +++ b/javascript/ql/test/query-tests/NodeJS/InvalidExport/InvalidExport.expected @@ -1,3 +1,2 @@ -| tst3.js:2:1:2:36 | exports ... a: 23 } | Assigning to 'exports' does not export anything. | | tst5.js:3:1:3:12 | exports = {} | Assigning to 'exports' does not export anything. | | tst.js:2:1:2:12 | exports = 56 | Assigning to 'exports' does not export anything. | diff --git a/javascript/ql/test/query-tests/NodeJS/InvalidExport/tst3.js b/javascript/ql/test/query-tests/NodeJS/InvalidExport/tst3.js index c836229edd46..93448cd102f9 100644 --- a/javascript/ql/test/query-tests/NodeJS/InvalidExport/tst3.js +++ b/javascript/ql/test/query-tests/NodeJS/InvalidExport/tst3.js @@ -1,2 +1,2 @@ -// NOT OK: useless assignment +// OK: useless assignment flagged by other query exports = module.exports = { a: 23 }; diff --git a/javascript/ql/test/query-tests/NodeJS/InvalidExport/tst3b.js b/javascript/ql/test/query-tests/NodeJS/InvalidExport/tst3b.js new file mode 100644 index 000000000000..679437bd5b9d --- /dev/null +++ b/javascript/ql/test/query-tests/NodeJS/InvalidExport/tst3b.js @@ -0,0 +1,2 @@ +// OK: useless assignment flagged by other query +module.exports = exports = { a: 23 };