diff --git a/src/Abilities/AbilityRegistrar.php b/src/Abilities/AbilityRegistrar.php index 35a4626..9934bd5 100644 --- a/src/Abilities/AbilityRegistrar.php +++ b/src/Abilities/AbilityRegistrar.php @@ -63,7 +63,7 @@ final class AbilityRegistrar 'description' => $ability->description(), 'category' => self::CATEGORY, 'input_schema' => $ability->inputSchema(), - 'permission_callback' => static function (mixed $input = null) use ($ability): bool|WP_Error { + 'permission_callback' => static function (mixed $input = null) use ($ability, $guards): bool|WP_Error { if (! Access::currentUserMayUse()) { return Errors::forbidden('Dein Benutzerkonto ist für LIT MCP nicht freigegeben.'); } @@ -73,7 +73,12 @@ final class AbilityRegistrar } try { - return $ability->permission(is_array($input) ? $input : []); + $input = is_array($input) ? $input : []; + // Before permission(): a read tool whose loader breaks must answer with the guard's refusal + // (e.g. the Divi release lock), not with internal_error from its own permission check. + $refusal = self::guardRefusal($guards, $ability, $input); + + return $refusal ?? $ability->permission($input); } catch (Throwable $e) { Log::failure($ability->name(), $e); return Errors::internal(); @@ -81,14 +86,15 @@ final class AbilityRegistrar }, 'execute_callback' => static function (mixed $input = null) use ($ability, $guards): mixed { $input = is_array($input) ? $input : []; - foreach ($guards as $guard) { - $refusal = $guard->refusal($ability, $input); + + try { + // Second line of defence: the permission callback already asked the guards. A refusal there + // stops the call, so a write is logged as refused only once. + $refusal = self::guardRefusal($guards, $ability, $input); if ($refusal !== null) { return $refusal; } - } - try { $result = $ability->execute($input); foreach ($guards as $guard) { $result = $guard->annotate($ability, $result); @@ -125,6 +131,22 @@ final class AbilityRegistrar return $args; } + /** + * @param list $guards + * @param array $input + */ + private static function guardRefusal(array $guards, Ability $ability, array $input): ?WP_Error + { + foreach ($guards as $guard) { + $refusal = $guard->refusal($ability, $input); + if ($refusal !== null) { + return $refusal; + } + } + + return null; + } + /** * A partial write stays partial_write; otherwise the first ToolRefusal anywhere in the cause chain is the answer * (e.g. the Divi release lock, spec §5.5); a rolled-back write is write_failed; anything else internal_error. diff --git a/tests/Unit/Abilities/AbilityRegistrarTest.php b/tests/Unit/Abilities/AbilityRegistrarTest.php index 59bea30..f2128a3 100644 --- a/tests/Unit/Abilities/AbilityRegistrarTest.php +++ b/tests/Unit/Abilities/AbilityRegistrarTest.php @@ -393,6 +393,119 @@ final class AbilityRegistrarTest extends TestCase self::assertSame([['x' => 1]], $seen); } + private function throwingGuard(): AbilityGuard + { + return new class implements AbilityGuard { + public function refusal(Ability $ability, array $input): ?WP_Error + { + throw new RuntimeException('secret path /var/www/x.php'); + } + + public function annotate(Ability $ability, mixed $result): mixed + { + return $result; + } + + public function outputSchema(Ability $ability, ?array $schema): ?array + { + return $schema; + } + }; + } + + /** @param callable(): mixed $call */ + private function withErrorLog(callable $call): mixed + { + $log = (string) tempnam(sys_get_temp_dir(), 'lit-mcp-log'); + $previous = ini_set('error_log', $log); + + try { + return $call(); + } finally { + ini_set('error_log', (string) $previous); + unlink($log); + } + } + + public function testAGuardRefusalInThePermissionCallbackSkipsPermissionAndExecute(): void + { + Functions\when('current_user_can')->justReturn(true); + $seen = []; + $ability = $this->ability(false); + $ability->shouldNotReceive('permission'); + $ability->expects('execute')->never(); + $registrar = $this->guardedRegistrar($ability, [$this->guard('a', Errors::forbidden('Nein.'), $seen)]); + + $result = ($registrar->argsFor($ability)['permission_callback'])(['x' => 1]); + + self::assertToolError($result, 'forbidden'); + self::assertSame([['x' => 1]], $seen); + } + + public function testAReadToolWhosePermissionWouldThrowGetsTheGuardRefusalInstead(): void + { + Functions\when('current_user_can')->justReturn(true); + $ability = $this->ability(true); + $ability->shouldNotReceive('permission'); + $registrar = $this->guardedRegistrar($ability, [$this->guard('a', Errors::forbidden('Gesperrt.'))]); + + self::assertToolError(($registrar->argsFor($ability)['permission_callback'])([]), 'forbidden'); + } + + public function testAPassingGuardLeavesThePermissionToTheAbility(): void + { + Functions\when('current_user_can')->justReturn(true); + $ability = $this->ability(); + $ability->expects('permission')->with(['x' => 1])->andReturn(true); + $registrar = $this->guardedRegistrar($ability, [$this->guard('a')]); + + self::assertTrue(($registrar->argsFor($ability)['permission_callback'])(['x' => 1])); + } + + public function testAThrowingGuardIsAnInternalErrorInThePermissionCallback(): void + { + Functions\when('current_user_can')->justReturn(true); + $ability = $this->ability(false); + $ability->shouldNotReceive('permission'); + $ability->expects('execute')->never(); + $registrar = $this->guardedRegistrar($ability, [$this->throwingGuard()]); + + $result = $this->withErrorLog( + static fn (): mixed => ($registrar->argsFor($ability)['permission_callback'])([]) + ); + + $payload = self::assertToolError($result, 'internal_error'); + self::assertStringNotContainsString('/var/www', $result->get_error_message()); + self::assertIsArray($payload); + } + + public function testAThrowingGuardIsAnInternalErrorInTheExecuteCallbackAndNothingRuns(): void + { + $ability = $this->ability(false); + $ability->expects('execute')->never(); + $registrar = $this->guardedRegistrar($ability, [$this->throwingGuard()]); + + $result = $this->withErrorLog( + static fn (): mixed => ($registrar->argsFor($ability)['execute_callback'])([]) + ); + + self::assertToolError($result, 'internal_error'); + } + + public function testARealFeatureGateRefusesAReadToolInThePermissionCallback(): void + { + Functions\when('current_user_can')->justReturn(true); + DiviFeatureFixtures::lock(DiviFeature::PageRead); + $ability = $this->ability(true, [DiviFeature::PageRead]); + $ability->shouldNotReceive('permission'); + $ability->expects('execute')->never(); + + $result = ($this->diviRegistrar($ability)->argsFor($ability)['permission_callback'])(['post_id' => 7]); + + $payload = self::assertToolError($result, 'divi_untested'); + self::assertSame(['page-read'], array_column($payload['features'], 'id')); + } + public function testTheFirstRefusalWinsAndLaterGuardsAreNotAsked(): void { $seenSecond = [];