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

Filelib: Curl class does not honour proxybypass option

    Details

    • Type: Bug
    • Status: Closed
    • Priority: 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 Master Branch:
      MDL-38168-master

      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.

        Gliffy Diagrams

          Activity

          Hide
          quen 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
          quen 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
          quen 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
          quen 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
          poltawski 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
          poltawski 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
          quen 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
          quen 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 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 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
          samhemelryk 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
          samhemelryk 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
          rwijaya 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
          rwijaya 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
          rwijaya Rossiani Wijaya added a comment -

          I'm failing this issue for the above reason.

          Sorry.

          Show
          rwijaya Rossiani Wijaya added a comment - I'm failing this issue for the above reason. Sorry.
          Hide
          quen 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
          quen 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 Matteo Scaramuccia added a comment -

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

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

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

          Cheers guys

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

          Thanks for the explanation guys.

          Re-testing this issue.

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

          This is working as expected.

          Tested for 2.3, 2.4 and master

          Test passed.

          Show
          rwijaya Rossiani Wijaya added a comment - This is working as expected. Tested for 2.3, 2.4 and master Test passed.
          Hide
          stronk7 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
          stronk7 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:
                Fix Release Date:
                11/Mar/13