Skip to content

SONARJAVA-6488 Implement new rule S8908: Methods annotated with @CacheResult should not return void#5687

Draft
romainbrenguier wants to merge 4 commits into
masterfrom
new-rule/SONARJAVA-6488-S8908
Draft

SONARJAVA-6488 Implement new rule S8908: Methods annotated with @CacheResult should not return void#5687
romainbrenguier wants to merge 4 commits into
masterfrom
new-rule/SONARJAVA-6488-S8908

Conversation

@romainbrenguier

@romainbrenguier romainbrenguier commented Jun 22, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Implement new rule S8908 to detect Quarkus @CacheResult annotation used on void methods
  • Update ruling baseline for S8899 in eclipse-jetty project
  • Address review feedback on RequestPathNormalizationCheck (S8899)

Changes

New Rule: S8908

Detects methods annotated with @CacheResult that return void. Since void methods have no return value to cache, applying @CacheResult to them is meaningless and indicates a misunderstanding of how caching works.

Rule details:

  • Type: CODE_SMELL
  • Severity: Minor
  • Impact: MAINTAINABILITY (LOW)
  • Fix time: 5min

Implementation:

  • Added QuarkusCacheResultOnVoidMethodCheck to detect the pattern
  • Includes comprehensive test cases covering various scenarios
  • Proper documentation with examples and Quarkus documentation references

S8899 Updates

  • Updated ruling baseline for eclipse-jetty (1 new issue detected)
  • Addressed security-context detection review feedback to reduce false positives
  • Added additional test cases for non-security string operations

Test plan

  • Unit tests for S8908 pass
  • Ruling baselines updated
  • CI checks pass

🤖 Generated with Claude Code


Summary by Gitar

  • New Rule: S8908:
    • Implemented QuarkusCacheResultOnVoidMethodCheck and added corresponding metadata, documentation, and test cases.
  • CI/Test Infrastructure:
    • Updated build.yml to explicitly select Java 26 for the autoscan job.
    • Updated AutoScanTest to support conditional community edition testing and refactored orchestrator creation.
    • Registered diff_S8908.json and updated autoscan-diff-by-rules.json to reflect changes in integration testing baselines.

This will update automatically on new commits.

@hashicorp-vault-sonar-prod

hashicorp-vault-sonar-prod Bot commented Jun 22, 2026

Copy link
Copy Markdown
Contributor

SONARJAVA-6488

@romainbrenguier romainbrenguier force-pushed the new-rule/SONARJAVA-6488-S8908 branch from de1cfd6 to 7ad2462 Compare June 22, 2026 14:11
Comment thread .sonar-code-context/SONAR.md Outdated
@romainbrenguier romainbrenguier force-pushed the new-rule/SONARJAVA-6488-S8908 branch 2 times, most recently from 66b324a to 2509f1b Compare June 24, 2026 08:20
Comment thread its/autoscan/src/test/java/org/sonar/java/it/AutoScanTest.java Outdated
This rule detects methods annotated with @CacheResult that return void.
Since void methods have no return value to cache, applying @CacheResult
to them is meaningless and indicates a misunderstanding of how caching works.
@romainbrenguier romainbrenguier force-pushed the new-rule/SONARJAVA-6488-S8908 branch from a983d5c to 2022e5b Compare June 24, 2026 14:39
@gitar-bot

gitar-bot Bot commented Jun 24, 2026

Copy link
Copy Markdown
Code Review ✅ Approved 6 resolved / 6 findings

Implements rule S8908 to detect @CacheResult usage on void methods and updates S8899 to reduce false positives in path normalization checks. All identified test infrastructure and configuration issues have been resolved.

✅ 6 resolved
Bug: isNormalizationCall fails to recognize the "/{2,}" pattern

📄 java-checks/src/main/java/org/sonar/java/checks/security/RequestPathNormalizationCheck.java:208-209
The comment states the code matches normalization patterns like "/+", "//+" and "/{2,}". However the third check literal.matches(".*/{2,}.*") treats /{2,} as a regex applied to the literal string, i.e. it requires two or more consecutive / characters in the pattern string. The pattern string "/{2,}" itself contains only a single / followed by {2,}, so it does NOT match, and contains("/+")/contains("//+") also fail. As a result path.replaceAll("/{2,}", "/") is not recognized as a normalization call.

This does not break the current test normalizedWithRegexPattern only because an unrecognized normalization leaves the variable untracked, and untracked variables default to "safe" (no issue). But the intent expressed by the comment is not implemented: the /{2,} (and similar quantifier-based) collapse patterns are silently unhandled, which makes the heuristic inconsistent. Match against the literal pattern text instead of using String.matches, e.g. check literal.contains("/{2") or compare the literal directly.

Quality: Accidentally committed local dev-tooling config files

📄 .sonar-code-context/settings.json:1-15 📄 .cursor/rules/sonar_code_context.mdc:1-15 📄 .gitignore:42-46
This PR adds .sonar-code-context/settings.json and .cursor/rules/sonar_code_context.mdc, and modifies .gitignore specifically to keep tracking settings.json. These are local code-assistant / tooling configuration files (auto-generated CLI defaults, Cursor rules pointing at a SONAR.md that is not in the repo) and are unrelated to the S8908/S8899 rule implementation. One of the PR commits is even titled "Remove accidentally committed development files", suggesting these were not meant to be part of the change. Committing them to the upstream sonar-java repository adds noise and references a non-existent .sonar-code-context/SONAR.md. Remove these files and the related .gitignore carve-out from the PR.

Edge Case: S8899 flags String.equals as a path security check (FP risk)

📄 java-checks/src/main/java/org/sonar/java/checks/security/RequestPathNormalizationCheck.java:74-78 📄 java-checks/src/main/java/org/sonar/java/checks/security/RequestPathNormalizationCheck.java:148-155
SECURITY_CHECK_METHODS includes String.equals. For an exact-equality comparison like path.equals("/api/admin"), inserting extra slashes (e.g. //api/admin) does not match the literal, so an attacker cannot bypass the check the way they can with startsWith/matches prefix logic. Treating every equals on an un-normalized path inside any conditional/return as a HIGH-severity VULNERABILITY (rule default severity Critical) is likely to generate false positives — equals is an extremely common operation and isInSecurityContext only checks for surrounding control-flow constructs, not that the comparison is actually a security decision. Consider narrowing the matched methods (e.g. drop equals, or require a security-relevant string argument) to reduce noise for a security-impact rule.

Bug: quarkus-cache dependency missing; sample won't compile & test fails

📄 java-checks-test-sources/default/src/main/java/checks/QuarkusCacheResultOnVoidMethodCheckSample.java:3-5 📄 java-checks/src/main/java/org/sonar/java/checks/QuarkusCacheResultOnVoidMethodCheck.java:58-61 📄 java-checks/src/test/java/org/sonar/java/checks/QuarkusCacheResultOnVoidMethodCheckTest.java:26-32
This is the first rule in the repository to reference Quarkus (grep io.quarkus/quarkus matches only the files added by this PR), but no quarkus-cache artifact has been added to java-checks-test-sources/default/pom.xml. Two consequences:

  1. The compiling sample default/src/main/java/checks/QuarkusCacheResultOnVoidMethodCheckSample.java imports io.quarkus.cache.CacheResult, CacheInvalidate, and CacheInvalidateAll. Because these types are not on the module's compile classpath, this source (which Maven compiles as part of the module) will fail to compile and break the build.

  2. The check implements DependencyVersionAware and is only enabled when dependencyFinder.apply("quarkus-cache").isPresent() (line 60). In CheckVerifier, VisitorsBridge.isVisitorDependencyVersionCompatible evaluates this against the test classpath (java-frontend VisitorsBridge.java:124-130). With no quarkus-cache on the classpath the check is skipped entirely, so no issues are raised and verifyIssues() will fail because the // Noncompliant markers are never matched.

Add the io.quarkus:quarkus-cache dependency to java-checks-test-sources/default/pom.xml (and confirm the resolved artifactId matches the "quarkus-cache" string passed to dependencyFinder) so both the sample compiles and the dependency-aware check is enabled during tests.

Bug: Test adds non-existent quarkus-cache jar via bare relative path

📄 java-checks/src/test/java/org/sonar/java/checks/QuarkusCacheResultOnVoidMethodCheckTest.java:32-33 📄 java-checks/src/test/java/org/sonar/java/checks/QuarkusCacheResultOnVoidMethodCheckTest.java:43-44
Both test() and test_non_compiling() add new File("quarkus-cache-3.2.0.Final.jar") to the classpath. This is a bare relative path resolved against the JVM working directory (user.dir), and no such jar exists anywhere in the repository (confirmed by search). A non-existent File on the classpath is silently ignored, so the Quarkus @CacheResult annotation type will remain unresolved during analysis.

Consequences:

  • If the rule relies on resolving the fully-qualified annotation type (io.quarkus.cache.CacheResult), the issues will not be raised, and verifyIssues() will fail; or worse, the test only passes because the sample doesn't actually exercise type resolution, giving a false sense of coverage.
  • This contradicts the repo convention. Other tests obtain extra jars either through the module's resolved test-classpath.txt (by declaring the dependency in the module pom) or via TestClasspathUtils.getTestJars(List<String>), which resolves jars under java-checks-test-sources/target/test-jars.

Suggested fix: declare the quarkus-cache dependency in the java-checks-test-sources/default module pom so it is picked up automatically by DEFAULT_MODULE.getClassPath() (then the manual add(...) is unnecessary), or use TestClasspathUtils.getTestJars(List.of("quarkus-cache-3.2.0.Final")) so the jar is resolved from the proper test-jars directory rather than a hardcoded relative path.

...and 1 more resolved from earlier reviews

Options

Auto-apply is off → Gitar will not commit updates to this branch.
Display: compact → Showing less information.

Comment with these commands to change:

Auto-apply Compact
gitar auto-apply:on         
gitar display:verbose         

Was this helpful? React with 👍 / 👎 | Gitar

@sonarqube-next

Copy link
Copy Markdown

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant