From 1df655a74e13ae1f029e54cabd6de5b4d3245142 Mon Sep 17 00:00:00 2001 From: Jan-Niclas Loosen Date: Fri, 25 Sep 2026 21:51:41 +0200 Subject: [PATCH] fix: fail closed on permission() exceptions, harden RequestContext flag reset - AbilityRegistrar: wrap permission_callback in try/catch(Throwable) like execute_callback, so a throwing permission() denies (Errors::internal()) instead of skipping rest_request_after_callbacks and leaving RequestContext's flag set for a later request in the same PHP worker. - RequestContext::recordRoute() now recomputes the flag (and bearer token) unconditionally on every matched REST request instead of only setting it true for our own route, so a stale true left behind by an earlier request is reset by the next non-matching one (fail closed, defense in depth). - RequestContext::recordRoute() matches the Authorization "Bearer" scheme case-insensitively. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01At3V6SjwbMe6eMzVGW3Hs7 --- src/Abilities/AbilityRegistrar.php | 8 ++++- src/Server/RequestContext.php | 32 +++++++++++++------ tests/Unit/Abilities/AbilityRegistrarTest.php | 22 +++++++++++++ tests/Unit/Server/RequestContextTest.php | 31 ++++++++++++++++++ 4 files changed, 82 insertions(+), 11 deletions(-) diff --git a/src/Abilities/AbilityRegistrar.php b/src/Abilities/AbilityRegistrar.php index 4e0ceba..015b2b0 100644 --- a/src/Abilities/AbilityRegistrar.php +++ b/src/Abilities/AbilityRegistrar.php @@ -60,7 +60,13 @@ final class AbilityRegistrar return Errors::forbidden('LIT MCP ist nur über den OAuth-geschützten MCP-Server nutzbar.'); } - return $ability->permission(is_array($input) ? $input : []); + try { + return $ability->permission(is_array($input) ? $input : []); + } catch (Throwable $e) { + $message = sprintf('[wp-lit-mcp] %s failed: %s', $ability->name(), $e); + error_log($message); // phpcs:ignore WordPress.PHP.DevelopmentFunctions.error_log_error_log + return Errors::internal(); + } }, 'execute_callback' => static function (mixed $input = null) use ($ability): mixed { try { diff --git a/src/Server/RequestContext.php b/src/Server/RequestContext.php index 39fc398..fc143ec 100644 --- a/src/Server/RequestContext.php +++ b/src/Server/RequestContext.php @@ -17,8 +17,11 @@ use WPMedia\MCP\OAuth\Auth\SecretManager; * `rest_request_before_callbacks` for every matched REST route before any * permission_callback runs and `rest_request_after_callbacks` right after the * route callback returned (WP_REST_Server::respond_to_request()). The flag is - * set in the first and cleared in the second, so persistent PHP workers or a - * later `rest_do_request()` in the same process never inherit it. + * recomputed (not just set) in the first for every REST request – our route or + * not – and cleared in the second, so persistent PHP workers or a later + * `rest_do_request()` in the same process never inherit a stale "true", even + * if `rest_request_after_callbacks` never fired for a previous OAuth server + * request (e.g. a fatal error mid-dispatch). */ final class RequestContext { @@ -39,14 +42,13 @@ final class RequestContext */ public static function recordRoute(mixed $response, array $handler, WP_REST_Request $request): mixed { - if ($request->get_route() === self::route()) { - self::$isOAuthServerRequest = true; - self::$clientName = null; - $header = $request->get_header('authorization'); - self::$bearerToken = is_string($header) && str_starts_with($header, 'Bearer ') - ? substr($header, 7) - : null; - } + // Recomputed on every matched REST request (not just ours) so a later, + // unrelated request in the same persistent PHP worker can never inherit a + // stale "true" left behind by an earlier OAuth server request whose + // rest_request_after_callbacks somehow never ran (fail closed). + self::$isOAuthServerRequest = $request->get_route() === self::route(); + self::$clientName = null; + self::$bearerToken = self::$isOAuthServerRequest ? self::bearerToken($request) : null; return $response; } @@ -120,4 +122,14 @@ final class RequestContext { return '/' . ServerRegistrar::NAMESPACE . '/' . ServerRegistrar::ROUTE; } + + private static function bearerToken(WP_REST_Request $request): ?string + { + $header = $request->get_header('authorization'); + if (! is_string($header) || stripos($header, 'Bearer ') !== 0) { + return null; + } + + return substr($header, 7); + } } diff --git a/tests/Unit/Abilities/AbilityRegistrarTest.php b/tests/Unit/Abilities/AbilityRegistrarTest.php index 3e22e6c..155db29 100644 --- a/tests/Unit/Abilities/AbilityRegistrarTest.php +++ b/tests/Unit/Abilities/AbilityRegistrarTest.php @@ -113,6 +113,28 @@ final class AbilityRegistrarTest extends TestCase self::assertSame(['ok' => true], $result); } + public function testPermissionExceptionBecomesNeutralInternalError(): void + { + Functions\when('current_user_can')->justReturn(true); + // error_log() is a PHP internal and cannot be stubbed; redirect it to a temp file instead. + $log = (string) tempnam(sys_get_temp_dir(), 'lit-mcp-log'); + $previous = ini_set('error_log', $log); + $ability = $this->ability(); + $ability->expects('permission')->andThrow(new RuntimeException('secret path /var/www/x.php')); + + try { + $result = ($this->registrarFor($ability)->argsFor($ability)['permission_callback'])([]); + } finally { + ini_set('error_log', (string) $previous); + } + + self::assertInstanceOf(WP_Error::class, $result); + self::assertSame('lit_internal_error', $result->get_error_code()); + self::assertStringNotContainsString('/var/www', $result->get_error_message()); + self::assertStringContainsString('lit/test failed', (string) file_get_contents($log)); + unlink($log); + } + public function testExceptionBecomesNeutralInternalError(): void { // error_log() is a PHP internal and cannot be stubbed; redirect it to a temp file instead. diff --git a/tests/Unit/Server/RequestContextTest.php b/tests/Unit/Server/RequestContextTest.php index d3cb7cb..a3b80b3 100644 --- a/tests/Unit/Server/RequestContextTest.php +++ b/tests/Unit/Server/RequestContextTest.php @@ -40,6 +40,18 @@ final class RequestContextTest extends TestCase self::assertFalse(RequestContext::isOAuthServerRequest()); } + public function testRecordRouteResetsAStaleFlagOnALaterNonMatchingRequest(): void + { + // Simulates rest_request_after_callbacks never having fired for a + // previous OAuth server request (e.g. a fatal error mid-dispatch), + // leaving the flag orphaned as true in a persistent PHP worker. + RequestContext::reset(true); + + RequestContext::recordRoute(null, [], new WP_REST_Request('/wp/v2/users/me')); + + self::assertFalse(RequestContext::isOAuthServerRequest()); + } + public function testRecordRouteReturnsResponseUnchanged(): void { $response = new stdClass(); @@ -87,6 +99,25 @@ final class RequestContextTest extends TestCase self::assertSame('https://claude.ai/oauth/mcp-oauth-client-metadata', RequestContext::clientName()); } + public function testClientNameMatchesTheBearerSchemeCaseInsensitively(): void + { + Functions\when('get_option')->justReturn('test-secret'); + $token = JWT::encode([ + 'sub' => '3', + 'app_pass_id' => 'uuid-unknown', + 'client_id' => 'https://claude.ai/oauth/mcp-oauth-client-metadata', + 'exp' => time() + 60, + ], 'test-secret'); + + RequestContext::recordRoute( + null, + [], + new WP_REST_Request(self::ROUTE, ['authorization' => 'bearer ' . $token]) + ); + + self::assertSame('https://claude.ai/oauth/mcp-oauth-client-metadata', RequestContext::clientName()); + } + public function testClientNameIsEmptyForAnInvalidToken(): void { Functions\when('get_option')->justReturn('test-secret');