sec: harden portlet-only gated methods in RoleAjax#36345
Conversation
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 <noreply@anthropic.com>
mbiuki
added
Team : Scout
Team : Security
|
Claude finished @mbiuki's task in 1m 50s —— View job Rollback Safety Analysis
Result: ✅ Safe To RollbackThe label AnalysisThe diff touches 5 files. Each was checked against all rollback-unsafe categories:
Category-by-category verdict
All changes are pure security hardening at the Java auth layer. No storage model, index mapping, data migration, or API contract is altered. Rolling back to N-1 restores the previous auth behavior with no data loss or structural incompatibility. |
🤖 dotBot Review (Bedrock)Reviewed 5 file(s); 1 candidate(s) → 1 confirmed, 0 uncertain (unverified, kept for review). Confirmed findings
us.deepseek.r1-v1:0 · Run: #28394797390 · tokens: in: 18423 · out: 4822 · total: 23245 · calls: 8 · est. ~$0.051 |
Versions AffectedExploitable range: v21.02 → v26.06.22-03 (current latest)
No patched release has been cut yet — this PR is part of the fix. |
…#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 <noreply@anthropic.com>
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.
|
github-project-automation
Bot
moved this from Next Sprint
to Done
in dotCMS - Product Planning
|
@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
Bot
added
Area : Backend
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| @AfterClass | ||
| public static void cleanUp() throws Exception { | ||
| if (testLayout != null) { | ||
| APILocator.getRoleAPI().removeLayoutFromRole( |
There was a problem hiding this comment.
🟠 [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.
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 <noreply@anthropic.com>
|
|
||
| final User loggedInUser = new WebResource.InitBuilder(webResource) | ||
| .requiredBackendUser(true) | ||
| .requiredRoles(Role.CMS_ADMINISTRATOR_ROLE) |
There was a problem hiding this comment.
🟠 [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.
CI Results — Run 28387215869Overall: ❌ Failure (pre-existing flaky tests, unrelated to this PR) Job Summary
Flaky Test Failures in MainSuite 2bThe 5 failures are all pre-existing, unrelated to this PR:
The PR's new tests passed cleanly:
AssessmentThis PR is ready to merge. All failures are pre-existing flaky tests with no connection to the security hardening changes in
|
Vulnerability Introduction Timeline
|
| 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:
saveRoleLayoutswas 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.
2 participants
Summary
Follow-on to #36344. Hardens four
RoleAjaxDWR methods that used onlyvalidateRolesPortletPermissions()as their authorization gate.After reviewing the actual code, the scope is narrower than the issue assumed:
saveRolePermissiongetAdminUser()addedsaveRoleLayoutsgetAdminUser()present but after DB readsupdateLayoutdeleteLayoutsaveRolePermissionwas the only method genuinely missing the admin check.saveRoleLayoutshad it in the right place logically (before writes) but after two DB reads — tightened to fire before any data access.Changes
RoleAjax.javasaveRolePermission: addgetAdminUser()immediately after portlet-access checksaveRoleLayouts: move existinggetAdminUser()call from after DB reads to immediately after portlet-access check; remove now-unusedUser user =assignmentTest plan
DotSecurityExceptiononsaveRolePermissionvia DWRDotSecurityExceptiononsaveRoleLayoutsvia DWRupdateLayoutanddeleteLayoutbehavior unchangedReferences
🤖 Generated with Claude Code