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
    • Rank:
      15808

      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.

        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 Škoda 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 Škoda 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 Škoda 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 Škoda 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 Škoda added a comment -

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

          Show
          Petr Škoda 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 Škoda 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 Škoda 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: