Moodle
  1. Moodle
  2. MDL-31482

get_device_type incorrectly detects IE7/8 as IE6 (legacy)

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.1.4, 2.2.1, 2.3
    • Fix Version/s: 2.1.5, 2.2.2
    • Component/s: Libraries
    • Labels:
    • Testing Instructions:
      Hide

      To test, you need a VM (or real computer I guess) that is set up to run IE 7 or IE 8. You will also need another VM that is set up to run the real IE 6.

      It may also be possible to test this using the IETester software, which I think lets you configure user agents. I'm giving real-computer instructions for this test script.

      (On the IE 7/8 system)

      1) Run the registry editor (regedit) and navigate to:
      HKEY_LOCAL_MACHINE\SOFTWARE\Microsoft\Windows\CurrentVersion\Internet Settings\User Agent\Post Platform

      2) Right click and choose New / String value. An untitled string value REG_SZ should appear.

      3) Type 'MSIE 6.99' for the value, hit return.

      4) If you have a browser open, close it.

      (On your real machine)

      5) In your Moodle settings (Appearance / Theme / Theme selector), select a different theme for 'Legacy' device type compared to default, so you can tell when it is in use. For example, you could choose 'Serenity', which is probably ugly enough that you'll notice.

      (on the IE7 machine)

      6) Open IE 7 and visit your test server.
      + Verify that your normal theme (not Serenity) is in use.

      (on the IE6 machine)

      7) Open IE6 and visit your test server.
      + Verify that the Serenity theme is in use.

      X) also test unit tests @ /lib/simpletest/testmoodlelib.php ( only 1 expected fail. read comments from SamM below)

      Show
      To test, you need a VM (or real computer I guess) that is set up to run IE 7 or IE 8. You will also need another VM that is set up to run the real IE 6. It may also be possible to test this using the IETester software, which I think lets you configure user agents. I'm giving real-computer instructions for this test script. (On the IE 7/8 system) 1) Run the registry editor (regedit) and navigate to: HKEY_LOCAL_MACHINE\SOFTWARE\Microsoft\Windows\CurrentVersion\Internet Settings\User Agent\Post Platform 2) Right click and choose New / String value. An untitled string value REG_SZ should appear. 3) Type 'MSIE 6.99' for the value, hit return. 4) If you have a browser open, close it. (On your real machine) 5) In your Moodle settings (Appearance / Theme / Theme selector), select a different theme for 'Legacy' device type compared to default, so you can tell when it is in use. For example, you could choose 'Serenity', which is probably ugly enough that you'll notice. (on the IE7 machine) 6) Open IE 7 and visit your test server. + Verify that your normal theme (not Serenity) is in use. (on the IE6 machine) 7) Open IE6 and visit your test server. + Verify that the Serenity theme is in use. X) also test unit tests @ /lib/simpletest/testmoodlelib.php ( only 1 expected fail. read comments from SamM below)
    • Affected Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE, MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE
    • Pull Master Branch:
      MDL-31482-master
    • Rank:
      38012

      Description

      The get_device_type function includes this code:

          if (strpos($_SERVER['HTTP_USER_AGENT'], 'MSIE 6.') !== false) {
              return 'legacy';
          }
      

      Unfortunately this logic misdetects IE7/8 as IE6 under certain circumstances. In my testing, approximately 1.5% of users with MSIE7 or MSIE8 have user-agent strings that also contain the string 'MSIE 6.'.

      Here is an example user agent:

      Mozilla/4.0 (compatible; MSIE 8.0; Windows NT 5.1; Trident/4.0; BT Openworld BB; Mozilla/4.0 (compatible; MSIE 6.0; Windows NT 5.1; SV1) ; Hotbar 10.2.197.0; .NET CLR 3.0.4506.2152; .NET CLR 3.5.30729; .NET CLR 2.0.50727)

      Obviously this user agent string is completely broken. Agents like this (there are many different cases with no obvious common factors, other than the symptom of a 'nested' IE6 user agent) are common.

      Microsoft have fixed this problem in IE9 which doesn't add crap all over its user agent, so it is only MSIE 7 and MSIE 8 that are affected.

      (Yes we discovered this after doing something drastic so that our pages could sort of display in IE6... and then getting a bunch of helpdesk calls from irate IE7/8 users. Ooops.)

        Activity

        Hide
        Sam Marshall added a comment -

        My proposed solution is to check for an exact start of string. Based on our user data this should work (if you assume my browser analyser is working correctly).

        Here's the results of a check to verify this strategy. This check looks for all user agents (during a week in December) which contain the string 'MSIE 6.' (so would have been matched as legacy with the old/current version of code in moodlelib), but do NOT start with 'Mozilla/4.0 (compatible; MSIE 6.0;", and are also NOT MSIE 7 or 8 (ie the problematic results I'm trying to get rid of). In other words, I am trying to obtain the list of user agents which:

        • would previously have been treated as legacy
        • will not be treated as legacy under the new code
        • are not MSIE 7 or 8 (the ones this bug is specifically intended to remove from legacy treatment).
        $ cat 2011-12.useragents | grep -v ">Mozilla/4.0 (compatible; MSIE 6.0;" | grep "MSIE 6." | grep -v
         "MSIE 7" | grep -v "MSIE 8"
        <agent count='85' internal='1' external='84'>HTC_TyTN_II Mozilla/4.0 (compatible; MSIE 6.0; Windows
        CE; IEMobile 7.11)</agent>
        <agent count='4' internal='0' external='4'>Mozilla/4.0 (compatible- MSIE 6.0- Windows NT 5.1- SV1- .NET CLR 1.1.4322</agent>
        <agent count='6' internal='0' external='6'>Mozilla/4.0 (compatible; MSIE 6.1; Windows XP)</agent>
        <agent count='2' internal='0' external='2'>Outlook-Express/7.0 (MSIE 6.0; Windows NT 5.1; SV1; .NET
        CLR 2.0.50727; .NET CLR 3.0.4506.2152; .NET CLR 3.5.30729; TmstmpExt)</agent>
        <agent count='1' internal='1' external='0'>Xda_Stellar/ 240x320 (compatible; MSIE 6.0; Windows CE; IEMobile 7.11)</agent>
        <agent count='1' internal='0' external='1'>mAgent MSIE 6</agent>
        

        Looking at this list you can see that (a) none of them had more than a handful of page requests (that's the numbers), (b) with the possible exception of one string, none of them are actually IE 6. In other words I think this proposed change should work. Now I'd better do the code for it...

        Show
        Sam Marshall added a comment - My proposed solution is to check for an exact start of string. Based on our user data this should work (if you assume my browser analyser is working correctly). Here's the results of a check to verify this strategy. This check looks for all user agents (during a week in December) which contain the string 'MSIE 6.' (so would have been matched as legacy with the old/current version of code in moodlelib), but do NOT start with 'Mozilla/4.0 (compatible; MSIE 6.0;", and are also NOT MSIE 7 or 8 (ie the problematic results I'm trying to get rid of). In other words, I am trying to obtain the list of user agents which: would previously have been treated as legacy will not be treated as legacy under the new code are not MSIE 7 or 8 (the ones this bug is specifically intended to remove from legacy treatment). $ cat 2011-12.useragents | grep -v ">Mozilla/4.0 (compatible; MSIE 6.0;" | grep "MSIE 6." | grep -v "MSIE 7" | grep -v "MSIE 8" <agent count='85' internal='1' external='84'>HTC_TyTN_II Mozilla/4.0 (compatible; MSIE 6.0; Windows CE; IEMobile 7.11)</agent> <agent count='4' internal='0' external='4'>Mozilla/4.0 (compatible- MSIE 6.0- Windows NT 5.1- SV1- .NET CLR 1.1.4322</agent> <agent count='6' internal='0' external='6'>Mozilla/4.0 (compatible; MSIE 6.1; Windows XP)</agent> <agent count='2' internal='0' external='2'>Outlook-Express/7.0 (MSIE 6.0; Windows NT 5.1; SV1; .NET CLR 2.0.50727; .NET CLR 3.0.4506.2152; .NET CLR 3.5.30729; TmstmpExt)</agent> <agent count='1' internal='1' external='0'>Xda_Stellar/ 240x320 (compatible; MSIE 6.0; Windows CE; IEMobile 7.11)</agent> <agent count='1' internal='0' external='1'>mAgent MSIE 6</agent> Looking at this list you can see that (a) none of them had more than a handful of page requests (that's the numbers), (b) with the possible exception of one string, none of them are actually IE 6. In other words I think this proposed change should work. Now I'd better do the code for it...
        Hide
        Sam Marshall added a comment -

        Code done now.

        Show
        Sam Marshall added a comment - Code done now.
        Hide
        Anthony Forth added a comment -

        There are some IE6 user agents this won't catch.

        See http://www.useragentstring.com/pages/Internet%20Explorer/

        Show
        Anthony Forth added a comment - There are some IE6 user agents this won't catch. See http://www.useragentstring.com/pages/Internet%20Explorer/
        Hide
        Sam Marshall added a comment -

        Anthony: Are there? A lot of the agents on that page are bogus (for example, there is no such thing as IE 6.1).

        Show
        Sam Marshall added a comment - Anthony: Are there? A lot of the agents on that page are bogus (for example, there is no such thing as IE 6.1).
        Hide
        Sam Marshall added a comment -

        MSDN article here: http://msdn.microsoft.com/en-us/library/ms537503.aspx

        It doesn't mention different possibilities for that part of the string. I think it's likely that, unless molested (I think you can configure it in the registry to basically anything), IE6 will match the format I gave.

        Show
        Sam Marshall added a comment - MSDN article here: http://msdn.microsoft.com/en-us/library/ms537503.aspx It doesn't mention different possibilities for that part of the string. I think it's likely that, unless molested (I think you can configure it in the registry to basically anything), IE6 will match the format I gave.
        Hide
        Jason Fowler added a comment -

        Code looks good - makes sense.

        Show
        Jason Fowler added a comment - Code looks good - makes sense.
        Hide
        Sam Marshall added a comment -

        Thanks for review - submitting for integration.

        Show
        Sam Marshall added a comment - Thanks for review - submitting for integration.
        Hide
        Aparup Banerjee added a comment -

        added to git repository field

        Show
        Aparup Banerjee added a comment - added to git repository field
        Hide
        Aparup Banerjee added a comment -

        Hi SamM,
        given the list of agent here, it would be great to start off some unit tests for this function in lib/simpletest/testmooodlelib.php

        Show
        Aparup Banerjee added a comment - Hi SamM, given the list of agent here, it would be great to start off some unit tests for this function in lib/simpletest/testmooodlelib.php
        Hide
        Aparup Banerjee added a comment -

        reopening to get some unit tests in here.

        Show
        Aparup Banerjee added a comment - reopening to get some unit tests in here.
        Hide
        Sam Marshall added a comment -

        Agree, I have added a unit test that covers the two cases here (IE8 that was previously misrecognised as IE6, and a genuine IE6). All branches updated.

        Notes:

        1. Unit test does not cover other existing features of get_device_type.

        2. The unit tests for that file now pass except for one exception in the UTF-8 testing, which was failing before this change and is still failing after it...

        Show
        Sam Marshall added a comment - Agree, I have added a unit test that covers the two cases here (IE8 that was previously misrecognised as IE6, and a genuine IE6). All branches updated. Notes: 1. Unit test does not cover other existing features of get_device_type. 2. The unit tests for that file now pass except for one exception in the UTF-8 testing, which was failing before this change and is still failing after it...
        Hide
        Aparup Banerjee added a comment -

        Thanks Sam,
        thats been integrated into master, 22 and 21 now.
        I've added unit test verification (with the 1 failure) to test instructions too.
        cheers!

        Show
        Aparup Banerjee added a comment - Thanks Sam, thats been integrated into master, 22 and 21 now. I've added unit test verification (with the 1 failure) to test instructions too. cheers!
        Hide
        Michael de Raadt added a comment -

        Test result: Success. I had some trouble spoofing the user agent value through the registry, but I was able to test this with IETester. I can also confirm that IE7 and IE9 are not listed as legacy.

        Unit tests showed no problems related to the browser version.

        Show
        Michael de Raadt added a comment - Test result: Success. I had some trouble spoofing the user agent value through the registry, but I was able to test this with IETester. I can also confirm that IE7 and IE9 are not listed as legacy. Unit tests showed no problems related to the browser version.
        Hide
        Eloy Lafuente (stronk7) added a comment -

        A bit later this week, but finally your changes have been accepted and are now available in all the upstream git/cvs servers.

        Many thanks & ciao

        Show
        Eloy Lafuente (stronk7) added a comment - A bit later this week, but finally your changes have been accepted and are now available in all the upstream git/cvs servers. Many thanks & ciao

          People

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

            Dates

            • Created:
              Updated:
              Resolved: