SONARJAVA-5909 FN on S3752 when @RequestMapping in class (#5412)

Author: asya-vorobeva

its/autoscan/src/test/resources/autoscan/diffs/diff_S3752.json

@@ -1,6 +1,6 @@
 {
   "ruleKey": "S3752",
   "hasTruePositives": true,
-  "falseNegatives": 26,
+  "falseNegatives": 38,
   "falsePositives": 0
 }

its/autoscan/src/test/resources/autoscan/diffs/diff_S4488.json

@@ -1,6 +1,6 @@
 {
   "ruleKey": "S4488",
   "hasTruePositives": false,
-  "falseNegatives": 14,
+  "falseNegatives": 20,
   "falsePositives": 0
-}
+}

java-checks-test-sources/default/src/main/java/checks/spring/SpringRequestMappingMethodCheckSample.java

@@ -64,6 +64,39 @@ String get() {
       return "Hello from get";
     }
 
+    @RequestMapping(value = "/post", method = RequestMethod.POST) // Noncompliant
+    String post() {
+      return "Hello from post";
+    }
+
+    @RequestMapping(value = "/put", method = RequestMethod.PUT) // Noncompliant
+    String put() {
+      return "Hello from put";
+    }
+
+    @RequestMapping(value = "/delete", method = RequestMethod.DELETE) // Noncompliant
+    String delete() {
+      return "Hello from delete";
+    }
+  }
+
+  @RestController
+  @RequestMapping(path = "/update", method = RequestMethod.POST)
+  public static class UpdateController {
+    @RequestMapping(value = "/", method = RequestMethod.GET) // Noncompliant
+    String get() {
+      return "Hello from get";
+    }
+
+    @RequestMapping(value = "/head", method = RequestMethod.HEAD) // Noncompliant
+    String head() {
+      return "Hello from head";
+    }
+
+    @RequestMapping(value = "/delete", method = RequestMethod.DELETE)
+    String delete() {
+      return "Hello from delete";
+    }
   }
 
   public static class DerivedController extends OtherController {

java-checks/src/main/java/org/sonar/java/checks/spring/SpringRequestMappingMethodCheck.java

@@ -45,6 +45,11 @@ public class SpringRequestMappingMethodCheck extends IssuableSubscriptionVisitor
   private static final String REQUEST_METHOD = "method";
   public static final String MESSAGE = "Make sure allowing safe and unsafe HTTP methods is safe here.";
 
+  private boolean classHasSafeMethods;
+  private boolean classHasUnsafeMethods;
+  private boolean methodHasSafeMethods;
+  private boolean methodHasUnsafeMethods;
+
   @Override
   public List<Tree.Kind> nodesToVisit() {
     return Collections.singletonList(Tree.Kind.CLASS);
@@ -53,11 +58,11 @@ public List<Tree.Kind> nodesToVisit() {
   @Override
   public void visitNode(Tree tree) {
     ClassTree classTree = (ClassTree) tree;
+    resetFlags();
     findRequestMappingAnnotation(classTree.modifiers())
       .flatMap(SpringRequestMappingMethodCheck::findRequestMethods)
-      .filter(SpringRequestMappingMethodCheck::mixSafeAndUnsafeMethods)
+      .filter(this::mixesSafeAndUnsafeMethodsOnClass)
       .ifPresent(methods -> reportIssue(methods, MESSAGE));
-
     classTree.members().stream()
       .filter(member -> member.is(Tree.Kind.METHOD))
       .forEach(member -> checkMethod((MethodTree) member, classTree.symbol()));
@@ -69,9 +74,15 @@ private void checkMethod(MethodTree method, Symbol.TypeSymbol classSymbol) {
       .flatMap(SpringRequestMappingMethodCheck::findRequestMethods);
 
     if (requestMethods.isPresent()) {
-      requestMethods
-        .filter(SpringRequestMappingMethodCheck::mixSafeAndUnsafeMethods)
-        .ifPresent(methods -> reportIssue(methods, MESSAGE));
+      Optional<ExpressionTree> expressionTree = requestMethods
+        .filter(this::mixesSafeAndUnsafeMethodsOnMethod);
+      if (expressionTree.isPresent()) {
+        reportIssue(expressionTree.get(), MESSAGE);
+      } else {
+        if ((classHasSafeMethods && methodHasUnsafeMethods) || (classHasUnsafeMethods && methodHasSafeMethods)) {
+          reportIssue(requestMethods.get(), MESSAGE);
+        }
+      }
     } else if (requestMappingAnnotation.isPresent() && !inheritRequestMethod(classSymbol)) {
       reportIssue(requestMappingAnnotation.get().annotationType(), MESSAGE);
     }
@@ -109,12 +120,29 @@ private static boolean inheritRequestMethod(Symbol.TypeSymbol symbol) {
     return false;
   }
 
-  private static boolean mixSafeAndUnsafeMethods(ExpressionTree requestMethodsAssignment) {
+  private boolean mixesSafeAndUnsafeMethodsOnClass(ExpressionTree requestMethodsAssignment) {
+    HttpMethodVisitor visitor = new HttpMethodVisitor();
+    requestMethodsAssignment.accept(visitor);
+    classHasSafeMethods = visitor.hasSafeMethods;
+    classHasUnsafeMethods = visitor.hasUnsafeMethods;
+    return visitor.hasSafeMethods && visitor.hasUnsafeMethods;
+  }
+
+  private boolean mixesSafeAndUnsafeMethodsOnMethod(ExpressionTree requestMethodsAssignment) {
     HttpMethodVisitor visitor = new HttpMethodVisitor();
     requestMethodsAssignment.accept(visitor);
+    methodHasSafeMethods = visitor.hasSafeMethods;
+    methodHasUnsafeMethods = visitor.hasUnsafeMethods;
     return visitor.hasSafeMethods && visitor.hasUnsafeMethods;
   }
 
+  private void resetFlags() {
+    classHasSafeMethods = false;
+    classHasUnsafeMethods = false;
+    methodHasSafeMethods = false;
+    methodHasUnsafeMethods = false;
+  }
+
   private static class HttpMethodVisitor extends BaseTreeVisitor {
     private static final Set<String> SAFE_METHODS = new HashSet<>(Arrays.asList("GET", "HEAD", "OPTIONS", "TRACE"));
     private static final Set<String> UNSAFE_METHODS = new HashSet<>(Arrays.asList("DELETE", "PATCH", "POST", "PUT"));