Преглед на файлове

fix: rate-limit /api/v1/admin/* (SEC_REVIEW F29)

The admin route group attached `tokenAuth → impersonation →
auditContext` but no rate limit. Combined with F30 (LIKE '%q%' full-
table scan in IP search), F31 (deep-offset audit pagination), and F32
(N+1 enrichment per page row), any authenticated Viewer — which is
the OIDC default role — could fire unbounded heavy admin queries.
Pre-auth, the same group also gave the DB a free shot at
TokenRepository::findByHash() per invalid bearer (the F27 issue, but
F27's fix only covered /api/v1/* and /api/v1/auth/*, not the admin
prefix).

The admin group now attaches RateLimitMiddleware in the same two-
position pattern as the public and auth groups: outermost runs before
TokenAuth and keys on `ip:<REMOTE_ADDR>`, innermost runs after auth
and keys on `token:<tokenId>`. The two namespaces draw on independent
buckets so a legitimate authenticated burst is not amplified by the
IP cap, and an attacker rotating IPs cannot reset the per-token cap.

This bounds F30/F31/F32's blast radius until each lands its own fix,
and closes the pre-auth flooding hole on the admin prefix.

The prior testAdminRoutesNotRateLimited integration test encoded the
bug as expected behaviour and is replaced with
testAdminRoutesAreRateLimited (asserts capacity-bounded successes +
remainder 429s on /api/v1/admin/me) plus
testAdminAuditLogIsRateLimitedPerToken (asserts the heavy admin
endpoints flagged by F31 hit the ceiling on burst as a Viewer).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
chiappa преди 3 дни
родител
ревизия
a997d65818
променени са 2 файла, в които са добавени 50 реда и са изтрити 12 реда
  1. 9 5
      api/src/App/AppFactory.php
  2. 41 7
      api/tests/Integration/Public/RateLimitTest.php

+ 9 - 5
api/src/App/AppFactory.php

@@ -51,13 +51,15 @@ use Slim\Routing\RouteCollectorProxy;
  *
  * Route groups in M04:
  *  - Public    /api/v1/report               RateLimit(ip) → TokenAuth → AuditContext → RateLimit(token) → controller (kind check inside)
- *  - Admin     /api/v1/admin/{reporters,consumers,tokens}  TokenAuth → Impersonation → Rbac(Admin)
- *  - Admin     /api/v1/admin/me             TokenAuth → Impersonation → Rbac(Viewer)
+ *  - Admin     /api/v1/admin/*              RateLimit(ip) → TokenAuth → Impersonation → AuditContext → RateLimit(token) → Rbac(role)
  *  - Auth      /api/v1/auth/*               RateLimit(ip) → TokenAuth → AuditContext → RateLimit(token) → controller (kind=service inside)
  *
- * The two RateLimit positions (SEC_REVIEW F27) draw on independent
+ * The two RateLimit positions (SEC_REVIEW F27, F29) draw on independent
  * `ip:` and `token:` buckets so an invalid-bearer-token flood is
- * throttled before TokenAuthenticationMiddleware ever queries the DB.
+ * throttled before TokenAuthenticationMiddleware ever queries the DB,
+ * and an authenticated Viewer cannot drive pathological admin-side
+ * queries (F30 IP-search full-table scan, F31 deep-offset audit,
+ * F32 N+1 enrichment) without hitting the post-auth token bucket.
  */
 final class AppFactory
 {
@@ -356,9 +358,11 @@ final class AppFactory
             $admin->delete('/policies/{id}', [$policies, 'delete'])
                 ->add(RbacMiddleware::require($rf, Role::Admin));
         })
+            ->add($rateLimit)
             ->add($auditContext)
             ->add($impersonation)
-            ->add($tokenAuth);
+            ->add($tokenAuth)
+            ->add($rateLimit);
 
         // Internal jobs API: scheduler-only. Network gate (404 outside
         // RFC1918) → token gate (401) → controller. Order matters:

+ 41 - 7
api/tests/Integration/Public/RateLimitTest.php

@@ -78,16 +78,50 @@ final class RateLimitTest extends AppTestCase
         self::assertSame('1', $resp->getHeaderLine('Retry-After'));
     }
 
-    public function testAdminRoutesNotRateLimited(): void
+    /**
+     * SEC_REVIEW F29. The /api/v1/admin/* group previously attached only
+     * tokenAuth → impersonation → auditContext, no rate limit. Combined
+     * with F30 (IP-search full-table scan), F31 (deep-offset audit), and
+     * F32 (N+1 enrichment), an authenticated Viewer (the OIDC default
+     * role) could drive arbitrarily expensive queries at unbounded rate.
+     * RateLimitMiddleware is now attached, so a burst against any admin
+     * endpoint must produce 429s once the per-token bucket drains.
+     */
+    public function testAdminRoutesAreRateLimited(): void
     {
         $admin = $this->createToken(TokenKind::Admin, role: Role::Admin);
-        // Admin routes should never 429 even when smashed.
-        for ($i = 0; $i < 50; $i++) {
-            $resp = $this->request('GET', '/api/v1/admin/me', [
-                'Authorization' => 'Bearer ' . $admin,
-            ]);
-            self::assertNotSame(429, $resp->getStatusCode());
+        $headers = ['Authorization' => 'Bearer ' . $admin];
+
+        $statuses = [];
+        for ($i = 0; $i < 20; $i++) {
+            $statuses[] = $this->request('GET', '/api/v1/admin/me', $headers)->getStatusCode();
+        }
+
+        $accepted = count(array_filter($statuses, static fn (int $s): bool => $s === 200));
+        $limited = count(array_filter($statuses, static fn (int $s): bool => $s === 429));
+
+        // Capacity is 4 (refill=2, capacity=4 from setUp); the rest must 429.
+        self::assertSame(4, $accepted, 'capacity-bounded successes');
+        self::assertSame(16, $limited, 'remainder rate-limited');
+    }
+
+    /**
+     * SEC_REVIEW F29 — heavier admin endpoints (the very ones called out
+     * by F30/F31/F32) are gated by the same per-token bucket so a Viewer
+     * cannot pound them at unbounded rate.
+     */
+    public function testAdminAuditLogIsRateLimitedPerToken(): void
+    {
+        $viewer = $this->createToken(TokenKind::Admin, role: Role::Viewer);
+        $headers = ['Authorization' => 'Bearer ' . $viewer];
+
+        $statuses = [];
+        for ($i = 0; $i < 20; $i++) {
+            $statuses[] = $this->request('GET', '/api/v1/admin/audit-log', $headers)->getStatusCode();
         }
+
+        $limited = count(array_filter($statuses, static fn (int $s): bool => $s === 429));
+        self::assertGreaterThan(0, $limited, 'admin/audit-log must rate-limit');
     }
 
     /**