Skip to content

A store install creates, and can never replace a local object - #1687

Merged
rubenvdlinde merged 1 commit into
developmentfrom
fix/store-install-cannot-overwrite
Sep 2, 2026
Merged

A store install creates, and can never replace a local object#1687
rubenvdlinde merged 1 commit into
developmentfrom
fix/store-install-cannot-overwrite

Conversation

@rubenvdlinde

Copy link
Copy Markdown
Contributor

Found reviewing my own merged code from #1677. It is a hole in the boundary that PR was largely about, so it is worth stating plainly rather than filing quietly.

The defect

OpenRegister resolves the object it writes from the payload itself. ObjectService::extractUuidAndNormalizeObject() reads:

$providedId = $object['@self']['id'] ?? $object['id'] ?? null;

and treats a match as the uuid to UPDATE. saveObject is PUT-semantic, so keys the payload omits are nulled rather than left alone.

So a store item whose component carried the uuid of this municipality's live case type would not merely change it — it would gut it. Through a button labelled Install.

Why the allowlist did not catch it

INSTALLABLE_SLUGS governs which schema a component may write. It says nothing about whether the write creates or replaces. A component naming caseType is entirely legitimate — it is exactly what the store exists for — and that is what makes it the attack.

I wrote that allowlist and called it the security boundary. It was half of one.

The fix

id, uuid and @self are stripped before the write, so an installed component is always a new local object. Identity is not a remote registry's to supply.

If install ever needs to be idempotent, it must key on something dossiq controls rather than on a remote id.

Evidence

The test carries a negative control: removing the strip makes testAnInstalledComponentCannotOverwriteAnExistingObject fail. It asserts @self too, not just id, because @self.id is the one saveObject reads first.

2768 PHPUnit tests, phpstan clean.

🤖 Generated with Claude Code

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.
@rubenvdlinde

Copy link
Copy Markdown
Contributor Author

Swept the defect class rather than just this instance.

openbuild, the other GenericStoreService consumer, does NOT have this hole, and is structurally immune rather than accidentally safe. Its install never hands the remote payload to a write:

  • installFromTemplateArray() refuses on slug collision with a 409 before writing anything;
  • materialiseApplication() builds a NEW Application — new slug, owner-tagged record, companion-schema namespacing — so identity is minted locally and the remote template is a source of content only;
  • nothing in that path reads $template['id'], ['uuid'] or ['@self'].

dossiq's install was the outlier: it passed the component's object array straight to ConfiguredRegistryService::save(), and that is what let the remote id become the addressed object.

Worth stating for whoever writes the third consumer, since ADR-080 Decision 3 deliberately leaves install to each app: the engine gives you resolve(), and what comes back is somebody else's data. Build a new record from it; never write it.

@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/dossiq @ 4e554bb

Check PHP Vue Security License Tests
lint
phpcs
phpmd
psalm
phpstan
phpmetrics
eslint
stylelint
build
check-manifest
check-vue3-compile
test-l10n
format
check-schema-l10n
check-l10n-js
composer ✅ 106/106
npm ✅ 540/540
app:check-code ⏭️
info.xml
REUSE
PHPUnit
Newman ⏭️
Playwright ⏭️ deferred — runs on the promotion into beta/main, not on a pull request into development
Hydra gates

Quality workflow — 2026-09-02 19:21 UTC

Download the full PDF report from the workflow artifacts.

@rubenvdlinde
rubenvdlinde merged commit 282c881 into development Sep 2, 2026
49 checks passed
@rubenvdlinde
rubenvdlinde deleted the fix/store-install-cannot-overwrite branch September 2, 2026 19:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant