diff --git a/api/src/org/labkey/api/settings/RandomStartupProperties.java b/api/src/org/labkey/api/settings/RandomStartupProperties.java index 0363a54c538..191a485f65a 100644 --- a/api/src/org/labkey/api/settings/RandomStartupProperties.java +++ b/api/src/org/labkey/api/settings/RandomStartupProperties.java @@ -3,6 +3,7 @@ import org.apache.commons.lang3.StringUtils; import org.apache.logging.log4j.Logger; import org.labkey.api.files.FileContentService; +import org.labkey.api.util.FileUtil; import org.labkey.api.util.SafeToRenderEnum; import org.labkey.api.util.logging.LogHelper; @@ -34,6 +35,7 @@ public void setValue(WriteableAppProps writeable, String value) public void setValue(WriteableAppProps writeable, String value) { writeable.setAllowedFileExtensions(Arrays.asList(StringUtils.split(value, AppPropsImpl.EXTERNAL_HOST_DELIMITER))); + FileUtil.clearExtensionChecker(); // Not sure this is needed, but better safe than sorry. } }, fileUploadDisabled("Disable file upload") diff --git a/api/src/org/labkey/api/settings/WriteableAppProps.java b/api/src/org/labkey/api/settings/WriteableAppProps.java index aba9b5503bf..65bc81ba3b2 100644 --- a/api/src/org/labkey/api/settings/WriteableAppProps.java +++ b/api/src/org/labkey/api/settings/WriteableAppProps.java @@ -240,7 +240,7 @@ private void setExternalHosts(RandomStartupProperties propName, @NotNull Collect public void setAllowedFileExtensions(Collection allowedFileExtensions) { setExternalHosts(RandomStartupProperties.allowedFileExtensions, allowedFileExtensions); - FileUtil.setExtensionChecker(AppProps.getInstance()); + FileUtil.clearExtensionChecker(); } public void setAllowedExternalResourceHosts(String jsonArray) diff --git a/api/src/org/labkey/api/util/FileUtil.java b/api/src/org/labkey/api/util/FileUtil.java index 9238744a7f9..8446e0c3535 100644 --- a/api/src/org/labkey/api/util/FileUtil.java +++ b/api/src/org/labkey/api/util/FileUtil.java @@ -349,7 +349,7 @@ private static String checkExtension(String filename, AppProps appProps) return extensionChecker.matcher(filename).matches() ? null : extension; } - public static void setExtensionChecker(AppProps appProps) + private static void setExtensionChecker(AppProps appProps) { // Regex encode the allowed extensions (escape periods and add '|' optional matcher) String allowedExtensions = appProps.getAllowedExtensions().stream().map(Pattern::quote).collect(Collectors.joining("|")); @@ -357,6 +357,11 @@ public static void setExtensionChecker(AppProps appProps) extensionChecker = Pattern.compile(String.format("^[^\\.]*(%1$s)$", allowedExtensions), Pattern.CASE_INSENSITIVE); } + public static void clearExtensionChecker() + { + extensionChecker = null; + } + public static void checkAllowedFileName(String s, boolean checkFileExtension) throws IOException { String msg = isAllowedFileName(s, checkFileExtension); diff --git a/core/src/org/labkey/core/CoreModule.java b/core/src/org/labkey/core/CoreModule.java index f47bfafda84..6a518a3ce00 100644 --- a/core/src/org/labkey/core/CoreModule.java +++ b/core/src/org/labkey/core/CoreModule.java @@ -207,6 +207,7 @@ import org.labkey.core.admin.ActionsTsvWriter; import org.labkey.core.admin.AdminConsoleServiceImpl; import org.labkey.core.admin.AdminController; +import org.labkey.core.admin.AllowListType; import org.labkey.core.admin.CopyFileRootPipelineJob; import org.labkey.core.admin.CustomizeMenuForm; import org.labkey.core.admin.DisplayFormatAnalyzer; @@ -1358,6 +1359,7 @@ public Set getIntegrationTests() AdminController.SerializationTest.class, AdminController.TestCase.class, AdminController.WorkbookDeleteTestCase.class, + AllowListType.TestCase.class, AttachmentServiceImpl.TestCase.class, CoreController.TestCase.class, DataRegion.TestCase.class, diff --git a/core/src/org/labkey/core/admin/AllowListType.java b/core/src/org/labkey/core/admin/AllowListType.java index 1d122f7d2d1..6dc3be50834 100644 --- a/core/src/org/labkey/core/admin/AllowListType.java +++ b/core/src/org/labkey/core/admin/AllowListType.java @@ -3,16 +3,25 @@ import com.fasterxml.jackson.annotation.JsonIgnore; import org.apache.commons.lang3.StringUtils; import org.apache.commons.validator.routines.UrlValidator; +import org.junit.Assert; +import org.junit.Assume; +import org.junit.BeforeClass; +import org.junit.Test; import org.labkey.api.action.LabKeyError; import org.labkey.api.data.Container; import org.labkey.api.security.User; import org.labkey.api.settings.AppProps; import org.labkey.api.settings.WriteableAppProps; +import org.labkey.api.test.TestWhen; +import org.labkey.api.util.FileUtil; import org.labkey.api.util.HtmlString; import org.labkey.api.util.URLHelper; import org.labkey.api.view.ActionURL; import org.springframework.validation.BindException; +import java.io.IOException; +import java.util.ArrayList; +import java.util.Arrays; import java.util.Collection; import java.util.List; @@ -109,6 +118,7 @@ public void setValues(Collection allowedExtensions, User user) WriteableAppProps props = AppProps.getWriteableInstance(); props.setAllowedFileExtensions(allowedExtensions); props.save(user); + FileUtil.clearExtensionChecker(); // Should be redundant, but going to leave this here } @Override @@ -150,4 +160,42 @@ public URLHelper getSuccessURL(Container container) { return new ActionURL(AdminController.AllowListAction.class, container).addParameter("type", name()); } + + public static class TestCase extends Assert + { + @Test + public void testAllowedExtensions() + { + List existing = FileExtension.getValues(); + Assume.assumeTrue("Initial allowed extensions list should be empty to prevent overriding existing values", existing.isEmpty()); + + try + { + List newValues = Arrays.asList(".tar.gz", ".bar"); + FileExtension.setValues(newValues, User.getAdminServiceUser()); + + try + { + FileUtil.checkAllowedFileName("test.tar.gz", true); + FileUtil.checkAllowedFileName("foo.bar", true); + } + catch (IOException e) + { + fail("Filename should've been accepted: " + e.getMessage()); + } + + Assert.assertThrows("We dont allow 'extra' extensions", IOException.class, () -> FileUtil.checkAllowedFileName("test.foo.tar.gz", true)); + Assert.assertThrows("We dont allow partial extensions, first segment", IOException.class, () -> FileUtil.checkAllowedFileName("test.tar", true)); + Assert.assertThrows("We dont allow partial extensions, second segment", IOException.class, () -> FileUtil.checkAllowedFileName("test.gz", true)); + Assert.assertThrows("We dont allow files with no extensions", IOException.class, () -> FileUtil.checkAllowedFileName("test", true)); + } + finally + { + // Verify values were restored to original state. + FileExtension.setValues(existing, User.getAdminServiceUser()); + List current = FileExtension.getValues(); + assertEquals(existing, current); + } + } + } }