From 7a3707797dc0767bf052bf240c768ef04f43b950 Mon Sep 17 00:00:00 2001 From: Sara Arjona Date: Fri, 27 Apr 2018 12:42:53 +0200 Subject: [PATCH] MDL-62240 privacy: Avoid error when path is higher than allowed A new method shorten_filenames has been added to moodlelib. --- lib/moodlelib.php | 19 +++ lib/tests/moodlelib_test.php | 131 ++++++++++++++++ .../local/request/moodle_content_writer.php | 5 +- privacy/tests/moodle_content_writer_test.php | 145 ++++++++++++++++++ 4 files changed, 299 insertions(+), 1 deletion(-) diff --git a/lib/moodlelib.php b/lib/moodlelib.php index e7d1ad2d2a1..e1b368ab397 100644 --- a/lib/moodlelib.php +++ b/lib/moodlelib.php @@ -8290,6 +8290,25 @@ function shorten_filename($filename, $length = MAX_FILENAME_SIZE, $includehash = return $shortened; } +/** + * Shortens a given array of filenames by removing characters positioned after the ideal string length. + * + * @param array $path The paths to reduce the length. + * @param int $length Ideal string length + * @param bool $includehash Whether to include a file hash in the shortened version. This ensures uniqueness. + * @return array $result Shortened paths in array. + */ +function shorten_filenames(array $path, $length = MAX_FILENAME_SIZE, $includehash = false) { + $result = null; + + $result = array_reduce($path, function($carry, $singlepath) use ($length, $includehash) { + $carry[] = shorten_filename($singlepath, $length, $includehash); + return $carry; + }, []); + + return $result; +} + /** * Given dates in seconds, how many weeks is the date from startdate * The first week is 1, the second 2 etc ... diff --git a/lib/tests/moodlelib_test.php b/lib/tests/moodlelib_test.php index f49fb7816b4..80dc799168d 100644 --- a/lib/tests/moodlelib_test.php +++ b/lib/tests/moodlelib_test.php @@ -1110,6 +1110,137 @@ class core_moodlelib_testcase extends advanced_testcase { $this->assertSame($expected, shorten_filename($filename, $length, $includehash)); } + /** + * Provider for long filenames and its expected result, with and without hash. + * + * @return array of ($filename, $length, $expected, $includehash) + */ + public function shorten_filenames_provider() { + $shortfilename = 'sed ut perspiciatis unde omnis iste natus error sit voluptatem accusantium doloremque'; + $longfilename = 'sed ut perspiciatis unde omnis iste natus error sit voluptatem accusantium doloremque laudantium totam rem'; + $extfilename = $longfilename.'.zip'; + $expected = 'sed ut perspiciatis unde omnis iste natus error sit voluptatem accusantium doloremque laudantium tot'; + $expectedwithhash = 'sed ut perspiciatis unde omnis iste natus error sit voluptatem accusantium doloremque l - 3bec1da8b8'; + $expectedext = 'sed ut perspiciatis unde omnis iste natus error sit voluptatem accusantium doloremque laudantium tot.zip'; + $expectedextwithhash = 'sed ut perspiciatis unde omnis iste natus error sit voluptatem accusantium doloremque l - 3bec1da8b8.zip'; + $expected50 = 'sed ut perspiciatis unde omnis iste natus error si'; + $expected50withhash = 'sed ut perspiciatis unde omnis iste n - 3bec1da8b8'; + $expected50ext = 'sed ut perspiciatis unde omnis iste natus error si.zip'; + $expected50extwithhash = 'sed ut perspiciatis unde omnis iste n - 3bec1da8b8.zip'; + $expected50short = 'sed ut perspiciatis unde omnis iste n - 5fb6543490'; + + return [ + 'Empty array without hash' => [ + [], + null, + [], + false, + ], + 'Empty array with hash' => [ + [], + null, + [], + true, + ], + 'Array with less than 100 characters' => [ + [$shortfilename, $shortfilename, $shortfilename], + null, + [$shortfilename, $shortfilename, $shortfilename], + false, + ], + 'Array with more than 100 characters without hash' => [ + [$longfilename, $longfilename, $longfilename], + null, + [$expected, $expected, $expected], + false, + ], + 'Array with more than 100 characters with hash' => [ + [$longfilename, $longfilename, $longfilename], + null, + [$expectedwithhash, $expectedwithhash, $expectedwithhash], + true, + ], + 'Array with more than 100 characters with extension' => [ + [$extfilename, $extfilename, $extfilename], + null, + [$expectedext, $expectedext, $expectedext], + false, + ], + 'Array with more than 100 characters with extension and hash' => [ + [$extfilename, $extfilename, $extfilename], + null, + [$expectedextwithhash, $expectedextwithhash, $expectedextwithhash], + true, + ], + 'Array with more than 100 characters mix (short, long, with extension) without hash' => [ + [$shortfilename, $longfilename, $extfilename], + null, + [$shortfilename, $expected, $expectedext], + false, + ], + 'Array with more than 100 characters mix (short, long, with extension) with hash' => [ + [$shortfilename, $longfilename, $extfilename], + null, + [$shortfilename, $expectedwithhash, $expectedextwithhash], + true, + ], + 'Array with less than 50 characters without hash' => [ + [$longfilename, $longfilename, $longfilename], + 50, + [$expected50, $expected50, $expected50], + false, + ], + 'Array with less than 50 characters with hash' => [ + [$longfilename, $longfilename, $longfilename], + 50, + [$expected50withhash, $expected50withhash, $expected50withhash], + true, + ], + 'Array with less than 50 characters with extension' => [ + [$extfilename, $extfilename, $extfilename], + 50, + [$expected50ext, $expected50ext, $expected50ext], + false, + ], + 'Array with less than 50 characters with extension and hash' => [ + [$extfilename, $extfilename, $extfilename], + 50, + [$expected50extwithhash, $expected50extwithhash, $expected50extwithhash], + true, + ], + 'Array with less than 50 characters mix (short, long, with extension) without hash' => [ + [$shortfilename, $longfilename, $extfilename], + 50, + [$expected50, $expected50, $expected50ext], + false, + ], + 'Array with less than 50 characters mix (short, long, with extension) with hash' => [ + [$shortfilename, $longfilename, $extfilename], + 50, + [$expected50short, $expected50withhash, $expected50extwithhash], + true, + ], + ]; + } + + /** + * Test the {@link shorten_filenames()} method. + * + * @dataProvider shorten_filenames_provider + * + * @param string $filenames + * @param int $length + * @param string $expected + * @param boolean $includehash + */ + public function test_shorten_filenames($filenames, $length, $expected, $includehash) { + if (null === $length) { + $length = MAX_FILENAME_SIZE; + } + + $this->assertSame($expected, shorten_filenames($filenames, $length, $includehash)); + } + public function test_usergetdate() { global $USER, $CFG, $DB; $this->resetAfterTest(); diff --git a/privacy/classes/local/request/moodle_content_writer.php b/privacy/classes/local/request/moodle_content_writer.php index de607ccdf82..4e8f1009a56 100644 --- a/privacy/classes/local/request/moodle_content_writer.php +++ b/privacy/classes/local/request/moodle_content_writer.php @@ -244,7 +244,7 @@ class moodle_content_writer implements content_writer { $path = []; $contexts = array_reverse($this->context->get_parent_contexts(true)); foreach ($contexts as $context) { - $path[] = clean_param($context->get_context_name(), PARAM_FILE); + $path[] = shorten_filename(clean_param($context->get_context_name(), PARAM_FILE), MAX_FILENAME_SIZE, true); } return $path; @@ -258,6 +258,9 @@ class moodle_content_writer implements content_writer { * @return string The fully-qualfiied file path. */ protected function get_path(array $subcontext, string $name) : string { + $subcontext = shorten_filenames($subcontext, MAX_FILENAME_SIZE, true); + $name = shorten_filename($name, MAX_FILENAME_SIZE, true); + // Combine the context path, and the subcontext data. $path = array_merge( $this->get_context_path(), diff --git a/privacy/tests/moodle_content_writer_test.php b/privacy/tests/moodle_content_writer_test.php index b683348186f..a045829e89b 100644 --- a/privacy/tests/moodle_content_writer_test.php +++ b/privacy/tests/moodle_content_writer_test.php @@ -913,6 +913,151 @@ class moodle_content_writer_test extends advanced_testcase { ]; } + /** + * Test that exported data is shortened when exceeds the limit. + * + * @dataProvider long_filename_provider + * @param string $longtext + * @param string $expected + * @param string $text + */ + public function test_export_data_long_filename($longtext, $expected, $text) { + $context = \context_system::instance(); + $subcontext = [$longtext]; + $data = (object) ['key' => $text]; + + $writer = $this->get_writer_instance() + ->set_context($context) + ->export_data($subcontext, $data); + + $fileroot = $this->fetch_exported_content($writer); + + $contextpath = $this->get_context_path($context, $subcontext, 'data.json'); + $expectedpath = 'System/'.$expected.'/data.json'; + $this->assertEquals($expectedpath, $contextpath); + + $json = $fileroot->getChild($contextpath)->getContent(); + $this->assertRegExp("/$text/", $json); + + $expanded = json_decode($json); + $this->assertEquals($data, $expanded); + } + + /** + * Test that exported related data is shortened when exceeds the limit. + * + * @dataProvider long_filename_provider + * @param string $longtext + * @param string $expected + * @param string $text + */ + public function test_export_related_data_long_filename($longtext, $expected, $text) { + $context = \context_system::instance(); + $subcontext = [$longtext]; + $data = (object) ['key' => $text]; + + $writer = $this->get_writer_instance() + ->set_context($context) + ->export_related_data($subcontext, 'name', $data); + + $fileroot = $this->fetch_exported_content($writer); + + $contextpath = $this->get_context_path($context, $subcontext, 'name.json'); + $expectedpath = 'System/'.$expected.'/name.json'; + $this->assertEquals($expectedpath, $contextpath); + + $json = $fileroot->getChild($contextpath)->getContent(); + $this->assertRegExp("/$text/", $json); + + $expanded = json_decode($json); + $this->assertEquals($data, $expanded); + } + + /** + * Test that exported metadata is shortened when exceeds the limit. + * + * @dataProvider long_filename_provider + * @param string $longtext + * @param string $expected + * @param string $text + */ + public function test_export_metadata_long_filename($longtext, $expected, $text) { + $context = \context_system::instance(); + $subcontext = [$longtext]; + $data = (object) ['key' => $text]; + + $writer = $this->get_writer_instance() + ->set_context($context) + ->export_metadata($subcontext, $text, $text, $text); + + $fileroot = $this->fetch_exported_content($writer); + + $contextpath = $this->get_context_path($context, $subcontext, 'metadata.json'); + $expectedpath = 'System/'.$expected.'/metadata.json'; + $this->assertEquals($expectedpath, $contextpath); + + $json = $fileroot->getChild($contextpath)->getContent(); + $this->assertRegExp("/$text.*$text.*$text/", $json); + + $expanded = json_decode($json); + $this->assertTrue(isset($expanded->$text)); + $this->assertEquals($text, $expanded->$text->value); + $this->assertEquals($text, $expanded->$text->description); + } + + /** + * Test that exported user preference is shortened when exceeds the limit. + * + * @dataProvider long_filename_provider + * @param string $longtext + * @param string $expected + * @param string $text + */ + public function test_export_user_preference_long_filename($longtext, $expected, $text) { + $this->resetAfterTest(); + + if (!array_key_exists('json', core_filetypes::get_types())) { + // Add json as mime type to avoid lose the extension when shortening filenames. + core_filetypes::add_type('json', 'application/json', 'archive', [], '', 'JSON file archive'); + } + $expectedpath = 'System/User preferences/'.$expected.'.json'; + + $context = \context_system::instance(); + $component = $longtext; + + $writer = $this->get_writer_instance() + ->set_context($context) + ->export_user_preference($component, $text, $text, $text); + + $fileroot = $this->fetch_exported_content($writer); + + $contextpath = $this->get_context_path($context, [get_string('userpreferences')], "{$component}.json"); + $this->assertEquals($expectedpath, $contextpath); + + $json = $fileroot->getChild($contextpath)->getContent(); + $this->assertRegExp("/$text.*$text.*$text/", $json); + + $expanded = json_decode($json); + $this->assertTrue(isset($expanded->$text)); + $this->assertEquals($text, $expanded->$text->value); + $this->assertEquals($text, $expanded->$text->description); + } + + /** + * Provider for long filenames. + * + * @return array + */ + public function long_filename_provider() { + return [ + 'More than 100 characters' => [ + 'Etiam sit amet dui vel leo blandit viverra. Proin viverra suscipit velit. Aenean efficitur suscipit nibh nec suscipit', + 'Etiam sit amet dui vel leo blandit viverra. Proin viverra suscipit velit. Aenean effici - 22f7a5030d', + 'value', + ], + ]; + } + /** * Get a fresh content writer. *