Forráskód Böngészése

fix: cap /ips/{ip} path length at 80 chars (SEC_REVIEW F72)

The F43 fix tightened the `{ip:.+}` charset to `[0-9a-fA-F.:%]+` —
no more multi-segment / `..` / dash matches. But the regex was
still unbounded in length: a 10 MB payload of all `0`s would
match, hand the router a 10 MB string, get rawurldecoded into a
copy, and only then fail validation in `IpAddress::fromString`.
Web-server URL-length limits cap this in most deployments, but
the application-layer behaviour still depended on the
environment.

Add `{1,80}` to the quantifier on both routes:

  - api: `/api/v1/admin/ips/{ip:[0-9a-fA-F.:%]{1,80}}`
  - ui:  `/app/ips/{ip:[0-9a-fA-F.:%]{1,80}}`

80 covers every realistic IP-shaped value:

  - IPv4 dotted-quad: ≤ 15 chars (`255.255.255.255`).
  - IPv6 canonical:   ≤ 39 chars
    (`xxxx:xxxx:xxxx:xxxx:xxxx:xxxx:xxxx:xxxx`).
  - IPv6 with every colon urlencoded as `%3A`: ≤ 53 chars.

50% headroom over the worst case. Anything past 80 — including
the 10 MB string the review called out — fails to match the
route and 404s at the router before `rawurldecode`,
`IpAddress::fromString`, or any future filename / log key /
downstream URL use.

Regression tests in
`api/tests/Integration/Admin/IpsControllerTest.php`:
`testDetailRejectsNonIpShapedPaths` data-provider gains
`oversized digits` (81 × `1`) and `oversized hex` (200 × `a`),
both expected to 404.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
chiappa 3 napja
szülő
commit
dd4f688f77

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

@@ -291,15 +291,21 @@ final class AppFactory
             // matched as an IP.
             $admin->get('/ips/countries', [$ips, 'countries'])
                 ->add(RbacMiddleware::require($rf, Role::Viewer));
-            // SEC_REVIEW F43: pin to an IP-shaped charset rather than `.+`.
-            // 0-9, a-f/A-F (IPv6 hex), `.` (IPv4), `:` (IPv6), and `%`
-            // (so a UI `rawurlencode('2001:db8::1')` form like
-            // `2001%3Adb8%3A%3A1` still matches before the controller's
-            // `rawurldecode`). Anything else — `/`, `..`, `?`, spaces,
-            // dashes — fails to match the route and 404s before the
-            // handler can use the param as a filename / log key /
-            // downstream URL component.
-            $admin->get('/ips/{ip:[0-9a-fA-F.:%]+}', [$ips, 'show'])
+            // SEC_REVIEW F43 + F72: pin to an IP-shaped charset AND
+            // cap the length so a Viewer can't hand the handler a
+            // multi-megabyte string. 0-9, a-f/A-F (IPv6 hex), `.`
+            // (IPv4), `:` (IPv6), and `%` (so a UI
+            // `rawurlencode('2001:db8::1')` form like
+            // `2001%3Adb8%3A%3A1` still matches before the
+            // controller's `rawurldecode`). Length cap 80: an
+            // IPv4 dotted-quad is ≤ 15 chars, a canonical IPv6 is
+            // ≤ 39 chars, and the same IPv6 with every colon
+            // urlencoded (`%3A`) is ≤ 53 chars. 80 leaves ~50%
+            // headroom; anything past that is malformed-or-malicious.
+            // Anything else — `/`, `..`, `?`, spaces, dashes,
+            // multi-megabyte strings — fails to match the route
+            // and 404s before the handler reads `$args['ip']`.
+            $admin->get('/ips/{ip:[0-9a-fA-F.:%]{1,80}}', [$ips, 'show'])
                 ->add(RbacMiddleware::require($rf, Role::Viewer));
 
             /** @var StatsController $stats */

+ 7 - 0
api/tests/Integration/Admin/IpsControllerTest.php

@@ -295,6 +295,13 @@ final class IpsControllerTest extends AppTestCase
         yield 'with space' => ['/api/v1/admin/ips/192.0.2.1 admin'];
         yield 'with dash' => ['/api/v1/admin/ips/not-an-ip'];
         yield 'with brackets' => ['/api/v1/admin/ips/[2001:db8::1]'];
+        // SEC_REVIEW F72: oversized path. Even within the strict
+        // charset, an 81-char string isn't a real IP. The route
+        // length cap kicks in BEFORE `rawurldecode` runs on the
+        // value — defends against the multi-megabyte case the
+        // SEC_REVIEW called out.
+        yield 'oversized digits' => ['/api/v1/admin/ips/' . str_repeat('1', 81)];
+        yield 'oversized hex' => ['/api/v1/admin/ips/' . str_repeat('a', 200)];
     }
 
     #[\PHPUnit\Framework\Attributes\DataProvider('nonIpShapedPaths')]

+ 10 - 6
ui/src/App/AppFactory.php

@@ -134,12 +134,16 @@ final class AppFactory
             /** @var IpsController $ips */
             $ips = $container->get(IpsController::class);
             $group->get('/ips', [$ips, 'index']);
-            // SEC_REVIEW F43: pin to an IP-shaped charset rather than `.+`.
-            // 0-9, a-f/A-F (IPv6 hex), `.` (IPv4), `:` (IPv6), and `%`
-            // (urlencoded colons survive). Anything else — `/`, `..`,
-            // `?`, spaces, dashes — 404s at the route layer before
-            // anything reads `$args['ip']`.
-            $group->get('/ips/{ip:[0-9a-fA-F.:%]+}', [$ips, 'show']);
+            // SEC_REVIEW F43 + F72: pin to an IP-shaped charset AND
+            // cap the length. 0-9, a-f/A-F (IPv6 hex), `.` (IPv4),
+            // `:` (IPv6), and `%` (urlencoded colons survive).
+            // Length cap 80 covers IPv4 (≤15), canonical IPv6 (≤39),
+            // and IPv6-with-every-colon-urlencoded (≤53) with
+            // headroom; anything past that is malformed-or-
+            // malicious. Anything else — `/`, `..`, `?`, spaces,
+            // dashes, multi-megabyte strings — 404s at the route
+            // layer before anything reads `$args['ip']`.
+            $group->get('/ips/{ip:[0-9a-fA-F.:%]{1,80}}', [$ips, 'show']);
 
             /** @var ManualBlocksController $manualBlocks */
             $manualBlocks = $container->get(ManualBlocksController::class);