Repository navigation
Release: merge development into beta - #4580
github-actions[bot] wants to merge 304 commits into
Conversation
…d to carry on later Decision 180 (Ruben): a draft is the destination object with an explicit lifecycle status. @self.status is stored in a new _status column (draft, active; NULL keeps today's date-deduced status). A draft is validated whatever the hard-validation flag, with only required excused, so a wrong type, format or enum value is always refused. A client may create a draft and keep it one; only the form submit, after full validation, leaves draft, answers with the moment of receipt and dispatches ObjectActivatedEvent. Drafts answer to their owner only, on every list and read path, unless the schema sets draftsVisible. POST /api/forms/{id}/draft saves one; submit with _draft leaves it. info.xml version bump so AddObjectStateColumns adds _status.
…r-only, leaving draft
… goes through submit()
…m-destination-validator
… prettier-formatted
…ver rate bands A quantity accrues over ordered rate bands, optionally capped (ADR-118 C16): the shape of a stepped daily penalty, tiered pricing or a bracket table, so it is configuration for each of them. The validator now treats a lit operand as data, so a literal rate schedule is no longer refused as malformed.
metadata.completeVia = {app, action} makes the portal seam refuse a direct
completion with 409 complete-via-app, naming the action, after the party
check and before anything is stored. Only the named app completes the task
in-process (PortalTaskService::complete(viaApp: ...)). A portal reads the
same key off the task row and opens the app's action instead of the upload
form, so an app's own checks (a frozen checklist, a signature, a payment)
cannot be skipped by completing the task through the seam.
Change a-portal-task-names-the-app-that-completes-it.
…, failing closed A property can cite rows of another schema by a key property (referenceSchema + referenceKey, e.g. a code) and refuse their delete with onDelete RESTRICT. An unreadable citation blocks the delete as unverifiable; a citing table that does not exist yet counts as no citation. The property validator accepts the two keys and refuses referenceKey with any other action. uuid references are unchanged. Generic reference guard, capability C25 of hydra's procedures-are-configuration (decision 182).
…them by key Decision 186: a reusable decision table lives in OpenRegister as a generic capability (ADR-118 C15). A core decision-tables register, refused at save when the evaluator cannot run a table or its key is taken; DecisionTableStore resolves a table by key or uuid and evaluates it for apps; the decision-table flow step takes tableRef as an alternative to an inline table.
…and-lifetime feat(objects): a draft's owner makes it definitive, and a schema sets a draft lifetime (decision 216)
…ffects feat(objects): a draft holds its creation side effects until it leaves draft (decision 227)
* feat(rbac): a special right to manage the grants on a record (decision 234) An organisation's special right grant_manage lets its holder list, add and revoke the grants on a record it does not own, and name the owning group. It opens neither the record's contents nor its scope, links or invitations, a holder may not let itself in, and only an administrator or the organisation's owner may assign it. An entry can be narrowed to a register and schemas. * feat(rbac): give a special right in every organisation, for an app's upgrade step * fix(rbac): count the page once, outside the loop condition * docs(rbac): cite the change on grant() and revoke()
…choices
An input may now carry label and options ({value, label}) beside field and
required. publishedInputs() carries both through in every mode, so a provider
that knows the record can publish the choices that hold for it. Presentation
only: the allowlist and the writer's validation are unchanged, and an input
declaring neither key keeps the bare {field, required} shape.
First consumer: dossiq's closing move publishes the case type's result types,
which nc-vue's CnTransitionInputDialog renders as a picker.
…rs-choices feat(lifecycle): a transition input can name its field and offer its choices
…gration-pack-editor # Conflicts: # l10n/nl.json
…-editor feat(migration-packs): view and edit migration packs in OpenRegister
…arvest Task 2.4 is done: the authenticated harvest against real GitHub ran on the WOO-585 rig through a brokered github credential (24 of 24 shards, 698 components on 2026-10-01, 902 on 2026-10-13). The allow-rule requirement joins the main spec credential-broker, the change moves to the archive, and the test's @SPEC tag points at the main spec. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…s its token The GitHub code search refusal scenario said the broker refuses with 'no allow-rule matches'. That string is the logged reason; the caller gets the static 403 'Request not permitted'. Fixed in the main spec and in the archived delta. Task 2.4 no longer says no token exists next to its verification, and names the token the harvest used. Closes review findings: refusal scenario promises a response text the broker never returns; ticked task 2.4 still says no token exists yet. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Lock-only update (composer update symfony/yaml --with symfony/yaml:6.4.48 --no-install), staying on the 6.4 line the lock already used. composer audit --locked: no security vulnerability advisories found. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…-cves fix(deps): symfony/yaml 6.4.48 clears CVE-2026-109413 (Hydra gate 4)
…b-changes chore(openspec): archive github-provider-code-search after the live harvest
| * | ||
| * @spec openspec/changes/draft-owner-activation-and-lifetime/specs/form-destination/spec.md#requirement-a-draft-must-not-be-changed-by-somebody-who-cannot-read-it | ||
| */ | ||
| public function keptFrom(ObjectEntity $object, Schema $schema, ?string $userId): bool { |
There was a problem hiding this comment.
🔴 Bulk save lets any user with update rights overwrite another user's draft
A draft is hidden from other users on every read path (SQL RBAC predicate) and ObjectService::refuseSomebodyElsesDraft() refuses a single-object save over someone else's draft; keptFrom() (this method) is the only guard and it has exactly one caller. The bulk path does neither. SaveObjects::resolveSafeguardActionForRow() finds the existing row with _rbac: false, so the draft is "found" and the row becomes an update; the per-row check is PermissionHandler::hasPermission(action: update, objectOwner: draftOwner, object: draft), which has no draft rule. Any user who may update in the schema can therefore POST /api/bulk/{register}/{schema}/save with the draft's uuid and replace its property values (_status is carried forward, so it stays a draft and the owner finds it changed). Reproduces the exact hole fixed for the single path in 853d562, on the bulk route (BulkController::save is #[NoAdminRequired]).
Fix: In SaveObjects safeguard step, after the existing entity is resolved and $effectiveRbac === true, reject the row (same recordSafeguardRejection) when (new DraftStatusPolicy())->keptFrom(object: $existingEntity, schema: $rowSchema, userId: $currentUser?->getUID()) is true. Keep the answer indistinguishable from "not found/denied" as on the single path.
Done when: a bulk save by user B carrying the uuid of user A's draft (schema without draftsVisible) returns the row as rejected, the draft's stored data and _updated are unchanged, and a bulk save by A (the owner) still updates it. Unit seam: the permission step of SaveObjects::safeguardRows (new test beside tests/Unit/Service/Object/SaveObjectDraftStatusTest.php).
Verification: grep -rn "keptFrom" lib --include=*.php → only ObjectService.php:2339 and the definition; grep -n "Draft\|keptFrom" lib/Service/Object/SaveObjects.php → no draft handling besides ObjectEventGate dispatch
There was a problem hiding this comment.
Tracked in #4648 so it survives the next re-cut of this sync PR.
There was a problem hiding this comment.
🔧 Proposed fix in #4655, commit 728c921 (see the PR body for what changed). The thread stays open until it is merged.
| // the owner's own drafts in view on every path. | ||
| $drafts = new DraftStatusPolicy(); | ||
| if ($schema !== null && $drafts->draftsVisibleToOthers(schema: $schema) === false) { | ||
| $reachable = '(' . $reachable . ' AND ' . $drafts->notADraftSql(columnPrefix: $columnPrefix) . ')'; |
There was a problem hiding this comment.
🟡 A table that could not be healed still gets _status in the RBAC predicate
The self-healing change promises that a table whose metadata columns cannot be added "degrades quietly": MagicSearchHandler leaves the lifecycle predicates out for the columns MagicMetadataColumnHealer::missingFor() reports (REQ-MTH-002). reachableRowSqlFor() does not consult it: for every schema without draftsVisible it ANDs (_status IS NULL OR _status <> 'draft') into the read predicate unconditionally. For a non-admin on a degraded table (no ALTER permission, or an ALTER deferred inside an open MySQL transaction, both cases the healer handles by marking the column missing) the query therefore fails with an unknown-column error, while admins (RBAC bypass) get the quiet answer. The same call also feeds buildRbacPredicateForAlias() for related-row joins.
Fix: Give MagicRbacHandler the healer (or a missingFor(table) callback) and skip notADraftSql() when _status is in the missing list, as draftConditionSql() does; the table cannot hold a draft without the column.
Done when: a unit test with the healer reporting _status missing asserts buildRbacRowPredicateSql() contains no _status for a non-admin, and still contains it when the column exists.
Verification: grep -n "metadataColumns\|missingFor\|MagicMetadataColumnHealer" lib/Db/MagicMapper/MagicRbacHandler.php → no match
There was a problem hiding this comment.
🔧 Proposed fix in #4655, commit 363e607 (see the PR body for what changed). The thread stays open until it is merged.
|
|
||
| // An administrator's re-import is the explicit decision REQ-LCA-003 | ||
| // waits for, for every authorization block either side carries. | ||
| if ($this->decidedVia() !== null) { |
There was a problem hiding this comment.
🟡 Under an administrator's decision, a local authorization block disappears when the shipped schema declares none
With a decision in effect, line 165 adds every authorization root found in live OR incoming to the decisions, so AuthorizationBlocks::settle() turns the block's state into UPSTREAM; foldUpstream() with the block absent from incoming then drops it from the merged definition. Reproduced with the classes directly: baseline {title}, live {title, authorization: {read:[staff], update:[staff]}}, incoming {title} under ShippedAuthorizationDecision::run() gives merged {"title":"T2"} and replacedLocal: ["authorization"]. A schema an administrator locked down locally, in an app whose file declares no authorization for it, becomes default-open after --import (and after an example-set install, which runs the same decision). The design (D-3) only discusses a shipped block replacing a local one; open task 3.3 in the same change is still undecided, and every other change in this release treats an ambiguous authorization as deny.
Fix: Only decide roots the incoming definition carries ($this->blocks->roots($incoming)); a root present only in live keeps its normal state (LOCAL is preserved and reported).
Done when: a GuardedDescriptorMergeTest case with incoming lacking authorization and a local block, run under a decision, keeps the local block; the existing replace-when-shipped-has-it cases stay green.
Verification: php /tmp/claude-1000/-home-wilco-hydra/fe360b05-60b2-49fc-a902-849b84a31cd5/scratchpad/t4580.php (calls GuardedDescriptorMerge with a ShippedAuthorizationDecision) → merged lacks authorization, replacedLocal ["authorization"]
There was a problem hiding this comment.
🔧 Proposed fix in #4655, commit 03e338e (see the PR body for what changed). The thread stays open until it is merged.
| * | ||
| * @spec openspec/changes/form-destination-validator/specs/form-destination/spec.md#requirement-a-list-must-hide-drafts-unless-asked | ||
| */ | ||
| public function sql(string $mode, string $columnPrefix, ?string $owner): ?string { |
There was a problem hiding this comment.
🟡 The draft lens is spelled once for lists but not for the raw-SQL aggregation fast path
AggregationRunner builds its own WHERE (_deleted, _archived predicates, with a comment saying it must reproduce the list path's defaults) and takes the access rule from MagicMapper::rbacRowPredicateSql(). For an administrator that predicate is null, and no draft predicate is added, so a tile or aggregate counts every user's drafts while the list under it (DraftListLens::sql() EXCLUDE mode) hides them. With a schema's draftsListed: false the same mismatch applies to a user's own drafts (the RBAC owner-admit keeps them in the count; the list's NONE lens drops them). The change's own task 2b.8 says list total and facets must agree; aggregates are the third reader and were not wired to DraftListLens.
Fix: Add the draft predicate to the aggregation fast path from DraftListLens::sql() (mode from modeFor() with no query, so EXCLUDE/NONE), next to the _archived predicate, skipped when the table lacks _status.
Done when: a unit test over the aggregation fast path asserts the generated WHERE contains the draft predicate for an admin and that a count over a table with one draft and one active row returns 1.
Verification: grep -n "_status\|DraftListLens" lib/Service/Aggregation/AggregationRunner.php → no match (while _archived is handled at lines 2554-2556)
There was a problem hiding this comment.
🔧 Proposed fix in #4655, commit d4aa263 (see the PR body for what changed). The thread stays open until it is merged.
| * | ||
| * @spec openspec/changes/seed-objects-follow-the-shipped-version/specs/data-import-export/spec.md | ||
| */ | ||
| public function advanceVersion(ObjectEntity $entity, ?string $carried): void { |
There was a problem hiding this comment.
🟢 Any caller who may update an object can set its version to an arbitrary higher value
SaveObject::updateObject now passes @self.version from the request body to advanceVersion(), which stores it when it parses and is higher than the stored one. The seed-import comparison ("update a stored seed only when the shipped version is higher") then reads that object as newer than every later shipped version, so the app's seed fix never reaches it. The caller needs update rights on the object anyway, so this is integrity of the seed channel, not escalation.
Fix: Take the carried version only on the import path (an explicit flag from the seed importer), or ignore @self.version from the HTTP boundary.
Verification: grep -n "carriedVersion" lib/Service/Object/SaveObject.php → 4436-4441, read straight from $selfData
| // Free-form marks a consuming app reads (REQ-ATTR-007), validated at | ||
| // scan time; an empty map is no declaration and is not forwarded. | ||
| if ($attribute->annotations !== []) { | ||
| $taxonomy['annotations'] = $attribute->annotations; |
There was a problem hiding this comment.
🟢 #[McpTool] annotations may carry MCP's reserved hint keys
The free-form annotations map is forwarded in the descriptor under the key annotations, which tools/list hands to external MCP clients unchanged (McpToolsService::listTools() returns descriptors raw), and annotations is the key the MCP specification uses for tool hints (readOnlyHint, destructiveHint, idempotentHint, openWorldHint, title). malformedAnnotation() only checks "non-empty string key, scalar value", so annotations: ['readOnlyHint' => true] on a write tool is accepted and a client that auto-approves read-only tools would trust it, while OpenRegister's own hint and grant checks say otherwise. Authors are trusted code, so this is hardening, not an exploit.
Fix: reject the five reserved MCP hint keys in malformedAnnotation() (or forward the map under a namespaced key such as x-annotations).
Done when: AttributeToolScannerTest asserts a tool declaring readOnlyHint inside annotations is skipped and logged.
Verification: sed -n 395,405p lib/Mcp/AttributeToolScanner.php; sed -n 98,128p lib/Service/Mcp/McpToolsService.php → only key/scalar shape checked; descriptor returned as is
| * GET would, runs the same doelbinding guard, withholds `session`, `request` | ||
| * and `ipAddress`, and removes from each entry's `changed` the properties | ||
| * the caller may not see on the object. That per-object read check is what | ||
| * closes the wave-3 C7 IDOR now, instead of the admin gate: an arbitrary |
There was a problem hiding this comment.
🟡 GET .../{id}/logs still returns the raw audit rows that #4614 says a reader must not get
#4614 makes .../audit-trails readable by anyone who may read the object, and takes care to withhold session, request and ipAddress, to strip from changed the properties the caller may not see, and to run the doelbinding guard. The older route GET /api/objects/{register}/{schema}/{id}/logs (ObjectsController::logs, unchanged, @NoAdminRequired) answers the same question for the same caller: it resolves the object with RBAC and returns ObjectService::getLogs() rows unfiltered. A reader who may see the object but not one of its properties gets that property's old and new values, the IP address and the session id from /logs, so the redaction (and the 'nobody else' sentence in the new docblock and spec) is one URL away from void. The anchored line is the new docblock sentence; the leaking route is lib/Controller/ObjectsController.php::logs() (line 4784).
Fix: Make ObjectsController::logs() delegate to ObjectHistoryForReaders::page() for non-admins (same 404/purpose behaviour), or make it admin-only; keep its response shape if existing UI callers rely on it. Add the route to the spec requirement text so both URLs are named.
Done when: A non-admin who may read an object but not property x calls /logs and gets entries without x, session, request and ipAddress; an unreadable object gives 404.
Verification: sed -n 4780,4790p lib/Controller/ObjectsController.php; grep -n 'function findLogs' -A20 lib/Service/Object/GetObject.php | grep -n 'findAll' → ObjectsController::logs() is #[NoAdminRequired]-style, resolves the object with RBAC only and returns AuditTrailMapper::findAll() rows raw: no WITHHELD_FIELDS, no HistoryEntryRedactor, no PurposeGuard
Severity: ObjectsController::logs() exists unchanged on beta, so the exposure is not new in this promotion. It is filed because #4614 now establishes that object history is redacted for non-admin readers, and this sibling route still returns the raw rows.
There was a problem hiding this comment.
🔧 Proposed fix in #4655, commit 4e900c4 (see the PR body for what changed). The thread stays open until it is merged.
| * | ||
| * @var string | ||
| */ | ||
| private const USER_SETTINGS_HASH = '#user-settings'; |
There was a problem hiding this comment.
🟡 #4587 moved the per-person settings into the user settings dialog but left the pointers to the old place
#4587 deletes OpenRegisterPersonal, personalSettings.js and the <personal> entry in info.xml, so Nextcloud's personal settings page no longer shows anything of OpenRegister. These live copies still point there: (1) the 'Reconnect your account' notification (Notifier.php::prepareCredentialRelinkNeeded) says 'open Connected accounts in your personal settings' and its link is settings.PersonalSettings.index?section=additional, so a person whose connection broke lands on a page without the account list; (2) the admin hint in PushNotificationsConfiguration.vue:107 ('Users can opt in ... from their personal settings', with its nl.json translation); (3) the requirement 'A person can connect, see and repair a connection from personal settings' in openspec/specs/credential-oauth2-connect/spec.md:208; (4) stale header comments in BrowserNotificationsSection.vue:13 ('TODO mount ... registers NO PHP Personal settings class') and OAuth2Endpoints.php:20. The anchored line is the new hash constant; the notification link is on Notifier.php:628, not a changed line.
Fix: Point the notification link at the app page with #user-settings (as the OAuth callback now does) and reword the message and the admin hint to 'user settings'; update the spec requirement text and the two comments; update l10n keys with the new English strings.
Done when: git grep -n -i 'personal settings' over lib/, src/ and openspec/specs/ returns no live copy that describes where OpenRegister's per-person settings are, and the relink notification link opens the dialog.
Verification: git grep -n "settings.PersonalSettings.index\|in your personal settings\|from their personal settings\|personal settings SHALL\|No PHP Personal settings\|NO PHP Personal settings" -- lib src openspec/specs → lib/Notification/Notifier.php:621,628; src/views/settings/sections/PushNotificationsConfiguration.vue:107; openspec/specs/credential-oauth2-connect/spec.md:208; src/components/userSettings/BrowserNotificationsSection.vue:13; lib/Service/Credential/OAuth2Endpoints.php:20; l10n/nl.json:63
There was a problem hiding this comment.
🔧 Proposed fix in #4655, commit 5faeef9 (see the PR body for what changed). The thread stays open until it is merged.
| // into this schema against what it is about to become. An owning app | ||
| // set to `refuse` blocks the save; otherwise the broken forms are | ||
| // unpublished after the save, and the response lists them. | ||
| $formAssessment = $this->assessDependentForms(existing: $existingSchema, data: $data); |
There was a problem hiding this comment.
🟡 The 'saving a schema re-checks its forms' rule holds for SchemasController and nothing else
The spec says 'When a schema's properties or required list are saved, OpenRegister SHALL dispatch FormDestinationDependentsEvent'. The dispatch lives in SchemasController::applyUpdate() only (publishing a draft goes through it too). Schemas are also written by the configuration importer (Service/Configuration/ImportHandler, which is how an app upgrade ships a new required property), SchemaImportController, the AI SchemaTool and the MCP SchemasToolProvider; none of them assess the published forms, so a form can keep published and then 422 every resident at submit time with nobody notified. Separately, formDependentsCheck() swallows any Throwable from the container and returns null, which silently skips an owning app's refuse setting without a log line.
Fix: Move the assess/apply step into one service called by every schema write that changes properties or required (or state in the spec that only the controller path is covered), and log at warning level when the check cannot be built.
Done when: A schema import that adds a required property to a destination schema unpublishes (or refuses, per formDestinationBreak) the dependent forms; a container failure is logged.
Verification: grep -rn 'FormDependentsCheck\|FormDestinationDependentsEvent' lib --include=*.php -l; grep -rln 'schemaMapper->update\|updateFromArray' lib --include=*.php → FormDependentsCheck is referenced only by SchemasController; ImportHandler, SchemaImportController, SchemaTool, SchemasToolProvider, SchemaService also update schemas; formDependentsCheck() returns null on any Throwable and the check is skipped without a log line
There was a problem hiding this comment.
🔧 Proposed fix in #4655, commit 1f4eafb (partly: the PR body says what is left). The thread stays open until it is merged.
| * | ||
| * @spec openspec/changes/special-right-to-manage-grants/specs/object-ownership/spec.md | ||
| */ | ||
| public function letsItselfIn(?string $admittedAs, ?string $userId, array $userGroups, ?string $user, ?string $group): bool { |
There was a problem hiding this comment.
🟡 The "may not let itself in" check ignores remote principals
letsItselfIn() compares only the user and group grantee against the caller, but ObjectSharingService::GRANTABLE_TYPES also accepts remote and remote_group (ObjectSharingService.php:94-95). A grant_manage holder can therefore grant read to its own federated cloud id, or a remote group, and so read the record by one extra call, which is exactly what the docblock above says the guard prevents. The grant also outlives a rotation of the holder's own credentials.
Fix: refuse a holder-admitted remote / remote_group grant outright (no way to prove it is not the caller), or limit GRANTABLE_TYPES for holders to user and group.
Done when: a unit test in GrantManageSpecialRightTest grants remote as a holder and asserts the refusal.
Verification: sed -n 123,133p lib/Service/Rbac/GrantManageGuard.php; sed -n 91,96p lib/Service/Rbac/ObjectSharingService.php → only $user/$group compared; remote/remote_group grantable
Found by the persistence audit that ran alongside this review.
There was a problem hiding this comment.
🔧 Proposed fix in #4655, commit e19745b (see the PR body for what changed). The thread stays open until it is merged.
There was a problem hiding this comment.
Verdict: REQUEST_CHANGES (Strict)
Not ready for beta: six blockers in code that reached development without any review, most in the new forms, drafts, frozen-file and deferral work. Under Strict every 🟡 below blocks as well.
🔧 Fix PR: #4655 — 23 of 33 findings, one commit each
Merge it into development, then re-request the review. The other findings stay on their threads.
🔴 Blockers (6)
- Bulk save overwrites another user's draft —
lib/Service/Object/DraftStatusPolicy.php:295(introduced in #4566, #4600). A draft is hidden from other users on every read path (SQL RBAC predicate) andObjectService::refuseSomebodyElsesDraft()refuses a single-object save over someone else's draft;keptFrom()(this method) is the only guard and it has exactly one caller. Tracked in #4648. (fix proposed in #4655) - A required file property can never be satisfied by an upload token —
lib/Service/Form/FormSubmitService.php:644(introduced in #4563).mapPayload()moves an upload token's file into$plan['files']and leaves the property out of$plan['object']. Tracked in #4646. (fix proposed in #4655) - Review policy fails open for visibility when it cannot be read —
lib/Service/Decision/ReviewPolicyResolver.php:98(introduced in #4595). When the schema cannot be read, the resolver returnsownership: deciderwith no supervisors and calls that "fail closed", buthideOthersstays false, soDecisionVisibility::isHidden()returns false and another reviewer sees the verdict, bases and decisio Tracked in #4647. (fix proposed in #4655) - Frozen-file guard ignores writes made through a share —
lib/Listener/FrozenNodeWriteListener.php:172(introduced in #4549).refusalFor()returns null unless$node->getPath()starts with/openregister/files/. Tracked in #4649. (fix proposed in #4655) - Any admin-UI file-settings save clears the instance review default —
lib/Service/Settings/FileSettingsHandler.php:259(introduced in #4595).updateFileSettingsOnly()rebuilds the wholefileManagementvalue from the request and writesReviewPolicy::SETTINGS_KEY => $this->reviewDefault($fileData), which isnullwhen the request has noanonymisationReviewkey. Tracked in #4650. (fix proposed in #4655) - Deferred-chunk limit is 32000, but NC 32 and 33 accept 4000 —
lib/Service/Deferral/ListenerDeferralService.php:82(introduced in #4616).MAX_ARGUMENT_LENGTH = 32000is documented asOC\BackgroundJob\JobList::MAX_ARGUMENT_JSON_LENGTH. Tracked in #4651. (fix proposed in #4655)
🟡 Concerns (19)
- RBAC draft predicate ignores unhealed _status —
lib/Db/MagicMapper/MagicRbacHandler.php:359(introduced in #4566, #4610). The self-healing change promises that a table whose metadata columns cannot be added "degrades quietly":MagicSearchHandlerleaves the lifecycle predicates out for the columnsMagicMetadataColumnHealer::missingFor()reports (REQ-MTH-002). (fix proposed in #4655) - Forced re-import deletes local authorization the file lacks —
lib/Service/ShippedBaseline/GuardedDescriptorMerge.php:165(introduced in #4618). With a decision in effect, line 165 adds every authorization root found inliveORincomingto the decisions, soAuthorizationBlocks::settle()turns the block's state into UPSTREAM;foldUpstream()with the block absent fromincomingthen drops it fro (fix proposed in #4655) - Aggregation fast path counts drafts the list hides —
lib/Service/Object/DraftListLens.php:175(introduced in #4566, #4600).AggregationRunnerbuilds its own WHERE (_deleted,_archivedpredicates, with a comment saying it must reproduce the list path's defaults) and takes the access rule fromMagicMapper::rbacRowPredicateSql(). (fix proposed in #4655) - Upload endpoint ignores the form's audience —
lib/Controller/FormsController.php:274(introduced in #4563).submit()answers 401 when the visitor is anonymous and the form's audience is notpublic, butupload()(also#[PublicPage]) never reads$form['audience']. (fix proposed in #4655) - Idempotency key is neither subject-bound nor race-safe —
lib/Service/Form/FormSubmitService.php:268(introduced in #4563). The key scope is the form id only, so on a public form anyone who presents the sameIdempotency-Keywithin 24 hours gets the first submitter's stored answer (reference,id, and everyconfirmationproperty, which can be personal data). (fix proposed in #4655) - Draft cleanup deletes without re-checking the draft —
lib/Service/Object/DraftExpiry.php:368(introduced in #4600).expiredIn()reads every page of expired draft uuids first;remove()then deletes them one by one (up to 1000 per run) with_rbac: falseand no re-read. (fix proposed in #4655) - Dependent form reported unpublished when nothing was unpublished —
lib/Service/Form/FormDependentsCheck.php:158(introduced in #4563).unpublish()calls the closure only when$form->unpublish !== nulland otherwise falls through to notify the author and returnunpublished. (fix proposed in #4655) - Task wake still overwrites another listener's signal —
lib/Service/Flow/FlowTaskBridge.php:369(introduced in #4541). The docblock says the signal now carries the outcome bag "so it cannot erase what another listener on the same event signalled". (fix proposed in #4655) - Decision ownership check is skipped in two cases —
lib/Service/Decision/RelationDecisionService.php:106(introduced in #4595).decide()reads$currentand runs the ownership refusal only when$current !== null, outside the transaction. (fix proposed in #4655) - Failure to arm a statutory expiry timer is only logged —
lib/Service/Case/CasePlanItemTimers.php:219(introduced in #4544).arm()catches every Throwable fromFlowTimerService::armand returns false, andonEntered()just counts. (fix proposed in #4655) - Email recipient reference may point at any object in the organisation —
lib/Service/Notification/ReferencedAddressNotifier.php:308(introduced in #4559).findReferenced()reads the referenced object with_rbac: falseand_multitenancy: falseby id only;sameTenant()then requires equal organisation, and passes for any object with no organisation. - Aborted delete or rename reads as success to internal callers —
lib/Listener/FrozenNodeWriteListener.php:123(introduced in #4549). For delete and rename the listener callsabortOperation(). (fix proposed in #4655) - A later listener's allow() overrides an earlier refusal —
lib/Event/DecisionChangeRequestedEvent.php:129(introduced in #4595).DecisionChangeRequestedEventis a vetoable question ("may this user change this decision?"), butallow()setsallowed = trueand clears the reason regardless of an earlierrefuse(). (fix proposed in #4655) - Object GET /logs bypasses the audit-history redaction —
lib/Controller/AuditTrailController.php:569(introduced in #4614). #4614 makes.../audit-trailsreadable by anyone who may read the object, and takes care to withholdsession,requestandipAddress, to strip fromchangedthe properties the caller may not see, and to run the doelbinding guard. (fix proposed in #4655) - Copies still send users to the removed personal settings form —
lib/Controller/CredentialOauth2Controller.php:102(introduced in #4587). #4587 deletesOpenRegisterPersonal,personalSettings.jsand the<personal>entry ininfo.xml, so Nextcloud's personal settings page no longer shows anything of OpenRegister. (fix proposed in #4655) - Dependent-form re-check runs on one schema write path only —
lib/Controller/SchemasController.php:1281(introduced in #4563). The spec says 'When a schema's properties or required list are saved, OpenRegister SHALL dispatch FormDestinationDependentsEvent'. (fix proposed in #4655) - The "may not let itself in" check ignores remote principals —
lib/Service/Rbac/GrantManageGuard.php:123(introduced in #4601).letsItselfIn()compares only theuserandgroupgrantee against the caller, butObjectSharingService::GRANTABLE_TYPESalso acceptsremoteandremote_group(ObjectSharingService.php:94-95). (fix proposed in #4655) - Delegated grant changes leave no audit-trail row —
lib/Service/Rbac/ObjectOwnershipService.php:366(introduced in #4601), no changed line within reach, so no thread. Agrant_manageholder can grant, revoke and set the owning group, and an admin can change the organisation'sgrant_managelist, but none of those writes an OpenRegister audit-trail entry:ObjectSharingServicehas no audit call at all,setOwnerGroup()writes the block and returns, andOrganisationController::updaterecords nothing for the right.reassign()in the same class does write one (ownership_handover, viaAuditTrailMapper::createAuditTrail), so who widened access to a record is the one thing the trail cannot answer. Fix: write an audit row (actor, object or organisation, principal before/after) for grant, revoke, owning-group change andgrant_managelist change, the wayownership_handoveris written. Done when:grep -n -i audit lib/Service/Rbac/ObjectSharingService.phpfinds the write, and a test asserts the row per action. Verification:grep -c -i -E "auditTrail|AuditTrailMapper" lib/Service/Rbac/ObjectSharingService.php→ 0; ObjectOwnershipService.php:95AUDIT_ACTION = 'ownership_handover'used only byreassign()Found by the persistence audit that ran alongside this review. - Guard failure imports shipped authorization unguarded —
lib/Service/ShippedBaseline/ShippedConfigurationGuard.php:201(introduced in #4618), no changed line within reach, so no thread. When the guard throws, the catch returnsdefinition => $incomingwithguarded: false: the app's shipped authorization block replaces whatever the administrator set on this instance, which is the one thing the guard exists to prevent. The comment calls that "a known state", but for anauthorizationblock it is fail-open towards the shipped policy. Authorization changes taken because only the app changed them (applied) are also never written to the audit trail. Fix: on guard failure keep the instance's currentauthorization(or skip importing that key) and log; recordappliedauthorization changes on the trail asrecordReplaceddoes for explicit decisions. Done when: a test makes the guard throw and asserts the instance's authorization block is unchanged after the import. Verification:sed -n 201,216p lib/Service/ShippedBaseline/ShippedConfigurationGuard.php→ catch returns'definition' => $incoming, 'guarded' => falseFound by the persistence audit that ran alongside this review. (fix proposed in #4655)
🟢 Minor (8)
- Client-supplied @self.version raises stored version —
lib/Service/Object/ObjectVersionHandler.php:158(introduced in #4635).SaveObject::updateObjectnow passes@self.versionfrom the request body toadvanceVersion(), which stores it when it parses and is higher than the stored one. - Direct-invoke caller config is not limited to the node's config keys —
lib/Service/Flow/FlowRunService.php:1090(introduced in #4553).executeNode()merges the caller'sconfigover the published step's config with caller keys winning, without intersecting withIFlowNodeConfigKeys::configKeys(). - Upload tokens: orphan .bin never purged, size unbounded without maxSize —
lib/Service/Form/FormUploadStore.php:133(introduced in #4563).issue()writes<token>.binand then<token>.json;purge()walks.jsonfiles only, so a.binwhose.jsonwrite failed is never deleted. - Remove restores a change's path even if edited since —
lib/Service/ExampleSet/ExampleSetChanges.php:251(introduced in #4603, #4632).ExampleSetChanges::revert()writes the recorded old value back (or removes the path) without checking that the object still holds what the set wrote. - Organisation lookup errors read as gone or as absent —
lib/Service/ExampleSet/ExampleSetInstance.php:159(introduced in #4603).delete()treats anyThrowablefromfindByUuid()as "already gone" and returns true, so a database error makes remove forget the organisation uuid while the organisation stays. - Listener builds MagicMapper on every file write instance-wide —
lib/AppInfo/Application.php:2995(introduced in #4549).FrozenNodeWriteListeneris registered on four node events that fire for every file operation on the instance (all apps, thumbnails, uploads). - --check help says it writes nothing, but it imports registers —
lib/Command/ExampleSetInstallCommand.php:86(introduced in #4603). The--checkoption is described as "Validate every object against its schema and write nothing". - #[McpTool] annotations may carry MCP's reserved hint keys —
lib/Mcp/AttributeToolScanner.php:303(introduced in #4554). The free-formannotationsmap is forwarded in the descriptor under the keyannotations, whichtools/listhands to external MCP clients unchanged (McpToolsService::listTools()returns descriptors raw), andannotationsis the key the MCP specification
✅ Verified clean
- The five new migrations (
Version1Date20261009150000…20261011140000): idempotent guards, additive and nullable columns, no two PRs on one table. - #4589: a roles block that resolves to nothing grants nothing (
PermissionHandler.php:3622, fail-closed). - #4601: non-admins cannot change the organisation's
grant_managelist; patch routes through update. - Draft privacy on the single-object read and write paths (
MagicRbacHandler.php:336-363,ObjectService.php:2321); the bulk path is the exception (🔴 above). - #4571 graduated/bandRate: band boundaries, zero, negative quantity and the open last band match the spec and are asserted.
- OAuth2 connect:
safeReturnUrl()still blocks open redirects; the archived spec deltas are synced intoopenspec/specs; everyinfo.xmljob, command and repair class exists. - The beta release computes its version from tags, so the stale
info.xmlversion ondevelopmentis not a defect.
Scope decision: promotion of 296 commits, 0 direct. 37 merged PRs: 34 by rubenvdlinde with no review at all, read at full depth in four slices (data/RBAC/migrations, process engines, platform services, controllers/frontend/specs) plus an integration pass over the 59 files touched by two or more PRs; 3 by WilcoLouwerse (#4602, #4642, #4643), trusted on their earlier self-review after a spot-check (no open threads). Residual: the 53 files only those three touched were not re-read; per slice, most test files, l10n and parity JSON were skimmed, not read.
Gates: quality / Hydra Gates and quality / Quality Report green on b9e5d8df; what they cover (lint, licence headers, l10n parity, unit suite) was not re-checked.
Persistence audit: run on this checkout; its grant-related findings are posted above (remote grantees, missing audit rows, the shipped-authorization guard). Two audit items were refuted against the spec and dropped (a holder naming another owning group; a stored form without a mapping). LOW items not posted: occ openregister:example-set:install has no environment guard; the portal assertion uses one HS256 secret with no kid and no jti record; exports with a null retentionDays are kept forever.
Verification: every 🔴 was re-checked on the head by reading the code (nothing was run: no vendor tree, no rig). The Nextcloud 32/33 job-argument limit (plat-f3) was read from the server tags v32.0.0, v33.0.0 and v34.0.0.
…ly as a mock
A fresh instance refused the zuiddrecht set: brp and kvk are mock-only
descriptors, and the real-configuration re-import correctly refuses a mock.
A requires entry now names the descriptor it wants: a slug for the real
one, {"register": "<slug>", "mock": true} for the mock, imported through
reimportMock() under openregister.mock and recorded for remove as before.
A plain slug never falls back to the mock. zuiddrecht declares brp and kvk
as mock requirements.
…erty (#4656) ObjectTreeService walks descendants, ancestors or both along the parent property a schema already declares (x-openregister-hierarchy.parent, else x-openregister-party.parentProperty), one query per level, with levels, cycle protection, the declared depth, the requested depth and the node cap, and the caller's read access on every node. Exposed as GET /api/objects/{register}/{schema}/{id}/tree. Decision 250.
… and nothing else (#4657) BrpPersonProvider::countResidentsAtAddressObject() asks Haal Centraal ZoekMetAdresseerbaarObjectIdentificatie with one field and answers { total, meta }. It asks only when the brp-haalcentraal source carries configuration.residentCount: true, and the call reaches the connector with withholdBodies so the residents' numbers are never stored. The router gains sourceConfiguration() and the withholdBodies call option. This is the openregister half of portaliq identity-registered-details T07. Assisted-by: ClaudeCode:claude-opus-5-5
…ock-register fix(example-sets): a set may require a register OpenRegister ships only as a mock
…roperty, and a config field may be a dot path The rehearsal R2 install logged 16,038 'meta is not a valid attribute' errors: shillinq's _meta property has a _meta column, and every magic-table reader took each _ column for object metadata, so MagicStatisticsHandler failed the whole row and the other readers dropped the property. MagicMetadataColumn now decides: a property the schema declares is a property unless ObjectEntity has that attribute. An unknown metadata column is skipped and noted once at debug. opencatalogi's publiccode schema was rejected on every import because the save-time check compared description.nl.longDescription with the top-level keys; the reader walks dot paths, so the check now accepts a path whose first segment is a property. example-set --check counted changes into the per-schema object count (12 dossiq cases against 10 declared); changes are now counted and printed apart.
… bulk writer asks it too check:strict's phpmd flagged static access, a long variable name and the bulk writer's method length; the write-side rule moves into MagicMetadataColumn::skippedOnWrite().
fix(magic-tables,example-sets): underscore properties read as properties, dot-path config fields, check counts like install
This branch has not been deployed
2 participants
Automated PR to sync development changes to beta for beta release.
Merging this PR will trigger the beta release workflow.