Merge pull request #28 from esben-semmle/js/whitelist-empty-functions

JS: permit some calls with spurious arguments to empty functions

Author: Max Schaefer

change-notes/1.18/analysis-javascript.md

@@ -44,6 +44,7 @@
 | Missing X-Frame-Options HTTP header | Fewer false-positive results | This rule now treats header names case-insensitively. |
 | Reflected cross-site scripting | Fewer false-positive results | This rule now treats header names case-insensitively. |
 | Server-side URL redirect | More true-positive results | This rule now treats header names case-insensitively. |
+| Superfluous trailing arguments | Fewer false-positive results | This rule now ignores calls to some empty functions. |
 | Uncontrolled command line | More true-positive results | This rule now recognizes indirect command injection through `sh -c` and similar. |
 | Unused variable | Fewer results | This rule no longer flags class expressions that could be made anonymous. While technically true, these results are not interesting. |
 | Unused variable | Renamed | This rule has been renamed to "Unused variable, import, function or class" to reflect the fact that it flags different kinds of unused program elements. |

javascript/ql/src/LanguageFeatures/SpuriousArguments.ql

@@ -83,5 +83,14 @@ class SpuriousArguments extends Expr {
 
 from SpuriousArguments args, Function f, string arguments
 where f = args.getCall().getACallee() and
-      if args.getCount() = 1 then arguments = "argument" else arguments = "arguments"
+      if args.getCount() = 1 then arguments = "argument" else arguments = "arguments" and
+      (
+        // exclude empty functions, they are probably commented out debug utilities ...
+        exists(f.getABodyStmt()) or
+        // ... but include: constructors, arrows and externals/ambients
+        f instanceof Constructor or // unlikely to be a debug utility
+        f instanceof ArrowFunctionExpr or // cannot be empty
+        f instanceof ExternalFunction or // always empty
+        f.isAmbient() // always empty
+      )
 select args, "Superfluous " + arguments + " passed to $@.", f, f.describe()

javascript/ql/test/query-tests/LanguageFeatures/SpuriousArguments/SpuriousArguments.expected

@@ -1,14 +1,19 @@
 | es2015.js:4:16:4:17 | 23 | Superfluous argument passed to $@. | es2015.js:2:14:2:32 | (x) { this.x = x; } | constructor of class Class1 |
 | es2015.js:17:11:17:12 | 42 | Superfluous argument passed to $@. | es2015.js:15:13:15:12 | () {} | default constructor of class Other |
 | es2015.js:21:3:21:4 | 42 | Superfluous argument passed to $@. | tst.js:1:1:4:1 | functio ... x+19;\\n} | function f |
-| globals.js:15:13:15:13 | x | Superfluous argument passed to $@. | globals.js:9:1:9:25 | functio ... al() {} | function otherglobal |
-| globals.js:16:24:16:24 | x | Superfluous argument passed to $@. | globals.js:9:1:9:25 | functio ... al() {} | function otherglobal |
-| globals.js:17:24:17:27 | x | Superfluous arguments passed to $@. | globals.js:9:1:9:25 | functio ... al() {} | function otherglobal |
-| reflection.js:6:15:6:15 | 1 | Superfluous argument passed to $@. | reflection.js:1:1:1:16 | function f0() {} | function f0 |
-| reflection.js:7:15:7:18 | 1 | Superfluous arguments passed to $@. | reflection.js:1:1:1:16 | function f0() {} | function f0 |
-| reflection.js:12:18:12:18 | 2 | Superfluous argument passed to $@. | reflection.js:2:1:2:17 | function f1(x) {} | function f1 |
-| thisparameter.ts:4:11:4:12 | 45 | Superfluous argument passed to $@. | thisparameter.ts:1:1:1:38 | functio ... ber) {} | function foo |
+| globals.js:15:13:15:13 | x | Superfluous argument passed to $@. | globals.js:9:1:9:32 | functio ... eturn;} | function otherglobal |
+| globals.js:16:24:16:24 | x | Superfluous argument passed to $@. | globals.js:9:1:9:32 | functio ... eturn;} | function otherglobal |
+| globals.js:17:24:17:27 | x | Superfluous arguments passed to $@. | globals.js:9:1:9:32 | functio ... eturn;} | function otherglobal |
+| reflection.js:6:15:6:15 | 1 | Superfluous argument passed to $@. | reflection.js:1:1:1:23 | functio ... eturn;} | function f0 |
+| reflection.js:7:15:7:18 | 1 | Superfluous arguments passed to $@. | reflection.js:1:1:1:23 | functio ... eturn;} | function f0 |
+| reflection.js:12:18:12:18 | 2 | Superfluous argument passed to $@. | reflection.js:2:1:2:24 | functio ... eturn;} | function f1 |
+| thisparameter.ts:4:11:4:12 | 45 | Superfluous argument passed to $@. | thisparameter.ts:1:1:1:45 | functio ... eturn;} | function foo |
 | tst.js:11:3:11:5 | g() | Superfluous argument passed to $@. | tst.js:1:1:4:1 | functio ... x+19;\\n} | function f |
 | tst.js:33:15:33:18 | 2 | Superfluous arguments passed to $@. | externs.js:34:1:34:27 | functio ... str) {} | function String |
-| tst.js:37:4:37:5 | 42 | Superfluous argument passed to $@. | tst.js:38:4:38:16 | function() {} | anonymous function |
+| tst.js:37:4:37:5 | 42 | Superfluous argument passed to $@. | tst.js:38:4:38:23 | function() {return;} | anonymous function |
 | tst.js:46:19:46:20 | 10 | Superfluous argument passed to $@. | externs.js:36:1:36:27 | functio ... num) {} | function parseFloat |
+| tst.js:70:11:70:12 | 42 | Superfluous argument passed to $@. | tst.js:49:2:51:2 | functio ... urn;\\n\\t} | function nonEmpty |
+| tst.js:75:13:75:14 | 42 | Superfluous argument passed to $@. | tst.js:63:19:63:33 | () => undefined | function emptyArrow |
+| tst.js:76:31:76:32 | 42 | Superfluous argument passed to $@. | tst.js:64:33:64:32 | () {} | default constructor of class ImplicitEmptyConstructor |
+| tst.js:77:31:77:32 | 42 | Superfluous argument passed to $@. | tst.js:67:14:68:3 | (){\\n\\t\\t} | constructor of class ExplicitEmptyConstructor |
+| tst.js:78:20:78:21 | 10 | Superfluous argument passed to $@. | externs.js:36:1:36:27 | functio ... num) {} | function parseFloat |

javascript/ql/test/query-tests/LanguageFeatures/SpuriousArguments/globals.js

@@ -1,15 +1,15 @@
-function global() {}
+function global() {return;}
 
 (function(window) {
-  window.global = function (x) {};
+  window.global = function (x) {return;};
 })(this);
 
 global(x); // OK: might refer to function on line 4
 
-function otherglobal() {}
+function otherglobal() {return;}
 
 var o = {
-  otherglobal: function (x) {}
+  otherglobal: function (x) {return;}
 };
 
 otherglobal(x); // NOT OK: can never refer to function on line 12

javascript/ql/test/query-tests/LanguageFeatures/SpuriousArguments/reflection.js

@@ -1,5 +1,5 @@
-function f0() {}
-function f1(x) {}
+function f0() {return;}
+function f1(x) {return;}
 
 f0.call();
 f0.call(this);

javascript/ql/test/query-tests/LanguageFeatures/SpuriousArguments/thisparameter.ts

@@ -1,4 +1,4 @@
-function foo(this: void, x: number) {}
+function foo(this: void, x: number) {return;}
 
 foo(45); // OK
 foo(null, 45); // NOT OK

javascript/ql/test/query-tests/LanguageFeatures/SpuriousArguments/tst.js

@@ -35,12 +35,45 @@ new String(1, 2, 3);
 (function(f) {
 	// NOT OK
 	f(42);
-})(function() {});
+})(function() {return;});
 
 (function h(f) {
 	// OK
 	f(42);
 	h(function(x) { return x; });
 })(function() {});
 
-parseFloat("123", 10);
+parseFloat("123", 10);
+
+(function testWhitelistEmptyFunctions(){
+	function nonEmpty(){
+		return;
+	}
+	function empty(){
+
+	}
+	function emptyWithParam(p){
+	}
+	function commentedEmpty(){
+		// doStuff(arguments);
+	}
+	function commentedEmptyWithSpreadParam(...args){
+		// doStuff(args)
+	}
+	var emptyArrow = () => undefined;
+	class ImplicitEmptyConstructor {
+	}
+	class ExplicitEmptyConstructor {
+		constructor(){
+		}
+	}
+	nonEmpty(42); // NOT OK
+	empty(42); // OK
+	emptyWithParam(42, 87);	// OK
+	commentedEmpty(42); // OK
+	commentedEmptyWithSpreadParam(42, 87); // OK
+	emptyArrow(42); // NOT OK
+	new ImplicitEmptyConstructor(42); // NOT OK
+	new ExplicitEmptyConstructor(42); // NOT OK
+	parseFloat("123", 10); // NOT OK
+})