From d7e65d95e66cf074df67bc586782e878967f8565 Mon Sep 17 00:00:00 2001
From: Weimin Yu <weiminyu@google.com>
Date: Sat, 6 Feb 2021 19:28:38 -0500
Subject: [PATCH] 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                          | 39 ++++++++++++++++++-
 .../JpaTransactionManagerImpl.java            |  1 +
 2 files changed, 39 insertions(+), 1 deletion(-)

diff --git a/config/presubmits.py b/config/presubmits.py
index fa3c9e675..1ab395009 100644
--- a/config/presubmits.py
+++ b/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
diff --git a/core/src/main/java/google/registry/persistence/transaction/JpaTransactionManagerImpl.java b/core/src/main/java/google/registry/persistence/transaction/JpaTransactionManagerImpl.java
index 0da9c11d0..ff219f502 100644
--- a/core/src/main/java/google/registry/persistence/transaction/JpaTransactionManagerImpl.java
+++ b/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);