From 94379b4fe90090e6eb998ebe35acc99cab25c1b7 Mon Sep 17 00:00:00 2001 From: mscherer Date: Fri, 27 Jun 2025 22:08:43 +0200 Subject: [PATCH 1/8] Allow skipping redirects for extensions. --- .../CakeRedirectHandler.php | 1 + .../UnauthorizedHandler/RedirectHandler.php | 29 ++++++++++++- .../RedirectHandlerTest.php | 43 +++++++++++++++++++ 3 files changed, 71 insertions(+), 2 deletions(-) diff --git a/src/Middleware/UnauthorizedHandler/CakeRedirectHandler.php b/src/Middleware/UnauthorizedHandler/CakeRedirectHandler.php index e6318bf..c54b882 100644 --- a/src/Middleware/UnauthorizedHandler/CakeRedirectHandler.php +++ b/src/Middleware/UnauthorizedHandler/CakeRedirectHandler.php @@ -41,6 +41,7 @@ class CakeRedirectHandler extends RedirectHandler ], 'queryParam' => 'redirect', 'statusCode' => 302, + 'allowedRedirectExtensions' => false, ]; /** diff --git a/src/Middleware/UnauthorizedHandler/RedirectHandler.php b/src/Middleware/UnauthorizedHandler/RedirectHandler.php index 8910b16..863d53c 100644 --- a/src/Middleware/UnauthorizedHandler/RedirectHandler.php +++ b/src/Middleware/UnauthorizedHandler/RedirectHandler.php @@ -44,6 +44,7 @@ class RedirectHandler implements HandlerInterface 'url' => '/login', 'queryParam' => 'redirect', 'statusCode' => 302, + 'allowedRedirectExtensions' => false, ]; /** @@ -58,7 +59,7 @@ public function handle( ): ResponseInterface { $options += $this->defaultOptions; - if (!$this->checkException($exception, $options['exceptions'])) { + if (!$this->redirectAllowed($request, $options) || !$this->checkException($exception, $options['exceptions'])) { throw $exception; } @@ -106,7 +107,7 @@ protected function getUrl(ServerRequestInterface $request, array $options): stri $redirect .= '?' . $uri->getQuery(); } $query = urlencode($options['queryParam']) . '=' . urlencode($redirect); - if (strpos($url, '?') !== false) { + if (str_contains($url, '?')) { $query = '&' . $query; } else { $query = '?' . $query; @@ -117,4 +118,28 @@ protected function getUrl(ServerRequestInterface $request, array $options): stri return $url; } + + /** + * @param \Psr\Http\Message\ServerRequestInterface $request + * @param array $options + * @return bool + */ + protected function redirectAllowed(ServerRequestInterface $request, array $options): bool + { + $extensions = $options['allowedRedirectExtensions'] ?? false; + // BC: false disables it. + if ($extensions === false) { + return true; + } + + $extensions = (array)$extensions; + /** @var \Cake\Http\ServerRequest $request */ + $currentExtension = $request->getParam('_ext'); + + if (!$currentExtension || in_array($currentExtension, $extensions, true)) { + return true; + } + + return false; + } } diff --git a/tests/TestCase/Middleware/UnauthorizedHandler/RedirectHandlerTest.php b/tests/TestCase/Middleware/UnauthorizedHandler/RedirectHandlerTest.php index bbbaa81..39913a3 100644 --- a/tests/TestCase/Middleware/UnauthorizedHandler/RedirectHandlerTest.php +++ b/tests/TestCase/Middleware/UnauthorizedHandler/RedirectHandlerTest.php @@ -160,4 +160,47 @@ public function testHandleException() $this->expectException(Exception::class); $handler->handle($exception, $request); } + + public function testHandleRedirectionWithExtension(): void + { + $handler = new RedirectHandler(); + + $exception = new Exception(); + $request = ServerRequestFactory::fromGlobals( + ['REQUEST_METHOD' => 'GET'], + ); + + $this->expectException(Exception::class); + + $handler->handle($exception, $request, [ + 'exceptions' => [ + Exception::class, + ], + 'url' => '/users/login', + 'allowedRedirectExtensions' => [], + ]); + } + + public function testHandleRedirectionWithExtensionWhitelisted(): void + { + $handler = new RedirectHandler(); + + $exception = new Exception(); + $request = ServerRequestFactory::fromGlobals( + ['REQUEST_METHOD' => 'GET'], + ); + $request = $request->withParam('_ext', 'csv'); + + $response = $handler->handle($exception, $request, [ + 'exceptions' => [ + Exception::class, + ], + 'url' => '/users/login', + 'queryParam' => null, + 'allowedRedirectExtensions' => ['csv'], + ]); + + $this->assertSame(302, $response->getStatusCode()); + $this->assertSame('/users/login', $response->getHeaderLine('Location')); + } } From c85121cbcdae2f01b91e1d62c3678586bb9f444c Mon Sep 17 00:00:00 2001 From: mscherer Date: Fri, 27 Jun 2025 22:18:26 +0200 Subject: [PATCH 2/8] Allow skipping redirects for extensions. --- docs/en/middleware.rst | 5 +++++ src/Middleware/UnauthorizedHandler/RedirectHandler.php | 6 ++++-- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/docs/en/middleware.rst b/docs/en/middleware.rst index 69fd86e..cb95168 100644 --- a/docs/en/middleware.rst +++ b/docs/en/middleware.rst @@ -198,6 +198,10 @@ Both redirect handlers share the same configuration options: * ``queryParam`` - the accessed request URL will be attached to the redirect URL query parameter (``redirect`` by default). * ``statusCode`` - HTTP status code of a redirect, ``302`` by default. +* ``allowedRedirectExtensions`` - an array of allowed file extensions for redirecting. + If the request URL has a file extension that is not in this list, the redirect will not + happen and the exception will be rethrown. This is useful to prevent unauthorized access + to API based responses, that should not be redirecting in any case. `false` by default and not enabled then. For example:: @@ -212,6 +216,7 @@ For example:: MissingIdentityException::class, OtherException::class, ], + 'allowedRedirectExtensions' => ['csv', 'pdf'], ], ])); diff --git a/src/Middleware/UnauthorizedHandler/RedirectHandler.php b/src/Middleware/UnauthorizedHandler/RedirectHandler.php index 863d53c..f08e69f 100644 --- a/src/Middleware/UnauthorizedHandler/RedirectHandler.php +++ b/src/Middleware/UnauthorizedHandler/RedirectHandler.php @@ -132,11 +132,13 @@ protected function redirectAllowed(ServerRequestInterface $request, array $optio return true; } - $extensions = (array)$extensions; /** @var \Cake\Http\ServerRequest $request */ $currentExtension = $request->getParam('_ext'); + if (!$currentExtension || $extensions === true) { + return true; + } - if (!$currentExtension || in_array($currentExtension, $extensions, true)) { + if (in_array($currentExtension, (array)$extensions, true)) { return true; } From 203594c995cbf2489de6497e4fe4e3294935fa32 Mon Sep 17 00:00:00 2001 From: mscherer Date: Fri, 27 Jun 2025 22:19:29 +0200 Subject: [PATCH 3/8] Allow skipping redirects for extensions. --- .../Middleware/UnauthorizedHandler/RedirectHandlerTest.php | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/TestCase/Middleware/UnauthorizedHandler/RedirectHandlerTest.php b/tests/TestCase/Middleware/UnauthorizedHandler/RedirectHandlerTest.php index 39913a3..a192de8 100644 --- a/tests/TestCase/Middleware/UnauthorizedHandler/RedirectHandlerTest.php +++ b/tests/TestCase/Middleware/UnauthorizedHandler/RedirectHandlerTest.php @@ -169,6 +169,7 @@ public function testHandleRedirectionWithExtension(): void $request = ServerRequestFactory::fromGlobals( ['REQUEST_METHOD' => 'GET'], ); + $request = $request->withParam('_ext', 'csv'); $this->expectException(Exception::class); From 0856be0e40cc82e4e0fffce9369dd7afef70e027 Mon Sep 17 00:00:00 2001 From: Mark Scherer Date: Mon, 30 Jun 2025 02:17:03 +0200 Subject: [PATCH 4/8] Update docs/en/middleware.rst Co-authored-by: Mark Story --- docs/en/middleware.rst | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/docs/en/middleware.rst b/docs/en/middleware.rst index cb95168..17e54e3 100644 --- a/docs/en/middleware.rst +++ b/docs/en/middleware.rst @@ -200,8 +200,9 @@ Both redirect handlers share the same configuration options: * ``statusCode`` - HTTP status code of a redirect, ``302`` by default. * ``allowedRedirectExtensions`` - an array of allowed file extensions for redirecting. If the request URL has a file extension that is not in this list, the redirect will not - happen and the exception will be rethrown. This is useful to prevent unauthorized access - to API based responses, that should not be redirecting in any case. `false` by default and not enabled then. + happen and the exception will be rethrown. Can also be a boolean to toggle on/off + redirects entirely. This is useful to prevent unauthorized access to API based + responses, that should not be redirecting in any case. `false` by default and not enabled then. For example:: From ae4ed66201780b46e1a61925a68314b445b15573 Mon Sep 17 00:00:00 2001 From: Mark Scherer Date: Mon, 30 Jun 2025 02:17:07 +0200 Subject: [PATCH 5/8] Update tests/TestCase/Middleware/UnauthorizedHandler/RedirectHandlerTest.php Co-authored-by: Mark Story --- .../Middleware/UnauthorizedHandler/RedirectHandlerTest.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/TestCase/Middleware/UnauthorizedHandler/RedirectHandlerTest.php b/tests/TestCase/Middleware/UnauthorizedHandler/RedirectHandlerTest.php index a192de8..27b9d90 100644 --- a/tests/TestCase/Middleware/UnauthorizedHandler/RedirectHandlerTest.php +++ b/tests/TestCase/Middleware/UnauthorizedHandler/RedirectHandlerTest.php @@ -182,7 +182,7 @@ public function testHandleRedirectionWithExtension(): void ]); } - public function testHandleRedirectionWithExtensionWhitelisted(): void + public function testHandleRedirectionWithExtensionAllowlisted(): void { $handler = new RedirectHandler(); From 79fded8c9c8312c8ee7a33532d3f49a6ccf3e174 Mon Sep 17 00:00:00 2001 From: Mark Scherer Date: Mon, 30 Jun 2025 02:17:17 +0200 Subject: [PATCH 6/8] Update tests/TestCase/Middleware/UnauthorizedHandler/RedirectHandlerTest.php Co-authored-by: Mark Story --- .../Middleware/UnauthorizedHandler/RedirectHandlerTest.php | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/TestCase/Middleware/UnauthorizedHandler/RedirectHandlerTest.php b/tests/TestCase/Middleware/UnauthorizedHandler/RedirectHandlerTest.php index 27b9d90..c0170d2 100644 --- a/tests/TestCase/Middleware/UnauthorizedHandler/RedirectHandlerTest.php +++ b/tests/TestCase/Middleware/UnauthorizedHandler/RedirectHandlerTest.php @@ -165,17 +165,17 @@ public function testHandleRedirectionWithExtension(): void { $handler = new RedirectHandler(); - $exception = new Exception(); + $exception = new LogicException(); $request = ServerRequestFactory::fromGlobals( ['REQUEST_METHOD' => 'GET'], ); $request = $request->withParam('_ext', 'csv'); - $this->expectException(Exception::class); + $this->expectException(LogicException::class); $handler->handle($exception, $request, [ 'exceptions' => [ - Exception::class, + LogicException::class, ], 'url' => '/users/login', 'allowedRedirectExtensions' => [], From 1205024391f25aebbab4b3f6cdca3244298625e0 Mon Sep 17 00:00:00 2001 From: mscherer Date: Mon, 30 Jun 2025 10:47:42 +0200 Subject: [PATCH 7/8] Allow skipping redirects for extensions. --- docs/en/middleware.rst | 4 +- src/Exception/LogicException.php | 70 +++++++++++++++++++ .../CakeRedirectHandler.php | 2 +- .../UnauthorizedHandler/RedirectHandler.php | 12 ++-- .../RedirectHandlerTest.php | 1 + 5 files changed, 82 insertions(+), 7 deletions(-) create mode 100644 src/Exception/LogicException.php diff --git a/docs/en/middleware.rst b/docs/en/middleware.rst index 17e54e3..ff9cd32 100644 --- a/docs/en/middleware.rst +++ b/docs/en/middleware.rst @@ -201,8 +201,8 @@ Both redirect handlers share the same configuration options: * ``allowedRedirectExtensions`` - an array of allowed file extensions for redirecting. If the request URL has a file extension that is not in this list, the redirect will not happen and the exception will be rethrown. Can also be a boolean to toggle on/off - redirects entirely. This is useful to prevent unauthorized access to API based - responses, that should not be redirecting in any case. `false` by default and not enabled then. + redirects entirely. This is useful to prevent unauthorized access to API based + responses, that should not be redirecting in any case. `true` by default and not enabled then. For example:: diff --git a/src/Exception/LogicException.php b/src/Exception/LogicException.php new file mode 100644 index 0000000..b4e4808 --- /dev/null +++ b/src/Exception/LogicException.php @@ -0,0 +1,70 @@ +result = $result; + + parent::__construct($message, $code, $previous); + } + + /** + * Returns policy check result if passed to the exception. + * + * @return \Authorization\Policy\ResultInterface|null + */ + public function getResult(): ?ResultInterface + { + return $this->result; + } +} diff --git a/src/Middleware/UnauthorizedHandler/CakeRedirectHandler.php b/src/Middleware/UnauthorizedHandler/CakeRedirectHandler.php index c54b882..e5ec4f7 100644 --- a/src/Middleware/UnauthorizedHandler/CakeRedirectHandler.php +++ b/src/Middleware/UnauthorizedHandler/CakeRedirectHandler.php @@ -41,7 +41,7 @@ class CakeRedirectHandler extends RedirectHandler ], 'queryParam' => 'redirect', 'statusCode' => 302, - 'allowedRedirectExtensions' => false, + 'allowedRedirectExtensions' => true, ]; /** diff --git a/src/Middleware/UnauthorizedHandler/RedirectHandler.php b/src/Middleware/UnauthorizedHandler/RedirectHandler.php index f08e69f..25ed55a 100644 --- a/src/Middleware/UnauthorizedHandler/RedirectHandler.php +++ b/src/Middleware/UnauthorizedHandler/RedirectHandler.php @@ -34,6 +34,8 @@ class RedirectHandler implements HandlerInterface * - `url` - Url to redirect to. * - `queryParam` - Query parameter name for the target url. * - `statusCode` - Redirection status code. + * - `allowedRedirectExtensions` - If true, redirects are allowed for all extensions. + * Pass specific ones to allow list, or false to disallow redirects for any extension. * * @var array */ @@ -44,7 +46,7 @@ class RedirectHandler implements HandlerInterface 'url' => '/login', 'queryParam' => 'redirect', 'statusCode' => 302, - 'allowedRedirectExtensions' => false, + 'allowedRedirectExtensions' => true, ]; /** @@ -126,15 +128,17 @@ protected function getUrl(ServerRequestInterface $request, array $options): stri */ protected function redirectAllowed(ServerRequestInterface $request, array $options): bool { - $extensions = $options['allowedRedirectExtensions'] ?? false; - // BC: false disables it. + $extensions = $options['allowedRedirectExtensions'] ?? true; if ($extensions === false) { + return false; + } + if ($extensions === true) { return true; } /** @var \Cake\Http\ServerRequest $request */ $currentExtension = $request->getParam('_ext'); - if (!$currentExtension || $extensions === true) { + if (!$currentExtension) { return true; } diff --git a/tests/TestCase/Middleware/UnauthorizedHandler/RedirectHandlerTest.php b/tests/TestCase/Middleware/UnauthorizedHandler/RedirectHandlerTest.php index c0170d2..0313f38 100644 --- a/tests/TestCase/Middleware/UnauthorizedHandler/RedirectHandlerTest.php +++ b/tests/TestCase/Middleware/UnauthorizedHandler/RedirectHandlerTest.php @@ -17,6 +17,7 @@ namespace Authorization\Test\TestCase\Middleware\UnauthorizedHandler; use Authorization\Exception\Exception; +use Authorization\Exception\LogicException; use Authorization\Middleware\UnauthorizedHandler\RedirectHandler; use Cake\Core\Configure; use Cake\Http\ServerRequestFactory; From d65662eddd007d8ea2d9533d705524bf1ed972a6 Mon Sep 17 00:00:00 2001 From: Mark Story Date: Tue, 1 Jul 2025 12:01:31 -0400 Subject: [PATCH 8/8] Remove LogicException - We can use one of the existing exceptions from the plugin. - Expand test coverage to cover all the new branches --- src/Exception/LogicException.php | 70 ------------------- .../UnauthorizedHandler/RedirectHandler.php | 6 +- .../RedirectHandlerTest.php | 54 ++++++++++++-- 3 files changed, 50 insertions(+), 80 deletions(-) delete mode 100644 src/Exception/LogicException.php diff --git a/src/Exception/LogicException.php b/src/Exception/LogicException.php deleted file mode 100644 index b4e4808..0000000 --- a/src/Exception/LogicException.php +++ /dev/null @@ -1,70 +0,0 @@ -result = $result; - - parent::__construct($message, $code, $previous); - } - - /** - * Returns policy check result if passed to the exception. - * - * @return \Authorization\Policy\ResultInterface|null - */ - public function getResult(): ?ResultInterface - { - return $this->result; - } -} diff --git a/src/Middleware/UnauthorizedHandler/RedirectHandler.php b/src/Middleware/UnauthorizedHandler/RedirectHandler.php index 25ed55a..cd9498b 100644 --- a/src/Middleware/UnauthorizedHandler/RedirectHandler.php +++ b/src/Middleware/UnauthorizedHandler/RedirectHandler.php @@ -142,10 +142,6 @@ protected function redirectAllowed(ServerRequestInterface $request, array $optio return true; } - if (in_array($currentExtension, (array)$extensions, true)) { - return true; - } - - return false; + return in_array($currentExtension, (array)$extensions, true); } } diff --git a/tests/TestCase/Middleware/UnauthorizedHandler/RedirectHandlerTest.php b/tests/TestCase/Middleware/UnauthorizedHandler/RedirectHandlerTest.php index 0313f38..9eda131 100644 --- a/tests/TestCase/Middleware/UnauthorizedHandler/RedirectHandlerTest.php +++ b/tests/TestCase/Middleware/UnauthorizedHandler/RedirectHandlerTest.php @@ -17,11 +17,12 @@ namespace Authorization\Test\TestCase\Middleware\UnauthorizedHandler; use Authorization\Exception\Exception; -use Authorization\Exception\LogicException; +use Authorization\Exception\MissingIdentityException; use Authorization\Middleware\UnauthorizedHandler\RedirectHandler; use Cake\Core\Configure; use Cake\Http\ServerRequestFactory; use Cake\TestSuite\TestCase; +use LogicException; use PHPUnit\Framework\Attributes\DataProvider; class RedirectHandlerTest extends TestCase @@ -162,23 +163,44 @@ public function testHandleException() $handler->handle($exception, $request); } - public function testHandleRedirectionWithExtension(): void + public function testHandleRedirectionWithExtensionsFalse(): void { $handler = new RedirectHandler(); - $exception = new LogicException(); + $exception = new MissingIdentityException(); $request = ServerRequestFactory::fromGlobals( ['REQUEST_METHOD' => 'GET'], ); $request = $request->withParam('_ext', 'csv'); - $this->expectException(LogicException::class); + $this->expectException(MissingIdentityException::class); $handler->handle($exception, $request, [ 'exceptions' => [ LogicException::class, ], 'url' => '/users/login', + 'allowedRedirectExtensions' => false, + ]); + } + + public function testHandleRedirectionWithExtension(): void + { + $handler = new RedirectHandler(); + + $exception = new MissingIdentityException(); + $request = ServerRequestFactory::fromGlobals( + ['REQUEST_METHOD' => 'GET'], + ); + $request = $request->withParam('_ext', 'csv'); + + $this->expectException(MissingIdentityException::class); + + $handler->handle($exception, $request, [ + 'exceptions' => [ + MissingIdentityException::class, + ], + 'url' => '/users/login', 'allowedRedirectExtensions' => [], ]); } @@ -187,7 +209,7 @@ public function testHandleRedirectionWithExtensionAllowlisted(): void { $handler = new RedirectHandler(); - $exception = new Exception(); + $exception = new MissingIdentityException(); $request = ServerRequestFactory::fromGlobals( ['REQUEST_METHOD' => 'GET'], ); @@ -205,4 +227,26 @@ public function testHandleRedirectionWithExtensionAllowlisted(): void $this->assertSame(302, $response->getStatusCode()); $this->assertSame('/users/login', $response->getHeaderLine('Location')); } + + public function testHandleRedirectionWithExtensionAllowedNoExtensionInRequest(): void + { + $handler = new RedirectHandler(); + + $exception = new Exception(); + $request = ServerRequestFactory::fromGlobals( + ['REQUEST_METHOD' => 'GET'], + ); + + $response = $handler->handle($exception, $request, [ + 'exceptions' => [ + Exception::class, + ], + 'url' => '/users/login', + 'queryParam' => null, + 'allowedRedirectExtensions' => ['csv'], + ]); + + $this->assertSame(302, $response->getStatusCode()); + $this->assertSame('/users/login', $response->getHeaderLine('Location')); + } }