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) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01At3V6SjwbMe6eMzVGW3Hs7
This commit is contained in:
2026-09-25 21:51:41 +02:00
co-authored by Claude Opus 5.5
parent 720d0891c4
commit 1df655a74e
4 changed files with 82 additions and 11 deletions
+6
View File
@@ -60,7 +60,13 @@ final class AbilityRegistrar
return Errors::forbidden('LIT MCP ist nur über den OAuth-geschützten MCP-Server nutzbar.');
}
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 {
+21 -9
View File
@@ -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;
// 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;
$header = $request->get_header('authorization');
self::$bearerToken = is_string($header) && str_starts_with($header, 'Bearer ')
? substr($header, 7)
: 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);
}
}
@@ -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.
+31
View File
@@ -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');