From 769ef8fc77327ffeff94605878aa1b5140fbc963 Mon Sep 17 00:00:00 2001
From: Lucas Bartholemy <lucas@bartholemy.com>
Date: Tue, 14 Jul 2026 14:19:50 +0200
Subject: [PATCH] Fix infinite redirect loop when another module intercepts the
 current action
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The handler now cancels the intercepted action via $event->isValid
(returning false from an event handler has no effect) and yields when
another interceptor already canceled or redirected the request, so two
intercepting modules no longer overwrite each other's redirects
(humhub-internal#1261).

Unlike the approach in #117, the exemptions use a twofa-own,
security-reviewed list (check page, login/logout, push token update)
instead of honoring the generic $doNotInterceptActionIds flag — that
flag is set by controllers for unrelated reasons (REST module, live
polling, account deletion) and honoring it would exempt those actions
from the second factor: account deletion only requires the current
password, which is exactly what an attacker in the 2FA threat model
already has.

Supersedes #117.
---
 Events.php        | 51 ++++++++++++++++++++++++++++++++++++++---------
 docs/CHANGELOG.md |  4 ++++
 module.json       |  4 ++--
 3 files changed, 48 insertions(+), 11 deletions(-)

diff --git a/Events.php b/Events.php
index a83c2d4..b321983 100644
--- a/Events.php
+++ b/Events.php
@@ -12,6 +12,7 @@
 use humhub\modules\admin\controllers\UserController as AdminUserController;
 use humhub\modules\admin\grid\UserActionColumn;
 use humhub\modules\admin\permissions\ManageUsers;
+use humhub\modules\twofa\controllers\CheckController;
 use humhub\modules\twofa\events\BeforeCheck;
 use humhub\modules\twofa\helpers\TwofaHelper;
 use humhub\modules\twofa\helpers\TwofaUrl;
@@ -58,7 +59,7 @@ public static function registerAutoloader()
     public static function onBeforeAction($event)
     {
         if (Yii::$app->user->mustChangePassword()) {
-            return false;
+            return;
         }
 
         /** @var Controller $controller */
@@ -68,22 +69,54 @@ public static function onBeforeAction($event)
             Yii::$app->session->set('twofa.switchedUserId', Yii::$app->user->id);
         }
 
-        if (
-            $controller->module->id === 'fcm-push'
-            && $controller->id === 'token'
-            && $controller->action->id === 'update'
-        ) {
-            return false;
+        // Another event handler (e.g. from a module intercepting the same action) has
+        // already canceled or redirected the current action; overriding its redirect
+        // could produce a redirect loop between the two modules
+        if (!$event->isValid || Yii::$app->response->getIsRedirection()) {
+            return;
+        }
+
+        // Twofa-own allowlist — deliberately NOT the generic $doNotInterceptActionIds
+        // flag: that flag is set by controllers for unrelated reasons (e.g. the REST
+        // module, live polling, account deletion) and honoring it would exempt those
+        // actions from the second factor. Every entry here is a security decision.
+        if (self::isTwofaExemptRoute($controller, $event)) {
+            return;
         }
 
         $beforeVerifying = new BeforeCheck();
         Yii::$app->trigger($beforeVerifying->name, $beforeVerifying);
 
-        if (!$beforeVerifying->handled && TwofaHelper::isVerifyingRequired() && !Yii::$app->getModule('twofa')->isTwofaCheckUrl()) {
-            return Yii::$app->response->redirect(TwofaUrl::toCheck());
+        if (!$beforeVerifying->handled && TwofaHelper::isVerifyingRequired()) {
+            $event->isValid = false;
+            Yii::$app->response->redirect(TwofaUrl::toCheck());
         }
     }
 
+    /**
+     * Routes that stay reachable while the two-factor verification is pending.
+     *
+     * @param $controller Controller
+     * @return bool
+     */
+    protected static function isTwofaExemptRoute($controller, $event): bool
+    {
+        // The 2fa check page itself — redirecting it would loop onto itself
+        if ($controller instanceof CheckController) {
+            return true;
+        }
+
+        // Login and logout must stay reachable
+        if ($controller instanceof AuthController) {
+            return true;
+        }
+
+        // The mobile app updates its push token in the background
+        return $controller->module->id === 'fcm-push'
+            && $controller->id === 'token'
+            && $event->action->id === 'update';
+    }
+
     /**
      * Check if currently action "Impersonate" is called
      *
diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md
index cf45f3e..db638c2 100644
--- a/docs/CHANGELOG.md
+++ b/docs/CHANGELOG.md
@@ -1,6 +1,10 @@
 Changelog
 =========
 
+1.2.3 (Unreleased)
+------------------
+- Fix: Infinite redirect loop to the 2FA check page when another module intercepts the current action — the handler now yields when another interceptor already redirected the request, cancels the action via `$event->isValid` and uses a twofa-own, security-reviewed exemption list (check page, login/logout, push token update) instead of a generic opt-out flag
+
 1.2.2 (June 18, 2026)
 ---------------------
 - Enh: Automated code refactoring for HumHub 1.18.0-beta.6 using Rector
diff --git a/module.json b/module.json
index 7058c08..865a3b7 100644
--- a/module.json
+++ b/module.json
@@ -14,9 +14,9 @@
         "resources/screenshot3.png",
         "resources/screenshot4.png"
     ],
-    "version": "1.2.2",
+    "version": "1.2.3",
     "humhub": {
         "minVersion": "1.18.0-beta.6",
         "maxVersion": "1.18"
     }
-}
\ No newline at end of file
+}