Moodle

revert too strict URL cleaning in mod/url

Details

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

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 - The issue is considered finished, the resolution is correct. Issues which are closed should not be reopened.
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 - The issue is considered finished, the resolution is correct. Issues which are closed should not be reopened.

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
Petr Škoda 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 Škoda 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 Škoda added a comment -

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

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

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

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

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

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

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

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

Vote (0)
Watch (1)

Dates

  • Created:
    Updated:
    Resolved: