From fdb602c2fa25c3f5e4f6c563091c0ea8cce79867 Mon Sep 17 00:00:00 2001 From: mscherer Date: Tue, 12 May 2026 16:30:46 +0200 Subject: [PATCH 1/4] Replace assert() in AuthorizationService with explicit exception throws The three assert() calls in performCheck(), getCanHandler() and getScopeHandler() are compiled out when PHP runs with zend.assertions=-1, the standard production setting. Under that configuration a missing policy method silently falls through to the callable invocation and throws a generic Error instead of MissingMethodException, and an invalid policy return value flows on and causes a downstream type error. Convert the three checks to explicit if/throw so the documented exceptions fire in both development and production. --- src/AuthorizationService.php | 23 ++++++++++------------- 1 file changed, 10 insertions(+), 13 deletions(-) diff --git a/src/AuthorizationService.php b/src/AuthorizationService.php index 5320012..c5815b0 100644 --- a/src/AuthorizationService.php +++ b/src/AuthorizationService.php @@ -94,13 +94,12 @@ protected function performCheck( $handler = $this->getCanHandler($policy, $action); $result = $handler($user, $resource, ...$optionalArgs); - assert( - is_bool($result) || $result instanceof ResultInterface, - new Exception(sprintf( + if (!is_bool($result) && !$result instanceof ResultInterface) { + throw new Exception(sprintf( 'Authorization check method must return `%s` or `bool`.', ResultInterface::class, - )), - ); + )); + } return $result; } @@ -138,10 +137,9 @@ protected function getCanHandler(mixed $policy, string $action): Closure { $method = 'can' . ucfirst($action); - assert( - method_exists($policy, $method) || method_exists($policy, '__call'), - new MissingMethodException([$method, $action, $policy::class]), - ); + if (!method_exists($policy, $method) && !method_exists($policy, '__call')) { + throw new MissingMethodException([$method, $action, $policy::class]); + } /** @phpstan-ignore callable.nonCallable */ return [$policy, $method](...); @@ -159,10 +157,9 @@ protected function getScopeHandler(mixed $policy, string $action): Closure { $method = 'scope' . ucfirst($action); - assert( - method_exists($policy, $method) || method_exists($policy, '__call'), - new MissingMethodException([$method, $action, $policy::class]), - ); + if (!method_exists($policy, $method) && !method_exists($policy, '__call')) { + throw new MissingMethodException([$method, $action, $policy::class]); + } /** @phpstan-ignore callable.nonCallable */ return [$policy, $method](...); From 0c8fced651d15ceaed8fd3cd52a425a1fe7aa21c Mon Sep 17 00:00:00 2001 From: mscherer Date: Tue, 12 May 2026 16:30:54 +0200 Subject: [PATCH 2/4] Route policy exceptions through unauthorizedHandler in RequestAuthorizationMiddleware The call to AuthorizationService::canResult() sat outside the try/catch block, so any Authorization\Exception\Exception thrown from inside a RequestPolicy (e.g. MissingMethodException, a custom MissingIdentityException raised by canAccess(), or another policy-level failure) bypassed the configured unauthorizedHandler and bubbled out of the middleware unhandled. Move the canResult() call inside the try block to match the symmetry of AuthorizationMiddleware, so all policy-level exceptions are routed through the handler. Add a regression test using the Suppress handler. --- .../RequestAuthorizationMiddleware.php | 2 +- .../RequestAuthorizationMiddlewareTest.php | 28 +++++++++++++++++++ 2 files changed, 29 insertions(+), 1 deletion(-) diff --git a/src/Middleware/RequestAuthorizationMiddleware.php b/src/Middleware/RequestAuthorizationMiddleware.php index 0808bab..95a9f41 100644 --- a/src/Middleware/RequestAuthorizationMiddleware.php +++ b/src/Middleware/RequestAuthorizationMiddleware.php @@ -98,8 +98,8 @@ public function process(ServerRequestInterface $request, RequestHandlerInterface $service = $this->getServiceFromRequest($request); $identity = $request->getAttribute($this->getConfig('identityAttribute')); - $result = $service->canResult($identity, $this->getConfig('method'), $request); try { + $result = $service->canResult($identity, $this->getConfig('method'), $request); if (!$result->getStatus()) { throw new ForbiddenException($result, [$this->getConfig('method'), $request->getRequestTarget()]); } diff --git a/tests/TestCase/Middleware/RequestAuthorizationMiddlewareTest.php b/tests/TestCase/Middleware/RequestAuthorizationMiddlewareTest.php index 88eb8b5..b03bcc5 100644 --- a/tests/TestCase/Middleware/RequestAuthorizationMiddlewareTest.php +++ b/tests/TestCase/Middleware/RequestAuthorizationMiddlewareTest.php @@ -146,4 +146,32 @@ public function testUnauthorizedHandlerSuppress(): void $this->assertSame(200, $result->getStatusCode()); } + + public function testPolicyExceptionRoutedThroughUnauthorizedHandler(): void + { + $request = (new ServerRequest([ + 'url' => '/articles/index', + ])) + ->withParam('action', 'index') + ->withParam('controller', 'Articles'); + + $handler = new TestRequestHandler(); + + $resolver = new MapResolver([ + ServerRequest::class => new RequestPolicy(), + ]); + + $authService = new AuthorizationService($resolver); + $request = $request->withAttribute('authorization', $authService); + + // `doesNotExist` triggers MissingMethodException inside canResult(); + // the middleware should route it through the configured handler instead of letting it bubble. + $middleware = new RequestAuthorizationMiddleware([ + 'method' => 'doesNotExist', + 'unauthorizedHandler' => 'Suppress', + ]); + $result = $middleware->process($request, $handler); + + $this->assertSame(200, $result->getStatusCode()); + } } From 061c18b7972c7937b8e8a67b80f9f9f1db7ca81c Mon Sep 17 00:00:00 2001 From: mscherer Date: Tue, 12 May 2026 16:31:00 +0200 Subject: [PATCH 3/4] Honor GET-only guard in CakeRedirectHandler::getUrl() The parent RedirectHandler::getUrl() guards the redirect query-param appendage with a GET method check, so POST/PUT/DELETE/PATCH unauthorized responses do not receive a useless `?redirect=` payload that clients cannot follow. The Cake-flavoured override dropped that guard and appended the query param for every method. Mirror the parent behavior and add a data-provider test covering all common non-GET methods. --- .../CakeRedirectHandler.php | 2 +- .../CakeRedirectHandlerTest.php | 36 +++++++++++++++++++ 2 files changed, 37 insertions(+), 1 deletion(-) diff --git a/src/Middleware/UnauthorizedHandler/CakeRedirectHandler.php b/src/Middleware/UnauthorizedHandler/CakeRedirectHandler.php index e5ec4f7..8576fa5 100644 --- a/src/Middleware/UnauthorizedHandler/CakeRedirectHandler.php +++ b/src/Middleware/UnauthorizedHandler/CakeRedirectHandler.php @@ -68,7 +68,7 @@ public function __construct() protected function getUrl(ServerRequestInterface $request, array $options): string { $url = $options['url']; - if ($options['queryParam'] !== null) { + if ($options['queryParam'] !== null && $request->getMethod() === 'GET') { $uri = $request->getUri(); $redirect = $uri->getPath(); if ($uri->getQuery()) { diff --git a/tests/TestCase/Middleware/UnauthorizedHandler/CakeRedirectHandlerTest.php b/tests/TestCase/Middleware/UnauthorizedHandler/CakeRedirectHandlerTest.php index 156f02c..32b9fb6 100644 --- a/tests/TestCase/Middleware/UnauthorizedHandler/CakeRedirectHandlerTest.php +++ b/tests/TestCase/Middleware/UnauthorizedHandler/CakeRedirectHandlerTest.php @@ -22,6 +22,7 @@ use Cake\Http\ServerRequestFactory; use Cake\Routing\Router; use Cake\TestSuite\TestCase; +use PHPUnit\Framework\Attributes\DataProvider; class CakeRedirectHandlerTest extends TestCase { @@ -150,4 +151,39 @@ public function testHandleRedirectWithBasePath(): void $response->getHeaderLine('Location'), ); } + + public static function httpMethodProvider(): array + { + return [ + ['POST'], + ['PUT'], + ['DELETE'], + ['PATCH'], + ['OPTIONS'], + ['HEAD'], + ]; + } + + #[DataProvider('httpMethodProvider')] + public function testHandleRedirectionIgnoreNonIdempotentMethods(string $method): void + { + $handler = new CakeRedirectHandler(); + + $exception = new Exception(); + $request = ServerRequestFactory::fromGlobals( + [ + 'REQUEST_METHOD' => $method, + 'REQUEST_URI' => '/admin/dashboard', + ], + ); + + $response = $handler->handle($exception, $request, [ + 'exceptions' => [ + Exception::class, + ], + ]); + + $this->assertSame(302, $response->getStatusCode()); + $this->assertSame('/login', $response->getHeaderLine('Location')); + } } From 6f3ee84fea0608cb396d51941da12c5e2614b510 Mon Sep 17 00:00:00 2001 From: mscherer Date: Tue, 12 May 2026 16:45:10 +0200 Subject: [PATCH 4/4] Add regression test for invalid return type from policy method Covers the explicit-throw branch added in AuthorizationService::performCheck() when a policy method returns a value that is neither bool nor a ResultInterface. Adds a canInvalidReturnType() helper to ArticlePolicy returning a plain string and asserts the documented exception fires. --- tests/TestCase/AuthorizationServiceTest.php | 19 +++++++++++++++++++ .../test_app/TestApp/Policy/ArticlePolicy.php | 9 +++++++++ 2 files changed, 28 insertions(+) diff --git a/tests/TestCase/AuthorizationServiceTest.php b/tests/TestCase/AuthorizationServiceTest.php index c68d6d4..457f6a9 100644 --- a/tests/TestCase/AuthorizationServiceTest.php +++ b/tests/TestCase/AuthorizationServiceTest.php @@ -17,6 +17,7 @@ namespace Authorization\Test\TestCase; use Authorization\AuthorizationService; +use Authorization\Exception\Exception; use Authorization\IdentityDecorator; use Authorization\IdentityInterface; use Authorization\Policy\BeforePolicyInterface; @@ -539,4 +540,22 @@ public function testMissingMethod(): void $service->can($user, 'disable', $entity); } + + public function testCanThrowsWhenPolicyReturnsInvalidType(): void + { + $resolver = new MapResolver([ + Article::class => ArticlePolicy::class, + ]); + $service = new AuthorizationService($resolver); + $user = new IdentityDecorator($service, [ + 'role' => 'admin', + ]); + + $this->expectException(Exception::class); + $this->expectExceptionMessage( + 'Authorization check method must return `Authorization\Policy\ResultInterface` or `bool`.', + ); + + $service->can($user, 'invalidReturnType', new Article()); + } } diff --git a/tests/test_app/TestApp/Policy/ArticlePolicy.php b/tests/test_app/TestApp/Policy/ArticlePolicy.php index d5e219a..7211c07 100644 --- a/tests/test_app/TestApp/Policy/ArticlePolicy.php +++ b/tests/test_app/TestApp/Policy/ArticlePolicy.php @@ -141,4 +141,13 @@ public function canWithInjectedService($user, Article $article) { return $this->service->serviceLogic(); } + + /** + * Returns an invalid type (not bool, not ResultInterface) to exercise the + * defensive type guard in AuthorizationService::performCheck(). + */ + public function canInvalidReturnType($user, Article $article): string + { + return 'not a bool nor a Result'; + } }