From e9805ecf7d77608eb61f574fa2a020bfa812007d Mon Sep 17 00:00:00 2001
From: Weimin Yu <weiminyu@google.com>
Date: Wed, 10 Nov 2021 16:28:32 -0500
Subject: [PATCH] Remove the ineffective SQL injection check (#1412)

* Remove the ineffective SQL injection check

Remove the ineffective SQL-injection attack check in go/r3pr/954. It is
quite restrictive, causing a long exempt list. It also doesn't protect
queries made through helpers such as QueryComposer etc.

We will start from scratch for a new solution.
---
 config/presubmits.py                          | 45 -------------------
 .../JpaTransactionManagerImpl.java            |  1 -
 2 files changed, 46 deletions(-)

diff --git a/config/presubmits.py b/config/presubmits.py
index 4928fae30..2b6c9737d 100644
--- a/config/presubmits.py
+++ b/config/presubmits.py
@@ -187,51 +187,6 @@ PRESUBMITS = {
         {"/node_modules/", "google/registry/ui/js/util.js", "registrar_bin."},
     ):
         "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'.*\.(query|createQuery|createNativeQuery)\('
-        r'(?!(\s*([A-Z_]+|"([^"]|\\")*"(\s*\+\s*"([^"]|\\")*")*)'
-        r'(,|\s*\))))',
-        "java",
-        # ActivityReportingQueryBuilder deals with Dremel queries
-        {"src/test", "ActivityReportingQueryBuilder.java",
-         # This class contains helper method to make queries in Beam.
-         "RegistryJpaIO.java",
-         "CreateSyntheticHistoryEntriesAction.java",
-         # TODO(b/179158393): Remove everything below, which should be done
-         # using Criteria
-         "JpaTransactionManager.java",
-         "JpaTransactionManagerImpl.java",
-         # CriteriaQueryBuilder is a false positive
-         "CriteriaQueryBuilder.java",
-         "RdapDomainSearchAction.java",
-         "RdapNameserverSearchAction.java",
-         "ReadOnlyCheckingEntityManager.java",
-         "RegistryQuery",
-         },
-    ):
-        "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 020b84baa..602c3c41c 100644
--- a/core/src/main/java/google/registry/persistence/transaction/JpaTransactionManagerImpl.java
+++ b/core/src/main/java/google/registry/persistence/transaction/JpaTransactionManagerImpl.java
@@ -569,7 +569,6 @@ public class JpaTransactionManagerImpl implements JpaTransactionManager {
     }
     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 = query(sql);