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([]); +});