Java: Added new query java/visible-for-testing-abuse
#20178
Conversation
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
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
java/ql/src/Violations of Best Practice/Implementation Hiding/VisibleForTestingAbuse.ql
Fixed
Show fixed
Hide fixed
java/ql/src/Violations of Best Practice/Implementation Hiding/VisibleForTestingAbuse.ql
Fixed
Show fixed
Hide fixed
java/ql/src/Violations of Best Practice/Implementation Hiding/VisibleForTestingAbuse.ql
Fixed
Show fixed
Hide fixed
java/ql/src/Violations of Best Practice/Implementation Hiding/VisibleForTestingAbuse.ql
Fixed
Show fixed
Hide fixed
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 |
java/ql/src/Violations of Best Practice/Implementation Hiding/VisibleForTestingAbuse.ql
Outdated
Show resolved
Hide resolved
java/ql/src/Violations of Best Practice/Implementation Hiding/VisibleForTestingAbuse.ql
Outdated
Show resolved
Hide resolved
Napalys
force-pushed
the
java/visible-for-testing-abuse
branch
from
b2ba0d7 to
8708130
Compare
java/ql/src/Violations of Best Practice/Implementation Hiding/VisibleForTestingAbuse.ql
Outdated
Show resolved
Hide resolved
java/ql/src/Violations of Best Practice/Implementation Hiding/VisibleForTestingAbuse.ql
Outdated
Show resolved
Hide resolved
java/ql/src/Violations of Best Practice/Implementation Hiding/VisibleForTestingAbuse.ql
Outdated
Show resolved
Hide resolved
java/ql/src/Violations of Best Practice/Implementation Hiding/VisibleForTestingAbuse.ql
Outdated
Show resolved
Hide resolved
java/ql/src/Violations of Best Practice/Implementation Hiding/VisibleForTestingAbuse.ql
Outdated
Show resolved
Hide resolved
java/ql/src/Violations of Best Practice/Implementation Hiding/VisibleForTestingAbuse.ql
Outdated
Show resolved
Hide resolved
java/ql/src/Violations of Best Practice/Implementation Hiding/VisibleForTestingAbuse.ql
Outdated
Show resolved
Hide resolved
…sibleForTestingAbuse alerts
…VisibleForTestingAbuse.ql Co-authored-by: Copilot <[email protected]>
…VisibleForTestingAbuse.ql Co-authored-by: Michael Nebel <[email protected]>
Napalys
force-pushed
the
java/visible-for-testing-abuse
branch
from
bb37c26 to
4705ad2
Compare
java/ql/src/Violations of Best Practice/Implementation Hiding/VisibleForTestingAbuse.ql
Outdated
Show resolved
Hide resolved
java/ql/src/Violations of Best Practice/Implementation Hiding/VisibleForTestingAbuse.ql
Outdated
Show resolved
Hide resolved
| // 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 |
There was a problem hiding this comment.
Maybe also generalise this to support lambdas (to mirror the above).
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
…t tests from the `java/visible-for-testing-abuse` query.
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.