From bae4dbf8ca0c27ec5d2da9577367143678479189 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 (configurable, default 500) to prevent thread exhaustion. 5. Denial of Service (OOM): ConsoleHistoryDataAction now uses .setMaxResults() (configurable, default 500) on JPA native queries to prevent eager loading of the entire database into memory. Makes the history query limit and bulk domain action limit configurable via RegistryConfig, allowing smaller limits to be used in tests to avoid heavy resource persistence. Also removes an outdated Joda-Time migration reference from GEMINI.md. --- GEMINI.md | 2 +- .../src/app/domains/domainList.component.ts | 18 ++++++--- .../registrar/registrarDetails.component.html | 7 ++-- .../registrar/registrarsTable.component.html | 7 ++-- .../registrar/registrarsTable.component.ts | 2 +- .../settings/contact/contact.component.html | 13 ++++++- .../app/settings/contact/contact.component.ts | 9 +---- .../src/app/users/usersList.component.html | 2 +- .../registry/config/RegistryConfig.java | 12 ++++++ .../config/RegistryConfigSettings.java | 2 + .../registry/config/files/default-config.yaml | 6 +++ .../console/ConsoleEppPasswordAction.java | 3 ++ .../console/ConsoleHistoryDataAction.java | 4 ++ .../console/PasswordResetRequestAction.java | 26 +++++++++---- .../console/PasswordResetVerifyAction.java | 4 +- .../domains/ConsoleBulkDomainAction.java | 12 ++++++ .../console/ConsoleHistoryDataActionTest.java | 30 ++++++++++++++- .../domains/ConsoleBulkDomainActionTest.java | 37 +++++++++++++++++-- 18 files changed, 158 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..21327f1dff2 100644 --- a/console-webapp/src/app/domains/domainList.component.ts +++ b/console-webapp/src/app/domains/domainList.component.ts @@ -48,7 +48,11 @@ interface DomainData { selector: 'app-response-dialog', template: `

{{ data.title }}

- + + @for (line of data.content; track line) { +
{{ line }}
+ } + @@ -59,7 +63,7 @@ export class ResponseDialogComponent { constructor( public dialogRef: MatDialogRef, @Inject(MAT_DIALOG_DATA) - public data: { title: string; content: string } + public data: { title: string; content: string[] } ) {} onClose(): void { @@ -312,11 +316,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..5ec5f42da0e 100644 --- a/console-webapp/src/app/registrar/registrarsTable.component.html +++ b/console-webapp/src/app/registrar/registrarsTable.component.html @@ -49,10 +49,9 @@

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..69f892a3a58 100644 --- a/console-webapp/src/app/registrar/registrarsTable.component.ts +++ b/console-webapp/src/app/registrar/registrarsTable.component.ts @@ -56,7 +56,7 @@ export const columns = [ cell: (record: Registrar) => `${Object.entries(record.billingAccountMap || {}).reduce( (acc, [key, val]) => { - return `${acc}${key}=${val}
`; + return `${acc}${key}=${val}\n`; }, '' )}`, 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/config/RegistryConfig.java b/core/src/main/java/google/registry/config/RegistryConfig.java index 7c7b9448d77..cee88303343 100644 --- a/core/src/main/java/google/registry/config/RegistryConfig.java +++ b/core/src/main/java/google/registry/config/RegistryConfig.java @@ -173,6 +173,18 @@ public static String provideSupportEmail(RegistryConfigSettings config) { return config.registrarConsole.supportEmailAddress; } + @Provides + @Config("consoleHistoryQueryLimit") + public static int provideConsoleHistoryQueryLimit(RegistryConfigSettings config) { + return config.registrarConsole.historyQueryLimit; + } + + @Provides + @Config("consoleBulkDomainActionLimit") + public static int provideConsoleBulkDomainActionLimit(RegistryConfigSettings config) { + return config.registrarConsole.bulkDomainActionLimit; + } + /** * The DUM file name, used as a file name base for DUM csv file * diff --git a/core/src/main/java/google/registry/config/RegistryConfigSettings.java b/core/src/main/java/google/registry/config/RegistryConfigSettings.java index 1f2331b3769..5b8ddedadcd 100644 --- a/core/src/main/java/google/registry/config/RegistryConfigSettings.java +++ b/core/src/main/java/google/registry/config/RegistryConfigSettings.java @@ -181,6 +181,8 @@ public static class RegistrarConsole { public String supportPhoneNumber; public String supportEmailAddress; public String technicalDocsUrl; + public int historyQueryLimit; + public int bulkDomainActionLimit; } /** Configuration for monitoring. */ diff --git a/core/src/main/java/google/registry/config/files/default-config.yaml b/core/src/main/java/google/registry/config/files/default-config.yaml index 22f751de10c..9b8719d7511 100644 --- a/core/src/main/java/google/registry/config/files/default-config.yaml +++ b/core/src/main/java/google/registry/config/files/default-config.yaml @@ -380,6 +380,12 @@ registrarConsole: # URL linking to directory of technical support docs on the registry. technicalDocsUrl: http://example.com/your_support_docs/ + # Maximum number of history records returned in a single query. + historyQueryLimit: 500 + + # Maximum number of domains allowed in a single bulk action. + bulkDomainActionLimit: 500 + monitoring: # Max queries per second for the Google Cloud Monitoring V3 (aka Stackdriver) # API. The limit can be adjusted by contacting Cloud Support. 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..138e1a1f4b8 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 @@ -62,17 +62,20 @@ public class ConsoleHistoryDataAction extends ConsoleApiAction { private final Optional consoleUserEmail; private final String supportEmail; + private final int historyQueryLimit; @Inject public ConsoleHistoryDataAction( ConsoleApiParams consoleApiParams, @Config("supportEmail") String supportEmail, + @Config("consoleHistoryQueryLimit") int historyQueryLimit, @Parameter("registrarId") String registrarId, @Parameter("consoleUserEmail") Optional consoleUserEmail) { super(consoleApiParams); this.registrarId = registrarId; this.consoleUserEmail = consoleUserEmail; this.supportEmail = supportEmail; + this.historyQueryLimit = historyQueryLimit; } @Override @@ -117,6 +120,7 @@ private void historyByRegistrarId(User user, String registrarId) { .createNativeQuery(SQL_REGISTRAR_HISTORY, ConsoleUpdateHistory.class) .setParameter("registrarId", registrarId) .setHint("org.hibernate.fetchSize", 1000) + .setMaxResults(historyQueryLimit) .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..7de63e1999c 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; @@ -22,6 +23,7 @@ import com.google.common.collect.ImmutableMap; import com.google.gson.JsonElement; import com.google.gson.annotations.Expose; +import google.registry.config.RegistryConfig.Config; import google.registry.flows.EppController; import google.registry.flows.EppRequestSource; import google.registry.flows.PasswordOnlyTransportCredentials; @@ -64,11 +66,13 @@ public record BulkDomainList(@Expose List domainList) {} private final String registrarId; private final String bulkDomainAction; private final Optional optionalJsonPayload; + private final int bulkDomainActionLimit; @Inject public ConsoleBulkDomainAction( ConsoleApiParams consoleApiParams, EppController eppController, + @Config("consoleBulkDomainActionLimit") int bulkDomainActionLimit, @Parameter("registrarId") String registrarId, @Parameter("bulkDomainAction") String bulkDomainAction, @OptionalJsonPayload Optional optionalJsonPayload) { @@ -77,6 +81,7 @@ public ConsoleBulkDomainAction( this.registrarId = registrarId; this.bulkDomainAction = bulkDomainAction; this.optionalJsonPayload = optionalJsonPayload; + this.bulkDomainActionLimit = bulkDomainActionLimit; } @Override @@ -85,6 +90,13 @@ 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() <= bulkDomainActionLimit, + "Cannot process more than %s domains in a single bulk action", + bulkDomainActionLimit); 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..137d315724d 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_limitsResults() { + for (int i = 0; i < 10; 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(), 5); + action.run(); + assertThat(response.getStatus()).isEqualTo(SC_OK); + JsonArray payload = JsonParser.parseString(response.getPayload()).getAsJsonArray(); + assertThat(payload.size()).isEqualTo(5); + } + @Test void testFailure_getByRegistrar_noPermission() { ConsoleHistoryDataAction action = @@ -192,10 +215,15 @@ void testFailure_emptyRegistrarId() { private ConsoleHistoryDataAction createAction( AuthResult authResult, String registrarId, Optional consoleUserEmail) { + return createAction(authResult, registrarId, consoleUserEmail, 500); + } + + private ConsoleHistoryDataAction createAction( + AuthResult authResult, String registrarId, Optional consoleUserEmail, int limit) { consoleApiParams = ConsoleApiParamsUtils.createFake(authResult); when(consoleApiParams.request().getMethod()).thenReturn("GET"); response = (FakeResponse) consoleApiParams.response(); return new ConsoleHistoryDataAction( - consoleApiParams, SUPPORT_EMAIL, registrarId, consoleUserEmail); + consoleApiParams, SUPPORT_EMAIL, limit, registrarId, consoleUserEmail); } } 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..ed83fd74178 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(6, "domain.tld"), "reason", "reason")); + ConsoleBulkDomainAction action = createAction("DELETE", payload, fteUser, 5); + action.run(); + assertThat(response.getStatus()).isEqualTo(SC_BAD_REQUEST); + assertThat(response.getPayload()) + .isEqualTo("Cannot process more than 5 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 = @@ -232,15 +258,20 @@ void testFailure_noPermission() { // } private ConsoleBulkDomainAction createAction(String action, JsonElement payload) { - return createAction(action, payload, fteUser); + return createAction(action, payload, fteUser, 500); } private ConsoleBulkDomainAction createAction(String action, JsonElement payload, User user) { + return createAction(action, payload, user, 500); + } + + private ConsoleBulkDomainAction createAction( + String action, JsonElement payload, User user, int limit) { AuthResult authResult = AuthResult.createUser(user); ConsoleApiParams params = ConsoleApiParamsUtils.createFake(authResult); when(params.request().getMethod()).thenReturn("POST"); response = (FakeResponse) params.response(); return new ConsoleBulkDomainAction( - params, eppController, "TheRegistrar", action, Optional.ofNullable(payload)); + params, eppController, limit, "TheRegistrar", action, Optional.ofNullable(payload)); } }