Skip to content

Fix prod-mode asserts, middleware exception routing, redirect GET guard#330

Merged
dereuromark merged 5 commits into
3.xfrom
bugfix-asserts-middleware-redirect
May 12, 2026
Merged

Fix prod-mode asserts, middleware exception routing, redirect GET guard#330
dereuromark merged 5 commits into
3.xfrom
bugfix-asserts-middleware-redirect

Conversation

@dereuromark
Copy link
Copy Markdown
Member

Three independent, low-blast-radius bug fixes for the 3.x line.

1. AuthorizationService — replace assert() with explicit throws

The three assert(...) calls in performCheck(), getCanHandler() and getScopeHandler() are compiled out when PHP runs with zend.assertions=-1, which is the standard production setting. Under that configuration:

  • A missing policy method silently falls through to the [$policy, $method](...) invocation and produces a generic Error: Call to undefined method ... instead of the documented MissingMethodException.
  • A policy method returning an unexpected type (not bool and not ResultInterface) is not caught and produces a downstream type error in can() / 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 the try/catch block, so any Authorization\Exception\Exception thrown from inside a RequestPolicy (a MissingMethodException, a custom MissingIdentityException raised by canAccess(), or any other policy-level failure) bypassed the configured unauthorizedHandler and bubbled out of the middleware unhandled. This is the exact issue surfaced in #150 (last comment by matteorebeschi explicitly identifies the asymmetry with AuthorizationMiddleware, which does wrap its check in a try block).

Moving canResult(...) inside the existing try block restores the symmetry. A regression test using the Suppress handler confirms a MissingMethodException is now routed through the handler instead of escaping.

3. CakeRedirectHandler — honor the GET-only guard

The 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 at src/Middleware/UnauthorizedHandler/CakeRedirectHandler.php dropped that guard and appended the query param for every method.

Added the missing guard and a data-provider test mirroring RedirectHandlerTest::testHandleRedirectionIgnoreNonIdempotentMethods across 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.

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.
@dereuromark dereuromark added this to the 3.x milestone May 12, 2026
dereuromark and others added 2 commits May 12, 2026 16:45
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.
@dereuromark dereuromark merged commit da9a193 into 3.x May 12, 2026
8 checks passed
@dereuromark dereuromark deleted the bugfix-asserts-middleware-redirect branch May 12, 2026 16:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants