fix(review): #4580 review findings, round 1

[WilcoLouwerse]

Fixes for the review of #4580 (#4580 (review)). Merge into development; the next sync PR carries them to beta. Nothing here is merged by the reviewer.

Findings → commits

Finding	Commit	What changed
eng-f2 🔴 A required file property can never be satisfied by an upload token	59794316c6	An uploaded file counts as supplying its property, so a required file property no longer fails validation.
eng-f6 🔴 Review policy fails open for visibility when it cannot be read	402a14a9a9	Both lookups (schema read, file-to-object) fail closed to a decider-owned, hide-others policy.
data-f1 🔴 Bulk save overwrites another user's draft	728c921b61 2d0dbae3fb c5354845b3 d942f683a9	Bulk save, bulk delete and single delete refuse another user's draft (without announcing it exists); admins pass like the read path; the guard runs before the appendOnly check.
plat-f1 🔴 Frozen-file guard ignores writes made through a share	f67de4564a	The frozen-file guard resolves a node to its owner's home, so writes through user, group, federated and public-link shares are refused too.
plat-f2 🔴 Any admin-UI file-settings save clears the instance review default	f1f9c1d83a	A file-settings save keeps the stored review default when the key is absent; an explicit null still clears it.
plat-f3 🔴 Deferred-chunk limit is 32000, but NC 32 and 33 accept 4000	02b92226c4 fb4be1d927 59f9acaccb	The chunk limit is read from JobList::MAX_ARGUMENT_JSON_LENGTH when the server defines it (34+), else 4000; an entry that alone is too large is kept without oldData and the log names the rules that are then skipped. A refused webhook job argument no longer aborts the other webhooks.
eng-f1 🟡 Upload endpoint ignores the form's audience	227bc52ed7	upload() applies the form's audience like submit(); a file property without maxSize is capped at 20 MiB.
eng-f3 🟡 Idempotency key is neither subject-bound nor race-safe	94854b289f 224da09c48 6703ff76a5	The stored answer is bound to the submitter and a payload hash (409 otherwise), and the key is locked while the submit runs (409 for a parallel retry; needs Nextcloud file locking). Spec scenario and docs updated.
eng-f5 🟡 Draft cleanup deletes without re-checking the draft	f990f3dd52	The cleanup re-reads the object and deletes only a draft that is still expired.
eng-f7 🟡 Dependent form reported unpublished when nothing was unpublished	33f3e3fb57	A form with no unpublish closure is reported as unpublish-unsupported, not unpublished.
eng-f8 🟡 Task wake still overwrites another listener's signal	c967b2d6bd	The task wake merges its payload into the run signal instead of overwriting another listener's.
eng-f9 🟡 Decision ownership check is skipped in two cases	1dfdcd386f	The decision ownership check also runs for a dangling uuid and inside the transaction (compare-and-swap link).
data-f3 🟡 RBAC draft predicate ignores unhealed _status	363e6075c7	The draft predicate is skipped on a table the healer reports without _status.
data-f4 🟡 Forced re-import deletes local authorization the file lacks	03e338eb67	A forced re-import keeps the instance's authorization when the file has none.
data-f5 🟡 Aggregation fast path counts drafts the list hides	d4aa263863	The aggregation fast path applies the same draft predicate as the list.
plat-f5 🟡 A later listener's allow() overrides an earlier refusal	a33e49b49f	A refusal by an earlier listener can no longer be overridden by a later allow().
surf-f1 🟡 Object GET /logs bypasses the audit-history redaction	4e900c4582 902c28b14c	Non-admin readers of GET /logs get the redacted history #4614 introduced (page shape without next/prev); admins unchanged.
surf-f4 🟡 Copies still send users to the removed personal settings form	5faeef9582 9420851e65	Links and texts point at the user settings instead of the removed personal settings form (en and nl l10n; other languages via Transifex).
aud-p1 🟡 The "may not let itself in" check ignores remote principals	e19745bc96	A grant_manage holder can no longer grant to remote / remote_group principals.
aud-p4 🟡 Guard failure imports shipped authorization unguarded	d0d2725b32	When the shipped-configuration guard fails, the instance's live authorization is kept instead of the shipped block.
plat-f4 🟡 Aborted delete or rename reads as success to internal callers (partial)	71f4796dc4	DeleteFileHandler::deleteFile() returns false when the node survives a refused delete. Rename and move: see below.
surf-f6 🟡 Dependent-form re-check runs on one schema write path only (partial)	1f4eafbd73	A container that cannot build the dependents check now logs a warning instead of skipping silently. The other write paths: see below.
eng-f10 🟡 Failure to arm a statutory expiry timer is only logged (partial)	09d5d985fd 50bee16d8f	A failed statutory timer is logged at error level naming case, item and legal effect. Failing the transition is not possible from here: see below.

Left to the author (threads stay open)

• eng-f13 🟡 Email recipient reference may point at any object in the organisation — not done: checking the related object's schema needs a rule for reference properties without $ref, and failing closed there would disable untyped references. Recommendation: refuse on a $ref mismatch when one is declared, plus a per-rule allowlist for untyped references.
• aud-p3 🟡 Delegated grant changes leave no audit-trail row — not done: audit rows for grant, revoke, owning-group change and grant_manage list change. Recommendation: one AuditTrailMapper helper called from ObjectSharingService, ObjectOwnershipService::setOwnerGroup and OrganisationController::update, modelled on ownership_handover.
• surf-f6 🟡 Dependent-form re-check runs on one schema write path only — the check still runs only on SchemasController. The importer, SchemaImportController, SchemaTool, SchemasToolProvider and SchemaService need a decision on what refuse means for a write without a response. Recommendation: a schema-updated listener with a controller-suppression flag, unpublish-only on non-request paths.
• plat-f4 🟡 Aborted delete or rename reads as success to internal callers — the rename and move handlers still report success for a refused operation, and ExampleSetRemoveLiveTest has no case for it.
• eng-f10 🟡 Failure to arm a statutory expiry timer is only logged — CasePlanStateMachine::flushEvents() swallows listener exceptions after the transition commits, so a statutory item can stay active without its expiry timer. A visible marker on the item needs a schema field.
• plat-f3 follow-up: a webhook payload over the job-argument limit (4000 characters on Nextcloud 32/33) is now logged and not delivered; delivering it needs a payload reference or trimming.
• data-f1 note: like the existing single-save guard, the new draft guards exempt admins but not a userless caller inside SystemOperationContext (the read path also exempts that one, MagicRbacHandler::isTrustedSystemCaller). No current caller hits it (system jobs pass _rbac:false); add the same exemption if a system-context flow should touch other users' drafts.

Tracked elsewhere

• eng-f2 🔴 A required file property can never be satisfied by an upload token → Forms: a required file property can never be satisfied by an upload token #4646
• eng-f6 🔴 Review policy fails open for visibility when it cannot be read → Review policy falls back to the open default when the schema or the file's object cannot be read #4647
• data-f1 🔴 Bulk save overwrites another user's draft → Bulk save overwrites another user's draft #4648
• plat-f1 🔴 Frozen-file guard ignores writes made through a share → Frozen/archived file guard ignores writes made through a share #4649
• plat-f2 🔴 Any admin-UI file-settings save clears the instance review default → Saving the file settings in the admin UI resets the anonymisation review default to open #4650
• plat-f3 🔴 Deferred-chunk limit is 32000, but NC 32 and 33 accept 4000 → Deferred listener chunks exceed the 4000-character job argument limit of Nextcloud 32 and 33 #4651

Checks run on this branch

• bash hydra/scripts/run-hydra-gates.sh --scope-to-diff --base b9e5d8df40 → exit 0, COVERAGE 65 of 65 applicable gates ran (32 not applicable)
• vendor/bin/phpunit on every touched test file, then tests/Unit/{Service,Controller,Listener,Db,Event} → no failures other than 61 could not find driver / not loadable errors (no database in this environment; the same errors occur without these changes); tests/Unit/Service/Object + the draft-delete and bulk tests after the last commit → OK (2747 tests)
• vendor/bin/phpcs --standard=phpcs.xml --warning-severity=0 on every changed lib/ file → clean
• Pre-push adversarial review, three rounds → round 1 and 2 findings fixed in the pre-push commits above
• not run (no database or live Nextcloud): a Newman PUT as a sharee against a frozen object (plat-f1), a count over a table with a draft and an active row (data-f5; SQL and bindings are asserted instead), the guarded SQL update in EntityRelationMapper::linkDecision (eng-f9), a real parallel idempotent submit (eng-f3)

🤖 Generated with Claude Code

[github-actions]

Quality Report — ConductionNL/openregister @ f8342a5

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		✅
composer			✅	✅ 173/173
npm			✅	✅ 648/648
app:check-code	⏭️
info.xml	✅
REUSE	✅
lockfile sync		✅
PHPUnit					❌ 26239/26246 · 25 skipped
Newman					✅ 714/714
Playwright					⏭️ deferred: E2E runs locally and on the promotion path only. This pull request targets development, so the suite is asked once per promotion into beta and main rather than once per push per open pull request. Run it locally with npx playwright test, or from the Actions tab on a branch with no open pull request into development.
Hydra gates					❌

---

Quality workflow — 2026-10-15 17:44 UTC

Download the full PDF report from the workflow artifacts.