[Schema] Refuse an invalid x-mcp-header annotation at definition time (#441)
* [Schema] Refuse an invalid x-mcp-header annotation at definition time SEP-2243 calls a tool definition invalid when its x-mcp-header annotation is empty, is not an HTTP field name, collides case-insensitively with another, or sits on a number, array or object property - none of which can be mirrored onto a header. Nothing said so. The earliest place to is where the tool is defined, rather than on the request that trips over it. * Clarify x-mcp-header validation only reaches properties * Make checkHeaderAnnotations() a private instance method * Make headerAnnotations() a private instance method * Apply suggestion from @chr-hertel
This commit is contained in:
Christopher Hertel
GitHub
parent
cf2c4d4ead
commit
605df8771f
2 changed files with 251 additions and 0 deletions
|
|
@ -125,6 +125,97 @@ class Tool implements \JsonSerializable
|
||||||
// sub-schemas — not only SchemaGenerator / fromArray.
|
// sub-schemas — not only SchemaGenerator / fromArray.
|
||||||
$this->inputSchema = self::normalizeSchema($inputSchema);
|
$this->inputSchema = self::normalizeSchema($inputSchema);
|
||||||
$this->outputSchema = null !== $outputSchema ? self::normalizeSchema($outputSchema) : null;
|
$this->outputSchema = null !== $outputSchema ? self::normalizeSchema($outputSchema) : null;
|
||||||
|
|
||||||
|
// An out-of-bounds `x-mcp-header` reachable through `properties` makes
|
||||||
|
// the whole tool definition invalid, so it is refused where the tool
|
||||||
|
// is defined rather than discovered when a header comparison
|
||||||
|
// mysteriously fails.
|
||||||
|
if (null !== $reason = $this->checkHeaderAnnotations()) {
|
||||||
|
throw new InvalidArgumentException(\sprintf('Tool "%s" has an invalid "x-mcp-header" annotation: %s', $this->name, $reason));
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Validates every `x-mcp-header` annotation reachable through `properties`
|
||||||
|
* in an input schema (SEP-2243).
|
||||||
|
*
|
||||||
|
* The value becomes an HTTP field name, so it has to be one; it has to be
|
||||||
|
* unique case-insensitively, or two arguments would fight over one header;
|
||||||
|
* and it may only sit on a primitive that is not `number`, because a float
|
||||||
|
* has no single decimal spelling for a receiver to compare against.
|
||||||
|
*
|
||||||
|
* @return string|null the reason it is invalid, or null when every annotation is well-formed
|
||||||
|
*/
|
||||||
|
private function checkHeaderAnnotations(): ?string
|
||||||
|
{
|
||||||
|
$seen = [];
|
||||||
|
|
||||||
|
foreach ($this->headerAnnotations($this->inputSchema) as [$name, $type, $path]) {
|
||||||
|
if ('' === $name) {
|
||||||
|
return \sprintf('the annotation at "%s" is empty', $path);
|
||||||
|
}
|
||||||
|
|
||||||
|
// RFC 9110 tchar; excludes CR, LF and every other control character.
|
||||||
|
if (1 !== preg_match('/^[!#$%&\'*+\-.^_`|~0-9A-Za-z]+$/', $name)) {
|
||||||
|
return \sprintf('"%s" is not a valid HTTP field name', $name);
|
||||||
|
}
|
||||||
|
|
||||||
|
$folded = strtolower($name);
|
||||||
|
if (isset($seen[$folded])) {
|
||||||
|
return \sprintf('"%s" is declared twice, at "%s" and "%s"', $name, $seen[$folded], $path);
|
||||||
|
}
|
||||||
|
$seen[$folded] = $path;
|
||||||
|
|
||||||
|
if ('number' === $type) {
|
||||||
|
return \sprintf('"%s" is on a "number" property ("%s"), which cannot be mirrored', $name, $path);
|
||||||
|
}
|
||||||
|
|
||||||
|
if (null !== $type && !\in_array($type, ['string', 'integer', 'boolean'], true)) {
|
||||||
|
return \sprintf('"%s" is on a "%s" property ("%s"); only string, integer and boolean can be mirrored', $name, $type, $path);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
return null;
|
||||||
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Every annotation reachable through `properties` alone, as name, declared
|
||||||
|
* type and dotted path.
|
||||||
|
*
|
||||||
|
* @param array<string, mixed> $schema
|
||||||
|
*
|
||||||
|
* @return list<array{string, ?string, string}>
|
||||||
|
*/
|
||||||
|
private function headerAnnotations(array $schema, string $prefix = ''): array
|
||||||
|
{
|
||||||
|
$properties = $schema['properties'] ?? null;
|
||||||
|
|
||||||
|
if (!\is_array($properties)) {
|
||||||
|
return [];
|
||||||
|
}
|
||||||
|
|
||||||
|
$found = [];
|
||||||
|
|
||||||
|
foreach ($properties as $property => $definition) {
|
||||||
|
if (!\is_array($definition)) {
|
||||||
|
continue;
|
||||||
|
}
|
||||||
|
|
||||||
|
$path = '' === $prefix ? (string) $property : $prefix.'.'.$property;
|
||||||
|
$annotation = $definition['x-mcp-header'] ?? null;
|
||||||
|
|
||||||
|
if (null !== $annotation) {
|
||||||
|
if (!\is_string($annotation)) {
|
||||||
|
$found[] = ['', null, $path];
|
||||||
|
} else {
|
||||||
|
$found[] = [$annotation, \is_string($definition['type'] ?? null) ? $definition['type'] : null, $path];
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
$found = [...$found, ...$this->headerAnnotations($definition, $path)];
|
||||||
|
}
|
||||||
|
|
||||||
|
return $found;
|
||||||
}
|
}
|
||||||
|
|
||||||
/**
|
/**
|
||||||
|
|
|
||||||
160
tests/Unit/Schema/ToolHeaderAnnotationTest.php
Normal file
160
tests/Unit/Schema/ToolHeaderAnnotationTest.php
Normal file
|
|
@ -0,0 +1,160 @@
|
||||||
|
<?php
|
||||||
|
|
||||||
|
/*
|
||||||
|
* This file is part of the official PHP MCP SDK.
|
||||||
|
*
|
||||||
|
* A collaboration between Symfony and the PHP Foundation.
|
||||||
|
*
|
||||||
|
* For the full copyright and license information, please view the LICENSE
|
||||||
|
* file that was distributed with this source code.
|
||||||
|
*/
|
||||||
|
|
||||||
|
namespace Mcp\Tests\Unit\Schema;
|
||||||
|
|
||||||
|
use Mcp\Exception\InvalidArgumentException;
|
||||||
|
use Mcp\Schema\Tool;
|
||||||
|
use PHPUnit\Framework\Attributes\DataProvider;
|
||||||
|
use PHPUnit\Framework\Attributes\TestDox;
|
||||||
|
use PHPUnit\Framework\TestCase;
|
||||||
|
|
||||||
|
class ToolHeaderAnnotationTest extends TestCase
|
||||||
|
{
|
||||||
|
/**
|
||||||
|
* @param array<string, mixed> $properties
|
||||||
|
*/
|
||||||
|
private static function tool(array $properties): Tool
|
||||||
|
{
|
||||||
|
return new Tool(
|
||||||
|
name: 'a_tool',
|
||||||
|
title: null,
|
||||||
|
inputSchema: ['type' => 'object', 'properties' => $properties, 'required' => null],
|
||||||
|
description: 'x',
|
||||||
|
annotations: null,
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
|
#[TestDox('a well-formed annotation is accepted')]
|
||||||
|
public function testValidAnnotationIsAccepted(): void
|
||||||
|
{
|
||||||
|
$tool = self::tool([
|
||||||
|
'region' => ['type' => 'string', 'x-mcp-header' => 'Region'],
|
||||||
|
'retries' => ['type' => 'integer', 'x-mcp-header' => 'Retries'],
|
||||||
|
'dry_run' => ['type' => 'boolean', 'x-mcp-header' => 'Dry-Run'],
|
||||||
|
'query' => ['type' => 'string'],
|
||||||
|
]);
|
||||||
|
|
||||||
|
$this->assertSame('a_tool', $tool->name);
|
||||||
|
}
|
||||||
|
|
||||||
|
#[TestDox('an annotation on a nested property is accepted: the chain is all properties')]
|
||||||
|
public function testNestedAnnotationIsAccepted(): void
|
||||||
|
{
|
||||||
|
$tool = self::tool([
|
||||||
|
'target' => [
|
||||||
|
'type' => 'object',
|
||||||
|
'properties' => ['region' => ['type' => 'string', 'x-mcp-header' => 'Region']],
|
||||||
|
],
|
||||||
|
]);
|
||||||
|
|
||||||
|
$this->assertSame('a_tool', $tool->name);
|
||||||
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* @return iterable<string, array{array<string, mixed>, string}>
|
||||||
|
*/
|
||||||
|
public static function invalidAnnotations(): iterable
|
||||||
|
{
|
||||||
|
yield 'empty name' => [
|
||||||
|
['a' => ['type' => 'string', 'x-mcp-header' => '']],
|
||||||
|
'is empty',
|
||||||
|
];
|
||||||
|
|
||||||
|
yield 'name with a space' => [
|
||||||
|
['a' => ['type' => 'string', 'x-mcp-header' => 'My Header']],
|
||||||
|
'not a valid HTTP field name',
|
||||||
|
];
|
||||||
|
|
||||||
|
yield 'name with a newline' => [
|
||||||
|
['a' => ['type' => 'string', 'x-mcp-header' => "X\nInjected: yes"]],
|
||||||
|
'not a valid HTTP field name',
|
||||||
|
];
|
||||||
|
|
||||||
|
yield 'name with a carriage return' => [
|
||||||
|
['a' => ['type' => 'string', 'x-mcp-header' => "X\rY"]],
|
||||||
|
'not a valid HTTP field name',
|
||||||
|
];
|
||||||
|
|
||||||
|
yield 'name with a colon' => [
|
||||||
|
['a' => ['type' => 'string', 'x-mcp-header' => 'X:Y']],
|
||||||
|
'not a valid HTTP field name',
|
||||||
|
];
|
||||||
|
|
||||||
|
yield 'duplicate, differing only in case' => [
|
||||||
|
[
|
||||||
|
'a' => ['type' => 'string', 'x-mcp-header' => 'Region'],
|
||||||
|
'b' => ['type' => 'string', 'x-mcp-header' => 'region'],
|
||||||
|
],
|
||||||
|
'declared twice',
|
||||||
|
];
|
||||||
|
|
||||||
|
yield 'on a number' => [
|
||||||
|
['a' => ['type' => 'number', 'x-mcp-header' => 'Amount']],
|
||||||
|
'cannot be mirrored',
|
||||||
|
];
|
||||||
|
|
||||||
|
yield 'on an array' => [
|
||||||
|
['a' => ['type' => 'array', 'items' => ['type' => 'string'], 'x-mcp-header' => 'Tags']],
|
||||||
|
'only string, integer and boolean',
|
||||||
|
];
|
||||||
|
|
||||||
|
yield 'on an object' => [
|
||||||
|
['a' => ['type' => 'object', 'x-mcp-header' => 'Blob']],
|
||||||
|
'only string, integer and boolean',
|
||||||
|
];
|
||||||
|
|
||||||
|
yield 'non-string annotation value' => [
|
||||||
|
['a' => ['type' => 'string', 'x-mcp-header' => 42]],
|
||||||
|
'is empty',
|
||||||
|
];
|
||||||
|
|
||||||
|
yield 'duplicate across nesting levels' => [
|
||||||
|
[
|
||||||
|
'a' => ['type' => 'string', 'x-mcp-header' => 'Region'],
|
||||||
|
'nested' => [
|
||||||
|
'type' => 'object',
|
||||||
|
'properties' => ['b' => ['type' => 'string', 'x-mcp-header' => 'Region']],
|
||||||
|
],
|
||||||
|
],
|
||||||
|
'declared twice',
|
||||||
|
];
|
||||||
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* @param array<string, mixed> $properties
|
||||||
|
*/
|
||||||
|
#[DataProvider('invalidAnnotations')]
|
||||||
|
#[TestDox('an out-of-bounds annotation makes the tool definition invalid')]
|
||||||
|
public function testInvalidAnnotationIsRefused(array $properties, string $reason): void
|
||||||
|
{
|
||||||
|
$this->expectException(InvalidArgumentException::class);
|
||||||
|
$this->expectExceptionMessageMatches('/'.preg_quote($reason, '/').'/');
|
||||||
|
|
||||||
|
self::tool($properties);
|
||||||
|
}
|
||||||
|
|
||||||
|
#[TestDox('an annotation the walk cannot reach statically is simply not seen')]
|
||||||
|
public function testUnreachableAnnotationIsIgnored(): void
|
||||||
|
{
|
||||||
|
// Under `items`, so it is not reachable through `properties` alone.
|
||||||
|
// The spec calls such a definition invalid; this SDK does not mirror
|
||||||
|
// what it cannot reach, and does not pretend the annotation exists.
|
||||||
|
$tool = self::tool([
|
||||||
|
'tags' => [
|
||||||
|
'type' => 'array',
|
||||||
|
'items' => ['type' => 'string', 'x-mcp-header' => 'Bad Name With Spaces'],
|
||||||
|
],
|
||||||
|
]);
|
||||||
|
|
||||||
|
$this->assertSame('a_tool', $tool->name);
|
||||||
|
}
|
||||||
|
}
|
||||||
Loading…
Reference in a new issue