From 6bbbd4aecacb62fead7d85727d345517b596cd69 Mon Sep 17 00:00:00 2001 From: Alex Standiford Date: Fri, 12 Jun 2026 08:32:30 -0400 Subject: [PATCH] fix(rest): make derived permission_callback idempotent and per-controller MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit WordPress invokes a route's permission callbacks more than once per request: once at dispatch, then again for EVERY method registered on the matched route while rest_send_allow_header builds the Allow header — all against the same WP_REST_Request instance. checkPermissions() re-ran the controller middleware chain on every invocation. Middleware is not idempotent (ConvertCsvMiddleware converts a CSV param to an array in place), so the second pass threw a TypeError (explode() on an array), which catch(Exception) did not catch — fataling the request after the route callback had already produced a correct response. Clients saw HTTP 200 with an empty body on every middleware- bearing GET endpoint. - Memoize middleware outcomes per (request, controller class) and return the stored answer on repeat invocations without re-running the chain. Per-controller keying matters: the Allow-header pass checks the POST controller against a GET-shaped request, and its validation failure must not clobber the GET controller's passing outcome. - Store auth WP_Errors in the same map so repeated checks return the same rejection without re-running auth middleware. - Catch \Throwable, not Exception: middleware engine errors surface as a 500 WP_Error instead of fataling the request (wrapped for LoggerStrategy::logException(), which takes Exception). Four new regression tests pin: single run across repeated checks, memoized auth rejection, per-controller outcome isolation, and TypeError-to-WP_Error conversion. --- lib/Strategies/RestStrategy.php | 55 +++++++-- .../RestStrategyPermissionsTest.php | 109 ++++++++++++++++++ 2 files changed, 154 insertions(+), 10 deletions(-) diff --git a/lib/Strategies/RestStrategy.php b/lib/Strategies/RestStrategy.php index 2cd2f76..f021924 100644 --- a/lib/Strategies/RestStrategy.php +++ b/lib/Strategies/RestStrategy.php @@ -45,11 +45,20 @@ class RestStrategy implements CoreRestStrategy /** * Requests whose middleware chain already ran in the permission - * callback, mapped to the middleware outcome (true, or the caught - * non-auth RestException to be re-thrown by the route callback so - * clients keep the rich legacy error shape). + * callback, mapped per controller class to the middleware outcome + * (true, the caught non-auth RestException to be re-thrown by the + * route callback so clients keep the rich legacy error shape, or + * the WP_Error returned for auth rejections). * - * @var WeakMap + * Keyed by controller class because WordPress invokes the permission + * callback of EVERY method registered on a matched route for the same + * WP_REST_Request (rest_send_allow_header does this to build the + * Allow header), and one controller's outcome must not clobber + * another's. Memoized so the chain runs at most once per controller + * per request — middleware is not required to be idempotent (e.g. + * CSV conversion mutates request params). + * + * @var WeakMap> */ private WeakMap $middlewareOutcomes; @@ -89,27 +98,52 @@ private function checkPermissions(Controller $controller, WP_REST_Request $wpReq return true; } + $outcomes = $this->middlewareOutcomes[$wpRequest] ?? []; + $controllerClass = get_class($controller); + + // Already ran for this controller — return the same answer without + // re-running the chain. WordPress calls this more than once per + // request (dispatch, then rest_send_allow_header for every method + // on the route), and middleware may not be idempotent. + if (array_key_exists($controllerClass, $outcomes)) { + $stored = $outcomes[$controllerClass]; + + return $stored instanceof WP_Error ? $stored : true; + } + $request = $this->resolveRequest($wpRequest); try { Arr::each($controller->getMiddleware($request), fn(Middleware $middleware) => $middleware->process($request)); - $this->middlewareOutcomes[$wpRequest] = true; + $outcomes[$controllerClass] = true; } catch (RestException $e) { if (in_array($e->getCode(), [401, 403], true)) { - return new WP_Error( + $error = new WP_Error( $e->getCode() === 401 ? 'rest_not_logged_in' : 'rest_forbidden', $e->getMessage(), ['status' => $e->getCode()] ); + $outcomes[$controllerClass] = $error; + $this->middlewareOutcomes[$wpRequest] = $outcomes; + + return $error; } - $this->middlewareOutcomes[$wpRequest] = $e; - } catch (Exception $e) { - $this->logger->logException($e); + $outcomes[$controllerClass] = $e; + } catch (\Throwable $e) { + // \Throwable, not Exception: a TypeError thrown by middleware + // must surface as a 500, not fatal the whole request before + // WordPress can serve a body. logException() takes Exception, + // so non-Exception throwables are wrapped. + $this->logger->logException( + $e instanceof Exception ? $e : new Exception($e->getMessage(), (int) $e->getCode(), $e) + ); return new WP_Error('rest_unexpected_error', 'An unexpected error occurred.', ['status' => 500]); } + $this->middlewareOutcomes[$wpRequest] = $outcomes; + return true; } @@ -121,7 +155,8 @@ private function checkPermissions(Controller $controller, WP_REST_Request $wpReq */ private function wrapCallback(Controller $controller, Request $request, ?WP_REST_Request $wpRequest = null): Response { - $outcome = $wpRequest !== null ? ($this->middlewareOutcomes[$wpRequest] ?? null) : null; + $outcomes = $wpRequest !== null ? ($this->middlewareOutcomes[$wpRequest] ?? []) : []; + $outcome = $outcomes[get_class($controller)] ?? null; if ($outcome instanceof RestException) { // Middleware already ran in the permission callback and failed diff --git a/tests/Unit/Strategies/RestStrategyPermissionsTest.php b/tests/Unit/Strategies/RestStrategyPermissionsTest.php index 8739d18..32b8734 100644 --- a/tests/Unit/Strategies/RestStrategyPermissionsTest.php +++ b/tests/Unit/Strategies/RestStrategyPermissionsTest.php @@ -138,4 +138,113 @@ public function process(Request $request): void $this->assertSame(1, $middleware->runs); } + + public function testRepeatedPermissionChecksRunMiddlewareOnlyOnce(): void + { + // WordPress invokes permission_callback again per method on the + // matched route while building the Allow header + // (rest_send_allow_header), so the chain must be memoized — + // middleware is not required to be idempotent. + $middleware = new class implements Middleware { + public int $runs = 0; + + public function process(Request $request): void + { + $this->runs++; + + if ($this->runs > 1) { + throw new \TypeError('non-idempotent middleware ran twice'); + } + } + }; + + $controller = $this->makeControllerWithMiddleware($middleware); + $strategy = $this->makeStrategy(); + $wpRequest = new WP_REST_Request(); + + $first = $this->checkPermissions($strategy, $controller, $wpRequest); + $second = $this->checkPermissions($strategy, $controller, $wpRequest); + + $this->assertTrue($first); + $this->assertTrue($second); + $this->assertSame(1, $middleware->runs); + } + + public function testRepeatedAuthRejectionReturnsSameWpErrorWithoutRerun(): void + { + $middleware = new class implements Middleware { + public int $runs = 0; + + public function process(Request $request): void + { + $this->runs++; + throw new RestException('Forbidden.', [], 403); + } + }; + + $controller = $this->makeControllerWithMiddleware($middleware); + $strategy = $this->makeStrategy(); + $wpRequest = new WP_REST_Request(); + + $first = $this->checkPermissions($strategy, $controller, $wpRequest); + $second = $this->checkPermissions($strategy, $controller, $wpRequest); + + $this->assertInstanceOf(WP_Error::class, $first); + $this->assertInstanceOf(WP_Error::class, $second); + $this->assertSame(1, $middleware->runs); + } + + public function testOutcomeIsKeyedPerControllerNotPerRequest(): void + { + // rest_send_allow_header checks EVERY method registered on the + // route against the same WP_REST_Request; one controller's failed + // middleware must not clobber another controller's passing outcome. + $passing = new class implements Middleware { + public function process(Request $request): void + { + } + }; + $failing = new class implements Middleware { + public function process(Request $request): void + { + throw new RestException('Validation failed.', [], 422); + } + }; + + $getController = $this->makeControllerWithMiddleware($passing); + $postController = $this->makeControllerWithMiddleware($failing); + $strategy = $this->makeStrategy(); + $wpRequest = new WP_REST_Request(); + + $this->assertTrue($this->checkPermissions($strategy, $getController, $wpRequest)); + $this->assertTrue($this->checkPermissions($strategy, $postController, $wpRequest)); + + // The GET controller's stored outcome must still be a clean pass: + // wrapCallback must NOT throw the POST controller's 422. + $wrap = new ReflectionMethod($strategy, 'wrapCallback'); + $resolve = new ReflectionMethod($strategy, 'resolveRequest'); + + try { + $wrap->invoke($strategy, $getController, $resolve->invoke($strategy, $wpRequest), $wpRequest); + $this->fail('Expected getResponse() sentinel exception.'); + } catch (RestException $e) { + $this->fail('GET controller inherited the POST controller\'s middleware failure.'); + } catch (Exception $e) { + $this->assertSame('not used', $e->getMessage()); + } + } + + public function testMiddlewareTypeErrorBecomesWpErrorInsteadOfFatal(): void + { + $middleware = new class implements Middleware { + public function process(Request $request): void + { + throw new \TypeError('explode(): Argument #2 ($string) must be of type string, array given'); + } + }; + + $result = $this->checkPermissions($this->makeStrategy(), $this->makeControllerWithMiddleware($middleware), new WP_REST_Request()); + + $this->assertInstanceOf(WP_Error::class, $result); + } }