From 33a5b108a9ed9c9124fe0512f3ff10707fc04412 Mon Sep 17 00:00:00 2001 From: Conduction Release Bot Date: Wed, 2 Sep 2026 20:43:38 +0200 Subject: [PATCH] fix(store): an install creates, and can never replace a local object MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Found reviewing my own merged code from #1677, and it is a hole in the boundary that PR was largely about. OpenRegister resolves the object it writes FROM THE PAYLOAD: `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 — and `saveObject` is PUT-semantic, so keys the payload omits are nulled rather than 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. A component naming `caseType` — entirely legitimate, exactly what the store is for — is the attack. I wrote that allowlist and called it the boundary; it was half of one. Identity is not a remote registry's to supply. `id`, `uuid` and `@self` are stripped before the write, so an installed component is always a NEW local object. If install ever needs to be idempotent it must key on something dossiq controls. The test carries a negative control: removing the strip makes it fail. --- lib/Controller/StoreController.php | 38 +++++++++++++- .../specs/dossiq-store-surface/spec.md | 28 ++++++++++ tests/Unit/Controller/StoreControllerTest.php | 51 +++++++++++++++++++ 3 files changed, 116 insertions(+), 1 deletion(-) 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. *