Moodle
  1. Moodle
  2. MDL-42491

IE11 reports its useragent differently and breaks browser sniffing

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 2.4.6, 2.5.2
    • Fix Version/s: 2.4.7, 2.5.3
    • Component/s: Libraries
    • Labels:
    • Testing Instructions:
      Hide
      1. Run unit tests
        • 24 & 25 in lib\tests\moodlelib_test.php
        • master in lib\tests\useragent_test.php
      2. Repeat the following in an earlier IE (8,9 or 10), IE11 and at least one other browser...
        • Check the spell checker works
        • Export and Excel file from the bulk user download facility
        • Check the media player works with a variety of audio and video formats
      Show
      Run unit tests 24 & 25 in lib\tests\moodlelib_test.php master in lib\tests\useragent_test.php Repeat the following in an earlier IE (8,9 or 10), IE11 and at least one other browser... Check the spell checker works Export and Excel file from the bulk user download facility Check the media player works with a variety of audio and video formats
    • Affected Branches:
      MOODLE_24_STABLE, MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE, MOODLE_25_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      wip-mdl-42491-master

      Description

      IE11 reports its useragent as...

      Mozilla/5.0 (Windows NT 6.3; WOW64; Trident/7.0; rv:11.0) like Gecko
      

      Currently the browser sniffing relies on the IE useragent string including 'MSIE' as was the case in previous versions.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            Michael de Raadt added a comment -

            I found some sources reporting on the changes to IE11's useragent string.

            This change will need to be backported to earlier versions. I'll do that after review.

            I'm not sure my regular expression is the best for picking out version. It works, but perhaps it could be more generic in relation to the characters between the Trident version and the rv version, and perhaps even in relation to their relative positions. Feedback on that would be welcomed.

            Show
            Michael de Raadt added a comment - I found some sources reporting on the changes to IE11's useragent string. http://msdn.microsoft.com/en-us/library/ie/bg182625%28v=vs.85%29.aspx http://blogs.msdn.com/b/ieinternals/archive/2013/09/21/internet-explorer-11-user-agent-string-ua-string-sniffing-compatibility-with-gecko-webkit.aspx This change will need to be backported to earlier versions. I'll do that after review. I'm not sure my regular expression is the best for picking out version. It works, but perhaps it could be more generic in relation to the characters between the Trident version and the rv version, and perhaps even in relation to their relative positions. Feedback on that would be welcomed.
            Hide
            Iñaki Arenaza added a comment -

            Hi Michael,

            assuming you won't have two (or more) 'rv:XX.X' items in the UA string, you could use something like:

                     if (preg_match("/MSIE ([0-9\.]+)/", $useragent, $match)) {
                         $browser = $match[1];
                     } else if (preg_match('/Trident\/[0-9\.]+/', $useragent) && preg_match('/rv:([0-9\.]+)/', $useragent, $match)) {
                        $browser = $match[1];
                     } else {
                         return false;
                     } 
            

            Saludos.
            Iñaki.

            Show
            Iñaki Arenaza added a comment - Hi Michael, assuming you won't have two (or more) 'rv:XX.X' items in the UA string, you could use something like: if (preg_match("/MSIE ([0-9\.]+)/", $useragent, $match)) { $browser = $match[1]; } else if (preg_match('/Trident\/[0-9\.]+/', $useragent) && preg_match('/rv:([0-9\.]+)/', $useragent, $match)) { $browser = $match[1]; } else { return false; } Saludos. Iñaki.
            Hide
            Michael de Raadt added a comment -

            Thanks for that, Iñaki.

            Feel free to take this a step further and complete a peer review.

            Show
            Michael de Raadt added a comment - Thanks for that, Iñaki. Feel free to take this a step further and complete a peer review .
            Hide
            Damyon Wiese added a comment -

            Thanks Michael,

            This looks good - from the docs on the windows user agent string this looks like the correct way to match it. One minor point - you don't really need the first match in the regex. One major point - IMO it's a bug and needs backporting (people will use ie11 with stable Moodle versions too).

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

            Show
            Damyon Wiese added a comment - Thanks Michael, This looks good - from the docs on the windows user agent string this looks like the correct way to match it. One minor point - you don't really need the first match in the regex. One major point - IMO it's a bug and needs backporting (people will use ie11 with stable Moodle versions too). [Y] Syntax [Y] Whitespace [-] Output [-] Language [-] Databases [Y] Testing (instructions and automated tests) [-] Security [-] Documentation [Y] Git [-] Third party code [N] Sanity check (needs backport).
            Hide
            Michael de Raadt added a comment -

            Thanks for the peer review Damyon.

            I think it is important to find "Trident" in the useragent string. Getting the Trident version is not important. I have split this regular expression into two as Iñaki suggested.

            I've backported this change to 2.4 and 2.5 (as I mentioned I would) and squashed the master branch into a single commit.

            Show
            Michael de Raadt added a comment - Thanks for the peer review Damyon. I think it is important to find "Trident" in the useragent string. Getting the Trident version is not important. I have split this regular expression into two as Iñaki suggested. I've backported this change to 2.4 and 2.5 (as I mentioned I would) and squashed the master branch into a single commit.
            Hide
            Michael de Raadt added a comment -

            I've had a chat with Damyon about a couple of things and I believe this is ready for integration.

            Show
            Michael de Raadt added a comment - I've had a chat with Damyon about a couple of things and I believe this is ready for integration.
            Hide
            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
            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 Scaramuccia added a comment -

            FYI: my potential catch-22, for MDL-39810.

            Show
            Matteo Scaramuccia added a comment - FYI: my potential catch-22 , for MDL-39810 .
            Hide
            Michael de Raadt added a comment -

            I've put this issue up as blocking for MDL-39810.

            Show
            Michael de Raadt added a comment - I've put this issue up as blocking for MDL-39810 .
            Hide
            Dan Poltawski added a comment -

            Thanks Michael, integrated to master, 25 and 24

            Show
            Dan Poltawski added a comment - Thanks Michael, integrated to master, 25 and 24
            Hide
            Rossiani Wijaya added a comment -

            This is working as expected.

            Tested for 2.4, 2.5, and master with ie 8,9,10 and 11.

            Phpunit test for 2.4, 2.5 and 2.6 passed.

            Test passed.

            Show
            Rossiani Wijaya added a comment - This is working as expected. Tested for 2.4, 2.5, and master with ie 8,9,10 and 11. Phpunit test for 2.4, 2.5 and 2.6 passed. Test passed.
            Hide
            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
            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:
                2 Vote for this issue
                Watchers:
                6 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved: