From 278c4b9953b069091eb24c45c4f2062f7ed1ce39 Mon Sep 17 00:00:00 2001 From: nexxo Date: Tue, 28 Jul 2026 15:27:06 +0200 Subject: [PATCH] Match the logout exemption through AdminArea, not a bare route check MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The setup-route exemption already went through AdminArea::routeIs(), which matches both admin. and admin.via*.. The logout exemption next to it used a bare $request->routeIs('admin.logout'), which matches only the canonical name — on a recovery hostname the logout route is admin.via0.logout, so the check failed there and an unenrolled operator's logout POST bounced back to enrolment instead of being let through, on exactly the host that exists because the canonical one is not working. --- .../Middleware/RequireOperatorTwoFactor.php | 9 ++- .../Admin/ConsoleHostSeparationTest.php | 55 ++++++++++++++++++- 2 files changed, 60 insertions(+), 4 deletions(-) diff --git a/app/Http/Middleware/RequireOperatorTwoFactor.php b/app/Http/Middleware/RequireOperatorTwoFactor.php index 8ece44d..bec7898 100644 --- a/app/Http/Middleware/RequireOperatorTwoFactor.php +++ b/app/Http/Middleware/RequireOperatorTwoFactor.php @@ -40,8 +40,13 @@ class RequireOperatorTwoFactor } // The two pages that must stay reachable: where two-factor is - // enrolled, and signing out. - if (AdminArea::routeIs('admin.two-factor-setup') || $request->routeIs('admin.logout')) { + // enrolled, and signing out. Both through AdminArea::routeIs(), not + // a bare $request->routeIs() — a recovery hostname's routes are + // named admin.viaN.*, and a bare check matches only the canonical + // name, which would leave an unenrolled operator unable to sign out + // on exactly the host that exists because the canonical one is not + // working. + if (AdminArea::routeIs('admin.two-factor-setup') || AdminArea::routeIs('admin.logout')) { return $next($request); } diff --git a/tests/Feature/Admin/ConsoleHostSeparationTest.php b/tests/Feature/Admin/ConsoleHostSeparationTest.php index f26dd39..9518ec3 100644 --- a/tests/Feature/Admin/ConsoleHostSeparationTest.php +++ b/tests/Feature/Admin/ConsoleHostSeparationTest.php @@ -1,9 +1,15 @@ set('admin_access.hosts', ['admin.example.test', '127.0.0.1', 'localhost']); config()->set('admin_access.exclusive', true); - $router = new Illuminate\Routing\Router(app('events'), app()); + $router = new Router(app('events'), app()); Illuminate\Support\Facades\Route::swap($router); require base_path('routes/web.php'); $router->getRoutes()->refreshNameLookups(); @@ -174,7 +180,7 @@ it('keeps the navigation marked on a recovery hostname', function () { // serialise them — which silently breaks every exact name check, and the // navigation loses its active state exactly when something is already wrong. $named = fn (string $name) => tap(request(), fn ($r) => $r->setRouteResolver( - fn () => (new Illuminate\Routing\Route(['GET'], '/', fn () => null))->name($name), + fn () => (new Route(['GET'], '/', fn () => null))->name($name), )); $named('admin.overview'); @@ -184,3 +190,48 @@ it('keeps the navigation marked on a recovery hostname', function () { expect(AdminArea::routeIs('admin.overview'))->toBeTrue() ->and(AdminArea::routeIs('admin.settings'))->toBeFalse(); }); + +it('lets an unenrolled operator sign out on a recovery hostname, not just the canonical one', function () { + // Whole-branch review, P2: RequireOperatorTwoFactor's exemption used + // AdminArea::routeIs() for the setup route but bare + // $request->routeIs('admin.logout') for logout — which matches only the + // canonical name. On a recovery host the logout route is named + // admin.via0.logout (same reason the navigation test above exists), so + // the bare check failed there and the middleware bounced an unenrolled + // operator's logout POST back to enrolment — trapping them on exactly + // the host that exists because the canonical one is not working. A + // canonical-host test would pass whichever form the check takes, so this + // one is deliberately on the alternate. + config()->set('admin_access.hosts', ['admin.example.test', 'admin-recovery.example.test']); + config()->set('admin_access.exclusive', true); + Settings::set('console.require_2fa', true); + + // Register routes for real from the config just set, rather than + // asserting the via0 name by fiat — same technique as "it can cache its + // routes…" above. Proves the whole path: this host, this URL, genuinely + // resolves to admin.via0.logout before the middleware is ever involved. + $router = new Router(app('events'), app()); + Illuminate\Support\Facades\Route::swap($router); + require base_path('routes/web.php'); + $router->getRoutes()->refreshNameLookups(); + + $matched = $router->getRoutes()->match( + Request::create('http://admin-recovery.example.test/logout', 'POST'), + ); + expect($matched->getName())->toBe('admin.via0.logout'); + + $operator = Operator::factory()->role('Owner')->create(); + Auth::guard('operator')->login($operator); + + // Drive the middleware directly, on a request resolved to that genuinely + // registered route — RequireOperatorTwoFactor reads the route's name, not + // which router produced it, so this is the real check without needing a + // full dispatch (session/CSRF middleware) around it. + $request = Request::create('http://admin-recovery.example.test/logout', 'POST'); + $request->setRouteResolver(fn () => $matched); + app()->instance('request', $request); + + $response = (new RequireOperatorTwoFactor)->handle($request, fn ($req) => response('through')); + + expect($response->getContent())->toBe('through'); +});