Skip to content

Custom file roots should be considered valid server managed roots. #8232

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
labkey-klum wants to merge 6 commits into
base: develop
Choose a base branch
from
Open

Custom file roots should be considered valid server managed roots. #8232

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
cbf0b17
Custom file roots should be considered valid server managed roots.
labkey-klum Oct 26, 2026
69e68d5
simplify
labkey-klum Oct 26, 2026
c4cc43d
Merge branch 'develop' into fb_file_watcher_copy_path
labkey-klum Oct 27, 2026
375f8e0
use URIUtil.isDescendant
labkey-klum Oct 27, 2026
c8f108d
typo
labkey-klum Oct 27, 2026
8b8ab33
Merge branch 'develop' into fb_file_watcher_copy_path
labkey-klum Oct 28, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 11 additions & 2 deletions api/src/org/labkey/api/files/FileContentService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,10 +68,19 @@ static void setInstance(FileContentService impl)
}

/**
* Returns a list of Container in which the path resides.
* Returns the container whose file root holds the path, or null if none does.
*/
@Nullable
Container getContainerForFilePath(java.nio.file.Path path);

/** @deprecated use {@link #getContainerForFilePath(java.nio.file.Path)} */
@Deprecated
@NotNull
List<Container> getContainersForFilePath(java.nio.file.Path path);
default List<Container> getContainersForFilePath(java.nio.file.Path path)
{
Container c = getContainerForFilePath(path);
return c == null ? List.of() : List.of(c);
}

/**
* Returns the file root of the specified container. If not explicitly defined,
Expand Down
4 changes: 2 additions & 2 deletions experiment/src/org/labkey/experiment/controllers/exp/ExperimentController.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2452,8 +2452,8 @@ protected ModelAndView getDataView(DataForm form, BindException errors) throws I
if (!underFileRoot)
{
// Issue 35649: ImmPort module "publish" creates exp.data object in this container for paths that originate in a different container
List<Container> containers = fileSvc.getContainersForFilePath(dataPath);
if (containers.isEmpty() || containers.stream().noneMatch(c -> c.hasPermission(getUser(), ReadPermission.class)))
Container fileContainer = fileSvc.getContainerForFilePath(dataPath);
if (fileContainer == null || !fileContainer.hasPermission(getUser(), ReadPermission.class))
throw new UnauthorizedException("Data file is not under the pipeline root for this folder");
}
}
Expand Down
117 changes: 78 additions & 39 deletions filecontent/src/org/labkey/filecontent/FileContentServiceImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -193,57 +193,69 @@ private FileContentServiceImpl()
}

@Override
@NotNull
public List<Container> getContainersForFilePath(java.nio.file.Path path)
public @Nullable Container getContainerForFilePath(java.nio.file.Path path)

@cnathe cnathe Oct 27, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

claude code review came up with a few high level findings that might be worth reviewing before human code review:

Three security concerns from review:

1. **`getContainerForFilePath` doesn't normalize its input** (platform, `FileContentServiceImpl`). The custom-root check is a plain `startsWith`, so a path like `<ProjA root>/../ProjB/@files/secret.txt` resolves to ProjA. `ExperimentController` passes the raw `DataFileUrl` into its Read check, so a user with Read on ProjA can download ProjB files. Normalizing at the top of the method would fix it for every caller.
2. **Falling back to a parent container** (platform, `FileContentServiceImpl`). The develop code only accepted the deepest folder matched in the path. The new code moves up to a parent when that folder's root doesn't hold the path, e.g. the root is disabled or was moved. Files left under `<site>/P/Secret/@files/` are then attributed to `P`, so a user with Read on P but not on Secret can access them.
3. **Watch `location` has the same `..` gap** (premium, `FileWatcherPipelineTriggerType.validate`, existing code). `location` isn't normalized before its `startsWith(fileRoot)` check, so `<fileRoot>/../../../etc` passes, and a null pipeline root raises no error. This gets around the new copy-destination check from the source side, so it's worth fixing here or tracking as a follow-up.

@labkey-matthewb labkey-matthewb Oct 28, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can see that in practice this doesn't happen, but two containers could point to the same directory as a pipeline root. I'm not sure what the usage of this method are and how this is meant to be resolved, or if it's fine to be arbitrary?

{
// Ignore cloud files for now
if (FileUtil.hasCloudScheme(path))
return Collections.emptyList();
return null;

// Anchor on the most specific root holding the path: the site default root or a custom file root
Container anchor = null;
java.nio.file.Path anchorPath = null;
java.nio.file.Path defaultRootPath = getSiteDefaultRoot().toPath();
if (URIUtil.isDescendant(defaultRootPath.toUri(), path.toUri()))
{
anchor = ContainerManager.getRoot();
anchorPath = defaultRootPath;
}

// If the path is under the default root, do optimistic simple match for containers under the default root
File defaultRoot = getSiteDefaultRoot();
java.nio.file.Path defaultRootPath = defaultRoot.toPath();
if (path.startsWith(defaultRootPath))
for (FileRoot root : FileRootManager.get().getFileRoots())
{
java.nio.file.Path rel = defaultRootPath.relativize(path);
if (rel.getNameCount() > 0)
// Skip cloud roots before resolving: resolution goes through CloudStoreService, which may be absent or misconfigured
if (isCloudFileRoot(root.getPath()))
continue;
Container c = ContainerManager.getForId(root.getContainer());
if (c == null || c.isRoot() || isUseDefaultRoot(c))
continue;
java.nio.file.Path rootPath = getExistingFileRootPath(c);

@labkey-matthewb labkey-matthewb Oct 28, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

check hasCloudScheme() in this loop somewhere too?

@labkey-klum labkey-klum Oct 29, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a short circuit on line 215 but cloud scheme paths should get resolved during getExistingFileRootPath

if (rootPath != null && URIUtil.isDescendant(rootPath.toUri(), path.toUri()) && (anchorPath == null || URIUtil.isDescendant(anchorPath.toUri(), rootPath.toUri())))
{
Container root = ContainerManager.getRoot();
Container next = root;
while (rel.getNameCount() > 0)
{
// check if there exists a child container that matches the next path segment
java.nio.file.Path top = rel.subpath(0, 1);
assert top != null;
Container child = next.getChild(top.getFileName().toString());
if (child == null)
break;
anchor = c;
anchorPath = rootPath;
}
}

next = child;
if (anchor == null)
return null;

if(rel.getNameCount() > 1)
{
rel = rel.subpath(1, rel.getNameCount());
}
else
{
break;
}
}
// Child folder roots default to a matching subdirectory, so walk the path's segments down the container tree
List<Container> candidates = new ArrayList<>(List.of(anchor));
Container next = anchor;

if (next != null && !next.equals(root))
{
// verify our naive file path is correct for the container -- it may have a file root other than the default
java.nio.file.Path fileRoot = getFileRootPath(next);
if (fileRoot != null && path.startsWith(fileRoot))
return Collections.singletonList(next);
}
}
for (java.nio.file.Path segment : anchorPath.relativize(path))
{
next = next.getChild(segment.toString());
if (next == null)
break;
candidates.add(next);
}

// TODO: Create cache of file root and pipeline root paths -> list of containers
// A folder may have its own root elsewhere, so take the deepest candidate whose actual root holds the path
for (Container c : candidates.reversed())
{
if (c.isRoot())
continue;
java.nio.file.Path fileRoot = getExistingFileRootPath(c);
if (fileRoot != null && URIUtil.isDescendant(fileRoot.toUri(), path.toUri()))
return c;
}

return Collections.emptyList();
return null;
}

private @Nullable java.nio.file.Path getExistingFileRootPath(@NotNull Container c)
{
return isFileRootDisabled(c) ? null : resolveFileRootPath(c, false);
}

@Override
Expand Down Expand Up @@ -1849,6 +1861,33 @@ public void fileRootsTest()

}

@Test
public void testContainerForFilePath()
{
//pre-clean
cleanup();

FileContentService svc = FileContentService.get();
Assert.assertNotNull(svc);

Container project1 = ContainerManager.createContainer(ContainerManager.getRoot(), PROJECT1, TestContext.get().getUser());
Container subfolder1 = ContainerManager.createContainer(project1, PROJECT1_SUBFOLDER1, TestContext.get().getUser());
Container project2 = ContainerManager.createContainer(ContainerManager.getRoot(), PROJECT2, TestContext.get().getUser());

java.nio.file.Path project2File = svc.getFileRootPath(project2, ContentType.files).resolve(TXT_FILE);
Assert.assertEquals("Default root path", project2, svc.getContainerForFilePath(project2File));

File testRoot = getTestRoot();
svc.setFileRoot(project1, testRoot);

java.nio.file.Path subfolderFile = svc.getFileRootPath(subfolder1, ContentType.files).resolve(TXT_FILE);
Assert.assertEquals("Inherited custom root path", subfolder1, svc.getContainerForFilePath(subfolderFile));
Assert.assertEquals("Custom root path", project1, svc.getContainerForFilePath(testRoot.toPath().resolve("unmatched").resolve(TXT_FILE)));

java.nio.file.Path unmanaged = svc.getSiteDefaultRoot().toPath().resolve(FILE_ROOT_SUFFIX + "Unmanaged").resolve(TXT_FILE);
Assert.assertNull("Unmanaged path", svc.getContainerForFilePath(unmanaged));
}

private void assertPathsEqual(String msg, File expected, File actual)
{
String expectedPath = FileUtil.getAbsoluteCaseSensitiveFile(expected).getPath();
Expand Down
6 changes: 6 additions & 0 deletions filecontent/src/org/labkey/filecontent/FileRootManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
import org.labkey.api.files.FileRoot;
import org.labkey.api.security.User;

import java.util.Collection;
import java.util.Collections;
import java.util.HashMap;
import java.util.Map;
Expand Down Expand Up @@ -72,6 +73,11 @@ public FileRoot getFileRoot(Container c)
return null == root ? new FileRoot(c) : root;
}

public Collection<FileRoot> getFileRoots()
{
return CACHE.get(CACHE_KEY).values();
}

public void deleteFileRoot(Container c)
{
SimpleFilter filter = SimpleFilter.createContainerFilter(c);
Expand Down
Loading