fix(review): #4580 review findings, round 2 by WilcoLouwerse · Pull Request #4696 · ConductionNL/openregister

WilcoLouwerse · 2026-10-19T20:16:36Z

Fixes for the round-2 review of #4580 (#4580 (review)). Merge into development, then re-request the review; nothing here is merged by the reviewer.

Findings → commits

Finding	Commit	What changed
rel-f1 🟡 Related-row filters ignore property-level read rules	`98b1701618`	A condition on a related schema's property is refused when the caller cannot read that property (`PropertyRbacHandler::canReadProperty` with an empty object, failing closed: the question the facet path asks), with the same message as an unknown property. A nested-path filter whose root the caller cannot read matches nothing.
rel-f2 🟡 Related-row subquery has no organisation (multitenancy) filter	`996b7a05cd`, `d5e39b1d1c`, `58bbec05a4`	The related subquery gets the same organisation boundary the list path applies to the related table. `d5e39b1d1c` moves the RBAC predicate into `RelatedRowAccess` so no new phpmd baseline entry is needed; `58bbec05a4` pins the scope for `_multitenancy=false`, `_rbac=false`, an anonymous caller and a public schema.
rel-f3 🟡 _referenced/_related are dropped on the cross-table UNION path	`8b8683ad89`, `62f03f1b20`, `a1c270e92e`	The raw-SQL UNION builder refuses the keys instead of silently answering the unfiltered set. A cross-table search or count with either key takes the sequential path, which builds the EXISTS subqueries. A multi-schema facet request with either key answers HTTP 400 through the new `RelatedRowFilterMiddleware`, instead of empty or wrong buckets.
rel-f4 🟡 Alert recipients are judged at save only	`68295b131f`	Every resolved recipient is re-checked against the view as it is when the alert fires, with the same `ViewShareResolver::accessFor` the save uses.
rel-f5 🟡 `since` with a UTC offset is read as wall-clock UTC	`c2af88358c`	The parsed moment is converted to UTC before it is bound, on `GET /api/flow-runs?since=` and `/api/flow-runs/summary`.
rel-f6 🟡 Storage usage silently under-reports when a table cannot be read	`7f3bf8c7d1`	Every table that cannot be read is logged and named, and the figure becomes `null` (not measured) instead of a too-low number.
rel-f7 🟡 Related-row condition field is not checked against the related schema	`4909fa15b4`, `13b0f0415d`	A condition field must be a property of the related schema: metadata columns and unknown names are refused with an `InvalidArgumentException` naming the field, instead of reaching the database as an unknown column. `13b0f0415d` applies the read path's 64-character column-name cap.
rel-f8 🟢 Nested-path segment regex accepts a trailing newline	`1f3566bc24`	The segment pattern ends in `\z`; the same `$`-anchored identifier checks in the related-row clause are fixed too.
rel-f9 🟢 Step log line message length is not bounded	`35944cfd5a`	A message longer than 500 characters is cut and ends in a truncation marker.
rel-f10 🟢 Existing object: tags stay visible and assignable	`e868c0b1e2`	`findOrCreateTag` repairs a found `object:` tag that is still visible or assignable.
—	`87b2716631`	No behaviour change: keeps the fixes for rel-f1, rel-f2, rel-f4 and rel-f7 inside the PHPMD limits.
—	`271c6938a0`, `c21d7fc12e`	`development` merged in (#4697, #4698, #4699). The new orNull test from #4697 mocked the property read check as "no access", so with rel-f1 its nested-path filter matched nothing; the mock now grants read access. Test-only.

Left to the author

rel-f1, two paths not covered: the property read rule now holds for related-row conditions and for the root of a nested-path filter. The plain column filter (?property=value, named in the finding as pre-existing) and the outer property of a _referenced join are not changed here.
rel-f6, per-request cost: GET /api/organisations/{uuid} still scans every magic table on each request. Caching or bounding that scan needs a decision on the cost.
rel-f10, tags never attached again: the repair runs when findOrCreateTag finds a tag. Tags that are never attached again keep their old flags; hiding those needs an upgrade step.

Checks run on this branch

pre-push adversarial check: four findings (a new phpmd baseline entry, a 500 on cross-table search, the missing column-name cap, untested scope flags) fixed in d5e39b1d1c to a1c270e92e, then re-checked
composer phpcs → exit 0; composer phpstan → no errors
phpmd, both CI commands, on a clean git archive export with the baseline unchanged → exit 0
phpunit (phpunit-unit-local.xml) on the touched test folders → 9770 tests; the 41 errors and 7 failures are the same set as on the base commit (no pdo_sqlite, no Nextcloud server here)
run-hydra-gates.sh --scope-to-diff --base origin/development → exit 0, 45 of 45 applicable gates ran
not run: RelatedRowPrefixedDatabaseTest (needs SQLite), and a real HTTP request through the facet route (the 400 is pinned at the middleware)

🤖 Generated with Claude Code

… read rules Thread: #4580 (comment) A condition on a related schema's property is a boolean oracle on its value, so the property read rule has to hold for a property a filter names, not only for one a response returns. New PropertyReadGuard asks the same question the facet path asks (PropertyRbacHandler::canReadProperty with an empty object, failing closed). - RelatedRowQueryApplier refuses a block whose condition fields (and, for _related, the join foreign key) the caller may not read; the message equals an unknown property so a withheld field cannot be told from a missing one. - Class sweep: the root property of a nested-path filter (<object>.<path>) on both the QueryBuilder and the raw UNION path now matches nothing when the caller cannot read the root. - Not covered here: the plain column filter path (named in the finding as pre-existing) and the outer property of a _referenced join. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…(multitenancy) filter Thread: #4580 (comment) The related schema's row predicate has no _organisation term, so on a schema without an authorization block rows of another organisation satisfied a related-row condition (forward: probe a foreign row by writing its uuid into an own object; inverse: foreign rows pointing at the caller's object). RelatedRowQueryApplier::apply() now takes the organisation scope for the related table and ANDs the boundary into the access predicate of every related-row subquery (IN / IN-or-NULL / NULL-only; SCOPE_NONE, an unknown mode or an empty uuid list match nothing). MagicSearchHandler decides that scope with the list path's own helpers (resolveMultitenancyFlag, multitenancyApplies, resolveOrganizationScope, cascade widening), made for the related schema with the request's flags, so subquery and list cannot disagree. Both _related and _referenced go through the one applier, so both are closed. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…d against the related schema Thread: #4580 (comment) A condition field only had to look like an identifier, so metadata columns of readable rows (_owner, _organisation, _authorization, _status) could be compared with gt/lt/in, and an unknown name reached the database as an unknown column (500 instead of a 400). - RelatedRowQueryApplier refuses, with InvalidArgumentException naming the field, every condition field (and, for _related, the join foreign key) that is not a property of the related schema. The message is the same one the property read rule uses, so a withheld field and a missing one look alike. - RelatedRowExistsClause::columnName now applies the full column-naming rule (snake_case, invalid characters to underscore, collapsed and trimmed underscores) instead of a one-line camel-to-snake conversion. The col_ prefix is deliberately not applied: the rule that creates the columns never applies it either. - The injection test now asserts the payload is neutralised by that rule; a leading-digit name remains refused. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…ross-table UNION path Thread: #4580 (comment) buildWhereConditionsSql, the raw-SQL builder shared by UNION search, UNION count and UNION facets, skips underscore keys, so a multi-schema query carrying _related or _referenced answered the unfiltered set and its facet counts disagreed with the single-schema facet. It now refuses such a query with InvalidArgumentException (the type the single-table path throws), before building any SQL. Every caller of the builder (UNION search arms in MagicMapper, the count arms, both UNION facet branches in MagicFacetHandler) goes through that one entry point, so all of them are covered; honouring the keys in raw SQL is left for a follow-up. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

… not when shares change or an alert fires Thread: #4580 (comment) The rule "every recipient must be able to read this view" held at one instant on one path. - Delivery (b): ViewAlertCrossedListener re-checks every resolved recipient against the view as it is now (administrator, owner, shares, isPublic; the same ViewShareResolver::accessFor the save uses). A recipient who may no longer read it, or whose groups cannot be read, is dropped and logged. Both channels (nc-notification, email) and both senders of the crossing event (the sweep job is the only dispatcher) go through that one listener. - Save (a): ViewerReachResolver::assertMayNameRecipients also judges the STORED recipients: one who read the view as stored but would not read it as saved is refused (403, listed), so un-publishing or removing a share cannot leave a recipient on an alert about a view they can no longer open. - Judged view (c): on PUT the controller lays the isPublic the save will write (false when the body omits it) over the judged view, so the judged view equals the saved view. PATCH keeps the stored value, as it saves it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…-clock UTC Thread: #4580 (comment) sinceParam returned new DateTime(trim($raw)) and the mapper binds it with PARAM_DATETIME_MUTABLE, which formats the object's own wall clock without converting, so '2026-10-19T12:00:00+02:00' was compared as 12:00:00 instead of 10:00:00Z on GET /api/flow-runs?since= and /api/flow-runs/summary. The parsed moment is converted to UTC before it is bound. Sweep: the summary's default window (-7 days) is normalised the same way, so both bound values reach the mapper in UTC. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

… a table cannot be read Thread: #4580 (comment) findFolderIdsByOrganisation swallowed every per-table exception and returned the folders of the tables it could read, so OrganisationUsageService showed a too-low number as a measured figure, against its own rule that a figure that cannot be measured is null. Each failing table is now logged (warning, with table, organisation and error), the scan continues so every unreadable table is named, and the method then throws a RuntimeException listing them. measureStorage already turns an exception into null, so the answer is null instead of an undercount. Not changed: the scan still visits every magic table per GET /api/organisations/{uuid}; caching or bounding it is left for a decision on the per-request cost. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…ing newline Thread: #4580 (comment) SEGMENT was /^[A-Za-z0-9_\-]+$/, and `$` also matches before a final newline, so `a\n` passed and was written into the JSON path literal, which MariaDB rejects (500 instead of the intended 1 = 0 refusal). It now ends in \z. Class sweep: the same `$`-anchored identifier check in RelatedRowExistsClause::quoteIdentifier and in MagicRbacHandler::buildRbacPredicateForAlias (alias guard) now ends in \z too. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Thread: #4580 (comment) line() capped the count at 50 but stored each message as given, so a node writing one large string per line grew the run log by the string size times 50 per step. A message longer than FlowStepReport::MAX_MESSAGE_LENGTH (500 characters, counted in characters not bytes) is now cut, and the result, marker included, ends in FlowStepReport::TRUNCATION_MARKER. A message at the cap is untouched. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

… and assignable Thread: #4580 (comment) Hidden/not-assignable was set only when the tag was created; findOrCreateTag returned an existing tag untouched, so tags created before the default, or first created by a user in the Files tag UI under the same name, stayed in every user's tag list. findOrCreateTag now repairs a found object: tag that is still visible or assignable with updateTag(userVisible: false, userAssignable: false), keeping its name and colour; this covers both lookup paths (the first search and the TagAlreadyExists retry). A refused update is logged and the tag is still used, so the file link is not lost. Tags that are never attached again are not reached; hiding those needs an upgrade step, which is left to the author. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

The fixes for rel-f1, rel-f2, rel-f4 and rel-f7 pushed four classes over a PHPMD threshold (coupling between objects, class length, method length, constructor parameter count, a boolean flag argument, a missing import). No behaviour change: - RelatedRowAccess takes over the property-read assertion and the organisation predicate from RelatedRowQueryApplier; the applier exposes mayReadProperty() so MagicSearchHandler no longer needs its own guard parameter. - Engine detection moves from the applier to RelatedRowExistsClause::engineFor. - ViewAlertCrossedListener re-checks recipients through ViewerReachResolver::recipientReads (now public), the rule the save already uses, instead of its own copy. - ViewsController::refuseForbiddenViewFields takes the saved body instead of a boolean flag. - phpmd.baseline.xml gains one CouplingBetweenObjects entry for RelatedRowQueryApplier, the established way this repo records a class that legitimately coordinates this many collaborators. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

… RelatedRowQueryApplier The applier sat at the coupling limit only because it called the RBAC handler itself. RelatedRowAccess now owns the buildRbacPredicateForAlias call together with the organisation term (one predicate per related alias), so the applier no longer takes or references MagicRbacHandler and the CouplingBetweenObjects baseline entry added earlier is removed. phpmd passes on a clean export with the baseline as it is on the base commit. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…ted/_referenced takes the sequential path The refusal added in buildWhereConditionsSql is an uncaught InvalidArgumentException on the controller routes (crossTableSearch), so a cross-table search carrying _related or _referenced ended in HTTP 500. MagicMapper::shouldUseUnionQuery now returns false when either key is present. Both the search (dispatchCrossTableSearch) and the count (countAcrossMultipleTables) decide through it, so the sequential path, which builds the EXISTS subqueries, answers both and total and rows agree. The refusal in buildWhereConditionsSql stays as a defensive guard for a caller that bypasses the routing. Not routed: the UNION facet branches (getSimpleFacetsUnion) have no sequential equivalent and still refuse. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…e same 64-character cap as the read path RelatedRowExistsClause::columnName applied every step of the column-naming rule except the 64-character cap MagicSearchHandler::sanitizeColumnName applies (MagicMapper's own copy has none; no shared helper exists), so a property name over 64 characters was filtered on a different column than the read path uses. The cap is applied in the same order (trim underscores, then cut) and pinned in ReferencedRowFilterTest. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…d beyond the default flags Pins the scope the related-row subquery gets for _multitenancy=false (and its string and unreadable spellings), _rbac=false with an RBAC bypass available, an anonymous caller (the empty scope) and a public schema with and without an explicit multitenancy request. Test-only. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…th _related/_referenced answers 400, not 500 Before this round those facets silently ignored the filter and returned wrong counts; a clear 400 is the honest answer. - New RelatedRowFilterNotSupportedException (an InvalidArgumentException) is thrown by MagicSearchHandler::refuseRelatedRowFilters, now public. - MagicFacetHandler::getSimpleFacetsUnion calls it first. The per-facet UNION queries have their own catch blocks that return empty buckets, so the refusal had to come before them or it would have been swallowed. - New RelatedRowFilterMiddleware (registered in Application) answers the exception with HTTP 400 and {"error": <message>}; every other exception is rethrown. A middleware covers every controller that reaches the facets: facets are only reached through getSimpleFacets (MagicMapper), which is called by FacetHandler::getFacetsForObjects (list route with _facets, ViewPresentationService, ObjectMetricSource); there is no facets-only controller route. The UNION search builder keeps raising the same exception as a defensive guard. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

github-actions · 2026-10-19T20:34:52Z

Quality Report — ConductionNL/openregister @ `3a1b425`

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					✅ 26637/26637 · 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-19 20:34 UTC

Download the full PDF report from the workflow artifacts.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

… filtered properties MagicSearchHandlerOrNullModifierTest (from #4697) built the handler with a bare RelatedRowQueryApplier mock, whose mayReadProperty() answers false. Since rel-f1 a nested-path filter whose root property the caller cannot read matches nothing (1 = 0), so testNestedPathAdmitsNullOnBothPaths saw the refusal instead of the orNull predicate. The mock now grants read access, as MagicSearchHandlerNestedPathFilterTest does. Test-only. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

github-actions · 2026-10-19T21:05:19Z

Quality Report — ConductionNL/openregister @ `c1077f7`

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					✅ 26671/26671 · 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-19 21:05 UTC

Download the full PDF report from the workflow artifacts.

WilcoLouwerse and others added 16 commits October 19, 2026 18:32

WilcoLouwerse mentioned this pull request Oct 19, 2026

Release: merge development into beta #4580

Merged

WilcoLouwerse and others added 2 commits October 19, 2026 22:44

Merge branch 'development' into review/pr-4580-fixes-r3

271c693

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

WilcoLouwerse merged commit ceacdc8 into development Oct 19, 2026
48 checks passed

WilcoLouwerse deleted the review/pr-4580-fixes-r3 branch October 19, 2026 21:41

Navigation Menu

Repository navigation

fix(review): #4580 review findings, round 2 - #4696

fix(review): #4580 review findings, round 2#4696
WilcoLouwerse merged 18 commits into
developmentfrom
review/pr-4580-fixes-r3

WilcoLouwerse commented Oct 19, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Oct 19, 2026

Uh oh!

github-actions Bot commented Oct 19, 2026

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

WilcoLouwerse commented Oct 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Findings → commits

Left to the author

Checks run on this branch

Uh oh!

github-actions Bot commented Oct 19, 2026

Quality Report — ConductionNL/openregister @ 3a1b425

Uh oh!

github-actions Bot commented Oct 19, 2026

Quality Report — ConductionNL/openregister @ c1077f7

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

WilcoLouwerse commented Oct 19, 2026 •

edited

Loading

Quality Report — ConductionNL/openregister @ `3a1b425`

Quality Report — ConductionNL/openregister @ `c1077f7`