ソースを参照

docs: mark SEC_REVIEW F27 as fixed in 060119a

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
chiappa 4 日 前
コミット
8e7a5f7b46
1 ファイル変更37 行追加1 行削除
  1. 37 1
      doc/SEC_REVIEW.md

+ 37 - 1
doc/SEC_REVIEW.md

@@ -11,7 +11,7 @@
 >
 > Each finding is referenced as **F<N>** for later citation.
 >
-> **Findings rolled up:** 5 sev-3 (5 fixed, 0 open), 27 sev-2 (17 fixed, 10 open), 42 sev-1.
+> **Findings rolled up:** 5 sev-3 (5 fixed, 0 open), 27 sev-2 (18 fixed, 9 open), 42 sev-1.
 
 ---
 
@@ -965,6 +965,42 @@
   invalid bearer tokens incurs DB lookups (and `last_used_at` writes
   on hits) with zero backoff, pinning DB pool / connection budget.
 - **Severity: 2**
+- **Status:** Fixed in `060119a`. `RateLimitMiddleware` no longer
+  bypasses on missing principal; it now derives the bucket key from
+  whichever signal is present:
+    - principal attached → `token:<tokenId>` (post-auth, original
+      behaviour),
+    - principal missing  → `ip:<REMOTE_ADDR>` (pre-auth, new fail-closed
+      path; empty/absent REMOTE_ADDR collapses to a single
+      `ip:unknown` bucket so we never silently bypass).
+  `RateLimiter::tryConsume()` now accepts arbitrary string keys so
+  these namespaces stay independent — a single user's per-token bucket
+  cannot drain the per-IP bucket that throttles unauthenticated
+  traffic.
+
+  `AppFactory` registers `RateLimitMiddleware` twice on `/api/v1/*` and
+  `/api/v1/auth/*`: once as the outermost layer (runs before
+  `TokenAuthenticationMiddleware`, consumes from the IP bucket) and
+  once between `AuditContext` and the handler (consumes from the
+  token bucket once a principal is attached). The pre-auth position
+  is what closes the 401-bypass: invalid-bearer floods now hit the
+  IP bucket and 429 before the request reaches `tokens.findByHash()`.
+
+  Admin/internal/health/docs route groups still have no rate limit —
+  that is the scope of F29 (admin) and existing internal-network /
+  loopback gating, not this finding.
+
+  Tests:
+    - `RateLimiterTest::testTokenAndIpNamespacesDoNotShareABucket`
+      verifies the namespaces hold separate buckets.
+    - New `RateLimitMiddlewareTest` covers token-keyed buckets,
+      IP-keyed fallback, per-IP isolation, and the
+      missing/empty `REMOTE_ADDR` `ip:unknown` collapse.
+    - `RateLimitTest::testInvalidBearerTokenFloodIsRateLimitedBeforeAuth`
+      sends 20 requests with a junk bearer; the first 4 reach the
+      auth path (401), the rest 429 before any DB lookup.
+    - `RateLimitTest::testMissingBearerHeaderIsAlsoRateLimitedByIp`
+      covers the no-Authorization-header case on `/api/v1/blocklist`.
 
 ### F28 — Rate limiter buckets are per-process, unbounded, and reset per replica
 - **File:** `api/src/Infrastructure/Http/RateLimiter.php:23-49`