Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Problem by loading image in assets folder with whitespace #2971

Closed
ZhaoVitapublic opened this issue Jun 21, 2018 · 18 comments

Comments

Projects
3 participants
@ZhaoVitapublic
Copy link

commented Jun 21, 2018

Problem

  • Can not load assets in folder with whitespace with whitespace with standard nginx config.
  • Can not load image thumbnails wihout fallback with PublicServiceController.

Reason: nginx would not encode the whitespace character from "%20" into " ", the file path will be contains wrong character.

Steps to reproduce

  • Create Asset folder with whitespace. e. g. "Data Object",
  • Insert image example.jpg into the folder.
  • Access the image direct with url "{domain}/Data Object/example.jpg" in browser. (The url is comming from imageObject->getFullPath())

Expected behavior

Can load asset file by full path and can load image without fallback with PublicServiceController

Actual behavior

Can not load asset file in the folder "Data Object". The thumbnails can only load with PublicServiceController

Code example

    # Thumbnails
    location ~* .*/(image|video)-thumb__\d+__.* {
        try_files /var/tmp/$1-thumbnails$uri /app.php;
        expires 2w;
        access_log off;
        add_header Cache-Control "public";
    }

    # Assets
    # Still use a whitelist approach to prevent each and every missing asset to go through the PHP Engine.
    location ~* (.+?)\.((?:css|js)(?:\.map)?|jpe?g|gif|png|svgz?|eps|exe|gz|zip|mp\d|ogg|ogv|webm|pdf|docx?|xlsx?|pptx?)$ {
        try_files /var/assets$uri $uri =404;
        expires 2w;
        access_log off;
        log_not_found off;
        add_header Cache-Control "public";
    }
@ppi-buck

This comment has been minimized.

Copy link

commented Jun 27, 2018

Had a very similar issue with video file names that contain spaces in frontend and backend. Maybe this is connected

@brusch

This comment has been minimized.

Copy link
Member

commented Jun 27, 2018

@ZhaoVitapublic as I don't have a local dev environment running on nginx, it would be great if you could provide a tested PR - thanks!

@ZhaoVitapublic

This comment has been minimized.

Copy link
Author

commented Jun 29, 2018

@brusch
It's really hard to test this problem only using php, since this is a problem causes by combination of nginx and pimcore.
I have newly tried the same on the demo-site, and the problem is not appears for whitespace, but with question mark and hashrouter with following step.

  1. Logging on the demo-basic.pimcore.org
  2. upload image in assets folder with name example?.png (example#.png)
  3. access image direct in browser by https://demo-basic.pimcore.org/example%3F.png (https://demo-basic.pimcore.org/example%23.png)
  4. expect get 404

I hope, this example can help you to reproduce the problem.

Unlike the handling for documents, assets handing allow us to create folder and upload files with all special character like #,?,&. I think, the better way is, that by the upload progress, simply replace such character using some service like

\Pimcore\Model\Element\Service::getValidKey($originalFileName)

In additional, the following environment is currently used for my local pimcore:

system: centos
nginx:

php core:

  • version 7.2.5

php-fpm:

  • version 7.2.5-1
  • config
[pimcore]

user = nginx
group = nginx

listen = 127.0.0.1:9000
listen.allowed_clients = 127.0.0.1

pm = dynamic
pm.max_children = 50
pm.start_servers = 5
pm.min_spare_servers = 5
pm.max_spare_servers = 35

slowlog = /var/log/php-fpm/pimcore-slow.log

env[PIMCORE_ENVIRONMENT] = "dev"

php_admin_value[memory_limit] = 128M
php_admin_value[error_log] = /var/log/php-fpm/pimcore-error.log
php_admin_flag[log_errors] = on

catch_workers_output  = yes

Pimcore:

  • version: 5.2.2
@ppi-buck

This comment has been minimized.

Copy link

commented Jun 29, 2018

Maybe this helps

I'm using this docker-compose setup https://dockerwest.github.io/compose-pimcore/ which also uses nginx locally and as I said above has the same issue. It's fairly easy to set up so maybe this is worth giving a shot for debugging this issue.

@brusch

This comment has been minimized.

Copy link
Member

commented Jun 29, 2018

@ZhaoVitapublic as I said, I don't have a nginx development/testing environment, so a PR would be really helpful.

Thanks

@ZhaoVitapublic

This comment has been minimized.

Copy link
Author

commented Jun 29, 2018

@brusch
I think, what you need, is a docker image like @ppi-buck mentions. I have tried that, it only need python3 and docker on your machine, and this allows you also to debugging the scenarios. I can try to install a blank pimcore available via some url, but even that, you won't be able to identify the problem in codes.

Otherwise, as i mentions, this problem is also appears by the https://demo-basic.pimcore.org, I assume, that problem can also be relevant for apache and other http server.

@brusch brusch added this to To do in 25/06/2018 - 06/07/2018 via automation Jun 29, 2018

@brusch brusch self-assigned this Jun 29, 2018

@brusch brusch added the Bug label Jun 29, 2018

@brusch brusch added this to the 5.3.0 milestone Jun 29, 2018

@brusch brusch moved this from To do to In progress in 25/06/2018 - 06/07/2018 Jun 29, 2018

@brusch

This comment has been minimized.

Copy link
Member

commented Jul 4, 2018

Not so easy to solve it seems:
https://stackoverflow.com/questions/51170028/mod-rewrite-uses-escaped-string-in-substitution

Let's see if someone has an idea ...

@ZhaoVitapublic

This comment has been minimized.

Copy link
Author

commented Jul 4, 2018

@brusch
I think, so far the apache/nginx didn't provide the solution of this problem, is hard to fix this only with server config.
Maybe one suggestion:

  • We rename the filename/foldername internal with html_encode($name),
  • Show in the frontend with html_decode($realname)
@brusch

This comment has been minimized.

Copy link
Member

commented Jul 18, 2018

%20 seems to work at least with apache, so only # and probably other special characters can cause problems.

@brusch

This comment has been minimized.

Copy link
Member

commented Jul 18, 2018

Putting the files url-encoded onto the filesystem is also no option, since Apache unescapes certain characters automatically and so the file exist condition won't work.

@brusch brusch removed this from the 5.3.0 milestone Jul 18, 2018

@ZhaoVitapublic

This comment has been minimized.

Copy link
Author

commented Jul 18, 2018

Generally, the string after the router hash is not available from the server site and the string after the question mark should be used for the request parameter.
It isn't a good idea to upload those file into the file system. Maybe a validation is useful, and force the user to rename the file or folder.

@brusch

This comment has been minimized.

Copy link
Member

commented Jul 18, 2018

Well correctly escaped it's not a problem ...

@brusch

This comment has been minimized.

Copy link
Member

commented Jul 18, 2018

But yes, i guess it makes sense to disallow them.

@brusch

This comment has been minimized.

Copy link
Member

commented Jul 18, 2018

Disallowed characters: #?*:\<>|"
Tested special characters that work []@$&'()+,;=

brusch added a commit that referenced this issue Jul 18, 2018

@brusch

This comment has been minimized.

Copy link
Member

commented Jul 18, 2018

Fixed at least the issues with the characters #?*:\<>|" on apache, not sure about the space in combination with Nginx, probably there's a different solution to solve this.

@brusch

This comment has been minimized.

Copy link
Member

commented Jul 18, 2018

If there isn't any way to solve that using the Nginx config, we have to probably think about passing them through PHP or disallowing certain characters for asset filenames when using Nginx.

@ZhaoVitapublic

This comment has been minimized.

Copy link
Author

commented Jul 18, 2018

@brusch
Thanks for considering this issues and fix it in the Pimcore forward.
So far, we didn't find any general solution with the combination PHP-FPM + Nginx for the problem with whitespace.
Is it possible to set some config variable to set the disallowed character generally,
or otherwise, detect the server software and set the disallowed characters with whitespace, if necessary?

@brusch

This comment has been minimized.

Copy link
Member

commented Jul 18, 2018

Yep, the event pimcore.system.service.preGetValidKey allows you to apply your own filename policy.

@brusch brusch closed this Jul 18, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.