Moodle
  1. Moodle
  2. MDL-30005

revert too strict URL cleaning in mod/url

        Details

        • Type: Bug Bug
        • Status: Closed
        • Priority: Blocker 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

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

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

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

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

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

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

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

                Thank you, this has been integrated now.

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

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

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

                Show
                Petr Skoda added a comment - ooops, sorry for the sloppy copy pasting, branches updated...
                Hide
                Permalink
                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
                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
                Aparup Banerjee added a comment - - edited

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

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

                tested with 2.2, 2.2, 2.0 All worked fine.

                Show
                Adrian Greeve added a comment - tested with 2.2, 2.2, 2.0 All worked fine.
                Hide
                Permalink
                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
                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: