Moodle
  1. Moodle
  2. MDL-33749

URL downloader does not follow redirects

    Details

    • Testing Instructions:
      Hide

      1. Add a new File resource.
      2. In the Content Section, click on "Add..."
      3. In the file picker, select "URL downloader"
      4. Type google.com in the URL box and click Download.

      You should see at least one image available for download.

      Show
      1. Add a new File resource. 2. In the Content Section, click on "Add..." 3. In the file picker, select "URL downloader" 4. Type google.com in the URL box and click Download. You should see at least one image available for download.
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE, MOODLE_23_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      wip-MDL-33749-master
    • Rank:
      41782

      Description

      When adding a resource such as a file to a course, using the URL downloader in the File picker dialog fails when a website is entered that redirects the user.

      Steps to reproduce:
      1. Add a new File resource.
      2. In the Content Section, click on "Add..."
      3. In the file picker, select "URL downloader"
      4. Type google.com in the URL box and click Download.

      This is what comes up when following these steps:
      HTTP/1.1 301 Moved Permanently Location: http://www.google.com/ Content-Type: text/html; charset=UTF-8 Date: Thu, 14 Jun 2012 18:23:40 GMT Expires: Sat, 14 Jul 2012 18:23:40 GMT Cache-Control: public, max-age=2592000 Server: gws Content-Length: 219 X-XSS-Protection: 1; mode=block X-Frame-Options: SAMEORIGIN

      Note that typing www.google.com into the URL box does work. Google automatically redirects users from http://google.com to http://www.google.com, which the URL downloader cannot handle.

        Issue Links

          Activity

          Hide
          Kevin Wiliarty added a comment -

          It might be just a matter of uncommenting a line that directs curl to follow the redirects. I've posted a possible solution at:

          https://github.com/kwiliarty/moodle/compare/wip-MDL-33749-master

          Show
          Kevin Wiliarty added a comment - It might be just a matter of uncommenting a line that directs curl to follow the redirects. I've posted a possible solution at: https://github.com/kwiliarty/moodle/compare/wip-MDL-33749-master
          Hide
          Michael de Raadt added a comment -

          Thanks for pointing that out and providing a solution.

          Show
          Michael de Raadt added a comment - Thanks for pointing that out and providing a solution.
          Hide
          Charles Fulton added a comment -

          That line was commented out in MDL-21170, and it's not clear to me why. The inline comments make it sound like it should be on, especially since MAXREDIRS is set with a reasonable value.

          Show
          Charles Fulton added a comment - That line was commented out in MDL-21170 , and it's not clear to me why. The inline comments make it sound like it should be on, especially since MAXREDIRS is set with a reasonable value.
          Hide
          Dan Poltawski added a comment -

          Hi,

          I agree the comments are not clear, but I think its sensible that our core curl class doesn't follow redirects by default. Its a more 'robust' setting to default to.

          So I think you should just set it in the downloader repo plugin, you can do it with something like this:

          $curl->setopt(array('CURLOPT_FOLLOWLOCATION' => true, 'CURLOPT_MAXREDIRS' => 3));
          
          Show
          Dan Poltawski added a comment - Hi, I agree the comments are not clear, but I think its sensible that our core curl class doesn't follow redirects by default. Its a more 'robust' setting to default to. So I think you should just set it in the downloader repo plugin, you can do it with something like this: $curl->setopt(array('CURLOPT_FOLLOWLOCATION' => true , 'CURLOPT_MAXREDIRS' => 3));
          Hide
          Kevin Wiliarty added a comment -

          Thanks for the suggestion, Dan. I've made the change and rebased.

          Show
          Kevin Wiliarty added a comment - Thanks for the suggestion, Dan. I've made the change and rebased.
          Hide
          Dan Poltawski added a comment -

          Thanks, +1 - submitting for integration

          Show
          Dan Poltawski added a comment - Thanks, +1 - submitting for integration
          Hide
          Tim Barker added a comment -

          I replicated this with a redirect to a login page. This scenario needs to be handled also because the error that the user sees is not elegant.

          To replicate:

          Test Pre-requisites:

          • URL Downloader must be enabled.

          Test steps:

          1. Launch a filepicker anywhere in Moodle.
          2. Enter a URL to the page of Moodle that you are currently on.

          Expected result:

          • Moodle elegantly handles the redirect.

          Actual result:

          • Moodle displays a dialog containing either a non-elegant 302 or 303 error to the user e.g.:

          HTTP/1.1 303 See Other Date: Thu, 21 Jun 2012 07:36:39 GMT Server: Apache/2.2.22 (Ubuntu) X-Powered-By: PHP/5.3.10-1ubuntu3.2 Set-Cookie: MoodleSession=62e3monnrrv2tja8qmirjaujt0; path=/moodle/ Expires: Thu, 19 Nov 1981 08:52:00 GMT Cache-Control: no-store, no-cache, must-revalidate, post-check=0, pre-check=0 Pragma: no-cache Location: http://192.168.100.46/moodle/login/index.php Content-Language: en Vary: Accept-Encoding Content-Encoding: gzip Content-Length: 20 Keep-Alive: timeout=5, max=100 Connection: Keep-Alive Content-Type: text/html

          Show
          Tim Barker added a comment - I replicated this with a redirect to a login page. This scenario needs to be handled also because the error that the user sees is not elegant. To replicate: Test Pre-requisites: URL Downloader must be enabled. Test steps: Launch a filepicker anywhere in Moodle. Enter a URL to the page of Moodle that you are currently on. Expected result: Moodle elegantly handles the redirect. Actual result: Moodle displays a dialog containing either a non-elegant 302 or 303 error to the user e.g.: HTTP/1.1 303 See Other Date: Thu, 21 Jun 2012 07:36:39 GMT Server: Apache/2.2.22 (Ubuntu) X-Powered-By: PHP/5.3.10-1ubuntu3.2 Set-Cookie: MoodleSession=62e3monnrrv2tja8qmirjaujt0; path=/moodle/ Expires: Thu, 19 Nov 1981 08:52:00 GMT Cache-Control: no-store, no-cache, must-revalidate, post-check=0, pre-check=0 Pragma: no-cache Location: http://192.168.100.46/moodle/login/index.php Content-Language: en Vary: Accept-Encoding Content-Encoding: gzip Content-Length: 20 Keep-Alive: timeout=5, max=100 Connection: Keep-Alive Content-Type: text/html
          Hide
          Kevin Wiliarty added a comment -

          Hi Tim, I wonder if I missing or misunderstanding some detail of your scenario. When I try the steps you describe on a site using the patch above the URL Downloader seems to work fine.

          Show
          Kevin Wiliarty added a comment - Hi Tim, I wonder if I missing or misunderstanding some detail of your scenario. When I try the steps you describe on a site using the patch above the URL Downloader seems to work fine.
          Hide
          Petr Škoda added a comment -

          Stupid question, is this patch compatible with openbasedir restriction? There is a similar problem in download_file_content() from filelib.php. See MDL-33955.

          Show
          Petr Škoda added a comment - Stupid question, is this patch compatible with openbasedir restriction? There is a similar problem in download_file_content() from filelib.php. See MDL-33955 .
          Hide
          Dan Poltawski added a comment -

          Petr: does that mean it simply does not observe the redirect in those scenarios, or does it cause some other problem?

          Show
          Dan Poltawski added a comment - Petr: does that mean it simply does not observe the redirect in those scenarios, or does it cause some other problem?
          Hide
          Petr Škoda added a comment -

          I do not know, that is why I asked.

          Show
          Petr Škoda added a comment - I do not know, that is why I asked.
          Hide
          Dan Poltawski added a comment -

          (either way, we need to solve that in the curl class, since parts of Moodle are using that functionality in other parts of core).

          Show
          Dan Poltawski added a comment - (either way, we need to solve that in the curl class, since parts of Moodle are using that functionality in other parts of core).
          Hide
          Sam Hemelryk added a comment -

          Hi guys,

          I've just started looking at this now.
          Certainly it would be great to for our curl uses to support safe_mode and openbasedir but I agree with Dan that we need to find a core solution first and then upgrade our code to use it.
          As this code appears ready to go I think we should get it in and look for that core solution as part of MDL-33749, surely it will turn this area up as a location to upgrade.

          However... this patch doesn't cherry-pick cleanly.
          Kevin, could you please branches for the stable versions of Moodle for this patch. 21, 22, and 23 (it cherry-picks to 23 actually so no need to worry about that).

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi guys, I've just started looking at this now. Certainly it would be great to for our curl uses to support safe_mode and openbasedir but I agree with Dan that we need to find a core solution first and then upgrade our code to use it. As this code appears ready to go I think we should get it in and look for that core solution as part of MDL-33749 , surely it will turn this area up as a location to upgrade. However... this patch doesn't cherry-pick cleanly. Kevin, could you please branches for the stable versions of Moodle for this patch. 21, 22, and 23 (it cherry-picks to 23 actually so no need to worry about that). Cheers Sam
          Hide
          Kevin Wiliarty added a comment -

          Hi Sam,

          I've got patch branches now for MOODLE_21_STABLE, MOODLE_22_STABLE, MOODLE_23_STABLE and master, all of them rebased up to the the latest release.

          Cheers,
          Kevin

          Show
          Kevin Wiliarty added a comment - Hi Sam, I've got patch branches now for MOODLE_21_STABLE, MOODLE_22_STABLE, MOODLE_23_STABLE and master, all of them rebased up to the the latest release. Cheers, Kevin
          Hide
          Sam Hemelryk added a comment -

          On second thoughts no worries, the conflict appears more complex than it actually is.
          I've backported this now and reimplemented the fix on 22.

          Has been integrated now thanks.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - On second thoughts no worries, the conflict appears more complex than it actually is. I've backported this now and reimplemented the fix on 22. Has been integrated now thanks. Cheers Sam
          Hide
          Sam Hemelryk added a comment -

          Haha thanks Kevin, sorry I just saw your comment. We must've both been working on it at the same time.
          I'll check out the 21 branch now.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Haha thanks Kevin, sorry I just saw your comment. We must've both been working on it at the same time. I'll check out the 21 branch now. Cheers Sam
          Hide
          Sam Hemelryk added a comment -

          Backported to 21 as well.

          Show
          Sam Hemelryk added a comment - Backported to 21 as well.
          Hide
          Frédéric Massart added a comment -

          Test passed on 2.1, 2.2, 2.3 and master. Yo!

          Show
          Frédéric Massart added a comment - Test passed on 2.1, 2.2, 2.3 and master. Yo!
          Hide
          Sam Hemelryk added a comment -

          Congratulations your code is upstream - gold star for you!

          This issue + 79 others made it in in time for the minor releases.
          Thank you everyone involved for your exuberant efforts.

          Show
          Sam Hemelryk added a comment - Congratulations your code is upstream - gold star for you! This issue + 79 others made it in in time for the minor releases. Thank you everyone involved for your exuberant efforts.

            People

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

              Dates

              • Created:
                Updated:
                Resolved: