Fix prod-mode asserts, middleware exception routing, redirect GET guard#330
Merged
Conversation
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.
…zationMiddleware 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.
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.
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.
ADmad
approved these changes
May 12, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Three independent, low-blast-radius bug fixes for the 3.x line.
1.
AuthorizationService— replaceassert()with explicit throwsThe three
assert(...)calls inperformCheck(),getCanHandler()andgetScopeHandler()are compiled out when PHP runs withzend.assertions=-1, which is the standard production setting. Under that configuration:[$policy, $method](...)invocation and produces a genericError: Call to undefined method ...instead of the documentedMissingMethodException.booland notResultInterface) is not caught and produces a downstream type error incan()/canResult().Existing tests covered the dev-mode behavior via
expectException(MissingMethodException::class); the if/throw rewrite makes the same exceptions fire in production as well. Behavior in dev/test is unchanged.2.
RequestAuthorizationMiddleware— route policy exceptions through the configured handler (refs #150)The
$service->canResult(...)call sat outside thetry/catchblock, so anyAuthorization\Exception\Exceptionthrown from inside aRequestPolicy(aMissingMethodException, a customMissingIdentityExceptionraised bycanAccess(), or any other policy-level failure) bypassed the configuredunauthorizedHandlerand bubbled out of the middleware unhandled. This is the exact issue surfaced in #150 (last comment by matteorebeschi explicitly identifies the asymmetry withAuthorizationMiddleware, which does wrap its check in atryblock).Moving
canResult(...)inside the existingtryblock restores the symmetry. A regression test using theSuppresshandler confirms aMissingMethodExceptionis now routed through the handler instead of escaping.3.
CakeRedirectHandler— honor the GET-only guardThe parent
RedirectHandler::getUrl()guards the redirect query-param appendage with&& $request->getMethod() === 'GET', so POST/PUT/DELETE/PATCH unauthorized responses do not receive a useless?redirect=payload. The Cake-flavored override atsrc/Middleware/UnauthorizedHandler/CakeRedirectHandler.phpdropped that guard and appended the query param for every method.Added the missing guard and a data-provider test mirroring
RedirectHandlerTest::testHandleRedirectionIgnoreNonIdempotentMethodsacross the standard non-GET methods.Verification
composer test— 147 tests, 302 assertions (was 140 / 289). 7 new assertions across 7 new tests.composer stan— clean.composer cs-check— clean.