From af26bffa1b420e31104c5e5318f6de0f7b918690 Mon Sep 17 00:00:00 2001 From: labkey-jeckels Date: Thu, 15 May 2025 16:32:58 -0700 Subject: [PATCH 1/6] Issue 53027: Cannot insert a row into a sample type if Allowed File Extensions are set and no file is provided --- .../api/premium/DefaultAVMultipartResolver.java | 15 ++++++++++++--- api/src/org/labkey/api/util/FileUtil.java | 5 ----- 2 files changed, 12 insertions(+), 8 deletions(-) diff --git a/api/src/org/labkey/api/premium/DefaultAVMultipartResolver.java b/api/src/org/labkey/api/premium/DefaultAVMultipartResolver.java index cd3b963cbf4..b54f076d12f 100644 --- a/api/src/org/labkey/api/premium/DefaultAVMultipartResolver.java +++ b/api/src/org/labkey/api/premium/DefaultAVMultipartResolver.java @@ -3,8 +3,10 @@ import jakarta.servlet.ServletException; import jakarta.servlet.http.HttpServletRequest; import jakarta.servlet.http.Part; +import org.apache.commons.lang3.StringUtils; import org.jetbrains.annotations.NotNull; import org.labkey.api.util.FileUtil; +import org.labkey.api.view.UnauthorizedException; import org.springframework.web.multipart.MultipartException; import org.springframework.web.multipart.MultipartHttpServletRequest; import org.springframework.web.multipart.support.StandardMultipartHttpServletRequest; @@ -22,10 +24,17 @@ public class DefaultAVMultipartResolver extends StandardServletMultipartResolver for (Part part : request.getParts()) { // Filter to just file uploads - if (part.getSubmittedFileName() != null) + if (!StringUtils.isEmpty(part.getSubmittedFileName())) { - FileUtil.checkAllowedFileName(part.getSubmittedFileName(), true); - validate(part); + try + { + FileUtil.checkAllowedFileName(part.getSubmittedFileName(), true); + validate(part); + } + catch (IOException e) + { + throw new UnauthorizedException(e.getMessage()); + } } } } diff --git a/api/src/org/labkey/api/util/FileUtil.java b/api/src/org/labkey/api/util/FileUtil.java index 8446e0c3535..dfabf4fcbbc 100644 --- a/api/src/org/labkey/api/util/FileUtil.java +++ b/api/src/org/labkey/api/util/FileUtil.java @@ -1533,11 +1533,6 @@ else if (ch == '-' && if (ch == ' ' || ch == '.') ret[lastIndex] = '_'; - String result = new String(ret); - - assert !AppProps.getWriteableInstance().isInvalidFilenameBlocked() || isAllowedFileName(result, true) == null : - "Failed to make filename safe. Original: " + name + ", transformed: " + result + ", error: " + isAllowedFileName(result, true); - return new String(ret); } From c49433e43e31b82f2a9237d71ff3722581235164 Mon Sep 17 00:00:00 2001 From: labkey-jeckels Date: Fri, 16 May 2025 13:39:30 -0700 Subject: [PATCH 2/6] Switch exception type, code cleanup, include accepted file extensions --- .../premium/DefaultAVMultipartResolver.java | 11 +++++++- api/src/org/labkey/api/util/FileUtil.java | 2 +- .../MockHttpResponseWithRealPassthrough.java | 25 ++++++++++--------- 3 files changed, 24 insertions(+), 14 deletions(-) diff --git a/api/src/org/labkey/api/premium/DefaultAVMultipartResolver.java b/api/src/org/labkey/api/premium/DefaultAVMultipartResolver.java index b54f076d12f..0899c3b8f77 100644 --- a/api/src/org/labkey/api/premium/DefaultAVMultipartResolver.java +++ b/api/src/org/labkey/api/premium/DefaultAVMultipartResolver.java @@ -5,6 +5,7 @@ import jakarta.servlet.http.Part; import org.apache.commons.lang3.StringUtils; import org.jetbrains.annotations.NotNull; +import org.labkey.api.action.ApiUsageException; import org.labkey.api.util.FileUtil; import org.labkey.api.view.UnauthorizedException; import org.springframework.web.multipart.MultipartException; @@ -33,7 +34,7 @@ public class DefaultAVMultipartResolver extends StandardServletMultipartResolver } catch (IOException e) { - throw new UnauthorizedException(e.getMessage()); + throw new InvalidFileExtensionException(e.getMessage()); } } } @@ -49,4 +50,12 @@ protected void validate(Part part) { //do nothing by default, but give subclasses a chance to override } + + public static class InvalidFileExtensionException extends ApiUsageException + { + public InvalidFileExtensionException(String msg) + { + super(msg); + } + } } diff --git a/api/src/org/labkey/api/util/FileUtil.java b/api/src/org/labkey/api/util/FileUtil.java index dfabf4fcbbc..ff7dece5515 100644 --- a/api/src/org/labkey/api/util/FileUtil.java +++ b/api/src/org/labkey/api/util/FileUtil.java @@ -326,7 +326,7 @@ static String isAllowedFileName(String s, boolean checkFileExtension, AppProps a { String badExtension = checkExtension(s, AppProps.getInstance()); if (badExtension != null) - return "This file type [" + badExtension + "] is not allowed."; + return "This file type [" + badExtension + "] is not allowed. Accepted file extensions: " + AppProps.getInstance().getAllowedExtensions(); } return null; } diff --git a/api/src/org/labkey/api/view/MockHttpResponseWithRealPassthrough.java b/api/src/org/labkey/api/view/MockHttpResponseWithRealPassthrough.java index ef97b11cf24..41f136bbeb8 100644 --- a/api/src/org/labkey/api/view/MockHttpResponseWithRealPassthrough.java +++ b/api/src/org/labkey/api/view/MockHttpResponseWithRealPassthrough.java @@ -15,6 +15,7 @@ */ package org.labkey.api.view; +import org.jetbrains.annotations.NotNull; import org.labkey.api.action.ApiUsageException; import org.springframework.mock.web.MockHttpServletResponse; @@ -50,7 +51,7 @@ public HttpServletResponse getResponse() } @Override - public PrintWriter getWriter() throws UnsupportedEncodingException + public @NotNull PrintWriter getWriter() throws UnsupportedEncodingException { if (_writer == null) { @@ -76,7 +77,7 @@ public SizeLimitExceededException() /** Checks the size of the current output to make sure it hasn't exceeded the limit before appending the new content */ private class SizeLimitingPrintWriter extends PrintWriter { - private PrintWriter _out; + private final PrintWriter _out; public SizeLimitingPrintWriter(PrintWriter out) { @@ -100,28 +101,28 @@ private void checkSize() } @Override - public void write(char[] buf, int off, int len) + public void write(char @NotNull [] buf, int off, int len) { checkSize(); _out.write(buf, off, len); } @Override - public void write(char[] buf) + public void write(char @NotNull [] buf) { checkSize(); _out.write(buf); } @Override - public void write(String s, int off, int len) + public void write(@NotNull String s, int off, int len) { checkSize(); _out.write(s, off, len); } @Override - public void write(String s) + public void write(@NotNull String s) { checkSize(); _out.write(s); @@ -170,7 +171,7 @@ public void print(double d) } @Override - public void print(char[] s) + public void print(char @NotNull [] s) { checkSize(); _out.print(s); @@ -240,7 +241,7 @@ public void println(double x) } @Override - public void println(char[] x) + public void println(char @NotNull [] x) { checkSize(); _out.println(x); @@ -261,28 +262,28 @@ public void println(Object x) } @Override - public PrintWriter printf(String format, Object... args) + public PrintWriter printf(@NotNull String format, Object... args) { checkSize(); return _out.printf(format, args); } @Override - public PrintWriter printf(Locale l, String format, Object... args) + public PrintWriter printf(Locale l, @NotNull String format, Object... args) { checkSize(); return _out.printf(l, format, args); } @Override - public PrintWriter format(String format, Object... args) + public PrintWriter format(@NotNull String format, Object... args) { checkSize(); return _out.format(format, args); } @Override - public PrintWriter format(Locale l, String format, Object... args) + public PrintWriter format(Locale l, @NotNull String format, Object... args) { checkSize(); return _out.format(l, format, args); From fd4d12bd3209c5788dda7d16939349f1e73b8d8a Mon Sep 17 00:00:00 2001 From: labkey-jeckels Date: Mon, 19 May 2025 11:32:59 -0700 Subject: [PATCH 3/6] Add a metric for allow list usage (file extension and redirect hosts) --- core/src/org/labkey/core/CoreModule.java | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/core/src/org/labkey/core/CoreModule.java b/core/src/org/labkey/core/CoreModule.java index a6f8be9a8cb..6eda2d8a4d5 100644 --- a/core/src/org/labkey/core/CoreModule.java +++ b/core/src/org/labkey/core/CoreModule.java @@ -1196,6 +1196,14 @@ public void moduleStartupComplete(ServletContext servletContext) roleAssignments.put("dataClassDesignerCount", new SqlSelector(CoreSchema.getInstance().getSchema(), roleCountSql, "org.labkey.experiment.security.DataClassDesignerRole").getObject(Long.class)); roleAssignments.put("sampleTypeDesignerCount", new SqlSelector(CoreSchema.getInstance().getSchema(), roleCountSql, "org.labkey.experiment.security.SampleTypeDesignerRole").getObject(Long.class)); results.put("roleAssignments", roleAssignments); + + Map allowListCounts = new HashMap<>(); + for (AllowListType type : AllowListType.values()) + { + allowListCounts.put(type.name(), type.getValues().size()); + } + results.put("allowListCounts", allowListCounts); + return results; }); From 73f4600cb01ea51f868bff6628e95546a5627f9c Mon Sep 17 00:00:00 2001 From: labkey-jeckels Date: Mon, 9 Jun 2025 15:24:38 -0700 Subject: [PATCH 4/6] Sort file extensions centrally; Improve naming --- .../org/labkey/api/settings/AppPropsImpl.java | 18 +++++++++++------- .../api/settings/RandomStartupProperties.java | 4 ++-- .../labkey/api/settings/WriteableAppProps.java | 8 ++++---- .../org/labkey/core/admin/AdminController.java | 2 +- .../labkey/core/admin/existingListValues.jsp | 1 - 5 files changed, 18 insertions(+), 15 deletions(-) diff --git a/api/src/org/labkey/api/settings/AppPropsImpl.java b/api/src/org/labkey/api/settings/AppPropsImpl.java index c36c4070c2d..d424085b871 100644 --- a/api/src/org/labkey/api/settings/AppPropsImpl.java +++ b/api/src/org/labkey/api/settings/AppPropsImpl.java @@ -37,6 +37,7 @@ import org.labkey.api.util.MothershipReport; import org.labkey.api.util.PageFlowUtil; import org.labkey.api.util.Path; +import org.labkey.api.util.SortHelpers; import org.labkey.api.util.URLHelper; import org.labkey.api.util.UsageReportingLevel; import org.labkey.api.util.logging.LogHelper; @@ -49,6 +50,7 @@ import java.net.URISyntaxException; import java.util.ArrayList; import java.util.Arrays; +import java.util.Collections; import java.util.Comparator; import java.util.List; import java.util.Map; @@ -74,7 +76,7 @@ class AppPropsImpl extends AbstractWriteableSettingsGroup implements AppProps static final String LOOK_AND_FEEL_REVISION = "logoRevision"; static final String DEFAULT_LSID_AUTHORITY_PROP = "defaultLsidAuthority"; static final String OPTIONAL_FEATURE_PREFIX = OPTIONAL_FEATURE + "."; - static final String EXTERNAL_HOST_DELIMITER = "\n"; + static final String ALLOW_LIST_DELIMITER = "\n"; private static final String SITE_CONFIG_NAME = "SiteConfig"; private static final String SERVER_GUID = "serverGUID"; @@ -664,24 +666,26 @@ public boolean isIncludeServerHttpHeader() @NotNull public List getExternalRedirectHosts() { - return getExternalHosts(externalRedirectHostURLs); + return getAllowList(externalRedirectHostURLs); } @Override @NotNull public List getAllowedExtensions() { - return getExternalHosts(allowedFileExtensions); + return getAllowList(allowedFileExtensions); } - private List getExternalHosts(RandomStartupProperties propName) + private List getAllowList(RandomStartupProperties propName) { String urls = lookupStringValue(propName, ""); if (StringUtils.isNotBlank(urls)) { - return new ArrayList<>(Arrays.asList(urls.split(EXTERNAL_HOST_DELIMITER))); + List hosts = new ArrayList<>(Arrays.asList(urls.split(ALLOW_LIST_DELIMITER))); + hosts.sort(SortHelpers.getNaturalOrderStringComparator()); + return Collections.unmodifiableList(hosts); } - return new ArrayList<>(); + return Collections.emptyList(); } public static void populateSiteSettingsWithStartupProps() @@ -734,7 +738,7 @@ public List getExternalSourceHosts() String urls = lookupStringValue("externalSourceHostURLs", ""); if (StringUtils.isNotBlank(urls)) { - return new ArrayList<>(Arrays.asList(urls.split(EXTERNAL_HOST_DELIMITER))); + return new ArrayList<>(Arrays.asList(urls.split(ALLOW_LIST_DELIMITER))); } return new ArrayList<>(); } diff --git a/api/src/org/labkey/api/settings/RandomStartupProperties.java b/api/src/org/labkey/api/settings/RandomStartupProperties.java index 191a485f65a..e4ceefc70cc 100644 --- a/api/src/org/labkey/api/settings/RandomStartupProperties.java +++ b/api/src/org/labkey/api/settings/RandomStartupProperties.java @@ -26,7 +26,7 @@ public void setValue(WriteableAppProps writeable, String value) @Override public void setValue(WriteableAppProps writeable, String value) { - writeable.setExternalRedirectHosts(Arrays.asList(StringUtils.split(value, AppPropsImpl.EXTERNAL_HOST_DELIMITER))); + writeable.setExternalRedirectHosts(Arrays.asList(StringUtils.split(value, AppPropsImpl.ALLOW_LIST_DELIMITER))); } }, allowedFileExtensions("Allowed file extensions") @@ -34,7 +34,7 @@ public void setValue(WriteableAppProps writeable, String value) @Override public void setValue(WriteableAppProps writeable, String value) { - writeable.setAllowedFileExtensions(Arrays.asList(StringUtils.split(value, AppPropsImpl.EXTERNAL_HOST_DELIMITER))); + writeable.setAllowedFileExtensions(Arrays.asList(StringUtils.split(value, AppPropsImpl.ALLOW_LIST_DELIMITER))); FileUtil.clearExtensionChecker(); // Not sure this is needed, but better safe than sorry. } }, diff --git a/api/src/org/labkey/api/settings/WriteableAppProps.java b/api/src/org/labkey/api/settings/WriteableAppProps.java index 65bc81ba3b2..32933476119 100644 --- a/api/src/org/labkey/api/settings/WriteableAppProps.java +++ b/api/src/org/labkey/api/settings/WriteableAppProps.java @@ -229,17 +229,17 @@ public void setXFrameOption(String option) public void setExternalRedirectHosts(@NotNull Collection externalRedirectHosts) { - setExternalHosts(externalRedirectHostURLs, externalRedirectHosts); + setAllowList(externalRedirectHostURLs, externalRedirectHosts); } - private void setExternalHosts(RandomStartupProperties propName, @NotNull Collection externalSourceHosts) + private void setAllowList(RandomStartupProperties propName, @NotNull Collection externalSourceHosts) { - storeStringValue(propName, String.join(EXTERNAL_HOST_DELIMITER, externalSourceHosts)); + storeStringValue(propName, String.join(ALLOW_LIST_DELIMITER, externalSourceHosts)); } public void setAllowedFileExtensions(Collection allowedFileExtensions) { - setExternalHosts(RandomStartupProperties.allowedFileExtensions, allowedFileExtensions); + setAllowList(RandomStartupProperties.allowedFileExtensions, allowedFileExtensions); FileUtil.clearExtensionChecker(); } diff --git a/core/src/org/labkey/core/admin/AdminController.java b/core/src/org/labkey/core/admin/AdminController.java index d7835a6c098..247b8e8fa0f 100644 --- a/core/src/org/labkey/core/admin/AdminController.java +++ b/core/src/org/labkey/core/admin/AdminController.java @@ -10914,7 +10914,7 @@ public boolean handlePost(AllowListForm form, BindException errors) throws Excep if (form.isDelete()) { String urlToDelete = form.getExistingValue(); - List values = allowListType.getValues(); + List values = new ArrayList<>(allowListType.getValues()); for (String value : values) { if (null != urlToDelete && urlToDelete.trim().equalsIgnoreCase(value.trim())) diff --git a/core/src/org/labkey/core/admin/existingListValues.jsp b/core/src/org/labkey/core/admin/existingListValues.jsp index 57bdfdc37f6..c3a317aa570 100644 --- a/core/src/org/labkey/core/admin/existingListValues.jsp +++ b/core/src/org/labkey/core/admin/existingListValues.jsp @@ -68,7 +68,6 @@ <% AllowListForm bean = (AllowListForm) HttpView.currentModel(); List exitingValues = bean.getExistingValuesList(); - exitingValues.sort(SortHelpers.getNaturalOrderStringComparator()); %> From 3524b5458d60479ad692e35116362483b09438da Mon Sep 17 00:00:00 2001 From: labkey-danield Date: Tue, 10 Jun 2025 09:01:13 -0700 Subject: [PATCH 5/6] Add the new tar.gz file to the expected list of files. --- search/src/org/labkey/search/model/LuceneSearchServiceImpl.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/search/src/org/labkey/search/model/LuceneSearchServiceImpl.java b/search/src/org/labkey/search/model/LuceneSearchServiceImpl.java index 6ccbdbe58be..c3b9e934ad1 100644 --- a/search/src/org/labkey/search/model/LuceneSearchServiceImpl.java +++ b/search/src/org/labkey/search/model/LuceneSearchServiceImpl.java @@ -2035,7 +2035,7 @@ private Map> getExpectations() add(map, "xml_sample.xml", 444, "The Search module offers full-text search of server contents", "The Awesome LabKey Team"); add(map, "zip_sample.zip", 1935, "map a source tsv column", "if there are NO explicit import definitions", "SequenceNum\toriginal_column\toriginal_column_numeric"); add(map, "zip_sample.zip", 1935, "map a source tsv column", "if there are NO explicit import definitions", "SequenceNum\toriginal_column\toriginal_column_numeric"); - + add(map, "targz_sample.tar.gz", 74, "\n\ntargz_sample/._hello.txt\n\n\n\n\n\ntargz_sample/hello.txt"); return map; } From c51277491e61ed82a277230b973d9d2c075e00bd Mon Sep 17 00:00:00 2001 From: labkey-danield Date: Tue, 10 Jun 2025 09:23:46 -0700 Subject: [PATCH 6/6] Simpler strings to search for. --- search/src/org/labkey/search/model/LuceneSearchServiceImpl.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/search/src/org/labkey/search/model/LuceneSearchServiceImpl.java b/search/src/org/labkey/search/model/LuceneSearchServiceImpl.java index c3b9e934ad1..5f2351ac51c 100644 --- a/search/src/org/labkey/search/model/LuceneSearchServiceImpl.java +++ b/search/src/org/labkey/search/model/LuceneSearchServiceImpl.java @@ -2035,7 +2035,7 @@ private Map> getExpectations() add(map, "xml_sample.xml", 444, "The Search module offers full-text search of server contents", "The Awesome LabKey Team"); add(map, "zip_sample.zip", 1935, "map a source tsv column", "if there are NO explicit import definitions", "SequenceNum\toriginal_column\toriginal_column_numeric"); add(map, "zip_sample.zip", 1935, "map a source tsv column", "if there are NO explicit import definitions", "SequenceNum\toriginal_column\toriginal_column_numeric"); - add(map, "targz_sample.tar.gz", 74, "\n\ntargz_sample/._hello.txt\n\n\n\n\n\ntargz_sample/hello.txt"); + add(map, "targz_sample.tar.gz", 74, "targz_sample/._hello.txt", "targz_sample/hello.txt"); return map; }