From 44fe49469b9bc34be35f2cab43940b1b06119a11 Mon Sep 17 00:00:00 2001 From: Marcono1234 Date: Thu, 18 Jul 2024 23:09:09 +0200 Subject: [PATCH] Extend assertion-arguments-switched.ql to cover more constant expressions --- .../assertion-arguments-constant.ql | 2 +- .../assertion-arguments-switched.ql | 59 ++++++++++++++++--- .../queries/unit-tests/lib/AssertLib.qll | 23 ++++++-- 3 files changed, 71 insertions(+), 13 deletions(-) diff --git a/codeql-custom-queries-java/queries/unit-tests/assertion-arguments-constant.ql b/codeql-custom-queries-java/queries/unit-tests/assertion-arguments-constant.ql index 5011390..f689db0 100644 --- a/codeql-custom-queries-java/queries/unit-tests/assertion-arguments-constant.ql +++ b/codeql-custom-queries-java/queries/unit-tests/assertion-arguments-constant.ql @@ -16,6 +16,6 @@ from MethodAccess assertCall, AssertMethod assertMethod where assertMethod = assertCall.getMethod() and forex(int inputParamIndex | inputParamIndex = assertMethod.getAnInputParamIndex() | - assertCall.getArgument(inputParamIndex) instanceof CompileTimeConstantOrLiteral + assertCall.getArgument(inputParamIndex) instanceof ConstantExpr ) select assertCall, "Assertion will always have the same outcome" diff --git a/codeql-custom-queries-java/queries/unit-tests/assertion-arguments-switched.ql b/codeql-custom-queries-java/queries/unit-tests/assertion-arguments-switched.ql index 3b65847..cb65f7f 100644 --- a/codeql-custom-queries-java/queries/unit-tests/assertion-arguments-switched.ql +++ b/codeql-custom-queries-java/queries/unit-tests/assertion-arguments-switched.ql @@ -9,15 +9,60 @@ * assertEquals(actual, "expected"); * ``` * SonarSource rule: [RSPEC-3415](https://rules.sonarsource.com/java/RSPEC-3415) + * + * @kind problem + * @id TODO */ - import java - import lib.AssertLib +import java +import lib.AssertLib - from MethodAccess assertCall, AssertTwoArgumentsMethod assertMethod +/** + * Expression which seems to be intended as 'expected' argument for an assertion. + */ +class ExpectedValueExpr extends Expr { + ExpectedValueExpr() { + // Note: Can cause false positives if the value of a constant (= 'actual') defined in the same + // project is being checked, but on the other hand there are also cases where the constant is + // is intended as 'expected' value + this instanceof ConstantExpr + or + exists(MethodAccess call, Method m | + call = this and + m = call.getMethod() and + m.isStatic() and + forall(Expr arg | arg = call.getAnArgument() | arg instanceof ExpectedValueExpr) and + // Only consider methods from third-party libraries or JDK, otherwise intention might be to + // test result of method + not m.getSourceDeclaration().fromSource() + ) + or + exists(ClassInstanceExpr newExpr | + newExpr = this and + forall(Expr arg | arg = newExpr.getAnArgument() | arg instanceof ExpectedValueExpr) + // Do not exclude types declared in same project, assume that `newExpr` is always the 'expected' value + // If `newExpr` is intended as 'actual' then this might be misuse of `assertEquals` for `equals` + // implementation check (which is discouraged) + ) + or + exists(ArrayCreationExpr newArray | newArray = this | + // Either creates array with constant dimensions + forex(Expr dimExpr | dimExpr = newArray.getADimension() | + dimExpr instanceof ExpectedValueExpr + ) + or + // Or with init containing constants (checked transitively) + forex(Expr initValue | initValue = newArray.getInit().getAnInit+() | + initValue instanceof ArrayInit or initValue instanceof ExpectedValueExpr + ) + ) + } +} + +from MethodAccess assertCall, AssertTwoArgumentsMethod assertMethod where - assertMethod = assertCall.getMethod() - and assertCall.getArgument(assertMethod.getAssertionParamIndex()) instanceof CompileTimeConstantOrLiteral - // Ignore if both arguments are constant, that is already detected by separate query - and not assertCall.getArgument(assertMethod.getFixedParamIndex()) instanceof CompileTimeConstantOrLiteral + assertMethod = assertCall.getMethod() and + assertCall.getArgument(assertMethod.getAssertionParamIndex()) instanceof ExpectedValueExpr and + // Ignore if both arguments are constant, that is already detected by separate query + not assertCall.getArgument(assertMethod.getFixedParamIndex()) instanceof ExpectedValueExpr select assertCall, "Assertion arguments are switched" diff --git a/codeql-custom-queries-java/queries/unit-tests/lib/AssertLib.qll b/codeql-custom-queries-java/queries/unit-tests/lib/AssertLib.qll index 4eb1dad..714f895 100644 --- a/codeql-custom-queries-java/queries/unit-tests/lib/AssertLib.qll +++ b/codeql-custom-queries-java/queries/unit-tests/lib/AssertLib.qll @@ -200,13 +200,26 @@ abstract class AssertThrowsMethod extends AssertMethod { } /** - * A compile time constant expression or any literal. + * An expression with constant value. */ -class CompileTimeConstantOrLiteral extends Expr { - CompileTimeConstantOrLiteral() { +class ConstantExpr extends Expr { + ConstantExpr() { // CompileTimeConstantExpr does not include NullLiteral and TypeLiteral this instanceof CompileTimeConstantExpr - or this instanceof NullLiteral - or this instanceof TypeLiteral + or + this instanceof NullLiteral + or + this instanceof TypeLiteral + or + this.(FieldRead).getField() instanceof EnumConstant + or + exists(Field f | + f = this.(FieldRead).getField() and + f.isStatic() and + f.isFinal() and + // And field is declared in third-party library or JDK, otherwise test might intentionally + // use constant as 'actual' argument to check its value + not f.fromSource() + ) } }