From 0fbc538aed14dae66eefeeaab2fc839ff5521431 Mon Sep 17 00:00:00 2001 From: Coysh Date: Thu, 6 Aug 2026 06:56:55 +0100 Subject: [PATCH] fix: apply required and additionalProperties to an empty object SchemaValidator accepted `{}` against every published schema, however many fields that schema declared required. Confirmed against inventory.v1, system.v2, updates.v2, logins.v1 and backup.v3: all five returned no errors. json_decode('{}', true) returns [], and array_is_list([]) is true, so check() dispatched an empty object to checkArray() - which returns immediately when the schema declares no `items`. checkObject() holds both `required` and `additionalProperties`, and was never reached. matchesType() already carried this exact exception one screen down, where 'object' accepts $value === [], so the type gate passed first and the payload sailed through everything after it. Dispatch now consults the schema before PHP's shape: a node the schema declares as type `object` reaches checkObject() even when it arrives empty. A node with no declared type keeps the old behaviour, and an empty array against type `array` is still a list - so nothing that validated before stops validating, which the new test asserts directly. The effect downstream was not a lenient accept but a worse error. A payload the validator was supposed to refuse by name instead reached code that indexed a key which was not there: an inventory report of `{}` returned a 500 with a correlation id rather than a 422 naming the missing field, and a manifest whose encryption block had been emptied passed both the declaration schema and the manifest signature check before dying on an undefined key. It survived 368 tests because the suite empties an object by unsetting one key at a time, so every test payload kept at least one key and the fully-empty case was never constructed. tests/SchemaValidatorTest.php now covers the validator's own behaviour rather than any one schema's rules. No schema file was edited and no fixture was regenerated. The rules were already published; they were not being applied. --- CHANGELOG.md | 46 ++++++++++++++++++ composer.json | 2 +- src/Protocol.php | 2 +- src/SchemaValidator.php | 30 +++++++++--- tests/SchemaValidatorTest.php | 91 +++++++++++++++++++++++++++++++++++ 5 files changed, 163 insertions(+), 8 deletions(-) create mode 100644 tests/SchemaValidatorTest.php diff --git a/CHANGELOG.md b/CHANGELOG.md index c77af19..ecd692d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,52 @@ This package is the wire contract between the Manager platform and the connector A change here is a change to what a site may send, so every entry says what was added and - more usefully - what was deliberately left out of it. +## 1.7.0 + +`SchemaValidator` refused nothing at all when handed an empty JSON object. No schema changed; what +changed is that the schemas are now enforced in a case where they silently were not. + +### What was wrong + +`{}` validated against every published schema, however many fields that schema declared `required`. +Confirmed against `inventory.v1`, `system.v2`, `updates.v2`, `logins.v1` and `backup.v3`: all five +returned no errors at all. + +`json_decode('{}', true)` returns `[]`, and `array_is_list([])` is `true`, so `check()` dispatched an +empty object to `checkArray()` - which returns immediately when the schema declares no `items`. +`checkObject()` holds both `required` and `additionalProperties`, and was never reached. `matchesType()` +already carried the exception (`'object'` accepts `$value === []`), so the type gate passed first and +the payload sailed through everything after it. + +It reached production because the suite empties an object by unsetting one key at a time. Every test +payload therefore had at least one key left in it, and the fully-empty case was never constructed in +368 tests. + +The effect downstream was not a lenient accept but a worse error: a payload the validator was supposed +to refuse by name instead reached code that indexed a key which was not there. An inventory report of +`{}` returned a 500 with a correlation id rather than a 422 naming the missing field; a manifest whose +`encryption` block had been emptied passed both the declaration schema and the manifest signature check +before dying on an undefined key. + +### What changed + +- **Dispatch is decided by the schema first and by PHP's shape second.** A node the schema declares as + `type: object` goes to `checkObject()` even when it arrives empty. Everything else is unchanged: a + node with no declared `type` keeps the old behaviour, and an empty array against `type: array` is + still a list. +- **`tests/SchemaValidatorTest.php`** covers the validator's own behaviour rather than any one schema's + rules - every published schema refuses `{}`, a nested empty object is refused, an empty list is still + a list, and every valid fixture still validates unchanged. + +### What was deliberately left out + +No schema file was touched and no fixture was regenerated. This is a validator that was not applying +the rules already published, so the rules did not need to change - and a fixture that stopped +verifying would have meant a wire-format break, which this is not. + +Nothing was relaxed to accommodate existing callers. A connector sending `{}` was already broken; it +now learns so with a 422 that names the fields instead of a 500 that names nothing. + ## 1.6.0 `system.v2`. A volume now says where its files are, and - when it could not be measured - which of diff --git a/composer.json b/composer.json index f1e8195..d0a247f 100644 --- a/composer.json +++ b/composer.json @@ -1,7 +1,7 @@ { "name": "coysh-digital/manager-protocol", "description": "Wire contract shared by the Manager platform and the Manager Connector for Craft CMS.", - "version": "1.6.0", + "version": "1.7.0", "type": "library", "license": "MIT", "keywords": [ diff --git a/src/Protocol.php b/src/Protocol.php index 3a683eb..bd6faff 100644 --- a/src/Protocol.php +++ b/src/Protocol.php @@ -22,7 +22,7 @@ final class Protocol /** * @var string Package version. Independent of the platform and connector versions. */ - public const VERSION = '1.6.0'; + public const VERSION = '1.7.0'; /** * @var string Prefix on the canonical string a connector signs. diff --git a/src/SchemaValidator.php b/src/SchemaValidator.php index a32cbff..e34d5dd 100644 --- a/src/SchemaValidator.php +++ b/src/SchemaValidator.php @@ -137,12 +137,30 @@ private function check(mixed $value, array $schema, string $path, array &$errors } } - if (is_array($value) && array_is_list($value)) { - $this->checkArray($value, $schema, $path, $errors); - } - - if (is_array($value) && !array_is_list($value)) { - $this->checkObject($value, $schema, $path, $errors); + // Which branch an array takes is decided by the schema first and by PHP's shape second. + // + // `json_decode('{}', true)` returns `[]`, and `array_is_list([])` is true, so dispatching on + // shape alone sent an empty JSON object to checkArray() - which returns immediately when the + // schema declares no `items`. checkObject() holds `required` and `additionalProperties`, so + // neither ran, and `{}` validated against every published schema no matter how many fields + // it declared required. matchesType() already made exactly this exception one screen down + // ("object" accepts `$value === []`); this is the same exception, applied to the dispatch it + // was missing from. + // + // The tests never caught it because they empty an object by unsetting one key at a time and + // never all of them at once. + // + // A schema with no declared `type` keeps the old behaviour, so nothing that validates today + // starts failing: only a node the schema explicitly calls an object changes branch, and only + // when it arrived empty. + if (is_array($value)) { + $declaredObject = ($schema['type'] ?? null) === 'object'; + + if (!$declaredObject && array_is_list($value)) { + $this->checkArray($value, $schema, $path, $errors); + } else { + $this->checkObject($value, $schema, $path, $errors); + } } } diff --git a/tests/SchemaValidatorTest.php b/tests/SchemaValidatorTest.php new file mode 100644 index 0000000..f0611f8 --- /dev/null +++ b/tests/SchemaValidatorTest.php @@ -0,0 +1,91 @@ + + */ +dataset('schemas with required fields', [ + 'inventory.v1' => ['inventory.v1', 'collected_at'], + 'system.v1' => ['system.v1', 'collected_at'], + 'system.v2' => ['system.v2', 'collected_at'], + 'updates.v1' => ['updates.v1', 'checked_at'], + 'updates.v2' => ['updates.v2', 'checked_at'], + 'logins.v1' => ['logins.v1', 'failed_attempts'], + 'backup.v1' => ['backup.v1', 'artifact'], + 'backup.v2' => ['backup.v2', 'manifest_signature'], + 'backup.v3' => ['backup.v3', 'artifact_crc32c'], + 'backup-manifest.v2' => ['backup-manifest.v2', 'key_wrapping'], + 'backup-manifest.v3' => ['backup-manifest.v3', 'key_wrapping'], + 'backup-progress.v1' => ['backup-progress.v1', 'stage'], +]); + +it('refuses an empty object against a schema with required fields', function (string $schema, string $field): void { + $errors = SchemaValidator::forSchema($schema)->validate([]); + + expect($errors)->not->toBe([]) + ->and(implode(' ', $errors))->toContain($field); +})->with('schemas with required fields'); + +it('refuses an empty object nested inside a valid document', function (): void { + // The shape that matters most: a manifest whose `encryption` block has been emptied. It passed + // the signature check - the site signed it - and then died on an undefined key downstream rather + // than being refused here by name. + $manifest = fixture('backup-manifest.v3/valid.json'); + $manifest['encryption'] = []; + + $errors = SchemaValidator::forSchema('backup-manifest.v3')->validate($manifest); + + expect($errors)->not->toBe([]) + ->and(implode(' ', $errors))->toContain('encryption.'); +}); + +it('still refuses an unknown key on an object that is otherwise empty', function (): void { + // additionalProperties lives in the same branch as required, so it was skipped by the same bug + // whenever the object arrived carrying nothing the schema recognised. + $errors = SchemaValidator::forSchema('inventory.v1')->validate(['not_a_real_field' => 'x']); + + expect(implode(' ', $errors))->toContain('not a permitted field'); +}); + +it('still treats an empty list as a list where the schema says array', function (): void { + // The other half of the dispatch, and the reason this is keyed on the declared type rather than + // on emptiness: an empty array is a legitimate value for a list-typed field, and must not start + // being read as an object with every required field missing. + $report = fixture('inventory.v1/valid.json'); + $report['plugins'] = []; + + expect(SchemaValidator::forSchema('inventory.v1')->validate($report))->toBe([]); +}); + +it('accepts every valid fixture unchanged', function (): void { + // The regression guard for the fix itself. Changing which branch an array takes could only be + // safe if nothing that validated before stopped validating, and this asserts that directly + // rather than leaving it to the rest of the suite to notice. + expect(SchemaValidator::forSchema('inventory.v1')->validate(fixture('inventory.v1/valid.json')))->toBe([]) + ->and(SchemaValidator::forSchema('system.v2')->validate(fixture('system.v2/valid.json')))->toBe([]) + ->and(SchemaValidator::forSchema('updates.v2')->validate(fixture('updates.v2/valid.json')))->toBe([]) + ->and(SchemaValidator::forSchema('logins.v1')->validate(fixture('logins.v1/valid.json')))->toBe([]) + ->and(SchemaValidator::forSchema('backup-manifest.v3')->validate(fixture('backup-manifest.v3/valid.json')))->toBe([]); +});