diff --git a/GEMINI.md b/GEMINI.md index b46f4c986..51c0f0327 100644 --- a/GEMINI.md +++ b/GEMINI.md @@ -177,3 +177,4 @@ This protocol defines the standard for interacting with GitHub repositories and - **One Commit Per PR:** Ensure all changes are squashed into a single, clean commit. Use `git commit --amend --no-edit` for follow-up fixes. - **Clean Workspace:** Always run `git status` and verify the repository state before declaring a task complete. - **Package Lock:** The Gradle build automatically modifies `console-webapp/package-lock.json` via the `npmInstallDeps` task. ALWAYS revert this file (`git checkout console-webapp/package-lock.json`) before staging changes or finalizing a commit unless you explicitly modified NPM dependencies. +- **PR Description Synchronization:** Whenever you amend or update a commit's description, you **MUST** check whether the corresponding GitHub PR description (if one exists) matches the previous commit description. If the PR description was not manually customized (i.e., it simply reflects the older commit description), you must automatically update it via `gh pr edit` to keep it in sync with your newly updated commit description—**strictly after the updated commit has been pushed to the remote GitHub PR branch**. Do not update the remote PR description when changes are only committed locally. **CRITICAL:** When updating the PR description, you must strictly preserve any automated review links or footer blocks (such as Reviewable link blocks: `...`) at the bottom of the description. diff --git a/core/src/main/java/google/registry/model/registrar/Registrar.java b/core/src/main/java/google/registry/model/registrar/Registrar.java index 644ee6254..85fc9a83a 100644 --- a/core/src/main/java/google/registry/model/registrar/Registrar.java +++ b/core/src/main/java/google/registry/model/registrar/Registrar.java @@ -1030,6 +1030,11 @@ public class Registrar extends UpdateAutoTimestampEntity implements Buildable, J return tm().transact(() -> tm().loadAllOf(Registrar.class)); } + /** Loads all registrar entities directly from the database, sorted by the given field names. */ + public static Iterable loadAllSorted(String... sortFields) { + return tm().transact(() -> tm().loadAllOfSorted(Registrar.class, sortFields)); + } + /** Loads all registrar entities using an in-memory cache. */ public static Iterable loadAllCached() { return CACHE_BY_REGISTRAR_ID.get().values(); diff --git a/core/src/main/java/google/registry/persistence/transaction/DelegatingReplicaJpaTransactionManager.java b/core/src/main/java/google/registry/persistence/transaction/DelegatingReplicaJpaTransactionManager.java index 6d6141a97..be26a96e9 100644 --- a/core/src/main/java/google/registry/persistence/transaction/DelegatingReplicaJpaTransactionManager.java +++ b/core/src/main/java/google/registry/persistence/transaction/DelegatingReplicaJpaTransactionManager.java @@ -257,11 +257,21 @@ public class DelegatingReplicaJpaTransactionManager implements JpaTransactionMan return getReplica().loadAllOf(clazz); } + @Override + public ImmutableList loadAllOfSorted(Class clazz, String... sortFields) { + return getReplica().loadAllOfSorted(clazz, sortFields); + } + @Override public Stream loadAllOfStream(Class clazz) { return getReplica().loadAllOfStream(clazz); } + @Override + public Stream loadAllOfSortedStream(Class clazz, String... sortFields) { + return getReplica().loadAllOfSortedStream(clazz, sortFields); + } + @Override public Optional loadSingleton(Class clazz) { return getReplica().loadSingleton(clazz); 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 289933170..fe975a1cc 100644 --- a/core/src/main/java/google/registry/persistence/transaction/JpaTransactionManagerImpl.java +++ b/core/src/main/java/google/registry/persistence/transaction/JpaTransactionManagerImpl.java @@ -76,6 +76,7 @@ import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicLong; import java.util.function.Supplier; import java.util.function.UnaryOperator; +import java.util.regex.Pattern; import java.util.stream.Stream; import java.util.stream.StreamSupport; import javax.annotation.Nullable; @@ -88,6 +89,16 @@ public class JpaTransactionManagerImpl implements JpaTransactionManager { private static final FluentLogger logger = FluentLogger.forEnclosingClass(); private static final Retrier retrier = new Retrier(new SystemSleeper(), 6); + + /** + * Strict allowlist regex for property/field names in dynamic JPQL ORDER BY clauses. + * + *

JPA and database engines forbid bind parameters (e.g. ? or :param) for schema identifiers or + * property names in ORDER BY clauses. To prevent JPQL/SQL injection when dynamically constructing + * sort queries, every sort field MUST be validated against this pattern before concatenation. + */ + private static final Pattern VALID_SORT_FIELD_PATTERN = Pattern.compile("^[a-zA-Z0-9_.]+$"); + private static final String NESTED_TRANSACTION_MESSAGE = "Nested transaction detected. Try refactoring to avoid nested transactions. If unachievable," + " use reTransact() in nested transactions"; @@ -528,15 +539,38 @@ public class JpaTransactionManagerImpl implements JpaTransactionManager { @Override public ImmutableList loadAllOf(Class clazz) { - return loadAllOfStream(clazz).collect(toImmutableList()); + return loadAllOfSorted(clazz); } @Override public Stream loadAllOfStream(Class clazz) { + return loadAllOfSortedStream(clazz); + } + + @Override + public ImmutableList loadAllOfSorted(Class clazz, String... sortFields) { + return loadAllOfSortedStream(clazz, sortFields).collect(toImmutableList()); + } + + @Override + public Stream loadAllOfSortedStream(Class clazz, String... sortFields) { checkArgumentNotNull(clazz, "clazz must be specified"); + checkArgumentNotNull(sortFields, "sortFields must not be null"); assertInTransaction(); + StringBuilder queryString = + new StringBuilder(String.format("FROM %s", getEntityType(clazz).getName())); + if (sortFields.length > 0) { + for (String field : sortFields) { + checkArgument( + VALID_SORT_FIELD_PATTERN.matcher(field).matches(), + "Invalid sort field name: %s", + field); + } + queryString.append(" ORDER BY "); + queryString.append(String.join(", ", sortFields)); + } return getEntityManager() - .createQuery(String.format("FROM %s", getEntityType(clazz).getName()), clazz) + .createQuery(queryString.toString(), clazz) .getResultStream() .map(this::detach); } diff --git a/core/src/main/java/google/registry/persistence/transaction/TransactionManager.java b/core/src/main/java/google/registry/persistence/transaction/TransactionManager.java index 72a396f45..3d068736b 100644 --- a/core/src/main/java/google/registry/persistence/transaction/TransactionManager.java +++ b/core/src/main/java/google/registry/persistence/transaction/TransactionManager.java @@ -219,6 +219,14 @@ public interface TransactionManager { */ ImmutableList loadAllOf(Class clazz); + /** + * Returns a list of all entities of the given type that exist in the database, ordered by the + * specified field names in ascending order. + * + *

The resulting list is empty if there are no entities of this type. + */ + ImmutableList loadAllOfSorted(Class clazz, String... sortFields); + /** * Returns a stream of all entities of the given type that exist in the database. * @@ -226,6 +234,14 @@ public interface TransactionManager { */ Stream loadAllOfStream(Class clazz); + /** + * Returns a stream of all entities of the given type that exist in the database, ordered by the + * specified field names in ascending order. + * + *

The resulting stream is empty if there are no entities of this type. + */ + Stream loadAllOfSortedStream(Class clazz, String... sortFields); + /** * Loads the only instance of this particular class, or empty if none exists. * diff --git a/core/src/main/java/google/registry/ui/server/console/RegistrarsAction.java b/core/src/main/java/google/registry/ui/server/console/RegistrarsAction.java index a22f6fa4c..e68a95959 100644 --- a/core/src/main/java/google/registry/ui/server/console/RegistrarsAction.java +++ b/core/src/main/java/google/registry/ui/server/console/RegistrarsAction.java @@ -59,6 +59,7 @@ public class RegistrarsAction extends ConsoleApiAction { """ SELECT * FROM "Registrar" WHERE registrar_id in :registrarIds + ORDER BY registrar_name ASC, registrar_id ASC """; static final String PATH = "/console-api/registrars"; private final Optional registrar; @@ -83,7 +84,7 @@ public class RegistrarsAction extends ConsoleApiAction { ImmutableSet allowedRegistrarTypes = user.getUserRoles().isAdmin() ? TYPES_ALLOWED_FOR_ADMINS : TYPES_ALLOWED_FOR_USERS; ImmutableList registrars = - Streams.stream(Registrar.loadAll()) + Streams.stream(Registrar.loadAllSorted("registrarName", "registrarId")) .filter(r -> allowedRegistrarTypes.contains(r.getType())) .collect(ImmutableList.toImmutableList()); consoleApiParams.response().setPayload(consoleApiParams.gson().toJson(registrars)); diff --git a/core/src/test/java/google/registry/persistence/transaction/JpaTransactionManagerImplTest.java b/core/src/test/java/google/registry/persistence/transaction/JpaTransactionManagerImplTest.java index 78b6f2957..6f4012afc 100644 --- a/core/src/test/java/google/registry/persistence/transaction/JpaTransactionManagerImplTest.java +++ b/core/src/test/java/google/registry/persistence/transaction/JpaTransactionManagerImplTest.java @@ -404,6 +404,39 @@ class JpaTransactionManagerImplTest { .containsExactlyElementsIn(moreEntities); } + @Test + void loadAllOfSorted() { + TestEntity entityB = new TestEntity("b_entity", "gamma"); + TestEntity entityC = new TestEntity("c_entity", "alpha"); + TestEntity entityA = new TestEntity("a_entity", "beta"); + persistResources(ImmutableList.of(entityB, entityC, entityA)); + + assertThat(tm().transact(() -> tm().loadAllOfSorted(TestEntity.class, "name"))) + .containsExactly(entityA, entityB, entityC) + .inOrder(); + + assertThat(tm().transact(() -> tm().loadAllOfSorted(TestEntity.class, "data"))) + .containsExactly(entityC, entityA, entityB) + .inOrder(); + + assertThat(tm().transact(() -> tm().loadAllOfSorted(TestEntity.class, "data", "name"))) + .containsExactly(entityC, entityA, entityB) + .inOrder(); + } + + @Test + void loadAllOfSorted_invalidFieldName_throwsException() { + IllegalArgumentException thrown = + assertThrows( + IllegalArgumentException.class, + () -> + tm().transact( + () -> + tm().loadAllOfSorted( + TestEntity.class, "name; DROP TABLE TestEntity;"))); + assertThat(thrown).hasMessageThat().contains("Invalid sort field name"); + } + @Test void saveAllNew_rollsBackWhenFailure() { moreEntities.forEach(entity -> assertThat(tm().transact(() -> tm().exists(entity))).isFalse()); diff --git a/core/src/test/resources/google/registry/webdriver/goldens/chrome-linux/ConsoleScreenshotTest_globalRole_registrars_registrarsPage.png b/core/src/test/resources/google/registry/webdriver/goldens/chrome-linux/ConsoleScreenshotTest_globalRole_registrars_registrarsPage.png index 1c88e507e..20fe2a13f 100644 Binary files a/core/src/test/resources/google/registry/webdriver/goldens/chrome-linux/ConsoleScreenshotTest_globalRole_registrars_registrarsPage.png and b/core/src/test/resources/google/registry/webdriver/goldens/chrome-linux/ConsoleScreenshotTest_globalRole_registrars_registrarsPage.png differ