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 +}