From 0df91500f65d2ca795791719c5c627ab11c93830 Mon Sep 17 00:00:00 2001 From: Sylvester Damgaard Date: Tue, 30 Jun 2026 12:58:29 +0200 Subject: [PATCH] Fix confirmation token retry loop --- src/Auth/ConfirmationTokenManager.php | 199 +++++++++++++++--- src/Mcp/Tools/BaseRouter.php | 2 + .../Tools/Concerns/RequiresConfirmation.php | 7 +- .../Feature/ConfirmationActionsConfigTest.php | 117 ++++++++++ tests/Feature/McpJsonSchemaValidationTest.php | 38 ++++ .../Auth/ConfirmationTokenManagerTest.php | 146 ++++++++++++- 6 files changed, 471 insertions(+), 38 deletions(-) diff --git a/src/Auth/ConfirmationTokenManager.php b/src/Auth/ConfirmationTokenManager.php index 9bc5778..21bd261 100644 --- a/src/Auth/ConfirmationTokenManager.php +++ b/src/Auth/ConfirmationTokenManager.php @@ -4,6 +4,9 @@ namespace Cboxdk\StatamicMcp\Auth; +use Illuminate\Contracts\Encryption\DecryptException; +use Illuminate\Support\Facades\Crypt; + class ConfirmationTokenManager { /** @@ -14,11 +17,29 @@ class ConfirmationTokenManager public function generate(string $tool, array $arguments): string { $timestamp = time(); - $payload = $this->buildPayload($tool, $arguments, $timestamp); + $nonce = bin2hex(random_bytes(16)); + $arguments = $this->stripConfirmationToken($arguments); + $payload = $this->buildPayload($tool, $arguments, $timestamp, $nonce); $signature = hash_hmac('sha256', $payload, $this->getKey()); - return base64_encode($timestamp . '.' . $signature); + $encoded = json_encode([ + 'timestamp' => $timestamp, + 'nonce' => $nonce, + 'arguments' => $arguments, + 'signature' => $signature, + ], JSON_UNESCAPED_SLASHES | JSON_UNESCAPED_UNICODE); + + if ($encoded === false) { + throw new \InvalidArgumentException('Cannot encode confirmation token: ' . json_last_error_msg()); + } + + $compressed = gzdeflate($encoded, 9); + if ($compressed === false) { + throw new \InvalidArgumentException('Cannot compress confirmation token.'); + } + + return Crypt::encryptString(base64_encode($compressed)); } /** @@ -28,41 +49,37 @@ public function generate(string $tool, array $arguments): string */ public function validate(string $token, string $tool, array $arguments): bool { - if ($token === '') { - return false; - } - - $decoded = base64_decode($token, true); - if ($decoded === false) { + $parts = $this->parseToken($token); + if ($parts === null) { return false; } - $dotPos = strpos($decoded, '.'); - if ($dotPos === false) { + if ((time() - $parts['timestamp']) > $this->ttl()) { return false; } - $timestampStr = substr($decoded, 0, $dotPos); - $signature = substr($decoded, $dotPos + 1); - - if (! is_numeric($timestampStr) || $signature === '') { - return false; - } + // Rebuild payload and compare signatures + $expectedPayload = $this->buildPayload($tool, $parts['arguments'], $parts['timestamp'], $parts['nonce']); + $expectedSignature = hash_hmac('sha256', $expectedPayload, $this->getKey()); - $timestamp = (int) $timestampStr; + return hash_equals($expectedSignature, $parts['signature']) + && $this->canonicalArgumentsMatch($arguments, $parts['arguments']); + } - // Check expiry - /** @var int $ttl */ - $ttl = config('statamic.mcp.confirmation.ttl', 300); - if ((time() - $timestamp) > $ttl) { - return false; + /** + * Validate a confirmation token and return the originally confirmed arguments. + * + * @param array $arguments + * + * @return array|null + */ + public function validatedArguments(string $token, string $tool, array $arguments): ?array + { + if (! $this->validate($token, $tool, $arguments)) { + return null; } - // Rebuild payload and compare signatures - $expectedPayload = $this->buildPayload($tool, $arguments, $timestamp); - $expectedSignature = hash_hmac('sha256', $expectedPayload, $this->getKey()); - - return hash_equals($expectedSignature, $signature); + return $this->parseToken($token)['arguments'] ?? null; } /** @@ -86,21 +103,133 @@ public function isEnabled(): bool * * @param array $arguments */ - private function buildPayload(string $tool, array $arguments, int $timestamp): string + private function buildPayload(string $tool, array $arguments, int $timestamp, string $nonce): string { - // Strip the confirmation_token from arguments before canonicalizing - unset($arguments['confirmation_token']); + $arguments = $this->stripConfirmationToken($arguments); + $exact = json_encode($arguments, JSON_UNESCAPED_SLASHES | JSON_UNESCAPED_UNICODE); - // Sort keys for canonical ordering - ksort($arguments); + $canonicalArguments = $this->canonicalize($arguments); - $canonical = json_encode($arguments, JSON_UNESCAPED_SLASHES | JSON_UNESCAPED_UNICODE); + $canonical = json_encode($canonicalArguments, JSON_UNESCAPED_SLASHES | JSON_UNESCAPED_UNICODE); - if ($canonical === false) { + if ($canonical === false || $exact === false) { throw new \InvalidArgumentException('Cannot canonicalize arguments: ' . json_last_error_msg()); } - return $tool . '|' . $canonical . '|' . $timestamp; + return $tool . '|' . $canonical . '|' . $exact . '|' . $timestamp . '|' . $nonce; + } + + /** + * Strip the confirmation token from arguments before signing. + * + * @param array $arguments + * + * @return array + */ + private function stripConfirmationToken(array $arguments): array + { + unset($arguments['confirmation_token']); + + return $arguments; + } + + /** + * Recursively sort associative arrays while preserving list order. + * + * @param array $value + * + * @return array + */ + private function canonicalize(array $value): array + { + foreach ($value as $key => $item) { + if (is_array($item)) { + $value[$key] = $this->canonicalize($item); + } + } + + if (! array_is_list($value)) { + ksort($value); + } + + return $value; + } + + /** + * @param array $provided + * @param array $confirmed + */ + private function canonicalArgumentsMatch(array $provided, array $confirmed): bool + { + $provided = $this->canonicalize($this->stripConfirmationToken($provided)); + $confirmed = $this->canonicalize($this->stripConfirmationToken($confirmed)); + + return json_encode($provided, JSON_UNESCAPED_SLASHES | JSON_UNESCAPED_UNICODE) + === json_encode($confirmed, JSON_UNESCAPED_SLASHES | JSON_UNESCAPED_UNICODE); + } + + /** + * @return array{timestamp: int, nonce: string, arguments: array, signature: string}|null + */ + private function parseToken(string $token): ?array + { + if ($token === '') { + return null; + } + + try { + $encryptedPayload = Crypt::decryptString($token); + } catch (DecryptException) { + return null; + } + + $compressed = base64_decode($encryptedPayload, true); + if ($compressed === false) { + return null; + } + + $decoded = gzinflate($compressed); + if ($decoded === false) { + return null; + } + + $payload = json_decode($decoded, true); + if (! is_array($payload)) { + return null; + } + + $timestamp = $payload['timestamp'] ?? null; + $nonce = $payload['nonce'] ?? null; + $arguments = $payload['arguments'] ?? null; + $signature = $payload['signature'] ?? null; + + if (! is_int($timestamp) || ! is_string($nonce) || $nonce === '' || ! is_array($arguments) || ! is_string($signature) || $signature === '') { + return null; + } + + $confirmedArguments = []; + foreach ($arguments as $key => $value) { + if (! is_string($key)) { + return null; + } + + $confirmedArguments[$key] = $value; + } + + return [ + 'timestamp' => $timestamp, + 'nonce' => $nonce, + 'arguments' => $confirmedArguments, + 'signature' => $signature, + ]; + } + + private function ttl(): int + { + /** @var int $ttl */ + $ttl = config('statamic.mcp.confirmation.ttl', 300); + + return $ttl; } /** diff --git a/src/Mcp/Tools/BaseRouter.php b/src/Mcp/Tools/BaseRouter.php index 7b13e7c..4d2e61c 100644 --- a/src/Mcp/Tools/BaseRouter.php +++ b/src/Mcp/Tools/BaseRouter.php @@ -72,6 +72,8 @@ protected function defineSchema(JsonSchemaContract $schema): array 'resource_type' => JsonSchema::string() ->description('Resource subtype for routers that manage multiple resource kinds. See the specific tool description for valid values and required combinations with actions.') ->enum($types), + 'confirmation_token' => JsonSchema::string() + ->description('Optional token returned by a previous confirmation-required response. Provide it unchanged to confirm and execute gated actions.'), ]; } diff --git a/src/Mcp/Tools/Concerns/RequiresConfirmation.php b/src/Mcp/Tools/Concerns/RequiresConfirmation.php index f12e289..1cc2186 100644 --- a/src/Mcp/Tools/Concerns/RequiresConfirmation.php +++ b/src/Mcp/Tools/Concerns/RequiresConfirmation.php @@ -38,7 +38,7 @@ protected function requiresConfirmation(string $action): bool * * @return array|null */ - protected function handleConfirmation(string $action, array $arguments): ?array + protected function handleConfirmation(string $action, array &$arguments): ?array { // Skip if confirmation is not required for this action if (! $this->requiresConfirmation($action)) { @@ -62,7 +62,10 @@ protected function handleConfirmation(string $action, array $arguments): ?array $token = $arguments['confirmation_token'] ?? null; if (is_string($token) && $token !== '') { $toolName = $this->name(); - if ($manager->validate($token, $toolName, $arguments)) { + $confirmedArguments = $manager->validatedArguments($token, $toolName, $arguments); + if ($confirmedArguments !== null) { + $arguments = $confirmedArguments; + return null; // Token valid, proceed } diff --git a/tests/Feature/ConfirmationActionsConfigTest.php b/tests/Feature/ConfirmationActionsConfigTest.php index 3d117d6..9fe5486 100644 --- a/tests/Feature/ConfirmationActionsConfigTest.php +++ b/tests/Feature/ConfirmationActionsConfigTest.php @@ -15,6 +15,7 @@ ]); Config::set('statamic.mcp.confirmation.enabled', true); Config::set('statamic.mcp.confirmation.ttl', 300); + Config::set('statamic.mcp.security.force_web_mode', false); }); // --------------------------------------------------------------------------- @@ -75,6 +76,122 @@ expect(ConfirmationActionGate::gates('entries', 'publish'))->toBeTrue(); }); +it('accepts a returned confirmation token on the next gated call', function (): void { + Config::set('statamic.mcp.security.force_web_mode', true); + + $router = new class extends EntriesRouter + { + /** + * @param array $arguments + * + * @return array|null + */ + public function callHandleConfirmation(string $action, array &$arguments): ?array + { + return $this->handleConfirmation($action, $arguments); + } + }; + + $arguments = [ + 'action' => 'delete', + 'collection' => 'blog', + 'id' => 'entry-123', + ]; + + $firstResponse = $router->callHandleConfirmation('delete', $arguments); + + if (! is_array($firstResponse)) { + throw new RuntimeException('Expected confirmation response array.'); + } + + $responseData = $firstResponse['data'] ?? null; + if (! is_array($responseData)) { + throw new RuntimeException('Expected confirmation response data array.'); + } + + $confirmationToken = $responseData['confirmation_token'] ?? null; + if (! is_string($confirmationToken)) { + throw new RuntimeException('Expected confirmation token string.'); + } + if ($confirmationToken === '') { + throw new RuntimeException('Expected non-empty confirmation token.'); + } + + expect($firstResponse['success'])->toBeFalse() + ->and($responseData['requires_confirmation'] ?? null)->toBeTrue(); + + $confirmedArguments = array_merge($arguments, [ + 'confirmation_token' => $confirmationToken, + ]); + + expect($router->callHandleConfirmation('delete', $confirmedArguments))->toBeNull() + ->and($confirmedArguments)->toBe($arguments); +}); + +it('accepts reordered nested confirmation arguments but restores the confirmed payload', function (): void { + Config::set('statamic.mcp.security.force_web_mode', true); + Config::set('statamic.mcp.confirmation.actions.entries', ['update']); + + $router = new class extends EntriesRouter + { + /** + * @param array $arguments + * + * @return array|null + */ + public function callHandleConfirmation(string $action, array &$arguments): ?array + { + return $this->handleConfirmation($action, $arguments); + } + }; + + $arguments = [ + 'action' => 'update', + 'collection' => 'blog', + 'id' => 'entry-123', + 'data' => [ + 'title' => 'About', + 'seo' => [ + 'description' => 'About page', + 'title' => 'About us', + ], + ], + ]; + + $firstResponse = $router->callHandleConfirmation('update', $arguments); + + if (! is_array($firstResponse)) { + throw new RuntimeException('Expected confirmation response array.'); + } + + $responseData = $firstResponse['data'] ?? null; + if (! is_array($responseData)) { + throw new RuntimeException('Expected confirmation response data array.'); + } + + $confirmationToken = $responseData['confirmation_token'] ?? null; + if (! is_string($confirmationToken) || $confirmationToken === '') { + throw new RuntimeException('Expected non-empty confirmation token string.'); + } + + $confirmedArguments = [ + 'data' => [ + 'seo' => [ + 'title' => 'About us', + 'description' => 'About page', + ], + 'title' => 'About', + ], + 'id' => 'entry-123', + 'collection' => 'blog', + 'action' => 'update', + 'confirmation_token' => $confirmationToken, + ]; + + expect($router->callHandleConfirmation('update', $confirmedArguments))->toBeNull() + ->and($confirmedArguments)->toBe($arguments); +}); + // --------------------------------------------------------------------------- // handleConfirmation() short-circuits (env + CLI) // --------------------------------------------------------------------------- diff --git a/tests/Feature/McpJsonSchemaValidationTest.php b/tests/Feature/McpJsonSchemaValidationTest.php index c2f3f8c..bee304b 100644 --- a/tests/Feature/McpJsonSchemaValidationTest.php +++ b/tests/Feature/McpJsonSchemaValidationTest.php @@ -3,6 +3,7 @@ declare(strict_types=1); use Cboxdk\StatamicMcp\Mcp\Servers\StatamicMcpServer; +use Cboxdk\StatamicMcp\Mcp\Tools\BaseRouter; use Cboxdk\StatamicMcp\Tests\TestCase; use Laravel\Mcp\Server\Contracts\Transport; use Mockery\MockInterface; @@ -215,6 +216,43 @@ public function test_required_parameters_exist_in_properties(): void } } + public function test_router_tools_accept_confirmation_token_parameter(): void + { + $tools = $this->getServerTools(); + + foreach ($tools as $toolClass) { + $tool = app($toolClass); + + if (! $tool instanceof BaseRouter) { + continue; + } + + $toolArray = $tool->toArray(); + $schema = $toolArray['inputSchema'] ?? null; + if (! is_array($schema)) { + throw new RuntimeException("Router tool {$toolClass} must expose an input schema array"); + } + + $properties = $schema['properties'] ?? null; + if (! is_array($properties)) { + throw new RuntimeException("Router tool {$toolClass} must expose schema properties"); + } + + $this->assertArrayHasKey( + 'confirmation_token', + $properties, + "Router tool {$toolClass} must expose confirmation_token so MCP clients can resubmit gated actions", + ); + + $confirmationTokenSchema = $properties['confirmation_token'] ?? null; + if (! is_array($confirmationTokenSchema)) { + throw new RuntimeException("Router tool {$toolClass} confirmation_token schema must be an array"); + } + + $this->assertSame('string', $confirmationTokenSchema['type'] ?? null); + } + } + public function test_tool_names_follow_mcp_naming_convention(): void { $tools = $this->getServerTools(); diff --git a/tests/Unit/Auth/ConfirmationTokenManagerTest.php b/tests/Unit/Auth/ConfirmationTokenManagerTest.php index 574e114..8e204c4 100644 --- a/tests/Unit/Auth/ConfirmationTokenManagerTest.php +++ b/tests/Unit/Auth/ConfirmationTokenManagerTest.php @@ -14,7 +14,7 @@ // Generation // --------------------------------------------------------------------------- -it('generates a non-empty base64url token', function (): void { +it('generates a non-empty encrypted token', function (): void { $manager = new ConfirmationTokenManager; $token = $manager->generate('statamic-entries', ['action' => 'delete', 'handle' => 'about']); @@ -40,6 +40,58 @@ expect($token1)->not->toBe($token2); }); +it('generates different tokens for the same canonical arguments', function (): void { + $manager = new ConfirmationTokenManager; + + $args1 = [ + 'action' => 'update', + 'handle' => 'article', + 'data' => [ + 'title' => 'About', + 'seo' => [ + 'description' => 'About page', + 'title' => 'About us', + ], + ], + ]; + $args2 = [ + 'data' => [ + 'seo' => [ + 'title' => 'About us', + 'description' => 'About page', + ], + 'title' => 'About', + ], + 'handle' => 'article', + 'action' => 'update', + ]; + + $token1 = $manager->generate('statamic-entries', $args1); + $token2 = $manager->generate('statamic-entries', $args2); + + expect($token1)->not->toBe($token2) + ->and($manager->validatedArguments($token1, 'statamic-entries', array_merge($args1, [ + 'confirmation_token' => $token1, + ])))->toBe($args1) + ->and($manager->validatedArguments($token2, 'statamic-entries', array_merge($args2, [ + 'confirmation_token' => $token2, + ])))->toBe($args2); +}); + +it('keeps large confirmation tokens compact enough for the response envelope', function (): void { + $manager = new ConfirmationTokenManager; + + $token = $manager->generate('statamic-entries', [ + 'action' => 'update', + 'handle' => 'article', + 'data' => [ + 'body' => str_repeat('Long repeated content. ', 3000), + ], + ]); + + expect(strlen($token))->toBeLessThan(100000); +}); + // --------------------------------------------------------------------------- // Validation — success // --------------------------------------------------------------------------- @@ -77,6 +129,98 @@ expect($manager->validate($token, 'statamic-entries', $args2))->toBeTrue(); }); +it('validates with nested associative arguments in different key order', function (): void { + $manager = new ConfirmationTokenManager; + + $args1 = [ + 'action' => 'update', + 'handle' => 'article', + 'data' => [ + 'title' => 'About', + 'seo' => [ + 'description' => 'About page', + 'title' => 'About us', + ], + ], + ]; + $token = $manager->generate('statamic-entries', $args1); + + $args2 = [ + 'data' => [ + 'seo' => [ + 'title' => 'About us', + 'description' => 'About page', + ], + 'title' => 'About', + ], + 'handle' => 'article', + 'action' => 'update', + ]; + + expect($manager->validate($token, 'statamic-entries', $args2))->toBeTrue(); +}); + +it('returns the originally confirmed arguments after nested associative reordering', function (): void { + $manager = new ConfirmationTokenManager; + + $args1 = [ + 'action' => 'update', + 'handle' => 'article', + 'data' => [ + 'title' => 'About', + 'seo' => [ + 'description' => 'About page', + 'title' => 'About us', + ], + ], + ]; + $token = $manager->generate('statamic-entries', $args1); + + $args2 = [ + 'data' => [ + 'seo' => [ + 'title' => 'About us', + 'description' => 'About page', + ], + 'title' => 'About', + ], + 'handle' => 'article', + 'action' => 'update', + 'confirmation_token' => $token, + ]; + + expect($manager->validatedArguments($token, 'statamic-entries', $args2))->toBe($args1); +}); + +it('rejects reordered list arguments', function (): void { + $manager = new ConfirmationTokenManager; + + $args = [ + 'action' => 'update', + 'handle' => 'article', + 'data' => [ + 'sections' => [ + ['type' => 'text', 'content' => 'First'], + ['type' => 'text', 'content' => 'Second'], + ], + ], + ]; + $token = $manager->generate('statamic-entries', $args); + + $reordered = [ + 'action' => 'update', + 'handle' => 'article', + 'data' => [ + 'sections' => [ + ['type' => 'text', 'content' => 'Second'], + ['type' => 'text', 'content' => 'First'], + ], + ], + ]; + + expect($manager->validate($token, 'statamic-entries', $reordered))->toBeFalse(); +}); + // --------------------------------------------------------------------------- // Validation — failure // ---------------------------------------------------------------------------