From 69a9a011b2ad57e333c037c67972787209b5ddab Mon Sep 17 00:00:00 2001 From: labkey-matthewb Date: Mon, 17 Aug 2026 10:33:47 -0700 Subject: [PATCH 1/2] handle cloud root case --- .../labkey/core/admin/AdminController.java | 89 ++++++++++++++++--- 1 file changed, 79 insertions(+), 10 deletions(-) diff --git a/core/src/org/labkey/core/admin/AdminController.java b/core/src/org/labkey/core/admin/AdminController.java index 97175dc35ba..7addafe0e7b 100644 --- a/core/src/org/labkey/core/admin/AdminController.java +++ b/core/src/org/labkey/core/admin/AdminController.java @@ -6575,17 +6575,36 @@ private static void throwIfUnauthorizedFileRootChange(ViewContext ctx, FileConte // or if the file root is changed. if (!service.isUseDefaultRoot(ctx.getContainer())) { - Path fileRootPath = service.getFileRootPath(ctx.getContainer()); - if (null != fileRootPath) - { - String absolutePath = FileUtil.getAbsolutePath(ctx.getContainer(), fileRootPath); - if (Strings.CI.equals(absolutePath, form.getFolderRootPath())) - { - if (!ctx.getUser().hasRootPermission(AdminOperationsPermission.class)) - throw new UnauthorizedException("Only site admins can change file roots"); - } - } + requestedRoot = form.getCloudRootName(); + currentRoot = (!isUseDefaultRoot && service.isCloudRoot(ctx.getContainer())) ? service.getCloudRootName(ctx.getContainer()) : null; + } + else + { + requestedRoot = StringUtils.trimToNull(form.getFolderRootPath()); + Path fileRootPath = isUseDefaultRoot ? null : service.getFileRootPath(ctx.getContainer()); + currentRoot = null != fileRootPath ? FileUtil.getAbsolutePath(ctx.getContainer(), fileRootPath) : null; } + + if (!isFileRootChangeAuthorized(hasAdminOpsPermission, isUseDefaultRoot, currentRoot, requestedRoot)) + throw new UnauthorizedException("Only site admins can change file roots"); + } + + /** + * Pure decision logic behind {@link #throwIfUnauthorizedFileRootChange}, factored out for unit testing. + * @param hasAdminOpsPermission whether the requesting user holds root AdminOperationsPermission + * @param isUseDefaultRoot whether the target container currently uses the default (inherited) file root + * @param currentRoot the container's existing custom root path/cloud name, or null if there isn't one + * @param requestedRoot the root path/cloud name submitted in the request, or null if none was submitted + */ + static boolean isFileRootChangeAuthorized(boolean hasAdminOpsPermission, boolean isUseDefaultRoot, @Nullable String currentRoot, @Nullable String requestedRoot) + { + if (hasAdminOpsPermission) + return true; + if (null == requestedRoot) + return isUseDefaultRoot || null == currentRoot; // clearing an existing custom root is still a change to it + if (!isUseDefaultRoot &&requestedRoot.equalsIgnoreCase(currentRoot)) + return true; // no-op resubmission of the folder's own existing custom root + return false; } public static void setEnabledCloudStores(ViewContext ctx, FileManagementForm form, BindException errors) @@ -9564,6 +9583,56 @@ public void modulesWithSchemaVersionButNoScripts() } } + // Regression coverage for the file root privilege-escalation fix: a folder/project admin without root + // AdminOperationsPermission must never be able to switch a container to a custom file root, or change an + // existing custom root's path. + public static class FileRootPermissionTestCase extends Assert + { + @Test + public void defaultRootRequiresAdminOpsPermissionForNewCustomPath() + { + // This is the case the original (inverted) condition silently skipped: a container on the default + // root, submitting any custom path, from a user without AdminOperationsPermission. + assertFalse(isFileRootChangeAuthorized(false, true, null, "/some/path")); + } + + @Test + public void customRootRequiresAdminOpsPermissionForDifferentPath() + { + assertFalse(isFileRootChangeAuthorized(false, false, "/existing/path", "/attacker/path")); + } + + @Test + public void customRootResubmissionOfSamePathIsAllowed() + { + assertTrue(isFileRootChangeAuthorized(false, false, "/existing/path", "/existing/path")); + assertTrue(isFileRootChangeAuthorized(false, false, "/Existing/Path", "/existing/path")); + } + + @Test + public void noRequestedRootOnDefaultRootIsAllowed() + { + // No custom root requested and none currently exists -- nothing to protect. + assertTrue(isFileRootChangeAuthorized(false, true, null, null)); + } + + @Test + public void clearingAnExistingCustomRootRequiresAdminOpsPermission() + { + // A request that omits the root (e.g. a non-ops admin's disabled form fields not being submitted) + // must not be able to silently clear an existing custom root back to default. + assertFalse(isFileRootChangeAuthorized(false, false, "/existing/path", null)); + assertTrue(isFileRootChangeAuthorized(true, false, "/existing/path", null)); + } + + @Test + public void adminOpsPermissionIsAlwaysAllowed() + { + assertTrue(isFileRootChangeAuthorized(true, true, null, "/some/path")); + assertTrue(isFileRootChangeAuthorized(true, false, "/existing/path", "/attacker/path")); + } + } + public static class ModuleForm { private String _name; From ea21b9864860aa530d9d5577300f542e253913b6 Mon Sep 17 00:00:00 2001 From: labkey-matthewb Date: Mon, 17 Aug 2026 10:47:11 -0700 Subject: [PATCH 2/2] idea split my changelist --- core/src/org/labkey/core/CoreModule.java | 1 + .../src/org/labkey/core/admin/AdminController.java | 14 ++++++++++---- 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/core/src/org/labkey/core/CoreModule.java b/core/src/org/labkey/core/CoreModule.java index 44b4e02c3a3..a6267d86be4 100644 --- a/core/src/org/labkey/core/CoreModule.java +++ b/core/src/org/labkey/core/CoreModule.java @@ -1500,6 +1500,7 @@ public TabDisplayMode getTabDisplayMode() public @NotNull Set> getUnitTests() { return Set.of( + AdminController.FileRootPermissionTestCase.class, ApiJsonWriter.TestCase.class, ClassLoaderTestCase.class, CopyFileRootPipelineJob.TestCase.class, diff --git a/core/src/org/labkey/core/admin/AdminController.java b/core/src/org/labkey/core/admin/AdminController.java index 7addafe0e7b..8140b693eaa 100644 --- a/core/src/org/labkey/core/admin/AdminController.java +++ b/core/src/org/labkey/core/admin/AdminController.java @@ -6570,10 +6570,16 @@ private static void initiateCopyFilesPipelineJobs(ViewContext ctx, @NotNull List private static void throwIfUnauthorizedFileRootChange(ViewContext ctx, FileContentService service, FileManagementForm form) { - // test permissions. only site admins are able to turn on a custom file root for a folder - // this is only relevant if the folder is either being switched to a custom file root, - // or if the file root is changed. - if (!service.isUseDefaultRoot(ctx.getContainer())) + // Only site admins (AdminOperationsPermission) are able to switch a folder to a custom file root, or + // change an existing custom root's path -- doing so lets a folder admin point file storage anywhere the + // server process can read/write. Resubmitting the folder's own current custom root unchanged is a no-op + // and does not require the elevated permission. + boolean hasAdminOpsPermission = ctx.getUser().hasRootPermission(AdminOperationsPermission.class); + boolean isUseDefaultRoot = service.isUseDefaultRoot(ctx.getContainer()); + String requestedRoot; + String currentRoot; + + if (form.isCloudFileRoot()) { requestedRoot = form.getCloudRootName(); currentRoot = (!isUseDefaultRoot && service.isCloudRoot(ctx.getContainer())) ? service.getCloudRootName(ctx.getContainer()) : null;