Added a check for the existence of a file when uploading it #15232

Ruslan-Aleev · 2020-09-11T16:38:21Z

What does it do?

Added a check for the existence of a file when uploading it.
In the current version, if the file names match, then the file will be overwritten.

This PR fixes that:

p.s. For Amazon source, we can probably add a file existence check too, in line
https://github.com/modxcms/revolution/blob/2.x/core/model/modx/sources/mods3mediasource.class.php#L823

add

if (file_exists($container.$file['name'])) {
    $this->addError('path',sprintf($this->xpdo->lexicon('file_err_ae'),$file['name']));
    continue;
}

but I cannot test it as I don't use sources from Amazon.

Related issue(s)/PR(s)

#8371
#14641

Ruslan-Aleev · 2020-09-11T16:45:49Z

It was only after I submitted the PR that I noticed that this had already been fixed but not merged - 6b4cf38 ¯_(ツ)_/¯
Should I redo my PR?

Jako · 2020-09-14T07:18:42Z

The code of @webinmd checks the renaming of files too. Thats better. But the name of the new method is somehow strange.

Ruslan-Aleev · 2020-09-14T07:57:07Z

@Jako I redid the PR, check when you have time.

matdave · 2020-09-15T16:50:50Z

Would it make sense to have a system (or media source) setting to allow the original overwrite behavior? I personally use the overwrite method quite a bit to replace files, so this would add an extra step.

Ruslan-Aleev · 2020-09-15T16:58:24Z

I added a check for the file because the same check already exists when renaming the file or category. And it is strange that it was not there when the file was uploaded. Replacing a file without alerting - the behavior is not normal.
Probably, in the future, we can add settings of this kind, but it seems to me that this is unnecessary, although you use this :)

Ruslan-Aleev · 2020-09-15T17:05:44Z

There were other suggestions for fileloader behavior - #14566
Perhaps in the future it is worth considering all the wishes and expanding the functionality, but, again, I think this is superfluous.

matdave · 2020-09-15T17:09:32Z

I guess this is an improvement over silently overwriting. Would be ideal to later add a checkbox or setting to allow the previous behavior, but I can see how it could be perceived as a bug (instead of a feature, hah).

JoshuaLuckers · 2020-09-17T09:21:39Z

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

+     * @param string $objectName The object name displayed in the error message
+     * @return bool
+     */
+    public function checkObjectExist($objectPath, $objectName) {


If I see a method like this I expect it to return true if the object already exists.

I agree with Joshua, this should definitely return true if the object exists.

I don't see the issue: It returns true, if the file or the path exists.

That's because @Ruslan-Aleev force-pushed his changes after my comment 😋

theboxer · 2020-09-17T09:33:34Z

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

+     * @param string $objectName The object name displayed in the error message
+     * @return bool
+     */
+    public function checkObjectExist($objectPath, $objectName) {


I agree with Joshua, this should definitely return true if the object exists.

theboxer · 2020-09-17T09:34:46Z

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

-                return false;
-            }
-            $this->addError('name',sprintf($this->xpdo->lexicon('file_err_ae'),$newName));
+        if (!$this->checkObjectExist($newPath,$newName)) {


After the you adjust the checkObjectExist to return true if the object exists, this will need to change as well - removing the !.

theboxer · 2020-09-17T09:34:54Z

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

@@ -875,6 +888,10 @@ public function uploadObjectsToContainer($container,array $objects = array()) {
            $newPath = $this->fileHandler->sanitizePath($file['name']);
            $newPath = $directory->getPath().$newPath;

+            if (!$this->checkObjectExist($newPath,$file['name'])) {


Same as above.

Ruslan-Aleev · 2020-09-17T09:48:22Z

@JoshuaLuckers @theboxer I redid PR, please check.

opengeek

I'm concerned that this is an incomplete solution to the problem since it only resolves the bug if working with one of many potential media source implementations. But in order to resolve it appropriately, we would need to add this new method to the interface and expect all implementations to be updated to implement this method. Then we would find ourselves with a change that breaks backwards compatibility. I think this needs to be discussed and considered.

opengeek · 2020-09-17T18:56:54Z

To clarify, I have two concerns here. One is that if this is just changing the behavior of a single media source, the extracted method here should not be public in scope. The second is regarding the consistency of behavior across all core and non-core media sources. If the default file media source does not automatically overwrite existing files in this UX, then shouldn't this be consistent for all media sources?

Jako · 2020-09-17T19:13:29Z

@opengeek All your concerns are valid, but they can't be solved with 2.x. How many different media source implementations do currently exist? The S3 media source implementation in the core is outdated and does not work anymore (at least in Europe).

Ruslan-Aleev · 2020-09-17T19:23:58Z

It improves the situation anyway, doesn't it?
For example, if other types of file sources use core/model/modx/sources/modfilemediasource.class.php in their base, then there are no problems and the file is not rewritten silently, and if these types of sources have their own logic, then it is probably already implemented there check of this kind (for example, miniShop2 has its own check). And if there is no check, then the situation for such sources does not change.

opengeek

Make this a protected method and I'll merge it then.

Ruslan-Aleev · 2020-09-17T20:45:40Z

@opengeek I redid PR

SnowCreative · 2020-09-29T00:11:02Z

I'm not sure what's happening here. Are you changing the behavior so that overwriting existing files is impossible? If so, I don't think we should change this behavior until it can be done properly, offering an option to replace or rename files on uploading. All my clients are used to updating existing files (like PDFs) with new versions without renaming so that links to those files throughout their sites will not get broken.

opengeek · 2020-09-29T00:20:35Z

I'm not sure what's happening here. Are you changing the behavior so that overwriting existing files is impossible? If so, I don't think we should change this behavior until it can be done properly, offering an option to replace or rename files on uploading. All my clients are used to updating existing files (like PDFs) with new versions without renaming so that links to those files throughout their sites will not get broken.

This is certainly a valid concern if there is currently no way to replace the files.

Ruslan-Aleev · 2020-09-29T08:21:24Z

All my clients are used to updating existing files (like PDFs) with new versions without renaming so that links to those files throughout their sites will not get broken.

Many (if not all) systems indicate this when uploading existing files rather than overwrite them. By the way, clients point to this too.
This behavior is better for UX than accidentally overwriting an important file. In your case, clients can always delete and download the file again, although this requires more steps.

Ruslan-Aleev · 2020-09-29T08:27:05Z

Other options have already been discussed #14566, but many users chose the warning option, if the file exist.
By the way, if we do some kind of setting, then we need to remember about renaming the file, and not just replacing it when uploading.

opengeek · 2020-09-29T13:57:53Z

If we are changing behavior that existing clients are used to, then we need to account for that. #14566 was discussed for 3.x because it is a significant behavior change. In either case, IMO, this really needs to have an option to warn users before overwriting but allow them to be overwritten if they choose.

SnowCreative · 2020-10-07T15:18:39Z

I see this was pushed through anyway and incorporated into the new 2.8 update. This is disastrous for all my clients, and I will NOT be upgrading until this is corrected properly. As opengeek said, we need to have an option to overwrite if desired, not just block it entirely.

Mark-H · 2020-10-07T15:41:39Z

A PR was merged after discussion, and points raised after that merge did not lead to anyone providing a better pull request that matches the expectation of certain users before the release. Nothing's being "pushed through" - there has simply not been a better PR for review before the release. A full revert would not make it better overall.

Users that want to overwrite existing files will first need to delete them explicitly rather than that happening magically and without any form of user feedback or interaction. It's not impossible to do so.

I certainly understand some of your users may be accustomed to this and would welcome additional improvements that make it an optional checkbox of sorts in the upload window. Or a system setting that toggles it, perhaps, as proposed earlier in the thread.

Ruslan-Aleev · 2020-10-07T15:43:45Z

@Rainbowtiger

I see this was pushed through anyway...

Here you are substituting concepts. This PR was merged 7 days before you wrote your comment (look at the dates).

This is disastrous for all my clients...

Is it really for all, and is it really a disaster? Clients will make more clicks, yes, but no more.

I will NOT be upgrading until this is corrected properly.

You decide. But this now works properly. The fact that it was possible to overwrite the file without informing the user is just incorrect behavior and it has been fixed.
You can make a PR for the 2.x or 3.x branch based on #14566 and create a system setting, and wrap the file check condition.

Mark-H · 2020-10-07T15:49:47Z

@Rainbowtiger How about this: if you plant some trees (let's say 100) at https://ecologi.com/modmore I'll prepare a pull request today to add a setting to disable the overwrite behavior and do what I can to get that into 2.8.1 for you. 🌴

Ruslan-Aleev · 2020-10-07T15:53:58Z

@Mark-H if you are going to do PR, then I would leave 3 options for setting:

Check existing files (used by default);
Overwrite files;
Create copies like in Generate new file if file already exists #14566.

And the setting should also affect renaming existing files.

rthrash · 2020-10-07T16:04:58Z

This pull request has been mentioned on MODX Community. There might be relevant details there:

https://community.modx.com/t/version-2-8-is-out-i-wont-be-upgrading/3166/2

SnowCreative · 2020-10-07T16:45:44Z

Here you are substituting concepts. This PR was merged 7 days before you wrote your comment (look at the dates).

I'm not sure what you mean by "substituting concepts".

This is disastrous for all my clients...

Is it really for all, and is it really a disaster? Clients will make more clicks, yes, but no more.

Yes, it is. Most of my clients are NOT very tech-savvy, so anything creating difficulties for them is not good.

I will NOT be upgrading until this is corrected properly.

You decide. But this now works properly. The fact that it was possible to overwrite the file without informing the user is just incorrect behavior and it has been fixed.

I disagree. "Properly" would be having the option to overwrite, just like all file systems do. This change is only half of a "proper" fix. No doubt, it's better for some people. My point is that it's not better for ALL people. It gets rid of one deficiency, but creates another.

You can make a PR for the 2.x or 3.x branch based on #14566 and create a system setting, and wrap the file check condition.

Unfortunately, I'm not enough of a programmer to create pull requests.

SnowCreative · 2020-10-07T16:59:51Z

@Rainbowtiger How about this: if you plant some trees (let's say 100) at https://ecologi.com/modmore I'll prepare a pull request today to add a setting to disable the overwrite behavior and do what I can to get that into 2.8.1 for you. 🌴

Great idea! 100 trees have been purchased! ([email protected])

SnowCreative · 2020-10-07T17:40:21Z

A PR was merged after discussion, and points raised after that merge did not lead to anyone providing a better pull request that matches the expectation of certain users before the release. Nothing's being "pushed through" - there has simply not been a better PR for review before the release. A full revert would not make it better overall.

Users that want to overwrite existing files will first need to delete them explicitly rather than that happening magically and without any form of user feedback or interaction. It's not impossible to do so.

I certainly understand some of your users may be accustomed to this and would welcome additional improvements that make it an optional checkbox of sorts in the upload window. Or a system setting that toggles it, perhaps, as proposed earlier in the thread.

A system toggle is so easy to add, I just wonder why that wasn't included for this pull request. There was plenty of feedback on other threads relating to this topic before this final request was merged.

I do hope this topic continues and gets included in 3.0 with all the finished functionality. I'm not sure if there are still open issues around this slated for 3.0, or whether they were all closed because of this merge.

JoshuaLuckers · 2020-10-07T17:50:17Z

@Rainbowtiger How about this: if you plant some trees (let's say 100) at https://ecologi.com/modmore I'll prepare a pull request today to add a setting to disable the overwrite behavior and do what I can to get that into 2.8.1 for you. 🌴

I planted 100 trees!

Mark-H · 2020-10-07T18:08:15Z

I'll get coding in about an hour or two. ;)

…ith same name on upload + fix self-xss with payload in the file name As discussed in https://community.modx.com/t/version-2-8-is-out-easy-replacement-of-files-pdf-images-is-axed/3166 and the discussion in modxcms#15232, some people are used to being able of replacing files simply by uploading them with the same name. That behavior was removed in modxcms#15232 (v2.8.0-pl) to prefer a stricter check where users are presented with an error when a file already exists with the same name. The new setting, enabled by default, allows users to return to that old behavior and bypassing the check on upload. (The check is still in use, and valid, when renaming files. This is because the renameObject method on the media source will also fail if the target file already exists, so it makes sense to keep that there with the nice error.) Also noticed the `file_err_ae` error message used the unescaped objectname for rendering the error, which could lead to a self-xss. Tidied that up.

Mark-H · 2020-10-07T19:45:03Z

Tadaa: #15285

Thanks for all the trees, folks. Good day for the forest.

SnowCreative · 2020-10-07T19:46:13Z

You've got to tell me how you signed up to have a modmore Ecologi page. I'd love to do that as well.

SnowCreative · 2020-10-07T19:47:53Z

And now, I'm interpreting "upload a file through the tree" in a completely different way!

Mark-H · 2020-10-07T19:55:04Z

It's a business profile: https://ecologi.com/business - only takes a few minutes to set up. The basic idea is you subscribe based on the number of employees to get a number of trees planted (and CO2 offsets) monthly based on that. Renews automatically, and honestly is pretty cheap as far as "business costs" go. It's fun to see the forest grow and read about the projects they're supporting too - definitely recommend it.

They also have personal profiles which I think work mostly the same as business profiles, but are more suited for individual people.

Plus, I like asking for trees as an alternative to monetary donations for.. well, things like this ;)

Ibochkarev · 2020-10-07T20:02:03Z

@Mark-H Nice project and needed. Well, now you can do a good job and develop MODX!

SnowCreative · 2020-10-07T20:47:49Z

I signed up! Planted 72 more trees.

JoshuaLuckers · 2020-10-08T06:51:58Z

It's a business profile: https://ecologi.com/business - only takes a few minutes to set up. The basic idea is you subscribe based on the number of employees to get a number of trees planted (and CO2 offsets) monthly based on that. Renews automatically, and honestly is pretty cheap as far as "business costs" go. It's fun to see the forest grow and read about the projects they're supporting too - definitely recommend it.

They also have personal profiles which I think work mostly the same as business profiles, but are more suited for individual people.

Plus, I like asking for trees as an alternative to monetary donations for.. well, things like this ;)

Slightly off-topic, but my bank also plants trees for every 100 euro I spend using one of my cards. I love planting trees! 🌳

Ruslan-Aleev requested review from Mark-H and opengeek as code owners September 11, 2020 16:38

cla-bot bot added the cla-signed CLA confirmed for contributors to this PR. label Sep 11, 2020

opengeek added the pr/review-needed Pull request requires review and testing. label Sep 15, 2020

opengeek added this to the v2.8.0 milestone Sep 16, 2020

Jako approved these changes Sep 17, 2020

View reviewed changes

JoshuaLuckers reviewed Sep 17, 2020

View reviewed changes

theboxer requested changes Sep 17, 2020

View reviewed changes

Ruslan-Aleev requested a review from theboxer September 17, 2020 10:01

theboxer approved these changes Sep 17, 2020

View reviewed changes

theboxer added pr/ready-for-merging Pull request reviewed and tested and ready for merging. and removed pr/review-needed Pull request requires review and testing. labels Sep 17, 2020

opengeek removed the pr/ready-for-merging Pull request reviewed and tested and ready for merging. label Sep 17, 2020

opengeek reviewed Sep 17, 2020

View reviewed changes

opengeek requested changes Sep 17, 2020

View reviewed changes

Added check for directory and file exist

96d0e44

Ruslan-Aleev requested review from opengeek and theboxer September 17, 2020 20:44

opengeek merged commit bf1b817 into modxcms:2.x Sep 21, 2020

Ibochkarev mentioned this pull request Sep 21, 2020

Improve mediabrowser handling for existing files #14641

Closed

Ruslan-Aleev deleted the 2.x-bug-8371 branch September 22, 2020 07:21

Ruslan-Aleev mentioned this pull request Sep 22, 2020

Add check that the file exists when uploading it #15264

Merged

Mark-H mentioned this pull request Oct 7, 2020

Add upload_check_exists system setting to allow overwriting files with same name #15285

Merged

Ibochkarev mentioned this pull request Oct 22, 2020

File Browser allows silent overwriting of files #8371

Closed

Uh oh!

Added a check for the existence of a file when uploading it #15232

Added a check for the existence of a file when uploading it #15232

Uh oh!

Conversation

Ruslan-Aleev commented Sep 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does it do?

Related issue(s)/PR(s)

Uh oh!

Ruslan-Aleev commented Sep 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Jako commented Sep 14, 2020

Uh oh!

Ruslan-Aleev commented Sep 14, 2020

Uh oh!

matdave commented Sep 15, 2020

Uh oh!

Ruslan-Aleev commented Sep 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Ruslan-Aleev commented Sep 15, 2020

Uh oh!

matdave commented Sep 15, 2020

Uh oh!

JoshuaLuckers Sep 17, 2020

Choose a reason for hiding this comment

Uh oh!

theboxer Sep 17, 2020

Choose a reason for hiding this comment

Uh oh!

Jako Sep 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JoshuaLuckers Sep 17, 2020

Choose a reason for hiding this comment

Uh oh!

theboxer Sep 17, 2020

Choose a reason for hiding this comment

Uh oh!

theboxer Sep 17, 2020

Choose a reason for hiding this comment

Uh oh!

theboxer Sep 17, 2020

Choose a reason for hiding this comment

Uh oh!

Ruslan-Aleev commented Sep 17, 2020

Uh oh!

opengeek left a comment

Choose a reason for hiding this comment

Uh oh!

opengeek commented Sep 17, 2020

Uh oh!

Jako commented Sep 17, 2020

Uh oh!

Ruslan-Aleev commented Sep 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

opengeek left a comment

Choose a reason for hiding this comment

Uh oh!

Ruslan-Aleev commented Sep 17, 2020

Uh oh!

SnowCreative commented Sep 29, 2020

Uh oh!

opengeek commented Sep 29, 2020

Uh oh!

Ruslan-Aleev commented Sep 29, 2020

Uh oh!

Ruslan-Aleev commented Sep 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

opengeek commented Sep 29, 2020

Uh oh!

SnowCreative commented Oct 7, 2020

Uh oh!

Mark-H commented Oct 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Ruslan-Aleev commented Sep 11, 2020 •

edited

Loading

Ruslan-Aleev commented Sep 11, 2020 •

edited

Loading

Ruslan-Aleev commented Sep 15, 2020 •

edited

Loading

Jako Sep 17, 2020 •

edited

Loading

Ruslan-Aleev commented Sep 17, 2020 •

edited

Loading

Ruslan-Aleev commented Sep 29, 2020 •

edited

Loading

Mark-H commented Oct 7, 2020 •

edited

Loading

Ruslan-Aleev commented Oct 7, 2020 •

edited

Loading

Ruslan-Aleev commented Oct 7, 2020 •

edited

Loading

SnowCreative commented Oct 7, 2020 •

edited

Loading