Просмотр исходного кода

fix: audit `GET /auth/users/{id}` lookups to detect enumeration (SEC_REVIEW F17)

Adds a `user.fetched` audit row on every 200 / 404 response from
`AuthController::getUser`. Iterative enumeration of the users table
by a service-token holder now leaves a per-id trail that SOC tooling
can alert on. F14 already caps the throughput; this commit closes the
detection gap. The 400 malformed-id path stays silent — it's a
protocol error, not a valid-shape probe.

The 200/404 response bodies differ structurally, so existence is
trivially detectable from the body — defensive `usleep` would only
add latency to legitimate UI session refreshes without strengthening
the mitigation surface, and is intentionally skipped.

OpenAPI yaml and `openapi.php` document the audit emission and the
404 response. Regression tests cover found / not_found / invalid-id
paths and the multi-id sweep that produces the SOC alert pattern.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
chiappa 4 дней назад
Родитель
Сommit
57ab1ba034

+ 5 - 2
api/openapi.php

@@ -618,11 +618,14 @@ $paths = [
         'get' => [
             'tags' => ['Auth'],
             'summary' => 'Refetch a user record',
-            'description' => '**UI BFF only.** Used to refresh role / display_name during a session.',
+            'description' => "**UI BFF only.** Used to refresh role / display_name during a session.\n\nEvery call (200 and 404) emits a `user.fetched` audit row so iterative enumeration of user ids leaves a per-id trail (SEC_REVIEW F17). The route group is also rate-limited per service-token id (SEC_REVIEW F14).",
             'x-internal' => true,
             'security' => [['BearerAuth' => []]],
             'parameters' => [['name' => 'id', 'in' => 'path', 'required' => true, 'schema' => ['type' => 'integer']]],
-            'responses' => ['200' => ['description' => 'User record', 'content' => ['application/json' => ['schema' => ['$ref' => '#/components/schemas/User']]]]],
+            'responses' => [
+                '200' => ['description' => 'User record', 'content' => ['application/json' => ['schema' => ['$ref' => '#/components/schemas/User']]]],
+                '404' => ['description' => 'No user with this id', 'content' => ['application/json' => ['schema' => ['type' => 'object', 'properties' => ['error' => ['type' => 'string', 'example' => 'not_found']]]]]],
+            ],
         ],
     ],
 ];

+ 14 - 1
api/public/openapi.yaml

@@ -1468,7 +1468,10 @@ paths:
       tags:
         - Auth
       summary: Refetch a user record
-      description: '**UI BFF only.** Used to refresh role / display_name during a session.'
+      description: |
+        **UI BFF only.** Used to refresh role / display_name during a session.
+
+        Every call (200 and 404) emits a `user.fetched` audit row so iterative enumeration of user ids leaves a per-id trail (SEC_REVIEW F17). The route group is also rate-limited per service-token id (SEC_REVIEW F14).
       x-internal: true
       security:
         - BearerAuth: []
@@ -1485,6 +1488,16 @@ paths:
             'application/json':
               schema:
                 '$ref': '#/components/schemas/User'
+        '404':
+          description: No user with this id
+          content:
+            'application/json':
+              schema:
+                type: object
+                properties:
+                  error:
+                    type: string
+                    example: not_found
 components:
   securitySchemes:
     BearerAuth:

+ 26 - 1
api/src/Application/Auth/AuthController.php

@@ -178,11 +178,36 @@ final class AuthController
             return self::error($response, 400, 'invalid user id');
         }
 
-        $user = $this->users->findById((int) $id);
+        $requestedId = (int) $id;
+        $user = $this->users->findById($requestedId);
+        $auditCtx = self::auditContext($request);
+
+        // SEC_REVIEW F17: emit `user.fetched` for both 200 and 404 so
+        // iterative enumeration of `/users/{id}` by a service-token
+        // holder is observable in audit_log. Best-effort — a DB hiccup
+        // on the audit insert must not 500 a UI session refresh.
         if ($user === null) {
+            $this->audit->emit(
+                AuditAction::USER_FETCHED,
+                'user',
+                $requestedId,
+                ['outcome' => 'not_found'],
+                $auditCtx,
+                null,
+            );
+
             return self::error($response, 404, 'not_found');
         }
 
+        $this->audit->emit(
+            AuditAction::USER_FETCHED,
+            'user',
+            $user->id,
+            ['outcome' => 'found'],
+            $auditCtx,
+            $user->email ?? $user->displayName,
+        );
+
         return self::json_response($response, 200, [
             'user_id' => $user->id,
             'role' => $user->role->value,

+ 5 - 0
api/src/Domain/Audit/AuditAction.php

@@ -47,6 +47,11 @@ final class AuditAction
     public const USER_ROLE_CHANGED = 'user.role_changed';
     public const USER_DISABLED = 'user.disabled';
     public const USER_ENABLED = 'user.enabled';
+    // SEC_REVIEW F17: read-side audit. The only read action we record;
+    // emitted from `GET /api/v1/auth/users/{id}` so iterative
+    // enumeration of the users table by a service-token holder leaves
+    // a per-id trail that SOC tooling can alert on.
+    public const USER_FETCHED = 'user.fetched';
 
     public const OIDC_ROLE_MAPPING_CREATED = 'oidc_role_mapping.created';
     public const OIDC_ROLE_MAPPING_DELETED = 'oidc_role_mapping.deleted';

+ 104 - 0
api/tests/Integration/Auth/AuthEndpointsTest.php

@@ -453,4 +453,108 @@ final class AuthEndpointsTest extends AppTestCase
         );
         self::assertSame(403, $response->getStatusCode());
     }
+
+    /**
+     * SEC_REVIEW F17 regression: a successful `GET /api/v1/auth/users/{id}`
+     * must emit `user.fetched` so iterative enumeration leaves a per-id
+     * trail in audit_log.
+     */
+    public function testGetUserFoundEmitsUserFetchedAudit(): void
+    {
+        $token = $this->createToken(TokenKind::Service);
+        $userId = $this->createUser(Role::Operator, isLocal: false);
+
+        $response = $this->request(
+            'GET',
+            '/api/v1/auth/users/' . $userId,
+            ['Authorization' => 'Bearer ' . $token],
+        );
+        self::assertSame(200, $response->getStatusCode());
+
+        $row = $this->db->fetchAssociative(
+            "SELECT actor_kind, action, target_type, target_id, target_label, details_json FROM audit_log WHERE action = 'user.fetched' ORDER BY id DESC LIMIT 1"
+        );
+        self::assertIsArray($row, 'user.fetched audit row must exist on 200');
+        self::assertSame('system', $row['actor_kind']);
+        self::assertSame('user', $row['target_type']);
+        self::assertSame((string) $userId, $row['target_id']);
+        self::assertSame('user@example.com', $row['target_label']);
+        $details = json_decode((string) $row['details_json'], true);
+        self::assertIsArray($details);
+        self::assertSame('found', $details['outcome']);
+    }
+
+    /**
+     * SEC_REVIEW F17 regression: a 404 lookup must also emit
+     * `user.fetched` with outcome=not_found. This is the path attackers
+     * iterate over to enumerate which ids exist; without this row the
+     * probe is invisible to SOC tooling.
+     */
+    public function testGetUserNotFoundEmitsUserFetchedAudit(): void
+    {
+        $token = $this->createToken(TokenKind::Service);
+
+        $response = $this->request(
+            'GET',
+            '/api/v1/auth/users/999999',
+            ['Authorization' => 'Bearer ' . $token],
+        );
+        self::assertSame(404, $response->getStatusCode());
+
+        $row = $this->db->fetchAssociative(
+            "SELECT actor_kind, action, target_type, target_id, target_label, details_json FROM audit_log WHERE action = 'user.fetched' ORDER BY id DESC LIMIT 1"
+        );
+        self::assertIsArray($row, 'user.fetched audit row must exist on 404');
+        self::assertSame('system', $row['actor_kind']);
+        self::assertSame('user', $row['target_type']);
+        self::assertSame('999999', $row['target_id']);
+        self::assertNull($row['target_label']);
+        $details = json_decode((string) $row['details_json'], true);
+        self::assertIsArray($details);
+        self::assertSame('not_found', $details['outcome']);
+    }
+
+    /**
+     * SEC_REVIEW F17: a malformed id (non-numeric, leading zero, etc.)
+     * is rejected at the protocol layer with 400 and does NOT emit an
+     * audit row. The audit signal we care about is keyed on a valid id
+     * shape, where iterative enumeration becomes meaningful.
+     */
+    public function testGetUserInvalidIdDoesNotEmitAudit(): void
+    {
+        $token = $this->createToken(TokenKind::Service);
+
+        $response = $this->request(
+            'GET',
+            '/api/v1/auth/users/abc',
+            ['Authorization' => 'Bearer ' . $token],
+        );
+        self::assertSame(400, $response->getStatusCode());
+
+        $count = (int) $this->db->fetchOne(
+            "SELECT COUNT(*) FROM audit_log WHERE action = 'user.fetched'"
+        );
+        self::assertSame(0, $count, 'malformed-id 400 must not emit user.fetched');
+    }
+
+    /**
+     * SEC_REVIEW F17: an iterative scan over many ids leaves one audit
+     * row per probe. This is the SOC detection signal — a single
+     * service-token id producing many `user.fetched` rows in a tight
+     * window is the alert pattern.
+     */
+    public function testEnumerationProducesOneAuditRowPerProbe(): void
+    {
+        $token = $this->createToken(TokenKind::Service);
+        $headers = ['Authorization' => 'Bearer ' . $token];
+
+        for ($i = 1; $i <= 5; $i++) {
+            $this->request('GET', '/api/v1/auth/users/' . $i, $headers);
+        }
+
+        $count = (int) $this->db->fetchOne(
+            "SELECT COUNT(*) FROM audit_log WHERE action = 'user.fetched'"
+        );
+        self::assertSame(5, $count, 'each /users/{id} probe must emit one audit row');
+    }
 }