Add a presubmit check to require use of templated SQL string literals (#954)

* Add a presubmit check to require use of templated SQL string literals

This PR proposes a coding style convention that helps prevent
SQL-injection attacks, and is easy to enforce in the presubmit check.

SQL-injections can be effectively prevented if all parameterized queries
are generated using the proper param-binding methods. In our project
which uses Hibernate exclusively, this can be achieved if we all follow
a simple convention: only use constant sql templates assigned to static
final String variables as the first parameter to creat(Native)Query
methods.

This PR adds a presubmit check to enforce the proposed rule, and
modified one class as a demo. If the team agrees with this proposal, we
will change all other use cases.

config/presubmits.py

@@ -176,7 +176,44 @@ PRESUBMITS = {
         "js",
         {"/node_modules/", "google/registry/ui/js/util.js", "registrar_bin."},
     ):
-        "JavaScript files should not include console logging."
+        "JavaScript files should not include console logging.",
+    # SQL injection protection rule for java source file:
+    # The sql template passed to createQuery/createNativeQuery methods must be
+    # a variable name in UPPER_CASE_UNDERSCORE format, i.e., a static final
+    # String variable. This forces the use of parameter-binding on all queries
+    # that take parameters.
+    # The rule would forbid invocation of createQuery(Criteria). However, this
+    # can be handled by adding a helper method in an exempted class to make
+    # the calls.
+    # TODO(b/179158393): enable the 'ConstantName' Java style check to ensure
+    # that non-final variables do not use the UPPER_CASE_UNDERSCORE format.
+    PresubmitCheck(
+        # Line 1: the method names we check and the opening parenthesis, which
+        #    marks the beginning of the first parameter
+        # Line 2: The first parameter is a match if is NOT any of the following:
+        #    - final variable name: \s*([A-Z_]+
+        #    - string literal: "([^"]|\\")*"
+        #    - concatenation of literals: (\s*\+\s*"([^"]|\\")*")*
+        # Line 3: , or the closing parenthesis, marking the end of the first
+        #    parameter
+        r'.*\.create(Native)?Query\('
+        r'(?!(\s*([A-Z_]+|"([^"]|\\")*"(\s*\+\s*"([^"]|\\")*")*)'
+        r'(,|\s*\))))',
+        "java",
+        # ActivityReportingQueryBuilder deals with Dremel queries
+        {"src/test", "ActivityReportingQueryBuilder.java",
+         # TODO(b/179158393): Remove everything below, which should be done
+         # using Criteria
+         "ForeignKeyIndex.java",
+         "HistoryEntryDao.java",
+         "JpaTransactionManagerImpl.java",
+         },
+    ):
+        "The first String parameter to EntityManager.create(Native)Query "
+        "methods must be one of the following:\n"
+        "  - A String literal\n"
+        "  - Concatenation of String literals only\n"
+        "  - The name of a static final String variable"
 }
 
 # Note that this regex only works for one kind of Flyway file.  If we want to

core/src/main/java/google/registry/persistence/transaction/JpaTransactionManagerImpl.java

@@ -458,6 +458,7 @@ public class JpaTransactionManagerImpl implements JpaTransactionManager {
     assertInTransaction();
     EntityType<?> entityType = getEntityType(key.getKind());
     ImmutableSet<EntityId> entityIds = getEntityIdsFromSqlKey(entityType, key.getSqlKey());
+    // TODO(b/179158393): use Criteria for query to leave not doubt about sql injection risk.
     String sql =
         String.format("DELETE FROM %s WHERE %s", entityType.getName(), getAndClause(entityIds));
     Query query = getEntityManager().createQuery(sql);