Repository navigation
Custom file roots should be considered valid server managed roots. - #8232
Open
labkey-klum wants to merge 6 commits into
Open
labkey-klum wants to merge 6 commits into
labkey-klum wants to merge 6 commits into
Conversation
cnathe
reviewed
Oct 27, 2026
| @Override | ||
| @NotNull | ||
| public List<Container> getContainersForFilePath(java.nio.file.Path path) | ||
| public @Nullable Container getContainerForFilePath(java.nio.file.Path path) |
Contributor
There was a problem hiding this comment.
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.
| if (c == null || c.isRoot() || isUseDefaultRoot(c)) | ||
| continue; | ||
| java.nio.file.Path rootPath = getExistingFileRootPath(c); | ||
| if (rootPath != null && path.startsWith(rootPath) && (anchorPath == null || rootPath.startsWith(anchorPath))) |
Contributor
There was a problem hiding this comment.
PipeRoot does all this... Unfortunate it's not directly shareable. I think URIUtil.isDescendant() is preferred and handles some edge case.
labkey-matthewb
requested changes
Oct 27, 2026
| @Override | ||
| @NotNull | ||
| public List<Container> getContainersForFilePath(java.nio.file.Path path) | ||
| public @Nullable Container getContainerForFilePath(java.nio.file.Path path) |
Contributor
There was a problem hiding this comment.
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?
labkey-matthewb
approved these changes
Oct 28, 2026
| Container c = ContainerManager.getForId(root.getContainer()); | ||
| if (c == null || c.isRoot() || isUseDefaultRoot(c)) | ||
| continue; | ||
| java.nio.file.Path rootPath = getExistingFileRootPath(c); |
Contributor
There was a problem hiding this comment.
check hasCloudScheme() in this loop somewhere too?
Contributor
Author
There was a problem hiding this comment.
There is a short circuit on line 215 but cloud scheme paths should get resolved during getExistingFileRootPath
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Rationale
https://github.com/LabKey/internal-issues/issues/1660
The signature for
getContainersForFilePathwas :List<Container>but always returned a single container. This introduces a new method :getContainerForFilePathto reflect that reality and deprecates the old method.The existing implementation only considered paths that were based on the default site file root and custom file roots were not considered. The newer rewrite will now also consider custom file roots when determining which container a path belongs to.
Related Pull Requests
https://github.com/LabKey/premiumModules/pull/820