Merge pull request #375 from catalyst/redirect_exception

Properly check source auth page when detecting redir loops
catalyst · Mar 16, 2023 · 155db3d · 155db3d
2 parents c511822 + e058e0c
commit 155db3d
Show file tree

Hide file tree

Showing 4 changed files with 55 additions and 13 deletions.
diff --git a/auth.php b/auth.php
@@ -58,6 +58,9 @@
 // Perform state check.
 \tool_mfa\manager::resolve_mfa_status();
 
+// We have a valid landing here, before doing any actions, clear any redir loop progress.
+\tool_mfa\manager::clear_redirect_counter();
+
 $factor = \tool_mfa\plugininfo\factor::get_next_user_factor();
 // If ok, perform form actions for input factor.
 $form = new login_form($currenturl, ['factor' => $factor]);

diff --git a/classes/manager.php b/classes/manager.php
@@ -35,6 +35,9 @@ class manager {
     /** @var int */
     const REDIRECT_EXCEPTION = -1;
 
+    /** @var int */
+    const REDIR_LOOP_THRESHOLD = 5;
+
     /**
      * Displays a debug table with current factor information.
      */
@@ -402,7 +405,7 @@ private static function update_pass_time() {
     /**
      * Checks whether the user should be redirected from the provided url.
      *
-     * @param string $url
+     * @param \moodle_url $url
      * @param bool $preventredirect
      * @return int
      */
@@ -418,12 +421,6 @@ public static function should_require_mfa($url, $preventredirect) {
         // Remove all params before comparison.
         $url->remove_all_params();
 
-        // Dont redirect if already on auth.php.
-        $authurl = new \moodle_url('/admin/tool/mfa/auth.php');
-        if ($url->compare($authurl, URL_MATCH_BASE)) {
-            return self::NO_REDIRECT;
-        }
-
         // Checks for upgrades pending.
         if (is_siteadmin()) {
             // We should only allow an upgrade from the frontend to complete.
@@ -440,8 +437,8 @@ public static function should_require_mfa($url, $preventredirect) {
         }
 
         // Dont redirect logo images from pluginfile.php (for example: logo in header).
-        $authurl = new \moodle_url('/pluginfile.php/1/core_admin/logocompact/');
-        if ($url->compare($authurl)) {
+        $logourl = new \moodle_url('/pluginfile.php/1/core_admin/logocompact/');
+        if ($url->compare($logourl)) {
             return self::NO_REDIRECT;
         }
 
@@ -517,16 +514,18 @@ public static function should_require_mfa($url, $preventredirect) {
         }
 
         // Circular checks.
+        $authurl = new \moodle_url('/admin/tool/mfa/auth.php');
+        $authlocal = $authurl->out_as_local_url();
         if (isset($SESSION->mfa_redir_referer)
-            && $SESSION->mfa_redir_referer != $authurl) {
+            && $SESSION->mfa_redir_referer != $authlocal) {
             if ($SESSION->mfa_redir_referer == get_local_referer(true)) {
                 // Possible redirect loop.
                 if (!isset($SESSION->mfa_redir_count)) {
                     $SESSION->mfa_redir_count = 1;
                 } else {
                     $SESSION->mfa_redir_count++;
                 }
-                if ($SESSION->mfa_redir_count > 5) {
+                if ($SESSION->mfa_redir_count > self::REDIR_LOOP_THRESHOLD) {
                     return self::REDIRECT_EXCEPTION;
                 }
             } else {
@@ -537,9 +536,25 @@ public static function should_require_mfa($url, $preventredirect) {
         // Set referer after checks.
         $SESSION->mfa_redir_referer = get_local_referer(true);
 
+        // Don't redirect if already on auth.php.
+        if ($url->compare($authurl, URL_MATCH_BASE)) {
+            return self::NO_REDIRECT;
+        }
+
         return self::REDIRECT;
     }
 
+    /**
+     * Clears the redirect counter for infinite redirect loops. Called from auth.php when a valid load is resolved.
+     *
+     */
+    public static function clear_redirect_counter() {
+        global $SESSION;
+
+        unset($SESSION->mfa_redir_referer);
+        unset($SESSION->mfa_redir_count);
+    }
+
     /**
      * Gets all defined factor urls that should not redirect.
      *

diff --git a/tests/manager_test.php b/tests/manager_test.php
@@ -373,4 +373,28 @@ public function test_core_hooks() {
         $this->assertTrue($CFG->mfa_config_hook_test);
         $this->assertTrue($SESSION->mfa_login_hook_test);
     }
+
+    public function test_circular_redirect_auth() {
+        // Setup test and user.
+        $this->resetAfterTest(true);
+        $user = $this->getDataGenerator()->create_user();
+        $this->setUser($user);
+
+        // Spoof the referrer for the redirect check.
+        $_SERVER['HTTP_REFERER'] = '/admin/tool/mfa/auth.php';
+        $baseurl = new \moodle_url('/my/naughty/page.php');
+
+        // After a single check, we should redirect.
+        $this->assertEquals(\tool_mfa\manager::REDIRECT,
+            \tool_mfa\manager::should_require_mfa($baseurl, false));
+
+        // Now hammer it up to the threshold to emulate a repeated force browse from auth.php
+        for ($i = 0; $i < \tool_mfa\manager::REDIR_LOOP_THRESHOLD; $i++) {
+            \tool_mfa\manager::should_require_mfa($baseurl, false);
+        }
+
+        // Now finally confirm that a 6th access attempt (after loop safety trigger) still redirects.
+        $this->assertEquals(\tool_mfa\manager::REDIRECT,
+            \tool_mfa\manager::should_require_mfa($baseurl, false));
+    }
 }
diff --git a/version.php b/version.php
@@ -25,8 +25,8 @@
 
 defined('MOODLE_INTERNAL') || die();
 
-$plugin->version   = 2022120200;      // The current plugin version (Date: YYYYMMDDXX).
-$plugin->release   = 2022120200;      // Same as version.
+$plugin->version   = 2023031600;      // The current plugin version (Date: YYYYMMDDXX).
+$plugin->release   = 2023031600;      // Same as version.
 $plugin->requires  = 2017051500.00;   // Support back to 3.3 - Totara 12. Patches required.
 $plugin->component = 'tool_mfa';
 $plugin->maturity  = MATURITY_STABLE;