JavaScript: Teach InvalidExport to never flag module.exports = exports = ... and similar.

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

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

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

javascript/ql/test/query-tests/Declarations/DeadStoreOfLocal/tst3.js

@@ -0,0 +1,2 @@
+// NOT OK
+exports = module.exports = { a: 23 };

javascript/ql/test/query-tests/Declarations/DeadStoreOfLocal/tst3b.js

@@ -0,0 +1,2 @@
+// NOT OK
+module.exports = exports = { a: 23 };

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

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 };

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 };