From dffc4db9a2e3cd9c1b86bcf27a0750d4530e1186 Mon Sep 17 00:00:00 2001 From: fs Date: Sun, 26 Apr 2026 10:20:28 +0200 Subject: [PATCH] fix(routing): prevent login redirect loops on alias targets --- core/App/Module/ModuleRegistry.php | 1 + core/Http/AccessControl.php | 4 +- core/Http/RouteCatalog.php | 1 + .../User/UserAccessTemplateService.php | 4 +- tests/App/Module/ModuleRegistryTest.php | 15 +++ .../RouteAliasQuerySafetyContractTest.php | 126 ++++++++++++++++++ tests/Http/RouteCatalogTest.php | 2 +- 7 files changed, 149 insertions(+), 4 deletions(-) create mode 100644 tests/Architecture/RouteAliasQuerySafetyContractTest.php diff --git a/core/App/Module/ModuleRegistry.php b/core/App/Module/ModuleRegistry.php index 7fe5f19..c673636 100644 --- a/core/App/Module/ModuleRegistry.php +++ b/core/App/Module/ModuleRegistry.php @@ -251,6 +251,7 @@ final class ModuleRegistry $this->mergedRoutes[] = $route; if (!empty($route['public'])) { $this->mergedPublicPaths[] = $path; + $this->mergedPublicPaths[] = $target; } } diff --git a/core/Http/AccessControl.php b/core/Http/AccessControl.php index 90ada6a..40c73a3 100644 --- a/core/Http/AccessControl.php +++ b/core/Http/AccessControl.php @@ -82,7 +82,9 @@ class AccessControl } $locale = I18n::$locale ?? I18n::$defaultLocale; - $loginPath = Request::withLocale('login', $locale); + // Use the concrete target route. Minty alias rewrites compare the full + // REQUEST_URI (including query), so alias paths with query params are fragile. + $loginPath = Request::withLocale('auth/login', $locale); $requestUri = $_SERVER['REQUEST_URI'] ?? ''; $loginTarget = $this->intendedUrlService->buildLoginRedirect($loginPath, $requestUri); diff --git a/core/Http/RouteCatalog.php b/core/Http/RouteCatalog.php index ee2f5d0..35d7359 100644 --- a/core/Http/RouteCatalog.php +++ b/core/Http/RouteCatalog.php @@ -73,6 +73,7 @@ final class RouteCatalog if ($isPublic) { $publicPaths[] = $path; + $publicPaths[] = $target; } } diff --git a/core/Service/User/UserAccessTemplateService.php b/core/Service/User/UserAccessTemplateService.php index c479588..6230816 100644 --- a/core/Service/User/UserAccessTemplateService.php +++ b/core/Service/User/UserAccessTemplateService.php @@ -97,10 +97,10 @@ class UserAccessTemplateService } $tenantSlug = $this->tenantSsoService->tenantLoginSlug($tenant); if ($tenantSlug !== '') { - return appUrl(Request::withLocale('login?tenant=' . rawurlencode($tenantSlug), $locale)); + return appUrl(Request::withLocale('auth/login?tenant=' . rawurlencode($tenantSlug), $locale)); } } - return appUrl(Request::withLocale('login', $locale)); + return appUrl(Request::withLocale('auth/login', $locale)); } } diff --git a/tests/App/Module/ModuleRegistryTest.php b/tests/App/Module/ModuleRegistryTest.php index afa01bd..5170953 100644 --- a/tests/App/Module/ModuleRegistryTest.php +++ b/tests/App/Module/ModuleRegistryTest.php @@ -138,6 +138,21 @@ final class ModuleRegistryTest extends TestCase ModuleRegistry::boot($this->fixturesDir, ['mod-a', 'mod-b']); } + public function testPublicRouteAddsPathAndTargetToPublicPaths(): void + { + $this->createModuleManifest('mod-public', [ + 'id' => 'mod-public', + 'routes' => [ + ['path' => 'public/entry', 'target' => 'public/internal', 'public' => true], + ], + ]); + + $registry = ModuleRegistry::boot($this->fixturesDir, ['mod-public']); + + self::assertContains('public/entry', $registry->getPublicPaths()); + self::assertContains('public/internal', $registry->getPublicPaths()); + } + public function testUiSlotConflictThrows(): void { $panelTemplateA = $this->createFixtureFile('mod-a-panel.phtml', ''); diff --git a/tests/Architecture/RouteAliasQuerySafetyContractTest.php b/tests/Architecture/RouteAliasQuerySafetyContractTest.php new file mode 100644 index 0000000..dce8bc3 --- /dev/null +++ b/tests/Architecture/RouteAliasQuerySafetyContractTest.php @@ -0,0 +1,126 @@ +coreAliasPathsWithDifferentTargets(); + $this->assertNotSame([], $aliases, 'Expected at least one core route alias with path != target.'); + + $scanTargets = ['core', 'pages', 'templates', 'modules']; + $extensions = ['php', 'phtml']; + + $violations = []; + foreach ($this->runtimeSourceFiles($scanTargets, $extensions) as $relativePath) { + $content = $this->readProjectFile($relativePath); + foreach ($aliases as $aliasPath) { + $quotedAlias = preg_quote($aliasPath, '#'); + if (preg_match("#(?:lurl|Request::withLocale|Router::redirect)\\(\\s*['\"]{$quotedAlias}\\?#", $content) === 1) { + $violations[] = $relativePath . " uses alias '{$aliasPath}' with query string."; + } + } + } + + sort($violations); + $this->assertSame([], $violations, "Alias+query routing anti-pattern found:\n" . implode("\n", $violations)); + } + + public function testAccessControlUsesConcreteLoginTargetPathForReturnToRedirect(): void + { + $content = $this->readProjectFile('core/Http/AccessControl.php'); + + $this->assertStringContainsString( + "Request::withLocale('auth/login', \$locale)", + $content + ); + $this->assertStringNotContainsString( + "Request::withLocale('login', \$locale)", + $content + ); + } + + /** + * @return list + */ + private function coreAliasPathsWithDifferentTargets(): array + { + $routesFile = $this->projectRootPath() . '/config/routes.php'; + $this->assertFileExists($routesFile, 'Missing config/routes.php'); + + $routes = require $routesFile; + $this->assertIsArray($routes, 'config/routes.php must return an array'); + + $aliases = []; + foreach ($routes as $entry) { + if (!is_array($entry)) { + continue; + } + + $path = trim((string) ($entry['path'] ?? '')); + $target = trim((string) ($entry['target'] ?? '')); + if ($path === '' || $target === '' || $path === $target) { + continue; + } + + $aliases[] = $path; + } + + $aliases = array_values(array_unique($aliases)); + sort($aliases); + + return $aliases; + } + + /** + * @param list $scanTargets + * @param list $extensions + * @return list + */ + private function runtimeSourceFiles(array $scanTargets, array $extensions): array + { + $root = $this->projectRootPath(); + $files = []; + + foreach ($scanTargets as $target) { + $fullPath = $root . '/' . $target; + if (!is_dir($fullPath)) { + continue; + } + + $iterator = new \RecursiveIteratorIterator( + new \RecursiveDirectoryIterator($fullPath, \FilesystemIterator::SKIP_DOTS) + ); + + foreach ($iterator as $file) { + if (!$file->isFile()) { + continue; + } + + $extension = strtolower((string) $file->getExtension()); + if (!in_array($extension, $extensions, true)) { + continue; + } + + $files[] = str_replace($root . '/', '', $file->getPathname()); + } + } + + $files = array_values(array_unique($files)); + sort($files); + + return $files; + } +} diff --git a/tests/Http/RouteCatalogTest.php b/tests/Http/RouteCatalogTest.php index 064f9a3..563ddea 100644 --- a/tests/Http/RouteCatalogTest.php +++ b/tests/Http/RouteCatalogTest.php @@ -40,7 +40,7 @@ final class RouteCatalogTest extends TestCase ], $catalog->coreRoutes() ); - self::assertSame(['login'], $catalog->corePublicPaths()); + self::assertSame(['auth/login', 'login'], $catalog->corePublicPaths()); } public function testThrowsWhenRouteConfigDoesNotReturnArray(): void