MDL-54926 search: Reduce over-escaping in results

search/classes/document.php

@@ -555,6 +555,9 @@ class document implements \renderable, \templatable {
      * Although content is a required field when setting up the document, it accepts '' (empty) values
      * as they may be the result of striping out HTML.
      *
+     * SECURITY NOTE: It is the responsibility of the document to properly escape any text to be displayed.
+     * The renderer will output the content without any further cleaning.
+     *
      * @param renderer_base $output The renderer.
      * @return array
      */
@@ -580,13 +583,13 @@ class document implements \renderable, \templatable {
             if (count($files) > 1) {
                 $filenames = array();
                 foreach ($files as $file) {
-                    $filenames[] = $file->get_filename();
+                    $filenames[] = format_string($file->get_filename(), true, array('context' => $this->get('contextid')));
                 }
                 $data['multiplefiles'] = true;
                 $data['filenames'] = $filenames;
             } else {
                 $file = reset($files);
-                $data['filename'] = $file->get_filename();
+                $data['filename'] = format_string($file->get_filename(), true, array('context' => $this->get('contextid')));
             }
         }
 

search/templates/result.mustache

@@ -75,7 +75,7 @@
     {{/description2}}
     {{#filename}}
         <div class="result-content-filename">
-            {{#str}}matchingfile, search, {{filename}}{{/str}}
+            {{#str}}matchingfile, search, {{{filename}}}{{/str}}
         </div>
     {{/filename}}
     {{#multiplefiles}}
@@ -90,9 +90,9 @@
     {{/multiplefiles}}
     <div class="result-context-info">
         <a href="{{{contexturl}}}">{{#str}}viewresultincontext, search{{/str}}</a> -
-        <a href="{{{courseurl}}}">{{#str}}incourse, search, {{coursefullname}}{{/str}}</a>
+        <a href="{{{courseurl}}}">{{#str}}incourse, search, {{{coursefullname}}}{{/str}}</a>
         {{#userfullname}}
-            - <a href="{{{userurl}}}">{{#str}}byname, moodle, {{userfullname}}{{/str}}</a>
+            - <a href="{{{userurl}}}">{{#str}}byname, moodle, {{{userfullname}}}{{/str}}</a>
         {{/userfullname}}
     </div>
 </div>

search/tests/document_test.php

@@ -0,0 +1,114 @@
+<?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/>.
+
+/**
+ * Search document unit tests.
+ *
+ * @package     core_search
+ * @category    phpunit
+ * @copyright   2016 Eric Merrill {@link http://www.merrilldigital.com}
+ * @license     http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
+ */
+
+defined('MOODLE_INTERNAL') || die();
+
+require_once(__DIR__ . '/fixtures/testable_core_search.php');
+require_once(__DIR__ . '/fixtures/mock_search_area.php');
+
+/**
+ * Unit tests for search document.
+ *
+ * @package     core_search
+ * @category    phpunit
+ * @copyright   2016 Eric Merrill {@link http://www.merrilldigital.com}
+ * @license     http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
+ */
+class search_document_testcase extends advanced_testcase {
+
+    /**
+     * @var Instace of core_search_generator.
+     */
+    protected $generator = null;
+
+    public function setUp() {
+        $this->resetAfterTest();
+        set_config('enableglobalsearch', true);
+
+        // Set \core_search::instance to the mock_search_engine as we don't require the search engine to be working to test this.
+        $search = testable_core_search::instance();
+
+        $this->generator = self::getDataGenerator()->get_plugin_generator('core_search');
+        $this->generator->setup();
+    }
+
+    /**
+     * Adding this test here as get_areas_user_accesses process is the same, results just depend on the context level.
+     *
+     * @return void
+     */
+    public function test_search_user_accesses() {
+        global $DB, $PAGE;
+
+        $area = new \core_mocksearch\search\mock_search_area();
+        $renderer = $PAGE->get_renderer('core_search');
+        $engine = new \mock_search\engine();
+
+        $course = $this->getDataGenerator()->create_course(array('fullname' => 'Course & Title'));
+        $coursectx = context_course::instance($course->id);
+        $user = $this->getDataGenerator()->create_user(array('firstname' => 'User', 'lastname' => 'Escape & Name'));
+        $this->getDataGenerator()->enrol_user($user->id, $course->id, 'teacher');
+
+        // Make a record to enter in the search area.
+        $record = new \stdClass();
+        $record->title = 'Escape & Title';
+        $record->content = 'Escape & Content';
+        $record->description1 = 'Escape & Description1';
+        $record->description2 = 'Escape & Description2';
+        $record->userid = $user->id;
+        $record->courseid = $course->id;
+        $record = $this->generator->create_record($record);
+
+        // Convert to a 'doc data' type format.
+        $docdata = $area->convert_record_to_doc_array($record);
+
+        // First see that the docuemnt has the right information, unescaped.
+        $doc = $engine->to_document($area, $docdata);
+        $this->assertEquals('Escape & Title', $doc->get('title'));
+        $this->assertEquals('Escape & Content', $doc->get('content'));
+        $this->assertEquals('Escape & Description1', $doc->get('description1'));
+        $this->assertEquals('Escape & Description2', $doc->get('description2'));
+        $this->assertEquals('User Escape & Name', $doc->get('userfullname'));
+        $this->assertEquals('Course & Title', $doc->get('coursefullname'));
+
+        // Export for template, and see if it is escaped.
+        $export = $doc->export_for_template($renderer);
+        $this->assertEquals('Escape &amp; Title', $export['title']);
+        $this->assertEquals('Escape &amp; Content', $export['content']);
+        $this->assertEquals('Escape &amp; Description1', $export['description1']);
+        $this->assertEquals('Escape &amp; Description2', $export['description2']);
+        $this->assertEquals('User Escape &amp; Name', $export['userfullname']);
+        $this->assertEquals('Course &amp; Title', $export['coursefullname']);
+    }
+
+    public function tearDown() {
+        // For unit tests before PHP 7, teardown is called even on skip. So only do our teardown if we did setup.
+        if ($this->generator) {
+            // Moodle DML freaks out if we don't teardown the temp table after each run.
+            $this->generator->teardown();
+            $this->generator = null;
+        }
+    }
+}

search/tests/fixtures/mock_search_area.php

@@ -45,6 +45,19 @@ class mock_search_area extends \core_search\area\base {
         return $DB->get_recordset_sql("SELECT * FROM {temp_mock_search_area} WHERE timemodified >= ?", array($modifiedfrom));
     }
 
+
+    /**
+     * A helper function that will turn a record into 'data array', for use with document building.
+     */
+    public function convert_record_to_doc_array($record) {
+        $docdata = (array)unserialize($record->info);
+        $docdata['areaid'] = $this->get_area_id();
+        $docdata['itemid'] = $record->id;
+        $docdata['modified'] = $record->timemodified;
+
+        return $docdata;
+    }
+
     public function get_document($record, $options = array()) {
         global $USER;
 
@@ -54,6 +67,8 @@ class mock_search_area extends \core_search\area\base {
         $doc = \core_search\document_factory::instance($record->id, $this->componentname, $this->areaname);
         $doc->set('title', $info->title);
         $doc->set('content', $info->content);
+        $doc->set('description1', $info->description1);
+        $doc->set('description1', $info->description2);
         $doc->set('contextid', $info->contextid);
         $doc->set('courseid', $info->courseid);
         $doc->set('userid', $info->userid);

search/tests/fixtures/mock_search_engine.php

@@ -49,6 +49,10 @@ class engine extends \core_search\engine {
         return null;
     }
 
+    public function to_document(\core_search\area\base $searcharea, $docdata) {
+        return parent::to_document($searcharea, $docdata);
+    }
+
     public function get_course($courseid) {
         return parent::get_course($courseid);
     }

search/tests/generator/lib.php

@@ -99,6 +99,18 @@ class core_search_generator extends component_generator_base {
             $info->content = $options->content;
         }
 
+        if (!isset($options->description1)) {
+            $info->description1 = 'Description 1.';
+        } else {
+            $info->description1 = $options->description1;
+        }
+
+        if (!isset($options->description2)) {
+            $info->description2 = 'Description 2.';
+        } else {
+            $info->description2 = $options->description2;
+        }
+
         if (!isset($options->title)) {
             $info->title = 'A basic title';
         } else {