CR feedback 2

- Move isInvalidFilenameBlocked check
- Subclass Multipart Resolver so we can more easily check the file extension
- fix typos and messaging

api/src/org/labkey/api/premium/AntiVirusProviderRegistry.java

@@ -26,6 +26,6 @@ static void setInstance(AntiVirusProviderRegistry impl)
 
     default StandardServletMultipartResolver getMultipartResolver(ViewBackgroundInfo info)
     {
-        return new StandardServletMultipartResolver();
+        return new DefaultAVMultipartResolver();
     }
-}
+}

api/src/org/labkey/api/premium/DefaultAVMultipartResolver.java

@@ -0,0 +1,43 @@
+package org.labkey.api.premium;
+
+import jakarta.servlet.ServletException;
+import jakarta.servlet.http.HttpServletRequest;
+import jakarta.servlet.http.Part;
+import org.jetbrains.annotations.NotNull;
+import org.labkey.api.util.FileUtil;
+import org.springframework.web.multipart.MultipartException;
+import org.springframework.web.multipart.MultipartHttpServletRequest;
+import org.springframework.web.multipart.support.StandardMultipartHttpServletRequest;
+import org.springframework.web.multipart.support.StandardServletMultipartResolver;
+
+import java.io.IOException;
+
+public class DefaultAVMultipartResolver extends StandardServletMultipartResolver
+{
+    @Override
+    public @NotNull MultipartHttpServletRequest resolveMultipart(HttpServletRequest request) throws MultipartException
+    {
+        try
+        {
+            for (Part part : request.getParts())
+            {
+                // Filter to just file uploads
+                if (part.getSubmittedFileName() != null)
+                {
+                    FileUtil.checkAllowedFileName(part.getSubmittedFileName());
+                    validate(part);
+                }
+            }
+        }
+        catch (IOException | ServletException e)
+        {
+            throw new MultipartException("Couldn't get uploaded files", e);
+        }
+        return new StandardMultipartHttpServletRequest(request, false);
+    }
+
+    protected void validate(Part part)
+    {
+        //do nothing by default, but give subclasses a chance to override
+    }
+}

api/src/org/labkey/api/util/FileUtil.java

@@ -312,22 +312,25 @@ public static void copyDirectory(Path srcPath, Path destPath) throws IOException
 
     public static String isAllowedFileName(String s)
     {
-        if (StringUtils.isBlank(s))
-            return "Filename must not be blank";
-        if (!ViewServlet.validChars(s))
-            return "Filename must contain only valid unicode characters.";
-        if (StringUtils.containsAny(s, restrictedPrintable))
-            return "Filename may not contain any of these characters: " + restrictedPrintable;
-        if (StringUtils.containsAny(s, "\t\n\r"))
-            return "Filename may not contain 'tab', 'new line', or 'return' characters.";
-        if (StringUtils.contains("-$", s.charAt(0)))
-            return "Filename may not begin with any of these characters: -$";
-        if (Pattern.matches("(.*\\s--[^ ].*)|(.*\\s-[^- ].*)",s))
-            return "Filename may not contain space followed by dash.";
+        if (AppProps.getInstance().isInvalidFilenameBlocked())
+        {
+            if (StringUtils.isBlank(s))
+                return "Filename must not be blank";
+            if (!ViewServlet.validChars(s))
+                return "Filename must contain only valid unicode characters.";
+            if (StringUtils.containsAny(s, restrictedPrintable))
+                return "Filename may not contain any of these characters: " + restrictedPrintable;
+            if (StringUtils.containsAny(s, "\t\n\r"))
+                return "Filename may not contain 'tab', 'new line', or 'return' characters.";
+            if (StringUtils.contains("-$", s.charAt(0)))
+                return "Filename may not begin with any of these characters: -$";
+            if (Pattern.matches("(.*\\s--[^ ].*)|(.*\\s-[^- ].*)", s))
+                return "Filename may not contain space followed by dash.";
+        }
 
         String badExtension = checkExtension(s, AppProps.getInstance());
         if (badExtension != null)
-            return "This file type [" + badExtension + "] has been blocked by admins";
+            return "This file type [" + badExtension + "] is not allowed.";
         return null;
     }
 
@@ -1532,7 +1535,7 @@ else if (ch == '-' &&
 
         String result = new String(ret);
 
-        assert !AppProps.getWriteableInstance().isInvalidFilenameBlocked() || isAllowedFileName(result) == null :
+        assert isAllowedFileName(result) == null :
                 "Failed to make filename safe. Original: " + name + ", transformed: " + result + ", error: " + isAllowedFileName(result);
 
         return new String(ret);

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

@@ -120,7 +120,7 @@ public HtmlString getDescription()
             return HtmlString.unsafe("""
                 <div style="width: 700px">
                     <div>
-                        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.
+                        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 allow .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.

core/src/org/labkey/core/webdav/DavController.java

@@ -3074,9 +3074,11 @@ public void closeInputStream() throws IOException
                     return lastModified;
                 }
             };
-            if (FileUtil.isAllowedFileName(name) != null)
+
+            String notAllowedMsg = FileUtil.isAllowedFileName(name);
+            if (StringUtils.isNotBlank(notAllowedMsg))
             {
-                throw new IOException("The file extension is not allowed.");
+                throw new IOException(notAllowedMsg);
             }
             AntiVirusService avs = AntiVirusService.get();
 
@@ -6745,16 +6747,12 @@ public int read(byte @NotNull [] bytes, int i, int i1) throws IOException
 
     void checkAllowedFileName(String s) throws DavException
     {
-        if (!AppProps.getInstance().isInvalidFilenameUploadBlocked())
-            return;
-
         String msg = FileUtil.isAllowedFileName(s);
         if (null == msg)
             return;
         throw new DavException(WebdavStatus.SC_BAD_REQUEST, msg);
     }
 
-
     @TestWhen(TestWhen.When.BVT)
     public static class TestCase extends Assert
     {