Implement validate-self-folder-access: HTTP 403 + audit on cross-tenant @self.folder writes (#1342)

[rjzondervan]

Summary

Cross-tenant @self.folder writes are now denied with HTTP 403, audited, and surfaced through a dedicated FolderAccessDeniedException. Closes #1342.

Before this change, an authenticated caller could POST an object with @self.folder: "<another-user's-folder-id>" and silently bind it — and any child files written to that object — to a folder they had no permission to read. The hardening shuts that escape hatch and produces a forensic audit trail on every denial.

What changed

Exception type

• New OCA\OpenRegister\Exception\FolderAccessDeniedException extending \Exception directly (not NotPermittedException) — so generic catch blocks can't absorb a denial. Carries the attempted folder ID for the structured response body.

Service-layer access check

• FolderManagementHandler::assertFolderIsAccessible() (public, was private — see "implementation gaps" below) resolves via the user's user-folder mount and verifies Folder::isReadable(). Default-deny: any condition that doesn't end in a positive readability confirmation results in FolderAccessDeniedException.
• FolderManagementHandler::logFolderAccessDenied() writes a forensic audit-trail entry before propagating the exception (best-effort: a mapper failure logs at warning level but never swallows the denial).
• "Self" is precisely defined: explicit IUser $currentUser argument first, then IUserSession::getUser(), otherwise deny. No implied identities.

Controller mapping

• ObjectsController::create(), ::update(), ::postPatch() catch FolderAccessDeniedException (before generic \Exception) and return:

  HTTP 403
  { "error": "folder_access_denied", "folder": "<attempted-id>" }

  via a shared private helper folderAccessDeniedResponse().

getNodeById() retains the root-folder fallback for non-binding callers (anonymous file reads) — that path is untouched.

Implementation gaps revealed during verify (and fixed)

Live API testing surfaced three things the spec implicitly assumed but turned out not to be true in the existing codebase. All three are fixed in this PR:

1. @self.folder was never propagated to the entity. SaveObject::setSelfMetadata() only whitelisted slug/owner/organisation/tmlo — folder was silently dropped from the request. Folder propagation is now added (with the access check inline).
2. createObjectFolderById() doesn't run on the HTTP create path. Folder creation is lazy (file-op triggered). To make the access check govern HTTP saves the check moved into setSelfMetadata so it fires at the point folder lands on the entity, before persist. assertFolderIsAccessible() is now public; FolderManagementHandler is injected into SaveObject.
3. FolderAccessDeniedException was swallowed by three catch-all blocks — FolderManagementHandler::createEntityFolder, FileService::createEntityFolder, and ObjectService::ensureObjectFolderExists all caught generic \Exception and returned null. Each now has an explicit catch (FolderAccessDeniedException) { throw $e; } before the generic catch so the controller's 403 mapping wins.

Live smoke test (executed inside master-nextcloud-1)

=== Scenario A: bob → cross-tenant folder 215 (alice's) ===
{"error":"folder_access_denied","folder":"215"}
HTTP 403 ✓

=== Scenario B: bob → no @self.folder ===
HTTP 201 ✓ (auto-create deferred)

=== Scenario C: bob → own folder 227 ===
HTTP 201 ✓

Audit row in oc_openregister_audit_trails:

id: 186 | action: folder_access_denied | user: bob | register: 1 | folder: "215" | reason: "not found in user folder mount"

Quality

Tool	Status
PHPCS	✅ 0 errors
PHPMD	✅ no new warnings (1 ExcessiveClassLength suppressed at class level with rationale)
Psalm	✅ 0 errors on touched files
PHPStan	✅ 0 errors on touched files (2 fixed during section 9)
PHPUnit (targeted)	✅ 347/347 tests green inside master-nextcloud-1
API smoke (live)	✅ all 3 scenarios

Test plan

• CI quality runs green
• Manual smoke: in the local stack, repeat the three scenarios above with two different users
• Manual: verify audit-trail row exists for each denial
• Manual: confirm a user binding to their own folder still works (not over-restrictive)
• Optional destructive test: temporarily break AuditTrailMapper (e.g. mock the connection) and verify the denial still propagates as 403 — covered by testAuditFailureDoesNotSwallowDenial unit test, included for completeness

Files

New:

• lib/Exception/FolderAccessDeniedException.php
• tests/Unit/Exception/FolderAccessDeniedExceptionTest.php
• tests/Unit/Service/File/FolderManagementHandlerAccessControlTest.php

Modified:

• lib/Service/File/FolderManagementHandler.php — assertFolderIsAccessible(), logFolderAccessDenied(), audit-mapper DI, exception re-throw
• lib/Service/Object/SaveObject.php — FolderManagementHandler DI, folder propagation + check in setSelfMetadata
• lib/Service/FileService.php, lib/Service/ObjectService.php — exception re-throw before generic catch
• lib/Controller/ObjectsController.php — 403 catches in 3 endpoints + shared response helper
• lib/AppInfo/Application.php — DI registration update
• docs/api/objects.md — access-control contract section
• CHANGELOG.md — Breaking entry
• 5 existing SaveObject test files updated for the new constructor arg

🤖 Generated with Claude Code

[github-actions]

Quality Report — ConductionNL/openregister @ b285217

Check	PHP	Vue	Security	License	Tests
lint	✅
phpcs	❌
phpmd	✅
psalm	✅
phpstan	✅
phpmetrics	✅
eslint		✅
stylelint		✅
composer			✅	✅ 147/147
npm			✅	✅ 598/598
PHPUnit					⏭️
Newman					⏭️
Playwright					⏭️

---

Quality workflow — 2026-05-05 14:20 UTC

Download the full PDF report from the workflow artifacts.

[WilcoLouwerse]

🔴 Blocker — Double access-check with different currentUser contexts — duplicate audit trail + inconsistent enforcement

SaveObject::setSelfMetadata() (line ~3468, added in this PR) calls $this->folderManagementHandler->assertFolderIsAccessible(folderId: $folderValue, objectEntity: $objectEntity) without passing currentUser. This falls back to IUserSession::getUser() (the session user).

FolderManagementHandler::createObjectFolderById() (line ~250, also added in this PR) also calls assertFolderIsAccessible(folderId, currentUser: $currentUser, objectEntity) — using whatever $currentUser was passed to createObjectFolderById().

For HTTP paths this is currently the same session user so both agree. But:

1. Duplicate audit entries: on every successful bind the first call (in setSelfMetadata) runs the full assertFolderIsAccessible including logging on success path; if the folder is accessible both calls pass silently but they each hit rootFolder->getUserFolder and getById — two reads, no harm BUT on denial the first call throws immediately from setSelfMetadata, so the second call in createObjectFolderById is never reached. OK so far.
2. Inconsistent user on the createObjectFolderById path: the PR description states createObjectFolderById is NOT on the HTTP create path (lazy folder init). If it IS ever called on the HTTP path by some code path not yet identified, the currentUser passed to createObjectFolderById could differ from the session user used in setSelfMetadata. These two checks would then use different users to authorize the same bind — one check could pass while the other would deny, depending on which one fires.
3. More critically — silent bypass via update path: If a caller calls objectEntity->setFolder(id) directly (bypassing setSelfMetadata) and then calls createObjectFolderById with a privileged explicit $currentUser, the setSelfMetadata check is skipped entirely. The only guard then is the createObjectFolderById check. This is architecturally fragile.

The two checks should be consolidated into a single enforcement site. The setSelfMetadata check is the correct early-abort point for HTTP saves. The createObjectFolderById check provides defense-in-depth but uses a different user resolution path. The design needs either: (a) remove the assertFolderIsAccessible call from createObjectFolderById and rely solely on setSelfMetadata plus a contract that only pre-validated IDs reach createObjectFolderById, OR (b) pass currentUser explicitly through setSelfMetadata → assertFolderIsAccessible so both enforcement sites use the same user object.

Impact: On HTTP paths: currently both checks use session user so the enforcement is effectively identical. But the architecture is internally inconsistent, and any code path that calls createObjectFolderById directly without going through setSelfMetadata would only get the second check (with its explicit $currentUser). This creates a design that is correct today but fragile — a future caller that sets folder on the entity and calls createObjectFolderById with a privileged system user bypasses the user-identity check entirely.

Suggested fix: In setSelfMetadata, either (a) pass the result of $this->folderManagementHandler->assertFolderIsAccessible(...) and do NOT set the folder if it throws (current code already does this correctly since it throws), and remove the redundant check from createObjectFolderById, adding a doc comment that createObjectFolderById trusts pre-validated IDs; OR (b) add a ?IUser $currentUser = null parameter to setSelfMetadata and thread it through to assertFolderIsAccessible. At minimum add a code comment documenting why two enforcement sites exist and which is authoritative.

[WilcoLouwerse]

🔴 Blocker — update() and patch() controller 403 paths have no test coverage — only create() is tested

The PR adds FolderAccessDeniedException catch blocks to three controller methods: create(), update(), and patch() (the postPatch endpoint). Only testCreateReturns403WithStructuredBodyOnFolderAccessDenied (added at line ~1493) tests the create path.

No equivalent tests were added for update() and patch(). The PR description at task 7.3 explicitly says only one test was added for create. This means the catch (FolderAccessDeniedException) block in both update() and patch() are untested at the controller level — a copy-paste or ordering mistake in those blocks (e.g., catch placed after the generic \Exception catch instead of before) would be undetected by the test suite.

Strict-mode escalation: absence of tests for security catch-block ordering on two of three covered endpoints is a blocker for a security-sensitive PR.

Impact: If the catch-block order in update() or patch() is wrong (FolderAccessDeniedException caught after generic \Exception), a cross-tenant bind attempt via PUT/PATCH silently returns a 403 with a non-structured body (generic exception message) rather than the specified {error: folder_access_denied, folder: id} body — or worse, if the generic catch returns a different status, the denial leaks internal exception messages.

Suggested fix: Add testUpdateReturns403WithStructuredBodyOnFolderAccessDenied and testPatchReturns403WithStructuredBodyOnFolderAccessDenied following the same pattern as the create test. Three lines of setup each.

[WilcoLouwerse]

🔴 Blocker — 403 body echoes attempted folder ID — folder existence oracle / enumeration vector

The folderAccessDeniedResponse() helper (line ~3490) returns:

{"error": "folder_access_denied", "folder": "<id>"}

where folder is the caller-supplied numeric node ID echoed back verbatim. This is documented in docs/api/objects.md and the CHANGELOG as the specified response shape.

The problem: the fact that the server returns 403 (rather than 404) already confirms the folder exists in the Nextcloud instance. An attacker can enumerate valid folder node IDs across the instance by probing @self.folder with sequential integers: 403 means the folder exists (just not accessible), whereas 404 / auto-create would mean it doesn't. The folder field in the body additionally confirms which ID was checked, providing round-trip confirmation of the node ID (not necessary — the caller already knows the ID they sent).

The response body folder field adds no information for legitimate callers (they know which ID they sent) but is explicitly documented as echoing the attempted ID, cementing the enumeration oracle into the public API contract.

Strict-mode escalation: uncertain whether the 403 vs 404 distinction is an acceptable design tradeoff (HTTP semantics: 403 = exists but forbidden; 404 = does not exist or forbidden to reveal existence). Since this is a security-sensitive PR and the spec explicitly documents the 403 shape, the lack of a "404 for unknown IDs, 403 only for known-but-inaccessible" decision is uncertain → escalated to blocker.

Impact: An authenticated tenant can enumerate all valid folder node IDs on the Nextcloud instance by brute-forcing @self.folder values. For multi-tenant installs this reveals the existence of other users' folders.

Suggested fix (options): (a) Return 403 for any numeric folder ID regardless of whether it exists — do not distinguish "not found" from "not readable" in the HTTP response. Return {"error": "folder_access_denied"} without the folder field, since the caller knows which ID they tried. (b) Alternatively, document this as an accepted risk in the spec with a justification (e.g., node IDs are not secret, or the attack requires authentication), which would downgrade this to a concern rather than a blocker. If the decision was made and documented, the reviewer needs to see it.

[WilcoLouwerse]

🔴 Blocker — phpcs CI failure unresolved — PR is non-green on PHP Quality gate

CI shows quality / PHP Quality (phpcs) is failing. The tasks.md notes "PHPCS 0 errors" for the author's local run, but the CI gate disagrees. This could be:

• A PHPCS configuration mismatch between the author's local composer check:strict and the CI phpcs ruleset.
• A newly introduced violation in one of the 21 touched files that the author's local tooling missed.
• A pre-existing violation in a file touched (but not caused) by this PR that the CI ruleset catches on the diff.

For a security-sensitive PR with 21 files changed, a failing quality gate is a blocker — quality gate failures sometimes mask real security-relevant issues (e.g., unused catch variables, unreachable code after a throw) that the linter catches but the author dismissed.

Impact: The PR cannot be merged with a failing CI gate. The failing gate also means the quality claims in tasks.md step 10 are inconsistent with CI output.

Suggested fix: Identify the specific phpcs violation reported by CI (check the Actions log), fix it, and re-run CI to green. If the violation is pre-existing and unrelated to this PR, document it explicitly in the PR description and add a suppression comment with a justification.

[WilcoLouwerse]

(adjusted from line 400 to nearest hunk line 277)

🟡 Concern — assertFolderIsAccessible is now public — no subclass override guard or final annotation

The PR makes assertFolderIsAccessible public (was specified as private in the original task spec). The rationale is that SaveObject needs to call it directly. Making it public is the right fix, but it introduces a new API surface.

If any class extends FolderManagementHandler and overrides assertFolderIsAccessible, the override could remove the default-deny invariant. The class has no final annotation and the method has none either.

Impact: Downstream code that extends FolderManagementHandler could silently override the security check. Not a current risk (no known subclasses in the diff) but a maintenance risk.

Suggested fix: Either mark FolderManagementHandler as final, or mark assertFolderIsAccessible as final, or add a @internal annotation with a comment that overriding this method voids the access-control contract. Document the visibility change in the CHANGELOG as a breaking API change for downstream extenders.

[WilcoLouwerse]

(adjusted from line 3591 to nearest hunk line 3483)

🟡 Concern — Missing currentUser pass-through in SaveObject::setSelfMetadata — relies silently on session

setSelfMetadata() calls $this->folderManagementHandler->assertFolderIsAccessible(folderId: $folderValue, objectEntity: $objectEntity) with no currentUser argument. This falls back to IUserSession::getUser() inside assertFolderIsAccessible().

For HTTP calls this is correct. But setSelfMetadata is also called from non-HTTP paths (cron, import, batch jobs) where session may be null or stale. In those cases the check correctly defaults to deny (no session user → null → deny). However, those paths don't supply @self.folder in the first place (confirmed by the grep in tasks.md), so this is currently benign.

The risk is: if any future caller passes @self data with a folder key from a non-HTTP path (e.g., a cron that processes external JSON payloads), the check will deny even if the caller could legitimately provide an explicit $currentUser.

Suggested fix: Add ?IUser $currentUser = null as a parameter to setSelfMetadata and pass it through to assertFolderIsAccessible. This threads the explicit user from DI callers correctly and makes the contract explicit.

[WilcoLouwerse]

(adjusted from line 505 to nearest hunk line 277)

🟡 Concern — logFolderAccessDenied sets object=0 — audit entries for denied pre-persist objects are ambiguous

In logFolderAccessDenied(), the audit trail entry sets $auditTrail->setObject(0) with comment "no object persisted yet". Node ID 0 could collide with a real object ID if OpenRegister uses auto-increment starting from 1. Any audit query WHERE object = 0 correctly filters only these denial entries — but the audit trail UI may display 0 as a valid object reference or join it unexpectedly.

Also: setSession($actorUid) sets the session field to the user UID rather than an actual session token/ID. This is inconsistent with how createAuditTrail populates the session field for successful operations, making forensic correlation between denial events and session events harder.

Suggested fix: Use a dedicated sentinel like null or -1 for the object field if the schema allows null, or document that object=0 is the sentinel for pre-persist denial events. For session, use the actual session token if available from IUserSession::getSessionId(), or use null/empty-string rather than the UID (which is already in the user field).

[WilcoLouwerse]

(adjusted from line 773 to nearest hunk line 1493)

🟡 Concern — update() and patch() FolderAccessDeniedException 403 path missing controller-level tests

(See related 🔴 blocker above for the primary finding. This concern captures the secondary issue: the test gap means catch-block ordering cannot be regression-tested for two of the three covered endpoints.)

The comment in tasks.md step 7.3 explicitly says only testCreateReturns403WithStructuredBodyOnFolderAccessDenied was added. For a PR that adds security-critical catch blocks to three methods, all three should have analogous tests.

Suggested fix: Add analogous tests for update() and patch() following the same mock-and-assert pattern as the create test.

[WilcoLouwerse]

(adjusted from line 249 to nearest hunk line 92)

🟢 Minor — FolderAccessDeniedException constructor parameter $code defaults to 403 — but Exception code has no HTTP semantics

The constructor signature is __construct(string $attemptedFolderId, int $code=403, ?Exception $previous=null). Using HTTP status codes as exception codes conflates two different things: Exception::getCode() is typically an application error code, not an HTTP status. Controllers using $exception->getCode() anywhere for routing logic would get 403 (expected) but future callers might be confused by the dual-use.

Suggested fix: Consider using a named constant like self::HTTP_STATUS = 403 or simply 0 for the exception code and document that HTTP mapping is the controller's responsibility (which it is — the folderAccessDeniedResponse() method hardcodes statusCode: 403 correctly).

[WilcoLouwerse]

🟢 Minor — CHANGELOG has two '### Breaking Changes' sections at the same level — malformed markdown

The diff adds a new ### Breaking Changes entry at line 8, but line 11 already has an existing ### Breaking Changes entry. The result is two consecutive level-3 ### Breaking Changes headings under ## Unreleased. Changelog parsers and release tools that use headings for grouping will emit duplicate sections.

Suggested fix: Merge the new @self.folder entry into the existing ### Breaking Changes section rather than creating a duplicate heading.

[WilcoLouwerse]

(adjusted from line 486 to nearest hunk line 277)

🟢 Minor — logFolderAccessDenied is private — tests rely on side-effects (insert call) rather than direct invocation

The audit-trail tests correctly verify that AuditTrailMapper::insert is called, which indirectly tests logFolderAccessDenied. This is the right approach for a private method. Minor note only: the comment in the docblock says "Called immediately before each throw site" — this is accurate and the implementation matches. No action required; noted for completeness.

[WilcoLouwerse]

Strict-mode review — 4 blockers, 4 concerns, 3 minors.

Top blockers:

• 🔴 Double access-check with different currentUser contexts — du
• 🔴 update() and patch() controller 403 paths have no test cover
• 🔴 403 body echoes attempted folder ID — folder existence oracl
• 🔴 phpcs CI failure unresolved — PR is non-green on PHP Quality

Strict mode requires REQUEST_CHANGES on any 🔴 or 🟡. Please address blockers and concerns before merge.