Corruption downloading files from the manager that contain the 'comma' character

modxcms / revolution

MODX Revolution - Content Management Framework

https://modx.com/

GNU General Public License v2.0

1.35k stars 527 forks source link

Corruption downloading files from the manager that contain the 'comma' character #13855

Open alienbuild opened 6 years ago

alienbuild commented 6 years ago

Summary

File corruption occurs when attempting to downloading a file that contains the comma character.

Step to reproduce

Upload a file called something like 'Welcome Calls 2nd, 3rd & 4th April'.
Log into the MODX manager and navigate to the file via a media source.
Right click on the file and choose 'Download file'

Observed behavior

When downloading the file you get a corrupt file with no extension called 'Welcome Calls 2nd, 3rd_'

Expected behavior

Downloading the file correctly.

Environment

MODX 2.6.1-pl

bezumkin commented 6 years ago

I think, it is expected behaviour, because & used to split parameters in url.

Php will receive this array

Array ( 
    [a] => system/file/download 
    [file] => test/Welcome Calls 2nd 3rd 
    [_4th_April_txt] => 
    [wctx] => mgr 
    [source] => 1
 )

The question is - why we can create files with such names in manager?

OptimusCrime commented 6 years ago

Urlencode should fix this.

bezumkin commented 6 years ago

@OptimusCrime Yes.

I`m going to fix it in new media sources for 3.x

bezumkin commented 6 years ago

Fixed in https://github.com/modxcms/revolution/pull/13811 for MODX 3

alienbuild commented 6 years ago

Thanks - just to update the problematic character is actually the ',' character, and not the '&' character as I had originally posted.

bezumkin commented 6 years ago

I believe, it doesn't matter screen shot 2018-04-12 at 17 35 14

alienbuild commented 6 years ago

We're you able to successfully right click and download that file? If the filename contains & it works OK for me and downloads corrextly, but a filename with a comma corrupts the download.

bezumkin commented 6 years ago

Browser don't like commas in file name on download, so I replace it to _.

Made few changes, please, try this PR again.

alroniks commented 3 years ago

Seems it should e fixed by this one https://github.com/modxcms/revolution/pull/15505 But still valid for 3.x

Ibochkarev commented 3 years ago

@alroniks This has been fixed

alroniks commented 3 years ago

It does not work for me when I created the file via phpstorm. With commas and &. I will double check on 2.x and 3.x but seems combinations still may break the system.

alroniks commented 3 years ago