File path related issue bundle by XingY · Pull Request #6847 · LabKey/platform

XingY · 2025-07-15T20:32:46Z

Rationale

This PR include the following changes:

Reject out-of-scope (path outside of file root) file paths and path that doesn't exist on insert/update/import or update via file.
Make sure display/messaging doesn’t reveal file existence information by using a generic error message for invalid file on import/update, and generic handling of rendering unavailable files without revealing the reason.
Only show the file name and not file full path. This include disabling filter/sort for file columns and stop including full path in api responses and custom property views, etc.

Related Pull Requests

Changes

…oesn't exist

XingY · 2025-07-17T16:00:12Z

api/src/org/labkey/api/data/ExpDataFileConverter.java

+        {
+            URI uri = FileUtil.createUri(filePath);
+            if (FileUtil.FILE_SCHEME.equals(uri.getScheme()))
+                return new File(uri);


@labkey-matthewb Do you have any suggestions how to get around using new File and maybe use FileLike here? In order for f.isAbsolute() to work for path that starts with file:/, I had to convert string to uri first before passing to File constructor.

If you are stuck in File land, use FileUtil.appendPath() and FileUtil.appendName() instead of new File(File,String).

To get out of File land, you have to start with getting FileLike for the directories you expect to find the files in. E.g. PipeRoot.getRootFileLike() instead of PipeRoot.getPath(). Also PipeRoot has useful resolve() methods and isUnderRoot() methods.

I should have rewritten this whole class to be ExpDataFileLikeConverter and killed usages convert(File.class). Lets leave that cleanup for a separate PR.

…nt columns

labkey-matthewb · 2025-07-30T21:05:46Z

api/src/org/labkey/api/data/ExpDataFileConverter.java

@@ -218,49 +219,101 @@ public Object convert(Class type, Object value)
        User user = (User) QueryService.get().getEnvironment(QueryService.Environment.USER);
        Object fileRootPathObj = QueryService.get().getEnvironment(QueryService.Environment.FILEROOTPATH);


Consider putting a FileLike in the environment instead of a String? I don't love that we're using QueryService.get/setEnvironment() here, but that's a problem for another time.

2nd thought. Can we refactor this so there is a public convert() method that takes (user, fileRootPath, assayFilesPath) and then have _convert() call that method with these values? Then we could have QUS and DataLoader call that method directly. Other usages could still use still rely on thread-local storage until/unless we remove this from ConvertUtils.

I changed this to a static method that can be called directly. Currently there are 3 usages for importing assay results that could be refactored to use this static method. However, I didn't go ahead with the refactoring due to the much entangled run/result association. If we are removing this from ConvertUtils for sure then that's an effort worth trying. I've added a TODO comment.

Changes made to no longer register ExpDataFileConverter for File.class

labkey-matthewb · 2025-07-30T21:07:48Z

api/src/org/labkey/api/data/ExpDataFileConverter.java

                    {
                        // Interpret relative paths based on the file root
-                        f = new File(root.getRootPath(), f.getPath());
+                        fileUnderRoot = new File(root.getRootPath(), f.getPath());


FileUtil.appendPath()

Unfortunately switching to FileUtil.appendPath broke valid relative path such as "../@files/test.txt". But maybe we can just say this is not supported.

# Conflicts: # study/test/src/org/labkey/test/tests/study/StudyDatasetFileFieldTest.java

labkey-matthewb

I'm not sure I can track all the subclasses here, but this looks like a good step forward. I only have one question about files that exists on disk, but aren't in a known file root. Would getFileName() benefit from having a structured return value?

And the tests will be super helpful.

labkey-matthewb · 2025-07-30T22:41:32Z

api/src/org/labkey/api/reader/DataLoader.java

            }
+
+            // use File converter for known file fields even if inferTypes = false. If inferTypes, this is already done.
+            if (!getInferTypes())


I think this is where I get lost. Why do we we rely on this? I would hope that import would always work with no conversion in DataLoader. (AFAIK, the only code that "should" rely on this is currently the infer type UI)

labkey-matthewb · 2025-08-04T23:30:52Z

api/src/org/labkey/api/data/AbstractFileDisplayColumn.java

-            FileImageRenderHelper renderHelper = createRenderHelper(ctx, url, filename, displayName, fileIconUrl, popupIconUrl, thumbnail, isImage);
+            String displayName = getFileName(ctx, fileValue);
+            boolean unavailable = displayName.endsWith(UNAVAILABLE_FILE_SUFFIX);
+            String url = null == s || unavailable ? null : s.eval(ctx);


Nice check here. It would be nice if getFileName() had an explicit structured return (e.g. record(String fileDisplay,Boolean available), but this is workable. I wonder about the case in FileLinkDisplayColumn where we fall back to result = f.getName() when we don't find the file in an expected "file root".

What is the expected behavior in this case? Should we treat as "unavailable"? Should we render a URL?

In the case of result = f.getName() when we fail to find file in root, valid is false, and the "unavailable" suffix will be added.

I added selenium test for grid and update view for LKS unavailable file both due to invalid path (exists but out side of file root) and absent file (file deleted after upload) in this commit

XingY and others added 4 commits July 15, 2025 13:29

File path related bugs

a39b7b6

Merge branch 'refs/heads/develop' into fb_filePathIssues

a7dc3c7

fix relative path import

4f7a260

fix invalid path that starts with file scheme, or if under root but d…

a272233

…oesn't exist

XingY commented Jul 17, 2025

View reviewed changes

XingY added 7 commits July 20, 2025 19:58

Merge branch 'develop' into fb_filePathIssues

ef9b4a3

fix assay import exception handling, disable sort filter for attachme…

333ab4f

…nt columns

allow sort filter for attachment columns

d27a817

Bug fixes for cross folder import

8d8c1dd

Merge branch 'develop' into fb_filePathIssues

30c6657

Selenium tests

618ab5a

Fix assayfile import

b7f2c1f

This was referenced Jul 25, 2025

File path related issue bundle LabKey/labkey-ui-components#1833

Merged

Selenium tests for file path import LabKey/testAutomation#2582

Merged

XingY added 6 commits July 25, 2025 09:50

Merge branch 'develop' into fb_filePathIssues

4addc97

add test coverage for study dataset

7f0bfbc

fix various assay api, add tests for assay api

60a2b2a

clean

371b587

clean

734f58d

Merge branch 'develop' into fb_filePathIssues

6bb3c3d

XingY requested a review from labkey-matthewb July 28, 2025 18:49

XingY mentioned this pull request Jul 28, 2025

Update SignalDataRawTest to provide the correct file path for assay data LabKey/commonAssays#910

Merged

XingY added 4 commits July 29, 2025 13:21

Merge branch 'develop' into fb_filePathIssues

e3cbed5

rename

bdb93d6

refactor for attachment

a2b48a8

refactor for attachment

461682c

labkey-matthewb reviewed Jul 30, 2025

View reviewed changes

XingY added 2 commits July 30, 2025 14:47

Merge branch 'develop' into fb_filePathIssues

8a723bf

# Conflicts: # study/test/src/org/labkey/test/tests/study/StudyDatasetFileFieldTest.java

code review changes

36866fd

XingY added 7 commits July 31, 2025 18:51

Fix relative path

716c06f

Merge branch 'develop' into fb_filePathIssues

8d38384

no converter

72977bd

code review changes

a31ef1f

Fix assay run api, more cleaning up

233bb66

Merge branch 'develop' into fb_filePathIssues

a2997a9

fix more tests

f3053d5

XingY requested a review from labkey-matthewb August 4, 2025 15:31

XingY added 3 commits August 4, 2025 12:18

Merge branch 'develop' into fb_filePathIssues

92616e0

fix tests for Windows with quoted path

e2b8a2a

fix tests for Windows with quoted path

d5b5e25

labkey-matthewb approved these changes Aug 4, 2025

View reviewed changes

XingY added 3 commits August 4, 2025 18:37

more code review changes, add test for LKS file availability check

c2aee9e

Merge branch 'develop' into fb_filepathIssues

eec1c62

fix cross folder saveRows api

793f9ea

XingY merged commit 81f2c9e into develop Aug 5, 2025
8 of 9 checks passed

XingY deleted the fb_filePathIssues branch August 5, 2025 16:27

labkey-nicka mentioned this pull request Aug 18, 2025

Remove coerceTypes call from AbstractQueryUpdateService #6941

Merged

		@@ -218,49 +219,101 @@ public Object convert(Class type, Object value)
		User user = (User) QueryService.get().getEnvironment(QueryService.Environment.USER);
		Object fileRootPathObj = QueryService.get().getEnvironment(QueryService.Environment.FILEROOTPATH);

Conversation

XingY commented Jul 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rationale

Related Pull Requests

Changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

labkey-matthewb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

XingY commented Jul 15, 2025 •

edited

Loading