From ebac5e99cbcf2fd5c82aeee9dc49d59219a0e5d8 Mon Sep 17 00:00:00 2001 From: erwan-serandour-sonarsource Date: Thu, 7 Dec 2023 15:43:30 +0100 Subject: [PATCH] SONARJAVA-4723 in S6856, filter path variables used by Model Attribute when we extracting path variable from the url (#4606) --- .../resources/autoscan/diffs/diff_S6856.json | 2 +- ...ingPathVariableAnnotationCheckSample.java} | 62 ++++++++++++-- ...> MissingPathVariableAnnotationCheck.java} | 83 ++++++++++++------- ...ssingPathVariableAnnotationCheckTest.java} | 6 +- .../org/sonar/plugins/java/CheckList.java | 4 +- 5 files changed, 111 insertions(+), 46 deletions(-) rename java-checks-test-sources/default/src/main/java/checks/{PathVariableAnnotationShouldBePresentIfPathVariableIsUsedCheckSample.java => MissingPathVariableAnnotationCheckSample.java} (77%) rename java-checks/src/main/java/org/sonar/java/checks/{PathVariableAnnotationShouldBePresentIfPathVariableIsUsedCheck.java => MissingPathVariableAnnotationCheck.java} (66%) rename java-checks/src/test/java/org/sonar/java/checks/{PathVariableAnnotationShouldBePresentIfPathVariableIsUsedCheckTest.java => MissingPathVariableAnnotationCheckTest.java} (79%) diff --git a/its/autoscan/src/test/resources/autoscan/diffs/diff_S6856.json b/its/autoscan/src/test/resources/autoscan/diffs/diff_S6856.json index 75f20b0f251..9c8d93ad90b 100644 --- a/its/autoscan/src/test/resources/autoscan/diffs/diff_S6856.json +++ b/its/autoscan/src/test/resources/autoscan/diffs/diff_S6856.json @@ -1,6 +1,6 @@ { "ruleKey": "S6856", "hasTruePositives": false, - "falseNegatives": 21, + "falseNegatives": 22, "falsePositives": 0 } diff --git a/java-checks-test-sources/default/src/main/java/checks/PathVariableAnnotationShouldBePresentIfPathVariableIsUsedCheckSample.java b/java-checks-test-sources/default/src/main/java/checks/MissingPathVariableAnnotationCheckSample.java similarity index 77% rename from java-checks-test-sources/default/src/main/java/checks/PathVariableAnnotationShouldBePresentIfPathVariableIsUsedCheckSample.java rename to java-checks-test-sources/default/src/main/java/checks/MissingPathVariableAnnotationCheckSample.java index 445f5123f89..03d32d1fed8 100644 --- a/java-checks-test-sources/default/src/main/java/checks/PathVariableAnnotationShouldBePresentIfPathVariableIsUsedCheckSample.java +++ b/java-checks-test-sources/default/src/main/java/checks/MissingPathVariableAnnotationCheckSample.java @@ -4,12 +4,13 @@ import java.util.Optional; import org.springframework.web.bind.annotation.DeleteMapping; import org.springframework.web.bind.annotation.GetMapping; +import org.springframework.web.bind.annotation.ModelAttribute; import org.springframework.web.bind.annotation.PathVariable; import org.springframework.web.bind.annotation.PostMapping; import org.springframework.web.bind.annotation.PutMapping; -public class PathVariableAnnotationShouldBePresentIfPathVariableIsUsedCheckSample { - @GetMapping("/{id}") // Noncompliant {{Bind path variable "id" to a method parameter.}} +public class MissingPathVariableAnnotationCheckSample { + @GetMapping("/{id}") // Noncompliant [[sc=3;ec=23]] {{Bind path variable "id" to a method parameter.}} public String get(String id) { return "Hello World"; } @@ -141,13 +142,6 @@ public String withoutRequestMappingAnnotation(@PathVariable String id) { // com return "Hello World"; } - - @GetMapping("/{id}/{name}") // Noncompliant - public String mapStringToInt(@PathVariable Map map) { - return "Hello World"; - } - - @GetMapping( produces={"application/json", "application/xml"}, consumes={"application/json", "application/xml"}, @@ -187,4 +181,54 @@ public String getPlaceHolder(String id) { // compliant, we don't consider this c return "Hello World"; } + static class ModelA { + @ModelAttribute("user") + public String getUser(@PathVariable String id, @PathVariable String name) { + return "user"; + } + + @GetMapping("/{id}/{name}") + public String get() { // compliant, @ModelAttribute is always called before @GetMapping to generate the model. In our case model attribute + // consume the id and name path variables + return "Hello World"; + } + + @GetMapping("/{id}/{name}/{age}") // Compliant + public String get2(@PathVariable String age) { // compliant + return "Hello World"; + } + + @GetMapping("/{id}/{name}/{age}") // Noncompliant {{Bind path variable "age" to a method parameter.}} + public String get3() { + return "Hello World"; + } + } + + static class ModelB { + @ModelAttribute("user") + public String getUser(@PathVariable String id) { + return "user"; + } + + @ModelAttribute("id") + public String getId(@PathVariable String name) { + return "id"; + } + + @GetMapping("/{id}/{name}") + public String get() { // compliant + return "Hello World"; + } + + @GetMapping("/{id}/{name}/{age}") + public String get2(@PathVariable String age) { // compliant + return "Hello World"; + } + + @GetMapping("/{id}/{name}/{age}") // Noncompliant + public String get3() { + return "Hello World"; + } + } + } diff --git a/java-checks/src/main/java/org/sonar/java/checks/PathVariableAnnotationShouldBePresentIfPathVariableIsUsedCheck.java b/java-checks/src/main/java/org/sonar/java/checks/MissingPathVariableAnnotationCheck.java similarity index 66% rename from java-checks/src/main/java/org/sonar/java/checks/PathVariableAnnotationShouldBePresentIfPathVariableIsUsedCheck.java rename to java-checks/src/main/java/org/sonar/java/checks/MissingPathVariableAnnotationCheck.java index c526efdcec8..51237df8aa2 100644 --- a/java-checks/src/main/java/org/sonar/java/checks/PathVariableAnnotationShouldBePresentIfPathVariableIsUsedCheck.java +++ b/java-checks/src/main/java/org/sonar/java/checks/MissingPathVariableAnnotationCheck.java @@ -33,16 +33,18 @@ import org.sonar.plugins.java.api.IssuableSubscriptionVisitor; import org.sonar.plugins.java.api.semantic.SymbolMetadata; import org.sonar.plugins.java.api.semantic.Type; +import org.sonar.plugins.java.api.tree.ClassTree; import org.sonar.plugins.java.api.tree.ExpressionTree; import org.sonar.plugins.java.api.tree.MethodTree; import org.sonar.plugins.java.api.tree.Tree; import org.sonar.plugins.java.api.tree.VariableTree; @Rule(key = "S6856") -public class PathVariableAnnotationShouldBePresentIfPathVariableIsUsedCheck extends IssuableSubscriptionVisitor { +public class MissingPathVariableAnnotationCheck extends IssuableSubscriptionVisitor { private static final String PATH_VARIABLE_ANNOTATION = "org.springframework.web.bind.annotation.PathVariable"; - private static final Pattern EXTRACT_PATH_VARIABLE = Pattern.compile("([^:}/]*)(:.*)?\\}.*"); - private static final Predicate CONTAINS_PLACEHOLDER = Pattern.compile("\\$\\{.*\\}").asPredicate(); + private static final String MODEL_ATTRIBUTE_ANNOTATION = "org.springframework.web.bind.annotation.ModelAttribute"; + private static final Pattern EXTRACT_PATH_VARIABLE = Pattern.compile("([^:}/]*)(:.*)?}.*"); + private static final Predicate CONTAINS_PLACEHOLDER = Pattern.compile("\\$\\{.*}").asPredicate(); private static final List MAPPING_ANNOTATIONS = List.of( "org.springframework.web.bind.annotation.GetMapping", "org.springframework.web.bind.annotation.PostMapping", @@ -51,28 +53,31 @@ public class PathVariableAnnotationShouldBePresentIfPathVariableIsUsedCheck exte @Override public List nodesToVisit() { - return List.of(Tree.Kind.METHOD); + return List.of(Tree.Kind.CLASS); } @Override public void visitNode(Tree tree) { - MethodTree method = (MethodTree) tree; + ClassTree clazzTree = (ClassTree) tree; - MAPPING_ANNOTATIONS - .forEach(annotation -> reportIssueOnParameters(method, annotation)); - } + List methods = clazzTree.members().stream() + .filter(member -> member.is(Tree.Kind.METHOD)) + .map(MethodTree.class::cast) + .collect(Collectors.toList()); - private void reportIssueOnParameters(MethodTree method, String annotation) { - boolean containsMap = method.parameters().stream() - .filter(parameter -> parameter.symbol().metadata().isAnnotatedWith(PATH_VARIABLE_ANNOTATION)) - .anyMatch(parameter -> { - Type type = parameter.type().symbolType(); - // if the type is not Map, Spring will throw a ClassCastException exception at runtime - boolean stringToString = type.typeArguments().stream().allMatch(typeArgument -> typeArgument.is("java.lang.String")); - return type.isSubtypeOf("java.util.Map") && stringToString; - }); + Set modelAttributePathVariable = methods.stream() + .filter(method -> method.symbol().metadata().isAnnotatedWith(MODEL_ATTRIBUTE_ANNOTATION)) + .flatMap(method -> method.parameters().stream()) + .map(MissingPathVariableAnnotationCheck::pathVariableName) + .flatMap(Optional::stream) + .collect(Collectors.toSet()); + + methods.forEach(method -> MAPPING_ANNOTATIONS + .forEach(annotation -> checkParameters(method, annotation, modelAttributePathVariable))); + } - if (containsMap) { + private void checkParameters(MethodTree method, String annotation, Set modelAttributePathVariable) { + if (containsMap(method)) { /* * If any of the method parameters is a map, we assume all path variables are captured * and there is no mismatch with path variables in the request mapping. @@ -80,29 +85,45 @@ private void reportIssueOnParameters(MethodTree method, String annotation) { return; } - Set pathVariablesNames = method.parameters().stream() - .map(variable -> pathVariableName(variable)) + Set unusedPathVariables = findUnusedPathVariables(method, annotation, modelAttributePathVariable); + if (!unusedPathVariables.isEmpty()) { + reportIssue( + annotation(method, annotation), + "Bind path variable \"" + String.join("\", \"", unusedPathVariables) + "\" to a method parameter."); + } + } + + private static Set findUnusedPathVariables(MethodTree method, String annotation, Set modelAttributePathVariable) { + Set pathVariablesUsedInArguments = method.parameters().stream() + .map(MissingPathVariableAnnotationCheck::pathVariableName) .flatMap(Optional::stream) .collect(Collectors.toSet()); - extractPathArgumentFromMappingAnnotations(method, annotation) - .map(path -> extractPathVariables(path)) - .map(pathVariables -> { - pathVariables.removeAll(pathVariablesNames); - return pathVariables; + return extractPathArgumentFromMappingAnnotations(method, annotation) + .map(MissingPathVariableAnnotationCheck::extractPathVariables) + .flatMap(pathVariables -> { + pathVariables.removeAll(pathVariablesUsedInArguments); + pathVariables.removeAll(modelAttributePathVariable); + return pathVariables.stream(); }) - .filter(pathVariables -> !pathVariables.isEmpty()) - .forEach(pathVariables -> reportIssue( - annotation(method, annotation), - "Bind path variable \"" + String.join("\", \"", pathVariables) + "\" to a method parameter.")); + .collect(Collectors.toSet()); + } + + private static boolean containsMap(MethodTree method) { + return method.parameters().stream() + .filter(parameter -> parameter.symbol().metadata().isAnnotatedWith(PATH_VARIABLE_ANNOTATION)) + .anyMatch(parameter -> { + Type type = parameter.type().symbolType(); + return type.isSubtypeOf("java.util.Map"); + }); } private static ExpressionTree annotation(MethodTree method, String name) { return method.modifiers().annotations().stream() .filter(annotation -> annotation.symbolType().is(name)) .findFirst() - // it will never be null because we are filtering on the annotation before. - .orElse(null); + // it will never be empty because we are filtering on the annotation before. + .orElseThrow(); } private static Set extractPathVariables(String path) { diff --git a/java-checks/src/test/java/org/sonar/java/checks/PathVariableAnnotationShouldBePresentIfPathVariableIsUsedCheckTest.java b/java-checks/src/test/java/org/sonar/java/checks/MissingPathVariableAnnotationCheckTest.java similarity index 79% rename from java-checks/src/test/java/org/sonar/java/checks/PathVariableAnnotationShouldBePresentIfPathVariableIsUsedCheckTest.java rename to java-checks/src/test/java/org/sonar/java/checks/MissingPathVariableAnnotationCheckTest.java index 1d4b3fa736c..3b1ca555602 100644 --- a/java-checks/src/test/java/org/sonar/java/checks/PathVariableAnnotationShouldBePresentIfPathVariableIsUsedCheckTest.java +++ b/java-checks/src/test/java/org/sonar/java/checks/MissingPathVariableAnnotationCheckTest.java @@ -23,13 +23,13 @@ import org.sonar.java.checks.verifier.CheckVerifier; import org.sonar.java.checks.verifier.TestUtils; -class PathVariableAnnotationShouldBePresentIfPathVariableIsUsedCheckTest { +class MissingPathVariableAnnotationCheckTest { @Test void test() { CheckVerifier.newVerifier() - .onFile(TestUtils.mainCodeSourcesPath("checks/PathVariableAnnotationShouldBePresentIfPathVariableIsUsedCheckSample.java")) - .withCheck(new PathVariableAnnotationShouldBePresentIfPathVariableIsUsedCheck()) + .onFile(TestUtils.mainCodeSourcesPath("checks/MissingPathVariableAnnotationCheckSample.java")) + .withCheck(new MissingPathVariableAnnotationCheck()) .verifyIssues(); } diff --git a/sonar-java-plugin/src/main/java/org/sonar/plugins/java/CheckList.java b/sonar-java-plugin/src/main/java/org/sonar/plugins/java/CheckList.java index 36fdbd07a04..bf2231312d4 100644 --- a/sonar-java-plugin/src/main/java/org/sonar/plugins/java/CheckList.java +++ b/sonar-java-plugin/src/main/java/org/sonar/plugins/java/CheckList.java @@ -279,7 +279,7 @@ import org.sonar.java.checks.OverwrittenKeyCheck; import org.sonar.java.checks.ParameterReassignedToCheck; import org.sonar.java.checks.ParsingErrorCheck; -import org.sonar.java.checks.PathVariableAnnotationShouldBePresentIfPathVariableIsUsedCheck; +import org.sonar.java.checks.MissingPathVariableAnnotationCheck; import org.sonar.java.checks.PopulateBeansCheck; import org.sonar.java.checks.PredictableSeedCheck; import org.sonar.java.checks.PreferStreamAnyMatchCheck; @@ -969,7 +969,7 @@ public final class CheckList { OverrideAnnotationCheck.class, OverwrittenKeyCheck.class, PasswordEncoderCheck.class, - PathVariableAnnotationShouldBePresentIfPathVariableIsUsedCheck.class, + MissingPathVariableAnnotationCheck.class, PersistentEntityUsedAsRequestParameterCheck.class, PopulateBeansCheck.class, PossessiveQuantifierContinuationCheck.class,