From 4064642b37939bd3e45607cc8b6a16e9b4da6b73 Mon Sep 17 00:00:00 2001 From: Petr Skoda Date: Sun, 30 Oct 2011 23:53:17 +0100 Subject: [PATCH 1/2] MDL-30005 fix general URI support in URL module The recent change completely removed support for general URIs and enforced strict URL validation which was breaking backwards compatibility and was not user friendly at all. This patch finally adds full URI support with basic validation and fixes some older xhtml strict issues. Please note we do not try to explain the difference between URL and URI to our users. Also keep in mind that mod/url has XSS risk and does not do any security validation of the data. --- lib/resourcelib.php | 4 ++ mod/url/lang/en/url.php | 2 +- mod/url/lib.php | 14 ++-- mod/url/locallib.php | 123 ++++++++++++++++++++++++++------- mod/url/mod_form.php | 38 +++++++++- mod/url/simpletest/testlib.php | 61 ++++++++++++++++ mod/url/view.php | 14 ++-- 7 files changed, 215 insertions(+), 41 deletions(-) create mode 100644 mod/url/simpletest/testlib.php diff --git a/lib/resourcelib.php b/lib/resourcelib.php index 25c8e721b8f..4df0be9c834 100644 --- a/lib/resourcelib.php +++ b/lib/resourcelib.php @@ -233,6 +233,8 @@ function resourcelib_embed_mp3($fullurl, $title, $clicktoopen) { if ($fullurl instanceof moodle_url) { $fullurl = $fullurl->out(false); + } else { + $fullurl = str_replace('&', '&', $fullurl); } $id = 'resource_mp3_'.time(); //we need something unique because it might be stored in text cache @@ -259,6 +261,8 @@ function resourcelib_embed_flashvideo($fullurl, $title, $clicktoopen) { if ($fullurl instanceof moodle_url) { $fullurl = $fullurl->out(false); + } else { + $fullurl = str_replace('&', '&', $fullurl); } $id = 'resource_flv_'.time(); //we need something unique because it might be stored in text cache diff --git a/mod/url/lang/en/url.php b/mod/url/lang/en/url.php index 572b1984f81..5fb3ceec25a 100644 --- a/mod/url/lang/en/url.php +++ b/mod/url/lang/en/url.php @@ -44,7 +44,7 @@ $string['displayselect_help'] = 'This setting, together with the URL file type a $string['displayselectexplain'] = 'Choose display type, unfortunately not all types are suitable for all URLs.'; $string['externalurl'] = 'External URL'; $string['framesize'] = 'Frame height'; -$string['invalidstoredurl'] = 'Invalid URL'; +$string['invalidstoredurl'] = 'Can not dispaly this resource, URL is invalid.'; $string['chooseavariable'] = 'Choose a variable...'; $string['invalidurl'] = 'Entered URL is invalid'; $string['modulename'] = 'URL'; diff --git a/mod/url/lib.php b/mod/url/lib.php index e354511bc18..502ff0ce106 100644 --- a/mod/url/lib.php +++ b/mod/url/lib.php @@ -87,7 +87,9 @@ function url_get_post_actions() { * @return int new url instance id */ function url_add_instance($data, $mform) { - global $DB; + global $CFG, $DB; + + require_once($CFG->dirroot.'/mod/url/locallib.php'); $parameters = array(); for ($i=0; $i < 100; $i++) { @@ -111,9 +113,7 @@ function url_add_instance($data, $mform) { } $data->displayoptions = serialize($displayoptions); - if (!empty($data->externalurl) && (strpos($data->externalurl, '://') === false) && (strpos($data->externalurl, '/', 0) === false)) { - $data->externalurl = 'http://'.$data->externalurl; - } + $data->externalurl = url_fix_submitted_url($data->externalurl); $data->timemodified = time(); $data->id = $DB->insert_record('url', $data); @@ -130,6 +130,8 @@ function url_add_instance($data, $mform) { function url_update_instance($data, $mform) { global $CFG, $DB; + require_once($CFG->dirroot.'/mod/url/locallib.php'); + $parameters = array(); for ($i=0; $i < 100; $i++) { $parameter = "parameter_$i"; @@ -152,9 +154,7 @@ function url_update_instance($data, $mform) { } $data->displayoptions = serialize($displayoptions); - if (!empty($data->externalurl) && (strpos($data->externalurl, '://') === false) && (strpos($data->externalurl, '/', 0) === false)) { - $data->externalurl = 'http://'.$data->externalurl; - } + $data->externalurl = url_fix_submitted_url($data->externalurl); $data->timemodified = time(); $data->id = $data->instance; diff --git a/mod/url/locallib.php b/mod/url/locallib.php index 758e5b8ed82..b4610e3ac47 100644 --- a/mod/url/locallib.php +++ b/mod/url/locallib.php @@ -30,46 +30,117 @@ require_once("$CFG->libdir/filelib.php"); require_once("$CFG->libdir/resourcelib.php"); require_once("$CFG->dirroot/mod/url/lib.php"); +/** + * This methods does weak url validation, we are looking for major problems only, + * mo strict RFE validation. + * + * @param $url + * @return bool true is seems valid, false if definitely not valid URL + */ +function url_appears_valid_url($url) { + if (preg_match('/^(\/|https?:|ftp:)/i', $url)) { + // note: this is not exact validation, we look for severely malformed URLs only + return preg_match('/^[a-z]+:\/\/([^:@\s]+:[^@\s]+@)?[a-z0-9_\.\-]+(:[0-9]+)?(\/[^#]*)?(#.*)?$/i', $url); + } else { + return preg_match('/^[a-z]+:\/\/...*$/i', $url); + } +} + +/** + * Fix common URL problems that we want teachers to see fixed + * the next time they edit the resource. + * + * This function does not include any XSS protection. + * + * @param string $url + * @return string + */ +function url_fix_submitted_url($url) { + // note: empty urls are prevented in form validation + $url = trim($url); + + // remove encoded entities - we want the raw URI here + $url = html_entity_decode($url, ENT_QUOTES, 'UTF-8'); + + if (!preg_match('|^[a-z]+:|i', $url) and !preg_match('|^/|', $url)) { + // invalid URI, try to fix it by making it normal URL, + // please note relative urls are not allowed, /xx/yy links are ok + $url = 'http://'.$url; + } + + return $url; +} + /** * Return full url with all extra parameters + * + * This function does not include any XSS protection. + * * @param string $url * @param object $cm * @param object $course - * @return string url + * @param object $config + * @return string url with & encoded as & */ function url_get_full_url($url, $cm, $course, $config=null) { $parameters = empty($url->parameters) ? array() : unserialize($url->parameters); - if (empty($parameters)) { - // easy - no params - return $url->externalurl; + // make sure there are no encoded entities, it is ok to do this twice + $fullurl = html_entity_decode($url->externalurl, ENT_QUOTES, 'UTF-8'); + + if (preg_match('/^(\/|https?:|ftp:)/i', $fullurl) or preg_match('|^/|', $url)) { + // encode extra chars in URLs - this does not make it always valid, but it helps with some UTF-8 problems + $allowed = "a-zA-Z0-9".preg_quote(';/?:@=&$_.+!*(),-#%', '/'); + $fullurl = preg_replace_callback("/[^$allowed]/", 'url_filter_callback', $fullurl); + } else { + // encode special chars only + $fullurl = str_replace('"', '%22', $fullurl); + $fullurl = str_replace('\'', '%27', $fullurl); + $fullurl = str_replace(' ', '%20', $fullurl); + $fullurl = str_replace('<', '%3C', $fullurl); + $fullurl = str_replace('>', '%3E', $fullurl); } - if (!$config) { - $config = get_config('url'); - } - $paramvalues = url_get_variable_values($url, $cm, $course, $config); + // add variable url parameters + if (!empty($parameters)) { + if (!$config) { + $config = get_config('url'); + } + $paramvalues = url_get_variable_values($url, $cm, $course, $config); - foreach ($parameters as $parse=>$parameter) { - if (isset($paramvalues[$parameter])) { - $parameters[$parse] = urlencode($parse).'='.urlencode($paramvalues[$parameter]); - } else { - unset($parameters[$parse]); + foreach ($parameters as $parse=>$parameter) { + if (isset($paramvalues[$parameter])) { + $parameters[$parse] = rawurlencode($parse).'='.rawurlencode($paramvalues[$parameter]); + } else { + unset($parameters[$parse]); + } + } + + if (!empty($parameters)) { + if (stripos($fullurl, 'teamspeak://') === 0) { + $fullurl = $fullurl.'?'.implode('?', $parameters); + } else { + $join = (strpos($fullurl->externalurl, '?') === false) ? '?' : '&'; + $fullurl = $fullurl.$join.implode('&', $parameters); + } } } - if (empty($parameters)) { - // easy - no params available - return $url->externalurl; - } + // encode all & to & entity + $fullurl = str_replace('&', '&', $fullurl); - if (stripos($url->externalurl, 'teamspeak://') === 0) { - return $url->externalurl.'?'.implode('?', $parameters); - } else { - $join = (strpos($url->externalurl, '?') === false) ? '?' : '&'; - return $url->externalurl.$join.implode('&', $parameters); - } + return $fullurl; +} + +/** + * Unicode encoding helper callback + * @internal + * @param array $matches + * @return string + */ +function url_filter_callback($matches) { + return rawurlencode($matches[0]); } /** @@ -197,11 +268,12 @@ function url_print_workaround($url, $cm, $course) { $display = url_get_final_display_type($url); if ($display == RESOURCELIB_DISPLAY_POPUP) { + $jsfullurl = addslashes_js($fullurl); $options = empty($url->displayoptions) ? array() : unserialize($url->displayoptions); $width = empty($options['popupwidth']) ? 620 : $options['popupwidth']; $height = empty($options['popupheight']) ? 450 : $options['popupheight']; $wh = "width=$width,height=$height,toolbar=no,location=no,menubar=no,copyhistory=no,status=no,directories=no,scrollbars=yes,resizable=yes"; - $extra = "onclick=\"window.open('$fullurl', '', '$wh'); return false;\""; + $extra = "onclick=\"window.open('$jsfullurl', '', '$wh'); return false;\""; } else if ($display == RESOURCELIB_DISPLAY_NEW) { $extra = "onclick=\"this.target='_blank';\""; @@ -223,7 +295,6 @@ function url_print_workaround($url, $cm, $course) { * @param object $url * @param object $cm * @param object $course - * @param stored_file $file main file * @return does not return */ function url_display_embed($url, $cm, $course) { @@ -286,7 +357,7 @@ function url_display_embed($url, $cm, $course) { } /** - * Decide the best diaply format. + * Decide the best display format. * @param object $url * @return int display type constant */ diff --git a/mod/url/mod_form.php b/mod/url/mod_form.php index 790b85dc534..47d42512bb9 100644 --- a/mod/url/mod_form.php +++ b/mod/url/mod_form.php @@ -168,10 +168,42 @@ class mod_url_mod_form extends moodleform_mod { function validation($data, $files) { $errors = parent::validation($data, $files); - //Validating Entered url - $data['externalurl'] = clean_param($data['externalurl'], PARAM_URL); + + // Validating Entered url, we are looking for obvious problems only, + // teachers are responsible for testing if it actually works. + + // This is not a security validation!! Teachers are allowed to enter "javascript:alert(666)" for example. + + // NOTE: do not try to explain the difference between URL and URI, people would be only confused... + if (empty($data['externalurl'])) { - $errors['externalurl'] = get_string('invalidurl', 'url'); + $errors['externalurl'] = get_string('required'); + + } else { + $url = trim($data['externalurl']); + if (empty($url)) { + $errors['externalurl'] = get_string('required'); + + } else if (preg_match('|^/|', $url)) { + // links relative to server root are ok - no validation necessary + + } else if (preg_match('|^[a-z]+://|i', $url) or preg_match('|^https?:|i', $url) or preg_match('|^ftp:|i', $url)) { + // normal URL + if (!url_appears_valid_url($url)) { + $errors['externalurl'] = get_string('invalidurl', 'url'); + } + + } else if (preg_match('|^[a-z]+:|i', $url)) { + // general URI such as teamspeak, mailto, etc. - it may or may not work in all browsers, + // we do not validate these at all, sorry + + } else { + // invalid URI, we try to fix it by adding 'http://' prefix, + // relative links are NOT allowed because we display the link on different pages! + if (!url_appears_valid_url('http://'.$url)) { + $errors['externalurl'] = get_string('invalidurl', 'url'); + } + } } return $errors; } diff --git a/mod/url/simpletest/testlib.php b/mod/url/simpletest/testlib.php new file mode 100644 index 00000000000..2dbf1bab845 --- /dev/null +++ b/mod/url/simpletest/testlib.php @@ -0,0 +1,61 @@ +. + +/** + * Unit tests for some mod URL lib stuff. + * + * @package mod + * @subpackage url + * @copyright 2011 petr Skoda + * @license http://www.gnu.org/copyleft/gpl.html GNU Public License + */ + + +defined('MOODLE_INTERNAL') || die(); + +require_once($CFG->dirroot . '/mod/url/locallib.php'); + + +/** + * @copyright 2011 petr Skoda + */ +class url_lib_test extends UnitTestCase { + + public function test_url_appears_valid_url() { + + $this->assertTrue(url_appears_valid_url('http://example')); + $this->assertTrue(url_appears_valid_url('http://www.example.com')); + $this->assertTrue(url_appears_valid_url('http://www.exa-mple2.com')); + $this->assertTrue(url_appears_valid_url('http://www.example.com/~nobody/index.html')); + $this->assertTrue(url_appears_valid_url('http://www.example.com#hmm')); + $this->assertTrue(url_appears_valid_url('http://www.example.com/#hmm')); + $this->assertTrue(url_appears_valid_url('http://www.example.com/žlutý koníček/lala.txt')); + $this->assertTrue(url_appears_valid_url('http://www.example.com/žlutý koníček/lala.txt#hmmmm')); + $this->assertTrue(url_appears_valid_url('http://www.example.com/index.php?xx=yy&zz=aa')); + $this->assertTrue(url_appears_valid_url('https://user:password@www.example.com/žlutý koníček/lala.txt')); + $this->assertTrue(url_appears_valid_url('ftp://user:password@www.example.com/žlutý koníček/lala.txt')); + + $this->assertFalse(url_appears_valid_url('http:example.com')); + $this->assertFalse(url_appears_valid_url('http:/example.com')); + $this->assertFalse(url_appears_valid_url('http://')); + $this->assertFalse(url_appears_valid_url('http://www.exa mple.com')); + $this->assertFalse(url_appears_valid_url('http://www.examplé.com')); + $this->assertFalse(url_appears_valid_url('http://@www.example.com')); + $this->assertFalse(url_appears_valid_url('http://user:@www.example.com')); + + $this->assertTrue(url_appears_valid_url('lalala://@:@/')); + } +} \ No newline at end of file diff --git a/mod/url/view.php b/mod/url/view.php index 8996c22b1fd..6840abd7ab4 100644 --- a/mod/url/view.php +++ b/mod/url/view.php @@ -55,11 +55,17 @@ $completion->set_module_viewed($cm); $PAGE->set_url('/mod/url/view.php', array('id' => $cm->id)); -// Make sure URL is valid before generating output -$url->externalurl = clean_param($url->externalurl, PARAM_URL); -if (empty($url->externalurl)) { - print_error('invalidstoredurl', 'url'); +// Make sure URL exists before generating output - some older sites may contain empty urls +// Do not use PARAM_URL here, it is too strict and does not support general URIs! +$exturl = trim($url->externalurl); +if (empty($exturl) or $exturl === 'http://') { + url_print_header($url, $cm, $course); + url_print_heading($url, $cm, $course); + url_print_intro($url, $cm, $course); + notice(get_string('invalidstoredurl', 'url'), new moodle_url('/course/view.php', array('id'=>$cm->course))); + die; } +unset($exturl); if ($redirect) { // coming from course page or url index page, From 516dd9c525216e7fe3e3c5417eb74dcde382b984 Mon Sep 17 00:00:00 2001 From: Petr Skoda Date: Sun, 30 Oct 2011 12:54:29 +0100 Subject: [PATCH 2/2] MDL-30005 bump up version after lang string change --- mod/url/version.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mod/url/version.php b/mod/url/version.php index 5b4c1b54024..786c82e1b2a 100644 --- a/mod/url/version.php +++ b/mod/url/version.php @@ -26,7 +26,7 @@ defined('MOODLE_INTERNAL') || die; -$module->version = 2010101400; +$module->version = 2010101401; $module->requires = 2010080300; // Requires this Moodle version $module->cron = 0;