Add unit test

- fix regex quoting
- modified checker methods to be testable

adjusted description text

api/src/org/labkey/api/settings/WriteableAppProps.java

@@ -240,6 +240,6 @@ private void setExternalHosts(RandomStartupProperties propName, @NotNull Collect
     public void setAllowedFileExtensions(Collection<String> allowedFileExtensions)
     {
         setExternalHosts(RandomStartupProperties.allowedFileExtensions, allowedFileExtensions);
-        FileUtil.setExtensionChecker();
+        FileUtil.setExtensionChecker(AppProps.getInstance());
     }
 }

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,13 +76,16 @@
 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;
 import java.util.List;
 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<String> 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<String> 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));
+        }
     }
 }

core/src/org/labkey/core/admin/ExternalServerType.java

@@ -108,7 +108,7 @@ public HtmlString getDescription()
             return HtmlString.unsafe("""
                 <div style="width: 700px">
                     <div>
-                        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.
                     </div>
                     <div>
                     e.g., .tsv, .csv, .tar.gz, .sky.zip, etc.