Conversation
…iling only Client supplied sectionCrumbs could previously satisfy the admin entity security check for any entity, allowing privilege escalation. Co-Authored-By: Kevin Lee <kevin.lee@cognition.ai>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| //Authorization is performed exclusively against the security ceiling of the entity actually being operated | ||
| //on. Section crumbs are client supplied navigation state and must never widen the set of ceilings the user | ||
| //is authorized against. | ||
| String securityCeiling = persistencePackage.getSecurityCeilingEntityFullyQualifiedClassname(); |
There was a problem hiding this comment.
🔍 Sub-collection requests now authorize against the collection entity, not the owning section
The PR description states "nested-collection requests continue to authorize against the owning section's ceiling as before", but that is only true for the LOOKUP_FOR_UPDATE path that now explicitly sets a security ceiling (admin/broadleaf-open-admin-platform/src/main/java/org/broadleafcommerce/openadmin/server/service/AdminEntityServiceImpl.java:657). For ordinary sub-collection fetch/add/update/remove, PersistencePackageRequest.fromMetadata sets only ceilingEntityClassname to the collection ceiling (see admin/broadleaf-open-admin-platform/src/main/java/org/broadleafcommerce/openadmin/server/domain/PersistencePackageRequest.java:143-176 and AdminEntityServiceImpl.java:395-437), and PersistencePackage.getSecurityCeilingEntityFullyQualifiedClassname() falls back to that ceiling. Previously the section crumb ceiling (the owning section entity) also satisfied the check via the OR loop; now it does not. OSS seed data in core/broadleaf-framework/src/main/resources/config/bc/sql/load_admin_permissions.sql does register most collection entities (CategoryXrefImpl, ProductOptionXref, SkuAttribute, ...), so out-of-the-box flows should keep working, but downstream projects with custom collections that were never registered in BLC_ADMIN_PERMISSION_ENTITY will start getting SecurityServiceException on collection screens. Worth calling out explicitly in release notes/upgrade docs.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Correct, and that tightening is inherent to the fix: any path that previously passed only because a crumb ceiling satisfied the OR now requires permission on the target ceiling. OSS seed data covers the standard collection entities, but downstream projects with unregistered custom collection entities will need BLC_ADMIN_PERMISSION_ENTITY records. I've called this out in the PR description as an upgrade note.
| //the operation updates the looked up member of a collection owned by the entity currently being | ||
| //managed, so authorize against that owning entity's ceiling, as derived from server side metadata | ||
| ppr.withSecurityCeilingEntityClassname(StringUtils.isNotBlank(mainMetadata.getSecurityCeilingType()) | ||
| ? mainMetadata.getSecurityCeilingType() : mainMetadata.getCeilingType()); |
There was a problem hiding this comment.
📝 Info: LOOKUP_FOR_UPDATE still authorizes only against the owning entity, not the entity being modified
For AddMethodType.LOOKUP_FOR_UPDATE the persistence operation is a full update of the looked-up target record, yet authorization is now performed against the owning entity's ceiling (mainMetadata), mirroring the old sectionCrumbs[0] behavior. The client-controlled input is removed (good), but a user holding only UPDATE on the owning section can still mutate the target entity without holding permission on it. There are no OSS usages of LOOKUP_FOR_UPDATE, so this is latent; consider whether the target ceiling should additionally be checked (the new AND-semantics helper would support passing both).
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Same as the other LOOKUP_FOR_UPDATE thread — noted as latent and left unchanged deliberately; see #97 (comment).
Co-Authored-By: Kevin Lee <kevin.lee@cognition.ai>
| //the operation updates the looked up member of a collection owned by the entity currently being | ||
| //managed, so authorize against that owning entity's ceiling, as derived from server side metadata | ||
| ppr.withSecurityCeilingEntityClassname(StringUtils.isNotBlank(mainMetadata.getSecurityCeilingType()) | ||
| ? mainMetadata.getSecurityCeilingType() : mainMetadata.getCeilingType()); |
There was a problem hiding this comment.
📝 Info: Security ceiling for lookup-for-update now derives from the current section rather than the first crumb
mainMetadata in all three addSubCollectionEntity callers (AdminBasicEntityController selectize-add, add, addEmpty) is obtained via getSectionPersistencePackageRequest(mainClassName, ...), and AdminEntityServiceImpl.getClassMetadata (line 111) always populates securityCeilingType (it falls back to the ceiling class), so the isNotBlank(...) ? ... : getCeilingType() fallback here is effectively dead but harmless. Behaviorally, the ceiling used is now the current section entity, whereas the removed code in add(...) used sectionCrumbs[0], which after getSectionCrumbs appends the current section last is the outermost (client-supplied) crumb — i.e. this is a genuine tightening, not merely a source change. One residual difference: when no crumbs were present the old code left the security ceiling as the looked-up target class; now the owning entity's ceiling is always used, which is slightly more permissive in that (rare) case.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Thanks — the analysis matches my reading. On the residual no-crumbs case: it only arises for LOOKUP_FOR_UPDATE, which has no OSS usages, and the alternatives (leaving the ceiling as the looked-up target, or requiring both ceilings via the AND helper) are behavior decisions that would deny flows working today, so I'm surfacing them to the maintainers rather than deciding here. The fallback to getCeilingType() is defensive only, consistent with other callers.
| /** | ||
| * Verifies the current admin user is qualified for the given operation on <b>every</b> supplied ceiling. A user | ||
| * must never gain access to one ceiling by virtue of holding a permission on another. | ||
| */ | ||
| protected void securityCheck(String[] ceilingNames, EntityOperationType operationType) throws ServiceException { |
There was a problem hiding this comment.
📝 Info: Multi-ceiling helper semantics flipped from OR to AND with no remaining multi-ceiling caller
After this change the only production caller reaches securityCheck(String[]) through the single-string overload, so the AND semantics are currently unobservable outside tests. The method remains protected, meaning subclasses in downstream modules that previously relied on the OR ("qualified for any ceiling") contract will silently get inverted behavior. Since no in-repo caller passes multiple ceilings, consider making the array overload private or renaming it to make the new contract explicit for downstream extenders.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Intentionally left protected — narrowing visibility would be a source-breaking change for downstream extenders, and the inverted contract is the point of the fix (a subclass relying on the OR contract is relying on the vulnerability). The javadoc states the new AND semantics explicitly.
1 participant
A Brief Overview
AdminSecurityServiceRemote.securityCheck(PersistencePackage, EntityOperationType)authorized an operation against the union of the target entity's security ceiling and the identifiers of the request's section crumbs, passing as soon as the user qualified for any one of them. Section crumbs come from the client-controlledsectionCrumbsrequest parameter and are only validated to name some registered entity, so an admin holding a single permission (e.g. UPDATE on Product) could append a crumb naming that entity and pass the check for any other entity — including creating/alteringAdminUser/AdminRole/AdminPermissionfor full takeover.Authorization is now performed solely against the entity actually being operated on:
The multi-ceiling helper
securityCheck(String[], EntityOperationType)now requires the user to qualify for every supplied ceiling (fails on the first that is not qualified) so no future caller can regain the OR behavior; the unregistered-entity diagnostic reports the ceiling that actually failed.A variant of the same path is closed in
AdminEntityServiceImpl: forAddMethodType.LOOKUP_FOR_UPDATEthe security ceiling was overwritten withsectionCrumbs[0](also client-controlled) insideadd(...). It is now set inaddSubCollectionEntityfrom the server-sideClassMetadataof the entity that owns the collection:New
AdminSecurityServiceRemoteSpeccovers: a crumb naming an authorized entity no longer satisfies the check for an unrelated target (and the crumb ceiling is never queried), a permission on the target ceiling still passes, and an explicit security ceiling takes precedence over the ceiling entity.Add Labels to the right panel: Security, Status (ready-for-code-review)
Additional context
Upgrade note — any request that previously passed only because a crumb ceiling satisfied the OR is now denied:
BLC_ADMIN_PERMISSION_ENTITYwill need those records.Link to Devin session: https://app.devin.ai/sessions/ab99b537f9fd42ba9f7c18bf758975a7
Requested by: @kevinlrd
Devin Review