Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 19 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
0c14d93
Java: Added new query `java/visible-for-testing-abuse`
Napalys Aug 6, 2025
652e9cb
Java: Added inline test expectations for `java/visible-for-testing-ab…
Napalys Aug 6, 2025
ff6ddd2
Java: Promoted `java/visible-for-testing-abuse` to quality
Napalys Aug 6, 2025
2a16f48
Java: Expanded test suite of `java/visible-for-testing-abuse`
Napalys Aug 6, 2025
fbf18af
Java: enchanced check if it is within same package
Napalys Aug 6, 2025
9dfb4d4
Java: Enchanced `isWithinType` to also include lambdas, inner classes…
Napalys Aug 6, 2025
7e2a194
Java: Fix Predicate QLDoc style.
Napalys Aug 6, 2025
1e2e6ec
Java: Test @VisibleForTesting method accessing @VisibleForTesting mem…
Napalys Aug 6, 2025
e404240
Java: Resolve spurious VisibleForTestingAbuse alerts for inner class …
Napalys Aug 6, 2025
225723b
Java: Exclude @VisibleForTesting-to-@VisibleForTesting access from Vi…
Napalys Aug 6, 2025
eb46e54
Java: Refactor VisibleForTestingAbuse query to reduce complexity
Napalys Aug 6, 2025
ea831a8
Java: Fix VisibleForTestingAbuse false positives in annotations
Napalys Aug 6, 2025
d20fd5b
Java: updated `visible-for-testing-abuse` meta data and docs.
Napalys Aug 6, 2025
0b17208
Update java/ql/src/Violations of Best Practice/Implementation Hiding/…
Napalys Aug 7, 2025
66f2911
Update java/ql/src/Violations of Best Practice/Implementation Hiding/…
Napalys Aug 22, 2025
38b3df0
Java: Address comments
Napalys Aug 22, 2025
4705ad2
Java: Added extra test cases for fields
Napalys Aug 22, 2025
4149968
Java: Remove the hardcoded path filter that excluded CodeQL's own uni…
Napalys Aug 24, 2025
38f517e
Java: Add lambda-aware test detection to VisibleForTesting query
Napalys Aug 24, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
@@ -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/util/VisibleForTesting.html).
- Javadoc: [JetBrains VisibleForTesting](https://javadoc.io/doc/org.jetbrains/annotations/22.0.0/org/jetbrains/annotations/VisibleForTesting.html).
Original file line number Diff line number Diff line change
@@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe also generalise this to support lambdas (to mirror the above).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it necessary, I can't seem to figure out which case would not be covered currently? As this is responsible for exclusion of such https://github.com/Napalys/codeql/blob/38f517ecfaaa3eb1d6b18d0969900aeaddfca420/java/ql/test/query-tests/VisibleForTestingAbuse/packageone/SourcePackage1.java#L16

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh - right! Then it is already covered!

// not when used in annotation contexts
not e.getParent*() instanceof Annotation
select e, "Access of $@ annotated with VisibleForTesting found in production code.", annotated,
"element"
Original file line number Diff line number Diff line change
@@ -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 |
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
query: Violations of Best Practice/Implementation Hiding/VisibleForTestingAbuse.ql
postprocess: utils/test/InlineExpectationsTestQuery.ql
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
package packageone;

@VisibleForTesting
public class AnnotatedClass {
public AnnotatedClass() {}
}
Original file line number Diff line number Diff line change
@@ -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
}
Original file line number Diff line number Diff line change
@@ -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();
};
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
package packageone;

public @interface VisibleForTesting {
}
Original file line number Diff line number Diff line change
@@ -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;
}
}
Original file line number Diff line number Diff line change
@@ -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();
}
}
Loading