diff --git a/its/autoscan/src/test/resources/autoscan/diffs/diff_S3752.json b/its/autoscan/src/test/resources/autoscan/diffs/diff_S3752.json index 6e9d0dfa00..67a411366b 100644 --- a/its/autoscan/src/test/resources/autoscan/diffs/diff_S3752.json +++ b/its/autoscan/src/test/resources/autoscan/diffs/diff_S3752.json @@ -1,6 +1,6 @@ { "ruleKey": "S3752", "hasTruePositives": true, - "falseNegatives": 26, + "falseNegatives": 38, "falsePositives": 0 } diff --git a/its/autoscan/src/test/resources/autoscan/diffs/diff_S4488.json b/its/autoscan/src/test/resources/autoscan/diffs/diff_S4488.json index fe5998ef6d..31fac65d35 100644 --- a/its/autoscan/src/test/resources/autoscan/diffs/diff_S4488.json +++ b/its/autoscan/src/test/resources/autoscan/diffs/diff_S4488.json @@ -1,6 +1,6 @@ { "ruleKey": "S4488", "hasTruePositives": false, - "falseNegatives": 14, + "falseNegatives": 20, "falsePositives": 0 -} \ No newline at end of file +} diff --git a/java-checks-test-sources/default/src/main/java/checks/spring/SpringRequestMappingMethodCheckSample.java b/java-checks-test-sources/default/src/main/java/checks/spring/SpringRequestMappingMethodCheckSample.java index 0aee205f6d..5083ae812c 100644 --- a/java-checks-test-sources/default/src/main/java/checks/spring/SpringRequestMappingMethodCheckSample.java +++ b/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 { diff --git a/java-checks/src/main/java/org/sonar/java/checks/spring/SpringRequestMappingMethodCheck.java b/java-checks/src/main/java/org/sonar/java/checks/spring/SpringRequestMappingMethodCheck.java index d008ae337f..c460cab47d 100644 --- a/java-checks/src/main/java/org/sonar/java/checks/spring/SpringRequestMappingMethodCheck.java +++ b/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 nodesToVisit() { return Collections.singletonList(Tree.Kind.CLASS); @@ -53,11 +58,11 @@ public List 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 = 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 SAFE_METHODS = new HashSet<>(Arrays.asList("GET", "HEAD", "OPTIONS", "TRACE")); private static final Set UNSAFE_METHODS = new HashSet<>(Arrays.asList("DELETE", "PATCH", "POST", "PUT"));