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 ac96658d4b3..c49fd401a20 100644 --- a/mod/url/lib.php +++ b/mod/url/lib.php @@ -88,7 +88,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++) { @@ -112,9 +114,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); @@ -131,6 +131,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"; @@ -153,9 +155,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,