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 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);