MDL-77846 core: Make endpoint revision number checks stricter

In some places we prevented cache poisoning, in others we did not. We
also did not place any restriction on the minimum value for a revision.

This change introduces a new set of functions for configonly endpoints
which validates the revision numbers passed in. If the revision is
either too old, or too new, it is rejected and the file content is not
cached. The content is still served, but caching headers are not sent,
and any local storage caching is prevented.

The current time is used as the maximum version, with 60 seconds added
to allow for any clock skew between cluster nodes. Previously some
locations used one hour, but there should never be such a large clock
skew on a correctly configured system.

Co-authored-by: Andrew Nicols <[email protected]>

Author: 2 people

lib/configonlylib.php

@@ -205,3 +205,57 @@ function min_get_slash_argument($clean = true) {
     }
     return $relativepath;
 }
+
+/**
+ * Get the lowest possible currently valid revision number.
+ *
+ * This is based on the current Moodle version.
+ *
+ * @return int Unix timestamp
+ */
+function min_get_minimum_revision(): int {
+    static $timestamp = null;
+
+    if ($timestamp === null) {
+        global $CFG;
+        require("{$CFG->dirroot}/version.php");
+        // Get YYYYMMDD from version.
+        $datestring = floor($version / 100);
+        // Parse the date components.
+        $year = intval(substr($datestring, 0, 4));
+        $month = intval(substr($datestring, 4, 2));
+        $day = intval(substr($datestring, 6, 2));
+        // Return converted GMT Unix timestamp.
+        $timestamp = gmmktime(0, 0, 0, $month, $day, $year);
+    }
+
+    return $timestamp;
+}
+
+/**
+ * Get the highest possible currently valid revision number.
+ *
+ * This is based on the current time, allowing for a small amount of clock skew between servers.
+ *
+ * Future values beyond the clock skew are not allowed to avoid the possibility of cache poisoning.
+ *
+ * @return int
+ */
+function min_get_maximum_revision(): int {
+    return time() + 60;
+}
+
+/**
+ * Helper function to determine if the given revision number is valid.
+ *
+ * @param int $revision A numeric revision to check for validity
+ * @return bool Whether the revision is valid
+ */
+function min_is_revision_valid_and_current(int $revision): bool {
+    // Invalid revision.
+    if ($revision <= 0) {
+        return false;
+    }
+    // Check revision is within range.
+    return $revision >= min_get_minimum_revision() && $revision <= min_get_maximum_revision();
+}

lib/editor/tiny/lang.php

@@ -95,7 +95,8 @@ protected function parse_file_information_from_url(): void {
      */
     protected function serve_file(): void {
         // Attempt to send the cached langpack.
-        if ($this->rev > 0) {
+        // We only cache the file if the rev is valid.
+        if (min_is_revision_valid_and_current($this->rev)) {
             if ($this->is_candidate_file_available()) {
                 // The send_cached_file_if_available function will exit if successful.
                 // In theory the file could become unavailable after checking that the file exists.

lib/editor/tiny/loader.php

@@ -114,7 +114,8 @@ protected function parse_file_information_from_url(): void {
      */
     public function serve_file(): void {
         // Attempt to send the cached filepathpack.
-        if ($this->rev > 0) {
+        // We only cache the file if the rev is valid.
+        if (min_is_revision_valid_and_current($this->rev)) {
             if ($this->is_candidate_file_available()) {
                 // The send_cached_file_if_available function will exit if successful.
                 // In theory the file could become unavailable after checking that the file exists.

lib/javascript.php

@@ -47,6 +47,11 @@
     $file = min_optional_param('jsfile', '', 'RAW'); // 'file' would collide with URL rewriting!
 }
 
+if (!min_is_revision_valid_and_current($rev)) {
+    // If the rev is invalid, normalise it to -1 to disable all caching.
+    $rev = -1;
+}
+
 // some security first - pick only files with .js extension in dirroot
 $jsfiles = array();
 $files = explode(',', $file);
@@ -79,8 +84,7 @@
 
 $etag = sha1($rev.implode(',', $jsfiles));
 
-// Use the caching only for meaningful revision numbers which prevents future cache poisoning.
-if ($rev > 0 and $rev < (time() + 60*60)) {
+if ($rev > 0) {
     $candidate = $CFG->localcachedir.'/js/'.$etag;
 
     if (file_exists($candidate)) {

lib/tests/configonlylib_test.php

@@ -21,7 +21,6 @@
 // Global $CFG not used here intentionally to make sure it is not required inside the lib.
 require_once(__DIR__ . '/../configonlylib.php');
 
-
 /**
  * Unit tests for config only library functions.
  *
@@ -144,4 +143,76 @@ public function test_min_get_slash_argument() {
         // Windows dir separators are removed, multiple ... gets collapsed to one .
         $this->assertSame('/standardcore/./5/u/f1', min_get_slash_argument());
     }
+
+    /**
+     * Test the min_get_minimum_version function.
+     *
+     * @covers ::min_get_minimum_version
+     */
+    public function test_min_get_minimum_version(): void {
+        // This is fairly hard to write a test for, but we can at least check that it returns a number
+        // greater than the version when the feature was first introduced.
+        $firstintroduced = 1693612800; // Equivalent to 20230902 00:00:00 GMT.
+        $this->assertGreaterThanOrEqual($firstintroduced, min_get_minimum_revision());
+    }
+
+    /**
+     * Test the min_get_maximum_version function.
+     *
+     * @covers ::min_get_maximum_version
+     */
+    public function test_min_get_maximum_version(): void {
+        // The maximum version should be set to a time in the near future.
+        // This is currently defined as "in the next minute".
+        // Note: We use a 65 second window to allow for slow test runners.
+        $this->assertGreaterThan(time(), min_get_maximum_revision());
+        $this->assertLessThanOrEqual(time() + 65, min_get_maximum_revision());
+    }
+
+    /**
+     * Test the min_is_revision_valid_and_current function.
+     *
+     * @covers ::min_is_revision_valid_and_current
+     * @dataProvider min_is_revision_valid_and_current_provider
+     */
+    public function test_min_is_revision_valid_and_current(int $revision, bool $expected): void {
+        $this->assertEquals($expected, min_is_revision_valid_and_current($revision));
+    }
+
+    /**
+     * Data provider for the min_is_revision_valid_and_current tests.
+     *
+     * @return array
+     */
+    public function min_is_revision_valid_and_current_provider(): array {
+        return [
+            'Negative value' => [-1, false],
+            'Empty value' => [0, false],
+            'A time before the minimum accepted value' => [min_get_minimum_revision() - 1, false],
+            'The minimum accepted value' => [min_get_minimum_revision(), true],
+            'The current time' => [time(), true],
+            // Note: We have to be careful using time values because the data provider is run at the start of the phpunit run,
+            // but the test may not be run for some time.
+            // On a slower machine and/or database, this could be several hours.
+            // For a more specific time we must have a specific test function.
+            'A time in the future' => [time() + DAYSECS, false]
+        ];
+    }
+
+
+    /**
+     * Test the min_is_revision_valid_and_current function with close times.
+     *
+     * Note: These tests are incompatible with data providers.
+     *
+     * @covers ::min_is_revision_valid_and_current
+     */
+    public function test_min_is_revision_valid_and_current_close_proximity(): void {
+        // A time in the near future.
+        $this->assertTrue(min_is_revision_valid_and_current(time() + 55));
+
+        // A time in the too-far future.
+        $this->assertFalse(min_is_revision_valid_and_current(time() + 70));
+
+    }
 }

theme/font.php

@@ -49,6 +49,11 @@
     $font      = min_optional_param('font', '', 'RAW');
 }
 
+if (!min_is_revision_valid_and_current($rev)) {
+    // If the rev is invalid, normalise it to -1 to disable all caching.
+    $rev = -1;
+}
+
 if (!$font) {
     font_not_found();
 }

theme/image.php

@@ -58,6 +58,11 @@
     $usesvg    = (bool)min_optional_param('svg', '1', 'INT');
 }
 
+if (!min_is_revision_valid_and_current($rev)) {
+    // If the rev is invalid, normalise it to -1 to disable all caching.
+    $rev = -1;
+}
+
 if (empty($component) or $component === 'moodle' or $component === 'core') {
     $component = 'core';
 }

theme/javascript.php

@@ -50,6 +50,11 @@
     $type      = min_optional_param('type', 'head', 'RAW');
 }
 
+if (!min_is_revision_valid_and_current($rev)) {
+    // If the rev is invalid, normalise it to -1 to disable all caching.
+    $rev = -1;
+}
+
 if ($type !== 'head' and $type !== 'footer') {
     header('HTTP/1.0 404 not found');
     die('Theme was not found, sorry.');

theme/styles.php

@@ -66,6 +66,13 @@
     $themesubrev = min_clean_param($themesubrev, 'INT');
 }
 
+// Note: We only check validity of the revision number here, we do not check the theme sub-revision because this is
+// not solely based on time.
+if (!min_is_revision_valid_and_current($rev)) {
+    // If the rev is invalid, normalise it to -1 to disable all caching.
+    $rev = -1;
+}
+
 // Check that type fits into the expected values.
 if (!in_array($type, ['all', 'all-rtl', 'editor', 'editor-rtl'])) {
     css_send_css_not_found();

theme/yui_combo.php

@@ -231,8 +231,9 @@
             continue;
         }
         $revision = (int)array_shift($bits);
-        if ($revision === -1) {
-            // Revision -1 says please don't cache the JS
+        if (!min_is_revision_valid_and_current($revision)) {
+            // A non-current revision means please don't cache the JS
+            $revision = -1;
             $cache = false;
         }
         $frankenstyle = array_shift($bits);
@@ -278,8 +279,9 @@
             continue;
         }
         $revision = (int)array_shift($bits);
-        if ($revision === -1) {
-            // Revision -1 says please don't cache the JS
+        if (!min_is_revision_valid_and_current($revision)) {
+            // A non-current revision means please don't cache the JS
+            $revision = -1;
             $cache = false;
         }
         $contentfile = "$CFG->libdir/yuilib/gallery/" . join('/', $bits);