fix(routing): prevent login redirect loops on alias targets
This commit is contained in:
@@ -251,6 +251,7 @@ final class ModuleRegistry
|
|||||||
$this->mergedRoutes[] = $route;
|
$this->mergedRoutes[] = $route;
|
||||||
if (!empty($route['public'])) {
|
if (!empty($route['public'])) {
|
||||||
$this->mergedPublicPaths[] = $path;
|
$this->mergedPublicPaths[] = $path;
|
||||||
|
$this->mergedPublicPaths[] = $target;
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -82,7 +82,9 @@ class AccessControl
|
|||||||
}
|
}
|
||||||
|
|
||||||
$locale = I18n::$locale ?? I18n::$defaultLocale;
|
$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'] ?? '';
|
$requestUri = $_SERVER['REQUEST_URI'] ?? '';
|
||||||
$loginTarget = $this->intendedUrlService->buildLoginRedirect($loginPath, $requestUri);
|
$loginTarget = $this->intendedUrlService->buildLoginRedirect($loginPath, $requestUri);
|
||||||
|
|||||||
@@ -73,6 +73,7 @@ final class RouteCatalog
|
|||||||
|
|
||||||
if ($isPublic) {
|
if ($isPublic) {
|
||||||
$publicPaths[] = $path;
|
$publicPaths[] = $path;
|
||||||
|
$publicPaths[] = $target;
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -97,10 +97,10 @@ class UserAccessTemplateService
|
|||||||
}
|
}
|
||||||
$tenantSlug = $this->tenantSsoService->tenantLoginSlug($tenant);
|
$tenantSlug = $this->tenantSsoService->tenantLoginSlug($tenant);
|
||||||
if ($tenantSlug !== '') {
|
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));
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -138,6 +138,21 @@ final class ModuleRegistryTest extends TestCase
|
|||||||
ModuleRegistry::boot($this->fixturesDir, ['mod-a', 'mod-b']);
|
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
|
public function testUiSlotConflictThrows(): void
|
||||||
{
|
{
|
||||||
$panelTemplateA = $this->createFixtureFile('mod-a-panel.phtml', '<ul></ul>');
|
$panelTemplateA = $this->createFixtureFile('mod-a-panel.phtml', '<ul></ul>');
|
||||||
|
|||||||
126
tests/Architecture/RouteAliasQuerySafetyContractTest.php
Normal file
126
tests/Architecture/RouteAliasQuerySafetyContractTest.php
Normal file
@@ -0,0 +1,126 @@
|
|||||||
|
<?php
|
||||||
|
|
||||||
|
namespace MintyPHP\Tests\Architecture;
|
||||||
|
|
||||||
|
use PHPUnit\Framework\TestCase;
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Guard against fragile alias rewrites when query parameters are present.
|
||||||
|
*
|
||||||
|
* Minty's Router::applyRoutes() compares the full REQUEST_URI (including query)
|
||||||
|
* against alias source paths. Therefore, route aliases with query strings are
|
||||||
|
* brittle when path != target and should be avoided in runtime code.
|
||||||
|
*/
|
||||||
|
final class RouteAliasQuerySafetyContractTest extends TestCase
|
||||||
|
{
|
||||||
|
use ProjectFileAssertionSupport;
|
||||||
|
|
||||||
|
public function testRuntimeCodeDoesNotUseCoreAliasPathsWithQueryStringsWhenPathDiffersFromTarget(): void
|
||||||
|
{
|
||||||
|
$aliases = $this->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<string>
|
||||||
|
*/
|
||||||
|
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<string> $scanTargets
|
||||||
|
* @param list<string> $extensions
|
||||||
|
* @return list<string>
|
||||||
|
*/
|
||||||
|
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;
|
||||||
|
}
|
||||||
|
}
|
||||||
@@ -40,7 +40,7 @@ final class RouteCatalogTest extends TestCase
|
|||||||
],
|
],
|
||||||
$catalog->coreRoutes()
|
$catalog->coreRoutes()
|
||||||
);
|
);
|
||||||
self::assertSame(['login'], $catalog->corePublicPaths());
|
self::assertSame(['auth/login', 'login'], $catalog->corePublicPaths());
|
||||||
}
|
}
|
||||||
|
|
||||||
public function testThrowsWhenRouteConfigDoesNotReturnArray(): void
|
public function testThrowsWhenRouteConfigDoesNotReturnArray(): void
|
||||||
|
|||||||
Reference in New Issue
Block a user