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.
moodle · Oct 30, 2011 · 48f69e4 · 48f69e4
1 parent 38e9a1c
commit 48f69e4
Show file tree

Hide file tree

Showing 7 changed files with 215 additions and 41 deletions.
diff --git 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('&amp;', '&', $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('&amp;', '&', $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
@@ -44,7 +44,7 @@
 $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
@@ -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
@@ -30,46 +30,117 @@
 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 &amp;
  */
 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 (!$config) {
-        $config = get_config('url');
+    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);
     }
-    $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]);
+    // 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] = rawurlencode($parse).'='.rawurlencode($paramvalues[$parameter]);
+            } else {
+                unset($parameters[$parse]);
+            }
         }
-    }
 
-    if (empty($parameters)) {
-        // easy - no params available
-        return $url->externalurl;
+        if (!empty($parameters)) {
+            if (stripos($fullurl, 'teamspeak://') === 0) {
+                $fullurl = $fullurl.'?'.implode('?', $parameters);
+            } else {
+                $join = (strpos($fullurl->externalurl, '?') === false) ? '?' : '&';
+                $fullurl = $fullurl.$join.implode('&', $parameters);
+            }
+        }
     }
 
-    if (stripos($url->externalurl, 'teamspeak://') === 0) {
-        return $url->externalurl.'?'.implode('?', $parameters);
-    } else {
-        $join = (strpos($url->externalurl, '?') === false) ? '?' : '&amp;';
-        return $url->externalurl.$join.implode('&amp;', $parameters);
-    }
+    // encode all & to &amp; entity
+    $fullurl = str_replace('&', '&amp;', $fullurl);
+
+    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
@@ -168,10 +168,42 @@ function data_preprocessing(&$default_values) {
 
     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
@@ -0,0 +1,61 @@
+<?php
+// This file is part of Moodle - http://moodle.org/
+//
+// Moodle is free software: you can redistribute it and/or modify
+// it under the terms of the GNU General Public License as published by
+// the Free Software Foundation, either version 3 of the License, or
+// (at your option) any later version.
+//
+// Moodle is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+//
+// You should have received a copy of the GNU General Public License
+// along with Moodle.  If not, see <http://www.gnu.org/licenses/>.
+
+/**
+ * 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://@:@/'));
+    }
+}
diff --git a/mod/url/view.php b/mod/url/view.php
@@ -55,11 +55,17 @@
 
 $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,