Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
rubenvdlinde merged 1 commit into from
May 28, 2026
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
99 changes: 68 additions & 31 deletions lib/Controller/AuditTrailController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,9 +47,14 @@
*
* @psalm-suppress UnusedClass
*
* @SuppressWarnings(PHPMD.ExcessiveClassLength) Controller covers audit trail, verification, verwerkingsregister
* @SuppressWarnings(PHPMD.CouplingBetweenObjects) Necessary service dependencies
* @SuppressWarnings(PHPMD.TooManyPublicMethods) One public method per audit trail route
* @SuppressWarnings(PHPMD.ExcessiveClassLength) Controller covers audit trail, verification, verwerkingsregister
* @SuppressWarnings(PHPMD.CouplingBetweenObjects) Necessary service dependencies
* @SuppressWarnings(PHPMD.TooManyPublicMethods) One public method per audit trail route
* @SuppressWarnings(PHPMD.ExcessiveClassComplexity) Audit-trail surface fans out into hash-verify,
* verwerkingsregister, inzageverzoek, export, immutability + admin-gating; the controller's
* overall cyclomatic complexity sits one step above the default threshold (51 vs 50) after
* the wave-3 C6 admin-only gates on index()/show() were added. Splitting the controller
* would force a route-table reshuffle without removing any actual branching.
*/
class AuditTrailController extends Controller
{
Expand Down Expand Up @@ -77,12 +82,17 @@ public function __construct(
}//end __construct()

/**
* Gate destructive / bulk-export audit operations on admin membership.
* Gate destructive / bulk-export / bulk-read audit operations on admin membership.
*
* SECURITY: `clearAll` wipes the entire audit table — a chain of
* trust for AVG/GDPR Art 30 reviews. `export` dumps every row in
* bulk and is an obvious recon path across tenants. Both surfaces
* are admin-only at the framework level (the methods carry no
* bulk and is an obvious recon path across tenants. `index` and
* `show` expose cross-tenant audit-trail rows (per-row diffs of
* every object change in every register/schema, frequently PII or
* IP-heavy); without this gate any authenticated user — including
* users restricted to a single app group — could enumerate every
* other tenant's change history (wave-3 C6). All surfaces are
* admin-only at the framework level for sensitive routes (no
* `@NoAdminRequired`) and this body-level helper stays as
* defence-in-depth so removing the framework gate by accident does
* not silently open the surface.
Expand Down Expand Up @@ -157,8 +167,8 @@ private function extractRequestParameters(): array
// Extract search parameter.
$search = $params['search'] ?? $params['_search'] ?? null;

$sort = $this->buildSortFromParams($params);
$filters = $this->buildFiltersFromParams($params);
$sort = $this->buildSortFromParams(params: $params);
$filters = $this->buildFiltersFromParams(params: $params);

return [
'limit' => $limit,
Expand Down Expand Up @@ -191,12 +201,18 @@ private function buildSortFromParams(array $params): array
if (is_array($sortRaw) === true) {
// Bracket format: _sort[created]=DESC.
foreach ($sortRaw as $field => $direction) {
$sort[$field] = strtoupper($direction) === 'ASC' ? 'ASC' : 'DESC';
$sort[$field] = 'DESC';
if (strtoupper($direction) === 'ASC') {
$sort[$field] = 'ASC';
}
}
} else if ($sortRaw !== null) {
// Flat format: sort=created&order=DESC.
$sortOrder = $params['order'] ?? $params['_order'] ?? 'DESC';
$sort[$sortRaw] = strtoupper($sortOrder) === 'ASC' ? 'ASC' : 'DESC';
$sort[$sortRaw] = 'DESC';
if (strtoupper($sortOrder) === 'ASC') {
$sort[$sortRaw] = 'ASC';
}
}

if (empty($sort) === true) {
Expand All @@ -217,9 +233,25 @@ private function buildSortFromParams(array $params): array
private function buildFiltersFromParams(array $params): array
{
$systemKeys = [
'limit', '_limit', 'offset', '_offset', 'page', '_page',
'search', '_search', 'sort', '_sort', 'order', '_order',
'_route', 'id', 'register', 'schema', 'format', 'from', 'to',
'limit',
'_limit',
'offset',
'_offset',
'page',
'_page',
'search',
'_search',
'sort',
'_sort',
'order',
'_order',
'_route',
'id',
'register',
'schema',
'format',
'from',
'to',
'identifier',
];

Expand All @@ -235,22 +267,26 @@ function ($key) use ($systemKeys) {
/**
* Get all audit trail logs
*
* @NoAdminRequired
* Admin-only at the framework level (no @NoAdminRequired). Body
* `requireAdmin()` stays as defence-in-depth. The cross-tenant
* audit-trail index leaks per-row diffs of every object change
* across every register/schema — wave-3 C6. Returns 200 on
* success, 401 when anonymous, 403 when non-admin.
*
* @return JSONResponse JSON response containing list of audit trails
*
* @NoCSRFRequired
*
* @psalm-return JSONResponse<200,
* array{results: array<\OCA\OpenRegister\Db\AuditTrail>,
* total: int<0, max>, page: int|null, pages: float, limit: int,
* offset: int|null}, array<never, never>>
*
* @spec openspec/changes/retrofit-2026-04-23-annotate-openregister/tasks.md#task-8
* @spec openspec/changes/retrofit-2026-04-30-annotate-openregister/tasks.md#task-17
*/
public function index(): JSONResponse
{
$denial = $this->requireAdmin();
if ($denial !== null) {
return $denial;
}

// Extract common parameters.
$params = $this->extractRequestParameters();

Expand All @@ -276,29 +312,30 @@ public function index(): JSONResponse
/**
* Get a specific audit trail log by ID
*
* Admin-only at the framework level (no @NoAdminRequired). Body
* `requireAdmin()` stays as defence-in-depth. Without the gate,
* any authed caller could fetch any audit-trail row by ID — and
* IDs are sequential, so this is also a trivial enumeration path
* across every tenant's change history (wave-3 C6). Returns 200
* on success, 401 when anonymous, 403 when non-admin, 404 if
* the row does not exist.
*
* @param int $id The audit trail ID
*
* @return JSONResponse A JSON response containing the log
*
* @NoAdminRequired
*
* @NoCSRFRequired
*
* @psalm-return JSONResponse<
* 200,
* array<array-key, mixed>,
* array<never, never>
* >|JSONResponse<
* 404,
* array{error: 'Audit trail not found'},
* array<never, never>
* >
*
* @spec openspec/changes/retrofit-2026-04-23-annotate-openregister/tasks.md#task-8
* @spec openspec/changes/retrofit-2026-04-30-annotate-openregister/tasks.md#task-15
*/
public function show(int $id): JSONResponse
{
$denial = $this->requireAdmin();
if ($denial !== null) {
return $denial;
}

try {
$log = $this->logService->getLog($id);
return new JSONResponse(data: $log);
Expand Down
64 changes: 60 additions & 4 deletions lib/Controller/SearchTrailController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,9 @@
use OCP\AppFramework\Controller;
use OCP\AppFramework\Http\JSONResponse;
use OCP\AppFramework\Http\TemplateResponse;
use OCP\IGroupManager;
use OCP\IRequest;
use OCP\IUserSession;

/**
* Class SearchTrailController
Expand All @@ -49,15 +51,54 @@ class SearchTrailController extends Controller
* @param string $appName The name of the app
* @param IRequest $request The request object
* @param SearchTrailService $searchTrailService The search trail service
* @param IUserSession $userSession Active user session for caller identity
* @param IGroupManager $groupManager Group manager for admin / role checks
*/
public function __construct(
string $appName,
IRequest $request,
private readonly SearchTrailService $searchTrailService
private readonly SearchTrailService $searchTrailService,
private readonly IUserSession $userSession,
private readonly IGroupManager $groupManager
) {
parent::__construct(appName: $appName, request: $request);
}//end __construct()

/**
* Gate sensitive search-trail read operations on admin membership.
*
* SECURITY: search-trail rows record per-search IP address, user
* ID, user-agent and full query string for every search across
* every register/schema. Returning them to non-admin callers leaks
* PII (GDPR) and gives any authenticated user — including users
* restricted to a single app group — a recon view of what every
* other tenant is searching for (wave-3 C7). Surface stays
* admin-only at both the framework level (no `@NoAdminRequired`)
* and the body level (defence-in-depth).
*
* @return JSONResponse|null 401/403 response when blocked, null when allowed.
*/
private function requireAdmin(): ?JSONResponse
{
$user = $this->userSession->getUser();
if ($user === null) {
return new JSONResponse(
data: ['error' => 'Authentication required'],
statusCode: 401
);
}

if ($this->groupManager->isAdmin($user->getUID()) === false) {
return new JSONResponse(
data: ['error' => 'Forbidden: this search-trail operation is admin-only'],
statusCode: 403
);
}

return null;

}//end requireAdmin()

/**
* Extract pagination, filter, and search parameters from request
*
Expand Down Expand Up @@ -305,7 +346,8 @@ private function paginate(array $results, ?int $total=0, ?int $limit=20, ?int $o
/**
* Get all search trail logs
*
* @NoAdminRequired
* Admin-only at the framework level (no @NoAdminRequired). Body
* `requireAdmin()` stays as defence-in-depth — wave-3 C7.
*
* @NoCSRFRequired
*
Expand All @@ -315,6 +357,11 @@ private function paginate(array $results, ?int $total=0, ?int $limit=20, ?int $o
*/
public function index(): JSONResponse
{
$denial = $this->requireAdmin();
if ($denial !== null) {
return $denial;
}

try {
// Get raw request parameters (this is what the service expects).
$rawParams = $this->request->getParams();
Expand Down Expand Up @@ -353,9 +400,13 @@ public function index(): JSONResponse
/**
* Get a specific search trail log by ID
*
* @param int $id The search trail ID
* Admin-only at the framework level (no @NoAdminRequired). Body
* `requireAdmin()` stays as defence-in-depth — wave-3 C7. IDs
* are sequential so without the gate any authed caller could
* enumerate every tenant's recorded searches (IP, user ID,
* user-agent, query string).
*
* @NoAdminRequired
* @param int $id The search trail ID
*
* @NoCSRFRequired
*
Expand All @@ -365,6 +416,11 @@ public function index(): JSONResponse
*/
public function show(int $id): JSONResponse
{
$denial = $this->requireAdmin();
if ($denial !== null) {
return $denial;
}

try {
$log = $this->searchTrailService->getSearchTrail($id);
return new JSONResponse(data: $log);
Expand Down
16 changes: 14 additions & 2 deletions tests/Service/ControllersIntegrationTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@
use OCP\AppFramework\Http\JSONResponse;
use OCP\IAppConfig;
use OCP\IDBConnection;
use OCP\IGroupManager;
use OCP\IRequest;
use OCP\IUser;
use OCP\IUserSession;
Expand Down Expand Up @@ -277,11 +278,15 @@ protected function setUp(): void
\OC::$server->get(LoggerInterface::class)
);

// Build SearchTrailController.
// Build SearchTrailController. Reuse the shared userSession mock and
// wire a real IGroupManager so the search-trail admin-only gate
// (wave-3 C7) reflects production behaviour in integration runs.
$this->searchTrailController = new SearchTrailController(
'openregister',
$this->request,
\OC::$server->get(SearchTrailService::class)
\OC::$server->get(SearchTrailService::class),
$this->userSession,
\OC::$server->get(IGroupManager::class)
);

// Build EndpointsController.
Expand Down Expand Up @@ -1780,6 +1785,8 @@ public function testSettingsReindexNegativeMaxObjects(): void
*/
public function testSearchTrailIndex(): void
{
// SearchTrail index is admin-only (wave-3 C7).
$this->setupAdminUser();
$this->request->method('getParams')->willReturn([]);
$this->request->method('getRequestUri')->willReturn('/api/search-trails');

Expand All @@ -1798,6 +1805,7 @@ public function testSearchTrailIndex(): void
*/
public function testSearchTrailIndexWithPagination(): void
{
$this->setupAdminUser();
$this->request->method('getParams')->willReturn([
'_limit' => 5,
'_offset' => 0,
Expand All @@ -1818,6 +1826,7 @@ public function testSearchTrailIndexWithPagination(): void
*/
public function testSearchTrailIndexWithPage(): void
{
$this->setupAdminUser();
$this->request->method('getParams')->willReturn([
'_limit' => 10,
'_page' => 2,
Expand All @@ -1838,6 +1847,7 @@ public function testSearchTrailIndexWithPage(): void
*/
public function testSearchTrailShowNotFound(): void
{
$this->setupAdminUser();
$response = $this->searchTrailController->show(999999);

$this->assertInstanceOf(JSONResponse::class, $response);
Expand Down Expand Up @@ -2119,6 +2129,7 @@ public function testSearchTrailExportCsv(): void
*/
public function testSearchTrailIndexWithDateFilters(): void
{
$this->setupAdminUser();
$this->request->method('getParams')->willReturn([
'from' => '2024-01-01',
'to' => '2025-12-31',
Expand All @@ -2138,6 +2149,7 @@ public function testSearchTrailIndexWithDateFilters(): void
*/
public function testSearchTrailIndexWithSort(): void
{
$this->setupAdminUser();
$this->request->method('getParams')->willReturn([
'_sort' => 'created',
'_order' => 'ASC',
Expand Down
Loading