From d326a2b60fb388674741350051aa4408a49c31bb Mon Sep 17 00:00:00 2001 From: Ben McIlwain Date: Mon, 1 Jun 2026 11:57:46 -0400 Subject: [PATCH] Fix Console API and Angular XSS security flaws This commit addresses the following security vulnerabilities identified in the recent audit of the Console App and Backend APIs: 1. Angular XSS: Removed unsafe [innerHTML] bindings across all console-webapp templates (Contact, Registrars, Registrar Details, Users List) in favor of standard Angular interpolation. 2. Broken Access Control (IDOR): PasswordResetRequestAction and PasswordResetVerifyAction now explicitly verify that the target user's email belongs to the authorized registrarId. 3. Missing Permission Check: ConsoleEppPasswordAction now explicitly checks for CONFIGURE_EPP_CONNECTION permission before updating the EPP password. 4. Denial of Service (DoS): ConsoleBulkDomainAction now strictly limits the size of bulk domain lists to 500 domains to prevent thread exhaustion. 5. Denial of Service (OOM): ConsoleHistoryDataAction now uses .setMaxResults(500) on JPA native queries to prevent eager loading of the entire database into memory. Also removes an outdated Joda-Time migration reference from GEMINI.md. --- GEMINI.md | 2 +- .../src/app/domains/domainList.component.ts | 10 ++++--- .../registrar/registrarDetails.component.html | 7 ++--- .../registrar/registrarsTable.component.html | 5 +--- .../registrar/registrarsTable.component.ts | 9 ++---- .../settings/contact/contact.component.html | 13 ++++++++- .../app/settings/contact/contact.component.ts | 9 +----- .../src/app/users/usersList.component.html | 2 +- .../console/ConsoleEppPasswordAction.java | 3 ++ .../console/ConsoleHistoryDataAction.java | 1 + .../console/PasswordResetRequestAction.java | 26 ++++++++++++----- .../console/PasswordResetVerifyAction.java | 4 ++- .../domains/ConsoleBulkDomainAction.java | 7 +++++ .../console/ConsoleHistoryDataActionTest.java | 23 +++++++++++++++ .../domains/ConsoleBulkDomainActionTest.java | 28 ++++++++++++++++++- 15 files changed, 111 insertions(+), 38 deletions(-) diff --git a/GEMINI.md b/GEMINI.md index b393c07733a..ab00dcdb1a2 100644 --- a/GEMINI.md +++ b/GEMINI.md @@ -94,7 +94,7 @@ Do not wait for the user to tell you to improve the skills; it is your responsib # Gemini Engineering Guide: Nomulus Codebase -This document captures high-level architectural patterns, lessons learned from large-scale refactorings (like the Joda-Time to `java.time` migration), and specific instructions to avoid common pitfalls in this environment. +This document captures high-level architectural patterns, lessons learned from large-scale refactorings, and specific instructions to avoid common pitfalls in this environment. ## 🏛 Architecture Overview diff --git a/console-webapp/src/app/domains/domainList.component.ts b/console-webapp/src/app/domains/domainList.component.ts index 0cc3e8ccf96..19bc5d7cbe2 100644 --- a/console-webapp/src/app/domains/domainList.component.ts +++ b/console-webapp/src/app/domains/domainList.component.ts @@ -312,11 +312,13 @@ export class DomainListComponent { this.dialog.open(ResponseDialogComponent, { data: { title: 'Domain Deletion Results', - content: `Successfully deleted - ${successCount} domain(s)
Failed to delete - ${failureCount} domain(s)
${ + content: [ + `Successfully deleted - ${successCount} domain(s)`, + `Failed to delete - ${failureCount} domain(s)`, failureCount - ? 'Some domains could not be deleted due to ongoing processes or server errors. ' - : '' - }Please check the table for more information.`, + ? 'Some domains could not be deleted due to ongoing processes or server errors. Please check the table for more information.' + : 'Please check the table for more information.', + ], }, }); this.selection.clear(); diff --git a/console-webapp/src/app/registrar/registrarDetails.component.html b/console-webapp/src/app/registrar/registrarDetails.component.html index 1eda62f338f..38937c15e0c 100644 --- a/console-webapp/src/app/registrar/registrarDetails.component.html +++ b/console-webapp/src/app/registrar/registrarDetails.component.html @@ -97,10 +97,9 @@

Registrar details

@for (column of columns; track column.columnDef) { {{ column.header }} - + {{ + column.cell(registrarInEdit) + }} } diff --git a/console-webapp/src/app/registrar/registrarsTable.component.html b/console-webapp/src/app/registrar/registrarsTable.component.html index d2878b7f283..bd8cf598d64 100644 --- a/console-webapp/src/app/registrar/registrarsTable.component.html +++ b/console-webapp/src/app/registrar/registrarsTable.component.html @@ -49,10 +49,7 @@

Registrars

{{ column.header }} - + {{ column.cell(row) }} } diff --git a/console-webapp/src/app/registrar/registrarsTable.component.ts b/console-webapp/src/app/registrar/registrarsTable.component.ts index 702a61bcd2d..25149167f8a 100644 --- a/console-webapp/src/app/registrar/registrarsTable.component.ts +++ b/console-webapp/src/app/registrar/registrarsTable.component.ts @@ -54,12 +54,9 @@ export const columns = [ columnDef: 'billingAccountMap', header: 'Billing Accounts', cell: (record: Registrar) => - `${Object.entries(record.billingAccountMap || {}).reduce( - (acc, [key, val]) => { - return `${acc}${key}=${val}
`; - }, - '' - )}`, + `${Object.entries(record.billingAccountMap || {}) + .map(([key, val]) => `${key}=${val}`) + .join(', ')}`, }, { columnDef: 'registryLockAllowed', diff --git a/console-webapp/src/app/settings/contact/contact.component.html b/console-webapp/src/app/settings/contact/contact.component.html index 362c8396c6b..4b3528ab608 100644 --- a/console-webapp/src/app/settings/contact/contact.component.html +++ b/console-webapp/src/app/settings/contact/contact.component.html @@ -25,7 +25,18 @@

No contacts found

@for (column of columns; track column) { {{ column.header }} - + + @if (column.columnDef === 'name') { +
+
{{ row.name }}
+
+ {{ row.userFriendlyTypes.join(" • ") }} +
+
+ } @else { + {{ column.cell(row) }} + } + } diff --git a/console-webapp/src/app/settings/contact/contact.component.ts b/console-webapp/src/app/settings/contact/contact.component.ts index cc23a7c3b89..c9dd0c2f3d9 100644 --- a/console-webapp/src/app/settings/contact/contact.component.ts +++ b/console-webapp/src/app/settings/contact/contact.component.ts @@ -34,14 +34,7 @@ export default class ContactComponent { { columnDef: 'name', header: 'Name', - cell: (contact: ViewReadyContact) => ` -
-
${contact.name}
-
${contact.userFriendlyTypes.join( - ' • ' - )}
-
- `, + cell: (contact: ViewReadyContact) => `${contact.name}`, }, { columnDef: 'emailAddress', diff --git a/console-webapp/src/app/users/usersList.component.html b/console-webapp/src/app/users/usersList.component.html index eb88a02b3b7..8cc7b2a0c61 100644 --- a/console-webapp/src/app/users/usersList.component.html +++ b/console-webapp/src/app/users/usersList.component.html @@ -10,7 +10,7 @@ {{ column.header }} - + {{ column.cell(row) }} } diff --git a/core/src/main/java/google/registry/ui/server/console/ConsoleEppPasswordAction.java b/core/src/main/java/google/registry/ui/server/console/ConsoleEppPasswordAction.java index a9c8943315f..a7384e361c7 100644 --- a/core/src/main/java/google/registry/ui/server/console/ConsoleEppPasswordAction.java +++ b/core/src/main/java/google/registry/ui/server/console/ConsoleEppPasswordAction.java @@ -27,6 +27,7 @@ import com.google.gson.annotations.Expose; import google.registry.flows.EppException.AuthenticationErrorException; import google.registry.flows.PasswordOnlyTransportCredentials; +import google.registry.model.console.ConsolePermission; import google.registry.model.console.ConsoleUpdateHistory; import google.registry.model.console.User; import google.registry.model.registrar.Registrar; @@ -84,6 +85,8 @@ protected void postHandler(User user) { eppRequestBody.newPassword().equals(eppRequestBody.newPasswordRepeat()), "New password fields don't match"); + checkPermission(user, eppRequestBody.registrarId(), ConsolePermission.CONFIGURE_EPP_CONNECTION); + Registrar registrar; try { registrar = registrarAccessor.getRegistrar(eppRequestBody.registrarId()); diff --git a/core/src/main/java/google/registry/ui/server/console/ConsoleHistoryDataAction.java b/core/src/main/java/google/registry/ui/server/console/ConsoleHistoryDataAction.java index d967e1e3e02..255339b5f01 100644 --- a/core/src/main/java/google/registry/ui/server/console/ConsoleHistoryDataAction.java +++ b/core/src/main/java/google/registry/ui/server/console/ConsoleHistoryDataAction.java @@ -117,6 +117,7 @@ private void historyByRegistrarId(User user, String registrarId) { .createNativeQuery(SQL_REGISTRAR_HISTORY, ConsoleUpdateHistory.class) .setParameter("registrarId", registrarId) .setHint("org.hibernate.fetchSize", 1000) + .setMaxResults(500) .getResultList()); List formattedHistoryList = diff --git a/core/src/main/java/google/registry/ui/server/console/PasswordResetRequestAction.java b/core/src/main/java/google/registry/ui/server/console/PasswordResetRequestAction.java index 732e97f5a20..899b2aa69f8 100644 --- a/core/src/main/java/google/registry/ui/server/console/PasswordResetRequestAction.java +++ b/core/src/main/java/google/registry/ui/server/console/PasswordResetRequestAction.java @@ -86,7 +86,7 @@ private void performRequest(User user) { "Must provide registry lock email to reset"); requiredPermission = ConsolePermission.MANAGE_USERS; destinationEmail = passwordResetRequestData.registryLockEmail; - checkUserExistsWithRegistryLockEmail(destinationEmail); + checkUserExistsWithRegistryLockEmail(destinationEmail, registrarId); emailSubject = "Registry lock password reset request"; } default -> throw new IllegalArgumentException("Unknown type " + type); @@ -121,12 +121,24 @@ private void performRequest(User user) { .sendEmail(EmailMessage.create(emailSubject, body, destinationAddress)); } - static User checkUserExistsWithRegistryLockEmail(String destinationEmail) { - return tm().createQueryComposer(User.class) - .where("registryLockEmailAddress", QueryComposer.Comparator.EQ, destinationEmail) - .first() - .orElseThrow( - () -> new IllegalArgumentException("Unknown user with lock email " + destinationEmail)); + static User checkUserExistsWithRegistryLockEmail(String destinationEmail, String registrarId) { + User targetUser = + tm().createQueryComposer(User.class) + .where("registryLockEmailAddress", QueryComposer.Comparator.EQ, destinationEmail) + .first() + .orElseThrow( + () -> + new IllegalArgumentException( + "Unknown user with lock email " + destinationEmail)); + + // Prevent IDOR: Ensure the resolved user actually belongs to the registrar the requester + // has permissions for, or is a global admin. + if (!targetUser.getUserRoles().isAdmin() + && !targetUser.getUserRoles().getRegistrarRoles().containsKey(registrarId)) { + throw new IllegalArgumentException( + "User with lock email " + destinationEmail + " is not associated with " + registrarId); + } + return targetUser; } private String getAdminPocEmail(String registrarId) { diff --git a/core/src/main/java/google/registry/ui/server/console/PasswordResetVerifyAction.java b/core/src/main/java/google/registry/ui/server/console/PasswordResetVerifyAction.java index 796c4512fad..52c2cc889a2 100644 --- a/core/src/main/java/google/registry/ui/server/console/PasswordResetVerifyAction.java +++ b/core/src/main/java/google/registry/ui/server/console/PasswordResetVerifyAction.java @@ -98,7 +98,9 @@ private void handleEppPasswordReset(PasswordResetRequest request) { } private void handleRegistryLockPasswordReset(PasswordResetRequest request) { - User affectedUser = checkUserExistsWithRegistryLockEmail(request.getDestinationEmail()); + User affectedUser = + checkUserExistsWithRegistryLockEmail( + request.getDestinationEmail(), request.getRegistrarId()); tm().put( affectedUser .asBuilder() diff --git a/core/src/main/java/google/registry/ui/server/console/domains/ConsoleBulkDomainAction.java b/core/src/main/java/google/registry/ui/server/console/domains/ConsoleBulkDomainAction.java index b4ed47db12e..c4db9eb9d64 100644 --- a/core/src/main/java/google/registry/ui/server/console/domains/ConsoleBulkDomainAction.java +++ b/core/src/main/java/google/registry/ui/server/console/domains/ConsoleBulkDomainAction.java @@ -14,6 +14,7 @@ package google.registry.ui.server.console.domains; +import static com.google.common.base.Preconditions.checkArgument; import static com.google.common.collect.ImmutableMap.toImmutableMap; import static google.registry.persistence.transaction.TransactionManagerFactory.tm; import static jakarta.servlet.http.HttpServletResponse.SC_OK; @@ -85,6 +86,12 @@ protected void postHandler(User user) { optionalJsonPayload.orElseThrow( () -> new IllegalArgumentException("Bulk action payload must be present")); BulkDomainList domainList = consoleApiParams.gson().fromJson(jsonPayload, BulkDomainList.class); + checkArgument( + domainList.domainList != null && !domainList.domainList.isEmpty(), + "Domain list cannot be empty"); + checkArgument( + domainList.domainList.size() <= 500, + "Cannot process more than 500 domains in a single bulk action"); ConsoleDomainActionType actionType = ConsoleDomainActionType.parseActionType(bulkDomainAction, jsonPayload); diff --git a/core/src/test/java/google/registry/ui/server/console/ConsoleHistoryDataActionTest.java b/core/src/test/java/google/registry/ui/server/console/ConsoleHistoryDataActionTest.java index d7fee4abce5..c932f1b815b 100644 --- a/core/src/test/java/google/registry/ui/server/console/ConsoleHistoryDataActionTest.java +++ b/core/src/test/java/google/registry/ui/server/console/ConsoleHistoryDataActionTest.java @@ -23,6 +23,8 @@ import com.google.common.collect.ImmutableMap; import com.google.gson.Gson; +import com.google.gson.JsonArray; +import com.google.gson.JsonParser; import google.registry.model.console.ConsoleUpdateHistory; import google.registry.model.console.RegistrarRole; import google.registry.model.console.User; @@ -162,6 +164,27 @@ void testSuccess_noResults() { assertThat(response.getPayload()).isEqualTo("[]"); } + @Test + void testSuccess_limitsResultsTo500() { + for (int i = 0; i < 505; i++) { + DatabaseHelper.persistResource( + new ConsoleUpdateHistory.Builder() + .setType(ConsoleUpdateHistory.Type.REGISTRAR_UPDATE) + .setDescription("TheRegistrar|some detail " + i) + .setActingUser(fteUser) + .setUrl("https://test.com") + .setMethod("GET") + .setModificationTime(clock.now()) + .build()); + } + ConsoleHistoryDataAction action = + createAction(AuthResult.createUser(fteUser), "TheRegistrar", Optional.empty()); + action.run(); + assertThat(response.getStatus()).isEqualTo(SC_OK); + JsonArray payload = JsonParser.parseString(response.getPayload()).getAsJsonArray(); + assertThat(payload.size()).isEqualTo(500); + } + @Test void testFailure_getByRegistrar_noPermission() { ConsoleHistoryDataAction action = diff --git a/core/src/test/java/google/registry/ui/server/console/domains/ConsoleBulkDomainActionTest.java b/core/src/test/java/google/registry/ui/server/console/domains/ConsoleBulkDomainActionTest.java index 84afa9956da..fc3701c8949 100644 --- a/core/src/test/java/google/registry/ui/server/console/domains/ConsoleBulkDomainActionTest.java +++ b/core/src/test/java/google/registry/ui/server/console/domains/ConsoleBulkDomainActionTest.java @@ -45,6 +45,7 @@ import google.registry.testing.FakeResponse; import google.registry.ui.server.console.ConsoleActionBaseTestCase; import google.registry.ui.server.console.ConsoleApiParams; +import java.util.Collections; import java.util.Optional; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; @@ -173,7 +174,9 @@ void testHalfSuccess_halfNonexistent() throws Exception { @Test void testFailure_badActionString() { - ConsoleBulkDomainAction action = createAction("bad", GSON.toJsonTree(ImmutableMap.of())); + ConsoleBulkDomainAction action = + createAction( + "bad", GSON.toJsonTree(ImmutableMap.of("domainList", ImmutableList.of("domain.tld")))); action.run(); assertThat(response.getStatus()).isEqualTo(SC_BAD_REQUEST); assertThat(response.getPayload()) @@ -190,6 +193,29 @@ void testFailure_emptyBody() { assertThat(response.getPayload()).isEqualTo("Bulk action payload must be present"); } + @Test + void testFailure_listTooLarge() { + JsonElement payload = + GSON.toJsonTree( + ImmutableMap.of( + "domainList", Collections.nCopies(501, "domain.tld"), "reason", "reason")); + ConsoleBulkDomainAction action = createAction("DELETE", payload); + action.run(); + assertThat(response.getStatus()).isEqualTo(SC_BAD_REQUEST); + assertThat(response.getPayload()) + .isEqualTo("Cannot process more than 500 domains in a single bulk action"); + } + + @Test + void testFailure_emptyList() { + JsonElement payload = + GSON.toJsonTree(ImmutableMap.of("domainList", ImmutableList.of(), "reason", "reason")); + ConsoleBulkDomainAction action = createAction("DELETE", payload); + action.run(); + assertThat(response.getStatus()).isEqualTo(SC_BAD_REQUEST); + assertThat(response.getPayload()).isEqualTo("Domain list cannot be empty"); + } + @Test void testFailure_noPermission() { JsonElement payload =