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

revert too strict URL cleaning in mod/url

          Details

          • Type: Bug
          • Status: Closed
          • Priority: Blocker
          • Resolution: Fixed
          • Affects Version/s: 2.0.5, 2.1.2, 2.2
          • Fix Version/s: 2.0.6, 2.1.3
          • Component/s: Resource
          • Labels:
            None
          • Testing Instructions:
            Hide

            1/ create new url resource - try various valid and invalid URLs and general URIs, use different display modes
            2/ edit 'url' table - set external url to '' and 'https://' - verify the error message
            3/ run tests from mod/url/simpletest/testlib.php

            Show
            1/ create new url resource - try various valid and invalid URLs and general URIs, use different display modes 2/ edit 'url' table - set external url to '' and 'https://' - verify the error message 3/ run tests from mod/url/simpletest/testlib.php
          • Affected Branches:
            MOODLE_20_STABLE, MOODLE_21_STABLE, MOODLE_22_STABLE
          • Fixed Branches:
            MOODLE_20_STABLE, MOODLE_21_STABLE
          • Pull from Repository:
          • Pull Master Branch:
            w43_MDL-30005_m22_uri
          • Pull Master Diff URL:

            Description

            The problem is we allow not only http and https, but also people do not understand that they must use encoded entities.

            We should not use PARAM_URL because it is too strict which breaks backwards compatibility, technically we previously supported general URIs, not just web URLs.

              Gliffy Diagrams

                Attachments

                  Issue Links

                  has a non-specific relationship to

                  Sub-task - Issues are sometimes broken into multiple sub-tasks. MDL-20509 "http://" content in "add link" text box can be confusing

                  • Minor - Minor loss of function where workaround is possible
                  • Closed
                  is a regression caused by

                  Bug - A problem which impairs or prevents Moodle from functioning correctly. MDL-28483 adding URL resource without an url leads to error

                  • Critical - Crashes server, loss of data, severe memory leak
                  • Closed

                    Activity

                    Ascending order - Click to sort in descending order
                    Hide
                    Permalink
                    skodak Petr Skoda added a comment -

                    also we have the following 

                    if (!empty($data->externalurl) && (strpos($data->externalurl, '://') === false) && (strpos($data->externalurl, '/', 0) === false)) {
                    		 
                        $data->externalurl = 'http://'.$data->externalurl;
                    		 
                    }

                    that tires to fix stuff for us, it needs changes too though...

                    Show
                    skodak Petr Skoda added a comment - also we have the following if (!empty($data->externalurl) && (strpos($data->externalurl, '://') === false) && (strpos($data->externalurl, '/', 0) === false)) { $data->externalurl = 'http://'.$data->externalurl; } that tires to fix stuff for us, it needs changes too though...
                    Hide
                    Permalink
                    skodak Petr Skoda added a comment -

                    I am going to improve the handling of html entities a bit more

                    Show
                    skodak Petr Skoda added a comment - I am going to improve the handling of html entities a bit more
                    Hide
                    Permalink
                    skodak Petr Skoda added a comment -

                    done, hopefully now works as our teachers expect it...

                    Show
                    skodak Petr Skoda added a comment - done, hopefully now works as our teachers expect it...
                    Hide
                    Permalink
                    skodak Petr Skoda added a comment -

                    I have updated the repos once more, sorry...

                    Show
                    skodak Petr Skoda added a comment - I have updated the repos once more, sorry...
                    Hide
                    Permalink
                    nebgor Aparup Banerjee added a comment -

                    Thank you, this has been integrated now.

                    tester: please test with various URLs and strange (custom?) ones too.

                    Show
                    nebgor Aparup Banerjee added a comment - Thank you, this has been integrated now. tester: please test with various URLs and strange (custom?) ones too.
                    Hide
                    Permalink
                    salvetore Michael de Raadt added a comment - - edited

                    I was able to create regular protocol://url urls, but when I tried a mailto : email and skype:id, it produced an error...

                    Warning: preg_match() expects parameter 2 to be string, object given in D:\xampp\htdocs\moodle_testing\mod\url\locallib.php on line 92

                    Show
                    salvetore Michael de Raadt added a comment - - edited I was able to create regular protocol://url urls, but when I tried a mailto : email and skype:id , it produced an error... Warning: preg_match() expects parameter 2 to be string, object given in D:\xampp\htdocs\moodle_testing\mod\url\locallib.php on line 92
                    Hide
                    Permalink
                    skodak Petr Skoda added a comment -

                    ooops, sorry for the sloppy copy pasting, branches updated...

                    Show
                    skodak Petr Skoda added a comment - ooops, sorry for the sloppy copy pasting, branches updated...
                    Hide
                    Permalink
                    skodak Petr Skoda added a comment -

                    I have also noticed the 20 integration branch is missing the version bump from my branch, that will be hopefully fixed during the merge too.

                    Show
                    skodak Petr Skoda added a comment - I have also noticed the 20 integration branch is missing the version bump from my branch, that will be hopefully fixed during the merge too.
                    Hide
                    Permalink
                    nebgor Aparup Banerjee added a comment - - edited

                    thanks, changes integrated. up for testing again. (including version bump)

                    Show
                    nebgor Aparup Banerjee added a comment - - edited thanks, changes integrated. up for testing again. (including version bump)
                    Hide
                    Permalink
                    abgreeve Adrian Greeve added a comment -

                    tested with 2.2, 2.2, 2.0 All worked fine.

                    Show
                    abgreeve Adrian Greeve added a comment - tested with 2.2, 2.2, 2.0 All worked fine.
                    Hide
                    Permalink
                    stronk7 Eloy Lafuente (stronk7) added a comment -

                    And this has been sent upstream (already available @ git and cvs repos). Many, many thanks!

                    Closing as fixed, ciao

                    Show
                    stronk7 Eloy Lafuente (stronk7) added a comment - And this has been sent upstream (already available @ git and cvs repos). Many, many thanks! Closing as fixed, ciao

                      People

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

                        Dates

                        • Created:
                          Updated:
                          Resolved:
                          Fix Release Date:
                          28/Nov/11