Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions GEMINI.md
Original file line number Diff line number Diff line change
Expand Up @@ -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: `<!-- Reviewable:start -->...<!-- Reviewable:end -->`) at the bottom of the description.
Original file line number Diff line number Diff line change
Expand Up @@ -1030,6 +1030,11 @@ public static Iterable<Registrar> loadAll() {
return tm().transact(() -> tm().loadAllOf(Registrar.class));
}

/** Loads all registrar entities directly from the database, sorted by the given field names. */
public static Iterable<Registrar> loadAllSorted(String... sortFields) {
return tm().transact(() -> tm().loadAllOfSorted(Registrar.class, sortFields));
}

/** Loads all registrar entities using an in-memory cache. */
public static Iterable<Registrar> loadAllCached() {
return CACHE_BY_REGISTRAR_ID.get().values();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -257,11 +257,21 @@ public <T> ImmutableList<T> loadAllOf(Class<T> clazz) {
return getReplica().loadAllOf(clazz);
}

@Override
public <T> ImmutableList<T> loadAllOfSorted(Class<T> clazz, String... sortFields) {
return getReplica().loadAllOfSorted(clazz, sortFields);
}

@Override
public <T> Stream<T> loadAllOfStream(Class<T> clazz) {
return getReplica().loadAllOfStream(clazz);
}

@Override
public <T> Stream<T> loadAllOfSortedStream(Class<T> clazz, String... sortFields) {
return getReplica().loadAllOfSortedStream(clazz, sortFields);
}

@Override
public <T> Optional<T> loadSingleton(Class<T> clazz) {
return getReplica().loadSingleton(clazz);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@
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;
Expand All @@ -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.
*
* <p>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_.]+$");

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a comment explaining why this exists / what it does.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Added Javadoc explaining why regex validation is required to prevent JPQL/SQL injection in dynamic ORDER BY clauses.


private static final String NESTED_TRANSACTION_MESSAGE =
"Nested transaction detected. Try refactoring to avoid nested transactions. If unachievable,"
+ " use reTransact() in nested transactions";
Expand Down Expand Up @@ -528,15 +539,38 @@ public <T> ImmutableList<T> loadByEntities(Iterable<T> entities) {

@Override
public <T> ImmutableList<T> loadAllOf(Class<T> clazz) {
return loadAllOfStream(clazz).collect(toImmutableList());
return loadAllOfSorted(clazz);
}

@Override
public <T> Stream<T> loadAllOfStream(Class<T> clazz) {
return loadAllOfSortedStream(clazz);
}

@Override
public <T> ImmutableList<T> loadAllOfSorted(Class<T> clazz, String... sortFields) {
return loadAllOfSortedStream(clazz, sortFields).collect(toImmutableList());
}

@Override
public <T> Stream<T> loadAllOfSortedStream(Class<T> 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);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -219,13 +219,29 @@ <T> ImmutableMap<VKey<? extends T>, T> loadByKeysIfPresent(
*/
<T> ImmutableList<T> loadAllOf(Class<T> 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.
*
* <p>The resulting list is empty if there are no entities of this type.
*/
<T> ImmutableList<T> loadAllOfSorted(Class<T> clazz, String... sortFields);

/**
* Returns a stream of all entities of the given type that exist in the database.
*
* <p>The resulting stream is empty if there are no entities of this type.
*/
<T> Stream<T> loadAllOfStream(Class<T> 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.
*
* <p>The resulting stream is empty if there are no entities of this type.
*/
<T> Stream<T> loadAllOfSortedStream(Class<T> clazz, String... sortFields);

/**
* Loads the only instance of this particular class, or empty if none exists.
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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> registrar;
Expand All @@ -83,7 +84,7 @@ protected void getHandler(User user) {
ImmutableSet<Registrar.Type> allowedRegistrarTypes =
user.getUserRoles().isAdmin() ? TYPES_ALLOWED_FOR_ADMINS : TYPES_ALLOWED_FOR_USERS;
ImmutableList<Registrar> 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));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -404,6 +404,39 @@ void saveAllNew_succeeds() {
.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());
Expand Down
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading