brendanheywood / moodle-local_cleanurls

Lets drag Moodle's url structure into this century...
37 stars 24 forks source link

Image upload not working when cleanurls enabled #52

Closed jking1224 closed 7 years ago

jking1224 commented 7 years ago

In a text area, click the "image" button, proceed to "Upload a file", choose a file from local machine. Rather than upload the jpg (or whatever file type) and show a preview, the URL box shows: (domain address)/draftfile.php With no other appendages and the preview shows a broken image. With the clean urls turned off, the the URL box shows something like: (domain address)/draftfile.php/14/user/draft/818123688/mypix.jpg

So, really its weak programming by the file uploader I imagine, but is there code in clean URLs that address it?

brendanheywood commented 7 years ago

@roperto I'm pretty sure this is actually fixed already, so assigning to you to just test and probably close.

If it is still an issue:

1) the best solution is to stop these types of files ever going through the router in the first place. This should be as easy as avoiding the rewrite if the path contains ".php/" which should capture /draftfile.php/blah and others like /file.php/foo and similar stuff in the themes. The general case is broadly any moodle script which calls get_file_argument() or anything which directly uses $_SERVER['PATH_INFO']. But I think that logic should capture them all.

2) We need to add this as another check to ensure it's good either way. I think this one will be tested quite differently to the other tests. We would make a plugin specific file API callback, eg local_cleanurls_pluginfile(...) and regardless of arugments it will just return a text file with 'OK' as the contents. Then the test script can request a url like:

$CFG->wwwroot/pluginfile.php/1/local_cleanurls/whatever.txt

and check we get the file out the other side. There might be some more hoop jumping setting up a valid context or something.

https://docs.moodle.org/dev/File_API#Serving_files_to_users

jking1224 commented 7 years ago

What would be the proper syntax to add to my Apache config to do as suggested: avoid rewrite for ".php/"

brendanheywood commented 7 years ago

Off the top of my head (and untested) something like:

RewriteCond %{REQUEST_FILENAME} !-f          # existing
RewriteCond %{REQUEST_FILENAME} !-d          # existing
RewriteCond %{REQUEST_URI}      !\.php/      # new

@roperto is looking into this and will confirm the apache and nginx syntax in the readme and also make a dynamic test page to make sure it's working at run time.

roperto commented 7 years ago

Hi @jking1224 and @brendanheywood

I will be creating a pull request still this week to address this and other issues. I will ping you both back when ready.

Cheers,

Daniel

roperto commented 7 years ago

Hi @jking1224 I believe it is a server configuration.

Please update you Clean URLs with the latest version here on GitHub and try the following URL:

http://your.moodle.test/local/cleanurls/webservertest.php

It should point you to which configuration item is not working properly.

If you still cannot make it work, feel free to reopen this issue and add the results of the test above.

Cheers,

Daniel