Merge pull request #20178 from Napalys/java/visible-for-testing-abuse

Java: Added new query `java/visible-for-testing-abuse`

java/ql/integration-tests/java/query-suite/java-code-quality-extended.qls.expected

@@ -73,6 +73,7 @@ ql/java/ql/src/Violations of Best Practice/Exception Handling/IgnoreExceptionalR
 ql/java/ql/src/Violations of Best Practice/Exception Handling/NumberFormatException.ql
 ql/java/ql/src/Violations of Best Practice/Implementation Hiding/AbstractToConcreteCollection.ql
 ql/java/ql/src/Violations of Best Practice/Implementation Hiding/ExposeRepresentation.ql
+ql/java/ql/src/Violations of Best Practice/Implementation Hiding/VisibleForTestingAbuse.ql
 ql/java/ql/src/Violations of Best Practice/Naming Conventions/AmbiguousOuterSuper.ql
 ql/java/ql/src/Violations of Best Practice/Naming Conventions/ConfusingMethodNames.ql
 ql/java/ql/src/Violations of Best Practice/Naming Conventions/ConfusingOverloading.ql

java/ql/integration-tests/java/query-suite/java-code-quality.qls.expected

@@ -71,6 +71,7 @@ ql/java/ql/src/Violations of Best Practice/Exception Handling/IgnoreExceptionalR
 ql/java/ql/src/Violations of Best Practice/Exception Handling/NumberFormatException.ql
 ql/java/ql/src/Violations of Best Practice/Implementation Hiding/AbstractToConcreteCollection.ql
 ql/java/ql/src/Violations of Best Practice/Implementation Hiding/ExposeRepresentation.ql
+ql/java/ql/src/Violations of Best Practice/Implementation Hiding/VisibleForTestingAbuse.ql
 ql/java/ql/src/Violations of Best Practice/Naming Conventions/AmbiguousOuterSuper.ql
 ql/java/ql/src/Violations of Best Practice/Naming Conventions/ConfusingMethodNames.ql
 ql/java/ql/src/Violations of Best Practice/Naming Conventions/ConfusingOverloading.ql

java/ql/src/Violations of Best Practice/Implementation Hiding/VisibleForTestingAbuse.md

@@ -0,0 +1,36 @@
+## Overview
+
+Accessing class members annotated with `@VisibleForTesting` from production code goes against the intention of the annotation and may indicate programmer error.
+
+The `@VisibleForTesting` annotation serves to increase visibility of methods, fields or classes for the purposes of testing. Accessing these annotated elements in production code (not test code) abuses the intention of the annotation.
+
+## Recommendation
+
+Only access methods, fields or classes annotated with `@VisibleForTesting` from test code. If the visibility of the methods, fields or classes should generally be relaxed, use Java language access modifiers.
+
+## Example
+
+```java
+public class Annotated {
+    @VisibleForTesting static int f() { return 42; }
+}
+
+/* src/test/java/Test.java */
+int i = Annotated.f(); // COMPLIANT
+
+/* src/main/Source.java */
+int i = Annotated.f(); // NON_COMPLIANT
+```
+
+## Implementation notes
+
+This rule alerts on any implementation of the annotation `VisibleForTesting`, regardless of where it is provided from.
+
+The rule also uses the following logic to determine what an abuse of the annotation is:
+
+  1. If a public or protected member/type is annotated with `@VisibleForTesting`, it's assumed that package-private access is enough for production code. Therefore the rule alerts when a public or protected member/type annotated with `@VisibleForTesting` is used outside of its declaring package.
+  2. If a package-private member/type is annotated with `@VisibleForTesting`, it's assumed that private access is enough for production code. Therefore the rule alerts when a package-private member/type annotated with `@VisibleForTesting` is used outside its declaring class.
+
+## References
+- Javadoc: [AssertJ VisibleForTesting](https://javadoc.io/doc/org.assertj/assertj-core/latest/org.assertj.core/org/assertj/core/util/VisibleForTesting.html).
+- Javadoc: [JetBrains VisibleForTesting](https://javadoc.io/doc/org.jetbrains/annotations/22.0.0/org/jetbrains/annotations/VisibleForTesting.html).

java/ql/src/Violations of Best Practice/Implementation Hiding/VisibleForTestingAbuse.ql

@@ -0,0 +1,112 @@
+/**
+ * @id java/visible-for-testing-abuse
+ * @name Use of VisibleForTesting in production code
+ * @description Accessing methods, fields or classes annotated with `@VisibleForTesting` from
+ *              production code goes against the intention of the annotation and may indicate
+ *              programmer error.
+ * @kind problem
+ * @precision high
+ * @problem.severity warning
+ * @tags quality
+ *       maintainability
+ *       readability
+ */
+
+import java
+
+/**
+ * Holds if a `Callable` is within the same type hierarchy as `RefType`
+ * (including through lambdas, inner classes, and outer classes).
+ */
+predicate isWithinType(Callable c, RefType t) {
+  // Either the callable is in the target type, or they share a common enclosing type
+  c.getDeclaringType().getEnclosingType*() = t.getEnclosingType*()
+}
+
+/**
+ * Holds if `e` is within the same package as `t`.
+ */
+predicate isWithinPackage(Expr e, RefType t) {
+  e.getCompilationUnit().getPackage() = t.getPackage()
+}
+
+/**
+ * Holds if a callable or any of its enclosing callables is annotated with @VisibleForTesting.
+ */
+predicate isWithinVisibleForTestingContext(Callable c) {
+  c.getAnAnnotation().getType().hasName("VisibleForTesting")
+  or
+  isWithinVisibleForTestingContext(c.getEnclosingCallable())
+}
+
+/**
+ * Holds if `e` is within a test method context, including lambda expressions
+ * within test methods and nested lambdas.
+ */
+private predicate isWithinTest(Expr e) {
+  e.getEnclosingCallable() instanceof LikelyTestMethod
+  or
+  exists(Method lambda, LambdaExpr lambdaExpr |
+    lambda = lambdaExpr.asMethod() and
+    lambda.getEnclosingCallable*() instanceof LikelyTestMethod and
+    e.getEnclosingCallable() = lambda
+  )
+}
+
+from Annotatable annotated, Expr e
+where
+  annotated.getAnAnnotation().getType().hasName("VisibleForTesting") and
+  (
+    // field access
+    e =
+      any(FieldAccess v |
+        v.getField() = annotated and
+        // depending on the visibility of the field, using the annotation to abuse the visibility may/may not be occurring
+        (
+          // if its package protected report when its used outside its class because it should have been private (class only permitted)
+          v.getField().isPackageProtected() and
+          not isWithinType(v.getEnclosingCallable(), v.getField().getDeclaringType())
+          or
+          // if public or protected report when its used outside its package because package protected should have been enough (package only permitted)
+          (v.getField().isPublic() or v.getField().isProtected()) and
+          not isWithinPackage(v, v.getField().getDeclaringType())
+        )
+      )
+    or
+    // method access
+    e =
+      any(MethodCall c |
+        c.getMethod() = annotated and
+        // depending on the visibility of the method, using the annotation to abuse the visibility may/may not be occurring
+        (
+          // if its package protected report when its used outside its class because it should have been private (class only permitted)
+          c.getMethod().isPackageProtected() and
+          not isWithinType(c.getEnclosingCallable(), c.getMethod().getDeclaringType())
+          or
+          // if public or protected report when its used outside its package because package protected should have been enough (package only permitted)
+          (c.getMethod().isPublic() or c.getMethod().isProtected()) and
+          not isWithinPackage(c, c.getMethod().getDeclaringType())
+        )
+      )
+    or
+    // Class instantiation - report if used outside appropriate scope
+    e =
+      any(ClassInstanceExpr c |
+        c.getConstructedType() = annotated and
+        (
+          c.getConstructedType().isPublic() and not isWithinPackage(c, c.getConstructedType())
+          or
+          c.getConstructedType().hasNoModifier() and
+          c.getConstructedType() instanceof NestedClass and
+          not isWithinType(c.getEnclosingCallable(), c.getConstructedType())
+        )
+      )
+  ) and
+  // not in a test where use is appropriate
+  not isWithinTest(e) and
+  // not when the accessing method or any enclosing method is @VisibleForTesting (test-to-test communication)
+  not isWithinVisibleForTestingContext(e.getEnclosingCallable()) and
+  // not when used in annotation contexts
+  not e.getParent*() instanceof Annotation
+select e, "Access of $@ annotated with VisibleForTesting found in production code.", annotated,
+  "element"

java/ql/test/query-tests/VisibleForTestingAbuse/VisibleForTestingAbuse.expected

@@ -0,0 +1,17 @@
+| packageone/SourcePackage.java:9:21:9:32 | Annotated.m1 | Access of $@ annotated with VisibleForTesting found in production code. | packagetwo/Annotated.java:9:29:9:30 | m1 | element |
+| packageone/SourcePackage.java:10:21:10:32 | Annotated.m2 | Access of $@ annotated with VisibleForTesting found in production code. | packagetwo/Annotated.java:11:26:11:27 | m2 | element |
+| packageone/SourcePackage.java:16:18:16:36 | fPublic(...) | Access of $@ annotated with VisibleForTesting found in production code. | packagetwo/Annotated.java:26:23:26:29 | fPublic | element |
+| packageone/SourcePackage.java:17:18:17:39 | fProtected(...) | Access of $@ annotated with VisibleForTesting found in production code. | packagetwo/Annotated.java:31:26:31:35 | fProtected | element |
+| packageone/SourcePackage.java:25:31:25:42 | Annotated.m1 | Access of $@ annotated with VisibleForTesting found in production code. | packagetwo/Annotated.java:9:29:9:30 | m1 | element |
+| packageone/SourcePackage.java:26:31:26:42 | Annotated.m2 | Access of $@ annotated with VisibleForTesting found in production code. | packagetwo/Annotated.java:11:26:11:27 | m2 | element |
+| packageone/SourcePackage.java:29:28:29:46 | fPublic(...) | Access of $@ annotated with VisibleForTesting found in production code. | packagetwo/Annotated.java:26:23:26:29 | fPublic | element |
+| packageone/SourcePackage.java:30:28:30:49 | fProtected(...) | Access of $@ annotated with VisibleForTesting found in production code. | packagetwo/Annotated.java:31:26:31:35 | fProtected | element |
+| packageone/SourcePackage.java:34:23:34:34 | Annotated.m1 | Access of $@ annotated with VisibleForTesting found in production code. | packagetwo/Annotated.java:9:29:9:30 | m1 | element |
+| packageone/SourcePackage.java:35:30:35:41 | Annotated.m1 | Access of $@ annotated with VisibleForTesting found in production code. | packagetwo/Annotated.java:9:29:9:30 | m1 | element |
+| packageone/SourcePackage.java:36:31:36:42 | Annotated.m1 | Access of $@ annotated with VisibleForTesting found in production code. | packagetwo/Annotated.java:9:29:9:30 | m1 | element |
+| packageone/SourcePackage.java:37:33:37:44 | Annotated.m1 | Access of $@ annotated with VisibleForTesting found in production code. | packagetwo/Annotated.java:9:29:9:30 | m1 | element |
+| packagetwo/Source.java:8:20:8:30 | Annotated.m | Access of $@ annotated with VisibleForTesting found in production code. | packagetwo/Annotated.java:7:19:7:19 | m | element |
+| packagetwo/Source.java:14:17:14:29 | f(...) | Access of $@ annotated with VisibleForTesting found in production code. | packagetwo/Annotated.java:16:16:16:16 | f | element |
+| packagetwo/Source.java:20:28:20:47 | new AnnotatedClass(...) | Access of $@ annotated with VisibleForTesting found in production code. | packageone/AnnotatedClass.java:4:14:4:27 | AnnotatedClass | element |
+| packagetwo/Source.java:24:30:24:40 | Annotated.m | Access of $@ annotated with VisibleForTesting found in production code. | packagetwo/Annotated.java:7:19:7:19 | m | element |
+| packagetwo/Source.java:28:27:28:39 | f(...) | Access of $@ annotated with VisibleForTesting found in production code. | packagetwo/Annotated.java:16:16:16:16 | f | element |

java/ql/test/query-tests/VisibleForTestingAbuse/VisibleForTestingAbuse.qlref

@@ -0,0 +1,2 @@
+query: Violations of Best Practice/Implementation Hiding/VisibleForTestingAbuse.ql
+postprocess: utils/test/InlineExpectationsTestQuery.ql

java/ql/test/query-tests/VisibleForTestingAbuse/packageone/AnnotatedClass.java

@@ -0,0 +1,6 @@
+package packageone;
+
+@VisibleForTesting
+public class AnnotatedClass {
+    public AnnotatedClass() {}
+}

java/ql/test/query-tests/VisibleForTestingAbuse/packageone/SourcePackage.java

@@ -0,0 +1,38 @@
+package packageone;
+
+import packagetwo.Annotated;
+
+public class SourcePackage extends Annotated {
+    void f() {
+        // Fields - cross-package access (only accessible ones)
+        // String s = Annotated.m; // Cannot access package-private from different package
+        String s1 = Annotated.m1; // $ Alert
+        String s2 = Annotated.m2; // $ Alert
+        // String s3 = Annotated.m3; // Cannot access private field
+        
+        // Methods - cross-package access (only accessible ones)
+        // int i = Annotated.f(); // Cannot access package-private from different package
+        // int i1 = Annotated.fPrivate(); // Cannot access private method
+        int i2 = Annotated.fPublic(); // $ Alert
+        int i3 = Annotated.fProtected(); // $ Alert
+        
+        // Same package class
+        AnnotatedClass a = new AnnotatedClass(); // COMPLIANT - same package
+        
+        // Lambda usage - cross-package (only accessible members)
+        Runnable lambda = () -> {
+            // String lambdaS = Annotated.m; // Cannot access package-private
+            String lambdaS1 = Annotated.m1; // $ Alert
+            String lambdaS2 = Annotated.m2; // $ Alert
+            
+            // int lambdaI = Annotated.f(); // Cannot access package-private
+            int lambdaI2 = Annotated.fPublic(); // $ Alert
+            int lambdaI3 = Annotated.fProtected(); // $ Alert
+        };
+        lambda.run();
+    }
+    String myField1 = Annotated.m1; // $ Alert
+    public String myField2 = Annotated.m1; // $ Alert
+    private String myField3 = Annotated.m1; // $ Alert
+    protected String myField4 = Annotated.m1; // $ Alert
+}

java/ql/test/query-tests/VisibleForTestingAbuse/packageone/SourcePackage1.java

@@ -0,0 +1,22 @@
+package packageone;
+
+import packagetwo.Annotated;
+
+public class SourcePackage1 extends Annotated {
+    @VisibleForTesting
+    public void f() {
+
+        String s1 = Annotated.m1;
+        String s2 = Annotated.m2;
+
+        int i2 = Annotated.fPublic();
+        int i3 = Annotated.fProtected();
+
+        Runnable lambda = () -> {
+            String lambdaS1 = Annotated.m1;
+            String lambdaS2 = Annotated.m2;
+            int lambdaI2 = Annotated.fPublic();
+            int lambdaI3 = Annotated.fProtected();
+        };
+    }
+}

java/ql/test/query-tests/VisibleForTestingAbuse/packageone/VisibleForTesting.java

@@ -0,0 +1,4 @@
+package packageone;
+
+public @interface VisibleForTesting {
+}

java/ql/test/query-tests/VisibleForTestingAbuse/packagetwo/Annotated.java

@@ -0,0 +1,92 @@
+package packagetwo;
+
+import packageone.*;
+
+public class Annotated {
+    @VisibleForTesting
+    static String m;
+    @VisibleForTesting
+    static protected String m1;
+    @VisibleForTesting
+    static public String m2;
+    @VisibleForTesting
+    static private String m3;
+
+    @VisibleForTesting
+    static int f() {
+        return 1;
+    }
+
+    @VisibleForTesting
+    static private int fPrivate() {
+        return 1;
+    }
+
+    @VisibleForTesting
+    static public int fPublic() {
+        return 1;
+    }
+
+    @VisibleForTesting
+    static protected int fProtected() {
+        return 1;
+    }
+
+    private static void resetPriorities() {
+        String priority = m;
+        String priority1 = m1;
+        String priority2 = m2;
+        String priority3 = m3;
+
+        int result = f();
+        int resultPrivate = fPrivate();
+        int resultPublic = fPublic();
+        int resultProtected = fProtected();
+    }
+
+    private static void resetPriorities2() {
+        Runnable task = () -> {
+            String priority = m;
+            String priority1 = m1;
+            String priority2 = m2;
+            String priority3 = m3;
+
+            int result = f();
+            int resultPrivate = fPrivate();
+            int resultPublic = fPublic();
+            int resultProtected = fProtected();
+        };
+        task.run();
+    }
+
+    private static class InnerClass {
+        void useVisibleForMembers() {
+            String field = m;
+            String field1 = m1;
+            String field2 = m2;
+            String field3 = m3;
+
+            int method = f();
+            int methodPrivate = fPrivate();
+            int methodPublic = fPublic();
+            int methodProtected = fProtected();
+        }
+    }
+
+    @VisibleForTesting
+    static class InnerTestClass {
+        @VisibleForTesting
+        int getSize() {
+            return 42;
+        }
+        
+        @VisibleForTesting
+        private String data;
+    }
+    
+    private void useInnerClass() {
+        InnerTestClass inner = new InnerTestClass();
+        int size = inner.getSize();
+        String value = inner.data;
+    }
+}

java/ql/test/query-tests/VisibleForTestingAbuse/packagetwo/Source.java

@@ -0,0 +1,34 @@
+package packagetwo;
+
+import packageone.*;
+
+public class Source {
+    void f() {
+        // Fields
+        String s = Annotated.m; // $ Alert
+        String s1 = Annotated.m1; // COMPLIANT - same package
+        String s2 = Annotated.m2;
+        // String s3 = Annotated.m3; // Cannot access private field
+        
+        // Methods
+        int i = Annotated.f(); // $ Alert
+        // int i1 = Annotated.fPrivate(); // Cannot access private method
+        int i2 = Annotated.fPublic();
+        int i3 = Annotated.fProtected();
+        
+        // Other class
+        AnnotatedClass a = new AnnotatedClass(); // $ Alert
+        
+        // Lambda usage
+        Runnable lambda = () -> {
+            String lambdaS = Annotated.m; // $ Alert
+            String lambdaS1 = Annotated.m1;
+            String lambdaS2 = Annotated.m2;
+            
+            int lambdaI = Annotated.f(); // $ Alert
+            int lambdaI2 = Annotated.fPublic();
+            int lambdaI3 = Annotated.fProtected();
+        };
+        lambda.run();
+    }
+}

java/ql/test/query-tests/VisibleForTestingAbuse/packagetwo/Test.java

@@ -0,0 +1,34 @@
+package packagetwo;
+
+import packageone.*;
+
+public class Test {
+    void f() {
+        // Fields
+        String s = Annotated.m; // COMPLIANT
+        String s1 = Annotated.m1; // COMPLIANT
+        String s2 = Annotated.m2; // COMPLIANT
+        // String s3 = Annotated.m3; // Cannot access private field
+        
+        // Methods
+        int i = Annotated.f(); // COMPLIANT
+        // int i1 = Annotated.fPrivate(); // Cannot access private method
+        int i2 = Annotated.fPublic(); // COMPLIANT
+        int i3 = Annotated.fProtected(); // COMPLIANT
+        
+        // Other class
+        AnnotatedClass a = new AnnotatedClass(); // COMPLIANT
+        
+        // Lambda usage
+        Runnable lambda = () -> {
+            String lambdaS = Annotated.m; // COMPLIANT
+            String lambdaS1 = Annotated.m1; // COMPLIANT
+            String lambdaS2 = Annotated.m2; // COMPLIANT
+            
+            int lambdaI = Annotated.f(); // COMPLIANT
+            int lambdaI2 = Annotated.fPublic(); // COMPLIANT
+            int lambdaI3 = Annotated.fProtected(); // COMPLIANT
+        };
+        lambda.run();
+    }
+}

java/ql/test/query-tests/VisibleForTestingAbuse/packagetwo/UseWithinAnnotation.java

@@ -0,0 +1,18 @@
+package packagetwo;
+
+import packageone.*;
+
+@interface Range {
+    int min() default 0;
+    int max() default 100;
+}
+
+public class UseWithinAnnotation {
+    @VisibleForTesting
+    static final int MAX_LISTING_LENGTH_MIN = 1;
+    @VisibleForTesting
+    static final int MAX_LISTING_LENGTH_MAX = 1000;
+
+    @Range(min = MAX_LISTING_LENGTH_MIN, max = MAX_LISTING_LENGTH_MAX)
+    private int maxListingLength = MAX_LISTING_LENGTH_MAX;
+}