SONARJAVA-6440 S2245: Add security-context heuristic to reduce false positives#5667
SONARJAVA-6440 S2245: Add security-context heuristic to reduce false positives#5667Luqmansonar wants to merge 8 commits into
Conversation
Agentic Analysis: Early ResultsAgentic Analysis and Context Augmentation are available on your project. Here are some issues that could have been prevented. Follow the links to learn how to put them into action. 86 issue(s) found across 6 file(s):
Showing 50 of 86 issues. Analyzed by SonarQube Agentic Analysis in 3.5 s |
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
…positives Port the DART-276 heuristic to Java. PRNG usages are now reported only when the surrounding code is plausibly security-relevant, determined by: - Part 1: file contains an import from a known crypto/auth package prefix (java.security., javax.crypto., org.springframework.security., org.bouncycastle., io.jsonwebtoken., com.auth0.jwt., at.favre.lib.crypto., de.mkammerer.argon2.) - Part 2: the enclosing method/constructor scope (or enclosing class for field/static initializers) contains an identifier whose camelCase / snake_case tokens intersect with a fixed set of 29 security keywords. The heuristic is applied to all six PRNG sources the rule currently flags (Random, JVMRandom, Math.random, ThreadLocalRandom, RandomUtils, RandomStringUtils). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Use ImportClauseTree as the loop variable type (S4838) - Extract crypto-prefix matching into a helper to remove the multiple continues from hasCryptoImport (S135) - Memoize scopeHasSecurityKeyword results per scope Tree in an IdentityHashMap, cleared on each COMPILATION_UNIT visit - Update sonar-server ruling expectation for S2245: the recovery indexer schedule delay using RandomUtils is now correctly suppressed by the new security-context heuristic (no enclosing security keyword, no crypto imports) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Three additional FPs are correctly suppressed by the new security-context heuristic. Inspected sources to confirm each is a legitimate suppression (no crypto imports + no security-keyword identifiers in enclosing method/class scope per the Java keyword set from the ticket): - mall UmsMemberCouponServiceImpl#generateCouponCode (line 91): Random() used to build a 16-digit coupon code; method/class identifiers contain no listed keyword (`coupon`, `member`, `code`, etc. are not in the set). UmsMemberServiceImpl#generateAuthCode (line 112) stays flagged because the file imports org.springframework.security.* -> Part 1 triggers. - eclipse-jetty ShutdownMonitor#setupSocket (line 287): Math.random() used to derive a stop-port key. The local variable is named `key`, which is in the Kotlin spec but not the Java spec (SONARJAVA-6440), so the heuristic correctly suppresses per the ticket's keyword list. DefaultSessionIdManager (line 369) stays flagged because the file imports java.security.SecureRandom -> Part 1 triggers. Applied to both eclipse-jetty and eclipse-jetty-similar-to-main (used by the incremental ruling test). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Luqmansonar
force-pushed
the
SONARJAVA-6440-s2245-security-context-heuristic
branch
from
d2f846c to
d08db1a
Compare
AutoScanTest reports the total FN count dropped from 4146 to 4142 (-4) and the FP count is unchanged at 18. The only rule modified by this branch is S2245, so the entire delta is attributable to it: 4 of the issues the autoscan version of the rule was previously missing in java-checks-test-sources are now correctly suppressed by the new security-context heuristic in the full analyzer too, removing them from the FN diff. Update diff_S2245.json falseNegatives: 26 -> 22 to match the new analyzer behaviour. The recipe in the failure log (copy target/actual/autoscan-diffs/diff_S2245.json over the checked-in expectation) was not directly available locally, but the metric in the file is just falseNegatives, the only rule touched by this PR is S2245, and the announced delta is exactly -4. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The previous adjustment (S2245 26 -> 22) was based on the reported total FN delta of -4. The next CI iteration revealed two per-rule deltas instead of a single total: - S2245: 26 -> 17 (-9). My new test samples and the heuristic together remove more autoscan-misses than the original total suggested. - S2440 (ClassWithOnlyStaticMethodsInstantiationCheck): 2 -> 4 (+2). The new PseudoRandomCheckNoContextSample.java instantiates RandomUtils and RandomStringUtils (utility classes with only static methods); full mode raises S2440 on each, autoscan misses both without semantics, so the FN diff grows by 2. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
nils-werner-sonarsource
left a comment
nils-werner-sonarsource
left a comment
There was a problem hiding this comment.
Overall this is a well-structured and well-documented implementation — the PR description is thorough and the per-scenario test files are a good pattern. A few bugs and concerns below.
| private static boolean matchesCryptoPrefix(String importName) { | ||
| for (String prefix : CRYPTO_IMPORT_PREFIXES) { | ||
| if (importName.startsWith(prefix)) { | ||
| return true; |
There was a problem hiding this comment.
Bug: wildcard import not detected
A wildcard import like import java.security.*; concatenates to "java.security.*". The check importName.startsWith("java.security.") won't match because the string ends with ".*".
Suggested fix — normalise the import name before matching:
private static boolean matchesCryptoPrefix(String importName) {
// Normalise wildcard imports: "java.security.*" -> "java.security."
String name = importName.endsWith(".*")
? importName.substring(0, importName.length() - 1)
: importName + ".";
for (String prefix : CRYPTO_IMPORT_PREFIXES) {
if (name.startsWith(prefix)) return true;
}
return false;
}Please also add a test case with import java.security.*; in PseudoRandomCheckCryptoImportSample.java.
There was a problem hiding this comment.
Verified locally: "java.security.*".startsWith("java.security.") returns true. ExpressionsHelper.concatenate produces "java.security.*" and the prefix "java.security." contains no *, so startsWith already matches. Added PseudoRandomCheckWildcardImportSample.java (and the static-import sample) in c1708df to lock both cases in.
| // a single lowercase word when all-uppercase, or split further on capital-letter boundaries. | ||
| static List<String> tokenizeIdentifier(String identifier) { | ||
| List<String> words = new java.util.ArrayList<>(); | ||
| for (String part : identifier.split("_")) { |
There was a problem hiding this comment.
Nit: inline java.util.ArrayList should be a proper import
List<String> words = new java.util.ArrayList<>();This is the only class in this file referenced by fully-qualified name. Please add import java.util.ArrayList; at the top and use new ArrayList<>() here.
|
|
||
| private boolean cryptoImportPresent = false; | ||
| private final Map<Tree, Boolean> scopeSecurityContextCache = new IdentityHashMap<>(); | ||
|
|
There was a problem hiding this comment.
Concern: cache may not be worth the complexity
The scopeSecurityContextCache avoids re-walking a scope when multiple PRNG calls appear in the same method. In practice this is rare, and the IdentifierCollector walk over a method body is cheap. Meanwhile:
- The cache pins AST node references for the file's lifetime.
- The
clear()invisitNode(COMPILATION_UNIT)creates an implicit lifecycle contract that is easy to break.
Consider removing the cache and calling scopeHasSecurityKeyword directly:
private boolean isInSecurityContext(Tree tree) {
if (cryptoImportPresent) return true;
Tree scope = findDeclarationScope(tree);
return scope != null && scopeHasSecurityKeyword(scope);
}If profiling ever shows this matters, the cache can be reintroduced — but the clear() should then be in leaveNode(COMPILATION_UNIT) to make the lifecycle explicit.
There was a problem hiding this comment.
Kept the cache; moved clear() + the cryptoImportPresent reset to a new leaveNode(COMPILATION_UNIT) per your sub-suggestion. Lifecycle is now explicit. c1708df.
| } | ||
| Tree scope = findDeclarationScope(tree); | ||
| if (scope == null) { | ||
| return false; |
There was a problem hiding this comment.
Minor: silent false on null scope suppresses issues without explanation
findDeclarationScope returning null means the PRNG call has no enclosing MethodTree or ClassTree, which shouldn't occur in valid Java. Returning false silently drops the issue in this edge case.
Prefer returning true (flag by default when context is unknown) or add a comment explaining why suppressing is the right tradeoff:
if (scope == null) {
// Defensive: shouldn't happen in valid Java; flag to avoid silent FN.
return true;
}There was a problem hiding this comment.
Changed to return true with the suggested comment. c1708df.
| ); | ||
|
|
||
| // SONARJAVA-6440: ported from DART-276 security-context heuristic. | ||
| // When any of these prefixes appear in the file's imports, all PRNG usages in the file are reported. |
There was a problem hiding this comment.
Nit: comment explains origin, not the invariant
Ticket references rot as context. The comment should explain why this is the right design, not where the idea came from.
Suggestion:
// A crypto library import signals the whole file is security-relevant;
// report all PRNG calls without needing per-scope keyword analysis.
private static final List<String> CRYPTO_IMPORT_PREFIXES = List.of(There was a problem hiding this comment.
Update on this: I had to remove the explanatory comment in 29e9bef. SQS flagged the two-line block as S125 (Sections of code should not be commented out) — a false positive on the rule's part, since the lines are plain English, but the cleanest unblock was just to drop the comment. The constant name CRYPTO_IMPORT_PREFIXES plus the prefix list are reasonably self-evident on their own. Happy to reinstate a tighter wording if you'd like.
| import org.apache.commons.lang3.RandomStringUtils; | ||
|
|
||
| // SONARJAVA-6440: no crypto import. Security keywords reached via per-scope identifier scan. | ||
| class PseudoRandomCheckSecurityKeywordsSample { |
There was a problem hiding this comment.
Suggestion: a few edge cases worth adding to the test suite
-
Digit-containing uppercase identifiers —
AES256has no lowercase letter soisAllUppercaseWithLetterreturnstrueand it lowercases to"aes256", which does not match the keyword"aes". A variableAES256_KEYwould therefore not trigger the rule. Please add a test to document this (Compliant or Noncompliant — either is fine, but it should be intentional). -
Wildcard crypto import —
import java.security.*;should trigger Part 1 (see comment onmatchesCryptoPrefix). Add a test case inPseudoRandomCheckCryptoImportSample.java. -
Nested inner class isolation —
findDeclarationScopereturns the innerClassTreefirst, so a PRNG call in an inner class should not pick up keywords from the outer class. Worth an explicit Compliant test to confirm this isolation is intentional. -
Static import of a crypto class —
import static javax.crypto.Cipher.ENCRYPT_MODE;is stillTree.Kind.IMPORT, so it should be caught byhasCryptoImport. A targeted test would confirm this.
There was a problem hiding this comment.
All four added in c1708df: AES256_KEY (Compliant, with comment documenting Dart-faithful behaviour), wildcard import sample, static import sample, inner-class isolation (TokenAware outer + NeutralInner inner). Tokenization gap (digit-suffix + ChaCha20-style) documented on the SONARJAVA-6440 description; cross-language follow-up tracked under APPSEC-3004.
| "hasTruePositives": true, | ||
| "falseNegatives": 2, | ||
| "falseNegatives": 4, | ||
| "falsePositives": 0 |
There was a problem hiding this comment.
Unrelated regression: S2440 false-negatives increased
S2440 (StringConcatenationInLoop) is a completely different rule from S2245. The falseNegatives going from 2 → 4 here doesn't appear related to this PR's changes. Can you clarify whether this is expected? If not, please revert.
There was a problem hiding this comment.
Root cause: my new PseudoRandomCheckNoContextSample.java instantiated new RandomUtils() and new RandomStringUtils() — utility classes with only static methods, which S2440 = ClassWithOnlyStaticMethodsInstantiationCheck (not StringConcatenationInLoop) legitimately flags but autoscan misses without semantics. Dropped those instantiations and reverted diff_S2440.json to FN=2. c1708df.
- Style: import java.util.ArrayList and drop the inline FQN.
- Lifecycle: move scopeSecurityContextCache.clear() and
cryptoImportPresent reset from visitNode(COMPILATION_UNIT) to a new
leaveNode(COMPILATION_UNIT) override, so the cache lifecycle is
explicit rather than relying on a re-visit of the root.
- Defensive: if findDeclarationScope returns null (shouldn't happen in
valid Java) fail open (return true / flag) instead of silently
suppressing.
- Comment: replace the ticket-reference comment on
CRYPTO_IMPORT_PREFIXES with one that explains the invariant the
constant encodes.
- Tests: add four edge cases:
* AES256 (all-uppercase with digit suffix; documents that the
Dart-faithful tokenization keeps it as a single `aes256` token,
which does not match the `aes` keyword).
* `import java.security.*;` wildcard crypto import.
* `import static javax.crypto.Cipher.ENCRYPT_MODE;` static crypto
import.
* Inner class with neutral identifiers inside an outer class whose
name carries a keyword; the inner class's PRNG call is Compliant
because findDeclarationScope returns the inner ClassTree.
- Test sample cleanup: drop the unneeded `new RandomUtils()` and
`new RandomStringUtils()` instantiations from the no-context
sample (they were triggering S2440 as a side effect). Revert
diff_S2440.json to its original falseNegatives: 2.
Not addressed: Nils's wildcard-import bug claim - verified locally
that `"java.security.*".startsWith("java.security.")` returns true,
so the wildcard case already works; will reply on the PR.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Remove the two-line comment above CRYPTO_IMPORT_PREFIXES. The constant name plus the prefix list are self-explanatory. - Rename the digit-suffixed test variable from AES256 to AES256_KEY to match the exact identifier Nils called out in review; extend the comment to reference APPSEC-3004 as the cross-language follow-up for the tokenizer gap. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
CI artifact from run 27343009526 lists only diff_S1874.json and
diff_S2119.json as mismatched (autoscan only writes a diff file when
it differs from the expectation). The deltas match the +3 total
("expected 4137, actual 4140"):
* S1874 ("@deprecated" code should not be used) 246 -> 248: the new
PseudoRandomCheckNoContextSample.java calls deprecated lang3 APIs
RandomUtils.nextFloat() and RandomStringUtils.random(int) (deprecated
since lang3 3.13). Full mode flags both; autoscan misses both because
resolving the @deprecated annotation needs semantic info.
* S2119 ("Random" objects should be reused) 2 -> 3: one extra
`new Random()` in the new samples is flagged by full mode and missed
by autoscan for the same reason (Random type resolution requires
semantics).
diff_S2245.json already contains the correct value (17) since
77ebaba and is intentionally untouched here.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Quick context on the autoscan baseline updates in commit 8123b0d. The failing rules were S1874 and S2119, not S2245. The new test samples added to exercise the heuristic include a few deprecated Apache Commons calls like RandomUtils.nextFloat and RandomStringUtils.random, plus one extra new Random constructor. Full analysis catches all of those, autoscan does not because it lacks semantics to resolve the Deprecated annotation and the Random type. That accounts for plus two on S1874, plus one on S2119, total three, matching the expected 4137 actual 4140 message. The S2245 baseline was already correct at 17 on the branch, so it stays as is. |
Code Review ✅ Approved 1 resolved / 1 findingsImplements a security-context heuristic for S2245 by checking for crypto imports and scope-level keywords to reduce false positives. Resolved an issue where scope subtrees were incorrectly traversed for multiple PRNG usages. ✅ 1 resolved✅ Performance: Scope subtree re-traversed per PRNG usage in same scope
OptionsAuto-apply is off → Gitar will not commit updates to this branch. Comment with these commands to change:
Was this helpful? React with 👍 / 👎 | Gitar |
|
2 participants
Summary
Ports the DART-276 security-context heuristic to Java for S2245 (
PseudoRandomCheck). Pseudorandom number generator usages are now reported only when the surrounding code is plausibly security-relevant.The heuristic has two parts:
java.security.,javax.crypto.,org.springframework.security.,org.bouncycastle.,io.jsonwebtoken.,com.auth0.jwt.,at.favre.lib.crypto.,de.mkammerer.argon2.MethodTree(method/constructor); fall back to the enclosingClassTreefor field/static initializers. Collect every identifier in that scope, tokenize it (split on_, then on camelCase / PascalCase boundaries; all-uppercase parts stay whole), lowercase each token, and intersect with the 29-keyword set from the ticket. Any hit ⇒ flag.Decisions
Random,JVMRandom,Math.random(),ThreadLocalRandom,RandomUtils,RandomStringUtils) — not only the two literally listed in the ticket. Confirmed with the requester. The ticket's motivation (false positives in non-security code) applies equally to all six.declarationScopesemantics (method for local code, class for field code). Balances FP reduction against recall on the commonprivate static final Random R = new Random();pattern._splitIntoWordsfromsonar-dart). One faithful-port quirk worth knowing: the keywordrandombytesis all-lowercase, sorandomBytes(camelCase) which tokenizes to[random, bytes]does NOT match — only literalrandombytesorRANDOMBYTESdoes. Same behavior as Dart; documented in the test sample.S2245.html) was NOT modified — the ticket does not request a doc change. Happy to update separately if desired.Test plan
PseudoRandomCheckSample.javaassertions still pass (file importsjava.security.SecureRandom→ Part 1 triggers, all current// Noncompliantmarkers preserved).PseudoRandomCheckNoContextSample.java: neutral file, all calls Compliant (heuristic suppresses).PseudoRandomCheckSecurityKeywordsSample.java: camelCase / snake_case / UPPER / method-name / parameter keyword cases trigger; negative cases (unrelatedScope,splitRandomBytes) do not.PseudoRandomCheckCryptoImportSample.java:org.bouncycastleimport → all calls flagged via Part 1.PseudoRandomCheckFieldScopeSample.java: field / static-initializer calls fall back to class scope;TokenGenerator,Holder(withpasswordfield),CipherBundleflagged; neutral class is not.mvn -pl java-checks test -Dtest=PseudoRandomCheckTest→ 5 tests, 0 failures.🤖 Generated with Claude Code