Repository navigation
[Server][Capability] Bound what a schema can cost to validate (SEP-2106) - #436
Open
chr-hertel wants to merge 8 commits into
Open
chr-hertel wants to merge 8 commits into
chr-hertel wants to merge 8 commits into
Conversation
Copy link
Copy Markdown
Member
Sorry, something went wrong.
chr-hertel
requested review from
CodeWithKyrian,
Nyholm and
soyuka
as code owners
August 18, 2026 06:59
Copy link
Copy Markdown
Contributor
There was a problem hiding this comment.
Pull request overview
Introduces a schema “complexity guard” to preflight JSON Schemas used in capability discovery/tool argument validation, preventing unsafe $ref usage and bounding validation cost before opis/json-schema performs an expensive walk.
Changes:
- Add Mcp\Capability\Discovery\SchemaComplexityGuard to reject non-local $ref and structurally “ruinous” schemas (depth/subschema/property-map ceilings).
- Wire the guard into SchemaValidator by default (configurable via constructor), cap reported errors at 100, and improve messaging for unsupported $schema dialects.
- Add focused unit coverage for the guard and document the feature in CHANGELOG.md.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tests/Unit/Capability/Discovery/SchemaComplexityGuardTest.php | Adds unit tests covering external $ref rejection and complexity bounds behavior. |
| src/Capability/Discovery/SchemaValidator.php | Integrates the guard, caps/maxes errors, and improves unsupported dialect error reporting. |
| src/Capability/Discovery/SchemaComplexityGuard.php | New pre-validation guard implementing external $ref refusal and complexity estimation. |
| CHANGELOG.md | Documents the new guard, the error cap, and the improved dialect error message. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Sorry, something went wrong.
Comment thread
src/Capability/Discovery/SchemaComplexityGuard.php
Comment on lines
+68
to
+83
| public function check(array|object $schema): ?string | ||
| { | ||
| $root = self::toArray($schema); | ||
|
|
||
| if (null !== $reason = $this->findExternalRef($root, 0)) { | ||
| return $reason; | ||
| } | ||
|
|
||
| try { | ||
| $this->cost($root, $root, [], 0, new \stdClass()); | ||
| } catch (\OverflowException $e) { | ||
| return $e->getMessage(); | ||
| } | ||
|
|
||
| return null; | ||
| } |
Copy link
Copy Markdown
Member
Author
There was a problem hiding this comment.
Fixed in 0672eb9 — check() now catches \JsonException from toArray() and returns it as a refusal reason instead of leaking it.
Sorry, something went wrong.
Comment thread
src/Capability/Discovery/SchemaComplexityGuard.php
Comment on lines
+210
to
+214
| private static function resolve(string $pointer, array $root): ?array | ||
| { | ||
| if ('#' === $pointer || '' === $pointer) { | ||
| return $root; | ||
| } |
Copy link
Copy Markdown
Member
Author
There was a problem hiding this comment.
Fixed in 0672eb9 — dropped the unreachable '' === $pointer branch in resolve().
Sorry, something went wrong.
Refuses two shapes before opis/json-schema walks them (SEP-2106): a $ref naming anything outside the document, and a composition that expands past a subschema budget, a nesting depth, or a property-map size. The external $ref was already safe, but only by omission - the SDK registers no resolver, so it failed as an opaque "unresolved reference". The guard now states the rule up front. The composition bound was a real hole: sixteen nested two-branch anyOfs took 9.0s and 65536 error objects. Validator:: setMaxErrors() bounds the report, not the walk, so the guard is structural and runs first. The budget resolves same-document $refs, so the $defs- compressed form of the same bomb - a few hundred bytes on the wire - is caught along with the expanded one. Recursive schemas and long reference chains still pass. SchemaValidator also caps reported errors at 100, and reports an unsupported $schema dialect as such, naming it, instead of as an internal fault.
A chain of {"$ref": ...} nodes is meant to cost nothing regardless of
length, but cost()/refCost() resolved it by mutual recursion - one
native call frame per link. A chain long enough (~17-20k links, well
within default maxProperties/maxDepth combined across sibling maps)
exhausted the stack or its backing memory before the subschema budget
or depth ceiling ever got a chance to refuse it: the guard was
bypassable by the exact class of input it exists to stop.
refCost() now walks the chain in a loop at constant stack depth,
handing only the schema found at the end of it to cost() for its own
already depth-bounded recursion.
chr-hertel
force-pushed
the
pr/schema-complexity-guard
branch
from
bc3f595 to
616f142
Compare
August 29, 2026 13:26
A server killed by a signal writes nothing, so the conditional dump stayed silent on exactly the failure being chased. Drop with the commit below it.
Isolates whether the inspector SIGSEGV comes from the enlarged Opis error tree (setMaxErrors 100 vs its default 1) and collectSubErrors' unbounded recursion over it. Revert with the two diagnostic commits.
Restores MAX_REPORTED_ERRORS to 100 (the maxErrors=1 experiment came back negative) and marks each phase, so the last line before the SIGSEGV names which one dies. Drop with the other TEMP commits.
All eight CI segfaults die between guard:in and guard:out. This splits check() into toArray / findExternalRef / cost and dumps the schema it was handed, since the schema published by tools/list is accepted in 0.03ms locally. Drop with the other TEMP commits.
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Reviewers
At least 1 approving review is required to merge this pull request.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
New Mcp\Capability\Discovery\SchemaComplexityGuard, wired into SchemaValidator by default and configurable through its constructor, refuses two shapes before opis/json-schema walks them:
The budget resolves same-document $refs, so the $defs-compressed form of a composition bomb — a few hundred bytes on the wire, a million subschema evaluations to walk — is caught along with the expanded one. Recursive schemas and long reference chains still pass.
SchemaValidator also caps reported errors at 100, and reports an unsupported $schema dialect as such, naming the dialect, rather than as an opaque internal fault.
Part of SEP-2106.