diff --git a/javascript/ql/src/Expressions/HeterogeneousComparison.ql b/javascript/ql/src/Expressions/HeterogeneousComparison.ql index f29ea9d3896..9a84d92e8ce 100644 --- a/javascript/ql/src/Expressions/HeterogeneousComparison.ql +++ b/javascript/ql/src/Expressions/HeterogeneousComparison.ql @@ -170,6 +170,28 @@ string getTypeDescription(string message1, string message2, int complexity1, int result = message1 } +/** + * Holds if `e` directly uses a parameter's initial value as passed in from the caller. + */ +predicate isInitialParameterUse(Expr e) { + exists (SimpleParameter p, SsaExplicitDefinition ssa | + ssa.getAContributingVarDef() = p and + ssa.getVariable().getAUse() = e and + not p.isRestParameter() + ) +} + +/** + * Holds if `e` is an expression that should not be considered in a heterogeneous comparison. + * + * We currently whitelist these kinds of expressions: + * + * - parameters, as passed in from the caller + */ +predicate whitelist(Expr e) { + isInitialParameterUse(e) +} + from ASTNode cmp, DataFlow::AnalyzedNode left, DataFlow::AnalyzedNode right, string leftTypes, string rightTypes, @@ -177,6 +199,8 @@ from ASTNode cmp, int leftTypeCount, int rightTypeCount , string leftTypeDescription, string rightTypeDescription where isHeterogeneousComparison(cmp, left, right, leftTypes, rightTypes) and + not whitelist(left.asExpr()) and + not whitelist(right.asExpr()) and leftExprDescription = capitalize(getDescription(left.asExpr(), "this expression")) and rightExprDescription = getDescription(right.asExpr(), "an expression") and leftTypeCount = strictcount(left.getAType()) and diff --git a/javascript/ql/src/Statements/UselessConditional.ql b/javascript/ql/src/Statements/UselessConditional.ql index 41389a75c0a..40f4dcf34b7 100644 --- a/javascript/ql/src/Statements/UselessConditional.ql +++ b/javascript/ql/src/Statements/UselessConditional.ql @@ -14,6 +14,7 @@ import javascript import semmle.javascript.RestrictedLocations +import semmle.javascript.dataflow.Refinements /** * Holds if `va` is a defensive truthiness check that may be worth keeping, even if it @@ -62,19 +63,52 @@ predicate isConstant(Expr e) { isSymbolicConstant(e.(VarAccess).getVariable()) } +/** + * Holds if `e` directly uses a parameter's initial value as passed in from the caller. + */ +predicate isInitialParameterUse(Expr e) { + exists (SimpleParameter p, SsaExplicitDefinition ssa | + ssa.getAContributingVarDef() = p and + ssa.getVariable().getAUse() = e and + not p.isRestParameter() + ) + or + isInitialParameterUse(e.(LogNotExpr).getOperand()) +} + +/** + * Holds if `e` directly uses the returned value from a function call that returns a constant boolean value. + */ +predicate isConstantBooleanReturnValue(Expr e) { + exists (DataFlow::CallNode call | + exists (call.analyze().getTheBooleanValue()) | + e = call.asExpr() or + exists (SsaExplicitDefinition ssa | + ssa.getDef().getSource() = call.asExpr() and + ssa.getVariable().getAUse() = e + ) + ) + or + isConstantBooleanReturnValue(e.(LogNotExpr).getOperand()) +} + /** * Holds if `e` is an expression that should not be flagged as a useless condition. * - * We currently whitelist three kinds of expressions: + * We currently whitelist these kinds of expressions: * * - constants (including references to literal constants); * - negations of constants; - * - defensive checks. + * - defensive checks; + * - parameters, as passed in from the caller; + * - constant boolean returned values */ predicate whitelist(Expr e) { isConstant(e) or isConstant(e.(LogNotExpr).getOperand()) or - isDefensiveInit(e) + isDefensiveInit(e) or + isInitialParameterUse(e) or + isConstantBooleanReturnValue(e) } /** @@ -107,4 +141,4 @@ where isConditional(cond, op.asExpr()) and msg = "This expression always evaluates to " + cv + "." ) -select sel, msg \ No newline at end of file +select sel, msg diff --git a/javascript/ql/test/library-tests/DataFlow/incomplete.expected b/javascript/ql/test/library-tests/DataFlow/incomplete.expected index fd7bf9aeade..7345402845f 100644 --- a/javascript/ql/test/library-tests/DataFlow/incomplete.expected +++ b/javascript/ql/test/library-tests/DataFlow/incomplete.expected @@ -26,7 +26,6 @@ | tst.js:65:3:65:10 | yield 42 | yield | | tst.js:66:13:66:25 | function.sent | yield | | tst.js:68:12:68:14 | h() | call | -| tst.js:69:1:69:9 | iter.next | call | | tst.js:69:1:69:9 | iter.next | heap | | tst.js:69:1:69:13 | iter.next(23) | call | | tst.js:72:3:72:11 | await p() | await | diff --git a/javascript/ql/test/library-tests/frameworks/HTTP-heuristics/src/middleware-attacher-getter.js b/javascript/ql/test/library-tests/frameworks/HTTP-heuristics/src/middleware-attacher-getter.js index 6001b1bf52b..402113ab534 100644 --- a/javascript/ql/test/library-tests/frameworks/HTTP-heuristics/src/middleware-attacher-getter.js +++ b/javascript/ql/test/library-tests/frameworks/HTTP-heuristics/src/middleware-attacher-getter.js @@ -7,7 +7,7 @@ function getAttacher1 (app) { var app = express(); getAttacher1(app); - +confuse(getAttacher2); // disable the type inference function getAttacher2 (app) { return function(h) { @@ -17,3 +17,4 @@ function getAttacher2 (app) { var app = express(); getAttacher2(app)(function(req, res){}); +confuse(getAttacher2); // disable the type inference diff --git a/javascript/ql/test/library-tests/frameworks/HTTP-heuristics/src/route-objects.js b/javascript/ql/test/library-tests/frameworks/HTTP-heuristics/src/route-objects.js index 43c67fdaeb4..64dbe455560 100644 --- a/javascript/ql/test/library-tests/frameworks/HTTP-heuristics/src/route-objects.js +++ b/javascript/ql/test/library-tests/frameworks/HTTP-heuristics/src/route-objects.js @@ -58,3 +58,4 @@ function wrap(f){ } } app[route3.method](route3.url, wrap(route3.handler)); +confuse(wrap); // confuse the type inference diff --git a/javascript/ql/test/query-tests/Expressions/HeterogeneousComparison/HeterogeneousComparison.expected b/javascript/ql/test/query-tests/Expressions/HeterogeneousComparison/HeterogeneousComparison.expected index 5170ec43a93..c04f07c6f44 100644 --- a/javascript/ql/test/query-tests/Expressions/HeterogeneousComparison/HeterogeneousComparison.expected +++ b/javascript/ql/test/query-tests/Expressions/HeterogeneousComparison/HeterogeneousComparison.expected @@ -46,3 +46,5 @@ | tst.js:208:5:208:6 | t5 | Variable 't5' cannot be of type function or regular expression, but it is compared to $@ of type function or regular expression. | tst.js:208:12:208:13 | t2 | variable 't2' | | tst.js:209:5:209:6 | t3 | Variable 't3' is of type function, object or regular expression, but it is compared to $@ of type boolean, null, number, string or undefined. | tst.js:209:12:209:13 | t5 | variable 't5' | | tst.js:210:5:210:6 | t5 | Variable 't5' is of type boolean, null, number, string or undefined, but it is compared to $@ of type function, object or regular expression. | tst.js:210:12:210:13 | t3 | variable 't3' | +| tst.js:225:13:225:14 | xy | Variable 'xy' is of type undefined, but it is compared to $@ of type string. | tst.js:225:20:225:24 | "foo" | an expression | +| tst.js:233:5:233:5 | x | Variable 'x' is of type object, but it is compared to $@ of type number. | tst.js:233:11:233:12 | 42 | an expression | diff --git a/javascript/ql/test/query-tests/Expressions/HeterogeneousComparison/tst.js b/javascript/ql/test/query-tests/Expressions/HeterogeneousComparison/tst.js index 7de04e29c61..46199104674 100644 --- a/javascript/ql/test/query-tests/Expressions/HeterogeneousComparison/tst.js +++ b/javascript/ql/test/query-tests/Expressions/HeterogeneousComparison/tst.js @@ -212,4 +212,25 @@ function l() { 1n == 1; // OK +(function tooGeneralLocalFunctions(){ + function f1(x) { + if (x === "foo") { // OK, whitelisted + + } + } + f1(undefined); + + function f2(x, y) { + var xy = o.q? x: y; + if (xy === "foo") { // NOT OK (not whitelisted like above) + + } + } + f2(undefined, undefined); +})(); + +function f(...x) { + x === 42 +}; + // semmle-extractor-options: --experimental diff --git a/javascript/ql/test/query-tests/Statements/UselessConditional/UselessConditional.expected b/javascript/ql/test/query-tests/Statements/UselessConditional/UselessConditional.expected index a1f2aae962d..2488d37de71 100644 --- a/javascript/ql/test/query-tests/Statements/UselessConditional/UselessConditional.expected +++ b/javascript/ql/test/query-tests/Statements/UselessConditional/UselessConditional.expected @@ -10,3 +10,12 @@ | UselessConditional.js:29:8:29:8 | x | Variable 'x' always evaluates to true here. | | UselessConditional.js:30:8:30:14 | new X() | This expression always evaluates to true. | | UselessConditional.js:33:7:33:7 | x | Variable 'x' always evaluates to false here. | +| UselessConditional.js:54:9:54:13 | known | Variable 'known' always evaluates to false here. | +| UselessConditional.js:60:9:60:15 | unknown | Variable 'unknown' always evaluates to false here. | +| UselessConditional.js:65:5:65:5 | x | Variable 'x' always evaluates to true here. | +| UselessConditional.js:76:13:76:13 | x | Variable 'x' always evaluates to true here. | +| UselessConditional.js:82:13:82:13 | x | Variable 'x' always evaluates to true here. | +| UselessConditionalGood.js:58:12:58:13 | x2 | Variable 'x2' always evaluates to false here. | +| UselessConditionalGood.js:69:12:69:13 | xy | Variable 'xy' always evaluates to false here. | +| UselessConditionalGood.js:85:12:85:13 | xy | Variable 'xy' always evaluates to false here. | +| UselessConditionalGood.js:97:12:97:13 | xy | Variable 'xy' always evaluates to false here. | diff --git a/javascript/ql/test/query-tests/Statements/UselessConditional/UselessConditional.js b/javascript/ql/test/query-tests/Statements/UselessConditional/UselessConditional.js index 7586ed6af38..5beff0e17a0 100644 --- a/javascript/ql/test/query-tests/Statements/UselessConditional/UselessConditional.js +++ b/javascript/ql/test/query-tests/Statements/UselessConditional/UselessConditional.js @@ -44,4 +44,44 @@ async function awaitFlow(){ } } +(function(){ + function knownF() { + return false; + } + var known = knownF(); + if (known) + return; + if (known) + return; + + var unknown = unknownF(); + if (unknown) + return; + if (unknown) // NOT OK + return; +}); + +(function (...x) { + x || y // NOT OK +}); + +(function() { + function f1(x) { + x || y // NOT OK, but whitelisted + } + f1(true); + + function f2(x) { + while (true) + x || y // NOT OK + } + f2(true); + + function f3(x) { + (function(){ + x || y // NOT OK + }); + } + f3(true); +}); // semmle-extractor-options: --experimental diff --git a/javascript/ql/test/query-tests/Statements/UselessConditional/UselessConditionalGood.js b/javascript/ql/test/query-tests/Statements/UselessConditional/UselessConditionalGood.js index 667a1eca83f..0a1c5888e09 100644 --- a/javascript/ql/test/query-tests/Statements/UselessConditional/UselessConditionalGood.js +++ b/javascript/ql/test/query-tests/Statements/UselessConditional/UselessConditionalGood.js @@ -5,4 +5,97 @@ function getLastLine(input) { if (!lines.length) throw new Error("No lines!"); return lines[lines.length-1]; -} \ No newline at end of file +} + +(function tooSpecificFunctions(){ + function f1() { + return false + } + if(f1()){} // OK, whitelisted + + function f2() { + return false + } + if(!f2()){} // OK, whitelisted + + function f3() { + return false + } + if(!!f3()){} // OK, whitelisted + + function f4() { + return false + } + if(f4() || o.p){} // OK, whitelisted + + function f5() { + return false + } + var v5 = f5(); + if(v5){} // OK, whitelisted + + function f6() { + return false + } + var v6 = f6(); + if(!!v6){} // OK, whitelisted +})(); + +(function tooGeneralFunctions(){ + function f1(x) { + if(x){} // OK, whitelisted + } + f1(undefined); + f1({}); + + function f2(x) { + if(x){} // OK, whitelisted + } + f2(undefined); + + function f3(x1) { + var x2 = x1; + if(x2){} // NOT OK, not whitelisted + } + f3(undefined); + + function f4(x) { + if(x && o.p){} // OK, whitelisted + } + f4(undefined); + + function f5(x, y) { + var xy = o.q? x: y; + if(xy && o.p){} // NOT OK, not whitelisted + } + f5(undefined, undefined); + + function f6(x) { + if(!x){} // OK, whitelisted + } + f6(true); + + function f7(x) { + if(!!x){} // OK, whitelisted + } + f7(true); + + function f8(x, y) { + var xy = x || y; + if(xy){} // NOT OK, not whitelisted + } + f8(undefined, undefined); + + function f9(x, y) { + var xy = !x || y; + if(xy){} // OK, whitelisted + } + f9(undefined, undefined); + + function f10(x, y) { + var xy = !!x || y; + if(xy){} // NOT OK, not whitelisted + } + f10(undefined, undefined); + +})();