From 061c51b52437de20e5b87fa762d062f84231d854 Mon Sep 17 00:00:00 2001 From: asya-vorobeva Date: Thu, 29 Jan 2026 11:31:19 +0100 Subject: [PATCH 1/3] SONARJAVA-5909 FN on S3752 when @RequestMapping in class --- .../resources/autoscan/diffs/diff_S3752.json | 2 +- .../resources/autoscan/diffs/diff_S4488.json | 4 +-- ...SpringRequestMappingMethodCheckSample.java | 33 +++++++++++++++++++ .../SpringRequestMappingMethodCheck.java | 33 +++++++++++++++---- 4 files changed, 63 insertions(+), 9 deletions(-) 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 6e9d0dfa008..67a411366bd 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 fe5998ef6d1..31fac65d359 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 0aee205f6d8..5083ae812cb 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 d008ae337f0..2e2f8da59a0 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,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 nodesToVisit() { return Collections.singletonList(Tree.Kind.CLASS); @@ -53,11 +60,12 @@ public List 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 = 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; } From 4ba99913d8edcc5b2d28c11e69d68efbda25838c Mon Sep 17 00:00:00 2001 From: asya-vorobeva Date: Thu, 29 Jan 2026 16:31:00 +0100 Subject: [PATCH 2/3] Fix --- .../spring/SpringRequestMappingMethodCheck.java | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) 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 2e2f8da59a0..4332915e771 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 @@ -63,7 +63,7 @@ public void visitNode(Tree tree) { isClassVisited = true; findRequestMappingAnnotation(classTree.modifiers()) .flatMap(SpringRequestMappingMethodCheck::findRequestMethods) - .filter(this::mixSafeAndUnsafeMethods) + .filter(this::mixesSafeAndUnsafeMethods) .ifPresent(methods -> reportIssue(methods, MESSAGE)); isClassVisited = false; classTree.members().stream() @@ -78,7 +78,7 @@ private void checkMethod(MethodTree method, Symbol.TypeSymbol classSymbol) { if (requestMethods.isPresent()) { Optional expressionTree = requestMethods - .filter(this::mixSafeAndUnsafeMethods); + .filter(this::mixesSafeAndUnsafeMethods); if (expressionTree.isPresent()) { reportIssue(expressionTree.get(), MESSAGE); } else { @@ -123,9 +123,14 @@ private static boolean inheritRequestMethod(Symbol.TypeSymbol symbol) { return false; } - private boolean mixSafeAndUnsafeMethods(ExpressionTree requestMethodsAssignment) { + private boolean mixesSafeAndUnsafeMethods(ExpressionTree requestMethodsAssignment) { HttpMethodVisitor visitor = new HttpMethodVisitor(); requestMethodsAssignment.accept(visitor); + updateFlags(visitor); + return visitor.hasSafeMethods && visitor.hasUnsafeMethods; + } + + private void updateFlags(HttpMethodVisitor visitor) { if (isClassVisited) { classHasSafeMethods = visitor.hasSafeMethods; classHasUnsafeMethods = visitor.hasUnsafeMethods; @@ -133,7 +138,6 @@ private boolean mixSafeAndUnsafeMethods(ExpressionTree requestMethodsAssignment) methodHasSafeMethods = visitor.hasSafeMethods; methodHasUnsafeMethods = visitor.hasUnsafeMethods; } - return visitor.hasSafeMethods && visitor.hasUnsafeMethods; } private static class HttpMethodVisitor extends BaseTreeVisitor { From c69b9d9b5ed6d0dce3d10ee647a0d7a290f9e114 Mon Sep 17 00:00:00 2001 From: asya-vorobeva Date: Thu, 29 Jan 2026 17:15:29 +0100 Subject: [PATCH 3/3] Fix --- .../SpringRequestMappingMethodCheck.java | 43 ++++++++++--------- 1 file changed, 23 insertions(+), 20 deletions(-) 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 4332915e771..c460cab47d1 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,12 +45,10 @@ 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; + private boolean classHasSafeMethods; + private boolean classHasUnsafeMethods; + private boolean methodHasSafeMethods; + private boolean methodHasUnsafeMethods; @Override public List nodesToVisit() { @@ -60,12 +58,11 @@ public List nodesToVisit() { @Override public void visitNode(Tree tree) { ClassTree classTree = (ClassTree) tree; - isClassVisited = true; + resetFlags(); findRequestMappingAnnotation(classTree.modifiers()) .flatMap(SpringRequestMappingMethodCheck::findRequestMethods) - .filter(this::mixesSafeAndUnsafeMethods) + .filter(this::mixesSafeAndUnsafeMethodsOnClass) .ifPresent(methods -> reportIssue(methods, MESSAGE)); - isClassVisited = false; classTree.members().stream() .filter(member -> member.is(Tree.Kind.METHOD)) .forEach(member -> checkMethod((MethodTree) member, classTree.symbol())); @@ -78,7 +75,7 @@ private void checkMethod(MethodTree method, Symbol.TypeSymbol classSymbol) { if (requestMethods.isPresent()) { Optional expressionTree = requestMethods - .filter(this::mixesSafeAndUnsafeMethods); + .filter(this::mixesSafeAndUnsafeMethodsOnMethod); if (expressionTree.isPresent()) { reportIssue(expressionTree.get(), MESSAGE); } else { @@ -123,21 +120,27 @@ private static boolean inheritRequestMethod(Symbol.TypeSymbol symbol) { return false; } - private boolean mixesSafeAndUnsafeMethods(ExpressionTree requestMethodsAssignment) { + private boolean mixesSafeAndUnsafeMethodsOnClass(ExpressionTree requestMethodsAssignment) { HttpMethodVisitor visitor = new HttpMethodVisitor(); requestMethodsAssignment.accept(visitor); - updateFlags(visitor); + classHasSafeMethods = visitor.hasSafeMethods; + classHasUnsafeMethods = visitor.hasUnsafeMethods; return visitor.hasSafeMethods && visitor.hasUnsafeMethods; } - private void updateFlags(HttpMethodVisitor visitor) { - if (isClassVisited) { - classHasSafeMethods = visitor.hasSafeMethods; - classHasUnsafeMethods = visitor.hasUnsafeMethods; - } else { - methodHasSafeMethods = visitor.hasSafeMethods; - methodHasUnsafeMethods = 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 {