From 71ad320d194c54cdbe91fcd76d90cffe578fbc4c Mon Sep 17 00:00:00 2001 From: mbiuki Date: Sun, 28 Jun 2026 14:28:11 -0400 Subject: [PATCH 1/4] sec: harden portlet-only gated methods in RoleAjax (#643) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit saveRolePermission had no admin check at all — only the portlet-access gate. Added getAdminUser() to enforce the same admin requirement used by addNewRole/updateRole/deleteRole/lockRole/unlockRole. saveRoleLayouts already called getAdminUser() but after two DB reads. Moved the call to immediately after the portlet check so the admin gate fires before any data access, consistent with the pattern in updateLayout and deleteLayout. updateLayout and deleteLayout were already correctly ordered — no change. Closes: dotCMS/private-issues#643 Co-Authored-By: Claude Sonnet 4.6 --- .../src/main/java/com/dotmarketing/business/ajax/RoleAjax.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/dotCMS/src/main/java/com/dotmarketing/business/ajax/RoleAjax.java b/dotCMS/src/main/java/com/dotmarketing/business/ajax/RoleAjax.java index 2229f4f41822..1b5aacbbf10e 100644 --- a/dotCMS/src/main/java/com/dotmarketing/business/ajax/RoleAjax.java +++ b/dotCMS/src/main/java/com/dotmarketing/business/ajax/RoleAjax.java @@ -570,11 +570,11 @@ public void saveRoleLayouts(String roleId, String[] layoutIds) throws DotDataExc //Validate if this logged in user has the required permissions to access the roles portlet validateRolesPortletPermissions(getLoggedInUser()); + getAdminUser(); LayoutAPI layoutAPI = APILocator.getLayoutAPI(); RoleAPI roleAPI = APILocator.getRoleAPI(); Role role = roleAPI.loadRoleById(roleId); - User user = getAdminUser(); List layouts = layoutAPI.loadLayoutsForRole(role); //Looking for removed layouts @@ -803,6 +803,7 @@ public List> getRolePermissions(String roleId) throws DotDat public void saveRolePermission(String roleId, String folderHostId, Map permissions, boolean cascade) throws DotDataException, DotSecurityException, PortalException, SystemException { //Validate if this logged in user has the required permissions to access the roles portlet validateRolesPortletPermissions(getLoggedInUser()); + getAdminUser(); Logger.info(this, "Applying role permissions for role " + roleId + " and folder/host id " + folderHostId); From d3863c8008fbb522dfd521263b41784494fcde10 Mon Sep 17 00:00:00 2001 From: mbiuki Date: Sun, 28 Jun 2026 15:06:59 -0400 Subject: [PATCH 2/4] sec: enforce CMS Administrator on _addtouser endpoint (private-issues#642) Any authenticated backend user could call PUT /api/v1/toolgroups/{id}/_addtouser to self-assign arbitrary layouts, including the admin Settings layout containing the roles/users portlets. Combined with the pre-existing DWR addUserToRole check (getAdminUser()), this broke the full privilege-escalation + RCE chain. Adds .requiredRoles(Role.CMS_ADMINISTRATOR_ROLE) to the _addtouser InitBuilder so non-admin callers receive a 403/SecurityException before any layout operation runs. Also adds regression tests: - ToolGroupResourceTest: verifies 403 for non-admin, success for admin - RoleAjaxSecurityTest: verifies addUserToRole blocks a non-admin who already holds roles-portlet access (post-step-1 state), confirming independent defense-in-depth Co-Authored-By: Claude Sonnet 4.6 --- .../api/v1/portlet/ToolGroupResource.java | 2 + .../src/test/java/com/dotcms/MainSuite2b.java | 4 + .../api/v1/portlet/ToolGroupResourceTest.java | 102 ++++++++++++++++ .../business/ajax/RoleAjaxSecurityTest.java | 113 ++++++++++++++++++ 4 files changed, 221 insertions(+) create mode 100644 dotcms-integration/src/test/java/com/dotcms/rest/api/v1/portlet/ToolGroupResourceTest.java create mode 100644 dotcms-integration/src/test/java/com/dotmarketing/business/ajax/RoleAjaxSecurityTest.java diff --git a/dotCMS/src/main/java/com/dotcms/rest/api/v1/portlet/ToolGroupResource.java b/dotCMS/src/main/java/com/dotcms/rest/api/v1/portlet/ToolGroupResource.java index ece7ceb94695..ea02cc8ebf4f 100644 --- a/dotCMS/src/main/java/com/dotcms/rest/api/v1/portlet/ToolGroupResource.java +++ b/dotCMS/src/main/java/com/dotcms/rest/api/v1/portlet/ToolGroupResource.java @@ -7,6 +7,7 @@ import com.dotmarketing.business.APILocator; import com.dotmarketing.business.ApiProvider; import com.dotmarketing.business.Layout; +import com.dotmarketing.business.Role; import com.dotmarketing.exception.DotDataException; import com.dotmarketing.exception.DotSecurityException; import com.liferay.portal.model.User; @@ -111,6 +112,7 @@ public final Response addToolGroupToUser(@Context final HttpServletRequest reque final User loggedInUser = new WebResource.InitBuilder(webResource) .requiredBackendUser(true) + .requiredRoles(Role.CMS_ADMINISTRATOR_ROLE) .requestAndResponse(request, response) .rejectWhenNoUser(true) .init() diff --git a/dotcms-integration/src/test/java/com/dotcms/MainSuite2b.java b/dotcms-integration/src/test/java/com/dotcms/MainSuite2b.java index 4c48d54ccd2a..fdaccff000c8 100644 --- a/dotcms-integration/src/test/java/com/dotcms/MainSuite2b.java +++ b/dotcms-integration/src/test/java/com/dotcms/MainSuite2b.java @@ -108,6 +108,7 @@ import com.dotcms.rest.api.v1.folder.FolderResourceTest; import com.dotcms.rest.api.v1.maintenance.ClusterLogCollectorTest; import com.dotcms.rest.api.v1.menu.MenuResourceTest; +import com.dotcms.rest.api.v1.portlet.ToolGroupResourceTest; import com.dotcms.rest.api.v1.publishing.BundleManagementResourceIntegrationTest; import com.dotcms.rest.api.v1.publishing.PublishingResourceIntegrationTest; import com.dotcms.rest.api.v1.pushpublish.PushPublishFilterResourceTest; @@ -137,6 +138,7 @@ import com.dotmarketing.business.VersionableFactoryImplTest; import com.dotmarketing.business.helper.PermissionHelperTest; import com.dotmarketing.common.db.DBTimeZoneCheckTest; +import com.dotmarketing.business.ajax.RoleAjaxSecurityTest; import com.dotmarketing.filters.AutoLoginFilterTest; import com.dotmarketing.filters.CMSUrlUtilIntegrationTest; import com.dotmarketing.image.focalpoint.FocalPointAPITest; @@ -367,6 +369,7 @@ Task220606UpdatePushNowActionletNameTest.class, BundlerUtilTest.class, MenuResourceTest.class, + ToolGroupResourceTest.class, AWSS3PublisherTest.class, ContentTypeInitializerTest.class, CSSPreProcessServletIT.class, @@ -463,6 +466,7 @@ com.dotmarketing.business.IdentifierAPITest.class, com.dotmarketing.business.CommitListenerCacheWrapperTest.class, com.dotmarketing.business.RoleAPITest.class, + RoleAjaxSecurityTest.class, com.dotmarketing.business.IdentifierConsistencyIntegrationTest.class, com.dotmarketing.business.LayoutAPITest.class, com.dotmarketing.business.PermissionAPIIntegrationTest.class, diff --git a/dotcms-integration/src/test/java/com/dotcms/rest/api/v1/portlet/ToolGroupResourceTest.java b/dotcms-integration/src/test/java/com/dotcms/rest/api/v1/portlet/ToolGroupResourceTest.java new file mode 100644 index 000000000000..b78d4d7bd435 --- /dev/null +++ b/dotcms-integration/src/test/java/com/dotcms/rest/api/v1/portlet/ToolGroupResourceTest.java @@ -0,0 +1,102 @@ +package com.dotcms.rest.api.v1.portlet; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotNull; + +import com.dotcms.IntegrationTestBase; +import com.dotcms.datagen.LayoutDataGen; +import com.dotcms.datagen.TestUserUtils; +import com.dotcms.datagen.UserDataGen; +import com.dotcms.mock.request.MockAttributeRequest; +import com.dotcms.mock.request.MockHttpRequestIntegrationTest; +import com.dotcms.mock.response.MockHttpResponse; +import com.dotcms.rest.exception.SecurityException; +import com.dotcms.util.IntegrationTestInitService; +import com.dotmarketing.business.APILocator; +import com.dotmarketing.business.Layout; +import com.liferay.portal.model.User; +import com.liferay.portal.util.WebKeys; +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletResponse; +import javax.ws.rs.core.Response; +import org.junit.AfterClass; +import org.junit.BeforeClass; +import org.junit.Test; + +/** + * Integration tests for {@link ToolGroupResource}. + * Verifies that the {@code /_addtouser} endpoint enforces CMS Administrator access, + * closing the privilege-escalation path documented in private-issues#642. + */ +public class ToolGroupResourceTest extends IntegrationTestBase { + + private static ToolGroupResource resource; + private static HttpServletResponse mockResponse; + private static User adminUser; + private static User backendUser; + private static Layout testLayout; + + @BeforeClass + public static void prepare() throws Exception { + IntegrationTestInitService.getInstance().init(); + + resource = new ToolGroupResource(); + mockResponse = new MockHttpResponse().response(); + + adminUser = TestUserUtils.getAdminUser(); + + backendUser = new UserDataGen().nextPersisted(); + APILocator.getRoleAPI().addRoleToUser( + APILocator.getRoleAPI().loadBackEndUserRole(), backendUser); + + testLayout = new LayoutDataGen().name("ToolGroupResourceTest-layout").nextPersisted(); + } + + @AfterClass + public static void cleanUp() throws Exception { + if (testLayout != null) { + APILocator.getRoleAPI().removeLayoutFromRole( + testLayout, adminUser.getUserRole()); + APILocator.getLayoutAPI().removeLayout(testLayout); + } + if (backendUser != null) { + APILocator.getUserAPI().delete(backendUser, APILocator.systemUser(), false); + } + } + + /** + * Method to test: {@link ToolGroupResource#addToolGroupToUser} + * Given Scenario: A low-privilege backend user (non-admin) calls {@code PUT /{layoutId}/_addtouser} + * Expected Result: A {@link SecurityException} is thrown and the user does NOT gain the layout, + * closing the first step of private-issues#642 privilege escalation chain. + */ + @Test(expected = SecurityException.class) + public void test_addToolGroupToUser_lowPrivilegeUser_throwsSecurityException() throws Exception { + final HttpServletRequest request = requestFor(backendUser); + resource.addToolGroupToUser(request, mockResponse, testLayout.getId(), null); + } + + /** + * Method to test: {@link ToolGroupResource#addToolGroupToUser} + * Given Scenario: A CMS Administrator calls {@code PUT /{layoutId}/_addtouser} + * Expected Result: HTTP 200 OK is returned and the layout is added to the admin's user role. + */ + @Test + public void test_addToolGroupToUser_adminUser_succeeds() throws Exception { + final HttpServletRequest request = requestFor(adminUser); + final Response response = resource.addToolGroupToUser( + request, mockResponse, testLayout.getId(), null); + + assertNotNull("Response must not be null", response); + assertEquals("Admin should get HTTP 200", Response.Status.OK.getStatusCode(), + response.getStatus()); + } + + private static HttpServletRequest requestFor(final User user) { + final HttpServletRequest request = new MockAttributeRequest( + new MockHttpRequestIntegrationTest("localhost", "/api/v1/toolgroups").request() + ).request(); + request.setAttribute(WebKeys.USER, user); + return request; + } +} diff --git a/dotcms-integration/src/test/java/com/dotmarketing/business/ajax/RoleAjaxSecurityTest.java b/dotcms-integration/src/test/java/com/dotmarketing/business/ajax/RoleAjaxSecurityTest.java new file mode 100644 index 000000000000..0bc22ab64f24 --- /dev/null +++ b/dotcms-integration/src/test/java/com/dotmarketing/business/ajax/RoleAjaxSecurityTest.java @@ -0,0 +1,113 @@ +package com.dotmarketing.business.ajax; + +import static org.junit.Assert.assertTrue; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +import com.dotcms.IntegrationTestBase; +import com.dotcms.datagen.LayoutDataGen; +import com.dotcms.datagen.UserDataGen; +import com.dotcms.repackage.org.directwebremoting.WebContext; +import com.dotcms.repackage.org.directwebremoting.WebContextFactory; +import com.dotcms.util.IntegrationTestInitService; +import com.dotmarketing.business.APILocator; +import com.dotmarketing.business.Layout; +import com.dotmarketing.exception.DotSecurityException; +import com.liferay.portal.model.User; +import com.liferay.portal.util.WebKeys; +import com.liferay.util.servlet.SessionMessages; +import java.util.LinkedHashMap; +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpSession; +import org.junit.AfterClass; +import org.junit.BeforeClass; +import org.junit.Test; + +/** + * Regression tests for the privilege-escalation fix in {@link RoleAjax#addUserToRole}. + * + * The attack chain from private-issues#642 requires that a non-admin backend user first + * self-assigns a layout that includes the "roles" portlet via the {@code _addtouser} endpoint + * (now blocked), and then calls {@code addUserToRole} to add themselves to CMS Administrator. + * This test verifies that step 2 is independently blocked by the {@code getAdminUser()} check, + * even when the caller already holds roles-portlet access. + */ +public class RoleAjaxSecurityTest extends IntegrationTestBase { + + private static User backendUser; + private static Layout rolesPortletLayout; + private static String cmsAdminRoleId; + + @BeforeClass + public static void prepare() throws Exception { + IntegrationTestInitService.getInstance().init(); + + backendUser = new UserDataGen().nextPersisted(); + APILocator.getRoleAPI().addRoleToUser( + APILocator.getRoleAPI().loadBackEndUserRole(), backendUser); + + // Simulate the post-step-1 state: give the backend user access to the roles portlet + // (this is what _addtouser would have provided before it was hardened). + rolesPortletLayout = new LayoutDataGen().name("RoleAjaxSecurityTest-roles-layout") + .portletIds("roles", "users") + .nextPersisted(); + APILocator.getRoleAPI().addLayoutToRole(rolesPortletLayout, backendUser.getUserRole()); + + assertTrue("backend user should now have roles portlet access", + APILocator.getLayoutAPI().doesUserHaveAccessToPortlet("roles", backendUser)); + + cmsAdminRoleId = APILocator.getRoleAPI().loadCMSAdminRole().getId(); + } + + @AfterClass + public static void cleanUp() throws Exception { + if (rolesPortletLayout != null) { + APILocator.getRoleAPI().removeLayoutFromRole(rolesPortletLayout, + backendUser.getUserRole()); + APILocator.getLayoutAPI().removeLayout(rolesPortletLayout); + } + if (backendUser != null) { + APILocator.getUserAPI().delete(backendUser, APILocator.systemUser(), false); + } + } + + /** + * Method to test: {@link RoleAjax#addUserToRole} + * Given Scenario: A low-privilege backend user has already obtained roles-portlet access + * (simulating a successful step 1), then calls the DWR endpoint to add + * themselves to the CMS Administrator role (step 2 of the attack chain). + * Expected Result: {@link DotSecurityException} is thrown by the {@code getAdminUser()} + * admin check — the self-escalation is blocked. + */ + @Test(expected = DotSecurityException.class) + public void test_addUserToRole_withRolesPortletAccess_cannotSelfEscalateToAdmin() + throws Exception { + setUpDwrContext(backendUser); + + final RoleAjax roleAjax = new RoleAjax(); + roleAjax.addUserToRole(backendUser.getUserId(), cmsAdminRoleId); + } + + private static void setUpDwrContext(final User user) { + final HttpSession session = mock(HttpSession.class); + when(session.getAttribute(SessionMessages.KEY)).thenReturn(new LinkedHashMap<>()); + + final HttpServletRequest request = mock(HttpServletRequest.class); + when(request.getSession()).thenReturn(session); + when(request.getAttribute(WebKeys.USER)).thenReturn(user); + + final WebContext webContext = mock(WebContext.class); + when(webContext.getHttpServletRequest()).thenReturn(request); + + final WebContextFactory.WebContextBuilder builderMock = + mock(WebContextFactory.WebContextBuilder.class); + when(builderMock.get()).thenReturn(webContext); + + final com.dotcms.repackage.org.directwebremoting.Container containerMock = + mock(com.dotcms.repackage.org.directwebremoting.Container.class); + when(containerMock.getBean(WebContextFactory.WebContextBuilder.class)) + .thenReturn(builderMock); + + WebContextFactory.attach(containerMock); + } +} From 995f78eb82424980293f219297fcd6da964c8979 Mon Sep 17 00:00:00 2001 From: mbiuki Date: Mon, 29 Jun 2026 12:02:51 -0400 Subject: [PATCH 3/4] chore: trigger clean CI run From e46c48ffad79eda8270fd81bbe9639236dd47837 Mon Sep 17 00:00:00 2001 From: mbiuki Date: Mon, 29 Jun 2026 12:28:57 -0400 Subject: [PATCH 4/4] test: guard removeLayoutFromRole in ToolGroupResourceTest teardown prepare() never adds testLayout to adminUser's role; only the admin success test does via the API call. Wrapping in try-catch prevents a guaranteed DotDataException on teardown when that test is skipped or fails before the association is created. Co-Authored-By: Claude Sonnet 4.6 --- .../dotcms/rest/api/v1/portlet/ToolGroupResourceTest.java | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/dotcms-integration/src/test/java/com/dotcms/rest/api/v1/portlet/ToolGroupResourceTest.java b/dotcms-integration/src/test/java/com/dotcms/rest/api/v1/portlet/ToolGroupResourceTest.java index b78d4d7bd435..7a791dea136f 100644 --- a/dotcms-integration/src/test/java/com/dotcms/rest/api/v1/portlet/ToolGroupResourceTest.java +++ b/dotcms-integration/src/test/java/com/dotcms/rest/api/v1/portlet/ToolGroupResourceTest.java @@ -55,8 +55,12 @@ public static void prepare() throws Exception { @AfterClass public static void cleanUp() throws Exception { if (testLayout != null) { - APILocator.getRoleAPI().removeLayoutFromRole( - testLayout, adminUser.getUserRole()); + try { + APILocator.getRoleAPI().removeLayoutFromRole( + testLayout, adminUser.getUserRole()); + } catch (Exception e) { + // layout may not have been associated if the admin test did not run + } APILocator.getLayoutAPI().removeLayout(testLayout); } if (backendUser != null) {