Преглед изворни кода

fix: cap audit-log filter length and pagination depth (SEC_REVIEW F31)

`AuditController::list` now bounds every free-form filter (`action`,
`entity_type`, `entity_id`, `subject_kind`, `subject_id`) to 128 chars
and rejects any computed offset above 10 000 with 400
`validation_failed`. Stops a Viewer from forcing
`LIMIT 200 OFFSET huge` deep scans and from passing megabyte filter
strings into the query planner. The boundary case (offset=10 000) is
still accepted; one more page over is rejected.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
chiappa пре 4 дана
родитељ
комит
3a2564d14b

+ 36 - 4
api/src/Application/Admin/AuditController.php

@@ -23,6 +23,17 @@ final class AuditController
     private const ALLOWED_ACTOR_KINDS = ['user', 'admin-token', 'reporter', 'consumer', 'system'];
     private const ALLOWED_ACTOR_VIA = ['oidc', 'local', 'admin-token', 'service', 'reporter', 'consumer', 'system'];
 
+    // SEC_REVIEW F31: cap free-form filter strings so a caller can't push
+    // multi-megabyte garbage through the query planner. Audit values written
+    // by the system are well under these limits (action names ~30 chars,
+    // entity types/ids short identifiers); 128 is comfortable headroom.
+    private const MAX_FILTER_LENGTH = 128;
+
+    // SEC_REVIEW F31: cap pagination depth so a Viewer cannot force
+    // `LIMIT n OFFSET huge` deep scans. 10 000 rows is well past any
+    // human-driven browse and bounds the worst-case scan cost.
+    private const MAX_OFFSET = 10000;
+
     public function __construct(private readonly AuditRepository $audit)
     {
     }
@@ -65,15 +76,27 @@ final class AuditController
         }
 
         if (isset($params['action']) && is_string($params['action']) && $params['action'] !== '') {
-            $filters['action'] = $params['action'];
+            if (strlen($params['action']) > self::MAX_FILTER_LENGTH) {
+                $errors['action'] = 'must be at most ' . self::MAX_FILTER_LENGTH . ' characters';
+            } else {
+                $filters['action'] = $params['action'];
+            }
         }
 
         if (isset($params['entity_type']) && is_string($params['entity_type']) && $params['entity_type'] !== '') {
-            $filters['entity_type'] = $params['entity_type'];
+            if (strlen($params['entity_type']) > self::MAX_FILTER_LENGTH) {
+                $errors['entity_type'] = 'must be at most ' . self::MAX_FILTER_LENGTH . ' characters';
+            } else {
+                $filters['entity_type'] = $params['entity_type'];
+            }
         }
 
         if (isset($params['entity_id']) && is_string($params['entity_id']) && $params['entity_id'] !== '') {
-            $filters['entity_id'] = $params['entity_id'];
+            if (strlen($params['entity_id']) > self::MAX_FILTER_LENGTH) {
+                $errors['entity_id'] = 'must be at most ' . self::MAX_FILTER_LENGTH . ' characters';
+            } else {
+                $filters['entity_id'] = $params['entity_id'];
+            }
         }
 
         // Subject filter (used by reporter/consumer detail pages):
@@ -84,6 +107,8 @@ final class AuditController
         if ($subjectKind !== '' || $subjectId !== '') {
             if ($subjectKind === '' || $subjectId === '') {
                 $errors['subject'] = 'subject_kind and subject_id must be supplied together';
+            } elseif (strlen($subjectKind) > self::MAX_FILTER_LENGTH || strlen($subjectId) > self::MAX_FILTER_LENGTH) {
+                $errors['subject'] = 'subject_kind and subject_id must each be at most ' . self::MAX_FILTER_LENGTH . ' characters';
             } else {
                 $filters['subject_kind'] = $subjectKind;
                 $filters['subject_id'] = $subjectId;
@@ -108,11 +133,18 @@ final class AuditController
             }
         }
 
+        $offset = ($page - 1) * $pageSize;
+        if ($offset > self::MAX_OFFSET) {
+            // SEC_REVIEW F31: deep-pagination guard. Past MAX_OFFSET we
+            // refuse rather than silently reset, so a client that wanted
+            // an old row notices and switches to a `to=` cursor instead.
+            $errors['page'] = 'pagination depth exceeded; use `to=` to cursor into older rows';
+        }
+
         if ($errors !== []) {
             return self::validationFailed($response, $errors);
         }
 
-        $offset = ($page - 1) * $pageSize;
         $result = $this->audit->search($filters, $pageSize, $offset);
 
         return self::json($response, 200, [

+ 74 - 0
api/tests/Integration/Admin/AuditLogControllerTest.php

@@ -106,6 +106,80 @@ final class AuditLogControllerTest extends AppTestCase
         self::assertSame(401, $resp->getStatusCode());
     }
 
+    public function testOversizedFilterRejected(): void
+    {
+        // SEC_REVIEW F31: free-form filter strings are bounded at 128 chars.
+        $token = $this->createToken(TokenKind::Admin, Role::Viewer);
+        $long = str_repeat('a', 200);
+        $resp = $this->request(
+            'GET',
+            '/api/v1/admin/audit-log?action=' . $long,
+            ['Authorization' => 'Bearer ' . $token],
+        );
+        self::assertSame(400, $resp->getStatusCode());
+        self::assertArrayHasKey('action', $this->decode($resp)['details']);
+
+        $resp = $this->request(
+            'GET',
+            '/api/v1/admin/audit-log?entity_type=' . $long,
+            ['Authorization' => 'Bearer ' . $token],
+        );
+        self::assertSame(400, $resp->getStatusCode());
+        self::assertArrayHasKey('entity_type', $this->decode($resp)['details']);
+
+        $resp = $this->request(
+            'GET',
+            '/api/v1/admin/audit-log?entity_id=' . $long,
+            ['Authorization' => 'Bearer ' . $token],
+        );
+        self::assertSame(400, $resp->getStatusCode());
+        self::assertArrayHasKey('entity_id', $this->decode($resp)['details']);
+
+        $resp = $this->request(
+            'GET',
+            '/api/v1/admin/audit-log?subject_kind=' . $long . '&subject_id=5',
+            ['Authorization' => 'Bearer ' . $token],
+        );
+        self::assertSame(400, $resp->getStatusCode());
+        self::assertArrayHasKey('subject', $this->decode($resp)['details']);
+    }
+
+    public function testDeepOffsetRejected(): void
+    {
+        // SEC_REVIEW F31: pagination is capped at 10 000 offset rows so a
+        // Viewer can't force `LIMIT 200 OFFSET huge` deep scans.
+        $token = $this->createToken(TokenKind::Admin, Role::Viewer);
+        $resp = $this->request(
+            'GET',
+            '/api/v1/admin/audit-log?page=999999&page_size=200',
+            ['Authorization' => 'Bearer ' . $token],
+        );
+        self::assertSame(400, $resp->getStatusCode());
+        self::assertArrayHasKey('page', $this->decode($resp)['details']);
+    }
+
+    public function testDeepOffsetAtBoundaryAccepted(): void
+    {
+        // Right at offset = MAX_OFFSET (10 000) is fine; one more page over
+        // is rejected.
+        $token = $this->createToken(TokenKind::Admin, Role::Viewer);
+        // page=51, page_size=200 → offset=10000 (boundary, allowed)
+        $resp = $this->request(
+            'GET',
+            '/api/v1/admin/audit-log?page=51&page_size=200',
+            ['Authorization' => 'Bearer ' . $token],
+        );
+        self::assertSame(200, $resp->getStatusCode());
+
+        // page=52, page_size=200 → offset=10200 (over boundary, rejected)
+        $resp = $this->request(
+            'GET',
+            '/api/v1/admin/audit-log?page=52&page_size=200',
+            ['Authorization' => 'Bearer ' . $token],
+        );
+        self::assertSame(400, $resp->getStatusCode());
+    }
+
     private function seedAudit(string $kind, ?string $actorId, string $action, string $type, string $id, string $details, string $when): void
     {
         $this->db->insert('audit_log', [