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

      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

            Activity

            Hide
            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
            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
            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
            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
            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
            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
            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
            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
            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
            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
            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: