Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-39810

IE Compatibility View problem - Moodle 2.5 repository_ajax.php tries to download in browser

    Details

    • Testing Instructions:
      Hide

      (difficulty: easy, requires IE8, IE9, IE10 among the other supported browsers. For the nature of the patch backport, please test both 2.5 and 2.6|master branches)

      Note: You must test in all supported browsers including IE 8, 9, and 10.

      1. Upload a file resource using the most recent version of the supported browsers, IE8-9 and IE 10 in Compatibility View 1 included (being the ones affected by this issue): it must work as expected for each tested browser;
      2. Upload the same file resource again: a confirmation popup about overwriting the previous file will appear. For the developers (<F12> in your browser): the JSON response will be always returned as text/plain while the overwrite confirmation will be served as text/plain only for IE in Compatibility View, otherwise application/json.
      3. Confirm that no errors were shown and everything works as expected

      1 Configuring Compatibility View is quickly set by means of the IE Developer Tools, by pressing <F12>, then clicking on Browser Mode in the top menu and selecting the appropriate mode.

      Show
      (difficulty: easy, requires IE8, IE9, IE10 among the other supported browsers. For the nature of the patch backport, please test both 2.5 and 2.6|master branches) Note: You must test in all supported browsers including IE 8, 9, and 10. Upload a file resource using the most recent version of the supported browsers, IE8-9 and IE 10 in Compatibility View 1 included (being the ones affected by this issue): it must work as expected for each tested browser; Upload the same file resource again: a confirmation popup about overwriting the previous file will appear. For the developers ( <F12> in your browser): the JSON response will be always returned as text/plain while the overwrite confirmation will be served as text/plain only for IE in Compatibility View, otherwise application/json . Confirm that no errors were shown and everything works as expected 1 Configuring Compatibility View is quickly set by means of the IE Developer Tools , by pressing <F12> , then clicking on Browser Mode in the top menu and selecting the appropriate mode .
    • Affected Branches:
      MOODLE_25_STABLE, MOODLE_26_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      m26_MDL-39810_IE_Compatibility_View_Does_Not_Recognize_JSON_RFC4627_MIME_Type

      Description

      Users are encountering problems when they use IE and try to upload any file through the file resource.

      When they try and upload the file, they are asked if they wise to open or save "repository_ajax.php" file

      The fix is to add the "@header('Content-type: text/html; charset=utf-8');" code snippet after line 59 (echo $OUTPUT->header(); // send headers)

      So it should be:

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

      // add this code snippet
      @header('Content-type: text/html; charset=utf-8');
      // end add

      .....

      This was missed in 2.5 and it was included in earlier 2.x versions

      https://moodle.org/mod/forum/discuss.php?d=228568

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            danmarsden Dan Marsden added a comment -

            It might be helpful if you are able to specify which versions of IE you have tested that cause this issue.

            Show
            danmarsden Dan Marsden added a comment - It might be helpful if you are able to specify which versions of IE you have tested that cause this issue.
            Hide
            tdejager Tiffany Morgan added a comment -

            I was using IE 8 when I had the issue. When I tried to upload using Firefox I did not receive the prompt to open/save the php file.

            Show
            tdejager Tiffany Morgan added a comment - I was using IE 8 when I had the issue. When I tried to upload using Firefox I did not receive the prompt to open/save the php file.
            Hide
            moodlevcc Lawrence N added a comment -

            Same... IE 8... After I implemented the additional code string, it worked.

            Show
            moodlevcc Lawrence N added a comment - Same... IE 8... After I implemented the additional code string, it worked.
            Hide
            marina Marina Glancy added a comment -

            I think the proposed solution is incomplete and issue needs more investigation

            First of all repository_ajax.php does not return the result as html. It returns it as json-encoded string as it is already said in headers sent by core_renderer_ajax::header(). If it's not application/json it should be at least text/plain but definitely not text/html.

            The problem is with IE8 not recognising this content type. Lawrence proposes to change the header to html regardless of browser and only on one particular ajax page. So all normal browsers have to receive the incorrect header just because of one stupid browser.

            So the resume:

            • Header needs to be changed only for particular browser
            • Header needs to be changed for all AJAX files and not only repository_ajax.php . What happens on other pages that use AJAX requests?
              For example drag&drop moving the sections on course page? Try executing git grep "define..AJAX_SCRIPT" to find other places where moodle uses AJAX
              So it probably needs to be done in core_renderer_ajax::header() instead of on particular page
            • Also we need to google for solutions, I'm sure moodle is not the first application suffering with IE8
            Show
            marina Marina Glancy added a comment - I think the proposed solution is incomplete and issue needs more investigation First of all repository_ajax.php does not return the result as html. It returns it as json-encoded string as it is already said in headers sent by core_renderer_ajax::header(). If it's not application/json it should be at least text/plain but definitely not text/html. The problem is with IE8 not recognising this content type. Lawrence proposes to change the header to html regardless of browser and only on one particular ajax page. So all normal browsers have to receive the incorrect header just because of one stupid browser. So the resume: Header needs to be changed only for particular browser Header needs to be changed for all AJAX files and not only repository_ajax.php . What happens on other pages that use AJAX requests? For example drag&drop moving the sections on course page? Try executing git grep "define..AJAX_SCRIPT" to find other places where moodle uses AJAX So it probably needs to be done in core_renderer_ajax::header() instead of on particular page Also we need to google for solutions, I'm sure moodle is not the first application suffering with IE8
            Hide
            smithrn Ryan Smith added a comment -

            We had a report from an IE8 user experiencing this same problem. It does not, however, happen on all IE8 installations as a test VM that I use to test IE8 issues works just fine when uploading files.

            Show
            smithrn Ryan Smith added a comment - We had a report from an IE8 user experiencing this same problem. It does not, however, happen on all IE8 installations as a test VM that I use to test IE8 issues works just fine when uploading files.
            Hide
            matteo Matteo Scaramuccia added a comment -

            Hi Marina,
            what about improving core_renderer_ajax::header() with something similar to https://github.com/blueimp/jQuery-File-Upload/wiki/Setup#content-type-negotiation:

            if (isset($_SERVER['HTTP_ACCEPT']) &&
                (strpos($_SERVER['HTTP_ACCEPT'], 'application/json') !== false)) {
                header('Content-type: application/json');
            } else {
                header('Content-type: text/plain');
            }

            ?
            Another IE8 hacking approach could be to search for application/x-ms-application, details in http://baddotrobot.com/blog/2012/02/21/jersey-and-ie8/.
            My preference goes to the first suggested approach with a small change, to be sure that IE won't perform one of its own magics:

            if (isset($_SERVER['HTTP_ACCEPT']) &&
                (strpos($_SERVER['HTTP_ACCEPT'], 'application/json') !== false)) {
                header('Content-type: application/json');
            } else {
                header('Content-type: text/plain');
                header('X-Content-Type-Options: nosniff');
            }

            Unfortunately I do not have IE8 to test the code and submitting a working proposal via a github branch.

            HTH,
            Matteo

            Show
            matteo Matteo Scaramuccia added a comment - Hi Marina, what about improving core_renderer_ajax::header() with something similar to https://github.com/blueimp/jQuery-File-Upload/wiki/Setup#content-type-negotiation: if (isset($_SERVER['HTTP_ACCEPT']) && (strpos($_SERVER['HTTP_ACCEPT'], 'application/json') !== false)) { header('Content-type: application/json'); } else { header('Content-type: text/plain'); } ? Another IE8 hacking approach could be to search for application/x-ms-application , details in http://baddotrobot.com/blog/2012/02/21/jersey-and-ie8/ . My preference goes to the first suggested approach with a small change, to be sure that IE won't perform one of its own magics: if (isset($_SERVER['HTTP_ACCEPT']) && (strpos($_SERVER['HTTP_ACCEPT'], 'application/json') !== false)) { header('Content-type: application/json'); } else { header('Content-type: text/plain'); header('X-Content-Type-Options: nosniff'); } Unfortunately I do not have IE8 to test the code and submitting a working proposal via a github branch. HTH, Matteo
            Hide
            marina Marina Glancy added a comment - - edited

            I mark it as a blocker and confirm that this is a regression introduced in 2.5 (MDL-33680)

            +1 for Matteo's suggestion

            Show
            marina Marina Glancy added a comment - - edited I mark it as a blocker and confirm that this is a regression introduced in 2.5 ( MDL-33680 ) +1 for Matteo's suggestion
            Hide
            marina Marina Glancy added a comment -

            hm, I've just checked: the content-type is the same in Moodle 2.4. So maybe it's not a recent regression.

            Show
            marina Marina Glancy added a comment - hm, I've just checked: the content-type is the same in Moodle 2.4. So maybe it's not a recent regression.
            Hide
            salvetore Michael de Raadt added a comment -

            Thanks for reporting this, guys.

            I've assigned this to Andrew because of his original involvement in the linked issue.

            However, I know Andrew is in holidays, so if Tim, Eloy or anyone else wants to take this on and make it happen, please go ahead.

            Show
            salvetore Michael de Raadt added a comment - Thanks for reporting this, guys. I've assigned this to Andrew because of his original involvement in the linked issue. However, I know Andrew is in holidays, so if Tim, Eloy or anyone else wants to take this on and make it happen, please go ahead.
            Hide
            timhunt Tim Hunt added a comment -

            Presumably you have searched the web for this. If you, I hope you did better than me. I just found http://stackoverflow.com/questions/4715373/json-not-defined-internet-explorer-8.

            Show
            timhunt Tim Hunt added a comment - Presumably you have searched the web for this. If you, I hope you did better than me. I just found http://stackoverflow.com/questions/4715373/json-not-defined-internet-explorer-8 .
            Hide
            timhunt Tim Hunt added a comment -

            OK, a bit more searching. E.g. http://stackoverflow.com/questions/8151138/ie-jquery-form-multipart-json-response-ie-tries-to-download-response. It seems that the common solution is to lie to IE about the mime type. Matteo is probably right about the way and place in the code to do that, so I agree with him and Marina.

            Show
            timhunt Tim Hunt added a comment - OK, a bit more searching. E.g. http://stackoverflow.com/questions/8151138/ie-jquery-form-multipart-json-response-ie-tries-to-download-response . It seems that the common solution is to lie to IE about the mime type. Matteo is probably right about the way and place in the code to do that, so I agree with him and Marina.
            Hide
            matteo Matteo Scaramuccia added a comment -

            Hi Tiffany/Lawrence,
            could you test the new fix proposal?
            Unfortunately I miss IE8 and cannot test it by myself: I feel this issue should be quickly addressed, especially if we have a potential working solution and the Dev Audience agree on it (if it will work).

            TIA,
            Matteo

            Show
            matteo Matteo Scaramuccia added a comment - Hi Tiffany/Lawrence, could you test the new fix proposal? Unfortunately I miss IE8 and cannot test it by myself: I feel this issue should be quickly addressed, especially if we have a potential working solution and the Dev Audience agree on it (if it will work). TIA, Matteo
            Hide
            matteo Matteo Scaramuccia added a comment -

            Hi All,
            I've done a small fix/enh, waiting for IE8 testing.
            The effect of this patch is that even Accept: */** will be considered as "JSON MIME Type support unavailable" and text/plain will be used: we need to agree on that or just add an OR considering */* a valid condition to route to the official JSON MIME Type.
            Consider the proposal still a work in progress (WIP): now, I'm just interested in verifying if the key point for IE8 is X-Content-Type-Options: nosniff.

            @Andrew Nicols: I've just seen that MDL-33680 missed to cleanup some other files e.g. lib/ajax/getnavbranch.php:

            --- a/lib/ajax/getnavbranch.php
            +++ b/lib/ajax/getnavbranch.php
            @@ -124,6 +124,6 @@ if (empty($branch) || $branch->nodetype !== navigation_node::NODETYPE_BRANCH) {
             // Prepare an XML converter for the branch
             $converter->set_expandable($navigation->get_expandable());
             // Set XML headers
            -header('Content-type: text/plain; charset=utf-8');
            +echo $OUTPUT->header(); // Send headers.
             // Convert and output the branch as XML
             echo $converter->convert($branch);
            

            Would you like me to search for similar patterns and submit a separate issue, at least for master?

            * It should be a JSON client responsibility to setup that header according with what supported, when making a JSON request.

            Show
            matteo Matteo Scaramuccia added a comment - Hi All, I've done a small fix/enh, waiting for IE8 testing. The effect of this patch is that even Accept: */* * will be considered as "JSON MIME Type support unavailable" and text/plain will be used: we need to agree on that or just add an OR considering */* a valid condition to route to the official JSON MIME Type. Consider the proposal still a work in progress (WIP): now, I'm just interested in verifying if the key point for IE8 is X-Content-Type-Options: nosniff . @ Andrew Nicols : I've just seen that MDL-33680 missed to cleanup some other files e.g. lib/ajax/getnavbranch.php : --- a/lib/ajax/getnavbranch.php +++ b/lib/ajax/getnavbranch.php @@ -124,6 +124,6 @@ if (empty($branch) || $branch->nodetype !== navigation_node::NODETYPE_BRANCH) { // Prepare an XML converter for the branch $converter->set_expandable($navigation->get_expandable()); // Set XML headers -header('Content-type: text/plain; charset=utf-8'); +echo $OUTPUT->header(); // Send headers. // Convert and output the branch as XML echo $converter->convert($branch); Would you like me to search for similar patterns and submit a separate issue, at least for master ? * It should be a JSON client responsibility to setup that header according with what supported, when making a JSON request.
            Hide
            marina Marina Glancy added a comment -

            Matteo, thanks! imho this is an improvement and should go to master branch only, so we should deal with it in separate issue

            Show
            marina Marina Glancy added a comment - Matteo, thanks! imho this is an improvement and should go to master branch only, so we should deal with it in separate issue
            Hide
            matteo Matteo Scaramuccia added a comment -

            Hi Marina,
            OK (I was supposing the same), let's wait for Tiffany/Lawrence feedbacks about tests on IE8 and I'll change things according to their feedbacks.
            Small recap to verify if I've correctly understood:

            1. Wait for Tiffany/Lawrence feedbacks
            2. Add X-Content-Type-Options: nosniff to both 2.5 and 2.6, if it will solve the issue
            3. Add Accept branching in 2.6, if people will agree on it
            4. Cleanup AJAX (move from header('Content-type: ...' to $OUTPUT->header()) in 2.6

            Correct?

            Show
            matteo Matteo Scaramuccia added a comment - Hi Marina, OK (I was supposing the same), let's wait for Tiffany/Lawrence feedbacks about tests on IE8 and I'll change things according to their feedbacks. Small recap to verify if I've correctly understood: Wait for Tiffany/Lawrence feedbacks Add X-Content-Type-Options: nosniff to both 2.5 and 2.6, if it will solve the issue Add Accept branching in 2.6, if people will agree on it Cleanup AJAX (move from header('Content-type: ...' to $OUTPUT->header() ) in 2.6 Correct?
            Hide
            matteo Matteo Scaramuccia added a comment -

            Found IE8 : http://www.modern.ie/en-us/virtualization-tools#downloads, I'll download it and report back the results.

            Show
            matteo Matteo Scaramuccia added a comment - Found IE8 : http://www.modern.ie/en-us/virtualization-tools#downloads , I'll download it and report back the results.
            Hide
            dobedobedoh Andrew Nicols added a comment -

            Hi Marina,

            This needs to go to 2.5 too - these versions of IE will attempt to download the AJAX results without it.

            Show
            dobedobedoh Andrew Nicols added a comment - Hi Marina, This needs to go to 2.5 too - these versions of IE will attempt to download the AJAX results without it.
            Hide
            matteo Matteo Scaramuccia added a comment -

            These are the results:
            1. I was unable to replicate the issue using that IE8;
            2. the proposal "works" in terms of: it doesn't break anything.

            The machine is brand new as per Microsoft's own setup, except for having installed Fiddler and the required .Net Framework 2.0.

            I'll re-extract the machine and do the required limited changes (keyboard and network) to see if .Net Framework adds/fixes support for JSON MIME Type i.e. this is the last resort to find an environment to replicate this issue.

            Show
            matteo Matteo Scaramuccia added a comment - These are the results: 1. I was unable to replicate the issue using that IE8; 2. the proposal "works" in terms of: it doesn't break anything. The machine is brand new as per Microsoft's own setup, except for having installed Fiddler and the required .Net Framework 2.0. I'll re-extract the machine and do the required limited changes (keyboard and network) to see if .Net Framework adds/fixes support for JSON MIME Type i.e. this is the last resort to find an environment to replicate this issue.
            Hide
            matteo Matteo Scaramuccia added a comment - - edited

            Last resort failed, on my side no way to replicate the issue with IE8... waiting for Tiffany/Lawrence.

            For the record:

            1. To install that VM, you need to:
              1. extract it: I'm used to use 7-zip, regardless that file being an executable
              2. download the VMware Open Virtualization Format Tool (ovftool.exe): http://communities.vmware.com/community/vmtn/automationtools/ovf?view=overview
              3. open a DOS prompt to launch the conversion, to let VMware Player launch it, e.g.:

                C:\Program Files\VMware\VMware OVF Tool>ovftool.exe "E:\Virtual Machines\IE8.WinXP.For.WindowsVMWare\IE8 - WinXP.ovf" "E:\Virtual
                Machines\IE8.WinXP.For.WindowsVMWare\IE8 - WinXP.vmx"
                Opening OVF source: //:@matteo-nb:80\Virtual Machines\IE8.WinXP.For.WindowsVMWare\IE8 - WinXP.ovf
                The manifest validates
                Opening VMX target: E:\Virtual Machines\IE8.WinXP.For.WindowsVMWare\IE8 - WinXP.vmx
                Writing VMX file: E:\Virtual Machines\IE8.WinXP.For.WindowsVMWare\IE8 - WinXP.vmx
                Transfer Completed
                Completed successfully
                

              4. Edit the VM settings to adjust Network settings according to your local setup, probably using ethernet0.connectionType = "bridged"
              5. (optional) Lowering RAM to 256-320MB
            2. The machine SW versions are:
              • OS Version: Windows XP Service Pack 3
              • IE Version: 8.0.6001.18702
            3. While sniffing the traffic with Fiddler and double checking the hits to Apache I've found two minor unrelated issues:
              1. an HTTP 404 for an attempt to load the profile of the user whose id is equal to the rev number for the GFX:

                192.168.0.200 - - [12/Jun/2013:22:18:24 +0200] "GET /moodle-master/user/profile.php?id=1370780391 HTTP/1.1" 404 17034 "http://vm-centos5/moodle-master/course/modedit.php?add=resource&type=&course=2&section=2&return=0&sr=0" "Mozilla/4.0 (compatible; MSIE 8.0; Windows NT 5.1; Trident/4.0)"
                

              2. a consequent error in profile.php:

                [Wed Jun 12 22:18:24 2013] [error] [client 192.168.0.200] PHP Notice:  Trying to get property of non-object in /var/www/moodle-master-git-remote/user/profile.php on line 61, referer: http://vm-centos5/moodle-master/course/modedit.php?add=resource&type=&course=2&section=2&return=0&sr=0
                [Wed Jun 12 22:18:24 2013] [error] [client 192.168.0.200] PHP Notice:  Trying to get property of non-object in /var/www/moodle-master-git-remote/user/profile.php on line 69, referer: http://vm-centos5/moodle-master/course/modedit.php?add=resource&type=&course=2&section=2&return=0&sr=0
                

            Show
            matteo Matteo Scaramuccia added a comment - - edited Last resort failed, on my side no way to replicate the issue with IE8... waiting for Tiffany/Lawrence. For the record: To install that VM, you need to: extract it: I'm used to use 7-zip, regardless that file being an executable download the VMware Open Virtualization Format Tool ( ovftool.exe ): http://communities.vmware.com/community/vmtn/automationtools/ovf?view=overview open a DOS prompt to launch the conversion, to let VMware Player launch it, e.g.: C:\Program Files\VMware\VMware OVF Tool>ovftool.exe "E:\Virtual Machines\IE8.WinXP.For.WindowsVMWare\IE8 - WinXP.ovf" "E:\Virtual Machines\IE8.WinXP.For.WindowsVMWare\IE8 - WinXP.vmx" Opening OVF source: //:@matteo-nb:80\Virtual Machines\IE8.WinXP.For.WindowsVMWare\IE8 - WinXP.ovf The manifest validates Opening VMX target: E:\Virtual Machines\IE8.WinXP.For.WindowsVMWare\IE8 - WinXP.vmx Writing VMX file: E:\Virtual Machines\IE8.WinXP.For.WindowsVMWare\IE8 - WinXP.vmx Transfer Completed Completed successfully Edit the VM settings to adjust Network settings according to your local setup, probably using ethernet0.connectionType = "bridged" (optional) Lowering RAM to 256-320MB The machine SW versions are: OS Version: Windows XP Service Pack 3 IE Version: 8.0.6001.18702 While sniffing the traffic with Fiddler and double checking the hits to Apache I've found two minor unrelated issues: an HTTP 404 for an attempt to load the profile of the user whose id is equal to the rev number for the GFX: 192.168.0.200 - - [12/Jun/2013:22:18:24 +0200] "GET /moodle-master/user/profile.php?id=1370780391 HTTP/1.1" 404 17034 "http://vm-centos5/moodle-master/course/modedit.php?add=resource&type=&course=2&section=2&return=0&sr=0" "Mozilla/4.0 (compatible; MSIE 8.0; Windows NT 5.1; Trident/4.0)" a consequent error in profile.php : [Wed Jun 12 22:18:24 2013] [error] [client 192.168.0.200] PHP Notice: Trying to get property of non-object in /var/www/moodle-master-git-remote/user/profile.php on line 61, referer: http://vm-centos5/moodle-master/course/modedit.php?add=resource&type=&course=2&section=2&return=0&sr=0 [Wed Jun 12 22:18:24 2013] [error] [client 192.168.0.200] PHP Notice: Trying to get property of non-object in /var/www/moodle-master-git-remote/user/profile.php on line 69, referer: http://vm-centos5/moodle-master/course/modedit.php?add=resource&type=&course=2&section=2&return=0&sr=0
            Hide
            marina Marina Glancy added a comment -

            Hi Matteo, I don't think 'nosniff' needs to be here. It is used for stylesheets and scripts. Besides it is quite strange to include it when we actually send the wrong content types.
            Andrew, when I said about improvement in 2.6 I was answering Matteo's comment about wrong header is some other ajax files (see his comment right above mine). Obviously the content type patch in core_renderer_ajax should go in 2.5 if not even earlier versions.

            Show
            marina Marina Glancy added a comment - Hi Matteo, I don't think 'nosniff' needs to be here. It is used for stylesheets and scripts. Besides it is quite strange to include it when we actually send the wrong content types. Andrew, when I said about improvement in 2.6 I was answering Matteo's comment about wrong header is some other ajax files (see his comment right above mine). Obviously the content type patch in core_renderer_ajax should go in 2.5 if not even earlier versions.
            Hide
            matteo Matteo Scaramuccia added a comment -

            Hi Marina,
            a JSON payload is kind of a (piece of a) JS script, that's the reason why I was guessing to adopt it in an AJAX call.
            BTW, I'm with you especially when I found that the issue cannot be replicated using the plain vanilla IE8 provided by Microsoft. My opinion is that this issue should be closed as not replicable IF no one will provide the evidence of that, including the bits to replicate it in order to allow people to discover the best compromise|workaround|nice fix for this supposed issue.

            Show
            matteo Matteo Scaramuccia added a comment - Hi Marina, a JSON payload is kind of a (piece of a) JS script, that's the reason why I was guessing to adopt it in an AJAX call. BTW, I'm with you especially when I found that the issue cannot be replicated using the plain vanilla IE8 provided by Microsoft. My opinion is that this issue should be closed as not replicable IF no one will provide the evidence of that, including the bits to replicate it in order to allow people to discover the best compromise|workaround|nice fix for this supposed issue.
            Hide
            marina Marina Glancy added a comment -

            Let's just introduce IE tax like kogan did http://www.kogan.com/au/blog/new-internet-explorer-7-tax/ Pity we don't charge Moodle users and tax from 0 will be 0.

            Anyway, I think this should be fixed. What do our JS experts say?

            Show
            marina Marina Glancy added a comment - Let's just introduce IE tax like kogan did http://www.kogan.com/au/blog/new-internet-explorer-7-tax/ Pity we don't charge Moodle users and tax from 0 will be 0. Anyway, I think this should be fixed. What do our JS experts say?
            Hide
            smithrn Ryan Smith added a comment -

            For those that can reproduce this problem with IE8, did you try an IE8 reset? I'm curious on whether that "fixes" this problem. I've tried three different VMs with IE8 installed and it doesn't happen on any of the systems. I've seen the reset solve several strange IE8 problems with Moodle over the paste couple of years. The instructions for the reset are here:

            http://support.microsoft.com/kb/923737

            Show
            smithrn Ryan Smith added a comment - For those that can reproduce this problem with IE8, did you try an IE8 reset? I'm curious on whether that "fixes" this problem. I've tried three different VMs with IE8 installed and it doesn't happen on any of the systems. I've seen the reset solve several strange IE8 problems with Moodle over the paste couple of years. The instructions for the reset are here: http://support.microsoft.com/kb/923737
            Hide
            matteo Matteo Scaramuccia added a comment -

            Could MDL-36972 be an option to test this proposal, if MIME type is the real issue for MDL-36972 too?
            Unfortunately, I do not have an iPad.

            Show
            matteo Matteo Scaramuccia added a comment - Could MDL-36972 be an option to test this proposal, if MIME type is the real issue for MDL-36972 too? Unfortunately, I do not have an iPad.
            Hide
            robhogg Rob Hardy added a comment -

            This issue was reported by colleagues when using the file picker in IE 8. Have applied Matteo's patch, and can confirm that this has resolved the problem.

            Show
            robhogg Rob Hardy added a comment - This issue was reported by colleagues when using the file picker in IE 8. Have applied Matteo's patch, and can confirm that this has resolved the problem.
            Hide
            matteo Matteo Scaramuccia added a comment -

            Hi Rob,
            could you help us in identifying what part of the patch really address the issue?
            First you could comment out nosniff to see if this should be a key point in your case.
            Then you should take notes about the HTTP Headers of both the Request and the Response with and without the patch to help us in having a description of what a broken IE8 expects and what Moodle returns just in case to see if there is something more added by others (e.g. Proxy) then the Moodle code. To record and report back both Headers you could use Fiddler.

            TIA,
            Matteo

            Show
            matteo Matteo Scaramuccia added a comment - Hi Rob, could you help us in identifying what part of the patch really address the issue? First you could comment out nosniff to see if this should be a key point in your case. Then you should take notes about the HTTP Headers of both the Request and the Response with and without the patch to help us in having a description of what a broken IE8 expects and what Moodle returns just in case to see if there is something more added by others (e.g. Proxy) then the Moodle code. To record and report back both Headers you could use Fiddler . TIA, Matteo
            Hide
            robhogg Rob Hardy added a comment -

            OK, Matteo, I'll see if I can find some time to do that next week.

            Show
            robhogg Rob Hardy added a comment - OK, Matteo, I'll see if I can find some time to do that next week.
            Hide
            matteo Matteo Scaramuccia added a comment -

            Hi Rob,
            any chance for the suggested tests&feedbacks?
            As you read, I'm unable to replicate the issue using a plain IE8 (from Microsoft VM) so to let the supposed fix go into the main stream, we need, as the very first step for the integration, to be able to collect the required data to confirm that the fix works as per user feedbacks and that the patch is addressing what required and described by the user by means of examing those data.

            TIA,
            Matteo

            Show
            matteo Matteo Scaramuccia added a comment - Hi Rob, any chance for the suggested tests&feedbacks? As you read, I'm unable to replicate the issue using a plain IE8 (from Microsoft VM) so to let the supposed fix go into the main stream, we need, as the very first step for the integration, to be able to collect the required data to confirm that the fix works as per user feedbacks and that the patch is addressing what required and described by the user by means of examing those data. TIA, Matteo
            Hide
            robhogg Rob Hardy added a comment -

            Hi Matteo,

            Sorry about the delay. I've tested on a PC in our office that has IE8 (build 8.0.7600.16385), but can't replicate the problem from there, so I'll have to find out where it was happening.

            Show
            robhogg Rob Hardy added a comment - Hi Matteo, Sorry about the delay. I've tested on a PC in our office that has IE8 (build 8.0.7600.16385), but can't replicate the problem from there, so I'll have to find out where it was happening.
            Hide
            smithrn Ryan Smith added a comment -

            If anybody has a system with the problem, can you try the IE reset I posted about in these comments in June? I'm really interested to see if a reset on the system fixes the problem. I'm still unable to replicate it on test VMs on my end.

            Show
            smithrn Ryan Smith added a comment - If anybody has a system with the problem, can you try the IE reset I posted about in these comments in June? I'm really interested to see if a reset on the system fixes the problem. I'm still unable to replicate it on test VMs on my end.
            Hide
            sterud Stefan Rudin added a comment -

            We are using IE9 and we had the Problem after updating to Moodle 2.5.
            We used the hack under Description to fix the point.

            Show
            sterud Stefan Rudin added a comment - We are using IE9 and we had the Problem after updating to Moodle 2.5. We used the hack under Description to fix the point.
            Hide
            matteo Matteo Scaramuccia added a comment -

            Hi Stefan,
            if you read all the comments you'll see that the hack described in the description could not be considered as the real solution: an alternative proposal has been coded (2.5: https://github.com/scara/moodle/commit/c976aeac30831d8730aff32274e7220f2c671e6c) but nothing happened in the main stream since no one of the people like me or Moodle HQ people has been able to replicate the issue.
            You could help us describing:

            1. the network environment you're using to access Moodle (Proxy, Reverse Proxy, ...);
            2. if resetting IE settings will solve your issue;
            3. if applying the proposed patch works the same as the hack in the description;
            4. if selectively applying the patch works fine too: details here.

            TIA,
            Matteo

            Show
            matteo Matteo Scaramuccia added a comment - Hi Stefan, if you read all the comments you'll see that the hack described in the description could not be considered as the real solution: an alternative proposal has been coded (2.5: https://github.com/scara/moodle/commit/c976aeac30831d8730aff32274e7220f2c671e6c ) but nothing happened in the main stream since no one of the people like me or Moodle HQ people has been able to replicate the issue. You could help us describing: the network environment you're using to access Moodle (Proxy, Reverse Proxy, ...); if resetting IE settings will solve your issue; if applying the proposed patch works the same as the hack in the description; if selectively applying the patch works fine too: details here . TIA, Matteo
            Hide
            sterud Stefan Rudin added a comment -

            Hi Matteo

            1.the network environment you're using to access Moodle (Proxy, Reverse Proxy, ...);

            We have a very basic network setup - no Proxy no Reverse Proxy.

            2.if resetting IE settings will solve your issue;

            We have no admin rights on our window clients. So I can not test this fix.

            3.if applying the proposed patch works the same as the hack in the description;

            The patch works for the fileupload. After applying the patch the picutres
            primarly the background pictures applied with CSS ar not displayd any more.
            After I get this result I stoppd with testing.

            Stefan

            Show
            sterud Stefan Rudin added a comment - Hi Matteo 1.the network environment you're using to access Moodle (Proxy, Reverse Proxy, ...); We have a very basic network setup - no Proxy no Reverse Proxy. 2.if resetting IE settings will solve your issue; We have no admin rights on our window clients. So I can not test this fix. 3.if applying the proposed patch works the same as the hack in the description; The patch works for the fileupload. After applying the patch the picutres primarly the background pictures applied with CSS ar not displayd any more. After I get this result I stoppd with testing. Stefan
            Hide
            matteo Matteo Scaramuccia added a comment -

            Hi Stefan,
            TNX for your time.
            Question: did you add the changes just into the core_renderer_ajax::header function?
            CSS files are managed via bundling and minification (theme/yui_combo.php) and CSS images are served via theme/yui_image.php, which are files not affected by any change in the function above.

            Show
            matteo Matteo Scaramuccia added a comment - Hi Stefan, TNX for your time. Question: did you add the changes just into the core_renderer_ajax::header function? CSS files are managed via bundling and minification ( theme/yui_combo.php ) and CSS images are served via theme/yui_image.php , which are files not affected by any change in the function above.
            Hide
            sterud Stefan Rudin added a comment -

            Hi Matteo
            I just replaced the file outputrenderers.php under moodle/lib/ with the one here:
            https://raw.github.com/scara/moodle/c976aeac30831d8730aff32274e7220f2c671e6c/lib/outputrenderers.php
            Is there more to do?
            Stefan

            Show
            sterud Stefan Rudin added a comment - Hi Matteo I just replaced the file outputrenderers.php under moodle/lib/ with the one here: https://raw.github.com/scara/moodle/c976aeac30831d8730aff32274e7220f2c671e6c/lib/outputrenderers.php Is there more to do? Stefan
            Hide
            matteo Matteo Scaramuccia added a comment - - edited

            To be safe, you should patch the file - e.g.: curl https://github.com/scara/moodle/commit/c976aeac30831d8730aff32274e7220f2c671e6c.patch | git apply --stat -. Remove stat option to apply the patch and not just seeing the stats for this patch applying - and not replace the whole file with the one from my branch: more commits have been provided since my proposal and I never rebased it waiting for the right time, when the code could be integrated.

            You could try to revert to the original file and then search for the core_renderer_ajax class and, within it, search for the header method and replace it with the following code which represents the patched method:

                /**
                 * Prepares the start of an AJAX output.
                 */
                public function header() {
                    // Unfortunately YUI iframe upload does not support application/json.
                    if (!empty($_FILES)) {
                        @header('Content-type: text/plain; charset=utf-8');
                    } else if (isset($_SERVER['HTTP_ACCEPT']) &&
                        (strpos($_SERVER['HTTP_ACCEPT'], 'application/json') !== false)) {
                        @header('Content-type: application/json; charset=utf-8');
                    } else {
                        // Safest fall back.
                        @header('Content-type: text/plain; charset=utf-8');
                    }
                    // Disable one of the IE magics.
                    // Ref.: http://msdn.microsoft.com/en-us/library/ie/gg622941%28v=vs.85%29.aspx
                    @header('X-Content-Type-Options: nosniff');
             
                    // 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');
                }
            

            This way of patching should change what required correctly merging the changes into your Moodle file version.

            HTH,
            Matteo

            Show
            matteo Matteo Scaramuccia added a comment - - edited To be safe, you should patch the file - e.g.: curl https://github.com/scara/moodle/commit/c976aeac30831d8730aff32274e7220f2c671e6c.patch | git apply --stat - . Remove stat option to apply the patch and not just seeing the stats for this patch applying - and not replace the whole file with the one from my branch: more commits have been provided since my proposal and I never rebased it waiting for the right time, when the code could be integrated. You could try to revert to the original file and then search for the core_renderer_ajax class and, within it, search for the header method and replace it with the following code which represents the patched method: /** * Prepares the start of an AJAX output. */ public function header() { // Unfortunately YUI iframe upload does not support application/json. if (!empty($_FILES)) { @header('Content-type: text/plain; charset=utf-8'); } else if (isset($_SERVER['HTTP_ACCEPT']) && (strpos($_SERVER['HTTP_ACCEPT'], 'application/json') !== false)) { @header('Content-type: application/json; charset=utf-8'); } else { // Safest fall back. @header('Content-type: text/plain; charset=utf-8'); } // Disable one of the IE magics. // Ref.: http://msdn.microsoft.com/en-us/library/ie/gg622941%28v=vs.85%29.aspx @header('X-Content-Type-Options: nosniff');   // 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'); } This way of patching should change what required correctly merging the changes into your Moodle file version. HTH, Matteo
            Hide
            marina Marina Glancy added a comment -

            It looks like the suggested patch works. Matteo, do you want to rebase (if it is not already rebased) and submit for integration?

            Show
            marina Marina Glancy added a comment - It looks like the suggested patch works. Matteo, do you want to rebase (if it is not already rebased) and submit for integration?
            Hide
            sterud Stefan Rudin added a comment -

            Hi Matteo
            I applied the new version from the public function header().
            Know it works and I have no issue with pictures.
            Great!
            Stefan

            Show
            sterud Stefan Rudin added a comment - Hi Matteo I applied the new version from the public function header(). Know it works and I have no issue with pictures. Great! Stefan
            Hide
            matteo Matteo Scaramuccia added a comment -

            @Marina: will do my evening.
            @Stefan: TNX for your testing.

            Show
            matteo Matteo Scaramuccia added a comment - @Marina: will do my evening. @Stefan: TNX for your testing.
            Hide
            matteo Matteo Scaramuccia added a comment -

            Rebased.
            @Integrators:

            1. the "safest fallback" is often used. This is due to the fact that most (all?) of the requests comes with Accept:*/* even with browsers like Chrome. Maybe the JS charged to be the Moodle AJAX client should be improved too to add the right HTTP Header for Accept when using "modern" browsers. I'm not an expert in Moodle JS framework but I think that the client above are actually several "clients" e.g. one of them is configured in lib/form/filemanager.js and the Accept header hacking (locally via e.g. Y.io.header or globally via merging a global configuration) should be applied several times; maybe YUI is natively able to support the Accept Request Header but I was not able to find any documentation about it... again, I'm not an expert of YUI
            2. there are still places where the AJAX renderer is not used e.g. lib/ajax/getnavbranch.php
            3. Maybe this should land first in 2.6 and then backported, to decide about the "safest fallback" being used more than expected (read here the original proposal)
            Show
            matteo Matteo Scaramuccia added a comment - Rebased. @Integrators: the "safest fallback" is often used. This is due to the fact that most (all?) of the requests comes with Accept:* / * even with browsers like Chrome. Maybe the JS charged to be the Moodle AJAX client should be improved too to add the right HTTP Header for Accept when using "modern" browsers. I'm not an expert in Moodle JS framework but I think that the client above are actually several "clients" e.g. one of them is configured in lib/form/filemanager.js and the Accept header hacking (locally via e.g. Y.io.header or globally via merging a global configuration) should be applied several times; maybe YUI is natively able to support the Accept Request Header but I was not able to find any documentation about it... again, I'm not an expert of YUI there are still places where the AJAX renderer is not used e.g. lib/ajax/getnavbranch.php Maybe this should land first in 2.6 and then backported, to decide about the "safest fallback" being used more than expected (read here the original proposal)
            Hide
            matteo Matteo Scaramuccia added a comment -

            Rebased, in case it will be evaluated the next week (please, take care of my notes before integrating it).

            Show
            matteo Matteo Scaramuccia added a comment - Rebased, in case it will be evaluated the next week (please, take care of my notes before integrating it).
            Hide
            danmarsden Dan Marsden added a comment -

            Hi Matteo - you should push it through for peer review if you think it is ready.

            Show
            danmarsden Dan Marsden added a comment - Hi Matteo - you should push it through for peer review if you think it is ready.
            Hide
            matteo Matteo Scaramuccia added a comment -

            Hi Andrew,
            could you review the current proposal, taking care of the doubts I've summarized here too? My doubts are related to the fact that we should open more issues to address cleanups on both client side code and ajax renderer adoptions.

            TIA,
            Matteo

            P.S.: @Dan: TNX for having recalled me to push the button !

            Show
            matteo Matteo Scaramuccia added a comment - Hi Andrew, could you review the current proposal, taking care of the doubts I've summarized here too? My doubts are related to the fact that we should open more issues to address cleanups on both client side code and ajax renderer adoptions. TIA, Matteo P.S.: @Dan: TNX for having recalled me to push the button !
            Hide
            dobedobedoh Andrew Nicols added a comment -

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

            Sorry for the amount of nos - it's mostly there, but I have a few concerns and some of them just cover multiple areas

            Documentation:

            • No need to change the comment about YUI iframe - makes git blame a pain
            • Ditto with the comment on line 3479
            • The code checker will probably complain about missing trailing punctuation on the URL. Not sure what the best solution is there though :/

            Whitespace:

            • indentation on the second part of the isset() test on line 3469 is incorrect - should be double-indented according to coding guidelines

            Syntax:

            • Does the nosniff option cause any issues for other browsers - perhaps we should use the browser tests we have and only apply this for IE? I'm not keen on applying such a browser-specific hack to all browser in case there are other consequences
            • You also state that the safest fallback is to use text/plain, but since IE is the only browser affected and it is in breach of RFCs (no surprise there) is this really the safest? Surely the safe fallback is application/json given the issues with content-aware proxies that instigated the original change and we should force non-compliance only for dodgy browsers? I just feel that we may be doing the slightly backwards. Or it could just be that the comment is inappropriate and that I'm objecting to the phrase 'Safest fallback'

            Git:

            • Is this an IE 8 specific issue? There was a report of it affecting IE9 too
            • The commit messages doesn't really say what the change is, just a description of the problem which is confusing for history

            Testing:

            • We need to check in other browsers. Any change like this should be tested in as many browsers as possible, including mobile and things like Opera
            • Do we also need to try and test with any other web servers, or any other server configurations?

            As I say, most of these are minor issues and just require some clarification

            Show
            dobedobedoh Andrew Nicols added a comment - [N] Syntax [N] Whitespace [Y] Output [-] Language [-] Databases [N] Testing [Y] Security [-] Documentation [N] Git [N] Sanity check Sorry for the amount of nos - it's mostly there, but I have a few concerns and some of them just cover multiple areas Documentation: No need to change the comment about YUI iframe - makes git blame a pain Ditto with the comment on line 3479 The code checker will probably complain about missing trailing punctuation on the URL. Not sure what the best solution is there though :/ Whitespace: indentation on the second part of the isset() test on line 3469 is incorrect - should be double-indented according to coding guidelines Syntax: Does the nosniff option cause any issues for other browsers - perhaps we should use the browser tests we have and only apply this for IE? I'm not keen on applying such a browser-specific hack to all browser in case there are other consequences You also state that the safest fallback is to use text/plain, but since IE is the only browser affected and it is in breach of RFCs (no surprise there) is this really the safest? Surely the safe fallback is application/json given the issues with content-aware proxies that instigated the original change and we should force non-compliance only for dodgy browsers? I just feel that we may be doing the slightly backwards. Or it could just be that the comment is inappropriate and that I'm objecting to the phrase 'Safest fallback' Git: Is this an IE 8 specific issue? There was a report of it affecting IE9 too The commit messages doesn't really say what the change is, just a description of the problem which is confusing for history Testing: We need to check in other browsers. Any change like this should be tested in as many browsers as possible, including mobile and things like Opera Do we also need to try and test with any other web servers, or any other server configurations? As I say, most of these are minor issues and just require some clarification
            Hide
            marccouture Marc Couture added a comment -

            We are experiencing the same problem with many of our users. Tested using IE8 and IE9. The problem also occurs in IE10 if a Moodle site is displayed in its "compatibility mode". We are implementing the hack described in the tracker description while awaiting a permanent fix, as this bug is a show-stopper for our users.

            Show
            marccouture Marc Couture added a comment - We are experiencing the same problem with many of our users. Tested using IE8 and IE9. The problem also occurs in IE10 if a Moodle site is displayed in its "compatibility mode". We are implementing the hack described in the tracker description while awaiting a permanent fix, as this bug is a show-stopper for our users.
            Hide
            andyd Andrew Dalrymple added a comment -

            Same issue, using Moodle 2.5.2+ and IE9.0.8112.16421
            Working fine in Firefox 24.0

            Show
            andyd Andrew Dalrymple added a comment - Same issue, using Moodle 2.5.2+ and IE9.0.8112.16421 Working fine in Firefox 24.0
            Hide
            matteo Matteo Scaramuccia added a comment -

            Hi Andrew,
            TNX for your time spent on my request!
            I'm starting looking at your review: sorry for my long delay here.

            X-Content-Type-Options: nosniff: you're right, it could be overkilling and it impacts not only on IE since Chromium has its own implementation (with some ongoing refinements) and Firefox is trying to get it too. Besides someone is thinking/trying to create a specification around this "custom" header.
            It's worth noting that GitHub (using Varnish, among other things) makes strong usage of that when viewing the "raw" file, e.g.:

            # curl --head https://raw.github.com/moodle/moodle/master/theme/standard/pix/screenshot.png
            HTTP/1.1 200 OK
            Date: Thu, 17 Oct 2013 13:18:52 GMT
            Server: GitHub.com
            Content-Type: image/png
            Status: 200 OK
            X-RateLimit-Limit: 100
            X-RateLimit-Remaining: 100
            Strict-Transport-Security: max-age=2592000
            X-Frame-Options: deny
            Access-Control-Allow-Origin: https://render.github.com
            X-Content-Type-Options: nosniff
            Content-Disposition: inline
            Content-Transfer-Encoding: binary
            X-Runtime: 46
            ETag: "b88349f0fbb4cd740fb3953009385ed0"
            X-GitHub-Request-Id: B91F1015:1FA2:2CD17C0:525FE3BB
            Content-Length: 78664
            Accept-Ranges: bytes
            Via: 1.1 varnish
            Age: 0
            X-Served-By: cache-am70-AMS
            X-Cache: MISS
            X-Cache-Hits: 0
            Vary: Accept-Encoding
            Cache-Control: private

            The same applies when accessing that resource with both Firefox and Chrome.

            Given your review and the new comments, I'll try to replicate the issue again: here the difficult is that I'm not able to replicate the issue and I'm trying to collect the experience of others than me.

            I'll rebase the proposal and make changes according to your notes too.

            Show
            matteo Matteo Scaramuccia added a comment - Hi Andrew, TNX for your time spent on my request! I'm starting looking at your review: sorry for my long delay here. X-Content-Type-Options: nosniff : you're right, it could be overkilling and it impacts not only on IE since Chromium has its own implementation (with some ongoing refinements) and Firefox is trying to get it too . Besides someone is thinking/trying to create a specification around this "custom" header. It's worth noting that GitHub (using Varnish , among other things) makes strong usage of that when viewing the "raw" file, e.g.: # curl --head https://raw.github.com/moodle/moodle/master/theme/standard/pix/screenshot.png HTTP/1.1 200 OK Date: Thu, 17 Oct 2013 13:18:52 GMT Server: GitHub.com Content-Type: image/png Status: 200 OK X-RateLimit-Limit: 100 X-RateLimit-Remaining: 100 Strict-Transport-Security: max-age=2592000 X-Frame-Options: deny Access-Control-Allow-Origin: https://render.github.com X-Content-Type-Options: nosniff Content-Disposition: inline Content-Transfer-Encoding: binary X-Runtime: 46 ETag: "b88349f0fbb4cd740fb3953009385ed0" X-GitHub-Request-Id: B91F1015:1FA2:2CD17C0:525FE3BB Content-Length: 78664 Accept-Ranges: bytes Via: 1.1 varnish Age: 0 X-Served-By: cache-am70-AMS X-Cache: MISS X-Cache-Hits: 0 Vary: Accept-Encoding Cache-Control: private The same applies when accessing that resource with both Firefox and Chrome. Given your review and the new comments, I'll try to replicate the issue again: here the difficult is that I'm not able to replicate the issue and I'm trying to collect the experience of others than me. I'll rebase the proposal and make changes according to your notes too.
            Hide
            matteo Matteo Scaramuccia added a comment -

            Rebased, code changed to be safer in terms of not hurting those browsers not affected by this issue and hopefully fixed the other things written in Andrew's peer review.
            I'm requesting another peer review.

            Show
            matteo Matteo Scaramuccia added a comment - Rebased, code changed to be safer in terms of not hurting those browsers not affected by this issue and hopefully fixed the other things written in Andrew's peer review. I'm requesting another peer review.
            Hide
            dobedobedoh Andrew Nicols added a comment -

            Just added expected behaviour to the testing instructions but otherwise good to go.
            Submitting for integration

            Show
            dobedobedoh Andrew Nicols added a comment - Just added expected behaviour to the testing instructions but otherwise good to go. Submitting for integration
            Hide
            dobedobedoh Andrew Nicols added a comment -

            Note to integrators, this issue does not affect 2.4.

            Show
            dobedobedoh Andrew Nicols added a comment - Note to integrators, this issue does not affect 2.4.
            Hide
            marina Marina Glancy added a comment - - edited

            Hi guys, that's awesome that you've figured out how to fix it without affecting all normal browsers! Just one suggestion - can we create a separate function in core_useragent for 2.6 so code looks neater. Hopefully we don't need to use it anywhere else though. Something like

            class core_useragent {
                public static function supports_json_contenttype() {
                    return !self::is_ie() ||
                            (self::check_ie_version(10) && !preg_match("/Trident\/([0-9\.]+)/", self::get_user_agent_string()));
                }
            }
            

            btw $match in preg_match is not needed here

            Show
            marina Marina Glancy added a comment - - edited Hi guys, that's awesome that you've figured out how to fix it without affecting all normal browsers! Just one suggestion - can we create a separate function in core_useragent for 2.6 so code looks neater. Hopefully we don't need to use it anywhere else though. Something like class core_useragent { public static function supports_json_contenttype() { return !self::is_ie() || (self::check_ie_version(10) && !preg_match("/Trident\/([0-9\.]+)/", self::get_user_agent_string())); } } btw $match in preg_match is not needed here
            Hide
            matteo Matteo Scaramuccia added a comment -

            Hi Marina,
            will do in the weekend: $match was half a missing because I've found confirmations about IE running in Compatibility View and the UA signature of the different IE versions, but as you written it's better to remove it unless useful in the near future. Same issue in 2.5 branch, will clean up together with your suggestions, fixing the conditions too.
            Note: the affected version are "IE < 10 or IE in Compatibility View" and not "IE < 10 and in Compatibility View".

            Show
            matteo Matteo Scaramuccia added a comment - Hi Marina, will do in the weekend: $match was half a missing because I've found confirmations about IE running in Compatibility View and the UA signature of the different IE versions, but as you written it's better to remove it unless useful in the near future. Same issue in 2.5 branch, will clean up together with your suggestions, fixing the conditions too. Note: the affected version are "IE < 10 or IE in Compatibility View" and not "IE < 10 and in Compatibility View".
            Hide
            marina Marina Glancy added a comment -

            Hi Matteo, thanks.

            PS I corrected my code sample

            Show
            marina Marina Glancy added a comment - Hi Matteo, thanks. PS I corrected my code sample
            Hide
            matteo Matteo Scaramuccia added a comment - - edited

            I'm trying to narrow down the selection of IE affected versions/modes:

            • IE 8 (revamping the VM): Compatibility View seems not to impact, IE 7 mode is incompatible with the file manager
            • IE 9: affected only in Compatibility View
            • IE 10: affected only in Compatibility View

            I'm considering "IE affected versions" as below:

            • IE < 8
            • IE in Compatibility View
              Note: Compatibility View is the default setting for "Intranet" sites, that's a possibile response to the "strange" distribution of those people affected by this issue.

            Replicated on IE9/10 Compatibility View and patch tested on master.

            Show
            matteo Matteo Scaramuccia added a comment - - edited I'm trying to narrow down the selection of IE affected versions/modes: IE 8 ( revamping the VM ): Compatibility View seems not to impact, IE 7 mode is incompatible with the file manager IE 9: affected only in Compatibility View IE 10: affected only in Compatibility View I'm considering "IE affected versions" as below: IE < 8 IE in Compatibility View Note: Compatibility View is the default setting for "Intranet" sites, that's a possibile response to the "strange" distribution of those people affected by this issue. Replicated on IE9/10 Compatibility View and patch tested on master .
            Hide
            matteo Matteo Scaramuccia added a comment - - edited

            Hi Andrew,
            back again asking for a new peer review.
            Thanks to Marina's suggestion, I've decided, before doing any further code change, to try again to replicate the issue and... I've been successful, being Compatibility View the infamous culprit and I've added Marina's code as well as (definitively? ) fixed my previous proposal.
            I've revamped the usage of X-Content-Type-Options: nosniff 'cause of it is required - as thought in one of my first comments - to avoid IE prompting to save the repository_ajax.php file instead of reading&loading its JSON payload. I'm still using text/plain with IE: I find it more pertinent and less harmful than text/html, which could work without the need of nosniff.
            Until !supports_json_contenttype will be related to IE only, the nosniff adoption will not hurt any browser other than IE.

            Lines 3491-3493 in 2.6 and lines 3356-3358 in 2.5 are not required for this issue but available for future compatibility.

            My concerns are:

            1. Could we add X-Content-Type-Options: nosniff unconditionally when uploading files?
            2. Should we remove those 3 lines above since not related to this issue and probably not really useful in the near future?

            My own replies are:

            1. Yes, we could - see the example of raw.github.com - and we should - we'd save some CPU cycles - but I understand your reasons about adding Headers strictly when required;
            2. Yes, we could: I've tried to replicate a situation 1 that could require it but it should fail probably only in IE 7-.

            TIA,
            Matteo

            1 lib/ajax/getnavbranch.php:

            --- a/lib/ajax/getnavbranch.php
            +++ b/lib/ajax/getnavbranch.php
            @@ -124,6 +124,6 @@ if (empty($branch) || $branch->nodetype !== navigation_node::NODETYPE_BRANCH) {
             // Prepare an XML converter for the branch
             $converter->set_expandable($navigation->get_expandable());
             // Set XML headers
            -header('Content-type: text/plain; charset=utf-8');
            +echo $OUTPUT->header(); // Send headers.
             // Convert and output the branch as XML
             echo $converter->convert($branch);
            

            Show
            matteo Matteo Scaramuccia added a comment - - edited Hi Andrew, back again asking for a new peer review. Thanks to Marina's suggestion, I've decided, before doing any further code change, to try again to replicate the issue and... I've been successful, being Compatibility View the infamous culprit and I've added Marina's code as well as (definitively? ) fixed my previous proposal. I've revamped the usage of X-Content-Type-Options: nosniff 'cause of it is required - as thought in one of my first comments - to avoid IE prompting to save the repository_ajax.php file instead of reading&loading its JSON payload. I'm still using text/plain with IE: I find it more pertinent and less harmful than text/html , which could work without the need of nosniff . Until !supports_json_contenttype will be related to IE only, the nosniff adoption will not hurt any browser other than IE. Lines 3491-3493 in 2.6 and lines 3356-3358 in 2.5 are not required for this issue but available for future compatibility. My concerns are: Could we add X-Content-Type-Options: nosniff unconditionally when uploading files? Should we remove those 3 lines above since not related to this issue and probably not really useful in the near future? My own replies are: Yes, we could - see the example of raw.github.com - and we should - we'd save some CPU cycles - but I understand your reasons about adding Headers strictly when required; Yes, we could: I've tried to replicate a situation 1 that could require it but it should fail probably only in IE 7-. TIA, Matteo 1 lib/ajax/getnavbranch.php : --- a/lib/ajax/getnavbranch.php +++ b/lib/ajax/getnavbranch.php @@ -124,6 +124,6 @@ if (empty($branch) || $branch->nodetype !== navigation_node::NODETYPE_BRANCH) { // Prepare an XML converter for the branch $converter->set_expandable($navigation->get_expandable()); // Set XML headers -header('Content-type: text/plain; charset=utf-8'); +echo $OUTPUT->header(); // Send headers. // Convert and output the branch as XML echo $converter->convert($branch);
            Hide
            dobedobedoh Andrew Nicols added a comment -

            [Y] Syntax
            [Y] Whitespace
            [-] Output
            [-] Language
            [-] Databases
            [Y] Testing (instructions and automated tests)
            [Y] Security
            [Y] Documentation
            [Y] Git
            [-] Third party code
            [N] Sanity check

            Just clarified the testing instructions.

            Sanity check

            For the master version, in supports_json_contenttype(), I think that this should be broken into multiple lines. It's very confusing to read and work out what's happening when we return with an ||.

            It would improve the clarity and readability, but also mean that should we need to add any other tests in the future, it will be much easier and won't require as much messing with this. I think it should be of the form:

            // Appropriate comment...
            if (!foo()) {
                return true
            }
             
            // Appropriate comment here to explain why IE8+ is fine.
            if ((self::check_ie_version(8)) {
                return true
            }
             
            // Any other tests.
             
            // This browser does not support json.
            return false;
            

            Other than this, I think that this issue is good for integration.

            Show
            dobedobedoh Andrew Nicols added a comment - [Y] Syntax [Y] Whitespace [-] Output [-] Language [-] Databases [Y] Testing (instructions and automated tests) [Y] Security [Y] Documentation [Y] Git [-] Third party code [N] Sanity check Just clarified the testing instructions. Sanity check For the master version, in supports_json_contenttype(), I think that this should be broken into multiple lines. It's very confusing to read and work out what's happening when we return with an ||. It would improve the clarity and readability, but also mean that should we need to add any other tests in the future, it will be much easier and won't require as much messing with this. I think it should be of the form: // Appropriate comment... if (!foo()) { return true }   // Appropriate comment here to explain why IE8+ is fine. if ((self::check_ie_version(8)) { return true }   // Any other tests.   // This browser does not support json. return false; Other than this, I think that this issue is good for integration.
            Hide
            marina Marina Glancy added a comment -

            Matteo, it looks like there is some copy-pasted code in 2.5 that should not be working

            Show
            marina Marina Glancy added a comment - Matteo, it looks like there is some copy-pasted code in 2.5 that should not be working
            Hide
            matteo Matteo Scaramuccia added a comment -

            OK
            Will look this (better, my) evening on both branches: I've tested only the master branch and, as you said, copy-pasted for 2.5. Apologies!

            Show
            matteo Matteo Scaramuccia added a comment - OK Will look this (better, my) evening on both branches: I've tested only the master branch and, as you said, copy-pasted for 2.5. Apologies!
            Hide
            matteo Matteo Scaramuccia added a comment -

            @Andrew: core_useragent refactored to avoid code duplication. Quite verbose in documenting the Compatibility View mode.
            @Marina: fixed (apologies!) and tested on 2.5.2+ (Build: 20131018).

            Show
            matteo Matteo Scaramuccia added a comment - @Andrew: core_useragent refactored to avoid code duplication. Quite verbose in documenting the Compatibility View mode. @Marina: fixed (apologies!) and tested on 2.5.2+ (Build: 20131018) .
            Hide
            matteo Matteo Scaramuccia added a comment -

            Hopefully the last peer review request, if you'll agree with my refactoring.

            Show
            matteo Matteo Scaramuccia added a comment - Hopefully the last peer review request, if you'll agree with my refactoring.
            Hide
            dobedobedoh Andrew Nicols added a comment -

            Hi Matteo,

            I think we're good to go now. Thanks for all of your work on this!

            Submitting for integration,

            Andrew

            Show
            dobedobedoh Andrew Nicols added a comment - Hi Matteo, I think we're good to go now. Thanks for all of your work on this! Submitting for integration, Andrew
            Hide
            matteo Matteo Scaramuccia added a comment -

            TNX Andrew!
            In the mean time I've made a small change in a comment, s/call/response/: I'm not native english but I think it sounds better from a technical perspective since the main issue here is - long version - using 'text/plain' in Compatibility View for the body of an HTTP POST response with a valid JSON payload, due to a multipart/form-data encoded data submission.

            Show
            matteo Matteo Scaramuccia added a comment - TNX Andrew! In the mean time I've made a small change in a comment, s/call/response/ : I'm not native english but I think it sounds better from a technical perspective since the main issue here is - long version - using 'text/plain' in Compatibility View for the body of an HTTP POST response with a valid JSON payload, due to a multipart/form-data encoded data submission .
            Hide
            matteo Matteo Scaramuccia added a comment -

            Hi Marina,
            just read about MDL-42491: I'll wait for that being integrated and here I'll add tests for check_ie_compatibility_view() in https://github.com/moodle/moodle/blob/master/lib/tests/useragent_test.php. Could this be the preferred integration path for the iTeam?

            TIA,
            Matteo

            Show
            matteo Matteo Scaramuccia added a comment - Hi Marina, just read about MDL-42491 : I'll wait for that being integrated and here I'll add tests for check_ie_compatibility_view() in https://github.com/moodle/moodle/blob/master/lib/tests/useragent_test.php . Could this be the preferred integration path for the iTeam ? TIA, Matteo
            Hide
            matteo Matteo Scaramuccia added a comment -

            Added missing core_useragent::check_ie_compatibility_view() unit tests in 2.6.

            Show
            matteo Matteo Scaramuccia added a comment - Added missing core_useragent::check_ie_compatibility_view() unit tests in 2.6.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

            TIA and ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
            Hide
            matteo Matteo Scaramuccia added a comment - - edited

            Rebased.
            Question: I need to add IE11 tests for core_useragent::check_ie_compatibility_view(), just after the integration of MDL-42491. Two options here:

            • should I merge Michael work into this PR and then, on top of that, adding the required tests?
            • should iTeam integrate MDL-42491 first, then I'll rebased my PR on top of the integration repo and finally I'll add the missing tests?
            Show
            matteo Matteo Scaramuccia added a comment - - edited Rebased. Question: I need to add IE11 tests for core_useragent::check_ie_compatibility_view() , just after the integration of MDL-42491 . Two options here: should I merge Michael work into this PR and then, on top of that, adding the required tests? should iTeam integrate MDL-42491 first, then I'll rebased my PR on top of the integration repo and finally I'll add the missing tests?
            Hide
            damyon Damyon Wiese added a comment -

            Thanks Matteo,

            MDL-42491 is being tested now - so you can rebase on integration and add your test? In general though you can set another issue as a blocker and base your patch on top of the other issue. The only problem is that if the other issue gets modified (e.g. in integration like MDL-42491 did) you will have to rebase your branch again (or instruct the integrator to cherry pick if you don't think it will be affected).

            Cheers, Damyon

            Show
            damyon Damyon Wiese added a comment - Thanks Matteo, MDL-42491 is being tested now - so you can rebase on integration and add your test? In general though you can set another issue as a blocker and base your patch on top of the other issue. The only problem is that if the other issue gets modified (e.g. in integration like MDL-42491 did) you will have to rebase your branch again (or instruct the integrator to cherry pick if you don't think it will be affected). Cheers, Damyon
            Hide
            matteo Matteo Scaramuccia added a comment -

            TNX Damyon for the explanation. Michael has already added the blocker link: I've just rebased my PR and added the missing tests.
            I was unable to test it again except for successfully running the unit tests ($ vendor/bin/phpunit core_useragent_testcase lib/tests/useragent_test.php): when running on both IE9 and IE10 in Compatibility View the file picker (e.g.: Home > My profile > My private files > Add...) doesn't appear - just the "title bar"... - using the current master code, 2.6beta+ (Build: 20131025), and before applying my PR.

            Show
            matteo Matteo Scaramuccia added a comment - TNX Damyon for the explanation. Michael has already added the blocker link: I've just rebased my PR and added the missing tests. I was unable to test it again except for successfully running the unit tests ( $ vendor/bin/phpunit core_useragent_testcase lib/tests/useragent_test.php ): when running on both IE9 and IE10 in Compatibility View the file picker (e.g.: Home > My profile > My private files > Add... ) doesn't appear - just the "title bar"... - using the current master code, 2.6beta+ (Build: 20131025) , and before applying my PR.
            Hide
            damyon Damyon Wiese added a comment -

            Ok - back to this. I have a fix for the broken filepicker: MDL-42589 so we can test this patch.

            Show
            damyon Damyon Wiese added a comment - Ok - back to this. I have a fix for the broken filepicker: MDL-42589 so we can test this patch.
            Hide
            damyon Damyon Wiese added a comment -

            Thanks Matteo,

            Integrated to 25 and master. I tested this with the CSS fix for the filepicker and it seems to not break anything (reproducing the bug seems hard to do in the first place).

            Show
            damyon Damyon Wiese added a comment - Thanks Matteo, Integrated to 25 and master. I tested this with the CSS fix for the filepicker and it seems to not break anything (reproducing the bug seems hard to do in the first place).
            Hide
            damyon Damyon Wiese added a comment -

            Passing this test - tested in integration.

            Show
            damyon Damyon Wiese added a comment - Passing this test - tested in integration.
            Hide
            matteo Matteo Scaramuccia added a comment -

            Hi Damyon,
            TNX for the fix , just tested with IE9 in Compatibility Mode.

            Show
            matteo Matteo Scaramuccia added a comment - Hi Damyon, TNX for the fix , just tested with IE9 in Compatibility Mode .
            Hide
            poltawski Dan Poltawski added a comment -

            Congratulations - this issue has been included in Moodle and is now available on our git mirrors and shortly will become available on the download servers shortly.

            Show
            poltawski Dan Poltawski added a comment - Congratulations - this issue has been included in Moodle and is now available on our git mirrors and shortly will become available on the download servers shortly.

              People

              • Votes:
                10 Vote for this issue
                Watchers:
                21 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  11/Nov/13