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

      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.

        Gliffy Diagrams

          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 Skoda 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 Skoda 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 Skoda added a comment -

            I do not know, that is why I asked.

            Show
            Petr Skoda 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: