Change sanitize behavior for files with two dots in filename #14330

tolanych · 2019-02-05T14:23:21Z

What does it do?

For files uploaded via media-manager name with two or more dots will sanitize as when creating file.
UPD:
Change behavior. Now sanitize path before filename and allow filename had two dots. Update related browser/file processors.

Why is it needed?

To avoid the mistake like described in issue (cannot be removed).
And for consistency with behavior creating file.

Related issue(s)/PR(s)

This fixes issue #14320

Jako · 2019-02-06T17:00:38Z

core/model/modx/sources/modfilemediasource.class.php

@@ -873,7 +873,7 @@ public function uploadObjectsToContainer($container,array $objects = array()) {
            }

            $newPath = $this->fileHandler->sanitizePath($file['name']);
-            $newPath = $directory->getPath().$newPath;
+            $newPath = ltrim(strip_tags(preg_replace('/[\.]{2,}/', '', htmlspecialchars($directory->getPath().$newPath))));


Why not move this in sanitizePath?

I have fears to break other uses case fileHandler->sanitizePath.
This method might have been used before in external components and their processors

O.k. Then we maybe have to get the reason why two dots in the filename create that issue.

Some sanitize method on the filename removes too much elsewhere. It just should remove the possibility to ascend in the path and strips those two dots instead of slashes/backslashes surrounded by two dots. But I did not searched for this ...

Now I've realised that my decision (sanitize in method uploadObjectsToContainer) is not clear from the point of consistency.

This behavior I get from processor browser/file/create
This filter process directory path and filename.

In processor browser/file/upload the filter in fact process only directory path, but not filename.

In processor browser/file/remove preparing correct filepath without two dots and as result don't found file.

This had resulted in bug described in issue #14320.

As a solution propose for browser/file processors (create, upload, remove) to remove only two dots from path before filename and allow filename had two dots.
How are you viewing this?

If then it's also allowing to delete a file with two or multiple dots, than I'm alright with this 🚀

Ruslan-Aleev · 2019-02-09T14:11:47Z

Files are renamed when they are loaded, but files with two dots in the name cannot be deleted if they are not loaded via the file manager (for example, via ftp).

Jako · 2019-02-11T17:12:47Z

core/model/modx/processors/browser/file/remove.class.php

+        if (!empty($directory)) {
+            $directory .= DIRECTORY_SEPARATOR;
+        }
+        $name = htmlspecialchars(end(explode( DIRECTORY_SEPARATOR, $file )));


You could use pathinfo for both parts.

I did so the first time.
Pathinfo with PATHINFO_BASENAME and PATHINFO_FILENAME have trouble with filename with unciode symbols and more dots.
So after testing and catch more bugs I replace this on more simple - end->explode for filename.

e.g. http://codepad.org/73SaPqcF, first 4 characters in filename are loses.

pathinfo is locale aware, so you have to set an UTF8 capable locale before:

$oldlocale = setlocale(LC_ALL, 0); setlocale(LC_ALL,'C.UTF-8'); $pathinfo = pathinfo($file); setlocale(LC_ALL,$oldlocale);

You have to check, if 'C.UTF-8' is available.

Jako · 2019-02-11T17:13:08Z

core/model/modx/processors/browser/file/rename.class.php

+        if (!empty($directory)) {
+            $directory .= DIRECTORY_SEPARATOR;
+        }
+        $name = htmlspecialchars(end(explode( DIRECTORY_SEPARATOR, $newFile )));


@Jako,

my last fix with setlocale is not right yet?

Jako · 2019-03-11T12:05:13Z

core/model/modx/processors/browser/file/rename.class.php

-        $name = preg_replace('/[\.]{2,}/', '', htmlspecialchars($name));
-        $success = $this->source->renameObject($oldFile, $name);
+        $oldlocale = setlocale(LC_ALL, 0);
+        setlocale(LC_ALL,'C.UTF-8');


Please check here, wether setlocale is successful. Otherwise an error should be logged, describing possible issues. Maybe the 'C.UTF-8' string should be configurable.

This trouble solving if MODX Setting "locale" is correct encoding for current server environment.
Now I don't run setlocale manually, just log about encoding if function pathinfo return not what was expected

начинающиеся с «#» будут проигнорированы, а пустое сообщение

Ibochkarev · 2019-11-26T02:34:12Z

@Jako Please check the changes.

JoshuaLuckers · 2020-01-22T22:25:04Z

@cla-bot check

cla-bot · 2020-01-22T22:25:07Z

Thank you for your contribution! Before your pull request can be reviewed, please sign the MODX Contributor License Agreement (CLA). This is to make sure MODX has your written permission to distribute projects containing your contributions under the appropriate license.

Please make sure the CLA has been signed for GitHub user(s): @tolanych

Once you've signed, please reply with @cla-bot check to verify and update the status of this pull request.

cla-bot · 2020-01-22T22:25:10Z

The cla-bot has been summoned, and re-checked this pull request!

SnowCreative · 2020-06-10T15:26:18Z

Shouldn't the sanitizer also remove characters like ampersand and pound sign? Those cause problems as well, and shouldn't be allowed in file names. I have clients who upload files like this all the time and then ask me why linking to these files on their websites doesn't work.
See also #13412

tolanych requested review from Mark-H and opengeek as code owners February 5, 2019 14:23

JoshuaLuckers added area-core pr/review-needed Pull request requires review and testing. labels Feb 5, 2019

Jako reviewed Feb 6, 2019

View reviewed changes

Jako added this to the v2.7.1 milestone Feb 6, 2019

Jako added the bug The issue in the code or project, which should be addressed. label Feb 6, 2019

fix impossibility remove file with two dots

6fc582c

tolanych force-pushed the bug-14320 branch from 68a3c70 to 6fc582c Compare February 9, 2019 20:18

tolanych added 2 commits February 10, 2019 01:04

fix remove and for unicode symbols

d6c4cc9

fix rename file with two dots

f58fcf8

tolanych changed the title ~~Add sanitize two dots in filepath for file uploading~~ Change sanitize behavior for files with two dots in filename Feb 9, 2019

Jako reviewed Feb 11, 2019

View reviewed changes

fix locale filepath

aad01d9

alroniks modified the milestones: v2.7.1, v2.7.2 Feb 14, 2019

Mark-H added blocked Active participation around the pull request or issue required. Consensus is not reached. and removed pr/review-needed Pull request requires review and testing. labels Mar 5, 2019

Jako reviewed Mar 11, 2019

View reviewed changes

add log notify about trouble with ENV encoding

bc6261f

начинающиеся с «#» будут проигнорированы, а пустое сообщение

tolanych force-pushed the bug-14320 branch from a670010 to bc6261f Compare September 26, 2019 11:46

opengeek modified the milestones: v2.7.2, v2.7.3 Sep 26, 2019

JoshuaLuckers added pr/review-needed Pull request requires review and testing. and removed blocked Active participation around the pull request or issue required. Consensus is not reached. labels Dec 1, 2019

JoshuaLuckers requested a review from Jako December 1, 2019 11:15

cla-bot bot removed area-core bug The issue in the code or project, which should be addressed. pr/review-needed Pull request requires review and testing. labels Jan 22, 2020

JoshuaLuckers added area-core pr/review-needed Pull request requires review and testing. labels Jan 22, 2020

opengeek modified the milestones: v2.7.3, v2.8.0 Feb 27, 2020

opengeek removed this from the v2.8.0 milestone Oct 8, 2020

smg6511 self-assigned this Sep 27, 2023

rthrash added the bug The issue in the code or project, which should be addressed. label Jan 24, 2024

rthrash added this to the v2.8.7 milestone Jan 24, 2024

opengeek added the cla-signed CLA confirmed for contributors to this PR. label Mar 22, 2024

opengeek modified the milestones: v2.8.7, v2.8.8 Apr 9, 2024

opengeek modified the milestones: v2.8.8, v2.9.0 Nov 5, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change sanitize behavior for files with two dots in filename #14330

Change sanitize behavior for files with two dots in filename #14330

tolanych commented Feb 5, 2019 •

edited

Loading

Jako Feb 6, 2019

tolanych Feb 6, 2019

Jako Feb 7, 2019 •

edited

Loading

tolanych Feb 7, 2019

sebastian-marinescu Feb 8, 2019

Ruslan-Aleev commented Feb 9, 2019

Jako Feb 11, 2019

tolanych Feb 11, 2019

Jako Feb 11, 2019

Jako Feb 11, 2019

Jako Feb 11, 2019

tolanych Mar 11, 2019

Jako Mar 11, 2019 •

edited

Loading

tolanych Sep 26, 2019

Ibochkarev commented Nov 26, 2019

JoshuaLuckers commented Jan 22, 2020

cla-bot bot commented Jan 22, 2020

cla-bot bot commented Jan 22, 2020

SnowCreative commented Jun 10, 2020

Change sanitize behavior for files with two dots in filename #14330

Are you sure you want to change the base?

Change sanitize behavior for files with two dots in filename #14330

Conversation

tolanych commented Feb 5, 2019 • edited Loading

What does it do?

Why is it needed?

Related issue(s)/PR(s)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jako Feb 7, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ruslan-Aleev commented Feb 9, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jako Mar 11, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ibochkarev commented Nov 26, 2019

JoshuaLuckers commented Jan 22, 2020

cla-bot bot commented Jan 22, 2020

cla-bot bot commented Jan 22, 2020

SnowCreative commented Jun 10, 2020

tolanych commented Feb 5, 2019 •

edited

Loading

Jako Feb 7, 2019 •

edited

Loading

Jako Mar 11, 2019 •

edited

Loading