Java: Added new query java/visible-for-testing-abuse#20178
Merged
Napalys merged 22 commits intogithub:mainfrom Aug 29, 2025
Merged
Java: Added new query java/visible-for-testing-abuse#20178Napalys merged 22 commits intogithub:mainfrom
java/visible-for-testing-abuse#20178Napalys merged 22 commits intogithub:mainfrom
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a new CodeQL query java/visible-for-testing-abuse to detect misuse of the @VisibleForTesting annotation in production code. The query identifies cases where elements annotated with @VisibleForTesting are accessed from production code outside their intended scope, which violates the annotation's purpose of only increasing visibility for testing.
- Added the main query implementation with logic to detect inappropriate access patterns based on visibility modifiers
- Created comprehensive test cases covering various scenarios including cross-package access, lambda usage, and inner classes
- Integrated the query into the Java code quality query suites
Reviewed Changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
java/ql/src/Violations of Best Practice/Implementation Hiding/VisibleForTestingAbuse.ql |
Main query implementation detecting misuse of @VisibleForTesting annotation |
java/ql/src/Violations of Best Practice/Implementation Hiding/VisibleForTestingAbuse.md |
Documentation explaining the rule's purpose and implementation details |
java/ql/test/query-tests/VisibleForTestingAbuse/*.java |
Comprehensive test cases covering various usage scenarios |
java/ql/test/query-tests/VisibleForTestingAbuse/VisibleForTestingAbuse.qlref |
Test configuration referencing the query |
java/ql/test/query-tests/VisibleForTestingAbuse/VisibleForTestingAbuse.expected |
Expected test results |
java/ql/integration-tests/java/query-suite/*.expected |
Updated query suite integration test expectations |
Napalys
force-pushed
the
java/visible-for-testing-abuse
branch
from
b2ba0d7 to
8708130
Compare
Contributor
michaelnebel
left a comment
michaelnebel
left a comment
There was a problem hiding this comment.
Neat new query.
I have added some comments, will continue the review tomorrow.
…sibleForTestingAbuse alerts
…VisibleForTestingAbuse.ql Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…VisibleForTestingAbuse.ql Co-authored-by: Michael Nebel <michaelnebel@github.com>
Napalys
force-pushed
the
java/visible-for-testing-abuse
branch
from
bb37c26 to
4705ad2
Compare
| // not in a test where use is appropriate | ||
| not e.getEnclosingCallable() instanceof LikelyTestMethod and | ||
| // not when the accessing method or any enclosing method is @VisibleForTesting (test-to-test communication) | ||
| not isWithinVisibleForTestingContext(e.getEnclosingCallable()) and |
Contributor
There was a problem hiding this comment.
Maybe also generalise this to support lambdas (to mirror the above).
Contributor
Author
There was a problem hiding this comment.
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
Contributor
There was a problem hiding this comment.
Oh - right! Then it is already covered!
…t tests from the `java/visible-for-testing-abuse` query.
michaelnebel
previously approved these changes
Aug 25, 2025
Contributor
michaelnebel
left a comment
michaelnebel
left a comment
There was a problem hiding this comment.
Great work!
Maybe run DCA once more.
owen-mc
reviewed
Aug 28, 2025
Contributor
owen-mc
left a comment
owen-mc
left a comment
There was a problem hiding this comment.
Looks good. Just a few minor comments.
…VisibleForTestingAbuse.ql Co-authored-by: Owen Mansel-Chan <62447351+owen-mc@users.noreply.github.com>
…VisibleForTestingAbuse.md Co-authored-by: Owen Mansel-Chan <62447351+owen-mc@users.noreply.github.com>
Contributor
|
(Don't forget to add full stops to the end of all the QLDocs.) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Added new query
java/visible-for-testing-abuse.Initial MRVA 1000 run produced 5,108 results, after reducing false positives ended up with 2,611.
Autofix tends to remove
@VisibleForTesting, but it may requires deeper refactoring in which case it may not be accepted as a good fix.