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

      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.

        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: