MDL-21233 escaped parameter is far more importatnt than the overridep…

…arams in out() method - especially all JS urls should be converted to out(false); also it is possible to create new url with overrided parameters in constructor which might be actually work around the double encoding problems in the url (we should never use the out() in moodle_url constructor itself!)
anovikov · Jan 17, 2010 · b9bc201 · b9bc201
1 parent eb78806
commit b9bc201
Show file tree

Hide file tree

Showing 21 changed files with 48 additions and 53 deletions.
diff --git a/lib/ajax/ajaxlib.php b/lib/ajax/ajaxlib.php
@@ -130,7 +130,7 @@ protected function setup_core_javascript(moodle_page $page, core_renderer $outpu
         $config = array(
             'wwwroot'             => $CFG->httpswwwroot, // Yes, really. See above.
             'sesskey'             => sesskey(),
-            'loadingicon'         => $output->pix_url('i/loading_small', 'moodle')->out_raw(),
+            'loadingicon'         => $output->pix_url('i/loading_small', 'moodle')->out(false),
             'themerev'            => theme_get_revision(),
             'theme'               => $page->theme->name,
             'yui2loaderBase'      => $this->yui2loader->base,

diff --git a/lib/blocklib.php b/lib/blocklib.php
@@ -286,8 +286,8 @@ public function get_content_for_region($region, $output) {
      * @return string URL for moving block $this->movingblock to this position.
      */
     protected function get_move_target_url($region, $weight) {
-        return $this->page->url->out(array('bui_moveid' => $this->movingblock,
-                'bui_newregion' => $region, 'bui_newweight' => $weight, 'sesskey' => sesskey()), false);
+        return $this->page->url->out(false, array('bui_moveid' => $this->movingblock,
+                'bui_newregion' => $region, 'bui_newweight' => $weight, 'sesskey' => sesskey()));
     }
 
     /**
@@ -889,15 +889,15 @@ public function edit_controls($block) {
         }
 
         $controls = array();
-        $actionurl = $this->page->url->out(array('sesskey'=> sesskey()), false);
+        $actionurl = $this->page->url->out(false, array('sesskey'=> sesskey()));
 
         // Assign roles icon.
         if (has_capability('moodle/role:assign', $block->context)) {
             //TODO: please note it is sloppy to pass urls through page parameters!!
             //      it is shortened because some web servers (e.g. IIS by default) give
             //      a 'security' error if you try to pass a full URL as a GET parameter in another URL.
 
-            $return = $this->out_raw();
+            $return = $this->out(false);
             $return = str_replace($CFG->wwwroot . '/', '', $return);
 
             $controls[] = array('url' => $CFG->wwwroot . '/' . $CFG->admin .

diff --git a/lib/editor/tinymce/lib.php b/lib/editor/tinymce/lib.php
@@ -75,7 +75,7 @@ protected function get_init_params($elementid, array $options=null) {
         $strtime        = get_string('strftimetime');
         $strdate        = get_string('strftimedaydate');
         $lang           = str_replace('_utf8', '', current_language()); // use more standard language codes
-        $contentcss     = $PAGE->theme->editor_css_url()->out_raw();
+        $contentcss     = $PAGE->theme->editor_css_url()->out(false);
 
         $context = empty($options['context']) ? get_context_instance(CONTEXT_SYSTEM) : $options['context'];
         if (!empty($options['legacy'])) {

diff --git a/lib/listlib.php b/lib/listlib.php
@@ -267,7 +267,7 @@ function display_page_numbers() {
                     $html .= " $currentpage \n";
                 }
                 else {
-                    $html .= "<a href=\"".$this->pageurl->out(array($this->pageparamname => $currentpage))."\">";
+                    $html .= "<a href=\"".$this->pageurl->out(true, array($this->pageparamname => $currentpage))."\">";
                     $html .= " $currentpage </a>\n";
                 }
             }

diff --git a/lib/outputactions.php b/lib/outputactions.php
@@ -132,7 +132,7 @@ public function __construct($event, $url, $name='popup', $params=array()) {
                 $this->params[$var] = $params[$var];
             }
         }
-        parent::__construct($event, 'openpopup', array('url' => $url->out_raw(), 'name' => $name, 'options' => $this->get_js_options($params)));
+        parent::__construct($event, 'openpopup', array('url' => $url->out(false), 'name' => $name, 'options' => $this->get_js_options($params)));
     }
 
     /**

diff --git a/lib/outputcomponents.php b/lib/outputcomponents.php
@@ -1069,12 +1069,12 @@ public static function make_popup_form($baseurl, $name, $options, $formid, $sele
         }
 
         if (!empty($selected)) {
-            $selectedurl = $baseurl->out(array($name => $selected), false);
+            $selectedurl = $baseurl->out(false, array($name => $selected));
         }
 
         // Replace real value by formatted URLs
         foreach ($options as $value => $label) {
-            $options[$baseurl->out(array($name => $value), false)] = $label;
+            $options[$baseurl->out(false, array($name => $value))] = $label;
             unset($options[$value]);
         }
 

diff --git a/lib/outputlib.php b/lib/outputlib.php
@@ -835,7 +835,7 @@ protected function post_process($css) {
                 $replaced[$match[0]] = true;
                 $imagename = $match[2];
                 $component = rtrim($match[1], '|');
-                $imageurl = $this->pix_url($imagename, $component)->out_raw();
+                $imageurl = $this->pix_url($imagename, $component)->out(false);
                  // we do not need full url because the image.php is always in the same dir
                 $imageurl = str_replace("$CFG->httpswwwroot/theme/", '', $imageurl);
                 $css = str_replace($match[0], $imageurl, $css);

diff --git a/lib/outputrenderers.php b/lib/outputrenderers.php
@@ -740,7 +740,7 @@ public function block_controls($controls) {
         foreach ($controls as $control) {
             $controlshtml[] = html_writer::tag('a', array('class' => 'icon',
                     'title' => $control['caption'], 'href' => $control['url']),
-                    html_writer::empty_tag('img',  array('src' => $this->pix_url($control['icon'])->out_raw(),
+                    html_writer::empty_tag('img',  array('src' => $this->pix_url($control['icon'])->out(false),
                     'alt' => $control['caption'])));
         }
         return html_writer::tag('div', array('class' => 'commands'), implode('', $controlshtml));
@@ -820,7 +820,7 @@ protected function init_block_hider_js($bc) {
             $plaintitle = strip_tags($bc->title);
             $this->page->requires->js_function_call('new block_hider', array($bc->id, $userpref,
                     get_string('hideblocka', 'access', $plaintitle), get_string('showblocka', 'access', $plaintitle),
-                    $this->pix_url('t/switch_minus')->out_raw(), $this->pix_url('t/switch_plus')->out_raw()));
+                    $this->pix_url('t/switch_minus')->out(false), $this->pix_url('t/switch_plus')->out(false)));
         }
     }
 

diff --git a/lib/pagelib.php b/lib/pagelib.php
@@ -999,7 +999,7 @@ protected function starting_output() {
             if (empty($this->_block_actions_done)) {
                 $this->_block_actions_done = true;
                 if ($this->blocks->process_url_actions($this)) {
-                    redirect($this->url->out_raw());
+                    redirect($this->url->out(false));
                 }
             }
             $this->blocks->create_all_block_instances();
@@ -1403,7 +1403,7 @@ function url_get_path() {
      */
     function url_get_full($extraparams = array()) {
         debugging('Call to deprecated method moodle_page::url_get_full. Use $this->url->out() instead.');
-        return $this->url->out($extraparams);
+        return $this->url->out(true, $extraparams);
     }
 
     /**

diff --git a/lib/portfoliolib.php b/lib/portfoliolib.php
@@ -319,7 +319,7 @@ public function to_html($format=null, $addstr=null) {
         }
         // if we just want a url to redirect to, do it now
         if ($format == PORTFOLIO_ADD_FAKE_URL) {
-            return $url->out_raw();
+            return $url->out(false);
         }
 
         if (empty($addstr)) {

diff --git a/lib/weblib.php b/lib/weblib.php
@@ -491,7 +491,7 @@ public function get_query_string(array $overrideparams = null, $escaped = true)
      * @return string
      */
     public function __toString() {
-        return $this->out(null, true);
+        return $this->out(true);
     }
 
     /**
@@ -500,11 +500,15 @@ public function __toString() {
      * If you use the returned URL in HTML code, you want the escaped ampersands. If you use
      * the returned URL in HTTP headers, you want $escaped=false.
      *
-     * @param array $overrideparams params to add to the output url, these override existing ones with the same name.
      * @param boolean $escaped Use &amp; as params separator instead of plain &
+     * @param array $overrideparams params to add to the output url, these override existing ones with the same name.
      * @return string Resulting URL
      */
-    public function out(array $overrideparams = null, $escaped = true) {
+    public function out($escaped = true, array $overrideparams = null) {
+        if (!is_bool($escaped)) {
+            debugging('Escape parameter must be of type boolean, '.gettype($escaped).' given instead.');
+        }
+
         $uri = $this->out_omit_querystring();
 
         $querystring = $this->get_query_string($overrideparams, $escaped);
@@ -518,15 +522,6 @@ public function out(array $overrideparams = null, $escaped = true) {
         return $uri;
     }
 
-    /**
-     * Returns url in raw form without any escaping,
-     * useful especially when including urls and images in javascript.
-     * @return string
-     */
-    public function out_raw() {
-        return $this->out(null, false);
-    }
-
     /**
      * Returns url without parameters, everything before '?'.
      * @return string
@@ -552,7 +547,7 @@ public function out_omit_querystring() {
     public function out_action(array $overrideparams = null) {
         $overrideparams = (array)$overrideparams;
         $overrideparams = array('sesskey'=> sesskey()) + $overrideparams;
-        return $this->out($overrideparams);
+        return $this->out(true, $overrideparams);
     }
 
     /**
@@ -648,7 +643,7 @@ function prepare_url($url, $stripformparams=false) {
         if ($stripformparams) {
             $output = $url->out_omit_querystring();
         } else {
-            $output = $url->out_raw();
+            $output = $url->out(false);
         }
     }
 
@@ -657,7 +652,7 @@ function prepare_url($url, $stripformparams=false) {
         if (preg_match('/(.*)\/([A-Za-z0-9-_]*\.php)$/', $PAGE->url->out_omit_querystring(), $matches)) {
             return $matches[1] . "/$output";
         } else if ($output == '') {
-            return $PAGE->url->out_raw() . '#';
+            return $PAGE->url->out(false) . '#';
         } else {
             throw new coding_exception('Unrecognied URL scheme. Please check the formatting of the URL passed to this function. Absolute URLs are the preferred scheme.');
         }
@@ -2081,7 +2076,7 @@ function print_collapsible_region_start($classes, $id, $caption, $userpref = fal
     $output .= '</div><div id="' . $id . '_inner" class="collapsibleregioninner">';
     $PAGE->requires->js_function_call('new collapsible_region',
             array($id, $userpref, get_string('clicktohideshow'),
-            $OUTPUT->pix_url('t/collapsed')->out_raw(), $OUTPUT->pix_url('t/expanded')->out_raw()));
+            $OUTPUT->pix_url('t/collapsed')->out(false), $OUTPUT->pix_url('t/expanded')->out(false)));
 
     if ($return) {
         return $output;
@@ -2485,7 +2480,7 @@ function redirect($url, $message='', $delay=-1) {
     global $OUTPUT, $PAGE, $SESSION, $CFG;
 
     if ($url instanceof moodle_url) {
-        $url = $url->out_raw();
+        $url = $url->out(false);
     }
 
     if (!empty($CFG->usesid) && !isset($_COOKIE[session_name()])) {

diff --git a/mod/quiz/edit.php b/mod/quiz/edit.php
@@ -449,7 +449,7 @@ function module_specific_controls($totalnumber, $recurse, $category, $cmid, $cmo
 
 // Initialise the JavaScript.
 $quizeditconfig = new stdClass;
-$quizeditconfig->url = $thispageurl->out(array('qbanktool' => '0'));
+$quizeditconfig->url = $thispageurl->out(true, array('qbanktool' => '0'));
 $quizeditconfig->dialoglisteners = array();
 $numberoflisteners = max(quiz_number_of_pages($quiz->questions), 1);
 for ($pageiter = 1; $pageiter <= $numberoflisteners; $pageiter++) {
@@ -477,10 +477,10 @@ function module_specific_controls($totalnumber, $recurse, $category, $cmid, $cmo
 echo '<div class="questionbankwindow ' . $bankclass . 'sideblock">';
 echo '<div class="header"><div class="title"><h2>';
 echo get_string('questionbankcontents', 'quiz') .
-        ' <a href="' . $thispageurl->out(array('qbanktool' => '1')) .
+        ' <a href="' . $thispageurl->out(true, array('qbanktool' => '1')) .
        '" id="showbankcmd">[' . get_string('show').
        ']</a>
-       <a href="' . $thispageurl->out(array('qbanktool' => '0')) .
+       <a href="' . $thispageurl->out(true, array('qbanktool' => '0')) .
        '" id="hidebankcmd">[' . get_string('hide').
        ']</a>';
 echo '</h2></div></div><div class="content">';

diff --git a/mod/quiz/editlib.php b/mod/quiz/editlib.php
@@ -758,7 +758,7 @@ function quiz_print_randomquestion(&$question, &$pageurl, &$quiz, $quiz_qbanktoo
     $a->arrow = $OUTPUT->rarrow();
     $strshowcategorycontents = get_string('showcategorycontents', 'quiz', $a);
 
-    $openqbankurl = $pageurl->out(array('qbanktool' => 1,
+    $openqbankurl = $pageurl->out(true, array('qbanktool' => 1,
             'cat' => $category->id . ',' . $category->contextid));
     $linkcategorycontents = ' <a href="' . $openqbankurl . '">' . $strshowcategorycontents . '</a>';
 

diff --git a/mod/quiz/report/grading/report.php b/mod/quiz/report/grading/report.php
@@ -158,7 +158,7 @@ function display($quiz, $cm, $course) {
         /// find out current groups mode
 
         if ($groupmode = groups_get_activity_groupmode($this->cm)) {   // Groups are being used
-            groups_print_activity_menu($this->cm, $this->viewurl->out(array('userid'=>0, 'attemptid'=>0)));
+            groups_print_activity_menu($this->cm, $this->viewurl->out(true, array('userid'=>0, 'attemptid'=>0)));
         }
 
         if(empty($this->users)) {

diff --git a/mod/quiz/report/overview/report.php b/mod/quiz/report/overview/report.php
@@ -170,12 +170,12 @@ function display($quiz, $cm, $course) {
             $this->regrade_all_needed($quiz, $groupstudents);
         }
         if ($regradeall || $regradealldry || $regradealldrydo){
-            redirect($reporturl->out($displayoptions, false), '', 5);
+            redirect($reporturl->out(false, $displayoptions), '', 5);
         }
 
         if ($groupmode = groups_get_activity_groupmode($cm)) {   // Groups are being used
             if (!$table->is_downloading()) {
-                groups_print_activity_menu($cm, $reporturl->out($displayoptions));
+                groups_print_activity_menu($cm, $reporturl->out(treu, $displayoptions));
             }
         }
 
@@ -379,7 +379,7 @@ function display($quiz, $cm, $course) {
             $table->sortable(true, 'uniqueid');
 
             // Set up the table
-            $table->define_baseurl($reporturl->out($displayoptions));
+            $table->define_baseurl($reporturl->out(true, $displayoptions));
 
             $table->collapsible(false);
 

diff --git a/mod/quiz/report/responses/report.php b/mod/quiz/report/responses/report.php
@@ -138,7 +138,7 @@ function display($quiz, $cm, $course) {
 
         if ($groupmode = groups_get_activity_groupmode($cm)) {   // Groups are being used
             if (!$table->is_downloading()) {
-                groups_print_activity_menu($cm, $reporturl->out($displayoptions));
+                groups_print_activity_menu($cm, $reporturl->out(true, $displayoptions));
             }
         }
         // Print information on the number of existing attempts
@@ -306,7 +306,7 @@ function display($quiz, $cm, $course) {
             $table->sortable(true, 'concattedid');
 
             // Set up the table
-            $table->define_baseurl($reporturl->out($displayoptions));
+            $table->define_baseurl($reporturl->out(true, $displayoptions));
 
             $table->collapsible(true);
 

diff --git a/mod/workshop/allocation/manual/lib.php b/mod/workshop/allocation/manual/lib.php
@@ -87,7 +87,7 @@ public function init() {
                 }
             }
             $m = implode('-', $m);  // serialize message object to be passed via URL
-            redirect($PAGE->url->out(array('m' => $m), false));
+            redirect($PAGE->url->out(false, array('m' => $m)));
             break;
         case 'del':
             if (!confirm_sesskey()) {
@@ -119,7 +119,7 @@ public function init() {
                     }
                 }
                 $m = implode('-', $m);  // serialize message object to be passed via URL
-                redirect($PAGE->url->out(array('m' => $m), false));
+                redirect($PAGE->url->out(false, array('m' => $m)));
             }
             break;
         }

diff --git a/mod/workshop/allocation/random/lib.php b/mod/workshop/allocation/random/lib.php
@@ -66,7 +66,7 @@ public function init() {
         $customdata['workshop'] = $this->workshop;
         $this->mform = new workshop_random_allocator_form($PAGE->url, $customdata);
         if ($this->mform->is_cancelled()) {
-            redirect($PAGE->url->out_raw());
+            redirect($PAGE->url->out(false));
         } else if ($settings = $this->mform->get_data()) {
             // process validated data
             if (!confirm_sesskey()) {

diff --git a/question/category_class.php b/question/category_class.php
@@ -93,7 +93,7 @@ public function item_html($extraargs = array()){
         /// Each section adds html to be displayed as part of this list item
         $questionbankurl = "{$CFG->wwwroot}/question/edit.php?".
                 $this->parentlist->pageurl->get_query_string(array('category'=>"$category->id,$category->contextid"));
-        $catediturl = $this->parentlist->pageurl->out(array('edit'=>$this->id));
+        $catediturl = $this->parentlist->pageurl->out(true, array('edit'=>$this->id));
         $item = "<b><a title=\"{$str->edit}\" href=\"$catediturl\">".$category->name ."</a></b> <a title=\"$editqestions\" href=\"$questionbankurl\">".'('.$category->questioncount.')</a>';
 
         $item .= '&nbsp;'. $category->info;

diff --git a/question/editlib.php b/question/editlib.php
@@ -802,7 +802,7 @@ public function __construct($contexts, $pageurl, $course, $cm = null) {
         // Create the url of the new question page to forward to.
         // TODO: it is sloppy to pass around full URLs through page parameters and some servers do not like that
         $this->editquestionurl = new moodle_url("$CFG->wwwroot/question/question.php",
-                array('returnurl' => urlencode($pageurl->out_raw())));
+                array('returnurl' => urlencode($pageurl->out(false))));
         if ($cm !== null){
             $this->editquestionurl->param('cmid', $cm->id);
         } else {
@@ -999,7 +999,7 @@ public function new_sort_url($sort, $newsortreverse) {
         if (count($newsort) > question_bank_view::MAX_SORTS) {
             $newsort = array_slice($newsort, 0, question_bank_view::MAX_SORTS, true);
         }
-        return $this->baseurl->out($this->sort_to_params($newsort));
+        return $this->baseurl->out(true, $this->sort_to_params($newsort));
     }
 
     protected function build_query_sql($category, $recurse, $showhidden) {
@@ -1078,11 +1078,11 @@ public function base_url() {
     }
 
     public function edit_question_url($questionid) {
-        return $this->editquestionurl->out(array('id' => $questionid));
+        return $this->editquestionurl->out(true, array('id' => $questionid));
     }
 
     public function move_question_url($questionid) {
-        return $this->editquestionurl->out(array('id' => $questionid, 'movecontext' => 1));
+        return $this->editquestionurl->out(true, array('id' => $questionid, 'movecontext' => 1));
     }
 
     public function preview_question_url($questionid) {
@@ -1422,7 +1422,7 @@ public function process_actions() {
                         $checkforfiles = true;
                     }
                 }
-                $returnurl = $this->baseurl->out(array('category'=>"$tocategoryid,$contextid"));
+                $returnurl = $this->baseurl->out(true, array('category'=>"$tocategoryid,$contextid"));
                 if (!$checkforfiles){
                     if (!question_move_questions_to_category(implode(',', $questionids), $tocategory->id)) {
                         print_error('errormovingquestions', 'question', $returnurl, $questionids);

diff --git a/question/question.php b/question/question.php
@@ -219,7 +219,7 @@
     $returnurl = new moodle_url($returnurl);
     $returnurl->param('category', $fromform->category);
     // TODO: it is sloppy to pass arounf full URLs through page parameters and some servers do not like that
-    $returnurl = $returnurl->out_raw();
+    $returnurl = $returnurl->out(false);
 
     /// Call the appropriate method.
     if ($movecontext) {
-Original file line number
+Diff line change
@@ Expand Up @@
                     $this->params[$var] = $params[$var];
                 }
             }
-            parent::__construct($event, 'openpopup', array('url' => $url->out_raw(), 'name' => $name, 'options' => $this->get_js_options($params)));
+            parent::__construct($event, 'openpopup', array('url' => $url->out(false), 'name' => $name, 'options' => $this->get_js_options($params)));
         }
         /**
@@ Expand Down @@