From 1ac47ce00138fd35023cad544aadfa717efdd144 Mon Sep 17 00:00:00 2001
From: Ben McIlwain <mcilwain@google.com>
Date: Fri, 26 Jun 2026 22:34:13 -0400
Subject: [PATCH] Parameterize BSA native SQL queries for SQLi safety

Remediate BSA SQL Injection (A.2.2) by parameterizing native queries
in Queries.java. Rather than formatting operator-supplied domain list
tuples directly into a SQL query string:
- Dynamically build a parameterized SQL query utilizing Postgres named parameters.
- Bind the label and tld segments using .setParameter.
- Return early on empty domain sets to avoid empty IN clause syntax errors.
- Add test cases in QueriesTest.java to verify SQL injection safety and
  empty collection handling.

TAG=agy
CONV=610c2358-a99f-4605-94cd-ff0d4ee08176
---
 .../registry/bsa/persistence/Queries.java     | 43 ++++++++++++-------
 .../registry/bsa/persistence/QueriesTest.java | 17 ++++++++
 2 files changed, 44 insertions(+), 16 deletions(-)

diff --git a/core/src/main/java/google/registry/bsa/persistence/Queries.java b/core/src/main/java/google/registry/bsa/persistence/Queries.java
index e22c13f7115..187d0043c70 100644
--- a/core/src/main/java/google/registry/bsa/persistence/Queries.java
+++ b/core/src/main/java/google/registry/bsa/persistence/Queries.java
@@ -27,7 +27,6 @@
 import java.time.Instant;
 import java.util.List;
 import java.util.Optional;
-import java.util.stream.Collectors;
 import java.util.stream.Stream;
 
 /** Helpers for querying BSA JPA entities. */
@@ -114,21 +113,33 @@ static ImmutableList<BsaUnblockableDomain> batchReadUnblockables(
   }
 
   static ImmutableSet<String> queryUnblockablesByNames(ImmutableSet<String> domains) {
-    String labelTldParis =
-        domains.stream()
-            .map(
-                domain -> {
-                  List<String> parts = DOMAIN_SPLITTER.splitToList(domain);
-                  verify(parts.size() == 2, "Invalid domain name %s", domain);
-                  return String.format("('%s','%s')", parts.get(0), parts.get(1));
-                })
-            .collect(Collectors.joining(","));
-    String sql =
-        String.format(
-            "SELECT CONCAT(d.label, '.', d.tld) FROM \"BsaUnblockableDomain\" d "
-                + "WHERE (d.label, d.tld) IN (%s)",
-            labelTldParis);
-    return ImmutableSet.copyOf(tm().getEntityManager().createNativeQuery(sql).getResultList());
+    if (domains.isEmpty()) {
+      return ImmutableSet.of();
+    }
+    ImmutableList<String> domainList = ImmutableList.copyOf(domains);
+    StringBuilder sqlBuilder =
+        new StringBuilder(
+            "SELECT CONCAT(d.label, '.', d.tld) FROM \"BsaUnblockableDomain\" d WHERE (d.label,"
+                + " d.tld) IN (");
+    for (int i = 0; i < domainList.size(); i++) {
+      if (i > 0) {
+        sqlBuilder.append(",");
+      }
+      sqlBuilder.append("(:label").append(i).append(", :tld").append(i).append(")");
+    }
+    sqlBuilder.append(")");
+
+    var query = tm().getEntityManager().createNativeQuery(sqlBuilder.toString());
+    for (int i = 0; i < domainList.size(); i++) {
+      List<String> parts = DOMAIN_SPLITTER.splitToList(domainList.get(i));
+      verify(parts.size() == 2, "Invalid domain name %s", domainList.get(i));
+      query.setParameter("label" + i, parts.get(0));
+      query.setParameter("tld" + i, parts.get(1));
+    }
+
+    @SuppressWarnings("unchecked")
+    List<String> resultList = (List<String>) query.getResultList();
+    return ImmutableSet.copyOf(resultList);
   }
 
   static ImmutableSet<String> queryNewlyCreatedDomains(
diff --git a/core/src/test/java/google/registry/bsa/persistence/QueriesTest.java b/core/src/test/java/google/registry/bsa/persistence/QueriesTest.java
index f185aeb7cde..c7b0e3c1d07 100644
--- a/core/src/test/java/google/registry/bsa/persistence/QueriesTest.java
+++ b/core/src/test/java/google/registry/bsa/persistence/QueriesTest.java
@@ -316,4 +316,21 @@ void batchReadUnblockables_multiBatch() {
                 .domainName())
         .isEqualTo("label3.app");
   }
+
+  @Test
+  void testQueryUnblockablesByNames_sqlInjectionSafe() {
+    setupUnblockableDomains();
+    // Verify standard lookup works
+    assertThat(tm().transact(() -> queryUnblockablesByNames(ImmutableSet.of("a.tld1"))))
+        .containsExactly("a.tld1");
+
+    // Attempt SQL Injection payload in the name. Should be treated as literal and return empty.
+    assertThat(tm().transact(() -> queryUnblockablesByNames(ImmutableSet.of("a' OR '1'='1.tld1"))))
+        .isEmpty();
+  }
+
+  @Test
+  void testQueryUnblockablesByNames_emptySet() {
+    assertThat(tm().transact(() -> queryUnblockablesByNames(ImmutableSet.of()))).isEmpty();
+  }
 }