Moodle
  1. Moodle
  2. MDL-33680

Ajax calls with incorrect mime types

    Details

    • Testing Instructions:
      Hide

      Open the 'Network' tab of your browser

      • Navigate to Navigation -> My profile -> My private files
      • Click the 'Add' button to load the File picker
        • Confirm that the call to repository_ajax.php had a Type of 'application/json'
      • Close the file picker
      • Navigate to Administration -> Site administration -> Users -> Permissions -> Assign system roles
      • Select a role
      • Enter some content into the 'Search' box
        • Confirm that the call to /user/selector/search.php had a Type of 'application/json'
      • Edit /user/selector/search.php
        • Add a call to print_error('foo'); somewhere in the output
      • Enter some new content into the search box (no need to reload the page)
        • Confirm that the AjaxException dialogue was shown complete with your message and stack trace
      • throw a new moodle_exception (or similar)
      • Enter some new content into the search box (no need to reload the page)
        • Confirm that the Exception dialogue was shown complete with your message and stack trace
      Show
      Open the 'Network' tab of your browser Navigate to Navigation -> My profile -> My private files Click the 'Add' button to load the File picker Confirm that the call to repository_ajax.php had a Type of 'application/json' Close the file picker Navigate to Administration -> Site administration -> Users -> Permissions -> Assign system roles Select a role Enter some content into the 'Search' box Confirm that the call to /user/selector/search.php had a Type of 'application/json' Edit /user/selector/search.php Add a call to print_error('foo'); somewhere in the output Enter some new content into the search box (no need to reload the page) Confirm that the AjaxException dialogue was shown complete with your message and stack trace throw a new moodle_exception (or similar) Enter some new content into the search box (no need to reload the page) Confirm that the Exception dialogue was shown complete with your message and stack trace
    • Affected Branches:
      MOODLE_21_STABLE, MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE
    • Pull Master Branch:
    • Rank:
      41678

      Description

      While trying to run our Moodle website behind a proxy with Apache mod_proxy (for internal reasons) we encountered issues due to some ajax json responses not being sent using correct myme-type.

      Note: Some modules like file manager (blocks/file_manager/ajax.php) explicitly set their ajax response myme type to application/json

        Issue Links

          Activity

          Hide
          Michael de Raadt added a comment -

          This sounds very similar to MDL-26705.

          Could you please have a look at the advice given in that issue? Let me know if the issue is the same for you and if any of the information there helps.

          Show
          Michael de Raadt added a comment - This sounds very similar to MDL-26705 . Could you please have a look at the advice given in that issue? Let me know if the issue is the same for you and if any of the information there helps.
          Hide
          Andrew Nicols added a comment -

          Kassim

          Could you please confirm whether:

          • this is still an issue that you're facing; and
          • whether it is the same issueas discussed in MDL-26705.

          We're using reverse proxying here without issue on Moodle 2.3, though we're using nginx and haproxy rather than Apache with mod_proxy, and we aren't facing these issues.

          Cheers,

          Andrew

          Show
          Andrew Nicols added a comment - Kassim Could you please confirm whether: this is still an issue that you're facing; and whether it is the same issueas discussed in MDL-26705 . We're using reverse proxying here without issue on Moodle 2.3, though we're using nginx and haproxy rather than Apache with mod_proxy, and we aren't facing these issues. Cheers, Andrew
          Hide
          Andrew Nicols added a comment -

          Hi Kassim,

          I'm going to close this issue as we're unable to replicate it. If you still believe this to be an issue with a supported version of Moodle, could you please re-open the issue.

          If you do need to re-open the issue, if you could give us any additional details as to the errors you're seeing and an idea as to your setup, that would be most appreciated.

          Thanks for taking the time to report this issue and helping to make Moodle better

          Andrew

          Show
          Andrew Nicols added a comment - Hi Kassim, I'm going to close this issue as we're unable to replicate it. If you still believe this to be an issue with a supported version of Moodle, could you please re-open the issue. If you do need to re-open the issue, if you could give us any additional details as to the errors you're seeing and an idea as to your setup, that would be most appreciated. Thanks for taking the time to report this issue and helping to make Moodle better Andrew
          Hide
          Chris Martel added a comment -

          I would like to request that this be revisited. Our institution uses Juniper's SSL-VPN which implements an extensive content rewriting engine to allow users to access Moodle externally. This causes the dreaded "Invalid JSON String" error when a file is uploaded. I found that changing line 60 of /repository/repository_ajax.php (Moodle 2.2.6+) to "@header('Content-type: application/json;');" will fix this issue. I feel that any JSON web service should be using this header rather than text/html.

          If text/html is used, Juniper's appliance transforms the HTTP response from:

          {"url":"https:\/\/dev.moodle.lsuhsc.edu\/moodle20\/draftfile.php\/14\/user\/draft\/864081736\/img.jpg","id":864081736,"file":"img.jpg"}

          to:

          <SCRIPT language="javascript" id="dsshim" src="/dana-cached/js/shimdata.cgi"></SCRIPT>
          <SCRIPT language="javascript" id="dsfunc" src="/dana-cached/js/oth.js"></SCRIPT>
          <SCRIPT language="javascript" id="dstimeout" src="/dana-cached/js/sessiontimeout.js"></SCRIPT>
          <SCRIPT language="javascript" id="dsvar" >
          //<![CDATA[
          var dsnodocwrites = 0 ; var DanaCookie="MoodleSessionsandbox=jioalrm840r02e93s02d4ok811; MoodleSessionsandbox20=huvifijfdv4q6kaiguebgsesk1; MOODLEID1_=%2500%25FAI%2517%255B%25C7"; var DSHost="remote.lsuhsc.edu"; var DSObfuscateHostname=0;var DSTBSettings=16449;var DSTBLU='/dana/home/starter.cgi?startpageonly=1';;danaSetDSHost();
          //]]>
          </SCRIPT>

          {"url":"https:\/\/dev.moodle.lsuhsc.edu\/moodle20\/draftfile.php\/14\/user\/draft\/238163967\/img.jpg","id":238163967,"file":"img.jpg"}

          </noscript>
          <SCRIPT language="javascript" id="dstb-id">
          //<![CDATA[
          setTimeout("dstb()",0);
          //]]>
          </SCRIPT>

          If "application/json" is used (which is the correct mime type IMHO), no rewriting is performed.

          Show
          Chris Martel added a comment - I would like to request that this be revisited. Our institution uses Juniper's SSL-VPN which implements an extensive content rewriting engine to allow users to access Moodle externally. This causes the dreaded "Invalid JSON String" error when a file is uploaded. I found that changing line 60 of /repository/repository_ajax.php (Moodle 2.2.6+) to "@header('Content-type: application/json;');" will fix this issue. I feel that any JSON web service should be using this header rather than text/html. If text/html is used, Juniper's appliance transforms the HTTP response from: {"url":"https:\/\/dev.moodle.lsuhsc.edu\/moodle20\/draftfile.php\/14\/user\/draft\/864081736\/img.jpg","id":864081736,"file":"img.jpg"} to: <SCRIPT language="javascript" id="dsshim" src="/dana-cached/js/shimdata.cgi"></SCRIPT> <SCRIPT language="javascript" id="dsfunc" src="/dana-cached/js/oth.js"></SCRIPT> <SCRIPT language="javascript" id="dstimeout" src="/dana-cached/js/sessiontimeout.js"></SCRIPT> <SCRIPT language="javascript" id="dsvar" > //<![CDATA[ var dsnodocwrites = 0 ; var DanaCookie="MoodleSessionsandbox=jioalrm840r02e93s02d4ok811; MoodleSessionsandbox20=huvifijfdv4q6kaiguebgsesk1; MOODLEID1_=%2500%25FAI%2517%255B%25C7"; var DSHost="remote.lsuhsc.edu"; var DSObfuscateHostname=0;var DSTBSettings=16449;var DSTBLU='/dana/home/starter.cgi?startpageonly=1';;danaSetDSHost(); //]]> </SCRIPT> {"url":"https:\/\/dev.moodle.lsuhsc.edu\/moodle20\/draftfile.php\/14\/user\/draft\/238163967\/img.jpg","id":238163967,"file":"img.jpg"} </noscript> <SCRIPT language="javascript" id="dstb-id"> //<![CDATA[ setTimeout("dstb()",0); //]]> </SCRIPT> If "application/json" is used (which is the correct mime type IMHO), no rewriting is performed.
          Hide
          Michael de Raadt added a comment -

          Thanks for that additional information, Chris.

          Andrew: this might be worth looking at.

          Show
          Michael de Raadt added a comment - Thanks for that additional information, Chris. Andrew: this might be worth looking at.
          Hide
          Andrew Nicols added a comment -

          I wonder whether we should perhaps set the content-type for all scripts defined AJAX_SCRIPT to application/json which would solve this across the board.

          We'd need to take out the explicit overwriting of this (e.g. in repository/repository_ajax.php which sets it to text/html).

          Show
          Andrew Nicols added a comment - I wonder whether we should perhaps set the content-type for all scripts defined AJAX_SCRIPT to application/json which would solve this across the board. We'd need to take out the explicit overwriting of this (e.g. in repository/repository_ajax.php which sets it to text/html).
          Hide
          Andrew Nicols added a comment -

          Petr,

          I'm adding you as a watcher as I suspect that you're probably he best person to offer insight in this.

          I guess we would need to modify lib/setup.php to check whether AJAX_SCRIPT is true and set the header if so. I think that application/json is probably the most appropriate header as all of our AJAX scripts should respond with either some kind of RESTful output or JSON but I may be wrong...

          Adding Jerome too as he's most knowledgeable about web services and this /could/ affect him in some way too (either as a similar issue or related).

          Show
          Andrew Nicols added a comment - Petr, I'm adding you as a watcher as I suspect that you're probably he best person to offer insight in this. I guess we would need to modify lib/setup.php to check whether AJAX_SCRIPT is true and set the header if so. I think that application/json is probably the most appropriate header as all of our AJAX scripts should respond with either some kind of RESTful output or JSON but I may be wrong... Adding Jerome too as he's most knowledgeable about web services and this /could/ affect him in some way too (either as a similar issue or related).
          Hide
          Andrew Nicols added a comment -

          Actually, it turns out that whenever AJAX_SCRIPT is set, we use the core_renderer_ajax renderer, which sets the content type to application/json, except for file uploads:

          From lib/outputrenderers.php::core_renderer_ajax:

              public function header() {
                  // unfortunately YUI iframe upload does not support application/json
                  if (!empty($_FILES)) {
                      @header('Content-type: text/plain; charset=utf-8');
                  } else {
                      error_log("Setting application/json");
                      @header('Content-type: application/json; charset=utf-8');
                  }    
          

          So I believe we just need to remove the relevant places where the content-type is forced.

          So far I can only see one place where we set the Content-type for AJAX scripts, and that's in repository_ajax.php.

          /user/selector/search.php doesn't define itself as an AJAX_SCRIPT though and although it does have the correct content-type set, if it returns an error it will be an HTML error rather than JSON.

          Show
          Andrew Nicols added a comment - Actually, it turns out that whenever AJAX_SCRIPT is set, we use the core_renderer_ajax renderer, which sets the content type to application/json, except for file uploads: From lib/outputrenderers.php::core_renderer_ajax: public function header() { // unfortunately YUI iframe upload does not support application/json if (!empty($_FILES)) { @header('Content-type: text/plain; charset=utf-8'); } else { error_log( "Setting application/json" ); @header('Content-type: application/json; charset=utf-8'); } So I believe we just need to remove the relevant places where the content-type is forced. So far I can only see one place where we set the Content-type for AJAX scripts, and that's in repository_ajax.php. /user/selector/search.php doesn't define itself as an AJAX_SCRIPT though and although it does have the correct content-type set, if it returns an error it will be an HTML error rather than JSON.
          Hide
          Andrew Nicols added a comment -

          I'll provide backport branches for STABLE branches after a successful peer review.

          Show
          Andrew Nicols added a comment - I'll provide backport branches for STABLE branches after a successful peer review.
          Hide
          Petr Škoda added a comment -

          Hi, if I remember it correctly there was a problem with json mimetype in some browsers when uploading files or in file picker in general. Please do not change anything in stable branches because there is a high risk of regression. This needs to be tested in all imaginable browsers, servers and configurations...

          Show
          Petr Škoda added a comment - Hi, if I remember it correctly there was a problem with json mimetype in some browsers when uploading files or in file picker in general. Please do not change anything in stable branches because there is a high risk of regression. This needs to be tested in all imaginable browsers, servers and configurations...
          Hide
          Andrew Nicols added a comment -

          Hi Petr,

          I'm hoping that this will still be handled by http://git.moodle.org/gw?p=moodle.git;a=blob;f=lib/outputrenderers.php;h=4e0545b9ce2897a8de3f0f5f9fbfd0e1cc804f23;hb=HEAD#l3065 which forces the mimetype to text/plain for file uploads.

          Andrew

          Show
          Andrew Nicols added a comment - Hi Petr, I'm hoping that this will still be handled by http://git.moodle.org/gw?p=moodle.git;a=blob;f=lib/outputrenderers.php;h=4e0545b9ce2897a8de3f0f5f9fbfd0e1cc804f23;hb=HEAD#l3065 which forces the mimetype to text/plain for file uploads. Andrew
          Hide
          Andrew Davis added a comment -

          I notice that in user/selector/search.php there is still this in the other branch of the if
          header('Content-type: text/plain; charset=UTF-8');
          Does that header call trump the previous $OUTPUT->header();? Aternatively does echo $OUTPUT->header(); need to be at line 42?

          To me it looks like you'll need to modify your testing instructions as the user search will only be application/json if the tester is not in developer debug mode (which they almost certainly will be).

          [Y] Syntax
          [Y] Output
          [Y] Whitespace
          [NA] Language
          [NA] Databases
          [?] Testing
          [Y] Security
          [NA] Documentation
          [Y] Git
          [Y] Sanity check

          Otherwise it seems to make sense.

          Show
          Andrew Davis added a comment - I notice that in user/selector/search.php there is still this in the other branch of the if header('Content-type: text/plain; charset=UTF-8'); Does that header call trump the previous $OUTPUT->header();? Aternatively does echo $OUTPUT->header(); need to be at line 42? To me it looks like you'll need to modify your testing instructions as the user search will only be application/json if the tester is not in developer debug mode (which they almost certainly will be). [Y] Syntax [Y] Output [Y] Whitespace [NA] Language [NA] Databases [?] Testing [Y] Security [NA] Documentation [Y] Git [Y] Sanity check Otherwise it seems to make sense.
          Hide
          Andrew Nicols added a comment -

          Hmm, I think it's probably best to remove that debugging option rather than return two content-type headers as the

          I've added Tim Hunt here as he originally added that option and may have some additional views.

          Show
          Andrew Nicols added a comment - Hmm, I think it's probably best to remove that debugging option rather than return two content-type headers as the I've added Tim Hunt here as he originally added that option and may have some additional views.
          Hide
          Andrew Nicols added a comment -

          Note, that this is related in part to MDL-38287.

          Show
          Andrew Nicols added a comment - Note, that this is related in part to MDL-38287 .
          Hide
          Tim Hunt added a comment -

          I think the reason for text/html when debugging was so that when an error occurred, it was easy to see the error message in the browser. (Note that this is when I was developing the user selector in 2008. I'm not sure it is still needed.)

          Show
          Tim Hunt added a comment - I think the reason for text/html when debugging was so that when an error occurred, it was easy to see the error message in the browser. (Note that this is when I was developing the user selector in 2008. I'm not sure it is still needed.)
          Hide
          Andrew Nicols added a comment -

          Ah cool. I'll remove it then. Thanks!

          Show
          Andrew Nicols added a comment - Ah cool. I'll remove it then. Thanks!
          Hide
          Andrew Nicols added a comment -

          I've updated this branch and removed the debugging calls in user/selector/search.php.

          I've also added error handling to the JavaScript so I'd appreciate a quick PR to ensure I've not missed anything in my tidy up.

          Cheers,
          A

          Show
          Andrew Nicols added a comment - I've updated this branch and removed the debugging calls in user/selector/search.php. I've also added error handling to the JavaScript so I'd appreciate a quick PR to ensure I've not missed anything in my tidy up. Cheers, A
          Hide
          Tim Hunt added a comment -

          I've not seen the use of M.core.ajaxException before. I can only assume that is why we now need moodle-core-notification. (I have read your recent JS docs, and this was still news to me. Perhaps you should cover error handling there.)

          So, no problems withe the code, +1 from me.

          [Y] Syntax
          [Y] Output
          [Y] Whitespace
          [NA] Language
          [NA] Databases
          [Y] Testing
          [Y] Security
          [NA] Documentation
          [Y] Git
          [Y] Sanity check

          Show
          Tim Hunt added a comment - I've not seen the use of M.core.ajaxException before. I can only assume that is why we now need moodle-core-notification. (I have read your recent JS docs, and this was still news to me. Perhaps you should cover error handling there.) So, no problems withe the code, +1 from me. [Y] Syntax [Y] Output [Y] Whitespace [NA] Language [NA] Databases [Y] Testing [Y] Security [NA] Documentation [Y] Git [Y] Sanity check
          Hide
          Tim Hunt added a comment -

          Oh! stupid workflow state. Of course, Andrew D may still want to peer review this himself.

          Show
          Tim Hunt added a comment - Oh! stupid workflow state. Of course, Andrew D may still want to peer review this himself.
          Hide
          Andrew Nicols added a comment -

          Ah - thanks for the suggestion. I'll make a note to cover that.

          I was also wondering about creating a moodle YUI module to handle IO which wraps the error handling somehow. Haven't thought about how that would work yet though. Will also be writing API docs for moodle-core-notification soon with the hope that we can generate the docs and publish somewhere.

          Show
          Andrew Nicols added a comment - Ah - thanks for the suggestion. I'll make a note to cover that. I was also wondering about creating a moodle YUI module to handle IO which wraps the error handling somehow. Haven't thought about how that would work yet though. Will also be writing API docs for moodle-core-notification soon with the hope that we can generate the docs and publish somewhere.
          Hide
          Andrew Nicols added a comment -

          Oh and yes - M.core.ajaxException and M.core.exception are both provided by moodle-core-notification. I'd prefer to do some lazy-loading there to reduce dependencies, but at present it's included on that page already for something else and I need to check how lazy loading will work in a try/catch scenario.

          Show
          Andrew Nicols added a comment - Oh and yes - M.core.ajaxException and M.core.exception are both provided by moodle-core-notification. I'd prefer to do some lazy-loading there to reduce dependencies, but at present it's included on that page already for something else and I need to check how lazy loading will work in a try/catch scenario.
          Hide
          Andrew Nicols added a comment -

          Rebased on weekly master and submitting for integration.

          Show
          Andrew Nicols added a comment - Rebased on weekly master and submitting for integration.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Crossing fingers... integrated (master only), thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Crossing fingers... integrated (master only), thanks!
          Hide
          David Monllaó added a comment -

          Everything working as described

          Show
          David Monllaó added a comment - Everything working as described
          Hide
          Damyon Wiese added a comment -

          This issue has been integrated upstream and is now available via git (and in some hours, via mirrors and downloads).

          Thanks for your contributions!

          Show
          Damyon Wiese added a comment - This issue has been integrated upstream and is now available via git (and in some hours, via mirrors and downloads). Thanks for your contributions!
          Hide
          Marina Glancy added a comment - - edited

          I think we've introduced a regression here, apparently not all browsers accept application/json mimetype. IE wants to be different again - MDL-39810 (or maybe not)

          Show
          Marina Glancy added a comment - - edited I think we've introduced a regression here, apparently not all browsers accept application/json mimetype. IE wants to be different again - MDL-39810 (or maybe not)

            People

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

              Dates

              • Created:
                Updated:
                Resolved: