Moodle
  1. Moodle
  2. MDL-38168

Filelib: Curl class does not honour proxybypass option

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 2.3.4
    • Fix Version/s: 2.3.5, 2.4.2
    • Component/s: Libraries
    • Labels:
    • Testing Instructions:
      Hide

      WARNING: This test is a bit complicated. I've written it so you can do the test even if you don't normally use a proxy server.

      We are going to use the SCORM activity because it is a convenient way to exercise the curl class and you can tell whether it's using the proxy or not.

      The test instructions contain special lines beginning 'MOODLE 2.3' referring to differences that are expected on that platform.

      1. Go to Site administration / Plugins / Activity modules / SCORM package. Turn on 'Enable external package type' (allowtypeexternal).

      MOODLE 2.3: In addition to this, also ensure you have set 'debugging' option to 'Developer' as otherwise you cannot see the errors that we check below.

      2. In the admin screens under Server/HTTP, or in your config.php (that would be necessary if you have already done 'real' settings in config.php), set the following options:

      proxyhost = does.not.work.example.com
      proxyport = 80
      proxytype = HTTP
      proxybypass = localhost, 127.0.0.1 (default)

      3. Go to any course page, turn editing on, and select to add a 'SCORM package'.

      4. Type anything for name and description. Under 'Type' choose 'External SCORM manifest'.

      5. Under 'URL' type a URL that should normally work without needing a proxy, e.g. the root path of your Moodle server followed by /imsmanifest.xml. For example http://my.moodle.server/imsmanifest.xml.

      6. Hit 'Save and return to course'.

      EXPECTED (except Moodle 2.3): Next to the URL field you should see the error 'Invalid URL specified' with the debug message: Couldn't resolve proxy 'does.not.work.example.com'.

      This indicates that the server was correctly using the http proxy settings (which aren't valid) to retrieve the item.

      MOODLE 2.3: On Moodle 2.3 we expect the following behaviour if it is working correctly: debug text similar to 'cURL request for "http://whatever/imsmanifest.xml" failed with: Couldn't resolve proxy 'does.not.work.example.com' (5)', followed by an undefined property error.

      7. In another tab, go back to the HTTP admin settings and change proxybypass so that it includes the hostname part of the URL you are trying to retrieve; e.g. if the URL is http://my.moodle.server/imsmanifest.xml then you could add ".moodle.server" to the comma-separated list.

      8. Back in the form, hit 'Save and return to course' again.

      EXPECTED (except Moodle 2.3): There is still an error message but now it says 'HTTP/1.1 404 Not Found' indicating that Moodle correctly tried to retrieve the file without using the (bogus) proxy, because it is listed in the bypass list.

      MOODLE 2.3: On Moodle 2.3 we expect the debug text similar to 'cURL request for "http://whatever/imsmanifest.xml" failed, HTTP response code: HTTP/1.1 404 Not Found', followed by an undefined property error.

      BEFORE FIX: The 'Couldn't resolve proxy' error (or Moodle 2.3 equivalent) appears again, indicating that it's still trying to use the proxy server even though this server is on the bypass list.

      Show
      WARNING: This test is a bit complicated. I've written it so you can do the test even if you don't normally use a proxy server. We are going to use the SCORM activity because it is a convenient way to exercise the curl class and you can tell whether it's using the proxy or not. The test instructions contain special lines beginning 'MOODLE 2.3' referring to differences that are expected on that platform. 1. Go to Site administration / Plugins / Activity modules / SCORM package. Turn on 'Enable external package type' (allowtypeexternal). MOODLE 2.3: In addition to this, also ensure you have set 'debugging' option to 'Developer' as otherwise you cannot see the errors that we check below. 2. In the admin screens under Server/HTTP, or in your config.php (that would be necessary if you have already done 'real' settings in config.php), set the following options: proxyhost = does.not.work.example.com proxyport = 80 proxytype = HTTP proxybypass = localhost, 127.0.0.1 (default) 3. Go to any course page, turn editing on, and select to add a 'SCORM package'. 4. Type anything for name and description. Under 'Type' choose 'External SCORM manifest'. 5. Under 'URL' type a URL that should normally work without needing a proxy, e.g. the root path of your Moodle server followed by /imsmanifest.xml. For example http://my.moodle.server/imsmanifest.xml . 6. Hit 'Save and return to course'. EXPECTED (except Moodle 2.3): Next to the URL field you should see the error 'Invalid URL specified' with the debug message: Couldn't resolve proxy 'does.not.work.example.com'. This indicates that the server was correctly using the http proxy settings (which aren't valid) to retrieve the item. MOODLE 2.3: On Moodle 2.3 we expect the following behaviour if it is working correctly: debug text similar to 'cURL request for "http://whatever/imsmanifest.xml" failed with: Couldn't resolve proxy 'does.not.work.example.com' (5)', followed by an undefined property error. 7. In another tab, go back to the HTTP admin settings and change proxybypass so that it includes the hostname part of the URL you are trying to retrieve; e.g. if the URL is http://my.moodle.server/imsmanifest.xml then you could add ".moodle.server" to the comma-separated list. 8. Back in the form, hit 'Save and return to course' again. EXPECTED (except Moodle 2.3): There is still an error message but now it says 'HTTP/1.1 404 Not Found' indicating that Moodle correctly tried to retrieve the file without using the (bogus) proxy, because it is listed in the bypass list. MOODLE 2.3: On Moodle 2.3 we expect the debug text similar to 'cURL request for "http://whatever/imsmanifest.xml" failed, HTTP response code: HTTP/1.1 404 Not Found', followed by an undefined property error. BEFORE FIX: The 'Couldn't resolve proxy' error (or Moodle 2.3 equivalent) appears again, indicating that it's still trying to use the proxy server even though this server is on the bypass list.
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE
    • Pull 2.4 Branch:
      MDL-38168-m24
    • Pull Master Branch:
      MDL-38168-master
    • Rank:
      47999

      Description

      The 'curl' class in filelib.php (which is used, for example, when the SimplePie feed system retrieves files, but also in lots of other places) does not honour the 'proxybypass' option. If the proxy settings are enabled, it always uses the proxy regardless of the URL.

        Activity

        Hide
        Sam Marshall added a comment -

        Note: it is not really feasible to unit-test this change reliably because the curl class isn't designed to be testable and real testing relies on an actual proxy server and local network settings.

        Show
        Sam Marshall added a comment - Note: it is not really feasible to unit-test this change reliably because the curl class isn't designed to be testable and real testing relies on an actual proxy server and local network settings.
        Hide
        Sam Marshall added a comment -

        Please could somebody review this. I tried to make it a minimal patch just to fix this issue.

        Note: The code is where it is because I wanted to make sure it only affects the current request (in case you reuse the object for different URLs, only some of which go on the bypass list). It's OK to clear the proxy option at that point, because the proxy option is actually set further up in the same function.

        Show
        Sam Marshall added a comment - Please could somebody review this. I tried to make it a minimal patch just to fix this issue. Note: The code is where it is because I wanted to make sure it only affects the current request (in case you reuse the object for different URLs, only some of which go on the bypass list). It's OK to clear the proxy option at that point, because the proxy option is actually set further up in the same function.
        Hide
        Dan Poltawski added a comment -

        Sam, I think you could unit test this (I do it with simplepie by putting a nonsense proxy in place, you could test that you could still access it - see lib/tests/rsslib_test.php test_failproxy)

        (Having said that, these network tests arne't great)

        Show
        Dan Poltawski added a comment - Sam, I think you could unit test this (I do it with simplepie by putting a nonsense proxy in place, you could test that you could still access it - see lib/tests/rsslib_test.php test_failproxy) (Having said that, these network tests arne't great)
        Hide
        Sam Marshall added a comment -

        I wrote the following test before realising it's impossible:

            /**
             * Tests that the 'proxybypass' feature is supported.
             */
            public function test_proxybypass() {
                global $CFG;
        
                // Get the parent domain of host, such as might be used for proxy bypass.
                $host = preg_replace('~^[^/]*//[^.]+\.([^/]+).*$~', '$1', self::VALIDURL);
        
                // Set up a proxy that doesn't work, but bypass it.
                $oldproxyhost = $CFG->proxyhost;
                $oldproxybypass = $CFG->proxybypass;
                $CFG->proxyhost = 'xxxxxxxxxxxxxxx.moodle.org';
                $CFG->proxybypass = '.' . $host;
        
                // Get the feed and check it loads OK.
                $feed = new moodle_simplepie(self::VALIDURL);
                $this->assertEmpty($feed->error());
        
                // Put the globals back.
                $CFG->proxyhost = $oldproxyhost;
                $CFG->proxybypass = $oldproxybypass;
            }
        

        This test would work when using a system that does not require a proxy to access external files. However, it is supposed to be possible to run unit tests from behind a proxy, such as in the OU's system. Therefore I don't think it is possible to unit-test this; tests will have to be manual (and sketchy) like in my testing instructions.

        Show
        Sam Marshall added a comment - I wrote the following test before realising it's impossible: /** * Tests that the 'proxybypass' feature is supported. */ public function test_proxybypass() { global $CFG; // Get the parent domain of host, such as might be used for proxy bypass. $host = preg_replace('~^[^/]* //[^.]+\.([^/]+).*$~', '$1', self::VALIDURL); // Set up a proxy that doesn't work, but bypass it. $oldproxyhost = $CFG->proxyhost; $oldproxybypass = $CFG->proxybypass; $CFG->proxyhost = 'xxxxxxxxxxxxxxx.moodle.org'; $CFG->proxybypass = '.' . $host; // Get the feed and check it loads OK. $feed = new moodle_simplepie(self::VALIDURL); $ this ->assertEmpty($feed->error()); // Put the globals back. $CFG->proxyhost = $oldproxyhost; $CFG->proxybypass = $oldproxybypass; } This test would work when using a system that does not require a proxy to access external files. However, it is supposed to be possible to run unit tests from behind a proxy, such as in the OU's system. Therefore I don't think it is possible to unit-test this; tests will have to be manual (and sketchy) like in my testing instructions.
        Hide
        Damyon Wiese added a comment -

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

        Re the unit tests - IMO you should be able to run them completely offline so they are 100% reliable - we would need to add some mocking to the curl class to do this which is a bigger issue than this small change so I'm happy to not require unit tests for this (We'll see if the integrator agrees).

        Thanks Sam.

        Show
        Damyon Wiese added a comment - [Y] Syntax [-] Output [Y] Whitespace [-] Language [-] Databases [Y] Testing [-] Security [-] Documentation [Y] Git [Y] Sanity check Re the unit tests - IMO you should be able to run them completely offline so they are 100% reliable - we would need to add some mocking to the curl class to do this which is a bigger issue than this small change so I'm happy to not require unit tests for this (We'll see if the integrator agrees). Thanks Sam.
        Hide
        Sam Hemelryk added a comment -

        I agree, in this situation I think we are OK without the tests.
        This has been integrated now thanks Sam.

        Show
        Sam Hemelryk added a comment - I agree, in this situation I think we are OK without the tests. This has been integrated now thanks Sam.
        Hide
        Rossiani Wijaya added a comment -

        Hi Sam,

        The patch works great for 2.4 and Master.

        However when I tested for 2.3, I couldn't get the expected result for #6 and #8. The scorm is be saved but in redirect page it produced undefined propert error.

        Notice: Undefined property: stdClass::$elements in /integration/23/mod/scorm/datamodels/scormlib.php on line 517 
        

        Could you confirm this error please?

        Thanks.
        Rosie

        Show
        Rossiani Wijaya added a comment - Hi Sam, The patch works great for 2.4 and Master. However when I tested for 2.3, I couldn't get the expected result for #6 and #8. The scorm is be saved but in redirect page it produced undefined propert error. Notice: Undefined property: stdClass::$elements in /integration/23/mod/scorm/datamodels/scormlib.php on line 517 Could you confirm this error please? Thanks. Rosie
        Hide
        Rossiani Wijaya added a comment -

        I'm failing this issue for the above reason.

        Sorry.

        Show
        Rossiani Wijaya added a comment - I'm failing this issue for the above reason. Sorry.
        Hide
        Sam Marshall added a comment -

        There's probably nothing wrong with the change, but my test instructions don't work for Moodle 2.3 because there was apparently a bug in SCORM activity (fixed in 2.4) where it didn't detect the failure properly and instead displays that error you're seeing. This happens both with and without this patch applied.

        I've updated the test instructions to add a special Moodle 2.3 section. When there's another opportunity, please retest using these instructions.

        Show
        Sam Marshall added a comment - There's probably nothing wrong with the change, but my test instructions don't work for Moodle 2.3 because there was apparently a bug in SCORM activity (fixed in 2.4) where it didn't detect the failure properly and instead displays that error you're seeing. This happens both with and without this patch applied. I've updated the test instructions to add a special Moodle 2.3 section. When there's another opportunity, please retest using these instructions.
        Hide
        Matteo Scaramuccia added a comment -

        Hi Rosie and Sam,
        here it is: SCORM/$elements => MDL-36641

        Show
        Matteo Scaramuccia added a comment - Hi Rosie and Sam, here it is: SCORM/$elements => MDL-36641
        Hide
        Sam Hemelryk added a comment -

        Thanks for clarifying things Sam + Matteo.
        Rosie, I'm reopening this for testing again.

        Cheers guys

        Show
        Sam Hemelryk added a comment - Thanks for clarifying things Sam + Matteo. Rosie, I'm reopening this for testing again. Cheers guys
        Hide
        Rossiani Wijaya added a comment -

        Thanks for the explanation guys.

        Re-testing this issue.

        Show
        Rossiani Wijaya added a comment - Thanks for the explanation guys. Re-testing this issue.
        Hide
        Rossiani Wijaya added a comment -

        This is working as expected.

        Tested for 2.3, 2.4 and master

        Test passed.

        Show
        Rossiani Wijaya added a comment - This is working as expected. Tested for 2.3, 2.4 and master Test passed.
        Hide
        Eloy Lafuente (stronk7) added a comment -

        This is valid for unlimited entries to the, soon to be unveiled, Moodle Codebase Gardens. It includes free access to all facilities.

        Personal and non-transferable to all assignees, reviewers and testers in this issue. Valid until switching to Blackboard (100000€ penalization will be applied).

        Thanks, closing as fixed!

        Show
        Eloy Lafuente (stronk7) added a comment - This is valid for unlimited entries to the, soon to be unveiled, Moodle Codebase Gardens. It includes free access to all facilities. Personal and non-transferable to all assignees, reviewers and testers in this issue. Valid until switching to Blackboard (100000€ penalization will be applied). Thanks, closing as fixed!

          People

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

            Dates

            • Created:
              Updated:
              Resolved: