SONARJAVA-5909 FN on S3752 when @RequestMapping in class

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,13 @@ 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 = false;
+  private boolean classHasUnsafeMethods = false;
+  private boolean methodHasSafeMethods = false;
+  private boolean methodHasUnsafeMethods = false;
+
+  private boolean isClassVisited = false;
+
   @Override
   public List<Tree.Kind> nodesToVisit() {
     return Collections.singletonList(Tree.Kind.CLASS);
@@ -53,11 +60,12 @@ public List<Tree.Kind> nodesToVisit() {
   @Override
   public void visitNode(Tree tree) {
     ClassTree classTree = (ClassTree) tree;
+    isClassVisited = true;
     findRequestMappingAnnotation(classTree.modifiers())
       .flatMap(SpringRequestMappingMethodCheck::findRequestMethods)
-      .filter(SpringRequestMappingMethodCheck::mixSafeAndUnsafeMethods)
+      .filter(this::mixSafeAndUnsafeMethods)
       .ifPresent(methods -> reportIssue(methods, MESSAGE));
-
+    isClassVisited = false;
     classTree.members().stream()
       .filter(member -> member.is(Tree.Kind.METHOD))
       .forEach(member -> checkMethod((MethodTree) member, classTree.symbol()));
@@ -69,9 +77,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::mixSafeAndUnsafeMethods);
+      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,9 +123,16 @@ private static boolean inheritRequestMethod(Symbol.TypeSymbol symbol) {
     return false;
   }
 
-  private static boolean mixSafeAndUnsafeMethods(ExpressionTree requestMethodsAssignment) {
+  private boolean mixSafeAndUnsafeMethods(ExpressionTree requestMethodsAssignment) {
     HttpMethodVisitor visitor = new HttpMethodVisitor();
     requestMethodsAssignment.accept(visitor);
+    if (isClassVisited) {
+      classHasSafeMethods = visitor.hasSafeMethods;
+      classHasUnsafeMethods = visitor.hasUnsafeMethods;
+    } else {
+      methodHasSafeMethods = visitor.hasSafeMethods;
+      methodHasUnsafeMethods = visitor.hasUnsafeMethods;
+    }
     return visitor.hasSafeMethods && visitor.hasUnsafeMethods;
   }