SONARJAVA-5683 S2077 Fix FN on strings built with String.format()/formatted().

java-checks-test-sources/default/src/main/files/non-compiling/checks/SQLInjectionSample.java

@@ -0,0 +1,22 @@
+package checks;
+
+import java.sql.SQLException;
+import java.sql.Statement;
+
+class SQLInjectionSample {
+  private final Statement stmt;
+
+  public SQLInjectionSample(Statement stmt) {
+    this.stmt = stmt;
+  }
+
+  public void formatInline(String input) throws SQLException {
+    this.stmt.execute(String.format("SELECT %s", input)); // Noncompliant
+  }
+
+  public void formatLocale(Locale locale, int input, Unknown unknown) throws SQLException {
+    // Do not generate warnings on unknown types to avoid FPs.
+    String query = String.format(locale, "SELECT %s %s", input, unknown);
+    this.stmt.execute(query);
+  }
+}

java-checks-test-sources/default/src/main/java/checks/SQLInjection.java

@@ -7,6 +7,7 @@
 import java.sql.SQLException;
 import java.sql.Statement;
 import java.sql.PreparedStatement;
+import java.util.Locale;
 import org.hibernate.Session;
 import javax.persistence.EntityManager;
 
@@ -240,3 +241,88 @@ private void foo() {
     tmpl.queryForObject(user, String.class); // compliant
   }
 }
+
+class SQLFormat {
+  private final Statement stmt;
+
+  public SQLFormat(Statement stmt) {
+    this.stmt = stmt;
+  }
+
+  public void formatInline(String input) throws SQLException {
+    this.stmt.execute(String.format("SELECT %s", input)); // Noncompliant
+  }
+
+  public void formatInlineConst() throws SQLException {
+    // Allow strings built with `format` if the arguments are constants.
+    this.stmt.execute(String.format("SELECT %s", "1"));
+  }
+
+  public void formatVar(String input) throws SQLException {
+    String query = String.format("SELECT %s", input);
+    this.stmt.execute(query); // Noncompliant
+  }
+
+  public void formatVarObject(Object input) throws SQLException {
+    String query = String.format("SELECT %s", input);
+    this.stmt.execute(query); // Noncompliant
+  }
+
+  public void formatVarConst() throws SQLException {
+    String query = String.format("SELECT %s", "1");
+    this.stmt.execute(query);
+  }
+
+  public void formatLocale(Locale locale, String input) throws SQLException {
+    String query = String.format(locale, "SELECT %s", input);
+    this.stmt.execute(query); // Noncompliant
+  }
+
+  public void formatLocaleConst(Locale locale) throws SQLException {
+    String query = String.format(locale, "SELECT %s", "1");
+    this.stmt.execute(query);
+  }
+
+  public void formatPrimitiveVar(int input) throws SQLException {
+    String query = String.format("SELECT %s", input);
+    this.stmt.execute(query);
+  }
+
+  public void formatPrimitiveConst() throws SQLException {
+    String query = String.format("SELECT %s", 1);
+    this.stmt.execute(query);
+  }
+
+  public void formatPrimitiveWrapperVar(Long input) throws SQLException {
+    String query = String.format("SELECT %s", input);
+    this.stmt.execute(query);
+  }
+
+  public void formatPrimitiveWrapperConst() throws SQLException {
+    String query = String.format("SELECT %s", Long.valueOf(4));
+    this.stmt.execute(query);
+  }
+
+  public void formatted(String input) throws SQLException {
+    String query = "SELECT %s".formatted(input);
+    this.stmt.execute(query); // Noncompliant
+  }
+
+  public void formattedConst() throws SQLException {
+    String query = "SELECT %s".formatted("1");
+    this.stmt.execute(query);
+  }
+
+  public void plusAssignment(String input) throws SQLException {
+    String query = "SELECT";
+    query += String.format("WHERE col = %c", input);
+    this.stmt.execute(query); // Noncompliant
+  }
+
+  public void plusAssignmentConst() throws SQLException {
+    // FP, but probably rare and not worth complicating the code to fix it.
+    String query = "SELECT";
+    query += String.format("WHERE col = \"%c\"", "value");
+    this.stmt.execute(query); // Noncompliant
+  }
+}

java-checks/src/main/java/org/sonar/java/checks/SQLInjectionCheck.java

@@ -28,6 +28,7 @@
 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.semantic.Type;
 import org.sonar.plugins.java.api.tree.AssignmentExpressionTree;
 import org.sonar.plugins.java.api.tree.ExpressionTree;
 import org.sonar.plugins.java.api.tree.IdentifierTree;
@@ -112,6 +113,14 @@ public class SQLInjectionCheck extends IssuableSubscriptionVisitor {
       .withAnyParameters()
       .build());
 
+  private static final String JAVA_LANG_STRING = "java.lang.String";
+
+  private static final MethodMatchers FORMAT_METHODS = MethodMatchers.create()
+    .ofTypes(JAVA_LANG_STRING)
+    .names("format", "formatted")
+    .withAnyParameters()
+    .build();
+
   private static final String MAIN_MESSAGE = "Make sure using a dynamically formatted SQL query is safe here.";
 
   @Override
@@ -123,20 +132,20 @@ public List<Tree.Kind> nodesToVisit() {
   public void visitNode(Tree tree) {
     if (anyMatch(tree)) {
       Optional<ExpressionTree> sqlStringArg = arguments(tree)
-        .filter(arg -> arg.symbolType().is("java.lang.String"))
+        .filter(arg -> arg.symbolType().is(JAVA_LANG_STRING))
         .findFirst();
 
       if (sqlStringArg.isPresent()) {
         ExpressionTree sqlArg = sqlStringArg.get();
-        if (isDynamicConcatenation(sqlArg)) {
+        if (isDynamicString(sqlArg)) {
           reportIssue(sqlArg, MAIN_MESSAGE);
         } else if (sqlArg.is(Tree.Kind.IDENTIFIER)) {
           IdentifierTree identifierTree = (IdentifierTree) sqlArg;
           Symbol symbol = identifierTree.symbol();
           ExpressionTree initializerOrExpression = getInitializerOrExpression(symbol.declaration());
           List<AssignmentExpressionTree> reassignments = getReassignments(symbol.owner().declaration(), symbol.usages());
 
-          if ((initializerOrExpression != null && isDynamicConcatenation(initializerOrExpression)) ||
+          if ((initializerOrExpression != null && isDynamicString(initializerOrExpression)) ||
             reassignments.stream().anyMatch(SQLInjectionCheck::isDynamicPlusAssignment)) {
             reportIssue(sqlArg, MAIN_MESSAGE, secondaryLocations(initializerOrExpression, reassignments, identifierTree.name()), null);
           }
@@ -197,7 +206,35 @@ private static boolean isDynamicPlusAssignment(ExpressionTree arg) {
     return arg.is(Tree.Kind.PLUS_ASSIGNMENT) && !((AssignmentExpressionTree) arg).expression().asConstant().isPresent();
   }
 
+  private static boolean isDynamicString(ExpressionTree arg) {
+    return isDynamicConcatenation(arg) || isDynamicFormat(arg);
+  }
+
   private static boolean isDynamicConcatenation(ExpressionTree arg) {
     return arg.is(Tree.Kind.PLUS) && !arg.asConstant().isPresent();
   }
+
+  private static boolean isDynamicFormat(Tree tree) {
+    return tree instanceof MethodInvocationTree mit
+      && FORMAT_METHODS.matches(mit)
+      && hasDynamicStringParameters(mit);
+  }
+
+  /**
+   * Checks if parameters to format/formatted are dynamic variables susceptible to SQL injection.
+   */
+  private static boolean hasDynamicStringParameters(MethodInvocationTree mit) {
+    boolean firstArg = true;
+    for (ExpressionTree arg: mit.arguments()) {
+      Type type = arg.symbolType();
+      // `format` has a variant with Locale as the first argument - we do not need to check that parameter.
+      boolean isFirstLocaleArgument = firstArg && type.is("java.util.Locale");
+      // Primitives will not lead to SQL injection, so the code is compliant.
+      if (!isFirstLocaleArgument && !type.isUnknown() && !type.isPrimitive() && !type.isPrimitiveWrapper() && arg.asConstant().isEmpty()) {
+        return true;
+      }
+      firstArg = false;
+    }
+    return false;
+  }
 }

java-checks/src/test/java/org/sonar/java/checks/SQLInjectionCheckTest.java

@@ -21,6 +21,7 @@
 
 import static org.sonar.java.checks.verifier.TestUtils.mainCodeSourcesPath;
 import static org.sonar.java.checks.verifier.TestUtils.mainCodeSourcesPathInModule;
+import static org.sonar.java.checks.verifier.TestUtils.nonCompilingTestSourcesPath;
 import static org.sonar.java.test.classpath.TestClasspathUtils.SPRING_32_MODULE;
 
 class SQLInjectionCheckTest {
@@ -33,6 +34,14 @@ void test() {
       .verifyIssues();
   }
 
+  @Test
+  void test_non_compiling() {
+    CheckVerifier.newVerifier()
+      .onFile(nonCompilingTestSourcesPath("checks/SQLInjectionSample.java"))
+      .withCheck(new SQLInjectionCheck())
+      .verifyIssues();
+  }
+
   @Test
   void test_with_spring_3_2() {
     CheckVerifier.newVerifier()