From 1c1e29c7e8129cfbcae74558316ecd3ea50a8273 Mon Sep 17 00:00:00 2001 From: Mark Story Date: Sat, 13 Jun 2026 06:24:39 -0400 Subject: [PATCH 1/4] Fix an open redirect weakness in getLoginRedirect() Backport of #795 to 3.x Because of how browsers handle the `Location` header, values beginning with `\` can be leveraged to create redirect targets on other domains. Thanks to Volker Dusch and the PHP Ecosystem security team for reporting this. --- src/AuthenticationService.php | 16 +- tests/TestCase/AuthenticationServiceTest.php | 195 ++++++++++++++++++- 2 files changed, 209 insertions(+), 2 deletions(-) diff --git a/src/AuthenticationService.php b/src/AuthenticationService.php index b28d65bb..07bb80e8 100644 --- a/src/AuthenticationService.php +++ b/src/AuthenticationService.php @@ -413,8 +413,19 @@ public function getLoginRedirect(ServerRequestInterface $request): ?string ) { return null; } + $value = (string)$params[$redirectParam]; - $parsed = parse_url($params[$redirectParam]); + // In the `Location` header, Browsers normalize \ to / + // (see WHATWG URL Standard). + // We do the same to prevent injection via \ sequences. + $normalized = str_replace('\\', '/', $value); + + // A leading run of `//` or `\\` are rejected + if (str_starts_with($normalized, '//')) { + return null; + } + + $parsed = parse_url($normalized); if ($parsed === false) { return null; } @@ -422,6 +433,9 @@ public function getLoginRedirect(ServerRequestInterface $request): ?string return null; } $parsed += ['path' => '/', 'query' => '']; + if (str_contains($parsed['path'], '\\')) { + return null; + } if (strlen($parsed['path']) && $parsed['path'][0] !== '/') { $parsed['path'] = "/{$parsed['path']}"; } diff --git a/tests/TestCase/AuthenticationServiceTest.php b/tests/TestCase/AuthenticationServiceTest.php index 8be8b4fc..067a7e12 100644 --- a/tests/TestCase/AuthenticationServiceTest.php +++ b/tests/TestCase/AuthenticationServiceTest.php @@ -847,7 +847,7 @@ public function testGetUnauthenticatedRedirectUrlWithBasePath() ); } - public function testGetLoginRedirect() + public function testGetLoginRedirectInvalid() { $service = new AuthenticationService([ 'unauthenticatedRedirect' => '/users/login', @@ -888,6 +888,199 @@ public function testGetLoginRedirect() '/path/with?query=string', $service->getLoginRedirect($request) ); + + $request = ServerRequestFactory::fromGlobals( + ['REQUEST_URI' => '/login'], + ['redirect' => '/\\evil.com'] + ); + $this->assertNull( + $service->getLoginRedirect($request) + ); + + $request = ServerRequestFactory::fromGlobals( + ['REQUEST_URI' => '/login'], + ['redirect' => '\\/\\evil.com'] + ); + $this->assertNull( + $service->getLoginRedirect($request) + ); + + $request = ServerRequestFactory::fromGlobals( + ['REQUEST_URI' => '/login'], + ['redirect' => '\\evil.com/path'] + ); + $this->assertSame( + '/evil.com/path', + $service->getLoginRedirect($request) + ); + + $request = ServerRequestFactory::fromGlobals( + ['REQUEST_URI' => '/login'], + ['redirect' => '\\\\evil.com/path'] + ); + $this->assertNull( + $service->getLoginRedirect($request) + ); + } + + /** + * testGetLoginRedirectValidationDisabled + * + * @return void + */ + public function testGetLoginRedirectValidationDisabled() + { + $service = new AuthenticationService([ + 'unauthenticatedRedirect' => '/users/login', + 'queryParam' => 'redirect', + 'redirectValidation' => [ + 'enabled' => false, + ] + ]); + + // With validation disabled, even nested redirects should pass + $request = ServerRequestFactory::fromGlobals( + ['REQUEST_URI' => '/secrets'], + ['redirect' => '/login?redirect=/secrets'] + ); + $this->assertSame( + '/login?redirect=/secrets', + $service->getLoginRedirect($request) + ); + } + + /** + * testGetLoginRedirectValidationNestedRedirects + * + * @return void + */ + public function testGetLoginRedirectValidationNestedRedirects() + { + $service = new AuthenticationService([ + 'unauthenticatedRedirect' => '/users/login', + 'queryParam' => 'redirect', + 'redirectValidation' => [ + 'enabled' => true, + ] + ]); + + // Valid single-level redirect + $request = ServerRequestFactory::fromGlobals( + ['REQUEST_URI' => '/secrets'], + ['redirect' => '/articles/view/1'] + ); + $this->assertSame( + '/articles/view/1', + $service->getLoginRedirect($request) + ); + + // Nested redirect (should be blocked) + $request = ServerRequestFactory::fromGlobals( + ['REQUEST_URI' => '/secrets'], + ['redirect' => '/login?redirect=/articles/view/1'] + ); + $this->assertNull($service->getLoginRedirect($request)); + + // Deeply nested redirect (should be blocked) + $request = ServerRequestFactory::fromGlobals( + ['REQUEST_URI' => '/secrets'], + ['redirect' => '/login?redirect=%2Flogin%3Fredirect%3D%252Farticles'] + ); + $this->assertNull($service->getLoginRedirect($request)); + } + + /** + * testGetLoginRedirectValidationEncodingLevels + * + * @return void + */ + public function testGetLoginRedirectValidationEncodingLevels() + { + $service = new AuthenticationService([ + 'unauthenticatedRedirect' => '/users/login', + 'queryParam' => 'redirect', + 'redirectValidation' => [ + 'enabled' => true, + ] + ]); + + // Normal single-encoded URL (should pass) + $request = ServerRequestFactory::fromGlobals( + ['REQUEST_URI' => '/secrets'], + ['redirect' => '/articles%2Fview%2F1'], + ); + $this->assertSame( + '/articles%2Fview%2F1', + $service->getLoginRedirect($request) + ); + + // Double-encoded URL (should be blocked) + $request = ServerRequestFactory::fromGlobals( + ['REQUEST_URI' => '/secrets'], + ['redirect' => '/articles%252Fview%252F1'] + ); + $this->assertNull($service->getLoginRedirect($request)); + } + + /** + * testGetLoginRedirectValidationMaxLength + * + * @return void + */ + public function testGetLoginRedirectValidationMaxLength() + { + $service = new AuthenticationService([ + 'unauthenticatedRedirect' => '/users/login', + 'queryParam' => 'redirect', + 'redirectValidation' => [ + 'enabled' => true, + 'maxLength' => 100, + ] + ]); + + // Short URL (should pass) + $request = ServerRequestFactory::fromGlobals( + ['REQUEST_URI' => '/secrets'], + ['redirect' => '/articles/view/1'] + ); + $this->assertSame( + '/articles/view/1', + $service->getLoginRedirect($request) + ); + + // Excessively long URL (should be blocked) + $longUrl = '/articles/' . str_repeat('a', 150); + $request = ServerRequestFactory::fromGlobals( + ['REQUEST_URI' => '/secrets'], + ['redirect' => $longUrl] + ); + $this->assertNull($service->getLoginRedirect($request)); + } + + /** + * testGetLoginRedirectValidationWithQueryParameters + * + * @return void + */ + public function testGetLoginRedirectValidationWithQueryParameters() + { + $service = new AuthenticationService([ + 'unauthenticatedRedirect' => '/users/login', + 'queryParam' => 'redirect', + 'redirectValidation' => [ + 'enabled' => true, + ] + ]); + + // Valid redirect with query parameters + $request = ServerRequestFactory::fromGlobals( + ['REQUEST_URI' => '/secrets'], + ['redirect' => '/articles/index?sort=created&direction=desc'] + ); + $this->assertSame( + '/articles/index?sort=created&direction=desc', + $service->getLoginRedirect($request) + ); } /** From 62c0786cc6b922989a08f81a47d6969a5d63f3bd Mon Sep 17 00:00:00 2001 From: Mark Story Date: Sun, 14 Jun 2026 00:52:36 -0400 Subject: [PATCH 2/4] Update composer.json The firebase/php-jwt package contains a vulnerability that can only be resolved by going to 7.x. I didn't want to also change the requirements of this package as part of the redirect fix. --- composer.json | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/composer.json b/composer.json index f01ebb71..c97fb78a 100644 --- a/composer.json +++ b/composer.json @@ -80,6 +80,11 @@ "sort-packages": true, "allow-plugins": { "dealerdirect/phpcodesniffer-composer-installer": true + }, + "policy": { + "advisories": { + "ignore": ["PKSA-y2cr-5h3j-g3ys"] + } } }, "minimum-stability": "dev", From 84b0a2bc5545e527d5b4336492b2dbdb88d44248 Mon Sep 17 00:00:00 2001 From: Mark Story Date: Wed, 24 Jun 2026 00:21:08 -0400 Subject: [PATCH 3/4] Older PHP compatibility --- src/AuthenticationService.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/AuthenticationService.php b/src/AuthenticationService.php index 07bb80e8..5274fd20 100644 --- a/src/AuthenticationService.php +++ b/src/AuthenticationService.php @@ -421,7 +421,7 @@ public function getLoginRedirect(ServerRequestInterface $request): ?string $normalized = str_replace('\\', '/', $value); // A leading run of `//` or `\\` are rejected - if (str_starts_with($normalized, '//')) { + if (strpos($normalized, '//') === 0) { return null; } @@ -433,7 +433,7 @@ public function getLoginRedirect(ServerRequestInterface $request): ?string return null; } $parsed += ['path' => '/', 'query' => '']; - if (str_contains($parsed['path'], '\\')) { + if (strpos($parsed['path'], '\\') !== false) { return null; } if (strlen($parsed['path']) && $parsed['path'][0] !== '/') { From 5bc0e9901946af4d243deaf118fa19f010fde0d8 Mon Sep 17 00:00:00 2001 From: Mark Story Date: Wed, 24 Jun 2026 00:39:21 -0400 Subject: [PATCH 4/4] Reduce scope of patch We don't want this much from 3.x --- tests/TestCase/AuthenticationServiceTest.php | 160 ------------------- 1 file changed, 160 deletions(-) diff --git a/tests/TestCase/AuthenticationServiceTest.php b/tests/TestCase/AuthenticationServiceTest.php index 067a7e12..54fa0dbc 100644 --- a/tests/TestCase/AuthenticationServiceTest.php +++ b/tests/TestCase/AuthenticationServiceTest.php @@ -923,166 +923,6 @@ public function testGetLoginRedirectInvalid() ); } - /** - * testGetLoginRedirectValidationDisabled - * - * @return void - */ - public function testGetLoginRedirectValidationDisabled() - { - $service = new AuthenticationService([ - 'unauthenticatedRedirect' => '/users/login', - 'queryParam' => 'redirect', - 'redirectValidation' => [ - 'enabled' => false, - ] - ]); - - // With validation disabled, even nested redirects should pass - $request = ServerRequestFactory::fromGlobals( - ['REQUEST_URI' => '/secrets'], - ['redirect' => '/login?redirect=/secrets'] - ); - $this->assertSame( - '/login?redirect=/secrets', - $service->getLoginRedirect($request) - ); - } - - /** - * testGetLoginRedirectValidationNestedRedirects - * - * @return void - */ - public function testGetLoginRedirectValidationNestedRedirects() - { - $service = new AuthenticationService([ - 'unauthenticatedRedirect' => '/users/login', - 'queryParam' => 'redirect', - 'redirectValidation' => [ - 'enabled' => true, - ] - ]); - - // Valid single-level redirect - $request = ServerRequestFactory::fromGlobals( - ['REQUEST_URI' => '/secrets'], - ['redirect' => '/articles/view/1'] - ); - $this->assertSame( - '/articles/view/1', - $service->getLoginRedirect($request) - ); - - // Nested redirect (should be blocked) - $request = ServerRequestFactory::fromGlobals( - ['REQUEST_URI' => '/secrets'], - ['redirect' => '/login?redirect=/articles/view/1'] - ); - $this->assertNull($service->getLoginRedirect($request)); - - // Deeply nested redirect (should be blocked) - $request = ServerRequestFactory::fromGlobals( - ['REQUEST_URI' => '/secrets'], - ['redirect' => '/login?redirect=%2Flogin%3Fredirect%3D%252Farticles'] - ); - $this->assertNull($service->getLoginRedirect($request)); - } - - /** - * testGetLoginRedirectValidationEncodingLevels - * - * @return void - */ - public function testGetLoginRedirectValidationEncodingLevels() - { - $service = new AuthenticationService([ - 'unauthenticatedRedirect' => '/users/login', - 'queryParam' => 'redirect', - 'redirectValidation' => [ - 'enabled' => true, - ] - ]); - - // Normal single-encoded URL (should pass) - $request = ServerRequestFactory::fromGlobals( - ['REQUEST_URI' => '/secrets'], - ['redirect' => '/articles%2Fview%2F1'], - ); - $this->assertSame( - '/articles%2Fview%2F1', - $service->getLoginRedirect($request) - ); - - // Double-encoded URL (should be blocked) - $request = ServerRequestFactory::fromGlobals( - ['REQUEST_URI' => '/secrets'], - ['redirect' => '/articles%252Fview%252F1'] - ); - $this->assertNull($service->getLoginRedirect($request)); - } - - /** - * testGetLoginRedirectValidationMaxLength - * - * @return void - */ - public function testGetLoginRedirectValidationMaxLength() - { - $service = new AuthenticationService([ - 'unauthenticatedRedirect' => '/users/login', - 'queryParam' => 'redirect', - 'redirectValidation' => [ - 'enabled' => true, - 'maxLength' => 100, - ] - ]); - - // Short URL (should pass) - $request = ServerRequestFactory::fromGlobals( - ['REQUEST_URI' => '/secrets'], - ['redirect' => '/articles/view/1'] - ); - $this->assertSame( - '/articles/view/1', - $service->getLoginRedirect($request) - ); - - // Excessively long URL (should be blocked) - $longUrl = '/articles/' . str_repeat('a', 150); - $request = ServerRequestFactory::fromGlobals( - ['REQUEST_URI' => '/secrets'], - ['redirect' => $longUrl] - ); - $this->assertNull($service->getLoginRedirect($request)); - } - - /** - * testGetLoginRedirectValidationWithQueryParameters - * - * @return void - */ - public function testGetLoginRedirectValidationWithQueryParameters() - { - $service = new AuthenticationService([ - 'unauthenticatedRedirect' => '/users/login', - 'queryParam' => 'redirect', - 'redirectValidation' => [ - 'enabled' => true, - ] - ]); - - // Valid redirect with query parameters - $request = ServerRequestFactory::fromGlobals( - ['REQUEST_URI' => '/secrets'], - ['redirect' => '/articles/index?sort=created&direction=desc'] - ); - $this->assertSame( - '/articles/index?sort=created&direction=desc', - $service->getLoginRedirect($request) - ); - } - /** * testImpersonate *