diff --git a/change-notes/1.18/analysis-javascript.md b/change-notes/1.18/analysis-javascript.md index aac65ea9601..fb20a0e6ec4 100644 --- a/change-notes/1.18/analysis-javascript.md +++ b/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. | diff --git a/javascript/ql/src/LanguageFeatures/SpuriousArguments.ql b/javascript/ql/src/LanguageFeatures/SpuriousArguments.ql index 284aa32f116..1b882622b41 100644 --- a/javascript/ql/src/LanguageFeatures/SpuriousArguments.ql +++ b/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() diff --git a/javascript/ql/test/query-tests/LanguageFeatures/SpuriousArguments/SpuriousArguments.expected b/javascript/ql/test/query-tests/LanguageFeatures/SpuriousArguments/SpuriousArguments.expected index 90c75f46e59..4645d4167ce 100644 --- a/javascript/ql/test/query-tests/LanguageFeatures/SpuriousArguments/SpuriousArguments.expected +++ b/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 | diff --git a/javascript/ql/test/query-tests/LanguageFeatures/SpuriousArguments/globals.js b/javascript/ql/test/query-tests/LanguageFeatures/SpuriousArguments/globals.js index dc288c4a050..3771a18ed87 100644 --- a/javascript/ql/test/query-tests/LanguageFeatures/SpuriousArguments/globals.js +++ b/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 diff --git a/javascript/ql/test/query-tests/LanguageFeatures/SpuriousArguments/reflection.js b/javascript/ql/test/query-tests/LanguageFeatures/SpuriousArguments/reflection.js index b3b0278198c..22c8c3b537e 100644 --- a/javascript/ql/test/query-tests/LanguageFeatures/SpuriousArguments/reflection.js +++ b/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); diff --git a/javascript/ql/test/query-tests/LanguageFeatures/SpuriousArguments/thisparameter.ts b/javascript/ql/test/query-tests/LanguageFeatures/SpuriousArguments/thisparameter.ts index acf6d8543d8..d166c957858 100644 --- a/javascript/ql/test/query-tests/LanguageFeatures/SpuriousArguments/thisparameter.ts +++ b/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 diff --git a/javascript/ql/test/query-tests/LanguageFeatures/SpuriousArguments/tst.js b/javascript/ql/test/query-tests/LanguageFeatures/SpuriousArguments/tst.js index 6882391da03..5d1b6f57f85 100644 --- a/javascript/ql/test/query-tests/LanguageFeatures/SpuriousArguments/tst.js +++ b/javascript/ql/test/query-tests/LanguageFeatures/SpuriousArguments/tst.js @@ -35,7 +35,7 @@ new String(1, 2, 3); (function(f) { // NOT OK f(42); -})(function() {}); +})(function() {return;}); (function h(f) { // OK @@ -43,4 +43,37 @@ new String(1, 2, 3); h(function(x) { return x; }); })(function() {}); -parseFloat("123", 10); \ No newline at end of file +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 +})