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

[mridulpathak]

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 <mridul...@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;