Skip to content

fix(security): scope audit-trail + search-trail to admin (wave-3 C6/C7 cross-tenant leak)#2000

Merged
rubenvdlinde merged 1 commit into
developmentfrom
fix/audit-search-tenant-scope-wave3-c6-c7
May 28, 2026
Merged

fix(security): scope audit-trail + search-trail to admin (wave-3 C6/C7 cross-tenant leak)#2000
rubenvdlinde merged 1 commit into
developmentfrom
fix/audit-search-tenant-scope-wave3-c6-c7

Conversation

@rubenvdlinde

Copy link
Copy Markdown
Contributor

Summary

Two wave-3 critical findings closed: AuditTrailController::index/show (C6) and SearchTrailController::index/show (C7) were @NoAdminRequired, so any authenticated user could enumerate every other tenant's audit-trail rows and search-trail rows — both PII-heavy and recon-grade.

  • C6 — audit-trail rows carry per-row diffs of every object change: register/schema/object UUID, before/after values of every JSON property, actor UID, IP address, processing-activity tag. LogService::getAllLogs / getLog delegate straight to AuditTrailMapper::findAll / find with no scope filter, so the surface had no tenant boundary at all. With sequential IDs, show() doubled as a trivial enumeration path across the entire instance.
  • C7 — search-trail rows record the full query string, register/schema identifiers, result counts, the caller's IP, user-agent and session id for every search anyone has ever run. Same shape, same leak — plus GDPR-sensitive (IP + user-agent + query terms).

Fix (Option A — admin-only)

Both surfaces now go through a body-level requireAdmin() helper that returns 401 when anonymous and 403 when non-admin, mirroring the existing pattern used by AuditTrailController::export() / clearAll() and the wave-2 NotificationHistoryController. @NoAdminRequired is removed at the framework level too, so the gate sits at both layers (defence-in-depth — losing either still keeps the surface closed).

The non-list audit-trail surfaces (verify, verwerkingsregister, inzageverzoek, per-object objects()) stay reachable because they already key off identifiers the caller must know up front.

SearchTrailController gains IUserSession + IGroupManager constructor dependencies; NC's DI container auto-wires them, so no info.xml change is needed.

Files

  • lib/Controller/AuditTrailController.phpindex() + show() gated via existing requireAdmin(); helper docblock updated; pre-existing PHPCS run on buildSortFromParams / buildFiltersFromParams cleaned up in the same commit; new class-level @SuppressWarnings(PHPMD.ExcessiveClassComplexity) (the added gate calls bump cyclomatic complexity from 49 to 51).
  • lib/Controller/SearchTrailController.php — constructor + requireAdmin() helper added; index() + show() gated.
  • tests/Unit/Controller/AuditTrailControllerTest.php — 5 new denial-path tests covering 401/403 + service-not-invoked on forbidden access.
  • tests/Unit/Controller/SearchTrailControllerTest.php — 5 new denial-path tests + setUp default-admin so the existing happy paths still pass through the new gate.
  • tests/Unit/Controller/SearchTrailController{Coverage,Deep}Test.php — constructor sites updated.
  • tests/Service/ControllersIntegrationTest.phpSearchTrailController instantiation wires the shared userSession mock + a real IGroupManager from DI; testSearchTrailIndex* and testSearchTrailShowNotFound now call the existing setupAdminUser() helper.

Quality gates (all green on changed files)

Gate Result
php -l clean
composer phpstan (changed files) [OK] No errors
composer phpcs (changed files) clean
composer phpmd (changed files) clean
composer phpunit (4 affected unit suites, 136 tests) 136 passed, 1 skipped (pre-existing), 1 warning unrelated to this change

Pre-existing notes (not fixed here, out of hotfix scope):

  • tests/Service/ControllersIntegrationTest.php::testRegisters* already fails on development because RegistersController::__construct now expects 19 args (test passes 16) — verified by running the unmodified file from origin/development. Worth a follow-up but unrelated to this fix.

Follow-up issue (Option B)

Filing a separate issue after merge: allow admins to grant an auditor group read-only access to audit/search trails without granting full admin (useful for AVG/GDPR Art-30 reviewers and incident-response). Not blocking; admin-only is the safer default for now.

Test plan

  • CI runs PHPStan/PHPCS/PHPMD/PHPUnit on the changed files and reports green.
  • As a non-admin user in a single-app group, GET /apps/openregister/api/audit-trail returns 403.
  • As a non-admin user, GET /apps/openregister/api/audit-trail/{any-id} returns 403 — confirming sequential-ID enumeration is closed.
  • As a non-admin user, GET /apps/openregister/api/search-trail and GET /apps/openregister/api/search-trail/{any-id} return 403.
  • As anonymous, all four return 401 (not 403).
  • As admin, all four return 200 with the existing payload shape (no regression for legitimate users).

Refs: wave-3 triage (/tmp/triage-openregister.md) findings C6 and C7. DO NOT admin-merge.

…7 cross-tenant leak)

C6 — AuditTrailController::index/show were @NoAdminRequired, so any
authenticated user (including users restricted to a single app group)
could enumerate every other tenant's audit trail. Each row carries a
full diff of an object change — register/schema/object UUID, the new
and old values of every JSON property, the actor UID, IP address and
processing-activity tag. With sequential IDs, show() doubled as a
trivial enumeration path across the whole instance. LogService::
getAllLogs/getLog delegate straight to AuditTrailMapper::findAll/find
with no scope filter, so the surface had no tenant boundary at all.

C7 — SearchTrailController::index/show had the same shape against the
search-trail table. Rows record the full query string, register/schema
identifiers, result counts, the user's IP, user-agent and session id
for every search anyone has ever run — a GDPR-sensitive leak and an
obvious recon path ("what is tenant X searching for").

Both surfaces now go through a body-level requireAdmin() helper that
returns 401 when anonymous and 403 when non-admin, mirroring the
existing pattern used by AuditTrailController::export() / clearAll()
and the wave-2 NotificationHistoryController. Framework-level
@NoAdminRequired annotations are removed so the gate sits at both
layers (defence-in-depth — losing either still keeps the surface
closed). The non-list audit-trail surfaces (verify, verwerkingsregister,
inzageverzoek, per-object objects()) stay reachable because they
already key off identifiers the caller must know up front.

SearchTrailController gains IUserSession + IGroupManager constructor
deps; NC's DI container auto-wires them so info.xml needs no change.
Tests updated for both controllers — denial paths assert 401/403 and
verify the service layer is not invoked on a forbidden call (no DB
hit, no row read). Integration-test SearchTrail sites get an explicit
setupAdminUser() call to match production.

Pre-existing PHPCS violations on the buildSortFromParams /
buildFiltersFromParams pair (inline-if + multi-line-array nits) and a
PHPMD ExcessiveClassComplexity bump (50 -> 51 from the added
requireAdmin() calls) are fixed in the same commit so the diff lands
on a green gate.

Follow-up issue (Option B "auditor group"): a future change should
allow admins to grant an "auditor" group read-only access to audit /
search trails without granting full admin — useful for AVG/GDPR Art
30 reviewers. Not blocking; admin-only is the safer default for now.

Refs: wave-3 triage (/tmp/triage-openregister.md) findings C6, C7.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant