magento / magento2

Prior to making any Submission(s), you must sign an Adobe Contributor License Agreement, available here at: https://opensource.adobe.com/cla.html. All Submissions you make to Adobe Inc. and its affiliates, assigns and subsidiaries (collectively “Adobe”) are subject to the terms of the Adobe Contributor License Agreement.
http://www.magento.com
Open Software License 3.0
11.44k stars 9.29k forks source link

245-p6 - unable to view media folders and files when a file is present in any subdirectory of pub/media with a space in the filename #38463

Closed pixiemediaweb closed 5 months ago

pixiemediaweb commented 6 months ago

Preconditions and environment

Steps to reproduce

Login to Admin Go to a CMS page or block Click 'insert image' If you have any file in the pub/media/* location (even nested folders) that contains a space in the filename, you will not be able to see folder and files in the insert image popup.

Expected result

Should be able to see media folders and image assets

Actual result

No folders or image assets

Additional information

The issue is due to having any file in the /pub/media/ location (and below) that contains a space in the filename. You can make it reveal problematic files by running;

bin/magento media-gallery:sync

Though this will only tell you one file issue at a time. Once you find and update the reported filenames, the gallery works again as expected, though in most cases this is unrealistic for a merchant to do themselves.

Release note

No response

Triage and priority

m2-assistant[bot] commented 6 months ago

Hi @pixiemediaweb. Thank you for your report. To speed up processing of this issue, make sure that the issue is reproducible on the vanilla Magento instance following Steps to reproduce. To deploy vanilla Magento instance on our environment, Add a comment to the issue:

m2-assistant[bot] commented 6 months ago

Hi @engcom-Bravo. Thank you for working on this issue. In order to make sure that issue has enough information and ready for development, please read and check the following instruction: :point_down:

engcom-Bravo commented 6 months ago

Hi @pixiemediaweb,

Thank you for reporting and collaboration.

Verified the issue on Magento 2.4-develop instance and the issue is not reproducible.Kindly refer the screenshots.

Steps to reproduce

Screenshot from 2024-02-23 12-48-38

Screenshot from 2024-02-23 12-49-20

We are able to get the folders from pub/media/* location.

Kindly check the issue in Magento 2.4-develop instance and elaborate the steps to reproduce if the issue is still reproducible.

Thanks.

pixiemediaweb commented 6 months ago

Hi @engcom-Bravo - you need to upload a file in the /pub/media/ directory that has a space in the filename. This needs to be a manual upload as the wywisyg will automatically rename it if you upload from there.

Here is an example of the output you get from bin/magento media-gallery:sync when there is a file with a space in it;

Path "/var/www/dev/pub/media/my image name.jpg" cannot be used with directory "/var/www/dev/pub/media/"

Thanks

engcom-Bravo commented 5 months ago

Hi @pixiemediaweb,

Thanks for your update.

Verified the issue on Magento 2.4-develop instance and the issue is not reproducible.Kindly refer the screenshots.

Screenshot from 2024-02-26 12-50-45

Screenshot from 2024-02-26 12-51-02

We have uploaded a file in the /pub/media/ directory that has a space in the filename and the command bin/magento media-gallery:sync completed successfully.

Kindly check the issue in Magento 2.4-develop instance and elaborate the steps to reproduce if the issue is still reproducible.

Thanks.

jaminion commented 5 months ago

Hello @engcom-Bravo and @pixiemediaweb ,

I'm having a similar problem with a file that our custom code generates under var/export for some of our custom reports, as we have the file set up to be named like " - - .xlsx", the spaces around hyphens are triggering it. It's coming from the following preg_match() call which was added to the frontend of an if check in the validate() method in the magento/framework's Filesystem/Directory/PathValidator.php:

        if (preg_match('/(?:^-|\s-)/', $path)
            || (
                mb_strpos($actualPath, $realDirectoryPath) !== 0
                && rtrim($path, DIRECTORY_SEPARATOR) !== $realDirectoryPath
            )
        ) {

It's the \s- portion of the preg_match check that is triggering it. This also prevents filenames which start with a hyphen.

Note that the current version of this magento/framework:Filesystem/Directory/PathValidator.php file in the available source code for the 2.4-develop branch does not have this preg_match() function in it. This preg_match() check was not included in the previous release, 2.4.5-p5, but it is showing in the 2.4.5-p6 tagged version: https://github.com/magento/magento2/blob/2.4.5-p6/lib/internal/Magento/Framework/Filesystem/Directory/PathValidator.php. The 2.4.7-beta2 version of the file also does not have it included: https://github.com/magento/magento2/blob/2.4.7-beta2/lib/internal/Magento/Framework/Filesystem/Directory/PathValidator.php, but I guess that's from back in October and we can probably expect the 2.4.7-beta3 coming in a couple of weeks to include it.

It feels like this code is intended as helping to fix CVE-2024-20720 (Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection')), and they are not otherwise referenced in the source, I've been spinning wheels trying to figure out how it got injected into my composer-based sites and it turns out it was packaged that way by Magento.

maderlock commented 5 months ago

I think that 2.4-develop does have this in? https://github.com/magento/magento2/blob/a9f88e2428b72e930865eaf57b58b62b3c2379c7/lib/internal/Magento/Framework/Filesystem/Directory/PathValidator.php#L57

I'm seeing the same issue that has appeared on upgrading 2.4.5-p5 to 2.4.5-p6, though we have files with " -" in them, not just space, so have not confirmed the narrower issue.

The effect we're seeing is that file upload does not even show directories: image (27) I can understand that import fails. That seems to be what this validation should be doing: image (28)

jaminion commented 5 months ago

It looks like it was added in a PR yesterday: https://github.com/magento/magento2/commit/a9f88e2428b72e930865eaf57b58b62b3c2379c7#diff-0a86f23a4c8b01db4b820276a15815173b4aa97e8a2f272b9522015150b32551

Prepping for the 2.4.7-beta3 release in a couple weeks, I expect - "Sync 2.4 develop with 2.4.7 beta3 develop".

I have been back and forth on how to handle this but have ended up just changing file names. I'm not in a position where I can argue with an auditor over the exploitability of a CVE if I was to try and adjust that back through a composer patch, and I'm not certain it's tied to that CVE - just my best guess. Would probably be best if this new restriction of file names was mentioned as known issues in the release notes.

I have been using find pub/media -name '* \-*' -print to look for files which would trigger it and working through adjustments to those.

nathanjosiah commented 5 months ago

I am aware of this issue and having my team take a look.

nathanjosiah commented 5 months ago

It is intended that files with certain combinations of dashes and or spaces is not allowed. The case that we are trying to understand is from @pixiemediaweb above which shows the error

Path "/var/www/dev/pub/media/my image name.jpg" cannot be used with directory "/var/www/dev/pub/media/"

We are unable to reproduce a case where only spaces is triggering the error. Can anybody else provide reproduction steps for this specific issue where there are no dashes but you are still getting an error?

nathanjosiah commented 5 months ago

Given the lack of response and proof of the use case explained above I'm going to close this ticket. We are going to fix the use case where dashes are not allowed when on their own. For example My File - Version 1.jpg will be allowed with our fix.

Other combinations are not allowed by design. If anybody is seeing the issue that @pixiemediaweb reported where spaces by themselves are not allowed (e.g. My File.jpg) please let me know and we can investigate further.

ashraf-ktpl commented 3 months ago

@nathanjosiah @pixiemediaweb @engcom-Bravo

I have encountered this same issue. after Magento Upgrade from 2.4.6-p3 to 2.4.6-p5

Error : Path "amasty/amfile/attach/custom/upload/Installation Instructions - Rintal Zink Exterior Spiral Stairs.pdf" cannot be used with directory "/home/magento/pub/media/" Exception in /home/magento/vendor/magento/framework/Filesystem/Directory/PathValidator.php:63

Change in PathValidator.php -- if (mb_strpos($actualPath, $realDirectoryPath) !== 0 -- && rtrim($path, DIRECTORY_SEPARATOR) !== $realDirectoryPath

++ if (preg_match('/(?:^-|\s-)/', $path) ++ || ( ++ mb_strpos($actualPath, $realDirectoryPath) !== 0 ++ && rtrim($path, DIRECTORY_SEPARATOR) !== $realDirectoryPath

Please check

hostep commented 2 months ago

It appears that this problem was fixed in Magento 2.4.7-p1, 2.4.6-p6, 2.4.5-p8 and 2.4.4-p9

Maybe @nathanjosiah can confirm this?

PieterCappelle commented 1 month ago

I'm having the same issue with the import of a Product with an image at the following path var/import/images/MSII/MS II -B96+.png. This is a valid filename, but I'm having the same error.

PieterCappelle commented 1 month ago

I have created a patch:

diff --git a/Filesystem/Directory/PathValidator.php b/Filesystem/Directory/PathValidator.php
index 342f399..2e76c27 100644
--- a/Filesystem/Directory/PathValidator.php
+++ b/Filesystem/Directory/PathValidator.php
@@ -54,11 +54,9 @@ class PathValidator implements PathValidatorInterface
             $actualPath = $this->driver->getRealPathSafety($path);
         }

-        if (preg_match('/(?:^-|\s-)/', $path)
-            || (
-                mb_strpos($actualPath, $realDirectoryPath) !== 0
-                && rtrim($path, DIRECTORY_SEPARATOR) !== $realDirectoryPath
-            )
+        if (
+            mb_strpos($actualPath, $realDirectoryPath) !== 0
+            && rtrim($path, DIRECTORY_SEPARATOR) !== $realDirectoryPath
         ) {
             throw new ValidatorException(
                 new Phrase(

Add it to composer.json

    "extra": {
        "magento-force": "override",
        "composer-exit-on-patch-failure": true,
        "patches": {
            "magento/framework": {
                "Fix PathValidator, ignore regex": "patches/composer/github-issue-38463.diff"
            }
        }
    }