fix(security): close the default-open write hole on decidesk's OpenRegister objects by rubenvdlinde · Pull Request #521 · ConductionNL/decidesk

rubenvdlinde · 2026-08-17T07:51:18Z

⚠️ This PR changes behaviour. Read this paragraph first.

A user who is neither an object's owner, nor a Nextcloud admin, nor a member of decidesk-administrators can no longer UPDATE or DELETE another user's decidesk object. Reads, listings and creates are unchanged, for everyone.

Until now they could — through OpenRegister's own API, with no decidesk code involved.

The hole

Every decidesk object is reachable at /apps/openregister/api/objects/decidesk/<schema> — the API the frontend uses directly under ADR-022. No decidesk controller guard sits in front of it. What decides who may write there is the authorization block on the schema, or failing that on the register row. This tree had neither.

OpenRegister's Service/Object/PermissionHandler::hasGroupPermission():

if (empty($authorization) === true || $publicOptIn === true) {
    if ($this->isDefaultClosedEnforced() === true
        && in_array($action, self::DEFAULT_CLOSED_WRITE_ACTIONS, true)
        && $publicOptIn === false) { return false; }
    return true;                        // <- create / update / delete, granted
}

PHP's empty() is true for null and [] alike, so an absent block takes the same default-OPEN branch as an empty one — there is no branch anywhere that distinguishes them. isDefaultClosedEnforced() reads IAppConfig with default: false (and its catch also returns false), so on a stock instance the deny arm never fires; even switched on it covers only the three write actions and never read. The anonymous fail-closed rule from openregister#1955 does not help either — it is checked only on the $userId === null branch, and an authenticated caller never reaches it.

Measured across all 25 register files on this tree (lib/Settings/decidesk_register.json + 24 lib/Settings/register.d/*.json):

	count
schemas total	93
with their own `authorization` block	24 — every one of them `read`-only
`decidesk` register row	no block (`authorization: null`)
⇒ open to create/update/delete for every authenticated user	69

Decision, VotingRound, Vote, Participant and EngagementRecord are among the 69. This is the same shape as docudesk#631, where a plain user overwrote another user's template.

(Parse the fragments, not just the main file: the main file alone reads 39 schemas / 5 blocks and understates both sides.)

Why the fix is one declaration on the register row

PermissionHandler::resolveAuthorization() uses a schema's own block when it has one and falls back to the register's only when it does not. That cascade means a single register-level baseline reaches exactly the 69 unprotected schemas and changes nothing for the 24 that already declare their own — their public-read-under-publication-date rules are untouched, and they already fail closed on writes by naming no write action.

"authorization": {
  "read":   ["authenticated", "public"],
  "list":   ["authenticated", "public"],
  "create": ["authenticated"],
  "update": ["decidesk-administrators"],
  "delete": ["decidesk-administrators"]
}

🔴 `public` on the read actions is a CORRECTION, and CI found it

The first push of this PR omitted public, and all six PHPUnit legs failed:

NotAuthorizedException: User 'Anonymous' does not have permission to 'read' objects in schema 'Meeting'
  openregister/lib/Service/Object/PermissionHandler.php:1019
  decidesk/tests/Integration/Meeting/QuorumDeclarativeTest.php:341

Before any block existed, hasGroupPermission() took its default-OPEN branch for every principal — the anonymous one included. So a block naming only authenticated on read does not preserve the status quo, it closes anonymous reads. Omission is the deny, and that is as true of the principal as of the action. PHPUnit's CLI has no session, so those integration tests exercise exactly the path a #[PublicPage] citizen-participation surface takes: left as it was, this PR would have 403'd every public consultation and budget-proposal read.

public now appears on read and list — precisely the pre-change behaviour — and on no write action, so openregister#1955's anonymous fail-closed rule keeps denying anonymous create/update/delete exactly as before and the write hole stays closed. Closing anonymous reads may well be worth doing; it is a much larger policy change than this PR and does not belong smuggled inside it. A new test guards it, shown red on the omission.

Neither defect was findable locally — the unit environment stubs OCA\OpenRegister\*, so the integration tests that hit the real permission path are precisely the ones that cannot run there. (A second, smaller one came back with it: my own test used simplexml_load_file(), which worked locally and returned false on every CI leg. It now reads the file as text.)

Every canonical action is written out on purpose. Once a block is non-empty, hasGroupPermission() reaches if (empty($authorization[$action])) return false; — OpenRegister denies any action the block omits. A read-only block bolted onto 69 schemas would not secure this app, it would break every write in it. That trap has already cost this programme five bad PRs; read, list and create are named deliberately so nothing goes dark.

What actually narrows is only update and delete, and only for third parties. OpenRegister bypasses the object owner unconditionally and SQL-side, before any rule is consulted, and bypasses admin too — so an author keeps full control of their own object. decidesk-administrators is an admin-provisioned Nextcloud group for a griffie/secretariat that must edit records it did not create; on an instance where that group does not exist this resolves to owner-plus-admin, which is fail-closed rather than open.

Per-body actor authorization (the chair / signatory scopes in x-decidesk-rbac-scopes) is a separate layer and is unaffected — OpenRegister cannot template a group name per object, which is exactly why that determination is made at the app boundary in GovernanceScopeGuard.

Both version bumps are load-bearing

Neither is cosmetic, and either one alone leaves a correct block sitting on disk on every existing instance:

register 0.7.0 → 0.8.0 (and info.version with it). ImportHandler's REGISTER path skips outright when the incoming version is <= the stored one — and unlike the SCHEMA path (which falls back to comparing content) it has no content-differs escape.
app 0.4.6 → 0.4.7. InitializeSettings is a <post-migration> repair step, and post-migration steps run only on occ upgrade, which is a no-op when the app version has not moved.

Two of the seven tests assert exactly these, each against the last version that shipped without the block, so they survive future bumps.

The tests, and their red control

Seven tests. On development's own state — register JSON and info.xml reverted, tests kept — 5 of the 7 fail:

✘ The register declares a complete authorization block      ✘ The register version moved past the unprotected release
✘ Read list and create stay open to authenticated users     ✘ The app version moved so the repair step runs
✘ Update and delete are not open to every authenticated user
✔ Schemas with their own block still declare only reads      ← correctly green: those 24 are unchanged
Tests: 7, Errors: 2, Failures: 3   →  restored: OK (7 tests, 127 assertions)

They deliberately do not re-implement OpenRegister's evaluator. An instrument built from the same source as the bug reports zero, and zero reads as a pass. What this repository owns is the declaration the evaluator reads, so that is what is pinned — including the assertions that would catch a well-meaning future edit re-opening the hole (update/delete must contain neither authenticated nor public; no write action anywhere may name public, since publicGroupExplicitlyGranted() is the one thing that re-opens the anonymous writes openregister#1955 closed).

Two carry positive controls on their own reader, so a failure can never mean "read nothing": the app-version test also asserts info.xml's <id> reads decidesk, and the schema-block test asserts the count is 24 so it cannot pass vacuously if schemas are renamed or moved.

🔴 Verification I am NOT claiming

I did not run a two-account, non-owner probe against a live instance. decidesk is not installed on the shared dev instance, and standing up the rig was out of budget for this change.

This matters and is stated rather than glossed: the owner bypass is unconditional and SQL-side, so any test driven by a single seeded session — which in practice owns its objects, and is usually admin — cannot observe this denial at all and would report success over the exact hole. That is also why REQ-DCDH's new scenario carries an @e2e exclude naming that reason instead of a Playwright test.

What a reviewer should run before merging:

As user A, create a Decision. As user B (not admin, not in decidesk-administrators), PUT and DELETE that Decision through /apps/openregister/api/objects/decidesk/decision/<id> — both must be refused.
As user A, do the same on their own object — both must succeed (owner bypass).
As B, GET/list decidesk objects and POST a new one — all must still work.
Exercise any flow where one user legitimately edits an object another user created. The two I would check first are co-authored motions (MotionCoauthorService) and a griffier editing a meeting created by a chair. If either is a real requirement, the answer is to add that role's projected group to update, not to widen it back to authenticated.

Checks

Both sides on gate package 742f370e152b296acdca3289230a0119d8bb23b8:

check	result
`run-hydra-gates.sh --base a9de4096`	1 failing gate: gate-24 — the pre-existing `development` failure this PR does not touch. `64 of 64 applicable gates ran`.
`[gate-47] security-change-has-tests`	PASS
`[gate-16]` / `[gate-46]` / `[gate-22]` / `[gate-56]`	PASS
`phpunit -c phpunit.xml`	`Tests: 980, Assertions: 3836, Errors: 87, Failures: 1, Skipped: 33`
same suite without this PR's test file	`Tests: 973, Errors: 87, Failures: 1`

⚠️ The 87 errors + 1 failure are local-only and identical on the base — the documented OCA\OpenRegister\Service\FileService mocks; CI checks out openregister@development as an additional app and binds the real class. This PR contributes exactly +7 tests / +127 assertions and moves neither number.

Parity: this PR fails gate-24 and nothing else, which is precisely what development fails today. #520 is the PR that closes gate-24; the two are independent and can land in either order.

…gister objects BEHAVIOUR CHANGE, stated up front: a user who is neither an object's owner, nor a Nextcloud admin, nor a member of `decidesk-administrators` can no longer UPDATE or DELETE another user's decidesk object. Reads, listings and creates are unchanged. Until now they could. Every decidesk object is reachable at /apps/openregister/api/objects/decidesk/<schema> — the API the frontend uses directly under ADR-022 — and no decidesk controller guard sits in front of it. What decides who may write there is the `authorization` block on the schema, or failing that on the register row. This tree had neither. OpenRegister's PermissionHandler::hasGroupPermission() tests `empty($authorization)`, and PHP's empty() is true for null and [] alike, so an ABSENT block takes the same default-OPEN branch as an empty one. `enforce_default_closed` reads IAppConfig with default:false, so on a stock instance its deny arm never fires — and even switched on it covers only writes, never reads. Measured across all 25 register files on this tree: 93 schemas, 24 carrying a block (every one of them read-only), and the register row carrying none. So 69 schemas — Decision, VotingRound, Vote, Participant and EngagementRecord among them — granted create, update AND delete to any logged-in account. The same shape as docudesk#631, where a plain user overwrote another user's template. The fix sits on the REGISTER row because of the cascade: resolveAuthorization() uses a schema's own block when it has one and falls back to the register's only when it does not. So one declaration reaches exactly the 69 unprotected schemas and changes nothing for the 24 that already declare their own — their public-read publication rules are untouched. Every canonical action is written out deliberately. Once a block is non-empty, OpenRegister DENIES any action it omits, so a half-written block breaks the app rather than securing it: read/list/create stay `authenticated`, and only update/delete are narrowed. The owner bypass is unconditional and SQL-side and precedes every rule, so an author keeps full control of their own object. Both version bumps are load-bearing and neither is cosmetic. ImportHandler's REGISTER path skips outright when the incoming version is <= the stored one and, unlike the schema path, has no content-differs fallback — so the register goes 0.7.0 -> 0.8.0. And InitializeSettings is a <post-migration> repair step, which runs only on `occ upgrade`, which is a no-op when the app version has not moved — so appinfo goes 0.4.6 -> 0.4.7. Either bump alone leaves a correct block sitting on disk on every existing instance.

@e2e

Seven tests, shown red on development's own state before they were shown green: 5 of 7 fail there (the block is absent, and both versions are behind), and the two that stay green are the ones asserting the UNCHANGED 24 schema-level blocks — which is what they should do. They deliberately do NOT re-implement OpenRegister's evaluator. An instrument built from the same source as the bug reports zero, and zero reads as a pass. What this repository owns is the DECLARATION the evaluator reads, so that is what is pinned: the block exists and names every canonical action with a non-empty rule list; read/list/create still grant `authenticated` (if this goes red the fix has become an outage); update/delete grant neither `authenticated` nor `public`; no write action anywhere names `public`, which is the one thing that would re-open the anonymous writes openregister#1955 closed. Two of the seven guard the deploy path rather than the policy, because a correct block that never reaches an instance is a fix that reports success and changes nothing: the register/config version must be past 0.7.0 and the app version past 0.4.6, each asserted against the last release that shipped WITHOUT the block so the assertion survives future bumps. The app-version test carries a positive control on its own reader (info.xml's <id> must read `decidesk`), so a failure means 'not bumped' and never 'parsed an empty document'. The schema-block test carries the same kind of control: it asserts the COUNT is 24, so it cannot pass vacuously if the schemas are renamed, moved, or stop being found. REQ-RBAC-006 records the requirement, with an @e2e exclude that names the real reason: the owner bypass is unconditional and SQL-side, so a browser test driven by one seeded (owning, usually admin) session cannot observe this denial at all and would report success over the exact hole. The per-user behaviour needs a two-account probe against a live instance, and that is recorded as verification owed rather than claimed.

…hem, and CI proved it Two defects in the first version of this PR, both found by CI and neither findable locally. Recording what happened, because the first one is the exact trap this change was supposed to avoid and I walked into it anyway. 1. THE BLOCK CLOSED ANONYMOUS READS. All six PHPUnit legs failed with `NotAuthorizedException: User 'Anonymous' does not have permission to 'read' objects in schema 'Meeting'`. Before any block existed, hasGroupPermission() took its default-OPEN branch for EVERY principal — the anonymous one included — so a block naming only `authenticated` on `read` does not preserve the status quo, it CLOSES anonymous reads. Omission is the deny, and that is as true for the principal as it is for the action. PHPUnit's CLI has no session, so those integration tests exercise exactly the path a #[PublicPage] citizen-participation surface takes. Left as it was, this PR would have 403'd every public consultation and budget-proposal read. `read` and `list` now name `public` as well, which is precisely the pre-change behaviour. `public` appears on NO write action, so openregister #1955's anonymous fail-closed rule keeps denying anonymous create/update/delete exactly as before, and the write hole stays closed. Closing anonymous reads may well be worth doing — it is a far larger policy change than this PR, and it does not belong smuggled inside it. Guarded by a new test, shown red on the omission. 2. MY OWN TEST FAILED ON AN EXTENSION CI DOES NOT HAVE. `simplexml_load_file()` worked locally and returned FALSE on every CI leg, failing the suite on 'appinfo/info.xml must be readable XML'. The assertion is about one scalar in a file this repository owns; it now reads the file as text and preg_matches <version>, with the positive control moved to a string match on <id>. ⚠️ Local green could not have caught either one. The unit environment stubs OCA\OpenRegister\*, so the integration tests that hit the real permission path are precisely the ones that cannot run there — and the XML failure needed CI's own PHP image. Both are the 'local green means nothing for this bug class' shape.

github-actions · 2026-08-17T08:15:14Z

Quality Report — ConductionNL/decidesk @ `0269dab`

Check	PHP	Vue	Security	License	Tests
lint	✅
phpcs	✅
phpmd	✅
psalm	✅
phpstan	✅
phpmetrics	✅
eslint		✅
stylelint		✅
build		✅
check-manifest		✅
test-l10n		✅
format		✅
composer			✅	✅ 104/104
npm			✅	✅ 532/532
app:check-code	⏭️
info.xml	✅
REUSE	❌
PHPUnit					❌
Newman					✅
Playwright					❌
Hydra gates					❌

Quality workflow — 2026-08-17 08:15 UTC

Download the full PDF report from the workflow artifacts.

Picks up #522's composer cooldown in .github/dependabot.yml. The gate package moved from 742f370e to 0b189e30 mid-review and added gate-93 composer-cooldown-config, which this branch failed purely by predating the fix that development already carries.

github-actions · 2026-08-17T10:40:12Z

Quality Report — ConductionNL/decidesk @ `e5182e5`

Check	PHP	Vue	Security	License	Tests
lint	✅
phpcs	✅
phpmd	✅
psalm	✅
phpstan	✅
phpmetrics	✅
eslint		✅
stylelint		✅
build		✅
check-manifest		✅
test-l10n		✅
format		✅
composer			✅	✅ 104/104
npm			✅	✅ 532/532
app:check-code	⏭️
info.xml	✅
REUSE	❌
PHPUnit					✅
Newman					✅
Playwright					✅
Hydra gates					❌

Quality workflow — 2026-08-17 10:40 UTC

Download the full PDF report from the workflow artifacts.

github-actions · 2026-08-17T11:47:34Z

Quality Report — ConductionNL/decidesk @ `6688cfa`

Check	PHP	Vue	Security	License	Tests
lint	✅
phpcs	✅
phpmd	✅
psalm	✅
phpstan	✅
phpmetrics	✅
eslint		✅
stylelint		✅
build		✅
check-manifest		✅
test-l10n		✅
format		✅
composer			✅	✅ 104/104
npm			✅	✅ 532/532
app:check-code	⏭️
info.xml	✅
REUSE	❌
PHPUnit					✅
Newman					✅
Playwright					✅
Hydra gates					❌

Quality workflow — 2026-08-17 11:47 UTC

Download the full PDF report from the workflow artifacts.

juanclaude-conduction added 2 commits August 17, 2026 09:47

rubenvdlinde requested review from Rem-Dam, WilcoLouwerse, bbrands02 and rjzondervan as code owners August 17, 2026 07:51

rubenvdlinde merged commit 01f9f28 into development Aug 17, 2026
75 of 79 checks passed

Navigation Menu

Uh oh!

fix(security): close the default-open write hole on decidesk's OpenRegister objects - #521

fix(security): close the default-open write hole on decidesk's OpenRegister objects#521
rubenvdlinde merged 4 commits into
developmentfrom
fix/or-authorization-baseline

rubenvdlinde commented Aug 17, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Aug 17, 2026

Uh oh!

github-actions Bot commented Aug 17, 2026

Uh oh!

github-actions Bot commented Aug 17, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

rubenvdlinde commented Aug 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

⚠️ This PR changes behaviour. Read this paragraph first.

The hole

Why the fix is one declaration on the register row

🔴 public on the read actions is a CORRECTION, and CI found it

Both version bumps are load-bearing

The tests, and their red control

🔴 Verification I am NOT claiming

Checks

Uh oh!

github-actions Bot commented Aug 17, 2026

Quality Report — ConductionNL/decidesk @ 0269dab

Uh oh!

github-actions Bot commented Aug 17, 2026

Quality Report — ConductionNL/decidesk @ e5182e5

Uh oh!

github-actions Bot commented Aug 17, 2026

Quality Report — ConductionNL/decidesk @ 6688cfa

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

rubenvdlinde commented Aug 17, 2026 •

edited

Loading

🔴 `public` on the read actions is a CORRECTION, and CI found it

Quality Report — ConductionNL/decidesk @ `0269dab`

Quality Report — ConductionNL/decidesk @ `e5182e5`

Quality Report — ConductionNL/decidesk @ `6688cfa`