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

Commit 6b24be2

Browse files
WilcoLouwerseWilcoLouwerse
committed
fix: address 3 open review threads on PR #1419
- ActionsController: drop @NoAdminRequired / #[NoAdminRequired] from create/update/patch/destroy/test/migrateFromHooks. The methods were carrying the attribute *and* calling requireAdmin() in the body, which is the exact contradiction gate-9 (semantic-auth) detects. Without the attribute, the methods are admin-only at the framework level by default; the body-level requireAdmin() stays as defence-in-depth. Refs: PR #1419 review (discussion_r3200014786 / re-review follow-up). - AuditTrailController: same pattern on export() and clearAll(). Updated requireAdmin() docblock to reflect the new layering. - ImportService: move setRequestImportJobId($importJobId) inside the try block in importFromExcel and importFromCsv, so the set/clear pair is fully wrapped — guarantees the request-scoped field is cleared even if the set itself were ever to throw under a long-lived worker reusing the singleton mapper. Refs: PR #1419 review (discussion_r3187494469). Refs #1419
1 parent 0ae9c3b commit 6b24be2

3 files changed

Lines changed: 40 additions & 28 deletions

File tree

‎lib/Controller/ActionsController.php‎

Lines changed: 23 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -110,8 +110,11 @@ public function __construct(
110110
* could otherwise register an attacker-chosen workflow that
111111
* survives password reset, session revocation, and even the source
112112
* 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.
113+
* execution). Every write surface (`create`/`update`/`patch`/
114+
* `destroy`/`test`/`migrateFromHooks`) is admin-only at the
115+
* framework level (the methods carry no `@NoAdminRequired`); this
116+
* helper stays as defence-in-depth so a future refactor that
117+
* silently re-adds `@NoAdminRequired` does not open the surface.
115118
*
116119
* @return JSONResponse|null 403 response when not admin, null when allowed.
117120
*/
@@ -274,15 +277,15 @@ public function show(int $id): JSONResponse
274277
/**
275278
* Create a new action
276279
*
277-
* @return JSONResponse
280+
* Admin-only at the framework level (no @NoAdminRequired). Body
281+
* `requireAdmin()` stays as defence-in-depth.
278282
*
279-
* @NoAdminRequired
283+
* @return JSONResponse
280284
*
281285
* @NoCSRFRequired
282286
*
283287
* @spec openspec/changes/retrofit-2026-05-01-actions/tasks.md#task-1
284288
*/
285-
#[NoAdminRequired]
286289
#[NoCSRFRequired]
287290
public function create(): JSONResponse
288291
{
@@ -325,17 +328,17 @@ public function create(): JSONResponse
325328
/**
326329
* Update an action (full replacement)
327330
*
331+
* Admin-only at the framework level (no @NoAdminRequired). Body
332+
* `requireAdmin()` stays as defence-in-depth.
333+
*
328334
* @param int $id Action ID
329335
*
330336
* @return JSONResponse
331337
*
332-
* @NoAdminRequired
333-
*
334338
* @NoCSRFRequired
335339
*
336340
* @spec openspec/changes/retrofit-2026-05-01-actions/tasks.md#task-1
337341
*/
338-
#[NoAdminRequired]
339342
#[NoCSRFRequired]
340343
public function update(int $id): JSONResponse
341344
{
@@ -373,17 +376,17 @@ public function update(int $id): JSONResponse
373376
/**
374377
* Partial update an action
375378
*
379+
* Admin-only at the framework level (no @NoAdminRequired); update()
380+
* also runs requireAdmin() as defence-in-depth.
381+
*
376382
* @param int $id Action ID
377383
*
378384
* @return JSONResponse
379385
*
380-
* @NoAdminRequired
381-
*
382386
* @NoCSRFRequired
383387
*
384388
* @spec openspec/changes/retrofit-2026-05-01-actions/tasks.md#task-1
385389
*/
386-
#[NoAdminRequired]
387390
#[NoCSRFRequired]
388391
public function patch(int $id): JSONResponse
389392
{
@@ -394,17 +397,17 @@ public function patch(int $id): JSONResponse
394397
/**
395398
* Soft-delete an action
396399
*
400+
* Admin-only at the framework level (no @NoAdminRequired). Body
401+
* `requireAdmin()` stays as defence-in-depth.
402+
*
397403
* @param int $id Action ID
398404
*
399405
* @return JSONResponse
400406
*
401-
* @NoAdminRequired
402-
*
403407
* @NoCSRFRequired
404408
*
405409
* @spec openspec/changes/retrofit-2026-05-01-actions/tasks.md#task-1
406410
*/
407-
#[NoAdminRequired]
408411
#[NoCSRFRequired]
409412
public function destroy(int $id): JSONResponse
410413
{
@@ -432,15 +435,15 @@ public function destroy(int $id): JSONResponse
432435
/**
433436
* Test action with dry-run simulation
434437
*
438+
* Admin-only at the framework level (no @NoAdminRequired). Body
439+
* `requireAdmin()` stays as defence-in-depth.
440+
*
435441
* @param int $id Action ID
436442
*
437443
* @return JSONResponse
438444
*
439-
* @NoAdminRequired
440-
*
441445
* @NoCSRFRequired
442446
*/
443-
#[NoAdminRequired]
444447
#[NoCSRFRequired]
445448
public function test(int $id): JSONResponse
446449
{
@@ -526,15 +529,15 @@ function ($log) {
526529
/**
527530
* Migrate inline hooks from a schema to Action entities
528531
*
532+
* Admin-only at the framework level (no @NoAdminRequired). Body
533+
* `requireAdmin()` stays as defence-in-depth.
534+
*
529535
* @param int $schemaId Schema ID
530536
*
531537
* @return JSONResponse
532538
*
533-
* @NoAdminRequired
534-
*
535539
* @NoCSRFRequired
536540
*/
537-
#[NoAdminRequired]
538541
#[NoCSRFRequired]
539542
public function migrateFromHooks(int $schemaId): JSONResponse
540543
{

‎lib/Controller/AuditTrailController.php‎

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -77,8 +77,10 @@ public function __construct(
7777
* SECURITY: `clearAll` wipes the entire audit table — a chain of
7878
* trust for AVG/GDPR Art 30 reviews. `export` dumps every row in
7979
* bulk and is an obvious recon path across tenants. Both surfaces
80-
* stay `@NoAdminRequired` at the framework level so the existing
81-
* UI keeps loading, but reject non-admin callers in the body.
80+
* are admin-only at the framework level (the methods carry no
81+
* `@NoAdminRequired`) and this body-level helper stays as
82+
* defence-in-depth so removing the framework gate by accident does
83+
* not silently open the surface.
8284
*
8385
* @return JSONResponse|null 401/403 response when blocked, null when allowed.
8486
*/
@@ -368,7 +370,8 @@ public function objects(string $register, string $schema, string $id): JSONRespo
368370
/**
369371
* Export audit trail logs in specified format
370372
*
371-
* @NoAdminRequired
373+
* Admin-only at the framework level (no @NoAdminRequired). Body
374+
* `requireAdmin()` stays as defence-in-depth.
372375
*
373376
* @NoCSRFRequired
374377
*
@@ -481,7 +484,8 @@ public function destroyMultiple(): JSONResponse
481484
/**
482485
* Clear all audit trail logs
483486
*
484-
* @NoAdminRequired
487+
* Admin-only at the framework level (no @NoAdminRequired). Body
488+
* `requireAdmin()` stays as defence-in-depth.
485489
*
486490
* @NoCSRFRequired
487491
*

‎lib/Service/ImportService.php‎

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -352,12 +352,16 @@ public function importFromExcel(
352352

353353
// Generate a per-import UUID and stamp it on every audit row
354354
// produced during this call. ImportService::softDeleteByImportJobId
355-
// uses this UUID to roll the import back. Cleared in finally
356-
// so the value never leaks across requests.
355+
// uses this UUID to roll the import back. The set/clear pair is
356+
// wrapped in try/finally so the request-scoped field is always
357+
// cleared, including when the set itself or any subsequent line
358+
// throws — guards against cross-request bleed on long-lived
359+
// workers where the singleton mapper is reused.
357360
$importJobId = Uuid::v4()->toRfc4122();
358-
$this->auditTrailMapper->setRequestImportJobId(importJobId: $importJobId);
359361

360362
try {
363+
$this->auditTrailMapper->setRequestImportJobId(importJobId: $importJobId);
364+
361365
$reader = new Xlsx();
362366
$reader->setReadDataOnly(true);
363367
$spreadsheet = $reader->load($filePath);
@@ -458,9 +462,10 @@ public function importFromCsv(
458462

459463
// Per-import UUID — see importFromExcel() for the rationale.
460464
$importJobId = Uuid::v4()->toRfc4122();
461-
$this->auditTrailMapper->setRequestImportJobId(importJobId: $importJobId);
462465

463466
try {
467+
$this->auditTrailMapper->setRequestImportJobId(importJobId: $importJobId);
468+
464469
// Use PhpSpreadsheet CSV reader (works perfectly for multiline fields).
465470
$reader = new Csv();
466471
$reader->setReadDataOnly(true);

0 commit comments

Comments
 (0)