Moodle
  1. Moodle
  2. MDL-25895

repository_ajax.php tries to download to the browser

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 2.0.1
    • Fix Version/s: 2.0.2
    • Component/s: Repositories
    • Labels:
    • Environment:
      Windows 7, IE 8

      Description

      This issue is exactly the same as http://tracker.moodle.org/browse/MDL-24781. I've just implemented the fix Mauno describes in 24781 and the problem is solved.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            Dan Poltawski added a comment -

            Hi Ian,

            Which fix are from Mauno are you referring to? The one adding the header line? Because that should be there already?

            Show
            Dan Poltawski added a comment - Hi Ian, Which fix are from Mauno are you referring to? The one adding the header line? Because that should be there already?
            Hide
            Mauno Korpelainen added a comment -

            Looking at http://cvs.moodle.org/moodle/repository/repository_ajax.php?r1=1.52&r2=1.53

            @header('Content-type: text/plain');

            should probably be after

            echo $OUTPUT->header(); // send headers

            I suppose Ian means my explanation to Stephen Bloomer about correct place of

            echo $OUTPUT->header(); // send headers
            @header('Content-type: text/html');

            in previous version of bug fix by Dongsheng Cai

            ( http://cvs.moodle.org/moodle/repository/repository_ajax.php?revision=1.52&view=markup )

            in http://tracker.moodle.org/browse/MDL-24781

            Show
            Mauno Korpelainen added a comment - Looking at http://cvs.moodle.org/moodle/repository/repository_ajax.php?r1=1.52&r2=1.53 @header('Content-type: text/plain'); should probably be after echo $OUTPUT->header(); // send headers I suppose Ian means my explanation to Stephen Bloomer about correct place of echo $OUTPUT->header(); // send headers @header('Content-type: text/html'); in previous version of bug fix by Dongsheng Cai ( http://cvs.moodle.org/moodle/repository/repository_ajax.php?revision=1.52&view=markup ) in http://tracker.moodle.org/browse/MDL-24781
            Hide
            Piotr Kowalski added a comment -

            The reason is one: PHP file association in Windows environment. After deleting the association (File Folder Option in Win explorer) File Picker works fine.
            It is confusing and many users can meet the problem while installing any application causing PHP file association. In my case it was Notepad++. What is worse, I did it nearly in the same time frame when upgraded from Moodle 2.0 to 2.0.1 and of course Moodle was suspected.
            I haven't performed any changes in source files.

            Show
            Piotr Kowalski added a comment - The reason is one: PHP file association in Windows environment. After deleting the association (File Folder Option in Win explorer) File Picker works fine. It is confusing and many users can meet the problem while installing any application causing PHP file association. In my case it was Notepad++. What is worse, I did it nearly in the same time frame when upgraded from Moodle 2.0 to 2.0.1 and of course Moodle was suspected. I haven't performed any changes in source files.
            Hide
            Mauno Korpelainen added a comment -

            Piotr - it was exactly my first suggestion in http://tracker.moodle.org/browse/MDL-24781 but you don't need/have to to unssociate php files if source forces header content-type text/plain like Dongsheng Cai suggested in that original bug fix. Since that a new bug fix changed code of repository/repository_ajax.php and obviously made that IE bug visible again in certain environments.

            Show
            Mauno Korpelainen added a comment - Piotr - it was exactly my first suggestion in http://tracker.moodle.org/browse/MDL-24781 but you don't need/have to to unssociate php files if source forces header content-type text/plain like Dongsheng Cai suggested in that original bug fix. Since that a new bug fix changed code of repository/repository_ajax.php and obviously made that IE bug visible again in certain environments.
            Hide
            Piotr Kowalski added a comment -

            Mauno - you are right but it does really matter whether following the line
            echo $OUTPUT->header(); // send headers
            I place:
            @header('Content-type: text/html');
            or
            @header('Content-type: text/plain');

            In the second case (originally implemented in repository_ajax.php) it doesn't work well

            Show
            Piotr Kowalski added a comment - Mauno - you are right but it does really matter whether following the line echo $OUTPUT->header(); // send headers I place: @header('Content-type: text/html'); or @header('Content-type: text/plain'); In the second case (originally implemented in repository_ajax.php) it doesn't work well
            Hide
            Mauno Korpelainen added a comment -

            Well - this sounds a little odd but I could reproduce it again with reassigning php file extension to IE in this particular test PC I have used before. I am 100% sure that fix to MDL-25634 broke code again because if I use the old file ( http://cvs.moodle.org/moodle/repository/repository_ajax.php?revision=1.52&view=markup ) it does not matter if php is unassigned or not but if I use the new file forcing header content type does not work. But even if I moved from http://cvs.moodle.org/moodle/repository/repository_ajax.php?r1=1.52&r2=1.53 @header('Content-type: text/html');

            after

            echo $OUTPUT->header(); // send headers

            it did not have any influence. So the question is what in changed code

            http://cvs.moodle.org/moodle/repository/repository_ajax.php?r1=1.52&r2=1.53

            makes IE crazy?

            Is the reason in this other part (???):

            // if uploaded file is larger than post_max_size (php.ini) setting, $_POST content will lost
            if (empty($_POST) && !empty($action))

            { $err->error = get_string('errorpostmaxsize', 'repository'); die(json_encode($err)); }
            Show
            Mauno Korpelainen added a comment - Well - this sounds a little odd but I could reproduce it again with reassigning php file extension to IE in this particular test PC I have used before. I am 100% sure that fix to MDL-25634 broke code again because if I use the old file ( http://cvs.moodle.org/moodle/repository/repository_ajax.php?revision=1.52&view=markup ) it does not matter if php is unassigned or not but if I use the new file forcing header content type does not work. But even if I moved from http://cvs.moodle.org/moodle/repository/repository_ajax.php?r1=1.52&r2=1.53 @header('Content-type: text/html'); after echo $OUTPUT->header(); // send headers it did not have any influence. So the question is what in changed code http://cvs.moodle.org/moodle/repository/repository_ajax.php?r1=1.52&r2=1.53 makes IE crazy? Is the reason in this other part (???): // if uploaded file is larger than post_max_size (php.ini) setting, $_POST content will lost if (empty($_POST) && !empty($action)) { $err->error = get_string('errorpostmaxsize', 'repository'); die(json_encode($err)); }
            Hide
            Mauno Korpelainen added a comment -

            Ah - of course...sorted

            I removed line 55 from repository/repository_ajax.php

            @header('Content-type: text/plain');

            and added after

            // if uploaded file is larger than post_max_size (php.ini) setting, $_POST content will lost
            if (empty($_POST) && !empty($action))

            { $err->error = get_string('errorpostmaxsize', 'repository'); die(json_encode($err)); }

            list($context, $course, $cm) = get_context_info_array($contextid);
            require_login($course, false, $cm);
            $PAGE->set_context($context);

            echo $OUTPUT->header(); // send headers

            @header('Content-type: text/html');

            and it works with php file type association and without association. The current version is using content-type text/plain and this new line adds content-type text/html after sending headers.

            Show
            Mauno Korpelainen added a comment - Ah - of course...sorted I removed line 55 from repository/repository_ajax.php @header('Content-type: text/plain'); and added after // if uploaded file is larger than post_max_size (php.ini) setting, $_POST content will lost if (empty($_POST) && !empty($action)) { $err->error = get_string('errorpostmaxsize', 'repository'); die(json_encode($err)); } list($context, $course, $cm) = get_context_info_array($contextid); require_login($course, false, $cm); $PAGE->set_context($context); echo $OUTPUT->header(); // send headers @header('Content-type: text/html'); and it works with php file type association and without association. The current version is using content-type text/ plain and this new line adds content-type text/ html after sending headers.
            Hide
            Dongsheng Cai added a comment -

            Hi, thanks everyone for testing and suggestion.

            I attached a patch for this issue, can you please try if it works for you?

            Thanks.

            Show
            Dongsheng Cai added a comment - Hi, thanks everyone for testing and suggestion. I attached a patch for this issue, can you please try if it works for you? Thanks.
            Hide
            Mauno Korpelainen added a comment -

            No it does not - IE does not accept

            @header('Content-type: text/plain');

            IE needs to have

            @header('Content-type: text/html');

            if php file type is associated to IE so if you use html there instead of plain IE can handle your patch OK in all cases.

            Show
            Mauno Korpelainen added a comment - No it does not - IE does not accept @header('Content-type: text/plain'); IE needs to have @header('Content-type: text/html'); if php file type is associated to IE so if you use html there instead of plain IE can handle your patch OK in all cases.
            Hide
            Mauno Korpelainen added a comment -

            If you look back http://cvs.moodle.org/moodle/repository/repository_ajax.php?revision=1.52&view=markup this version did use content-type text/html but revision 1.53 ( changes made for MDL-25634 ) started to open the file again in IE (with php file type association) because content-type was text/plain (not text/html). So the other part of bug fix for MDL-25634 (max-post size issue) was ok - it looks like IE just needs to have

            echo $OUTPUT->header(); // send headers
            @header('Content-type: text/html');

            I tested this several times with different versions simply by changing this one word and every time when when content-type was text/plain IE (with php file type association) tried to open the file and when I changed content-type back to text/html uploading worked like normal.

            Show
            Mauno Korpelainen added a comment - If you look back http://cvs.moodle.org/moodle/repository/repository_ajax.php?revision=1.52&view=markup this version did use content-type text/html but revision 1.53 ( changes made for MDL-25634 ) started to open the file again in IE (with php file type association) because content-type was text/plain (not text/html). So the other part of bug fix for MDL-25634 (max-post size issue) was ok - it looks like IE just needs to have echo $OUTPUT->header(); // send headers @header('Content-type: text/html'); I tested this several times with different versions simply by changing this one word and every time when when content-type was text/plain IE (with php file type association) tried to open the file and when I changed content-type back to text/html uploading worked like normal.
            Hide
            Dongsheng Cai added a comment -

            Hi, Mauno

            Thanks a lot, the new patch change content-type to text/html.

            Show
            Dongsheng Cai added a comment - Hi, Mauno Thanks a lot, the new patch change content-type to text/html.
            Hide
            Eloy Lafuente (stronk7) added a comment -

            Thanks a lot Ian, Mauno, Dong!

            Just guessing if there isn't any other "official way" to add that header instead of using:

            @header('Content-type: text/html');

            so $OUTPUT->header() will include it automatically.

            Addressing to STABLE backlog, would be great to have it fixed asap, ciao .-)

            Show
            Eloy Lafuente (stronk7) added a comment - Thanks a lot Ian, Mauno, Dong! Just guessing if there isn't any other "official way" to add that header instead of using: @header('Content-type: text/html'); so $OUTPUT->header() will include it automatically. Addressing to STABLE backlog, would be great to have it fixed asap, ciao .-)
            Hide
            Petr Skoda added a comment -

            I do not want to spoil your party, but adding text/html header to all ajax requests may create nasty XSS problems, you have to make sure you do not encode anything dangerous there...

            Show
            Petr Skoda added a comment - I do not want to spoil your party, but adding text/html header to all ajax requests may create nasty XSS problems, you have to make sure you do not encode anything dangerous there...
            Hide
            Mauno Korpelainen added a comment -

            Petr,

            I have no idea how this @header('Content-type: text/html'); could be vulnerable (if charset does not change) but I have a new suggestion - hopefully it is not even worse

            If you delete line

            @header('Content-type: text/html');

            from repository/repository_ajax.php

            and change in lib/outputrenderers.php and in public function header() line 2722 to use content-type text/html instead of text/plain IE does not try to download repository_ajax.php because content-type text/html is part of

            echo $OUTPUT->header(); // send headers

            So the function header() would look like:

            public function header() {
            // unfortunately YUI iframe upload does not support application/json
            if (!empty($_FILES))

            { @header('Content-type: text/html'); }

            else

            { @header('Content-type: application/json'); }

            /// Headers to make it not cacheable and json
            @header('Cache-Control: no-store, no-cache, must-revalidate');
            @header('Cache-Control: post-check=0, pre-check=0', false);
            @header('Pragma: no-cache');
            @header('Expires: Mon, 20 Aug 1969 09:23:00 GMT');
            @header('Last-Modified: ' . gmdate('D, d M Y H:i:s') . ' GMT');
            @header('Accept-Ranges: none');
            }

            Is this better?

            Show
            Mauno Korpelainen added a comment - Petr, I have no idea how this @header('Content-type: text/html'); could be vulnerable (if charset does not change) but I have a new suggestion - hopefully it is not even worse If you delete line @header('Content-type: text/html'); from repository/repository_ajax.php and change in lib/outputrenderers.php and in public function header() line 2722 to use content-type text/html instead of text/plain IE does not try to download repository_ajax.php because content-type text/html is part of echo $OUTPUT->header(); // send headers So the function header() would look like: public function header() { // unfortunately YUI iframe upload does not support application/json if (!empty($_FILES)) { @header('Content-type: text/html'); } else { @header('Content-type: application/json'); } /// Headers to make it not cacheable and json @header('Cache-Control: no-store, no-cache, must-revalidate'); @header('Cache-Control: post-check=0, pre-check=0', false); @header('Pragma: no-cache'); @header('Expires: Mon, 20 Aug 1969 09:23:00 GMT'); @header('Last-Modified: ' . gmdate('D, d M Y H:i:s') . ' GMT'); @header('Accept-Ranges: none'); } Is this better?
            Hide
            Mauno Korpelainen added a comment -

            Or even better - this works just as well (in lib/outputrenderers.php) no matter if php file type is associated to IE and @header('Content-type: text/html'); or @header('Content-type: text/plain'); can both be taken away:

            public function header() {
            // unfortunately YUI iframe upload does not support application/json
            if (!empty($_FILES)) {

            } else

            { @header('Content-type: application/json'); }

            /// Headers to make it not cacheable and json
            @header('Cache-Control: no-store, no-cache, must-revalidate');
            @header('Cache-Control: post-check=0, pre-check=0', false);
            @header('Pragma: no-cache');
            @header('Expires: Mon, 20 Aug 1969 09:23:00 GMT');
            @header('Last-Modified: ' . gmdate('D, d M Y H:i:s') . ' GMT');
            @header('Accept-Ranges: none');
            }

            Show
            Mauno Korpelainen added a comment - Or even better - this works just as well (in lib/outputrenderers.php) no matter if php file type is associated to IE and @header('Content-type: text/html'); or @header('Content-type: text/plain'); can both be taken away: public function header() { // unfortunately YUI iframe upload does not support application/json if (!empty($_FILES)) { } else { @header('Content-type: application/json'); } /// Headers to make it not cacheable and json @header('Cache-Control: no-store, no-cache, must-revalidate'); @header('Cache-Control: post-check=0, pre-check=0', false); @header('Pragma: no-cache'); @header('Expires: Mon, 20 Aug 1969 09:23:00 GMT'); @header('Last-Modified: ' . gmdate('D, d M Y H:i:s') . ' GMT'); @header('Accept-Ranges: none'); }
            Hide
            Petr Skoda added a comment -

            If you send page with text/html header you simply must make sure the code is not unintentionally executed in the browser, in standard moodle page we do that by using s(), format_text() or format_string().

            I do not know how to solve this problem, I am just making sure you know why it is recommended to use appropriate headers.

            Show
            Petr Skoda added a comment - If you send page with text/html header you simply must make sure the code is not unintentionally executed in the browser, in standard moodle page we do that by using s(), format_text() or format_string(). I do not know how to solve this problem, I am just making sure you know why it is recommended to use appropriate headers.
            Show
            Mauno Korpelainen added a comment - Well, I can't fix IE http://www.howtocreate.co.uk/wrongWithIE/?chapter=Content-type%3A+text%2Fplain
            Hide
            Petr Skoda added a comment -

            Yeah, but we do not have to send the same sloppy headers to all browsers, right?

            Show
            Petr Skoda added a comment - Yeah, but we do not have to send the same sloppy headers to all browsers, right?
            Hide
            Mauno Korpelainen added a comment -

            Well, here you have the next suggestion: isn't moodle 2 using utf-8 everywhere?

            We could use in lib/outputrenderers.php

            public function header() {
            // unfortunately YUI iframe upload does not support application/json
            if (!empty($_FILES))

            { @header('Content-type: text/html; charset=UTF-8'); }

            else

            { @header('Content-type: application/json'); }

            /// Headers to make it not cacheable and json
            @header('Cache-Control: no-store, no-cache, must-revalidate');
            @header('Cache-Control: post-check=0, pre-check=0', false);
            @header('Pragma: no-cache');
            @header('Expires: Mon, 20 Aug 1969 09:23:00 GMT');
            @header('Last-Modified: ' . gmdate('D, d M Y H:i:s') . ' GMT');
            @header('Accept-Ranges: none');
            }

            Then we don't need separate

            @header('Content-type: text/html'); or @header('Content-type: text/html; charset=UTF-8');

            in repository/repository_ajax.php and UTF-7 type attacks are less likely and this works also with custom file type associations in IE.

            Does moodle always use some standard way in echo $OUTPUT tags to filter out invalid utf-8 characters before output (HTMLPurifier?) ?

            Show
            Mauno Korpelainen added a comment - Well, here you have the next suggestion: isn't moodle 2 using utf-8 everywhere? We could use in lib/outputrenderers.php public function header() { // unfortunately YUI iframe upload does not support application/json if (!empty($_FILES)) { @header('Content-type: text/html; charset=UTF-8'); } else { @header('Content-type: application/json'); } /// Headers to make it not cacheable and json @header('Cache-Control: no-store, no-cache, must-revalidate'); @header('Cache-Control: post-check=0, pre-check=0', false); @header('Pragma: no-cache'); @header('Expires: Mon, 20 Aug 1969 09:23:00 GMT'); @header('Last-Modified: ' . gmdate('D, d M Y H:i:s') . ' GMT'); @header('Accept-Ranges: none'); } Then we don't need separate @header('Content-type: text/html'); or @header('Content-type: text/html; charset=UTF-8'); in repository/repository_ajax.php and UTF-7 type attacks are less likely and this works also with custom file type associations in IE. Does moodle always use some standard way in echo $OUTPUT tags to filter out invalid utf-8 characters before output (HTMLPurifier?) ?
            Hide
            Petr Skoda added a comment - - edited

            Exploiting ajax may be trivial if it outputs text that looks like html with javascript - it just gets executed when you load it into browser as normal page, it does not matter if the document is valid html or not.

            Show
            Petr Skoda added a comment - - edited Exploiting ajax may be trivial if it outputs text that looks like html with javascript - it just gets executed when you load it into browser as normal page, it does not matter if the document is valid html or not.
            Hide
            Dongsheng Cai added a comment -

            Hi, Petr

            Is it ok to change header for repository_ajax.php only? the content there is generated by json_encode which should be good, right?

            Show
            Dongsheng Cai added a comment - Hi, Petr Is it ok to change header for repository_ajax.php only? the content there is generated by json_encode which should be good, right?
            Hide
            Thomas Tronhus added a comment -

            Hi i fiksed this issue, by comparing the file reposotory_ajax.php from moodle 2.0 to the 2.01 version. the one in moodle 2.0 dosent have the issue
            i moved the @header('Content-type: text/html'); to right after echo $OUTPUT->header(); // send headers

            Show
            Thomas Tronhus added a comment - Hi i fiksed this issue, by comparing the file reposotory_ajax.php from moodle 2.0 to the 2.01 version. the one in moodle 2.0 dosent have the issue i moved the @header('Content-type: text/html'); to right after echo $OUTPUT->header(); // send headers
            Hide
            Dongsheng Cai added a comment -

            Thanks for all input. Pull request submitted, this is issue is under review.

            Show
            Dongsheng Cai added a comment - Thanks for all input. Pull request submitted, this is issue is under review.

              People

              • Votes:
                1 Vote for this issue
                Watchers:
                8 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved: