diff --git a/its/ruling/src/test/resources/sonar-server/java-S1075.json b/its/ruling/src/test/resources/sonar-server/java-S1075.json index 2e5eda5f67e..b526a47868b 100644 --- a/its/ruling/src/test/resources/sonar-server/java-S1075.json +++ b/its/ruling/src/test/resources/sonar-server/java-S1075.json @@ -1,7 +1,4 @@ { -"org.sonarsource.sonarqube:sonar-server:src/main/java/org/sonar/server/authentication/AuthenticationError.java": [ -35 -], "org.sonarsource.sonarqube:sonar-server:src/main/java/org/sonar/server/authentication/AuthenticationFilter.java": [ 34 ] diff --git a/java-checks-test-sources/default/src/main/files/non-compiling/checks/HardcodedURICheckSample.java b/java-checks-test-sources/default/src/main/files/non-compiling/checks/HardcodedURICheckSample.java new file mode 100644 index 00000000000..4e52edeaf85 --- /dev/null +++ b/java-checks-test-sources/default/src/main/files/non-compiling/checks/HardcodedURICheckSample.java @@ -0,0 +1,10 @@ +package checks; + +public class HardcodedURICheckSample { + + String path = "/home/path/to/my/file.txt"; // Noncompliant + + String aVarPath = "/home/path/to/my/file.txt"; // FN, missing semantics + @MyAnnotation(aVarPath = "") + int x = 0; +} diff --git a/java-checks-test-sources/default/src/main/java/checks/HardcodedURICheckSample.java b/java-checks-test-sources/default/src/main/java/checks/HardcodedURICheckSample.java index 1a3a30b5ec4..8ecbcc7419c 100644 --- a/java-checks-test-sources/default/src/main/java/checks/HardcodedURICheckSample.java +++ b/java-checks-test-sources/default/src/main/java/checks/HardcodedURICheckSample.java @@ -8,6 +8,8 @@ class HardcodedURICheckSample { public static @interface MyAnnotation { String stuff() default "none"; + // we cannot name method path otherwise path is detected as an identifier used in annotation + //, and it creates clashes (FN) with path variables or fields String path() default "/"; } @@ -62,4 +64,30 @@ void foo(String s, String var) throws URISyntaxException { String v1 = s + "//" + s; // Compliant - not a file name } + + @interface MyAnnotation2 { + String aVar() default ""; + } + + static final String relativePath1 = "/search"; // Compliant, we don't raise issues on short relative uri in constants + static final String relativePath2 = "/group/members"; + static final String longRelativePath = "/group/members/list.json"; // Noncompliant + static final String urlPath = "https://www.mywebsite.com"; // Noncompliant + final String staticIsMissingPath = "/search"; // Noncompliant + static String finalIsMissingPath = "/search"; // Noncompliant + + static final String default_uri_path = "/a-great/path/for-this-example"; // Compliant, default_uri is constant and is used in an annotation + String aVarPath = "/a-great/path/for-this-example"; // Noncompliant + + @MyAnnotation2(aVar = default_uri_path) + void annotated(){} + + @MyAnnotation2() + String endpoint_url_path = "/a-great/path/for-this-example"; // Compliant, an annotation is applied on the variable + + void reachFullCoverage(){ + int path = 0; + path = 10; + } + } diff --git a/java-checks/src/main/java/org/sonar/java/checks/HardcodedURICheck.java b/java-checks/src/main/java/org/sonar/java/checks/HardcodedURICheck.java index f2e7975777a..63f18525b9b 100644 --- a/java-checks/src/main/java/org/sonar/java/checks/HardcodedURICheck.java +++ b/java-checks/src/main/java/org/sonar/java/checks/HardcodedURICheck.java @@ -16,16 +16,25 @@ */ package org.sonar.java.checks; +import java.util.ArrayDeque; +import java.util.ArrayList; import java.util.Arrays; +import java.util.Deque; +import java.util.HashSet; import java.util.List; +import java.util.Set; import java.util.regex.Pattern; import javax.annotation.CheckForNull; import javax.annotation.Nullable; import org.sonar.check.Rule; import org.sonar.java.model.ExpressionUtils; import org.sonar.java.model.LiteralUtils; +import org.sonar.java.model.ModifiersUtils; import org.sonar.plugins.java.api.IssuableSubscriptionVisitor; +import org.sonar.plugins.java.api.JavaFileScannerContext; import org.sonar.plugins.java.api.semantic.MethodMatchers; +import org.sonar.plugins.java.api.semantic.Symbol; +import org.sonar.plugins.java.api.tree.AnnotationTree; import org.sonar.plugins.java.api.tree.AssignmentExpressionTree; import org.sonar.plugins.java.api.tree.BaseTreeVisitor; import org.sonar.plugins.java.api.tree.BinaryExpressionTree; @@ -33,6 +42,7 @@ import org.sonar.plugins.java.api.tree.IdentifierTree; import org.sonar.plugins.java.api.tree.LiteralTree; import org.sonar.plugins.java.api.tree.MemberSelectExpressionTree; +import org.sonar.plugins.java.api.tree.Modifier; import org.sonar.plugins.java.api.tree.NewClassTree; import org.sonar.plugins.java.api.tree.Tree; import org.sonar.plugins.java.api.tree.VariableTree; @@ -67,20 +77,86 @@ public class HardcodedURICheck extends IssuableSubscriptionVisitor { private static final Pattern URI_PATTERN = Pattern.compile(URI_REGEX + "|" + LOCAL_URI + "|" + DISK_URI + "|" + BACKSLASH_LOCAL_URI); private static final Pattern VARIABLE_NAME_PATTERN = Pattern.compile("filename|path", Pattern.CASE_INSENSITIVE); private static final Pattern PATH_DELIMETERS_PATTERN = Pattern.compile("\"/\"|\"//\"|\"\\\\\\\\\"|\"\\\\\\\\\\\\\\\\\""); + private static final Pattern RELATIVE_URI_PATTERN = Pattern.compile("^(/[\\w-+!*.]+){1,2}"); + + + // we use these variables to track when we are visiting an annotation + private final Deque annotationsStack = new ArrayDeque<>(); + + private record IdentifierData(Symbol symbol, String identifier) { + } + + private final List identifiersUsedInAnnotations = new ArrayList<>(); + + private record VariableData(Symbol symbol, String identifier, ExpressionTree initializer) { + } + + private final List hardCodedUri = new ArrayList<>(); + + @Override + public void setContext(JavaFileScannerContext context) { + super.setContext(context); + annotationsStack.clear(); + identifiersUsedInAnnotations.clear(); + hardCodedUri.clear(); + } + + @Override + public void leaveFile(JavaFileScannerContext context) { + // now, we know all variable that are used in annotation so we can report issues + Set idSymbols = new HashSet<>(); + Set idNamesWithSemantic = new HashSet<>(); + Set idNamesWithoutSemantic = new HashSet<>(); + + for (IdentifierData i : identifiersUsedInAnnotations) { + if (i.symbol().isUnknown()) { + idNamesWithoutSemantic.add(i.identifier()); + } else { + idSymbols.add(i.symbol()); + idNamesWithSemantic.add(i.identifier()); + } + } + + for(VariableData v : hardCodedUri) { + // equals to an identifier with unknown semantic, we cannot compare their symbols + if (idNamesWithoutSemantic.contains(v.identifier())) { + continue; + } + + // idNamesWithSemantic is used to only compare the symbols when their string identifier are the same + // as comparing symbols is costly + if (idNamesWithSemantic.contains(v.identifier()) && idSymbols.contains(v.symbol())) { + continue; + } + reportHardcodedURI(v.initializer()); + } + } + @Override public List nodesToVisit() { - return Arrays.asList(Tree.Kind.NEW_CLASS, Tree.Kind.VARIABLE, Tree.Kind.ASSIGNMENT); + return Arrays.asList(Tree.Kind.NEW_CLASS, Tree.Kind.VARIABLE, Tree.Kind.ASSIGNMENT, Tree.Kind.ANNOTATION, Tree.Kind.IDENTIFIER); } @Override public void visitNode(Tree tree) { - if (tree.is(Tree.Kind.NEW_CLASS)) { - checkNewClassTree((NewClassTree) tree); - } else if (tree.is(Tree.Kind.VARIABLE)) { - checkVariable((VariableTree) tree); - } else { - checkAssignment((AssignmentExpressionTree) tree); + if (tree instanceof NewClassTree classTree) { + checkNewClassTree(classTree); + } else if (tree instanceof VariableTree variableTree) { + checkVariable(variableTree); + } else if (tree instanceof AnnotationTree annotationTree) { + annotationsStack.add(annotationTree); + } else if (tree instanceof IdentifierTree identifier && !annotationsStack.isEmpty()) { + identifiersUsedInAnnotations.add(new IdentifierData(identifier.symbol(), identifier.name())); + } else if (tree instanceof AssignmentExpressionTree assignment) { + checkAssignment(assignment); + } + } + + @Override + public void leaveNode(Tree tree) { + if (tree instanceof AnnotationTree) { + annotationsStack.pop(); } } @@ -91,8 +167,31 @@ private void checkNewClassTree(NewClassTree nct) { } private void checkVariable(VariableTree tree) { - if (isFileNameVariable(tree.simpleName())) { - checkExpression(tree.initializer()); + ExpressionTree initializer = tree.initializer(); + + if (!isFileNameVariable(tree.simpleName()) + || initializer == null + // we don't raise issues when the variable is annotated + || !tree.modifiers().annotations().isEmpty() + ) { + return; + } + + String stringLiteral = stringLiteral(initializer); + if (stringLiteral == null) { + return; + } + + // small relative Uri that are static and final are allowed + if (ModifiersUtils.hasAll(tree.modifiers(), Modifier.STATIC, Modifier.FINAL) + && RELATIVE_URI_PATTERN.matcher(stringLiteral).matches()) { + return; + } + + if (isHardcodedURI(initializer)) { + hardCodedUri.add(new VariableData(tree.symbol(), + tree.simpleName().name(), + initializer)); } } @@ -117,26 +216,30 @@ private static boolean isFileNameVariable(@Nullable IdentifierTree variable) { return variable != null && VARIABLE_NAME_PATTERN.matcher(variable.name()).find(); } - private void checkExpression(@Nullable ExpressionTree expr) { - if (expr != null) { - if (isHardcodedURI(expr)) { - reportHardcodedURI(expr); - } else { - reportStringConcatenationWithPathDelimiter(expr); - } + private void checkExpression(ExpressionTree expr) { + if (isHardcodedURI(expr)) { + reportHardcodedURI(expr); + } else { + reportStringConcatenationWithPathDelimiter(expr); } } private static boolean isHardcodedURI(ExpressionTree expr) { - ExpressionTree newExpr = ExpressionUtils.skipParentheses(expr); - if (!newExpr.is(Tree.Kind.STRING_LITERAL)) { - return false; - } - String stringLiteral = LiteralUtils.trimQuotes(((LiteralTree) newExpr).value()); - if(stringLiteral.contains("*") || stringLiteral.contains("$")) { - return false; + String stringLiteral = stringLiteral(expr); + return stringLiteral != null + && !stringLiteral.contains("*") + && !stringLiteral.contains("$") + && URI_PATTERN.matcher(stringLiteral).find(); + } + + @Nullable + private static String stringLiteral(ExpressionTree expr) { + ExpressionTree unquoted = ExpressionUtils.skipParentheses(expr); + + if (unquoted instanceof LiteralTree literalTree && literalTree.is(Tree.Kind.STRING_LITERAL)) { + return LiteralUtils.trimQuotes(literalTree.value()); } - return URI_PATTERN.matcher(stringLiteral).find(); + return null; } private void reportHardcodedURI(ExpressionTree hardcodedURI) { diff --git a/java-checks/src/test/java/org/sonar/java/checks/HardcodedURICheckTest.java b/java-checks/src/test/java/org/sonar/java/checks/HardcodedURICheckTest.java index 422a50c5558..d8e1f8457fa 100644 --- a/java-checks/src/test/java/org/sonar/java/checks/HardcodedURICheckTest.java +++ b/java-checks/src/test/java/org/sonar/java/checks/HardcodedURICheckTest.java @@ -20,6 +20,7 @@ import org.sonar.java.checks.verifier.CheckVerifier; import static org.sonar.java.checks.verifier.TestUtils.mainCodeSourcesPath; +import static org.sonar.java.checks.verifier.TestUtils.nonCompilingTestSourcesPath; class HardcodedURICheckTest { @Test @@ -30,4 +31,12 @@ void test() { .verifyIssues(); } + @Test + void test_without_semantic() { + CheckVerifier.newVerifier() + .onFile(nonCompilingTestSourcesPath("checks/HardcodedURICheckSample.java")) + .withCheck(new HardcodedURICheck()) + .verifyIssues(); + } + }