Skip to content
This repository was archived by the owner on May 29, 2026. It is now read-only.

Commit e1ba44c

Browse files
committed
fix(security): ActionsController — require admin for all mutations
Every write surface (`create`/`update`/`patch`/`destroy`/`test`/ `migrateFromHooks`) was `@NoAdminRequired` with no per-tenant or owner guard. Actions persist as workflow hooks that fire on every matching object lifecycle event — a non-admin who briefly auths could register attacker-chosen workflow logic that survives password reset, session revocation, and even the source account being disabled (the action row carries no owner check on execution → self-rearming hook). Add `requireAdmin()` helper (returns 401/403 JSONResponse or null) and call it at the top of every mutation. `index`/`show` stay `@NoAdminRequired` so the management UI is still legible to non-admins. Drive-by: `patch()` was forwarding to `update(objectId: $id)`, but `update`'s signature is `update(int $id)` — fixed to `update(id: $id)` so PATCH actually works. Refs: #1419 review (blocker 6) — discussion_r3187494440
1 parent 442eca2 commit e1ba44c

1 file changed

Lines changed: 61 additions & 2 deletions

File tree

lib/Controller/ActionsController.php

Lines changed: 61 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,9 @@
3131
use OCP\AppFramework\Http\Attribute\NoAdminRequired;
3232
use OCP\AppFramework\Http\Attribute\NoCSRFRequired;
3333
use OCP\AppFramework\Http\JSONResponse;
34+
use OCP\IGroupManager;
3435
use OCP\IRequest;
36+
use OCP\IUserSession;
3537
use Psr\Log\LoggerInterface;
3638

3739
/**
@@ -89,7 +91,9 @@ public function __construct(
8991
ActionMapper $actionMapper,
9092
ActionLogMapper $actionLogMapper,
9193
ActionService $actionService,
92-
LoggerInterface $logger
94+
LoggerInterface $logger,
95+
private readonly IUserSession $userSession,
96+
private readonly IGroupManager $groupManager
9397
) {
9498
parent::__construct(appName: $appName, request: $request);
9599
$this->actionMapper = $actionMapper;
@@ -98,6 +102,40 @@ public function __construct(
98102
$this->logger = $logger;
99103
}//end __construct()
100104

105+
/**
106+
* Gate Actions mutations to admin group members.
107+
*
108+
* SECURITY: Actions persist as workflow hooks that fire on every
109+
* matching object lifecycle event. A non-admin who briefly auth-es
110+
* could otherwise register an attacker-chosen workflow that
111+
* survives password reset, session revocation, and even the source
112+
* account being disabled (the action row carries no owner check on
113+
* execution). Gate every write surface (`create`/`update`/`patch`/
114+
* `destroy`/`test`/`migrateFromHooks`) on admin-group membership.
115+
*
116+
* @return JSONResponse|null 403 response when not admin, null when allowed.
117+
*/
118+
private function requireAdmin(): ?JSONResponse
119+
{
120+
$user = $this->userSession->getUser();
121+
if ($user === null) {
122+
return new JSONResponse(
123+
data: ['error' => 'Authentication required'],
124+
statusCode: 401
125+
);
126+
}
127+
128+
if ($this->groupManager->isAdmin($user->getUID()) === false) {
129+
return new JSONResponse(
130+
data: ['error' => 'Forbidden: Actions management is admin-only'],
131+
statusCode: 403
132+
);
133+
}
134+
135+
return null;
136+
137+
}//end requireAdmin()
138+
101139
/**
102140
* List all actions with pagination and filtering
103141
*
@@ -248,6 +286,10 @@ public function show(int $id): JSONResponse
248286
#[NoCSRFRequired]
249287
public function create(): JSONResponse
250288
{
289+
if (($denial = $this->requireAdmin()) !== null) {
290+
return $denial;
291+
}
292+
251293
try {
252294
$data = $this->request->getParams();
253295

@@ -297,6 +339,10 @@ public function create(): JSONResponse
297339
#[NoCSRFRequired]
298340
public function update(int $id): JSONResponse
299341
{
342+
if (($denial = $this->requireAdmin()) !== null) {
343+
return $denial;
344+
}
345+
300346
try {
301347
$data = $this->request->getParams();
302348

@@ -341,7 +387,8 @@ public function update(int $id): JSONResponse
341387
#[NoCSRFRequired]
342388
public function patch(int $id): JSONResponse
343389
{
344-
return $this->update(objectId: $id);
390+
// requireAdmin() runs inside update() — no need to duplicate here.
391+
return $this->update(id: $id);
345392
}//end patch()
346393

347394
/**
@@ -361,6 +408,10 @@ public function patch(int $id): JSONResponse
361408
#[NoCSRFRequired]
362409
public function destroy(int $id): JSONResponse
363410
{
411+
if (($denial = $this->requireAdmin()) !== null) {
412+
return $denial;
413+
}
414+
364415
try {
365416
$action = $this->actionService->deleteAction($id);
366417

@@ -393,6 +444,10 @@ public function destroy(int $id): JSONResponse
393444
#[NoCSRFRequired]
394445
public function test(int $id): JSONResponse
395446
{
447+
if (($denial = $this->requireAdmin()) !== null) {
448+
return $denial;
449+
}
450+
396451
try {
397452
$data = $this->request->getParams();
398453

@@ -483,6 +538,10 @@ function ($log) {
483538
#[NoCSRFRequired]
484539
public function migrateFromHooks(int $schemaId): JSONResponse
485540
{
541+
if (($denial = $this->requireAdmin()) !== null) {
542+
return $denial;
543+
}
544+
486545
try {
487546
$report = $this->actionService->migrateFromHooks($schemaId);
488547

0 commit comments

Comments
 (0)