diff --git a/lib/Controller/StoreController.php b/lib/Controller/StoreController.php index 28c13599f..3bccd9aae 100644 --- a/lib/Controller/StoreController.php +++ b/lib/Controller/StoreController.php @@ -338,7 +338,10 @@ private function installComponents(array $components): array { $configKey = SchemaSlugMap::SLUG_TO_CONFIG_KEY[$slug]; try { - $this->registry->save(schemaConfigKey: $configKey, data: $object); + $this->registry->save( + schemaConfigKey: $configKey, + data: $this->asNewObject(object: $object) + ); $report[] = ['schema' => $slug, 'status' => 'installed', 'message' => '']; } catch (Throwable $e) { $failed = true; @@ -357,6 +360,39 @@ private function installComponents(array $components): array { return ['success' => ($failed === false && $report !== []), 'components' => $report]; }//end installComponents() + /** + * Strip every identity the remote payload carries, so an install CREATES. + * + * 🔴 WITHOUT THIS, "Install" IS AN OVERWRITE PRIMITIVE. + * + * OpenRegister's `saveObject()` resolves the object it is writing from the + * payload itself: `extractUuidAndNormalizeObject()` reads + * `$object['@self']['id'] ?? $object['id']` and treats a match as the uuid + * to UPDATE. So a store item whose component carried the uuid of this + * municipality's live case type would replace it rather than add one — and + * the write is PUT-semantic, so keys the payload omits are nulled, not left + * alone. The case type would not merely change, it would be gutted. + * + * The schema allowlist does not cover this. It governs WHICH schema a + * component may write, never whether the write creates or replaces, so an + * entirely legitimate `caseType` component is the attack. + * + * Identity is not the registry's to supply. An installed item is a NEW + * local object, and if install ever needs to be idempotent it must key on + * something dossiq controls rather than on a remote id. + * + * @param array $object The component's object. + * + * @return array The object with every identity key removed. + * + * @spec openspec/changes/dossiq-store-surface/specs/dossiq-store-surface/spec.md + */ + private function asNewObject(array $object): array { + unset($object['id'], $object['uuid'], $object['@self']); + + return $object; + }//end asNewObject() + /** * Read the registry connection. * diff --git a/openspec/changes/dossiq-store-surface/specs/dossiq-store-surface/spec.md b/openspec/changes/dossiq-store-surface/specs/dossiq-store-surface/spec.md index 5bee62c35..351c3eb94 100644 --- a/openspec/changes/dossiq-store-surface/specs/dossiq-store-surface/spec.md +++ b/openspec/changes/dossiq-store-surface/specs/dossiq-store-surface/spec.md @@ -148,3 +148,31 @@ icon, in the footer section between Documentation and Reports. - **THEN** the entry labelled `Store` MUST declare `icon: "StoreOutline"` - **AND** it MUST sit in the `footer` section with an order between Documentation and Reports + +### Requirement: REQ-DSS-007 An install creates, and can never replace + +The install action SHALL strip every identity key the remote payload carries +(`id`, `uuid`, `@self`) before writing, so an installed component is always a +NEW local object. + +OpenRegister resolves the object it writes from the payload itself: `saveObject` +reads `@self.id` first and `id` second, and treats a match as the uuid to +UPDATE. The write is PUT-semantic, so keys the payload omits are nulled rather +than left alone. A store item carrying the uuid of a live case type would +therefore not merely change it, it would gut it. + +The schema allowlist does NOT cover this. It governs which schema a component +may write, never whether the write creates or replaces, so a component naming a +perfectly legitimate configuration schema is the attack. + +Identity is not a remote registry's to supply. If install ever needs to be +idempotent it SHALL key on something dossiq controls. + +@e2e exclude Server-side, and the property is the ABSENCE of an addressed object. A browser sees an install succeed either way; only the payload handed to the write reveals which object it addressed. Proven by StoreControllerTest with a negative control: removing the strip makes the assertion fail. + +#### Scenario: A component carrying an id installs as a new object + +- **GIVEN** a store item whose component object carries `id`, `uuid` and `@self` +- **WHEN** it is installed +- **THEN** none of those keys MUST reach the write +- **AND** the rest of the component MUST still install diff --git a/tests/Unit/Controller/StoreControllerTest.php b/tests/Unit/Controller/StoreControllerTest.php index 377423716..697b1f28c 100644 --- a/tests/Unit/Controller/StoreControllerTest.php +++ b/tests/Unit/Controller/StoreControllerTest.php @@ -412,6 +412,57 @@ public function testAFailedWriteIsReportedWithoutLeakingTheReason(): void { }//end testAFailedWriteIsReportedWithoutLeakingTheReason() + /** + * 🔴 A component carrying an id must NOT overwrite a local object. + * + * OpenRegister resolves the object it writes from the payload: + * `saveObject()` reads `$object['@self']['id'] ?? $object['id']` and treats + * a match as the uuid to UPDATE. So a registry that shipped the uuid of + * this municipality's live case type would REPLACE it — and the write is + * PUT-semantic, so omitted keys are nulled rather than left alone. + * + * The schema allowlist does not cover this: the component names an allowed + * schema, which is exactly what makes it dangerous. + * + * @return void + * + * @spec openspec/changes/dossiq-store-surface/specs/dossiq-store-surface/spec.md#requirement-req-dss-003-install-accepts-configuration-and-refuses-records + */ + public function testAnInstalledComponentCannotOverwriteAnExistingObject(): void { + $this->storeService->method('resolve')->willReturn( + [ + 'slug' => 'hostile-item', + 'components' => [ + [ + 'schema' => 'caseType', + 'object' => [ + 'id' => 'the-municipalitys-live-case-type', + 'uuid' => 'the-municipalitys-live-case-type', + '@self' => ['id' => 'the-municipalitys-live-case-type'], + 'title' => 'Replaced', + ], + ], + ], + ] + ); + + $written = null; + $this->registry->method('save')->willReturnCallback( + static function (string $key, array $data) use (&$written): array { + $written = $data; + return $data; + } + ); + + $this->controller->install(slug: 'hostile-item'); + + $this->assertIsArray($written); + $this->assertArrayNotHasKey('id', $written, 'a remote id must not address a local object'); + $this->assertArrayNotHasKey('uuid', $written, 'nor a remote uuid'); + $this->assertArrayNotHasKey('@self', $written, 'nor @self, which saveObject reads FIRST'); + $this->assertSame('Replaced', $written['title'], 'the rest of the payload still installs'); + }//end testAnInstalledComponentCannotOverwriteAnExistingObject() + /** * The token never comes back out. *