(ofbiz-framework) branch release24.09 updated: Improved: Check path containment before invoking SecuredUpload.isValidFileName in isValidProductImageFileName (#1685)

[mr...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

mridulpathak pushed a commit to branch release24.09
in repository https://gitbox.apache.org/repos/asf/ofbiz-framework.git


The following commit(s) were added to refs/heads/release24.09 by this push:
     new 44fccc4975 Improved: Check path containment before invoking SecuredUpload.isValidFileName in isValidProductImageFileName (#1685)
44fccc4975 is described below

commit 44fccc497526123d592261905bb0d13e92cd5d2b
Author: Mridul Pathak <mr...@gmail.com>
AuthorDate: Wed Aug 19 21:12:43 2026 +0530

    Improved: Check path containment before invoking SecuredUpload.isValidFileName in isValidProductImageFileName (#1685)
    
    Backported from trunk (#1684).
---
 .../product/imagemanagement/ImageManagementServices.java | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/applications/product/src/main/java/org/apache/ofbiz/product/imagemanagement/ImageManagementServices.java b/applications/product/src/main/java/org/apache/ofbiz/product/imagemanagement/ImageManagementServices.java
index b4ae9875b5..c73cc60a4c 100644
--- a/applications/product/src/main/java/org/apache/ofbiz/product/imagemanagement/ImageManagementServices.java
+++ b/applications/product/src/main/java/org/apache/ofbiz/product/imagemanagement/ImageManagementServices.java
@@ -75,21 +75,25 @@ public class ImageManagementServices {
     private static String imagePath;
 
     /**
-     * Ensures a user-supplied image file name is safe to use as a rename target: it must pass
+     * Ensures a user-supplied image file name is safe to use as a rename target: it must resolve,
+     * once normalized, directly inside the given product image directory, and must pass
      * {@link org.apache.ofbiz.security.SecuredUpload#isValidFileName} (no path separators, no
-     * traversal sequences, no denied extensions) and must resolve, once normalized, directly
-     * inside the given product image directory.
+     * traversal sequences, no denied extensions). The containment check runs first and only calls
+     * into {@code isValidFileName} once containment is confirmed, because that helper is not
+     * side-effect free: for a denied extension it deletes the file at the supplied path, and an
+     * absolute or traversal path checked before containment would let that delete escape the
+     * product directory entirely.
      */
     static boolean isValidProductImageFileName(String fileName, Path resolvedProductDir, Delegator delegator) {
         if (UtilValidate.isEmpty(fileName)) {
             return false;
         }
         try {
-            if (!org.apache.ofbiz.security.SecuredUpload.isValidFileName(fileName, delegator)) {
+            Path resolvedFilePath = resolvedProductDir.resolve(fileName).normalize();
+            if (resolvedFilePath.getParent() == null || !resolvedFilePath.getParent().equals(resolvedProductDir)) {
                 return false;
             }
-            Path resolvedFilePath = resolvedProductDir.resolve(fileName).normalize();
-            return resolvedFilePath.getParent() != null && resolvedFilePath.getParent().equals(resolvedProductDir);
+            return org.apache.ofbiz.security.SecuredUpload.isValidFileName(fileName, delegator);
         } catch (IOException | InvalidPathException e) {
             Debug.logError(e, MODULE);
             return false;