Skip to content

Commit 07e0837

Browse files
committed
fix(security): NotificationSubscriptionsController — verify register/schema access
`create()` forwarded `registerId` / `schemaId` straight to the mapper. The mapper validated only that at least one was non-null; never that the caller has read access to the referenced register/schema, or that they exist at all. Two issues: 1. A user could subscribe to any (register, schema) tuple in any tenant. Whether the downstream notification dispatcher re-checks permissions before sending is irrelevant — the subscription rows leak existence and let an attacker probe the schema namespace. 2. The mapper's success/404 distinction was a confirmed-existence channel for arbitrary integer IDs. Inject `RegisterMapper` + `SchemaMapper` and call `find()` (with default RBAC + multitenancy filters on) before insert. 404 on miss. Refs: #1419 review (concern 7) — discussion_r3187494490
1 parent 89380de commit 07e0837

1 file changed

Lines changed: 39 additions & 5 deletions

File tree

lib/Controller/NotificationSubscriptionsController.php

Lines changed: 39 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -37,23 +37,29 @@
3737
use OCP\AppFramework\Controller;
3838
use OCP\AppFramework\Http\JSONResponse;
3939
use OCP\IRequest;
40+
use OCA\OpenRegister\Db\RegisterMapper;
41+
use OCA\OpenRegister\Db\SchemaMapper;
4042
use OCP\IUserSession;
4143

4244
class NotificationSubscriptionsController extends Controller
4345
{
4446
/**
4547
* Constructor.
4648
*
47-
* @param string $appName App name.
48-
* @param IRequest $request Request.
49-
* @param NotificationSubscriptionMapper $mapper Subscription mapper.
50-
* @param IUserSession $userSession Current-user session.
49+
* @param string $appName App name.
50+
* @param IRequest $request Request.
51+
* @param NotificationSubscriptionMapper $mapper Subscription mapper.
52+
* @param IUserSession $userSession Current-user session.
53+
* @param RegisterMapper $registerMapper Register mapper for permission check.
54+
* @param SchemaMapper $schemaMapper Schema mapper for permission check.
5155
*/
5256
public function __construct(
5357
string $appName,
5458
IRequest $request,
5559
private readonly NotificationSubscriptionMapper $mapper,
56-
private readonly IUserSession $userSession
60+
private readonly IUserSession $userSession,
61+
private readonly RegisterMapper $registerMapper,
62+
private readonly SchemaMapper $schemaMapper
5763
) {
5864
parent::__construct(appName: $appName, request: $request);
5965

@@ -105,6 +111,34 @@ public function create(): JSONResponse
105111
$registerId = $this->coerceNullableInt(value: ($params['registerId'] ?? null));
106112
$schemaId = $this->coerceNullableInt(value: ($params['schemaId'] ?? null));
107113

114+
// SECURITY: load the referenced register/schema through the
115+
// standard RBAC + multitenancy filter before persisting the
116+
// subscription. Without this gate the mapper accepted any
117+
// (registerId, schemaId) pair — letting a caller subscribe
118+
// themselves to objects in other tenants and confirming the
119+
// existence of arbitrary IDs via the success/404 channel.
120+
if ($registerId !== null) {
121+
try {
122+
$this->registerMapper->find($registerId);
123+
} catch (\Throwable $e) {
124+
return new JSONResponse(
125+
data: ['error' => 'Register not found or not accessible'],
126+
statusCode: 404
127+
);
128+
}
129+
}
130+
131+
if ($schemaId !== null) {
132+
try {
133+
$this->schemaMapper->find($schemaId);
134+
} catch (\Throwable $e) {
135+
return new JSONResponse(
136+
data: ['error' => 'Schema not found or not accessible'],
137+
statusCode: 404
138+
);
139+
}
140+
}
141+
108142
try {
109143
$entity = $this->mapper->subscribe(
110144
userId: $userId,

0 commit comments

Comments
 (0)