From 52cf0f60131be1dd778c5a69a2cd160d9108656f Mon Sep 17 00:00:00 2001 From: ian Date: Mon, 11 Nov 2024 19:10:53 -0800 Subject: [PATCH] Add unit test - fix regex quoting - modified checker methods to be testable adjusted description text --- .../api/settings/WriteableAppProps.java | 2 +- api/src/org/labkey/api/util/FileUtil.java | 75 +++++++++++++++++-- .../labkey/core/admin/ExternalServerType.java | 2 +- 3 files changed, 71 insertions(+), 8 deletions(-) diff --git a/api/src/org/labkey/api/settings/WriteableAppProps.java b/api/src/org/labkey/api/settings/WriteableAppProps.java index ca86df91408..8edd2400e9c 100644 --- a/api/src/org/labkey/api/settings/WriteableAppProps.java +++ b/api/src/org/labkey/api/settings/WriteableAppProps.java @@ -240,6 +240,6 @@ private void setExternalHosts(RandomStartupProperties propName, @NotNull Collect public void setAllowedFileExtensions(Collection allowedFileExtensions) { setExternalHosts(RandomStartupProperties.allowedFileExtensions, allowedFileExtensions); - FileUtil.setExtensionChecker(); + FileUtil.setExtensionChecker(AppProps.getInstance()); } } diff --git a/api/src/org/labkey/api/util/FileUtil.java b/api/src/org/labkey/api/util/FileUtil.java index 11d5ed24e27..6146c098754 100644 --- a/api/src/org/labkey/api/util/FileUtil.java +++ b/api/src/org/labkey/api/util/FileUtil.java @@ -25,6 +25,9 @@ import org.apache.logging.log4j.Logger; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; +import org.jmock.Expectations; +import org.jmock.Mockery; +import org.jmock.lib.legacy.ClassImposteriser; import org.junit.Assert; import org.junit.Test; import org.labkey.api.cloud.CloudStoreService; @@ -73,6 +76,8 @@ import java.security.MessageDigest; import java.security.NoSuchAlgorithmException; import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collections; import java.util.Comparator; import java.util.HashSet; import java.util.LinkedList; @@ -80,6 +85,7 @@ import java.util.Objects; import java.util.Set; import java.util.regex.Pattern; +import java.util.stream.Collectors; import java.util.stream.Stream; public class FileUtil @@ -319,29 +325,29 @@ public static String isAllowedFileName(String s) if (Pattern.matches("(.*\\s--[^ ].*)|(.*\\s-[^- ].*)",s)) return "Filename may not contain space followed by dash."; - String badExtension = checkExtension(s); + String badExtension = checkExtension(s, AppProps.getInstance()); if (badExtension != null) return "This file type [" + badExtension + "] has been blocked by admins"; return null; } - private static String checkExtension(String filename) + private static String checkExtension(String filename, AppProps appProps) { // If the allow list is empty, allow any extension - if (AppProps.getInstance().getAllowedExtensions().isEmpty()) + if (appProps.getAllowedExtensions().isEmpty()) return null; if (extensionChecker == null) - setExtensionChecker(); + setExtensionChecker(appProps); String extension = FilenameUtils.getExtension(filename); return extensionChecker.matcher(filename).matches() ? null : extension; } - public static void setExtensionChecker() + public static void setExtensionChecker(AppProps appProps) { // Regex encode the allowed extensions (escape periods and add '|' optional matcher) - String allowedExtensions = String.join("|", AppProps.getInstance().getAllowedExtensions()).replace(".", "\\."); + String allowedExtensions = appProps.getAllowedExtensions().stream().map(Pattern::quote).collect(Collectors.joining("|")); // Allow any extension in the list unless it is preceeded by a '.' which we use as a proxy for double/multi extensions extensionChecker = Pattern.compile(String.format("^[^\\.]*(%1$s)$", allowedExtensions), Pattern.CASE_INSENSITIVE); } @@ -2247,5 +2253,62 @@ public void testAllowedFileName() assertNotNull(isAllowedFileName("-ab")); assertNotNull(isAllowedFileName("a`b")); } + + @Test + public void testAcceptableExtensions() + { + List allowedExtensions = Arrays.asList( + ".1", + ".txt", + ".tar", + ".tar.gz", + ".a_v", + ".xlsx", + ".l-()[]{}1☃"); + + //Test Setup + Mockery _context; + _context = new Mockery(); + _context.setImposteriser(ClassImposteriser.INSTANCE); + AppProps mockProps = _context.mock(AppProps.class); + _context.checking(new Expectations(){{ + allowing(mockProps).getAllowedExtensions(); + will(returnValue(allowedExtensions)); + }}); + + + assertNull("Extension should be allowed", checkExtension("test.txt", mockProps)); + assertNull("Multiple extension should be allowed", checkExtension("archive.tar.gz", mockProps)); + assertNull("Case-insensitive extension should be allowed", checkExtension("archive.TaR.Gz", mockProps)); + assertNull("Special characters aren't escaped properly", checkExtension("my test.l-()[]{}1☃", mockProps)); + assertNull("Numeric extension should be allowed", checkExtension("test.1", mockProps)); + assertNotNull("Multiple extension matched when it shouldn't", checkExtension("tar.gz", mockProps)); + assertNotNull("Matched unlist extension", checkExtension("my test.notListed", mockProps)); + assertNotNull("Combined multiple extension matched incorrectly", checkExtension("multi.a_v.tar", mockProps)); + assertNotNull("Multi-multi extension matched unexpectedly", checkExtension("multi.not.tar.gz", mockProps)); + assertNotNull("No extension matched unexpectedly", checkExtension("No extension", mockProps)); + } + + @Test + public void testNoAcceptableExtensions() + { + List allowedExtensions = Collections.emptyList(); + + //Test Setup + Mockery _context; + _context = new Mockery(); + _context.setImposteriser(ClassImposteriser.INSTANCE); + AppProps mockProps = _context.mock(AppProps.class); + _context.checking(new Expectations(){{ + allowing(mockProps).getAllowedExtensions(); + will(returnValue(allowedExtensions)); + }}); + + assertNull("Special characters aren't escaped properly", checkExtension("my test.l-()[]{}1☃", mockProps)); + assertNull("Unlisted extension should be allowed, but wasn't", checkExtension("my test.notListed", mockProps)); + assertNull("Combined extension should be allowed, but wasn't", checkExtension("multi.tar.a_v", mockProps)); + assertNull("No extension should be allowed, but wasn't", checkExtension("No extension", mockProps)); + assertNull("Numeric extension should be allowed", checkExtension("test.1", mockProps)); + } } } diff --git a/core/src/org/labkey/core/admin/ExternalServerType.java b/core/src/org/labkey/core/admin/ExternalServerType.java index 1192f538097..7c874902b02 100644 --- a/core/src/org/labkey/core/admin/ExternalServerType.java +++ b/core/src/org/labkey/core/admin/ExternalServerType.java @@ -108,7 +108,7 @@ public HtmlString getDescription() return HtmlString.unsafe("""
- This list is the set of file extensions that LabKey will allow through. Any extension that is not in this list will be rejected, this includes mulitple extensions. If the list is blank, then this check is ignored. + This list is the set of file extensions that LabKey will accept for uploads. Any extension that is not in this list will be rejected, this includes multiple extensions. For example, .gz is not sufficient to all .tar.gz; you must specify .tar.gz. If the list is empty, then this check is ignored.
e.g., .tsv, .csv, .tar.gz, .sky.zip, etc.