From 95077da502b86fff8c63a87182480558af19f279 Mon Sep 17 00:00:00 2001 From: Matthew Hilton Date: Tue, 17 Jan 2023 12:11:25 +1000 Subject: [PATCH 1/7] MDL-73317 session: Improve session diff detection Previously, newly added keys to the session were not detected. Objects with the same properties were also incorrectly reported as different. This commit improves this, and updates the unit tests to reflect the new functionality. --- lib/classes/session/manager.php | 74 +++++++++++++++---- lib/tests/session_manager_test.php | 115 +++++++++++++++++------------ 2 files changed, 127 insertions(+), 62 deletions(-) diff --git a/lib/classes/session/manager.php b/lib/classes/session/manager.php index e60a29d4298..901bd1f206f 100644 --- a/lib/classes/session/manager.php +++ b/lib/classes/session/manager.php @@ -1384,25 +1384,69 @@ class manager { } /** - * Compares two arrays outputs the difference. + * Compares two arrays and outputs the difference. * - * Note this does not use array_diff_assoc due to - * the use of stdClasses in Moodle sessions. + * Note - checking between objects and array type is only done at the top level. + * Any changes in types below the top level will not be detected. + * However, if their values are the same, they will be treated as equal. * - * @param array $array1 - * @param array $array2 + * Any changes, such as removals, edits or additions will be detected. + * + * @param array $previous + * @param array $current * @return array */ - private static function array_session_diff(array $array1, array $array2) : array { - $difference = []; - foreach ($array1 as $key => $value) { - if (!isset($array2[$key])) { - $difference[$key] = $value; - } else if ($array2[$key] !== $value) { - $difference[$key] = $value; - } - } + private static function array_session_diff(array $previous, array $current) : array { + // To use array_udiff_uassoc, the first array must have the most keys; this ensures every key is checked. + // To do this, we first need to sort them by the length of their keys. + $arrays = [$current, $previous]; - return $difference; + // Sort them by the length of their keys. + usort($arrays, function ($a, $b) { + return count(array_keys($b)) - count(array_keys($a)); + }); + + // The largest is the first value in the $arrays, after sorting. + // The smallest is then the last one. + // If they are the same size, it does not matter which is which. + $largest = $arrays[0]; + $smallest = $arrays[1]; + + // Defines a function that casts the values to arrays. + // This is so the properties are compared, instead any object's identities. + $casttoarray = function ($value) { + return json_decode(json_encode($value), true); + }; + + // Defines a function that compares all keys by their string value. + $keycompare = function ($a, $b) { + return strcmp($a, $b); + }; + + // Defines a function that compares all values by first their type, and then their values. + // If the value contains any objects, they are cast to arrays before comparison. + $valcompare = function ($a, $b) use ($casttoarray) { + // First compare type. + // If they are not the same type, they are definitely not the same. + // Note we do not check types recursively. + if (gettype($a) !== gettype($b)) { + return 1; + } + + // Next compare value. Cast any objects to arrays to compare their properties, + // instead of the identitiy of the object itself. + $v1 = $casttoarray($a); + $v2 = $casttoarray($b); + + if ($v1 !== $v2) { + return 1; + } + + return 0; + }; + + // Apply the comparison functions to the two given session arrays, + // making sure to use the largest array first, so that all keys are considered. + return array_udiff_uassoc($largest, $smallest, $valcompare, $keycompare); } } diff --git a/lib/tests/session_manager_test.php b/lib/tests/session_manager_test.php index 1d13d0fc5fb..6f9e223e794 100644 --- a/lib/tests/session_manager_test.php +++ b/lib/tests/session_manager_test.php @@ -846,60 +846,81 @@ class session_manager_test extends \advanced_testcase { $this->assertEquals('/good.php?id=4', $SESSION->recentsessionlocks[0]['url']); } - public function test_array_session_diff_same_array() { - $a = []; - $a['c'] = new \stdClass(); - $a['c']->o = new \stdClass(); - $a['c']->o->o = new \stdClass(); - $a['c']->o->o->l = 'cool'; + /** + * Data provider for the array_session_diff function. + * + * @return array + */ + public function array_session_diff_provider() { + // Create an instance of this object so the comparison object's identities are the same. + // Used in one of the tests below. + $compareobjectb = (object) ['array' => 'b']; - $class = new \ReflectionClass('\core\session\manager'); - $method = $class->getMethod('array_session_diff'); - $method->setAccessible(true); - - $result = $method->invokeArgs(null, [$a, $a]); - - $this->assertEmpty($result); + return [ + 'both same objects' => [ + 'a' => ['example' => (object) ['array' => 'a']], + 'b' => ['example' => (object) ['array' => 'a']], + 'expected' => [], + ], + 'both same arrays' => [ + 'a' => ['example' => ['array' => 'a']], + 'b' => ['example' => ['array' => 'a']], + 'expected' => [], + ], + 'both the same with nested objects' => [ + 'a' => ['example' => (object) ['array' => 'a', 'deeper' => (object) []]], + 'b' => ['example' => (object) ['array' => 'a', 'deeper' => (object) []]], + 'expected' => [], + ], + 'first array larger' => [ + 'a' => ['x' => 1, 'y' => 2], + 'b' => ['x' => 1], + 'expected' => ['y' => 2] + ], + 'second array larger' => [ + 'a' => ['x' => 1], + 'b' => ['x' => 1, 'y' => 2], + 'expected' => ['y' => 2] + ], + 'objects with different values but same keys' => [ + 'a' => ['example' => (object) ['array' => 'a']], + 'b' => ['example' => $compareobjectb], + 'expected' => ['example' => $compareobjectb] + ], + 'different arrays with top level indexes' => [ + 'a' => ['x', 'y'], + 'b' => ['x', 'y', 'z'], + 'expected' => [2 => 'z'] + ], + 'different types but same values as first level' => [ + 'a' => ['example' => (object) ['array' => 'a']], + 'b' => ['example' => ['array' => 'a']], + 'expected' => ['example' => ['array' => 'a']] + ], + 'different types but same values nested' => [ + 'a' => ['example' => (object) ['array' => ['a' => 'test']]], + 'b' => ['example' => (object) ['array' => (object) ['a' => 'test']]], + // Type checking is not done further than the first level, so we expect no difference. + 'expected' => [] + ] + ]; } - public function test_array_session_diff_first_array_larger() { - $a = []; - $a['stdClass'] = new \stdClass(); - $a['stdClass']->attribute = 'This is an attribute'; - $a['array'] = ['array', 'contents']; - - $b = []; - $b['array'] = ['array', 'contents']; - + /** + * Tests array diff method in various situations. + * + * @dataProvider array_session_diff_provider + * @covers \core\session\manager::array_session_diff + * @param array $a first value. + * @param array $b second value to compare to $a. + * @param array $expected the expected difference. + */ + public function test_array_session_diff(array $a, array $b, array $expected) { $class = new \ReflectionClass('\core\session\manager'); $method = $class->getMethod('array_session_diff'); $method->setAccessible(true); $result = $method->invokeArgs(null, [$a, $b]); - - $expected = []; - $expected['stdClass'] = new \stdClass(); - $expected['stdClass']->attribute = 'This is an attribute'; - $this->assertEquals($expected, $result); - } - - public function test_array_session_diff_second_array_larger() { - $a = []; - $a['array'] = ['array', 'contents']; - - $b = []; - $b['stdClass'] = new \stdClass(); - $b['stdClass']->attribute = 'This is an attribute'; - $b['array'] = ['array', 'contents']; - - $class = new \ReflectionClass('\core\session\manager'); - $method = $class->getMethod('array_session_diff'); - $method->setAccessible(true); - - $result = $method->invokeArgs(null, [$a, $b]); - - // It's empty because the first array contains all the contents of the second. - $expected = []; - $this->assertEquals($expected, $result); + $this->assertSame($expected, $result); } } From 9c8d8502c03f8011841b49846454aab31f01d56e Mon Sep 17 00:00:00 2001 From: Matthew Hilton Date: Mon, 12 Sep 2022 12:11:56 +1000 Subject: [PATCH 2/7] MDL-73317 session: Log session changes after close A snapshot of the session is now taken when write_close is called. The session at shutdown is then compared to the snapshot. If changes are detected, they are logged. This aids developers in seeing if early session closes may be having unintended consequences. --- lib/classes/session/manager.php | 48 +++++++++++++++++++++++++++++++++ 1 file changed, 48 insertions(+) diff --git a/lib/classes/session/manager.php b/lib/classes/session/manager.php index 901bd1f206f..99dd9d8aded 100644 --- a/lib/classes/session/manager.php +++ b/lib/classes/session/manager.php @@ -60,6 +60,9 @@ class manager { /** @var array Stores the the SESSION before a request is performed, used to check incorrect read-only modes */ private static $priorsession = []; + /** @var array Stores the the SESSION after write_close is called, used to check if it was mutated after the session is closed */ + private static $sessionatclose = []; + /** * @var bool Used to trigger the SESSION mutation warning without actually preventing SESSION mutation. * This variable is used to "copy" what the $requireslock parameter does in start_session(). @@ -686,6 +689,12 @@ class manager { global $PERF, $ME, $CFG; if (self::$sessionactive) { + // If debugging, take a snapshot of session at close and compare on shutdown to detect any accidental mutations. + if (debugging()) { + self::$sessionatclose = (array) $_SESSION['SESSION']; + \core_shutdown_manager::register_function('\core\session\manager::check_mutated_closed_session'); + } + // Grab the time when session lock is released. $PERF->sessionlock['released'] = microtime(true); if (!empty($PERF->sessionlock['gained'])) { @@ -735,6 +744,45 @@ class manager { self::$sessionactive = false; } + /** + * Checks if the session has been mutated since it was closed. + * In write_close the session is saved to the variable $sessionatclose + * If there is a difference between $sessionatclose and the current session, + * it means a script has erroneously closed the session too early. + * Script is usually called in shutdown_manager + */ + public static function check_mutated_closed_session() { + global $ME; + + // Session is still open, mutations are allowed. + if (self::$sessionactive) { + return; + } + + // Detect if session was cleared. + if (!isset($_SESSION['SESSION']) && isset(self::$sessionatclose)) { + debugging("Script $ME cleared the session after it was closed."); + return; + } else if (!isset($_SESSION['SESSION'])) { + // Else session is empty, nothing to check. + return; + } + + // Session is closed - compare the current session to the session when write_close was called. + $arraydiff = self::array_session_diff( + self::$sessionatclose, + (array) $_SESSION['SESSION'] + ); + + if ($arraydiff) { + $error = "Script $ME mutated the session after it was closed:"; + foreach ($arraydiff as $key => $value) { + $error .= ' $SESSION->' . $key; + } + debugging($error); + } + } + /** * Does the PHP session with given id exist? * From 4faa3204d643661013b63d2cab56726df4841286 Mon Sep 17 00:00:00 2001 From: Matthew Hilton Date: Thu, 15 Sep 2022 08:53:31 +1000 Subject: [PATCH 3/7] MDL-73317 session: Log extra details for cachestore changes More visibility depth is required for cachestore changes since they are usually multi dimensional arrays. --- lib/classes/session/manager.php | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/lib/classes/session/manager.php b/lib/classes/session/manager.php index 99dd9d8aded..ca1c3c41895 100644 --- a/lib/classes/session/manager.php +++ b/lib/classes/session/manager.php @@ -778,6 +778,11 @@ class manager { $error = "Script $ME mutated the session after it was closed:"; foreach ($arraydiff as $key => $value) { $error .= ' $SESSION->' . $key; + + // Extra debugging for cachestore session changes. + if (strpos($key, 'cachestore_') === 0 && is_array($value)) { + $error .= ': ' . implode(',', array_keys($value)); + } } debugging($error); } From 863bad8d7ebcdac112e84105e7d18e60b27da4e4 Mon Sep 17 00:00:00 2001 From: Matthew Hilton Date: Fri, 16 Sep 2022 10:35:14 +1000 Subject: [PATCH 4/7] MDL-73317 report_participation: Close session later in script The script has been reorganised so that it closes the session as early as possible, without causing session changes after close being logged. In this case, after the table has been output it is OK to close the session. --- report/participation/index.php | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/report/participation/index.php b/report/participation/index.php index f2b2f70878e..6683087ee89 100644 --- a/report/participation/index.php +++ b/report/participation/index.php @@ -86,8 +86,6 @@ echo $OUTPUT->header(); // Print the selector dropdown. $pluginname = get_string('pluginname', 'report_participation'); report_helper::print_report_selector($pluginname); -// Release session lock. -\core\session\manager::write_close(); // Logs will not have been recorded before the course timecreated time. $minlog = $course->timecreated; @@ -185,6 +183,9 @@ if (!empty($instanceid) && !empty($roleid)) { )); $table->setup(); + // Unlock the session only after outputting the table, since the table writes to the session. + \core\session\manager::write_close(); + // We want to query both the current context and parent contexts. list($relatedctxsql, $params) = $DB->get_in_or_equal($context->get_parent_context_ids(true), SQL_PARAMS_NAMED, 'relatedctx'); $params['roleid'] = $roleid; From 49c4cfb2d5faefbc4dd7b531f8552f15f7a4bc4f Mon Sep 17 00:00:00 2001 From: Matthew Hilton Date: Fri, 16 Sep 2022 10:37:34 +1000 Subject: [PATCH 5/7] MDL-73317 restore: Reset navcache before closing session As part of a restore, the session is closed early so it does not interrupt the users session during the restore. Currently the restore controller rebuilds the course caches while restoring. This inadvertently resets the navcache, which would edit the session despite it being closed. Because this tracker now adds logging for this behaviour, it means restoring now outputs a debugging message as a warning. To resolve the debugging message, the navcache is now reset just before closing the session. This is allowed, since the caches are designed to be volatile. --- backup/controller/restore_controller.class.php | 3 +++ 1 file changed, 3 insertions(+) diff --git a/backup/controller/restore_controller.class.php b/backup/controller/restore_controller.class.php index 0e60d08d53a..61032de826a 100644 --- a/backup/controller/restore_controller.class.php +++ b/backup/controller/restore_controller.class.php @@ -384,6 +384,9 @@ class restore_controller extends base_controller { // Release the session so other tabs in the same session are not blocked. if ($this->get_releasesession() === backup::RELEASESESSION_YES) { + // Preemptively reset the navcache before closing, so it remains the same on shutdown. + navigation_cache::destroy_volatile_caches(); + \core\session\manager::write_close(); } From f106babf5325e2172f34047527dca501b85fa6f9 Mon Sep 17 00:00:00 2001 From: Matthew Hilton Date: Fri, 16 Sep 2022 10:49:32 +1000 Subject: [PATCH 6/7] MDL-73317 search: Close session later in script The session write_close was moved to the earliest point in the script that does not modify the session. This is currently always after $OUTPUT->header() --- search/index.php | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/search/index.php b/search/index.php index 3908025920b..205f3b482f4 100644 --- a/search/index.php +++ b/search/index.php @@ -48,9 +48,6 @@ if (!empty($CFG->forcelogin)) { require_login(); } -// Unlock the session during a search. -\core\session\manager::write_close(); - require_capability('moodle/search:query', $context); $searchrenderer = $PAGE->get_renderer('core_search'); @@ -169,6 +166,9 @@ $PAGE->set_url($url); // We are ready to render. echo $OUTPUT->header(); +// Unlock the session only after outputting the header as this modifies the session cachestore. +\core\session\manager::write_close(); + // Get the results. if ($data) { $results = $search->paged_search($data, $page); From 5ea4d886ee0ba74442a8537a58bbbc9a62a8255d Mon Sep 17 00:00:00 2001 From: Matthew Hilton Date: Thu, 19 Jan 2023 11:33:53 +1000 Subject: [PATCH 7/7] MDL-73317 assign: Move useridlist cache construction Move the construction of the useridlist $SESSION cache to when a key is requested. This stops the writing of $SESSION when backing up or restoring mod_assign instances, which is neccessary since the backup and restore scripts close the session when processing. --- mod/assign/locallib.php | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/mod/assign/locallib.php b/mod/assign/locallib.php index 790b9b73a51..ab90fe9760d 100644 --- a/mod/assign/locallib.php +++ b/mod/assign/locallib.php @@ -208,8 +208,6 @@ class assign { * otherwise this class will load one from the context as required. */ public function __construct($coursemodulecontext, $coursemodule, $course) { - global $SESSION; - $this->context = $coursemodulecontext; $this->course = $course; @@ -224,10 +222,6 @@ class assign { // Extra entropy is required for uniqid() to work on cygwin. $this->useridlistid = clean_param(uniqid('', true), PARAM_ALPHANUM); - - if (!isset($SESSION->mod_assign_useridlist)) { - $SESSION->mod_assign_useridlist = []; - } } /** @@ -9345,6 +9339,13 @@ class assign { * @return string The key for the id, or new entry if no $id is passed. */ public function get_useridlist_key($id = null) { + global $SESSION; + + // Ensure the user id list cache is initialised. + if (!isset($SESSION->mod_assign_useridlist)) { + $SESSION->mod_assign_useridlist = []; + } + if ($id === null) { $id = $this->get_useridlist_key_id(); }