A saved mapping result may not address an existing object - #1826
Merged
rubenvdlinde merged 1 commit intoSep 3, 2026
Conversation
Found sweeping a defect class out of dossiq (ConductionNL/dossiq#1687, #1690) and openregister (#3345). `MappingsController::saveObject()` saves the OUTPUT OF A MAPPING TEST as an object — the UI button reads "save result as object", and the frontend posts `object: this.result`, the transformed output. A mapping transforms SOURCE data, and source records very often carry an `id`. `ObjectService::saveObject()` resolves its target from the payload (`@self.id` first, then `id`) and the write is PUT-semantic, so a result carrying either silently REPLACED whatever object shared that identifier, nulled every field the result omitted, and reported success. Identity here belongs to the object being created, not to the source record the mapping happened to read. BEHAVIOUR CHANGE, stated plainly: this endpoint now always creates. If anyone relies on it as an upsert, this breaks that — but the surface it serves is a mapping-test result panel, and an upsert keyed on the SOURCE system's id is not something that panel can have meant. Admin-only (`#[AuthorizedAdminSetting]`), so this is data integrity rather than privilege escalation. The test carries a negative control: removing the strip makes it fail.
rubenvdlinde
requested review from
WilcoLouwerse,
bbrands02 and
rjzondervan
as code owners
September 3, 2026 05:26
Contributor
Quality Report — ConductionNL/integriq @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ✅ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| build | ✅ | ||||
| build | ✅ | ||||
| check-specs | ✅ | ||||
| test-l10n | ✅ | ||||
| format | ✅ | ||||
| check-schema-l10n | ✅ | ||||
| check-l10n-js | ✅ | ||||
| composer | ✅ | ✅ 141/141 | |||
| npm | ✅ | ✅ 537/537 | |||
| 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-03 05:31 UTC
Download the full PDF report from the workflow artifacts.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Third app in a sweep that began in dossiq (ConductionNL/dossiq#1687, #1690) and continued in openregister (ConductionNL/openregister#3345).
The defect
MappingsController::saveObject()saves the output of a mapping test as an object. The UI button reads save result as object, and the frontend postsobject: this.result— the transformed output.A mapping transforms source data, and source records very often carry an
id. OpenRegister'sObjectService::saveObject()resolves its target from the payload:The write is PUT-semantic, so a result carrying either silently replaced whatever object shared that identifier, nulled every field the result omitted, and reported success.
Identity here belongs to the object being created, not to the source record the mapping happened to read.
Behaviour change, stated plainly
This endpoint now always creates. If anyone relies on it as an upsert, this breaks that. I do not believe anyone can: the surface it serves is a mapping-test result panel, and an upsert keyed on the source system's id is not something that panel can have meant. Flagging it so a reviewer who knows otherwise can say so.
Severity
Admin-only (
#[AuthorizedAdminSetting]), so this is data integrity, not privilege escalation. An administrator testing a mapping against real source data could destroy an unrelated object and see a success toast.Evidence
One test with a negative control: removing the strip makes it fail. It asserts
@selfas well asid, because@self.idis the keysaveObjectreads first.phpcs and phpmd clean on the changed file.
🤖 Generated with Claude Code