SONARJAVA-5134 S2245 considers RandomStringUtils.secure|secureStrong …

…as safe (#4952)
SonarSource · Dec 10, 2024 · 2758fee · 2758fee
1 parent 8e787b5
commit 2758fee
Show file tree

Hide file tree

Showing 7 changed files with 29 additions and 12 deletions.
diff --git a/its/autoscan/src/test/java/org/sonar/java/it/AutoScanTest.java b/its/autoscan/src/test/java/org/sonar/java/it/AutoScanTest.java
@@ -195,7 +195,7 @@ public void javaCheckTestSources() throws Exception {
     SoftAssertions softly = new SoftAssertions();
     softly.assertThat(newDiffs).containsExactlyInAnyOrderElementsOf(knownDiffs.values());
     softly.assertThat(newTotal).isEqualTo(knownTotal);
-    softly.assertThat(rulesCausingFPs).hasSize(11);
+    softly.assertThat(rulesCausingFPs).hasSize(10);
     softly.assertThat(rulesNotReporting).hasSize(10);
 
     /**

diff --git a/its/autoscan/src/test/resources/autoscan/diffs/diff_S1874.json b/its/autoscan/src/test/resources/autoscan/diffs/diff_S1874.json
@@ -1,6 +1,6 @@
 {
   "ruleKey": "S1874",
   "hasTruePositives": true,
-  "falseNegatives": 246,
-  "falsePositives": 1
+  "falseNegatives": 257,
+  "falsePositives": 0
 }
diff --git a/its/autoscan/src/test/resources/autoscan/diffs/diff_S2245.json b/its/autoscan/src/test/resources/autoscan/diffs/diff_S2245.json
@@ -1,6 +1,6 @@
 {
   "ruleKey": "S2245",
   "hasTruePositives": true,
-  "falseNegatives": 24,
+  "falseNegatives": 25,
   "falsePositives": 0
-}
+}
diff --git a/its/autoscan/src/test/resources/autoscan/diffs/diff_S2440.json b/its/autoscan/src/test/resources/autoscan/diffs/diff_S2440.json
@@ -1,6 +1,6 @@
 {
   "ruleKey": "S2440",
   "hasTruePositives": true,
-  "falseNegatives": 4,
+  "falseNegatives": 2,
   "falsePositives": 0
-}
+}
diff --git a/java-checks-test-sources/default/pom.xml b/java-checks-test-sources/default/pom.xml
@@ -502,7 +502,7 @@
     <dependency>
       <groupId>org.apache.commons</groupId>
       <artifactId>commons-lang3</artifactId>
-      <version>3.12.0</version>
+      <version>3.17.0</version>
       <type>jar</type>
       <scope>provided</scope>
     </dependency>

diff --git a/java-checks-test-sources/default/src/main/java/checks/PseudoRandomCheckSample.java b/java-checks-test-sources/default/src/main/java/checks/PseudoRandomCheckSample.java
@@ -74,6 +74,15 @@ void fun() {
 //                  ^^^^^^
   }
 
+  static String randomStringUtilsInstances(int value) {
+    return switch (value) {
+      case 0 -> org.apache.commons.lang3.RandomStringUtils.secureStrong().next(42); // Compliant
+      case 42 -> org.apache.commons.lang3.RandomStringUtils.secure().next(42); // Compliant
+      default -> org.apache.commons.lang3.RandomStringUtils.insecure().next(42); // Noncompliant
+      //                                                    ^^^^^^^^
+    };
+  }
+
   int nextInt() {
     return 42;
   }

diff --git a/java-checks/src/main/java/org/sonar/java/checks/PseudoRandomCheck.java b/java-checks/src/main/java/org/sonar/java/checks/PseudoRandomCheck.java
@@ -34,21 +34,29 @@ public class PseudoRandomCheck extends IssuableSubscriptionVisitor {
 
   private static final String MESSAGE = "Make sure that using this pseudorandom number generator is safe here.";
 
+  private static final String LANG3_RANDOM_STRING_UTILS = "org.apache.commons.lang3.RandomStringUtils";
+
   private static final MethodMatchers STATIC_RANDOM_METHODS = MethodMatchers.or(
     MethodMatchers.create().ofTypes("java.lang.Math").names("random").addWithoutParametersMatcher().build(),
     MethodMatchers.create()
       .ofSubTypes("java.util.concurrent.ThreadLocalRandom",
         "org.apache.commons.lang.math.RandomUtils",
         "org.apache.commons.lang3.RandomUtils",
         "org.apache.commons.lang.RandomStringUtils",
-        "org.apache.commons.lang3.RandomStringUtils")
+        LANG3_RANDOM_STRING_UTILS)
       .anyName()
       .withAnyParameters()
       .build()
   );
 
+  private static final MethodMatchers RANDOM_STRING_UTILS_SECURE_INSTANCES = MethodMatchers.create()
+    .ofSubTypes(LANG3_RANDOM_STRING_UTILS)
+      .names("secure", "secureStrong")
+      .withAnyParameters()
+      .build();
+
   private static final MethodMatchers RANDOM_STRING_UTILS_RANDOM_WITH_RANDOM_SOURCE = MethodMatchers.create()
-    .ofSubTypes("org.apache.commons.lang.RandomStringUtils", "org.apache.commons.lang3.RandomStringUtils")
+    .ofSubTypes("org.apache.commons.lang.RandomStringUtils", LANG3_RANDOM_STRING_UTILS)
     .names("random")
     .addParametersMatcher("int", "int", "int", "boolean", "boolean", "char[]", "java.util.Random")
     .build();
@@ -65,8 +73,7 @@ public List<Tree.Kind> nodesToVisit() {
 
   @Override
   public void visitNode(Tree tree) {
-    if (tree.is(Tree.Kind.METHOD_INVOCATION)) {
-      MethodInvocationTree mit = (MethodInvocationTree) tree;
+    if (tree instanceof MethodInvocationTree mit) {
       IdentifierTree reportLocation = ExpressionUtils.methodName(mit);
 
       if (isStaticCallToInsecureRandomMethod(mit)) {
@@ -83,6 +90,7 @@ public void visitNode(Tree tree) {
   private static boolean isStaticCallToInsecureRandomMethod(MethodInvocationTree mit) {
     return STATIC_RANDOM_METHODS.matches(mit)
       && !RANDOM_STRING_UTILS_RANDOM_WITH_RANDOM_SOURCE.matches(mit)
+      && !RANDOM_STRING_UTILS_SECURE_INSTANCES.matches(mit)
       && mit.methodSymbol().isStatic();
   }