From 4e2b5191d47ab853f25c3371bbb7e3794755a939 Mon Sep 17 00:00:00 2001 From: Eric Merrill Date: Tue, 28 Jun 2016 16:33:29 -0400 Subject: [PATCH] MDL-54926 search: Reduce over-escaping in results --- search/classes/document.php | 7 +- search/templates/result.mustache | 6 +- search/tests/document_test.php | 114 +++++++++++++++++++ search/tests/fixtures/mock_search_area.php | 15 +++ search/tests/fixtures/mock_search_engine.php | 4 + search/tests/generator/lib.php | 12 ++ 6 files changed, 153 insertions(+), 5 deletions(-) create mode 100644 search/tests/document_test.php diff --git a/search/classes/document.php b/search/classes/document.php index 61658f5b2eb..42f72299aa9 100644 --- a/search/classes/document.php +++ b/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'))); } } diff --git a/search/templates/result.mustache b/search/templates/result.mustache index f28c00a47fc..dc276950e39 100644 --- a/search/templates/result.mustache +++ b/search/templates/result.mustache @@ -75,7 +75,7 @@ {{/description2}} {{#filename}}
- {{#str}}matchingfile, search, {{filename}}{{/str}} + {{#str}}matchingfile, search, {{{filename}}}{{/str}}
{{/filename}} {{#multiplefiles}} @@ -90,9 +90,9 @@ {{/multiplefiles}}
{{#str}}viewresultincontext, search{{/str}} - - {{#str}}incourse, search, {{coursefullname}}{{/str}} + {{#str}}incourse, search, {{{coursefullname}}}{{/str}} {{#userfullname}} - - {{#str}}byname, moodle, {{userfullname}}{{/str}} + - {{#str}}byname, moodle, {{{userfullname}}}{{/str}} {{/userfullname}}
diff --git a/search/tests/document_test.php b/search/tests/document_test.php new file mode 100644 index 00000000000..d7196f3fbf8 --- /dev/null +++ b/search/tests/document_test.php @@ -0,0 +1,114 @@ +. + +/** + * 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 & Title', $export['title']); + $this->assertEquals('Escape & Content', $export['content']); + $this->assertEquals('Escape & Description1', $export['description1']); + $this->assertEquals('Escape & Description2', $export['description2']); + $this->assertEquals('User Escape & Name', $export['userfullname']); + $this->assertEquals('Course & 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; + } + } +} diff --git a/search/tests/fixtures/mock_search_area.php b/search/tests/fixtures/mock_search_area.php index 8a638d7a759..6ede9afabe8 100644 --- a/search/tests/fixtures/mock_search_area.php +++ b/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); diff --git a/search/tests/fixtures/mock_search_engine.php b/search/tests/fixtures/mock_search_engine.php index 29385768fc4..88b3472fd37 100644 --- a/search/tests/fixtures/mock_search_engine.php +++ b/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); } diff --git a/search/tests/generator/lib.php b/search/tests/generator/lib.php index b38ea140be7..4abf191a956 100644 --- a/search/tests/generator/lib.php +++ b/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 {