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

      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

              Activity

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