diff --git a/change-notes/1.19/analysis-javascript.md b/change-notes/1.19/analysis-javascript.md index 5630452ded37..4071deb9cb9d 100644 --- a/change-notes/1.19/analysis-javascript.md +++ b/change-notes/1.19/analysis-javascript.md @@ -23,6 +23,7 @@ | Replacement of a substring with itself (`js/identity-replacement`) | correctness, security, external/cwe/cwe-116 | Highlights string replacements that replace a string with itself, which usually indicates a mistake. Results shown on LGTM by default. | | Stored cross-site scripting (`js/stored-xss`) | security, external/cwe/cwe-079, external/cwe/cwe-116 | Highlights uncontrolled stored values flowing into HTML content, indicating a violation of [CWE-079](https://cwe.mitre.org/data/definitions/79.html). Results shown on LGTM by default. | | Unclear precedence of nested operators (`js/unclear-operator-precedence`) | maintainability, correctness, external/cwe/cwe-783 | Highlights nested binary operators whose relative precedence is easy to misunderstand. Results shown on LGTM by default. | +| Unneeded defensive code | correctness, external/cwe/cwe-570, external/cwe/cwe-571 | Highlights locations where defensive code is not needed. Results are shown on LGTM by default. | | Useless assignment to property | maintainability | Highlights property assignments whose value is always overwritten. Results are shown on LGTM by default. | | User-controlled data in file | security, external/cwe/cwe-912 | Highlights locations where user-controlled data is written to a file. Results are not shown on LGTM by default. | diff --git a/javascript/config/suites/javascript/maintainability-more b/javascript/config/suites/javascript/maintainability-more index 6cf17736f532..822a055b2e7b 100644 --- a/javascript/config/suites/javascript/maintainability-more +++ b/javascript/config/suites/javascript/maintainability-more @@ -6,6 +6,7 @@ + semmlecode-javascript-queries/Declarations/DuplicateVarDecl.ql: /Maintainability/Declarations + semmlecode-javascript-queries/Declarations/UnusedParameter.ql: /Maintainability/Declarations + semmlecode-javascript-queries/Declarations/UnusedVariable.ql: /Maintainability/Declarations ++ semmlecode-javascript-queries/Expressions/UselessDefensiveProgramming.ql: /Maintainability/Expressions + semmlecode-javascript-queries/LanguageFeatures/ArgumentsCallerCallee.ql: /Maintainability/Language Features + semmlecode-javascript-queries/LanguageFeatures/ConditionalComments.ql: /Maintainability/Language Features + semmlecode-javascript-queries/LanguageFeatures/Eval.ql: /Maintainability/Language Features diff --git a/javascript/ql/src/Expressions/HeterogeneousComparison.ql b/javascript/ql/src/Expressions/HeterogeneousComparison.ql index fd524466ddf4..ae1be5a7a989 100644 --- a/javascript/ql/src/Expressions/HeterogeneousComparison.ql +++ b/javascript/ql/src/Expressions/HeterogeneousComparison.ql @@ -15,6 +15,7 @@ import javascript private import semmle.javascript.dataflow.InferredTypes +private import semmle.javascript.DefensiveProgramming /** * Holds if `left` and `right` are the left and right operands, respectively, of `nd`, which is @@ -198,6 +199,7 @@ from ASTNode cmp, int leftTypeCount, int rightTypeCount , string leftTypeDescription, string rightTypeDescription where isHeterogeneousComparison(cmp, left, right, leftTypes, rightTypes) and + not exists (cmp.(Expr).flow().(DefensiveExpressionTest).getTheTestResult()) and not whitelist(left.asExpr()) and not whitelist(right.asExpr()) and leftExprDescription = capitalize(getDescription(left.asExpr(), "this expression")) and diff --git a/javascript/ql/src/Expressions/UnneededDefensiveProgramming.qhelp b/javascript/ql/src/Expressions/UnneededDefensiveProgramming.qhelp new file mode 100644 index 000000000000..fd7191dd7fa8 --- /dev/null +++ b/javascript/ql/src/Expressions/UnneededDefensiveProgramming.qhelp @@ -0,0 +1,91 @@ + + + +

+ + Defensive code can prevent unforeseen circumstances from + causing fatal program behaviors. + + A common defensive code pattern is to guard + against dereferencing the values null or + undefined. + + However, if the situation that some defensive code guards + against never can occur, then the defensive code serves no purpose and + can safely be removed. + +

+ + + + +

+ + Examine the surrounding code to determine if the defensive + code is worth keeping despite providing no practical use. If it is no + longer needed, remove it. + +

+ + + + +

+ + The following example shows a cleanupLater + function that asynchronously will perform a cleanup task after some + delay. When the cleanup task completes, the function invokes the + provided callback parameter cb. + + To prevent a crash by invoking cb when it + has the value undefined, defensive code guards + the invocation by checking if cb is truthy. + +

+ + + +

+ + However, the cleanupLater function is always + invoked with a callback argument, so the defensive code condition + always holds, and it is therefore not + required. The function can therefore be simplified to: + +

+ + + +

+ + Guarding against the same situation multiple times is + another example of defensive code that provides no practical use. The + example below shows a function that assigns a value to a property of + an object, where defensive code ensures that the assigned value is not + undefined or null. + +

+ + + +

+ + However, due to coercion rules, v == + undefined holds for both the situation where v + isundefined and the situation where v + isnull, so the v == null + guard serves no purpose, and can be removed: + +

+ + + + + + +
  • Wikipedia: Defensive programming.
    • + +     +     diff --git a/javascript/ql/src/Expressions/UnneededDefensiveProgramming.ql b/javascript/ql/src/Expressions/UnneededDefensiveProgramming.ql new file mode 100644 index 000000000000..bb36d94aef33 --- /dev/null +++ b/javascript/ql/src/Expressions/UnneededDefensiveProgramming.ql @@ -0,0 +1,31 @@ +/** + * @name Unneeded defensive code + * @description Defensive code that guards against a situation that never happens is not needed. + * @kind problem + * @problem.severity recommendation + * @id js/unneeded-defensive-code + * @tags correctness + * external/cwe/cwe-570 + * external/cwe/cwe-571 + * @precision very-high + */ + +import javascript +import semmle.javascript.DefensiveProgramming + +from DefensiveExpressionTest e, boolean cv +where e.getTheTestResult() = cv and + // whitelist + not ( + // module environment detection + exists (VarAccess access, string name | + name = "exports" or name = "module" | + e.asExpr().(Internal::TypeofUndefinedTest).getOperand() = access and + access.getName() = name and + not exists (access.getVariable().getADeclaration()) + ) + or + // too benign in practice + e instanceof Internal::DefensiveInit + ) +select e, "This guard always evaluates to " + cv + "." diff --git a/javascript/ql/src/Expressions/examples/UnneededDefensiveProgramming1_bad.js b/javascript/ql/src/Expressions/examples/UnneededDefensiveProgramming1_bad.js new file mode 100644 index 000000000000..d4816efdabb3 --- /dev/null +++ b/javascript/ql/src/Expressions/examples/UnneededDefensiveProgramming1_bad.js @@ -0,0 +1,10 @@ +function cleanupLater(delay, cb) { + setTimeout(function() { + cleanup(); + if (cb) { // BAD: useless check, `cb` is always truthy + cb(); + } + }, delay) +} + +cleanupLater(1000, function(){console.log("Cleanup done")}); diff --git a/javascript/ql/src/Expressions/examples/UnneededDefensiveProgramming1_good.js b/javascript/ql/src/Expressions/examples/UnneededDefensiveProgramming1_good.js new file mode 100644 index 000000000000..3a67bd563f88 --- /dev/null +++ b/javascript/ql/src/Expressions/examples/UnneededDefensiveProgramming1_good.js @@ -0,0 +1,9 @@ +function cleanupLater(delay, cb) { + setTimeout(function() { + cleanupNow(); + // GOOD: no need to guard the invocation + cb(); + }, delay) +} + +cleanupLater(function(){console.log("Cleanup done")}); diff --git a/javascript/ql/src/Expressions/examples/UnneededDefensiveProgramming2_bad.js b/javascript/ql/src/Expressions/examples/UnneededDefensiveProgramming2_bad.js new file mode 100644 index 000000000000..087a19497c9d --- /dev/null +++ b/javascript/ql/src/Expressions/examples/UnneededDefensiveProgramming2_bad.js @@ -0,0 +1,5 @@ +function setSafeStringProp(o, prop, v) { + // BAD: `v == null` is useless + var safe = v == undefined || v == null? '': v; + o[prop] = safe; +} diff --git a/javascript/ql/src/Expressions/examples/UnneededDefensiveProgramming2_good.js b/javascript/ql/src/Expressions/examples/UnneededDefensiveProgramming2_good.js new file mode 100644 index 000000000000..80734c2391ff --- /dev/null +++ b/javascript/ql/src/Expressions/examples/UnneededDefensiveProgramming2_good.js @@ -0,0 +1,5 @@ +function setSafeStringProp(o, prop, v) { + // GOOD: `v == undefined` handles both `undefined` and `null` + var safe = v == undefined? '': v; + o[prop] = safe; +} diff --git a/javascript/ql/src/Statements/UselessConditional.ql b/javascript/ql/src/Statements/UselessConditional.ql index 6e71d6827134..27b703f652ba 100644 --- a/javascript/ql/src/Statements/UselessConditional.ql +++ b/javascript/ql/src/Statements/UselessConditional.ql @@ -15,29 +15,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 - * is strictly speaking useless. - * - * We currently recognize three patterns: - * - * - the first `x` in `x || (x = e)` - * - the second `x` in `x = (x || e)` - * - the second `x` in `var x = x || e` - */ -predicate isDefensiveInit(VarAccess va) { - exists (LogOrExpr o, VarRef va2 | - va = o.getLeftOperand().getUnderlyingReference() and va2.getVariable() = va.getVariable() | - exists (AssignExpr assgn | va2 = assgn.getTarget() | - assgn = o.getRightOperand().stripParens() or - o = assgn.getRhs().getUnderlyingValue() - ) or - exists (VariableDeclarator vd | va2 = vd.getBindingPattern() | - o = vd.getInit().getUnderlyingValue() - ) - ) -} +import semmle.javascript.DefensiveProgramming /** * Holds if variable `v` looks like a symbolic constant, that is, it is assigned @@ -95,6 +73,21 @@ predicate isConstantBooleanReturnValue(Expr e) { isConstantBooleanReturnValue(e.(LogNotExpr).getOperand()) } +private Expr maybeStripLogNot(Expr e) { + result = maybeStripLogNot(e.(LogNotExpr).getOperand()) or + result = e +} + +/** + * Holds if `e` is a defensive expression with a fixed outcome. + */ +predicate isConstantDefensive(Expr e) { + exists(DefensiveExpressionTest defensive | + maybeStripLogNot(defensive.asExpr()) = e and + exists(defensive.getTheTestResult()) + ) +} + /** * Holds if `e` is an expression that should not be flagged as a useless condition. * @@ -109,7 +102,7 @@ predicate isConstantBooleanReturnValue(Expr e) { predicate whitelist(Expr e) { isConstant(e) or isConstant(e.(LogNotExpr).getOperand()) or - isDefensiveInit(e) or + isConstantDefensive(e) or // flagged by js/useless-defensive-code isInitialParameterUse(e) or isConstantBooleanReturnValue(e) } diff --git a/javascript/ql/src/semmle/javascript/DefensiveProgramming.qll b/javascript/ql/src/semmle/javascript/DefensiveProgramming.qll new file mode 100644 index 000000000000..6529566495b6 --- /dev/null +++ b/javascript/ql/src/semmle/javascript/DefensiveProgramming.qll @@ -0,0 +1,436 @@ +/** + * Provides classes for working with defensive programming patterns. + */ + +import javascript +private import semmle.javascript.dataflow.InferredTypes + +/** + * A test in a defensive programming pattern. + */ +abstract class DefensiveExpressionTest extends DataFlow::ValueNode { + /** Gets the unique Boolean value that this test evaluates to, if any. */ + abstract boolean getTheTestResult(); +} + +/** + * INTERNAL: Do not use directly; use `DefensiveExpressionTest` instead. + */ +module Internal { + /** + * A defensive truthiness check that may be worth keeping, even if it + * is strictly speaking useless. + * + * We currently recognize three patterns: + * + * - the first `x` in `x || (x = e)` + * - the second `x` in `x = (x || e)` + * - the second `x` in `var x = x || e` + */ + class DefensiveInit extends DefensiveExpressionTest { + DefensiveInit() { + exists(VarAccess va, LogOrExpr o, VarRef va2 | + va = astNode and + va = o.getLeftOperand().stripParens() and va2.getVariable() = va.getVariable() | + exists(AssignExpr assgn | va2 = assgn.getTarget() | + assgn = o.getRightOperand().stripParens() or + o = assgn.getRhs().stripParens() + ) + or + exists(VariableDeclarator vd | va2 = vd.getBindingPattern() | o = vd.getInit().stripParens()) + ) + } + + override boolean getTheTestResult() { + result = analyze().getTheBooleanValue() + } + } + + /** + * Gets the inner expression of `e`, with any surrounding parentheses and boolean nots removed. + * `polarity` is true iff the inner expression is nested in an even number of negations. + */ + private Expr stripNotsAndParens(Expr e, boolean polarity) { + exists (Expr inner | + inner = e.getUnderlyingValue() | + if inner instanceof LogNotExpr then + (result = stripNotsAndParens(inner.(LogNotExpr).getOperand(), polarity.booleanNot())) + else + (result = inner and polarity = true) + ) + } + + /** + * An equality test for `null` and `undefined`. + * + * Examples: `e === undefined` or `typeof e !== undefined`. + */ + private abstract class UndefinedNullTest extends EqualityTest { + /** Gets the unique Boolean value that this test evaluates to, if any. */ + abstract boolean getTheTestResult(); + + /** + * Gets the expression that is tested for being `null` or `undefined`. + */ + abstract Expr getOperand(); + } + + /** + * A dis- or conjunction that tests if an expression is `null` or `undefined` in either branch. + * + * Example: a branch in `x === null || x === undefined`. + */ + private class CompositeUndefinedNullTestPart extends DefensiveExpressionTest { + + UndefinedNullTest test; + + boolean polarity; + + CompositeUndefinedNullTestPart(){ + exists (LogicalBinaryExpr composite, Variable v, Expr op, Expr opOther, UndefinedNullTest testOther | + composite.hasOperands(op, opOther) and + this = op.flow() and + test = stripNotsAndParens(op, polarity) and + testOther = stripNotsAndParens(opOther, _) and + test.getOperand().(VarRef).getVariable() = v and + testOther.getOperand().(VarRef).getVariable() = v + ) + } + + override boolean getTheTestResult() { + polarity = true and result = test.getTheTestResult() + or + polarity = false and result = test.getTheTestResult().booleanNot() + } + + } + + /** + * A test for `undefined` or `null` in an if-statement. + * + * Example: `if (x === null) ...`. + */ + private class SanityCheckingUndefinedNullGuard extends DefensiveExpressionTest { + + UndefinedNullTest test; + + boolean polarity; + + SanityCheckingUndefinedNullGuard() { + exists (IfStmt c | + this = c.getCondition().flow() and + test = stripNotsAndParens(c.getCondition(), polarity) and + test.getOperand() instanceof VarRef + ) + } + + override boolean getTheTestResult() { + polarity = true and result = test.getTheTestResult() + or + polarity = false and result = test.getTheTestResult().booleanNot() + } + + } + + /** + * Holds if `t` is `null` or `undefined`. + */ + private predicate isNullOrUndefined(InferredType t) { + t = TTNull() or + t = TTUndefined() + } + + /** + * Holds if `t` is not `null` or `undefined`. + */ + private predicate isNotNullOrUndefined(InferredType t) { + not isNullOrUndefined(t) + } + + /** + * A value comparison for `null` and `undefined`. + * + * Examples: `x === null` or `x != undefined`. + */ + private class NullUndefinedComparison extends UndefinedNullTest { + + Expr operand; + + InferredType op2type; + + NullUndefinedComparison() { + exists (Expr op2 | + hasOperands(operand, op2) | + op2type = TTNull() and SyntacticConstants::isNull(op2) + or + op2type = TTUndefined() and SyntacticConstants::isUndefined(op2) + ) + } + + override boolean getTheTestResult() { + result = getPolarity() and + ( + if this instanceof StrictEqualityTest then + // case: `operand === null` or `operand === undefined` + operand.analyze().getTheType() = op2type + else + // case: `operand == null` or `operand == undefined` + not isNotNullOrUndefined(operand.analyze().getAType()) + ) + or + result = getPolarity().booleanNot() and + ( + if this instanceof StrictEqualityTest then + // case: `operand !== null` or `operand !== undefined` + not operand.analyze().getAType() = op2type + else + // case: `operand != null` or `operand != undefined` + not isNullOrUndefined(operand.analyze().getAType()) + ) + } + + override Expr getOperand() { + result = operand + } + } + + /** + * An expression that throws an exception if one of its subexpressions evaluates to `null` or `undefined`. + * + * Examples: `sub.p` or `sub()`. + */ + private class UndefinedNullCrashUse extends Expr { + + Expr target; + + UndefinedNullCrashUse() { + this.(InvokeExpr).getCallee().getUnderlyingValue() = target + or + this.(PropAccess).getBase().getUnderlyingValue() = target + or + this.(MethodCallExpr).getReceiver().getUnderlyingValue() = target + } + + /** + * Gets the subexpression that will cause an exception to be thrown if it is `null` or `undefined`. + */ + Expr getVulnerableSubexpression() { + result = target + } + + } + + /** + * An expression that throws an exception if one of its subexpressions is not a `function`. + * + * Example: `sub()`. + */ + private class NonFunctionCallCrashUse extends Expr { + + Expr target; + + NonFunctionCallCrashUse() { + this.(InvokeExpr).getCallee().getUnderlyingValue() = target + } + + /** + * Gets the subexpression that will cause an exception to be thrown if it is not a `function`. + */ + Expr getVulnerableSubexpression() { + result = target + } + + } + + /** + * Gets the first expression that is guarded by `guard`. + */ + private Expr getAGuardedExpr(Expr guard) { + exists(LogicalBinaryExpr op | + op.getLeftOperand() = guard and + op.getRightOperand() = result + ) + or + exists(IfStmt c, ExprStmt guardedStmt | + c.getCondition() = guard and + result = guardedStmt.getExpr() | + guardedStmt = c.getAControlledStmt() or + guardedStmt = c.getAControlledStmt().(BlockStmt).getStmt(0) + ) + or + exists (ConditionalExpr c | + c.getCondition() = guard | + result = c.getABranch() + ) + } + + /** + * Holds if `t` is `string`, `number` or `boolean`. + */ + private predicate isStringOrNumOrBool(InferredType t) { + t = TTString() or + t = TTNumber() or + t = TTBoolean() + } + + /** + * A defensive expression that tests for `undefined` and `null` using a truthiness test. + * + * Examples: The condition in `if(x) { x.p; }` or `!x || x.m()`. + */ + private class UndefinedNullTruthinessGuard extends DefensiveExpressionTest { + + VarRef guardVar; + + boolean polarity; + + UndefinedNullTruthinessGuard() { + exists (VarRef useVar | + guardVar = stripNotsAndParens(this.asExpr(), polarity) and + guardVar.getVariable() = useVar.getVariable() | + getAGuardedExpr(this.asExpr()).getUnderlyingValue().(UndefinedNullCrashUse).getVulnerableSubexpression() = useVar and + // exclude types whose truthiness depend on the value + not isStringOrNumOrBool(guardVar.analyze().getAType()) + ) + } + + override boolean getTheTestResult() { + exists (boolean testResult | + testResult = guardVar.analyze().getTheBooleanValue() | + if polarity = true then + result = testResult + else + result = testResult.booleanNot() + ) + } + + } + + /** + * A defensive expression that tests for `undefined` and `null`. + * + * Example: the condition in `if(x !== null) { x.p; }`. + */ + private class UndefinedNullTypeGuard extends DefensiveExpressionTest { + + UndefinedNullTest test; + + boolean polarity; + + UndefinedNullTypeGuard() { + exists (Expr guard, VarRef guardVar, VarRef useVar | + this = guard.flow() and + test = stripNotsAndParens(guard, polarity) and + test.getOperand() = guardVar and + guardVar.getVariable() = useVar.getVariable() | + getAGuardedExpr(guard).getUnderlyingValue().(UndefinedNullCrashUse).getVulnerableSubexpression() = useVar + ) + } + + override boolean getTheTestResult() { + polarity = true and result = test.getTheTestResult() + or + polarity = false and result = test.getTheTestResult().booleanNot() + } + + } + + /** + * A test for the value of a `typeof` expression. + * + * Example: `typeof x === 'undefined'`. + */ + private class TypeofTest extends EqualityTest { + Expr operand; + + TypeofTag tag; + + TypeofTest() { + exists (Expr op1, Expr op2 | + hasOperands(op1, op2) | + operand = op1.(TypeofExpr).getOperand() and + op2.mayHaveStringValue(tag) + ) + } + + boolean getTheTestResult() { + exists (boolean testResult | + testResult = true and operand.analyze().getTheType().getTypeofTag() = tag or + testResult = false and not operand.analyze().getAType().getTypeofTag() = tag | + if getPolarity() = true then + result = testResult + else + result = testResult.booleanNot() + ) + } + + /** + * Gets the operand used in the `typeof` expression. + */ + Expr getOperand() { + result = operand + } + + /** + * Gets the `typeof` tag that is tested. + */ + TypeofTag getTag() { + result = tag + } + } + + /** + * A defensive expression that tests if an expression has type `function`. + * + * Example: the condition in `if(typeof x === 'function') x()`. + */ + private class FunctionTypeGuard extends DefensiveExpressionTest { + + TypeofTest test; + + boolean polarity; + + FunctionTypeGuard() { + exists (Expr guard, VarRef guardVar, VarRef useVar | + this = guard.flow() and + test = stripNotsAndParens(guard, polarity) and + test.getOperand() = guardVar and + guardVar.getVariable() = useVar.getVariable() | + getAGuardedExpr(guard).getUnderlyingValue().(NonFunctionCallCrashUse).getVulnerableSubexpression() = useVar + ) and + test.getTag() = "function" + } + + override boolean getTheTestResult() { + polarity = true and result = test.getTheTestResult() + or + polarity = false and result = test.getTheTestResult().booleanNot() + } + + } + + /** + * A test for `undefined` using a `typeof` expression. + * + * Example: `typeof x === undefined'. + */ + class TypeofUndefinedTest extends UndefinedNullTest { + + TypeofTest test; + + TypeofUndefinedTest() { + this = test and + test.getTag() = "undefined" + } + + override boolean getTheTestResult() { + result = test.getTheTestResult() + } + + override Expr getOperand() { + result = test.getOperand() + } + + } + +} \ No newline at end of file diff --git a/javascript/ql/test/query-tests/Expressions/UnneededDefensiveProgramming/HeterogeneousComparison.expected b/javascript/ql/test/query-tests/Expressions/UnneededDefensiveProgramming/HeterogeneousComparison.expected new file mode 100644 index 000000000000..534048b0d15f --- /dev/null +++ b/javascript/ql/test/query-tests/Expressions/UnneededDefensiveProgramming/HeterogeneousComparison.expected @@ -0,0 +1,3 @@ +| tst.js:162:9:162:16 | typeof x | This expression is of type string, but it is compared to $@ of type undefined. | tst.js:162:22:162:30 | undefined | 'undefined' | +| tst.js:163:9:163:21 | typeof window | This expression is of type string, but it is compared to $@ of type undefined. | tst.js:163:27:163:35 | undefined | 'undefined' | +| tst.js:165:9:165:16 | typeof x | This expression is of type string, but it is compared to $@ of type undefined. | tst.js:165:22:165:22 | u | variable 'u' | diff --git a/javascript/ql/test/query-tests/Expressions/UnneededDefensiveProgramming/HeterogeneousComparison.qlref b/javascript/ql/test/query-tests/Expressions/UnneededDefensiveProgramming/HeterogeneousComparison.qlref new file mode 100644 index 000000000000..13b0e2a181cb --- /dev/null +++ b/javascript/ql/test/query-tests/Expressions/UnneededDefensiveProgramming/HeterogeneousComparison.qlref @@ -0,0 +1 @@ +Expressions/HeterogeneousComparison.ql \ No newline at end of file diff --git a/javascript/ql/test/query-tests/Expressions/UnneededDefensiveProgramming/UnneededDefensiveProgramming.expected b/javascript/ql/test/query-tests/Expressions/UnneededDefensiveProgramming/UnneededDefensiveProgramming.expected new file mode 100644 index 000000000000..f726f0a0b5df --- /dev/null +++ b/javascript/ql/test/query-tests/Expressions/UnneededDefensiveProgramming/UnneededDefensiveProgramming.expected @@ -0,0 +1,72 @@ +| module-environment-detection.js:23:8:23:36 | typeof ... efined' | This guard always evaluates to true. | +| tst2.js:4:12:4:35 | typeof ... efined" | This guard always evaluates to true. | +| tst.js:18:5:18:5 | u | This guard always evaluates to false. | +| tst.js:19:5:19:5 | n | This guard always evaluates to false. | +| tst.js:20:5:20:5 | o | This guard always evaluates to true. | +| tst.js:23:5:23:5 | u | This guard always evaluates to false. | +| tst.js:24:5:24:5 | n | This guard always evaluates to false. | +| tst.js:25:5:25:5 | o | This guard always evaluates to true. | +| tst.js:28:5:28:6 | !u | This guard always evaluates to true. | +| tst.js:29:5:29:6 | !n | This guard always evaluates to true. | +| tst.js:30:5:30:6 | !o | This guard always evaluates to false. | +| tst.js:33:5:33:7 | !!u | This guard always evaluates to false. | +| tst.js:34:5:34:7 | !!n | This guard always evaluates to false. | +| tst.js:35:5:35:7 | !!o | This guard always evaluates to true. | +| tst.js:38:5:38:18 | u != undefined | This guard always evaluates to false. | +| tst.js:39:5:39:18 | n != undefined | This guard always evaluates to false. | +| tst.js:40:5:40:18 | o != undefined | This guard always evaluates to true. | +| tst.js:43:5:43:18 | u == undefined | This guard always evaluates to true. | +| tst.js:44:5:44:18 | n == undefined | This guard always evaluates to true. | +| tst.js:45:5:45:18 | o == undefined | This guard always evaluates to false. | +| tst.js:48:5:48:19 | u === undefined | This guard always evaluates to true. | +| tst.js:49:5:49:19 | n === undefined | This guard always evaluates to false. | +| tst.js:50:5:50:19 | o === undefined | This guard always evaluates to false. | +| tst.js:53:9:53:9 | u | This guard always evaluates to false. | +| tst.js:56:9:56:9 | n | This guard always evaluates to false. | +| tst.js:59:9:59:9 | o | This guard always evaluates to true. | +| tst.js:66:5:66:5 | u | This guard always evaluates to false. | +| tst.js:67:5:67:5 | n | This guard always evaluates to false. | +| tst.js:68:5:68:5 | o | This guard always evaluates to true. | +| tst.js:71:9:71:23 | u !== undefined | This guard always evaluates to false. | +| tst.js:74:9:74:23 | n !== undefined | This guard always evaluates to true. | +| tst.js:77:9:77:23 | o !== undefined | This guard always evaluates to true. | +| tst.js:84:9:84:22 | u == undefined | This guard always evaluates to true. | +| tst.js:85:9:85:22 | n == undefined | This guard always evaluates to true. | +| tst.js:86:9:86:22 | o == undefined | This guard always evaluates to false. | +| tst.js:89:9:89:22 | u != undefined | This guard always evaluates to false. | +| tst.js:90:9:90:22 | n != undefined | This guard always evaluates to false. | +| tst.js:91:9:91:22 | o != undefined | This guard always evaluates to true. | +| tst.js:94:9:94:32 | typeof ... efined" | This guard always evaluates to true. | +| tst.js:95:9:95:32 | typeof ... efined" | This guard always evaluates to false. | +| tst.js:96:9:96:32 | typeof ... efined" | This guard always evaluates to false. | +| tst.js:100:5:100:27 | typeof ... nction" | This guard always evaluates to true. | +| tst.js:101:5:101:27 | typeof ... nction" | This guard always evaluates to false. | +| tst.js:114:5:114:15 | empty_array | This guard always evaluates to true. | +| tst.js:115:5:115:22 | pseudo_empty_array | This guard always evaluates to true. | +| tst.js:116:5:116:19 | non_empty_array | This guard always evaluates to true. | +| tst.js:124:6:124:20 | u !== undefined | This guard always evaluates to false. | +| tst.js:124:25:124:34 | u !== null | This guard always evaluates to true. | +| tst.js:125:5:125:19 | u !== undefined | This guard always evaluates to false. | +| tst.js:125:24:125:33 | u !== null | This guard always evaluates to true. | +| tst.js:127:5:127:18 | u != undefined | This guard always evaluates to false. | +| tst.js:127:23:127:31 | u != null | This guard always evaluates to false. | +| tst.js:127:23:127:31 | u != null | This guard always evaluates to true. | +| tst.js:128:5:128:18 | u == undefined | This guard always evaluates to true. | +| tst.js:128:23:128:31 | u == null | This guard always evaluates to false. | +| tst.js:128:23:128:31 | u == null | This guard always evaluates to true. | +| tst.js:129:5:129:19 | u !== undefined | This guard always evaluates to false. | +| tst.js:129:24:129:33 | u !== null | This guard always evaluates to true. | +| tst.js:130:5:130:22 | !(u === undefined) | This guard always evaluates to false. | +| tst.js:130:27:130:39 | !(u === null) | This guard always evaluates to true. | +| tst.js:131:5:131:19 | u === undefined | This guard always evaluates to true. | +| tst.js:131:24:131:33 | u === null | This guard always evaluates to false. | +| tst.js:132:7:132:21 | u === undefined | This guard always evaluates to true. | +| tst.js:132:26:132:35 | u === null | This guard always evaluates to false. | +| tst.js:133:5:133:22 | !(u === undefined) | This guard always evaluates to false. | +| tst.js:133:27:133:36 | u !== null | This guard always evaluates to true. | +| tst.js:135:5:135:18 | u == undefined | This guard always evaluates to true. | +| tst.js:135:23:135:31 | u == null | This guard always evaluates to true. | +| tst.js:138:24:138:33 | x === null | This guard always evaluates to false. | +| tst.js:140:13:140:22 | x === null | This guard always evaluates to false. | +| tst.js:156:23:156:31 | x != null | This guard always evaluates to true. | +| tst.js:158:13:158:21 | x != null | This guard always evaluates to true. | diff --git a/javascript/ql/test/query-tests/Expressions/UnneededDefensiveProgramming/UnneededDefensiveProgramming.qlref b/javascript/ql/test/query-tests/Expressions/UnneededDefensiveProgramming/UnneededDefensiveProgramming.qlref new file mode 100644 index 000000000000..13c05f1e74bd --- /dev/null +++ b/javascript/ql/test/query-tests/Expressions/UnneededDefensiveProgramming/UnneededDefensiveProgramming.qlref @@ -0,0 +1 @@ +Expressions/UnneededDefensiveProgramming.ql diff --git a/javascript/ql/test/query-tests/Expressions/UnneededDefensiveProgramming/UselessConditional.expected b/javascript/ql/test/query-tests/Expressions/UnneededDefensiveProgramming/UselessConditional.expected new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/javascript/ql/test/query-tests/Expressions/UnneededDefensiveProgramming/UselessConditional.qlref b/javascript/ql/test/query-tests/Expressions/UnneededDefensiveProgramming/UselessConditional.qlref new file mode 100644 index 000000000000..d29916245d6e --- /dev/null +++ b/javascript/ql/test/query-tests/Expressions/UnneededDefensiveProgramming/UselessConditional.qlref @@ -0,0 +1 @@ +Statements/UselessConditional.ql \ No newline at end of file diff --git a/javascript/ql/test/query-tests/Expressions/UnneededDefensiveProgramming/global-module-definition.js b/javascript/ql/test/query-tests/Expressions/UnneededDefensiveProgramming/global-module-definition.js new file mode 100644 index 000000000000..8c61f38d1132 --- /dev/null +++ b/javascript/ql/test/query-tests/Expressions/UnneededDefensiveProgramming/global-module-definition.js @@ -0,0 +1,11 @@ +var Mod1; +(function (Mod1) { + Mod1.p = 42; +})(Mod1 || (Mod1 = {})); + +(function(){ + var Mod2; + (function (Mod2) { + Mod2.p = 42; + })(Mod2 || (Mod2 = {})); // NOT OK +}); diff --git a/javascript/ql/test/query-tests/Expressions/UnneededDefensiveProgramming/module-environment-detection.js b/javascript/ql/test/query-tests/Expressions/UnneededDefensiveProgramming/module-environment-detection.js new file mode 100644 index 000000000000..913684d6f6aa --- /dev/null +++ b/javascript/ql/test/query-tests/Expressions/UnneededDefensiveProgramming/module-environment-detection.js @@ -0,0 +1,24 @@ +var _ = (function() { + if (typeof exports !== 'undefined') { + if (typeof module !== 'undefined' && module.exports) { + exports = module.exports = _; + } + exports._ = _; + } + return { + define: function(name, factory) { + } + }; +})(this); + +if (typeof exports !== 'undefined') { + if (typeof module !== 'undefined' && module.exports) { + exports = module.exports = emmet; + } + exports.emmet = emmet; +} + +(function(){ + var module; + if(typeof module === 'undefined'); // NOT OK +}); diff --git a/javascript/ql/test/query-tests/Expressions/UnneededDefensiveProgramming/tst.js b/javascript/ql/test/query-tests/Expressions/UnneededDefensiveProgramming/tst.js new file mode 100644 index 000000000000..1e03c4e3ba1a --- /dev/null +++ b/javascript/ql/test/query-tests/Expressions/UnneededDefensiveProgramming/tst.js @@ -0,0 +1,174 @@ +(function(){ + var v; + var u = undefined; + var n = null; + var o = {}; + var x = functionWithUnknownReturnValue(); + + var u_ = u; + var n_ = n; + var o_ = o; + var x_ = x; + + u_ = u_ || e; // NOT OK + n_ = n_ || e; // NOT OK + o_ = o_ || e; // NOT OK + x_ = x_ || e; + + u && u.p; // NOT OK + n && n.p; // NOT OK + o && o.p; // NOT OK + x && x.p; + + u && u(); // NOT OK + n && n(); // NOT OK + o && o(); // NOT OK + x && x(); + + !u || u.p; // NOT OK + !n || n.p; // NOT OK + !o || o.p; // NOT OK + !x || x.p; + + !!u && u.p; // NOT OK + !!n && n.p; // NOT OK + !!o && o.p; // NOT OK + !!x && x.p; + + u != undefined && u.p; // NOT OK + n != undefined && n.p; // NOT OK + o != undefined && o.p; // NOT OK + x != undefined && x.p; + + u == undefined || u.p; // NOT OK + n == undefined || n.p; // NOT OK + o == undefined || o.p; // NOT OK + x == undefined || x.p; + + u === undefined || u.p; // NOT OK + n === undefined || n.p; // NOT OK + o === undefined || o.p; // NOT OK + x === undefined || x.p; + + if (u) { // NOT OK + u.p; + } + if (n) { // NOT OK + n.p; + } + if (o) { // NOT OK + o.p; + } + if (x) { + x.p; + } + + u? u():_; // NOT OK + n? n(): _; // NOT OK + o? o(): _; // NOT OK + x? x(): _; + + if (u !== undefined) { // NOT OK + u.p; + } + if (n !== undefined) { // NOT OK + n.p; + } + if (o !== undefined) { // NOT OK + o.p; + } + if (x !== undefined) { + x.p; + } + + if (u == undefined){} // NOT OK + if (n == undefined){} // NOT OK + if (o == undefined){} // NOT OK + if (x == undefined){} + + if (u != undefined){} // NOT OK + if (n != undefined){} // NOT OK + if (o != undefined){} // NOT OK + if (x != undefined){} + + if (typeof u === "undefined"){} // NOT OK + if (typeof n === "undefined"){} // NOT OK + if (typeof o === "undefined"){} // NOT OK + if (typeof x === "undefined"){} + + function f() { } + typeof f === "function" && f(); // NOT OK + typeof u === "function" && u(); // NOT OK + typeof x === "function" && x(); + + var empty_array = []; + var pseudo_empty_array = ['']; + var non_empty_array = ['foo']; + var empty_string = ""; + var non_empty_string = "foo"; + var zero = 0; + var neg = -1; + var _true = true; + var _false = false; + + empty_array && empty_array.pop(); // NOT OK + pseudo_empty_array && pseudo_empty_array.pop(); // NOT OK + non_empty_array && non_empty_array.pop(); // NOT OK + empty_string && empty_string.charAt(0); + non_empty_string && non_empty_string.charAt(0); + zero && zero(); + neg && neg(); + _true && _true(); + _false && _false(); + + (u !== undefined && u !== null) && u.p; // NOT OK + u !== undefined && u !== null && u.p; // NOT OK + + u != undefined && u != null; // NOT OK + u == undefined || u == null; // NOT OK + u !== undefined && u !== null; // NOT OK + !(u === undefined) && !(u === null); // NOT OK + u === undefined || u === null; // NOT OK + !(u === undefined || u === null); // NOT OK + !(u === undefined) && u !== null; // NOT OK + u !== undefined && n !== null; + u == undefined && u == null; // NOT OK + x == undefined && x == null; + + x === undefined && x === null; // NOT OK + if (x === undefined) { + if (x === null) { // NOT OK + } + } + + x !== undefined && x !== null; + if (x !== undefined) { + if (x !== null) { + } + } + + x == undefined && x == null; + if (x == undefined) { + if (x == null) { + } + } + + x != undefined && x != null; // NOT OK + if (x != undefined) { + if (x != null) { // NOT OK + } + } + + if (typeof x !== undefined); + if (typeof window !== undefined); + if (typeof x !== x); + if (typeof x !== u); // NOT OK + + if (typeof window !== "undefined"); + if (typeof module !== "undefined"); + if (typeof global !== "undefined"); + + if (typeof window !== "undefined" && window.document); + if (typeof module !== "undefined" && module.exports); + if (typeof global !== "undefined" && global.process); +}); diff --git a/javascript/ql/test/query-tests/Expressions/UnneededDefensiveProgramming/tst2.js b/javascript/ql/test/query-tests/Expressions/UnneededDefensiveProgramming/tst2.js new file mode 100644 index 000000000000..4c6ad02a6bfb --- /dev/null +++ b/javascript/ql/test/query-tests/Expressions/UnneededDefensiveProgramming/tst2.js @@ -0,0 +1,10 @@ +(function(){ + var v; + (function(){ + if(typeof v === "undefined"){ // NOT OK + v = 42; + } + for(var v in x){ + } + }); +});