fix: apply required and additionalProperties to an empty object

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

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": [

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.

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

tests/SchemaValidatorTest.php

@@ -0,0 +1,91 @@
+<?php
+
+declare(strict_types=1);
+
+use coyshdigital\managerprotocol\SchemaValidator;
+
+/**
+ * The validator's own behaviour, rather than any one schema's rules.
+ *
+ * Every other schema test asks whether a particular document is accepted. This one asks whether the
+ * validator is capable of refusing at all - which is a different question, and the one that went
+ * unasked while `{}` validated against every published schema no matter how many fields it declared
+ * required.
+ *
+ * How that survived is worth keeping in front of whoever changes this next. The suite emptied objects
+ * by unsetting one key at a time, so every test payload had at least one key left in it and the empty
+ * case was never constructed. `json_decode('{}', true)` returns `[]`, `array_is_list([])` is true, and
+ * an empty object was therefore dispatched as an empty *array* - to a branch that returns immediately
+ * when the schema declares no `items`, and that has never heard of `required`.
+ */
+
+/**
+ * Every published schema that declares required fields, and one field each that must be missing from
+ * `{}`. Named individually rather than globbed so that adding a schema without adding it here is a
+ * visible omission rather than a silent gap in the sweep.
+ *
+ * @return array<string, array{0: string, 1: string}>
+ */
+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([]);
+});