sec: harden portlet-only gated methods in RoleAjax

[mbiuki]

Summary

Follow-on to #36344. Hardens four RoleAjax DWR methods that used only validateRolesPortletPermissions() as their authorization gate.

After reviewing the actual code, the scope is narrower than the issue assumed:

Method	Before	After
saveRolePermission	Portlet check only — no admin gate	+ getAdminUser() added
saveRoleLayouts	getAdminUser() present but after DB reads	Moved before DB reads
updateLayout	Already correctly ordered	No change
deleteLayout	Already correctly ordered	No change

saveRolePermission was the only method genuinely missing the admin check. saveRoleLayouts had it in the right place logically (before writes) but after two DB reads — tightened to fire before any data access.

Changes

RoleAjax.java

• saveRolePermission: add getAdminUser() immediately after portlet-access check
• saveRoleLayouts: move existing getAdminUser() call from after DB reads to immediately after portlet-access check; remove now-unused User user = assignment

Test plan

• Non-admin user with roles portlet access receives DotSecurityException on saveRolePermission via DWR
• Non-admin user with roles portlet access receives DotSecurityException on saveRoleLayouts via DWR
• CMS Administrator can save role layouts and role permissions normally via the admin UI
• updateLayout and deleteLayout behavior unchanged

References

• Closes: dotCMS/private-issues#643
• Parent fix: sec: enforce admin check on layout assignment and role grant endpoints #36344 (dotCMS/private-issues#642)

🤖 Generated with Claude Code

[claude]

Claude finished @mbiuki's task in 1m 3s —— View job

---

I'll analyze this and get back to you.

[github-actions]

🤖 dotBot Review (Bedrock)

Reviewed 5 file(s); 3 candidate(s) → 2 confirmed, 0 uncertain (unverified, kept for review).

Confirmed findings

• 🔴 Critical dotCMS/src/main/java/com/dotmarketing/business/ajax/RoleAjax.java:573 — Admin check bypass in role layout updates
  Removal of 'User user = getAdminUser()' assignment causes subsequent permission check to use logged-in user instead of admin, allowing non-admins with roles portlet access to modify role layouts. The permission check at line 573 now references an undefined 'user' variable, which likely resolves to the current user rather than an admin.
• 🟡 Medium dotcms-integration/src/test/java/com/dotcms/rest/api/v1/portlet/ToolGroupResourceTest.java:60 — Silent exception swallowing in test cleanup
  In ToolGroupResourceTest.java line 60, exceptions during test role cleanup are caught but not logged or reported. This could mask failures in test teardown that might lead to data leaks between tests. While test cleanup is often best-effort, silent swallowing makes debugging harder when dependencies exist between tests.

---

_{us.deepseek.r1-v1:0 · Run: #28672054880 · tokens: in: 21925 · out: 6849 · total: 28774 · calls: 10 · est. ~$0.067}

[mbiuki]

Versions Affected

Exploitable range: v21.02 → v26.06.22-03 (current latest)

Gap	Introduced	First vulnerable release
RoleAjax.saveRolePermission (portlet-only gate, no admin check)	March 2012	v3.0
ToolGroupResource._addtouser (enabling step)	December 2020	v21.02

No patched release has been cut yet — this PR is part of the fix.

[mbiuki]

CI Re-run Results ✅

All checks passed on the re-run (run #28331839580).

The previous failures (QuartzUtils DB deadlock, StoryBlockMap NPE, E2E/Postman timeouts) were confirmed as pre-existing flaky infrastructure issues unrelated to this PR's changes.

This PR is clear to merge.

Suite	Result
Integration Tests - MainSuite 1a	✅ pass
Integration Tests - MainSuite 1b	✅ pass
Integration Tests - MainSuite 2a/2b/3a	✅ pass
Integration Tests - Junit5 Suite 1	✅ pass
Integration Tests - OpenSearch Upgrade	✅ pass
E2E Tests - Nx Playwright	✅ pass
Postman Tests - GraphQL	✅ pass
All other Postman/Karate/CLI/JVM suites	✅ pass

[wezell]

@mbiuki @sfreudenthaler we should do a pass over all the DWR endpoints to make sure there are no other vulnerabilities. These all predate any of our security hardening efforts. Here is a list of these endpoints.

https://github.com/dotCMS/core/blob/main/dotCMS/src/main/webapp/WEB-INF/dwr.xml

[github-actions]

🟠 [High] Test cleanup removes layout from role without verifying existence

In ToolGroupResourceTest's cleanUp(), lines 58-59 attempt to remove testLayout from adminUser's role via roleAPI.removeLayoutFromRole(), but the test setup (prepare()) never adds this layout to the role. This results in a guaranteed DotDataException during teardown when trying to remove a non-existent layout-role association. Test infrastructure code should either: 1) Add the layout to the role during setup before removal, or 2) Check if the association exists before attempting removal.

[github-actions]

🟠 [High] Over-restrictive role requirement in ToolGroupResource

The PR adds CMS_ADMINISTRATOR_ROLE to @RolesAllowed annotation, requiring admin privileges for a method that should be accessible to non-admin users with proper portlet permissions. This breaks existing access control logic by introducing an unnecessary admin role requirement, as evidenced by the test cases in ToolGroupResourceTest which don't validate admin-only access.

[mbiuki]

CI Results — Run 28387215869

Overall: ❌ Failure (pre-existing flaky tests, unrelated to this PR)

Job Summary

Job	Result
Initialize	✅ Pass
PR Build / Initial Artifact Build	✅ Pass
PR Test / Integration Tests - MainSuite 2b	❌ Fail (flaky tests)
All other test jobs (27/30)	✅ Pass
Finalize / Final Status	❌ Fail (downstream of MainSuite 2b)

Flaky Test Failures in MainSuite 2b

The 5 failures are all pre-existing, unrelated to this PR:

• PermissionAPITest.issue560 / issue781 / test_templateLayout_parentPermissionableIsHost — intermittent DotSecurityException and hostname uniqueness constraint errors
• StoryBlockMapTest.* (10 tests) — NPE: HttpSession.getAttribute() is null (known flaky test, also failed in prior runs)
• ShortyServletAndTitleImageTest.test_ShortyServlet_With_AuthenticatedUser — unrelated infrastructure failure

The PR's new tests passed cleanly:

• ✅ ToolGroupResourceTest — verifies _addtouser now enforces CMS Administrator access
• ✅ RoleAjaxSecurityTest — verifies addUserToRole blocks non-admin self-escalation

Assessment

This PR is ready to merge. All failures are pre-existing flaky tests with no connection to the security hardening changes in RoleAjax.java and ToolGroupResource.java.

Note: The ai-automatic-review / bedrock-harness check flagged .requiredRoles(Role.CMS_ADMINISTRATOR_ROLE) as "over-restrictive" — this is a false positive. Restricting _addtouser to CMS Administrators is the intentional security fix closing the privilege escalation path documented in private-issues#642.

[mbiuki]

Vulnerability Introduction Timeline

RoleAjax.saveRolePermission — admin check accidentally removed

Date	Commit	Author	Event
pre-2016	(pre-repo)	—	getAdminUser() admin guard present in saveRolePermission
2018-01-18	95391d6613	Daniel Silva	Guard accidentally deleted in "Fix more jenkins tests" (#13427) — drive-by cleanup removed the security check
2025-11-04	3babaf0d17	—	Partial portlet-level mitigation added (private-issues#482) — requires Roles portlet access but not admin
PR #36345	—	—	Full fix: getAdminUser() restored — not yet in any release

Note: saveRoleLayouts was not vulnerable — getAdminUser() was always present. This PR only moves it to fail-fast before DB reads.

Affected Versions

• First vulnerable release: 5.0.0 (2018-08-08) — check removed Jan 2018, first release Aug 2018
• 4.x LTS unaffected — ran on a separate branch that did not receive commit 95391d6613
• All affected: 5.0.0 → v26.06.22-03 (current latest) — ~8 years of exposure
• Partial mitigation from: v25.11.07-1 (exploitability requires Roles portlet access, which PR sec: enforce admin check on layout assignment and role grant endpoints #36344 now closes)
• Fully fixed in: not yet released

Combined Attack Chain Exposure Window

Both vulnerabilities (_addtouser from 2020 + saveRolePermission from 2018) were independently exploitable but chained together in the full RCE path documented in private-issues#642. The chain as described required both to be present — so the combined exploit window is 21.02 → present.

[mbiuki]

Red Team Review — PR #36345

Three independent reviewers examined this PR in parallel across security, performance, and test coverage.

Verdicts

Dimension	Verdict
Security	APPROVED_WITH_COMMENTS
Performance	MINOR_IMPROVEMENT
Test Coverage	NEEDS_IMPROVEMENT

---

Security

The critical fix is correctly placed — getAdminUser() fires before the transaction boundary in saveRolePermission and before all DB reads in saveRoleLayouts. The full attack chain (private-issues#642) is closed.

Findings:

Severity	Location	Finding
Info	getAdminUser()	Checks "users-portlet access OR CMS Admin" — not strictly CMS Admin only. All layout assignment paths now require admin, but canAddPortletToLayout("users") should be verified.
Medium	RoleAjax.java:573	getAdminUser() return value discarded in saveRoleLayouts — audit trail gap vs. other call sites that capture the user for AdminLogger.
Medium (pre-existing)	RoleAjax.java:293, 324	addUserToRole + removeUsersFromRole do DB reads before their admin guard — inconsistent with the hardened pattern here, not introduced by this PR.
Medium (pre-existing)	RoleAjax.java:211	getUsersByRole() has zero auth check — any authenticated user can enumerate CMS Admin role members.
Low (pre-existing)	ToolGroupResource.java:66–92	_removefromuser has no admin gate for cross-user layout removal.

All bypass vectors confirmed closed (direct DWR, _addtouser chain, saveRoleLayouts chain, DWR reflection — all blocked).

---

Performance

Net positive. The saveRoleLayouts fail-fast reorder eliminates 2 DB reads (loadRoleById + loadLayoutsForRole) for unauthorized callers. The new getAdminUser() call in saveRolePermission adds ~1 in-memory RoleCache lookup on the happy path — sub-microsecond. No N+1 risks, no new DB hits on the happy path anywhere.

---

Test Coverage

The two methods most central to this PR have zero regression tests.

Gap	Impact
saveRolePermission — not tested	Primary vulnerability fixed here has no regression test
saveRoleLayouts — not tested	Second method in PR title has no regression test
No happy-path test for admin calling addUserToRole	Only sad path exercised
ToolGroupResource ?userid= delegation path untested	Cross-user assignment attack surface not covered
No HTTP-level DWR test	All tests use direct Java calls; real attackers hit /DotAjaxDirector

Minimum recommended additions:

@Test(expected = DotSecurityException.class)
public void test_saveRolePermission_nonAdmin_throwsSecurityException() { ... }

@Test(expected = DotSecurityException.class)
public void test_saveRoleLayouts_nonAdmin_throwsSecurityException() { ... }

@Test
public void test_addUserToRole_adminUser_succeeds() { ... }

---

Non-Blocking Follow-ups

1. Add saveRolePermission + saveRoleLayouts non-admin regression tests
2. Capture getAdminUser() return in saveRoleLayouts for audit logging
3. Move getAdminUser() to method-top in addUserToRole and removeUsersFromRole
4. Add auth check to getUsersByRole() (pre-existing info disclosure)
5. Verify canAddPortletToLayout("users") and canAddPortletToLayout("roles") return false
6. Consider admin gate for _removefromuser when userid targets a different user

The fix is correct and the full attack chain is closed. All follow-ups are non-blocking.

[github-actions]

🔴 [Critical] Admin check bypass in role layout updates

Removal of 'User user = getAdminUser()' assignment causes subsequent permission check to use logged-in user instead of admin, allowing non-admins with roles portlet access to modify role layouts. The permission check at line 573 now references an undefined 'user' variable, which likely resolves to the current user rather than an admin.