diff --git a/its/autoscan/src/test/resources/autoscan/autoscan-diff-by-rules.json b/its/autoscan/src/test/resources/autoscan/autoscan-diff-by-rules.json index 26081c34a82..b93a181759d 100644 --- a/its/autoscan/src/test/resources/autoscan/autoscan-diff-by-rules.json +++ b/its/autoscan/src/test/resources/autoscan/autoscan-diff-by-rules.json @@ -2939,6 +2939,12 @@ "falseNegatives": 2, "falsePositives": 0 }, + { + "ruleKey": "S6857", + "hasTruePositives": false, + "falseNegatives": 57, + "falsePositives": 0 + }, { "ruleKey": "S6862", "hasTruePositives": false, diff --git a/its/autoscan/src/test/resources/autoscan/diffs/diff_S1874.json b/its/autoscan/src/test/resources/autoscan/diffs/diff_S1874.json index 20e1f10dc01..0d3e11b01f7 100644 --- a/its/autoscan/src/test/resources/autoscan/diffs/diff_S1874.json +++ b/its/autoscan/src/test/resources/autoscan/diffs/diff_S1874.json @@ -1,6 +1,6 @@ { "ruleKey": "S1874", "hasTruePositives": true, - "falseNegatives": 103, + "falseNegatives": 102, "falsePositives": 0 -} +} \ No newline at end of file diff --git a/its/autoscan/src/test/resources/autoscan/diffs/diff_S2259.json b/its/autoscan/src/test/resources/autoscan/diffs/diff_S2259.json index 4f3754dc0d1..afd1e289cc8 100644 --- a/its/autoscan/src/test/resources/autoscan/diffs/diff_S2259.json +++ b/its/autoscan/src/test/resources/autoscan/diffs/diff_S2259.json @@ -1,6 +1,6 @@ { "ruleKey": "S2259", "hasTruePositives": true, - "falseNegatives": 4, + "falseNegatives": 3, "falsePositives": 0 } \ No newline at end of file diff --git a/its/autoscan/src/test/resources/autoscan/diffs/diff_S2583.json b/its/autoscan/src/test/resources/autoscan/diffs/diff_S2583.json index 4eae7420554..008c636a2db 100644 --- a/its/autoscan/src/test/resources/autoscan/diffs/diff_S2583.json +++ b/its/autoscan/src/test/resources/autoscan/diffs/diff_S2583.json @@ -1,6 +1,6 @@ { "ruleKey": "S2583", "hasTruePositives": true, - "falseNegatives": 21, + "falseNegatives": 12, "falsePositives": 0 } \ No newline at end of file diff --git a/its/autoscan/src/test/resources/autoscan/diffs/diff_S2589.json b/its/autoscan/src/test/resources/autoscan/diffs/diff_S2589.json index 4b37788c793..e33ebc90f34 100644 --- a/its/autoscan/src/test/resources/autoscan/diffs/diff_S2589.json +++ b/its/autoscan/src/test/resources/autoscan/diffs/diff_S2589.json @@ -1,6 +1,6 @@ { "ruleKey": "S2589", "hasTruePositives": true, - "falseNegatives": 5, + "falseNegatives": 3, "falsePositives": 0 } \ No newline at end of file diff --git a/its/autoscan/src/test/resources/autoscan/diffs/diff_S2637.json b/its/autoscan/src/test/resources/autoscan/diffs/diff_S2637.json index 59d10c6656e..1d6075f96a6 100644 --- a/its/autoscan/src/test/resources/autoscan/diffs/diff_S2637.json +++ b/its/autoscan/src/test/resources/autoscan/diffs/diff_S2637.json @@ -1,6 +1,6 @@ { "ruleKey": "S2637", "hasTruePositives": true, - "falseNegatives": 21, + "falseNegatives": 20, "falsePositives": 0 } \ No newline at end of file diff --git a/its/autoscan/src/test/resources/autoscan/diffs/diff_S3416.json b/its/autoscan/src/test/resources/autoscan/diffs/diff_S3416.json index aa73dabb9bb..edf15f79a60 100644 --- a/its/autoscan/src/test/resources/autoscan/diffs/diff_S3416.json +++ b/its/autoscan/src/test/resources/autoscan/diffs/diff_S3416.json @@ -1,6 +1,6 @@ { "ruleKey": "S3416", "hasTruePositives": true, - "falseNegatives": 6, + "falseNegatives": 5, "falsePositives": 0 } \ No newline at end of file diff --git a/its/autoscan/src/test/resources/autoscan/diffs/diff_S3516.json b/its/autoscan/src/test/resources/autoscan/diffs/diff_S3516.json index 0f30846588c..5442dbd84ff 100644 --- a/its/autoscan/src/test/resources/autoscan/diffs/diff_S3516.json +++ b/its/autoscan/src/test/resources/autoscan/diffs/diff_S3516.json @@ -1,6 +1,6 @@ { "ruleKey": "S3516", "hasTruePositives": true, - "falseNegatives": 11, + "falseNegatives": 6, "falsePositives": 0 } \ No newline at end of file diff --git a/its/autoscan/src/test/resources/autoscan/diffs/diff_S4449.json b/its/autoscan/src/test/resources/autoscan/diffs/diff_S4449.json index 4a4671652a8..3c45ec277a8 100644 --- a/its/autoscan/src/test/resources/autoscan/diffs/diff_S4449.json +++ b/its/autoscan/src/test/resources/autoscan/diffs/diff_S4449.json @@ -1,6 +1,6 @@ { "ruleKey": "S4449", - "hasTruePositives": false, - "falseNegatives": 27, + "hasTruePositives": true, + "falseNegatives": 23, "falsePositives": 0 } \ No newline at end of file diff --git a/its/autoscan/src/test/resources/autoscan/diffs/diff_S4790.json b/its/autoscan/src/test/resources/autoscan/diffs/diff_S4790.json index 1daa224ddb4..9b80778c68b 100644 --- a/its/autoscan/src/test/resources/autoscan/diffs/diff_S4790.json +++ b/its/autoscan/src/test/resources/autoscan/diffs/diff_S4790.json @@ -1,6 +1,6 @@ { "ruleKey": "S4790", "hasTruePositives": true, - "falseNegatives": 37, + "falseNegatives": 31, "falsePositives": 0 } \ No newline at end of file diff --git a/its/autoscan/src/test/resources/autoscan/diffs/diff_S6804.json b/its/autoscan/src/test/resources/autoscan/diffs/diff_S6804.json index 0b2a2f89b4e..afe70bee20c 100644 --- a/its/autoscan/src/test/resources/autoscan/diffs/diff_S6804.json +++ b/its/autoscan/src/test/resources/autoscan/diffs/diff_S6804.json @@ -1,6 +1,6 @@ { "ruleKey": "S6804", "hasTruePositives": false, - "falseNegatives": 12, + "falseNegatives": 48, "falsePositives": 0 -} +} \ No newline at end of file diff --git a/its/autoscan/src/test/resources/autoscan/diffs/diff_S6857.json b/its/autoscan/src/test/resources/autoscan/diffs/diff_S6857.json new file mode 100644 index 00000000000..d1147e3385a --- /dev/null +++ b/its/autoscan/src/test/resources/autoscan/diffs/diff_S6857.json @@ -0,0 +1,6 @@ +{ + "ruleKey": "S6857", + "hasTruePositives": false, + "falseNegatives": 68, + "falsePositives": 0 +} \ No newline at end of file diff --git a/java-checks-test-sources/default/src/main/java/checks/spring/SpelExpressionCheckSample.java b/java-checks-test-sources/default/src/main/java/checks/spring/SpelExpressionCheckSample.java new file mode 100644 index 00000000000..b5da31f271e --- /dev/null +++ b/java-checks-test-sources/default/src/main/java/checks/spring/SpelExpressionCheckSample.java @@ -0,0 +1,396 @@ +package checks.spring; + +import java.util.List; +import org.springframework.beans.factory.annotation.Value; +import org.springframework.data.mongodb.repository.Query; +import org.springframework.data.repository.query.Param; +import org.springframework.stereotype.Controller; +import org.springframework.web.bind.annotation.RequestMapping; + +public class SpelExpressionCheckSample { + + private static final String UNCLOSED = "${1 + 2 + 3"; + private static final String INVALID_SPEL = "#{1 * * 2}"; + private static final String INVALID_PROPERTY_PLACEHOLDER = "${foo.bar[}"; + private static final String VALID_PROPERTY_PLACEHOLDER = "${foo.bar}"; + + @Value(UNCLOSED) // Noncompliant [[sc=10;ec=18]] {{Add missing '}' for this property placeholder or SpEL expression.}} + private String complexArgument1; + + @Value(INVALID_SPEL) // Noncompliant [[sc=10;ec=22]] {{Correct this malformed SpEL expression.}} + private String complexArgument2; + + @Value(INVALID_PROPERTY_PLACEHOLDER) // Noncompliant [[sc=10;ec=38]] {{Correct this malformed property placeholder.}} + private String complexArgument3; + + @Value(value = UNCLOSED) // Noncompliant [[sc=18;ec=26]] {{Add missing '}' for this property placeholder or SpEL expression.}} + private String complexArgument4; + + @Value(value = INVALID_SPEL) // Noncompliant [[sc=18;ec=30]] {{Correct this malformed SpEL expression.}} + private String complexArgument5; + + @Value(value = INVALID_PROPERTY_PLACEHOLDER) // Noncompliant [[sc=18;ec=46]] {{Correct this malformed property placeholder.}} + private String complexArgument6; + + @Value(value = "${1 + 2 + 3") // Noncompliant [[sc=19;ec=30]] {{Add missing '}' for this property placeholder or SpEL expression.}} + private String complexArgument7; + + @Value(value = "#{1 * * 2}") // Noncompliant [[sc=19;ec=29]] {{Correct this malformed SpEL expression.}} + private String complexArgument8; + + @Value(value = "${foo.bar[}") // Noncompliant [[sc=19;ec=30]] {{Correct this malformed property placeholder.}} + private String complexArgument9; + + @Value(value = "#{1 + 2 + 3}") // Compliant + private String complexArgument10; + + @Value(value = VALID_PROPERTY_PLACEHOLDER) // Compliant + private String complexArgument11; + + @Value("#{systemProperties['user.region'}") // Noncompliant [[sc=11;ec=44]] {{Correct this malformed SpEL expression.}} + private String region1; + + @Value("#{systemProperties['user.region']}") // Compliant + private String region2; + + @Value("${user.region}") // Compliant + private String region3; + + @Value("${user.2region}") // Noncompliant + private String region4; + + @Value("${user.region:defaultRegion}") // Compliant + private String multi1; + + @Value("${user.region::defaultRegion}") // Noncompliant {{Correct this malformed property placeholder.}} + private String multi2; + + @Value("${:user.region:defaultRegion}") // Noncompliant {{Correct this malformed property placeholder.}} + private String multi3; + + @Value("${user.region:defaultRegion:}") // Noncompliant {{Correct this malformed property placeholder.}} + private String multi4; + + @Value("${ user.region : defaultRegion }") // Compliant + private String multi5; + + @Value("${user.region:#{null}}") // Compliant + private String multi6; + + @Value("${user.region:#{ null }}") // Compliant + private String multi7; + + @Value("${user.region:#{ null + 3 }}") // Compliant + private String multi8; + + @Value("${user.region:#{ null + * 3 }}") // Noncompliant [[sc=25;ec=41]] {{Correct this malformed SpEL expression.}} + private String multi9; + + @Value("${user.region:#{'D'+'E'}}") // Compliant + private String multi10; + + @Value("${user.region:#{null}:#{null}:foo.bar}") // Compliant + private String multi11; + + @Value("${user.region:#{null}:#{4**4}:foo.bar}") // Noncompliant [[sc=33;ec=40]] {{Correct this malformed SpEL expression.}} + private String multi12; + + @Value("${user.region:#{null}:#{4*4}:foo.bar}") // Compliant + private String multi13; + + @Value("${user.region:#{null}:#{4*4}:foo..bar}") // Noncompliant + private String multi14; + + @Value("${user.region:#{4**4}:#{4**4}:foo.bar}") // Noncompliant + private String multi15; + + @Value("${:defaultRegion}") // Noncompliant + private String multi16; + + @Value("${user.2region:default-region}") // Noncompliant + private String multi17; + + @Value("#{'${listOfValues}' split(',')}") // Noncompliant [[sc=11;ec=42]] {{Correct this malformed SpEL expression.}} + private List valuesListNc; + + @Value("#{'${listOfValues}'.split(',')}") // Compliant + private List valuesListC; + + @Value("#{T(java.lang.Math).random() * 64h}") // Noncompliant [[sc=11;ec=46]] {{Correct this malformed SpEL expression.}} + private Double randPercentNc; + + @Value("#{T(java.lang.Math).random() * 100.0}") // Compliant + private Double randPercentC; + + static class User { + String status; + } + + static class Customer { + String firstname; + String name; + } + + interface Repo { + + @Query("select u from User u where u.age = ?#{[0]}") // Compliant + List findUsersByAge1(int age); + + @Query("select u from User u where u.age = ?#{[0}") // Noncompliant [[sc=49;ec=54]] + List findUsersByAge2(int age); + + @Query("select u from User u where u.age = ?#{[0]") // Noncompliant [[sc=49;ec=54]] {{Add missing '}' for this property placeholder or SpEL expression.}} + List findUsersByAge3(int age); + + @Query("select u from User u where u.age = ?#{[0*]}") // Noncompliant + List findUsersByAge4(int age); + + @Query("select u from User u where u.firstname = :#{#customer.firstname}") // Compliant + List findUsersByCustomersFirstname1(@Param("customer") Customer customer); + + @Query("select u from User u where u.firstname = :#{#customer firstname}") // Noncompliant + List findUsersByCustomersFirstname2(@Param("customer") Customer customer); + + @Query("select u from User u where u.name = :#{#customer.name} and u.firstname = :#{#customer.firstname}") // Compliant + List findUsersByCustomersFullName1(@Param("customer") Customer customer); + + @Query("select u from User u where u.name = :#{#customer.name} and u.firstname = :#{#customer.firstname") // Noncompliant {{Add missing '}' for this property placeholder or SpEL expression.}} + List findUsersByCustomersFullName2(@Param("customer") Customer customer); + + @Query("select u from User u where u.name = :#{#customer.name and u.firstname = :#{#customer.firstname}") // Noncompliant {{Add missing '}' for this property placeholder or SpEL expression.}} + List findUsersByCustomersFullName3(@Param("customer") Customer customer); + + @Query("select u from User u where u.name = :#{#customer.name and u.firstname} = :#{#*customer.firstname}") // Noncompliant {{Correct this malformed SpEL expression.}} + List findUsersByCustomersFullName4(@Param("customer") Customer customer); + + @Query("select u from User u where u.firstname = :#{#customer.firstname} and u.role=${admin}") // Compliant + List findAdminUsersByFirstname1(@Param("customer") Customer customer); + + @Query("select u from User u where u.firstname = :#{#customer.firstname} and u.role=${admin") // Noncompliant + List findAdminUsersByFirstname2(@Param("customer") Customer customer); + } + + @Controller + @RequestMapping("#{1+2+3}") // Compliant + public static class RequestController1 { } + + @Controller + @RequestMapping("#{1+2+}") // Noncompliant [[sc=20;ec=27]] + public static class RequestController2 { } + + @Value("foo") // Compliant + String noTemplate0; + + @Value("foo.bar") // Compliant + String noTemplate1; + + @Value("foo[10]") // Compliant + String noTemplate2; + + @Value("foo[10][20]") // Compliant + String noTemplate3; + + @Value("foo[10].bar") // Compliant + String noTemplate4; + + @Value("foo.bar[10][20].baz") // Compliant + String noTemplate5; + + @Value("{}") // Compliant + String delimiters0; + + @Value("{123") // Compliant + String delimiters1; + + @Value("123}") // Compliant + String delimiters2; + + @Value("$") // Compliant + String delimiters3; + + @Value("$ ") // Compliant + String delimiters4; + + @Value("#") // Compliant + String delimiters5; + + @Value("# ") // Compliant + String delimiters6; + + @Value("$123") // Compliant + String delimiters7; + + @Value("$foo") // Compliant + String delimiters8; + + @Value("${}") // Noncompliant [[sc=11;ec=14]] + String delimiters9; + + @Value("${123") // Noncompliant + String delimiters10; + + @Value("$123}") // Compliant + String delimiters11; + + @Value("#{}") // Noncompliant + String delimiters12; + + @Value("#{123") // Noncompliant + String delimiters13; + + @Value("#123}") // Compliant + String delimiters14; + + @Value("#{123}}") // Compliant + String delimiters15; + + @Value("#{{123}") // Noncompliant + String delimiters16; + + @Value("#{12{}3}") // Noncompliant, open + String delimiters17; + + @Value("#{{12}3{}") // Noncompliant, open + String delimiters18; + + @Value("#{{12}3{4{5}6}") // Noncompliant + String delimiters19; + + @Value("{ }") // Compliant + String delimiters20; + + @Value("${ }") // Noncompliant {{Correct this malformed property placeholder.}} + String delimiters21; + + @Value("#{ }") // Noncompliant + String delimiters22; + + @Value("${") // Noncompliant [[sc=11;ec=13]] + String delimiters23; + + @Value("${ ") // Noncompliant [[sc=11;ec=14]] + String delimiters24; + + @Value("#{ ") // Noncompliant [[sc=11;ec=14]] + String delimiters25; + + @Value("#{ " + "") // Noncompliant + String delimiters25_2; + + @Value("$ {") // Compliant + String delimiters26; + + @Value("# {") // Compliant + String delimiters27; + + @Value("$}") // Compliant + String delimiters28; + + @Value("$ }") // Compliant + String delimiters29; + + @Value("# }") // Compliant + String delimiters30; + + @Value("${3foo}") // Noncompliant + String ncPlaceholder0; + + @Value("${foo bar}") // Noncompliant + String ncPlaceholder1; + + @Value("${foo .bar}") // Noncompliant + String ncPlaceholder2; + + @Value("${foo,bar}") // Noncompliant + String ncPlaceholder3; + + @Value("${foo..bar}") // Noncompliant {{Correct this malformed property placeholder.}} + String ncPlaceholder4; + + @Value("${foo.}") // Noncompliant {{Correct this malformed property placeholder.}} + String ncPlaceholder5; + + @Value("${.bar}") // Noncompliant + String ncPlaceholder6; + + @Value("${foo[10}") // Noncompliant + String ncPlaceholder7; + + @Value("${foo 10]}") // Noncompliant + String ncPlaceholder8; + + @Value("${foo[10 20]}") // Noncompliant + String ncPlaceholder9; + + @Value("${foo[10.bar]}") // Noncompliant + String ncPlaceholder10; + + @Value("${foo.bar[][20].baz}") // Noncompliant + String ncPlaceholder11; + + @Value("${foo[]}") // Noncompliant + String ncPlaceholder12; + + @Value("${[10]}") // Noncompliant + String ncPlaceholder13; + + @Value("${[[10]}") // Noncompliant + String ncPlaceholder14; + + @Value("${foo + bar}") // Noncompliant + String ncPlaceholder15; + + @Value("${foo}") // Compliant + String cPlaceholder0; + + @Value("${foo.bar}") // Compliant + String cPlaceholder1; + + @Value("${foo[10]}") // Compliant + String cPlaceholder2; + + @Value("${foo[10][20]}") // Compliant + String cPlaceholder3; + + @Value("${foo[10].bar}") // Compliant + String cPlaceholder4; + + @Value("${foo.bar[10][20].baz}") // Compliant + String cPlaceholder5; + + @Value("#{foo bar}") // Noncompliant + String spel0; + + @Value("#{foo .bar}") // Compliant + String spel1; + + @Value("#{foo,bar}") // Noncompliant + String spel2; + + @Value("#{foo}") // Compliant + String spel3; + + @Value("#{foo.bar}") // Compliant + String spel4; + + @Value("#{10 * (20 + foo)}") // Compliant + String spel5; + + @Value("#{foo[10].bar}") // Compliant + String spel6; + + @Value("#{foo.bar[10][20].baz}") // Compliant + String spel7; + + @Value("#(123))") // Compliant + String spel8; + + @Value("#())") // Compliant + String spel9; + + @Value("#{()})") // Noncompliant + String spel10; + + @Value("#{(42)})") // Compliant + String spel11; +} diff --git a/java-checks/pom.xml b/java-checks/pom.xml index fe1b3f0d372..34f5c8ddc44 100644 --- a/java-checks/pom.xml +++ b/java-checks/pom.xml @@ -74,7 +74,10 @@ org.apache.commons commons-lang3 - + + org.springframework + spring-expression + org.junit.jupiter junit-jupiter diff --git a/java-checks/src/main/java/org/sonar/java/checks/spring/SpelExpressionCheck.java b/java-checks/src/main/java/org/sonar/java/checks/spring/SpelExpressionCheck.java new file mode 100644 index 00000000000..c89073cdeaf --- /dev/null +++ b/java-checks/src/main/java/org/sonar/java/checks/spring/SpelExpressionCheck.java @@ -0,0 +1,301 @@ +/* + * SonarQube Java + * Copyright (C) 2012-2023 SonarSource SA + * mailto:info AT sonarsource DOT com + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 3 of the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public License + * along with this program; if not, write to the Free Software Foundation, + * Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. + */ +package org.sonar.java.checks.spring; + +import java.util.Collection; +import java.util.Collections; +import java.util.List; +import java.util.Map; +import java.util.Objects; +import java.util.function.ObjIntConsumer; +import java.util.regex.Pattern; +import java.util.stream.Stream; +import javax.annotation.CheckForNull; +import org.sonar.check.Rule; +import org.sonar.java.checks.helpers.ExpressionsHelper; +import org.sonar.java.model.DefaultJavaFileScannerContext; +import org.sonar.java.reporting.AnalyzerMessage; +import org.sonar.plugins.java.api.IssuableSubscriptionVisitor; +import org.sonar.plugins.java.api.location.Position; +import org.sonar.plugins.java.api.tree.AnnotationTree; +import org.sonar.plugins.java.api.tree.AssignmentExpressionTree; +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; +import org.springframework.expression.ParseException; +import org.springframework.expression.spel.standard.SpelExpressionParser; + +@Rule(key = "S6857") +public class SpelExpressionCheck extends IssuableSubscriptionVisitor { + + private static final String SPRING_PREFIX = "org.springframework"; + + /** + * Regular expression for a property placeholder segment that is not a SpEL expression. + * It implements the following grammar with possessive quantifiers: + *

+ * 

+   * PropertyPlaceholder ::= Identifier IndexExpression* ("." Identifier IndexExpression*)*
+   * Identifier          ::= [a-zA-Z_][a-zA-Z0-9_]*
+   * IndexExpression     ::= "[" [0-9]+ "]"
+   *
+ *

+ * Some examples for accepted inputs: + *

+ * 

+   * foo
+   * foo.bar
+   * foo[42].bar23
+   * bar[23][42]
+   *
+ */ + private static final Pattern PROPERTY_PLACEHOLDER_PATTERN = Pattern.compile( + "[a-zA-Z_]\\w*+(\\[\\d++])*+(\\.[a-zA-Z_]\\w*+(\\[\\d++])*+)*+" + ); + + public List nodesToVisit() { + return List.of(Tree.Kind.CLASS, Tree.Kind.INTERFACE); + } + + @Override + public void visitNode(Tree tree) { + getClassAndMemberAnnotations((ClassTree) tree) + .filter(SpelExpressionCheck::isSpringAnnotation) + .forEach(this::checkSpringAnnotationArguments); + } + + private static Stream getClassAndMemberAnnotations(ClassTree cls) { + return Stream.concat( + Stream.of(cls.modifiers().annotations()), + cls.members().stream().map(SpelExpressionCheck::getMemberAnnotations) + ).flatMap(Collection::stream); + } + + private static List getMemberAnnotations(Tree member) { + if (member.is(Tree.Kind.METHOD)) { + return ((MethodTree) member).modifiers().annotations(); + } else if (member.is(Tree.Kind.VARIABLE)) { + return ((VariableTree) member).modifiers().annotations(); + } else { + return Collections.emptyList(); + } + } + + private static boolean isSpringAnnotation(AnnotationTree annotation) { + return annotation.symbolType().fullyQualifiedName().startsWith(SPRING_PREFIX); + } + + private void checkSpringAnnotationArguments(AnnotationTree annotation) { + annotation.arguments().stream().map(SpelExpressionCheck::extractArgumentValue).filter(Objects::nonNull) + .forEach(this::checkSpringExpressionsInString); + } + + @CheckForNull + private static Map.Entry extractArgumentValue(ExpressionTree expression) { + expression = getExpressionOrAssignmentRhs(expression); + var stringValue = ExpressionsHelper.getConstantValueAsString(expression).value(); + if (stringValue == null) { + return null; + } + return Map.entry(expression, stringValue); + } + + private static ExpressionTree getExpressionOrAssignmentRhs(ExpressionTree expression) { + return expression.is(Tree.Kind.ASSIGNMENT) ? ((AssignmentExpressionTree) expression).expression() : expression; + } + + private void checkSpringExpressionsInString(Map.Entry entry) { + var expression = entry.getKey(); + try { + var argValue = entry.getValue(); + if (expression.is(Tree.Kind.STRING_LITERAL)) { + checkStringContents(argValue, 1); + } else { + checkStringContents(argValue, 0); + } + } catch (SyntaxError e) { + reportIssue(expression, e); + } + } + + private void reportIssue(Tree expression, SyntaxError error) { + if (expression.is(Tree.Kind.STRING_LITERAL)) { + // For string literals, report exact issue location within the string. + var tokenStart = Position.startOf(expression); + var textSpan = new AnalyzerMessage.TextSpan( + tokenStart.line(), + tokenStart.columnOffset() + error.startColumn, + tokenStart.line(), + tokenStart.columnOffset() + error.endColumn + ); + + var analyzerMessage = new AnalyzerMessage(this, context.getInputFile(), textSpan, error.getMessage(), 0); + ((DefaultJavaFileScannerContext) context).reportIssue(analyzerMessage); + } else { + reportIssue(expression, error.getMessage()); + } + } + + private static void checkStringContents(String content, int startColumn) throws SyntaxError { + var i = 0; + while (i < content.length()) { + var c = content.charAt(i); + switch (c) { + case '$': + i = parseDelimitersAndContents(content, i + 1, startColumn + i, SpelExpressionCheck::parseValidPropertyPlaceholder); + break; + case '#': + i = parseDelimitersAndContents(content, i + 1, startColumn + i, SpelExpressionCheck::parseValidSpelExpression); + break; + default: + i++; + break; + } + } + } + + /** + * Parses the following grammatical expression, starting at startIndex in `value`: + * + * 
+   * ('{' contents '}')?
+   *
+ *

+ * Where correct bracing is checked and then contents is parsed using the given parseContents function. + * + * @param value string containing the character sequence to parse + * @param startIndex index of the opening delimiter we start from in value + * @param startColumn offset with the position of value within a potentially longer original string (used for reporting) + * @param parseContents function to parse contents + * @throws SyntaxError when the input does not comply with the expected grammatical expression + */ + private static int parseDelimitersAndContents( + String value, + int startIndex, + int startColumn, + ObjIntConsumer parseContents + ) throws SyntaxError { + if (startIndex == value.length()) { + return startIndex; + } + var endIndex = parseDelimiterBraces(value, startIndex, startColumn); + if (endIndex == startIndex) { + return endIndex; + } + var contents = value.substring(startIndex + 1, endIndex - 1); + parseContents.accept(contents, startColumn); + return endIndex; + } + + private static int parseDelimiterBraces(String value, int startIndex, int startColumn) throws SyntaxError { + if (value.charAt(startIndex) != '{') { + return startIndex; + } + + int openCount = 1; + for (var i = startIndex + 1; i < value.length(); i++) { + var c = value.charAt(i); + if (c == '{') { + openCount++; + } else if (c == '}') { + openCount--; + if (openCount == 0) { + return i + 1; + } + } + } + + // +1 because of prefix `$` or `#` + var endColumn = startColumn + value.length() - startIndex + 1; + throw new SyntaxError("Add missing '}' for this property placeholder or SpEL expression.", startColumn, endColumn); + } + + private static void parseValidPropertyPlaceholder(String placeholder, int startColumn) throws SyntaxError { + if (!isValidPropertyPlaceholder(placeholder, startColumn)) { + // +3 because of delimiter `#{` and `}` + var endColumn = startColumn + placeholder.length() + 3; + throw new SyntaxError("Correct this malformed property placeholder.", startColumn, endColumn); + } + } + + private static boolean isValidPropertyPlaceholder(String placeholder, int startColumn) throws SyntaxError { + var startIndex = 0; + var endIndex = placeholder.indexOf(':'); + + while (endIndex != -1) { + var segment = placeholder.substring(startIndex, endIndex); + if (!isValidPropertyPlaceholderSegment(segment, startColumn + startIndex)) { + return false; + } + startIndex = endIndex + 1; + endIndex = placeholder.indexOf(':', startIndex); + } + var segment = placeholder.substring(startIndex); + return isValidPropertyPlaceholderSegment(segment, startColumn + startIndex); + } + + private static boolean isValidPropertyPlaceholderSegment(String segment, int startColumn) throws SyntaxError { + var stripped = segment.stripLeading(); + startColumn += segment.length() - stripped.length(); + stripped = stripped.stripTrailing(); + + if (stripped.startsWith("#{")) { + parseDelimitersAndContents(stripped, 1, startColumn + 2, SpelExpressionCheck::parseValidSpelExpression); + return true; + } else { + return PROPERTY_PLACEHOLDER_PATTERN.matcher(stripped).matches(); + } + } + + private static void parseValidSpelExpression(String expressionString, int startColumn) throws SyntaxError { + if (!isValidSpelExpression(expressionString)) { + // +3 because of delimiter `${` and `}` + var endColumn = startColumn + expressionString.length() + 3; + throw new SyntaxError("Correct this malformed SpEL expression.", startColumn, endColumn); + } + } + + private static boolean isValidSpelExpression(String expressionString) { + expressionString = expressionString.strip(); + if (expressionString.isEmpty()) { + return false; + } + try { + new SpelExpressionParser().parseExpression(expressionString); + } catch (ParseException | IllegalStateException e) { + return false; + } + return true; + } + + private static class SyntaxError extends RuntimeException { + + SyntaxError(String message, int startColumn, int endColumn) { + super(message); + this.startColumn = startColumn; + this.endColumn = endColumn; + } + + public final int startColumn; + public final int endColumn; + } +} diff --git a/java-checks/src/test/java/org/sonar/java/checks/spring/SpelExpressionCheckTest.java b/java-checks/src/test/java/org/sonar/java/checks/spring/SpelExpressionCheckTest.java new file mode 100644 index 00000000000..bcba6bbadd1 --- /dev/null +++ b/java-checks/src/test/java/org/sonar/java/checks/spring/SpelExpressionCheckTest.java @@ -0,0 +1,36 @@ +/* + * SonarQube Java + * Copyright (C) 2012-2023 SonarSource SA + * mailto:info AT sonarsource DOT com + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 3 of the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public License + * along with this program; if not, write to the Free Software Foundation, + * Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. + */ +package org.sonar.java.checks.spring; + +import org.junit.jupiter.api.Test; +import org.sonar.java.checks.verifier.CheckVerifier; + +import static org.sonar.java.checks.verifier.TestUtils.mainCodeSourcesPath; + +class SpelExpressionCheckTest { + + @Test + void test() { + CheckVerifier.newVerifier() + .onFile(mainCodeSourcesPath("checks/spring/SpelExpressionCheckSample.java")) + .withCheck(new SpelExpressionCheck()) + .verifyIssues(); + } +} diff --git a/pom.xml b/pom.xml index b522e6612ff..aa82a4c5938 100644 --- a/pom.xml +++ b/pom.xml @@ -266,6 +266,11 @@ commons-lang3 3.12.0 + + org.springframework + spring-expression + 6.1.1 + commons-io commons-io diff --git a/sonar-java-plugin/pom.xml b/sonar-java-plugin/pom.xml index c959f729325..4a2601dbac1 100644 --- a/sonar-java-plugin/pom.xml +++ b/sonar-java-plugin/pom.xml @@ -168,8 +168,8 @@ - 18500000 - 17000000 + 21500000 + 20000000 ${project.build.directory}/${project.build.finalName}.jar 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 bf2231312d4..dd3707be2bd 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 @@ -572,6 +572,7 @@ import org.sonar.java.checks.spring.OptionalRestParametersShouldBeObjectsCheck; import org.sonar.java.checks.spring.PersistentEntityUsedAsRequestParameterCheck; import org.sonar.java.checks.spring.RequestMappingMethodPublicCheck; +import org.sonar.java.checks.spring.SpelExpressionCheck; import org.sonar.java.checks.spring.SpringAntMatcherOrderCheck; import org.sonar.java.checks.spring.SpringAutoConfigurationCheck; import org.sonar.java.checks.spring.SpringBeanNamingConventionCheck; @@ -675,6 +676,7 @@ public final class CheckList { public static final String REPOSITORY_KEY = "java"; private static final List> JAVA_MAIN_CHECKS = Arrays.asList( + // fast JavaFileScanner (not IssuableSubscriptionVisitor) ordered from the fastest to the slowest LeftCurlyBraceEndLineCheck.class, IndentationCheck.class, @@ -1046,6 +1048,7 @@ public final class CheckList { SingleCharCharacterClassCheck.class, SingletonUsageCheck.class, SpecializedFunctionalInterfacesCheck.class, + SpelExpressionCheck.class, SpringAntMatcherOrderCheck.class, SpringAutoConfigurationCheck.class, SpringBeanNamingConventionCheck.class, diff --git a/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S6857.html b/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S6857.html new file mode 100644 index 00000000000..a3276a1bbc9 --- /dev/null +++ b/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S6857.html @@ -0,0 +1,52 @@ +

This rule reports syntax errors in Spring Expression Language (SpEL) expressions.

+

Why is this an issue?

+

SpEL is used in Spring annotations and is parsed by the Spring framework, not by the Java compiler. This means that invalid SpEL expressions are +not detected during Java compile time. They will cause exceptions during runtime instead, or even fail silently with the expression string interpreted +as a simple string literal by Spring.

+

Exceptions

+

This rule does report syntactical errors in SpEL expressions but does not consider semantic errors, such as unknown identifiers or incompatible +operand data types.

+

How to fix it

+

Correct the syntax error in the SpEL expression.

+

Code examples

+

Noncompliant code example

+
+@Value("#{systemProperties['user.region'}") // Noncompliant, unclosed "["
+private String region;
+
+
+@Value("#{'${listOfValues}' split(',')}") // Noncompliant, missing operator
+private List<String> valuesList;
+
+
+@Value("#{T(java.lang.Math).random() * 64h}") // Noncompliant, invalid number
+private Double randPercent;
+
+
+@Query("SELECT u FROM User u WHERE u.status = :#{#status+}") // Noncompliant, missing operand for "+"
+List<User> findUsersByStatus(@Param("status") String status);
+
+

Compliant solution

+
+@Value("#{systemProperties['user.region']}") // Compliant
+private String region;
+
+
+@Value("#{'${listOfValues}'.split(',')}") // Compliant
+private List<String> valuesList;
+
+
+@Value("#{T(java.lang.Math).random() * 100.0}") // Compliant
+private Double randPercent;
+
+
+@Query("SELECT u FROM User u WHERE u.status = :#{#status+42}") // Compliant
+List<User> findUsersByStatus(@Param("status") String status);
+
+

Resources

+

Documentation

+ + diff --git a/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S6857.json b/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S6857.json new file mode 100644 index 00000000000..254b2429fc2 --- /dev/null +++ b/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S6857.json @@ -0,0 +1,23 @@ +{ + "title": "SpEL expression should have a valid syntax", + "type": "BUG", + "status": "ready", + "remediation": { + "func": "Constant\/Issue", + "constantCost": "5min" + }, + "tags": [ + "spring" + ], + "defaultSeverity": "Major", + "ruleSpecification": "RSPEC-6857", + "sqKey": "S6857", + "scope": "All", + "quickfix": "unknown", + "code": { + "impacts": { + "RELIABILITY": "HIGH" + }, + "attribute": "CONVENTIONAL" + } +} diff --git a/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/Sonar_way_profile.json b/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/Sonar_way_profile.json index 991707121ea..a3dd5ba01a7 100644 --- a/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/Sonar_way_profile.json +++ b/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/Sonar_way_profile.json @@ -497,6 +497,7 @@ "S6837", "S6838", "S6856", + "S6857", "S6862" ] }