Skip to content

A federated create may not address an existing object, and the organisation pin did not hold - #3345

Merged
rubenvdlinde merged 3 commits into
developmentfrom
fix/federated-create-cannot-address-an-object
Sep 3, 2026
Merged

A federated create may not address an existing object, and the organisation pin did not hold#3345
rubenvdlinde merged 3 commits into
developmentfrom
fix/federated-create-cannot-address-an-object

Conversation

@rubenvdlinde

Copy link
Copy Markdown
Contributor

Found sweeping a defect class out of dossiq (ConductionNL/dossiq#1687, #1690). Two holes in one line, and the second is the one that line was written to prevent.

The line

FederationController::createObject() is #[PublicPage], #[NoCSRFRequired], and calls saveObject() with _rbac: false and _multitenancy: false — so nothing downstream refuses what it lets through.

$data['@self'] = (($data['@self'] ?? []) + ['organisation' => $share->getOrganisation()]);

PHP's + keeps the left operand's keys. For a caller holding a writable share token:

1. @self.id survived. saveObject() resolves its target from the payload — extractUuidAndNormalizeObject() reads @self.id first, then id — and the write is PUT-semantic, so omitted fields are nulled. A share granting the right to add objects also granted the right to overwrite every existing object in the shared register/schema.

2. @self.organisation survived too, which defeats the pin. The comment on that very line reads:

Pin the object to the sharing organisation — a federated writer can never plant an object into another organisation.

It could.

The second was found by the first test written for the first. A pin has to overwrite, or it is only a default.

Also fixed

DsarCaseController::create() — guarded by requireAuthenticated() rather than an admin setting, and stripped only _route, so a create could address an existing DSAR case.

Evidence

Three new tests, each failing against the original code and passing against the fix. The third asserts the pin and that the payload still arrives, so the guard cannot have traded one defect for another by over-stripping.

Full suite: 19037 tests, phpcs clean.

Not claimed

End-to-end exploitability. Both paths need a valid read-write share token, and I did not drive this against a live federated pair. The code path and the + semantics are demonstrated; the operational reachability is not.

🤖 Generated with Claude Code

Conduction Release Bot added 2 commits September 3, 2026 07:07
TWO holes in one line, and the second is the one the line was written to
prevent.

`FederationController::createObject()` is `#[PublicPage]`, `#[NoCSRFRequired]`,
and calls `saveObject()` with `_rbac: false` and `_multitenancy: false`, so
nothing downstream refuses what it lets through. It stripped `shareToken` and
`_route`, then pinned the organisation with:

    $data['@self'] = (($data['@self'] ?? []) + ['organisation' => ...]);

PHP's `+` keeps the LEFT operand's keys. So for a caller holding a writable
share token:

  1. `@self.id` SURVIVED. `saveObject()` resolves its target from the payload —
     `extractUuidAndNormalizeObject()` reads `@self.id` first, then `id` — and
     the write is PUT-semantic, so omitted fields are NULLED. A share that
     grants the right to ADD objects also granted the right to overwrite every
     existing object in the shared register/schema.

  2. `@self.organisation` SURVIVED TOO, which defeats the pin. The comment on
     that very line reads "a federated writer can never plant an object into
     another organisation". It could.

The second was found by the first test written for the first: a pin has to
overwrite, or it is only a default. Assigned rather than merged now.

`DsarCaseController::create()` gets the same treatment. It is guarded by
`requireAuthenticated()` rather than an admin setting, and stripped only
`_route`, so a create could address an existing DSAR case.

All three new tests fail against the original code and pass against the fix.
Full suite: 19037 tests, phpcs clean.

NOT CLAIMED: end-to-end exploitability. Both paths need a valid `read-write`
share token, and this was not driven against a live federated pair.
phpmd's ElseExpression rule, on the identity strip added in the previous
commit. The unset is unconditional now and the conditional only re-adds a
non-empty @self, which reads better anyway.

Scoped phpmd on the file: clean. The full-tree run is deliberately left to CI —
it OOM-killed this host once already.
@github-actions

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/openregister @ 919b1e2

Check PHP Vue Security License Tests
lint
phpcs
phpmd
psalm
phpstan
phpmetrics
eslint
stylelint
build
check-specs
test-l10n
test-l10n-parity
format
check-schema-l10n
check-l10n-js
composer ✅ 174/174
npm ✅ 543/543
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:17 UTC

Download the full PDF report from the workflow artifacts.

@github-actions

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/openregister @ 472abbe

Check PHP Vue Security License Tests
lint
phpcs
phpmd
psalm
phpstan
phpmetrics
eslint
stylelint
build
check-specs
test-l10n
test-l10n-parity
format
check-schema-l10n
check-l10n-js
composer ✅ 174/174
npm ✅ 543/543
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:25 UTC

Download the full PDF report from the workflow artifacts.

createObject() was the one changed method without one. Tagged to the same
federation-scope-enforcement spec its three sibling methods in this controller
already point at.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@github-actions

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/openregister @ cc4f422

Check PHP Vue Security License Tests
lint
phpcs
phpmd
psalm
phpstan
phpmetrics
eslint
stylelint
build
check-specs
test-l10n
test-l10n-parity
format
check-schema-l10n
check-l10n-js
composer ✅ 174/174
npm ✅ 543/543
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:34 UTC

Download the full PDF report from the workflow artifacts.

@rubenvdlinde
rubenvdlinde merged commit 85e8321 into development Sep 3, 2026
47 checks passed
@rubenvdlinde
rubenvdlinde deleted the fix/federated-create-cannot-address-an-object branch September 3, 2026 05:41
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