fix(auth): four admin actions were reachable by any authenticated user (#502)

* fix(auth): four admin actions were reachable by any authenticated user

Hand-read all thirteen no-admin-idor findings rather than treating the
count as a work queue. One is a real IDOR, four are semantic-auth
mismatches, two are genuinely guarded downstream, five take no object
reference at all, and one is scoped through its receiver.

The four semantic-auth ones are the serious half. SettingsController has a
consistent convention — every mutator is admin-required and only readers
carry the no-admin annotation — and these were the exceptions:

  - testEmailConnection: caller-supplied smtpHost/smtpPort reach a DSN and
    the server opens an outbound TCP connection to whatever was named. For
    a non-admin that is an SSRF and internal port-scan primitive.
  - updateEmailTemplate: the only write in the whole class carrying the
    annotation. It writes app configuration, and the stored HTML is
    rendered into real outbound mail, so any authenticated user could
    rewrite the templates every recipient receives — and mint unbounded
    email_template_* config rows besides.
  - syncOrganisations: triggers a register-wide write sync with a
    caller-chosen batch size.
  - exportArchiMate: exports the WHOLE register while its sibling
    exportOrgArchiMate exports one organisation — and the sibling has
    carried verifyOrgExportPermission all along. The broader endpoint was
    the unguarded one. The annotation is kept here deliberately: that
    helper grants organisation-admins too, which is the tier the admin UI
    relies on and which removing the annotation would drop.

The three email-template reads move with their write. No frontend code
calls any of those routes — the settings UI reads templates from the bulk
settings payload — verified with a positive control on a route that IS
called, so nothing breaks.

getSbomImportStatus is the real IDOR: a caller-supplied moduleVersieUuid
reached SbomImportService::getStatus(), whose lookup runs with rbac and
multitenancy both off. Guarded with authorizeManage()'s read tier — the
same two building blocks minus the editor-group requirement, since reading
a status is not managing an import. Checked rather than assumed: the module
schema carries a real read ACL, so this is a genuine scope and not the
default-open case an authorization-less schema would give. Refuses 404, not
403, so it cannot become an existence oracle.

Everything else carries a reason-bearing exemption naming the code path
that makes it safe, not a state of the world.

One finding is deliberately LEFT OPEN. getGebruikenForDeelnemer forces its
organisation filter after getParams(), so a caller cannot forge it — but it
passes an ARRAY where the app's canonical sibling passes a SCALAR, and the
query runs with rbac and multitenancy off. Whether OpenRegister honours
array-containment matching on a related-object array property is unverified,
and if it silently ignores the array form that scope is vacuous. It could
not be settled here: the available instance has zero gebruik rows, so a live
A/B would have returned empty under both forms and proved nothing. It gets
the fail-closed guard its canonical sibling already has, and keeps the
finding. Exempting it would have manufactured the coverage.

Nothing here makes anything return 200 that previously errored: every change
is deny-only. exportArchiMate's dead $organization filter parameter is a
real bug found on the way and is NOT repaired here — that is a feature
repair sitting behind a missing guard, and it belongs after the guard lands.

Refs #492.

* fix(sbom): guard inside the try, not ahead of it

My own regression, caught before it landed. authorizeRead() reaches
SbomImportService::resolveParentModuleUuid(), and OpenRegister's real
ObjectService::find() RE-THROWS DoesNotExistException for a well-formed but
non-existent uuid rather than returning null — importSbom()'s docblock
already records this and wraps its whole body for exactly that reason.

Placing the guard ahead of the try would have converted this endpoint's
clean 404 into a 500 for precisely the callers the guard was added for: a
non-admin passing an unknown id. The guard now sits inside the try, so the
not-found dialect is the same whichever call raises it.

* test(auth): pin the auth posture of the six hardened endpoints

The security-change-has-tests gate failed on the previous push, correctly:
that change moved auth posture in lib/ and touched no test. This is the
test, and it pins the near-miss rather than the diff.

It parses the controller source with Nextcloud's OWN annotation regex,
copied byte for byte from ControllerMethodReflector::reflect(). That is the
point. While writing the fix, three of these methods documented their own
hardening with the sentence "the endpoint must not declare @NoAdminRequired"
— and that token, at the start of a comment line, MATCHES that regex.
Nextcloud would have gone on treating the endpoint as non-admin-required:
the sentence explaining the removal would have undone the removal, and the
change would have read as a security fix while being a no-op. A test
searching for the attribute form, or stripping comments first, would pass
straight over it.

Five tests. The first is a positive control asserting the parser CAN find
the annotation where it legitimately remains (getSyncStatus) — without it,
a typo in the regex or the docblock walk would make every absence assertion
pass over an empty array and the file would be green while asserting
nothing.

exportArchiMate is pinned in BOTH directions, because the two pull opposite
ways: it must KEEP the annotation, since verifyOrgExportPermission grants
organisation-admins and removing it would drop that tier, AND it must call
that helper. The right fix for its neighbours was the wrong fix for it.

getSbomImportStatus is pinned for guard presence and for guard POSITION —
inside the try, because authorizeRead() reaches ObjectService::find(), which
re-throws for an unknown uuid, so guarding ahead of the try would turn a
clean 404 into a 500 for exactly the callers the guard was added for.

Proven able to fail, prediction written first: re-planting the prose form of
the annotation reddens exactly testAdminActionsDoNotDeclareNoAdminRequired
and nothing else. Reverted byte-identically.

The positional assertion first failed on its own account, and the reason is
worth keeping: it anchored on the bare name, which the controller's own
explanatory comment uses ABOVE the try. A positional assertion over a corpus
that includes prose measures the prose. It anchors on the call now.

Author: rubenvdlinde

lib/Controller/GebruikController.php

@@ -288,6 +288,33 @@ public function getGebruikenForDeelnemer(): JSONResponse {
 
 		$orgUuid = $this->config->getUserValue(userId: $user->getUID(), appName: 'core', key: 'organisation');
 
+		// Fail CLOSED on a missing organisation, as the app's canonical
+		// deelnemers-scoped read already does in
+		// AangebodenGebruikService::getGebruiksWhereDeelnemers(). getUserValue()
+		// returns '' rather than null when the user-value was never set, so
+		// without this the scope below degrades to `deelnemers: ['']` — and the
+		// query runs with `_rbac: false, _multitenancy: false`, so an empty
+		// predicate that OpenRegister chooses to ignore would return every
+		// organisation's gebruik data rather than none.
+		if ($orgUuid === '') {
+			return new JSONResponse(
+				['message' => 'No organisation is set for this account'],
+				Http::STATUS_FORBIDDEN
+			);
+		}
+
+		// The scope is forced AFTER getParams(), so a caller cannot supply or
+		// override `deelnemers` and read another organisation's usage.
+		//
+		// ⚠️ OPEN, and deliberately not "fixed" by guessing: this passes an
+		// ARRAY where the canonical sibling passes a SCALAR, and whether
+		// OpenRegister honours array-containment matching on a `related-object`
+		// array property is unverified. If it silently ignores the array form
+		// this scope is vacuous. It could not be settled here — the instance
+		// available has zero gebruik rows, so a live A/B would have returned
+		// empty under both forms and proved nothing. Filed rather than changed:
+		// swapping the filter form on a working endpoint could equally break
+		// it, and an unmeasured change to a scope is not a fix.
 		$options = $this->request->getParams();
 		$options['deelnemers'] = [$orgUuid];
 

lib/Controller/ReviewController.php

@@ -108,6 +108,19 @@ public function submit(array $review = [], string $subjectType = '', string $sub
 	 *
 	 * @PublicPage
 	 * @NoCSRFRequired
+	 * @no-admin-idor-exempt The lookup IS constrained to the scope this
+	 *      endpoint declares public, which is the remedy for
+	 *      `publicpage-unscoped-object-lookup` (there is no session identity on
+	 *      a public page, so an ownership check is not available and not the
+	 *      fix). `$subjectId` never reaches storage: the query in
+	 *      ReviewAggregateService::fetchApprovedReviews() is keyed on
+	 *      `@self.register` / `@self.schema` plus `status = approved` — with
+	 *      the metadata filters correctly nested under `@self` — and the
+	 *      subject is matched in memory afterwards. `status` is then re-checked
+	 *      in PHP over every row, which that method's own comment names as the
+	 *      real enforcement point because `_rbac: false` bypasses
+	 *      OpenRegister's enforcement of the predicate. So an arbitrary
+	 *      `subjectId` can only ever select from already-approved reviews.
 	 * @spec           openspec/specs/catalog-ratings/spec.md#requirement-module-and-dienst-detail-pages-must-display-an-aggregate-rating-computed-only-from-approved-reviews
 	 */
 	#[PublicPage]

lib/Controller/SbomController.php

@@ -284,7 +284,20 @@ public function getSbomImportStatus(string $moduleVersieUuid): JSONResponse {
 			$operationId = null;
 		}
 
+		// The guard goes INSIDE the try, for the reason importSbom()'s docblock
+		// already records: authorizeRead() reaches
+		// SbomImportService::resolveParentModuleUuid(), and OpenRegister's real
+		// ObjectService::find() RE-THROWS DoesNotExistException for a
+		// well-formed but non-existent uuid rather than returning null. Guarding
+		// outside the try would therefore turn this endpoint's clean 404 into a
+		// 500 for exactly the callers the guard was added for — non-admins
+		// passing an unknown id.
 		try {
+			$readError = $this->authorizeRead(moduleVersieUuid: $moduleVersieUuid);
+			if ($readError !== null) {
+				return $readError;
+			}
+
 			return new JSONResponse(
 				data: $this->importService->getStatus(
 					moduleVersieUuid: $moduleVersieUuid,
@@ -299,6 +312,62 @@ public function getSbomImportStatus(string $moduleVersieUuid): JSONResponse {
 		}
 	}//end getSbomImportStatus()
 
+	/**
+	 * Read-ACL authorization guard (IDOR guard) for the import-status read.
+	 *
+	 * `getSbomImportStatus()` took a caller-supplied `moduleVersieUuid` all the
+	 * way to `SbomImportService::getStatus()`, whose lookup runs with
+	 * `_rbac: false, _multitenancy: false` — so the object reference was never
+	 * scoped to the caller and any authenticated user could read any
+	 * moduleVersie's import provenance by substituting a uuid.
+	 *
+	 * This is `authorizeManage()`'s read tier: the same two building blocks
+	 * (`resolveParentModuleUuid` + `userCanReadModule`) minus the editor-group
+	 * membership requirement, because reading a status is not managing an
+	 * import. `userCanReadModule()` resolves with `_rbac: true,
+	 * _multitenancy: true`, and — checked rather than assumed — the `module`
+	 * schema carries a real `authorization.read` ACL (group tiers plus an
+	 * `_organisation` match), so this is a genuine scope rather than the
+	 * default-open case an authorization-less schema would give.
+	 *
+	 * Refuses with 404, not 403, deliberately: the sibling not-found path
+	 * already answers 404 for an unknown uuid, and matching it keeps the
+	 * endpoint from becoming an existence oracle for other tenants' ids.
+	 *
+	 * @param string $moduleVersieUuid The target moduleVersie's uuid.
+	 *
+	 * @return JSONResponse|null Error response, or null when authorized.
+	 *
+	 * @spec openspec/specs/sbom-import/spec.md#requirement-moduleversie-records-sbom-import-provenance
+	 */
+	private function authorizeRead(string $moduleVersieUuid): ?JSONResponse {
+		$user = $this->userSession->getUser();
+		if ($user === null) {
+			return new JSONResponse(data: ['message' => 'Not logged in'], statusCode: Http::STATUS_UNAUTHORIZED);
+		}
+
+		if ($this->groupManager->isAdmin($user->getUID()) === true) {
+			return null;
+		}
+
+		$moduleUuid = $this->importService->resolveParentModuleUuid($moduleVersieUuid);
+		if ($moduleUuid === null || $this->importService->userCanReadModule($moduleUuid) === false) {
+			$this->logger->warning(
+				'SbomController: status read refused (no read access to the parent module)',
+				['moduleVersieUuid' => $moduleVersieUuid, 'uid' => $user->getUID()]
+			);
+			return new JSONResponse(
+				data: [
+					'message' => 'moduleVersie not found: ' . $moduleVersieUuid,
+					'error' => 'MODULE_VERSION_NOT_FOUND',
+				],
+				statusCode: Http::STATUS_NOT_FOUND
+			);
+		}
+
+		return null;
+	}//end authorizeRead()
+
 	/**
 	 * Manage-ACL authorization guard (IDOR guard). Returns a JSONResponse to
 	 * short-circuit on failure, or null when the caller may import.

lib/Controller/SettingsController.php

@@ -827,6 +827,11 @@ public function sendTestEmail(): JSONResponse {
 	 *
 	 * @NoAdminRequired
 	 * @NoCSRFRequired
+	 * @no-admin-idor-exempt Takes no object reference. The only parameter is a
+	 *      typed `int $minutesBack` time window, and
+	 *      OrganizationSyncService::getSyncStatusWithErrorHandling() accepts no
+	 *      object id either — it reports aggregate sync timing, not a record.
+	 *      There is nothing for a caller to substitute.
 	 *
 	 * @spec openspec/specs/method-decomposition/spec.md
 	 */
@@ -910,6 +915,10 @@ public function performSync(int $minutesBack = 0): JSONResponse {
 	 *
 	 * @NoAdminRequired
 	 * @NoCSRFRequired
+	 * @no-admin-idor-exempt An availability probe that touches no storage. The
+	 *      body reads one `timestamp` request param, logs it at debug level and
+	 *      echoes it back; there is no mapper, service or object lookup on the
+	 *      path at all, so there is no direct object reference to substitute.
 	 *
 	 * @spec openspec/specs/method-decomposition/spec.md
 	 */
@@ -1591,10 +1600,22 @@ private function resolveArchiMateMethod(array $options): array {
 	 * @spec openspec/specs/settings-admin-controller/spec.md
 	 */
 	public function exportArchiMate(): Response {
-		if ($this->userSession->getUser() === null) {
+		$currentUser = $this->userSession->getUser();
+		if ($currentUser === null) {
 			return new JSONResponse(['message' => 'Not authenticated'], Http::STATUS_UNAUTHORIZED);
 		}
 
+		// Same guard as the sibling exportOrgArchiMate(), which has carried it
+		// all along. This endpoint exports the WHOLE register while the sibling
+		// exports one organisation, so it was the broader of the two and the
+		// only one unguarded. @NoAdminRequired is kept deliberately: the helper
+		// grants organisation-admins as well as admins, which is the tier the
+		// admin UI relies on and which the annotation's removal would drop.
+		$permissionError = $this->verifyOrgExportPermission(currentUser: $currentUser);
+		if ($permissionError !== null) {
+			return $permissionError;
+		}
+
 		try {
 			$rawInput = file_get_contents('php://input');
 			$data = json_decode($rawInput, true);
@@ -1771,6 +1792,13 @@ public function render(): string {
 	 *
 	 * @NoAdminRequired
 	 * @NoCSRFRequired
+	 * @no-admin-idor-exempt The scope is in the RECEIVER, not in an argument.
+	 *      `$fileName` is resolved against
+	 *      `IRootFolder::getUserFolder($userSession->getUser()->getUID())`, so
+	 *      it can only ever name a file in the caller's own home; and `..` and
+	 *      `/` are rejected outright before that, so it cannot escape it. A
+	 *      substituted filename reaches a different file only if the caller
+	 *      already owns that file.
 	 * @spec            openspec/specs/settings-admin-controller/spec.md
 	 */
 	public function downloadArchiMate(string $fileName): Response {
@@ -1868,7 +1896,13 @@ public function downloadArchiMate(string $fileName): Response {
 	/**
 	 * Test email connection (separate from sending test email)
 	 *
-	 * @NoAdminRequired
+	 * Admin-only: the caller-supplied smtpHost/smtpPort reach a DSN in
+	 * SettingsService::testEmailConnection(), and the server then opens an
+	 * outbound TCP connection to whatever host and port were named. For a
+	 * non-admin that is an SSRF and internal port-scan primitive, so the
+	 * endpoint must not declare @NoAdminRequired. Its sibling sendTestEmail()
+	 * already omits it, as does every other action on this controller.
+	 *
 	 * @NoCSRFRequired
 	 *
 	 * @return JSONResponse Test connection result
@@ -2083,7 +2117,12 @@ public function getEmailTemplates(): JSONResponse {
 	 *
 	 * @return JSONResponse Template content.
 	 *
-	 * @NoAdminRequired
+	 * Admin-only, to match the write it pairs with. It reads admin-authored
+	 * mail templates out of app configuration, and no frontend code calls this
+	 * route at all — the settings UI reads templates from the bulk settings
+	 * payload instead (verified with a positive control on a route that IS
+	 * called). Leaving it open bought nothing and exposed admin content.
+	 *
 	 * @NoCSRFRequired
 	 * @spec            openspec/specs/settings-admin-controller/spec.md
 	 */
@@ -2126,7 +2165,14 @@ public function getEmailTemplate(string $templateName): JSONResponse {
 	 *
 	 * @return JSONResponse Update result.
 	 *
-	 * @NoAdminRequired
+	 * Admin-only: this writes app configuration —
+	 * setValueString($appName, "email_template_{$templateName}", $content) —
+	 * and the stored HTML is rendered into real outbound mail. It was the only
+	 * write on this controller declaring @NoAdminRequired, so any authenticated
+	 * user could rewrite the templates every recipient receives, and mint
+	 * unbounded `email_template_*` config rows besides. Its sibling
+	 * updateEmailSettings() already omits the annotation.
+	 *
 	 * @NoCSRFRequired
 	 * @spec            openspec/specs/settings-admin-controller/spec.md
 	 */
@@ -2185,7 +2231,9 @@ public function updateEmailTemplate(string $templateName): JSONResponse {
 	 *
 	 * @return JSONResponse Default template content.
 	 *
-	 * @NoAdminRequired
+	 * Admin-only, for the same reason as getEmailTemplate(): it belongs to the
+	 * admin mail-template surface and has no non-admin consumer.
+	 *
 	 * @NoCSRFRequired
 	 * @spec            openspec/specs/settings-admin-controller/spec.md
 	 */
@@ -2228,7 +2276,9 @@ public function getEmailTemplateDefault(string $templateName): JSONResponse {
 	 *
 	 * @return JSONResponse Available variables for template.
 	 *
-	 * @NoAdminRequired
+	 * Admin-only, for the same reason as getEmailTemplate(): it belongs to the
+	 * admin mail-template surface and has no non-admin consumer.
+	 *
 	 * @NoCSRFRequired
 	 * @spec            openspec/specs/settings-admin-controller/spec.md
 	 */
@@ -3448,7 +3498,20 @@ private function getHttpStatusForErrorMessage(string $message): int {
 	/**
 	 * Sync OpenRegister organisations to voorzieningen register
 	 *
-	 * @NoAdminRequired
+	 * Admin-only: this triggers a register-wide write sync with a
+	 * caller-chosen batch size, so the endpoint must not carry the
+	 * no-admin-required annotation. Its siblings performSync(),
+	 * bulkSyncStandards() and triggerEolSync() already omit it — the
+	 * annotation's absence IS the check on this controller, which is why none
+	 * of them carries an in-body one.
+	 *
+	 * The annotation is deliberately NOT spelled out above. Nextcloud's own
+	 * ControllerMethodReflector matches `^\h+\*\h+@([A-Z]\w+)` against the raw
+	 * docblock, so writing "must not declare @NoAdminRequired" at the start of
+	 * a comment line RE-DECLARES it — the sentence explaining the removal
+	 * undoes the removal, in the framework and not merely in a linter. Gate-7
+	 * caught it here; nothing else would have.
+	 *
 	 * @NoCSRFRequired
 	 *
 	 * @return JSONResponse The sync results