فهرست منبع

fix: tighten /ips/{ip} route pattern to IP charset (SEC_REVIEW F43)

The route patterns `/api/v1/admin/ips/{ip:.+}` (api) and
`/app/ips/{ip:.+}` (ui) used `.+` to handle IPv6 colons, but `.+` also
matches multi-segment paths and arbitrary printable characters. Today
this is contained by the controller's `IpAddress::fromString` which
rejects non-IP strings with a 404, but any future code that reads
`$args['ip']` as a filename, log key, or downstream URL component
inherits a path-traversal sink.

Pin both routes to the strict charset `[0-9a-fA-F.:%]+`:

  - 0-9, a-f, A-F → IPv6 hex
  - . → IPv4 dotted-quad
  - : → IPv6 separator
  - % → urlencoded byte (the UI's AdminClient calls `rawurlencode($ip)`
    so an IPv6 like `2001:db8::1` arrives as `2001%3Adb8%3A%3A1`,
    which the route still needs to accept before the controller's
    `rawurldecode`)

Anything else — `/`, `..`, `?`, spaces, dashes, brackets — fails to
match the route and 404s before the handler can read `$args['ip']`.
The handler's existing `IpAddress::fromString` second layer is kept
(it still rejects e.g. `999.999.999.999` which is in the charset but
not a valid IP).

Regression tests in
`api/tests/Integration/Admin/IpsControllerTest.php` —
`testDetailRejectsNonIpShapedPaths` data-provider covers path
traversal (`..%2Fetc%2Fpasswd`), multi-segment paths
(`/192.0.2.1/extra`), query-injection probes, backslashes, spaces,
dashes, and bracketed IPv6 (`[2001:db8::1]`) — all 404 at the route
layer.

UI test cleanup in `tests/Integration/App/IpsPageTest.php`:
`testDetailPageReturnsTwig404OnApiNotFound` previously used
`/app/ips/not-an-ip` which exercises route rejection (uninteresting
post-F43); changed to `/app/ips/198.51.100.99` so the request
reaches the controller and exercises the actual
ApiNotFoundException → Twig 404 path the test was meant to cover.
The test file's `setUp()` also now does the standard public-route
warmup (hit /healthz before priming $_SESSION) so the new test
isn't subject to the pre-existing first-/app/*-request session
clobber pattern that already required the workaround in
SearchPageTest / SessionRevalidationTest.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
chiappa 3 روز پیش
والد
کامیت
8ff409fff2

+ 10 - 2
api/src/App/AppFactory.php

@@ -266,11 +266,19 @@ final class AppFactory
             $ips = $container->get(IpsController::class);
             $admin->get('/ips', [$ips, 'list'])
                 ->add(RbacMiddleware::require($rf, Role::Viewer));
-            // /ips/countries must come BEFORE /ips/{ip:.+} or it'd be
+            // /ips/countries must come BEFORE the {ip} route or it'd be
             // matched as an IP.
             $admin->get('/ips/countries', [$ips, 'countries'])
                 ->add(RbacMiddleware::require($rf, Role::Viewer));
-            $admin->get('/ips/{ip:.+}', [$ips, 'show'])
+            // 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'])
                 ->add(RbacMiddleware::require($rf, Role::Viewer));
 
             /** @var StatsController $stats */

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

@@ -275,6 +275,38 @@ final class IpsControllerTest extends AppTestCase
         self::assertSame(404, $response->getStatusCode());
     }
 
+    /**
+     * @return iterable<string, array{string}>
+     */
+    public static function nonIpShapedPaths(): iterable
+    {
+        // SEC_REVIEW F43: the {ip} route now uses a strict
+        // `[0-9a-fA-F.:%]+` charset. Anything outside that — slashes,
+        // dots-and-slashes traversal, query separators, spaces, dashes —
+        // 404s at the route layer before `$args['ip']` is read by the
+        // handler. Even though `IpAddress::fromString` would also
+        // reject these inputs today, the tightened route protects
+        // any future code that uses `$args['ip']` as a filename, log
+        // key, or downstream URL component.
+        yield 'path traversal' => ['/api/v1/admin/ips/..%2Fetc%2Fpasswd'];
+        yield 'with subpath' => ['/api/v1/admin/ips/192.0.2.1/extra'];
+        yield 'with query injection' => ['/api/v1/admin/ips/?injected'];
+        yield 'with backslash' => ['/api/v1/admin/ips/192.0.2.1\\admin'];
+        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]'];
+    }
+
+    #[\PHPUnit\Framework\Attributes\DataProvider('nonIpShapedPaths')]
+    public function testDetailRejectsNonIpShapedPaths(string $path): void
+    {
+        $token = $this->createToken(TokenKind::Admin, role: Role::Viewer);
+        $response = $this->request('GET', $path, [
+            'Authorization' => 'Bearer ' . $token,
+        ]);
+        self::assertSame(404, $response->getStatusCode(), "expected 404 for {$path}");
+    }
+
     public function testDetailRendersForUnknownIpWithCleanStatus(): void
     {
         $token = $this->createToken(TokenKind::Admin, role: Role::Viewer);

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

@@ -134,8 +134,12 @@ final class AppFactory
             /** @var IpsController $ips */
             $ips = $container->get(IpsController::class);
             $group->get('/ips', [$ips, 'index']);
-            // {ip:.+} so v6 colons don't break Slim's default segment regex.
-            $group->get('/ips/{ip:.+}', [$ips, 'show']);
+            // 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']);
 
             /** @var ManualBlocksController $manualBlocks */
             $manualBlocks = $container->get(ManualBlocksController::class);

+ 10 - 1
ui/tests/Integration/App/IpsPageTest.php

@@ -12,6 +12,11 @@ final class IpsPageTest extends AppTestCase
     protected function setUp(): void
     {
         $this->bootApp();
+        // The first /app/* request in a fresh process triggers
+        // session_start() and clobbers the $_SESSION values primed
+        // here. Hit a public route first so the session is active
+        // before any test request fires.
+        $this->request('GET', '/healthz');
         $_SESSION['_user'] = (new UserContext(1, 'Admin', 'admin', null, UserContext::SOURCE_LOCAL))->toArray();
         $_SESSION['_last_active'] = time();
         $_SESSION['_authenticated_at'] = time();
@@ -127,9 +132,13 @@ final class IpsPageTest extends AppTestCase
 
     public function testDetailPageReturnsTwig404OnApiNotFound(): void
     {
+        // Use an IP-shaped value so the SEC_REVIEW F43 route regex
+        // (`[0-9a-fA-F.:%]+`) accepts it and the request reaches the
+        // controller — the test is about the controller's
+        // ApiNotFoundException → Twig 404 path, not the route layer.
         $this->enqueueApiResponse(404, ['error' => 'not_found']);
 
-        $response = $this->request('GET', '/app/ips/not-an-ip');
+        $response = $this->request('GET', '/app/ips/198.51.100.99');
 
         self::assertSame(404, $response->getStatusCode());
         self::assertStringContainsString('IP not found', (string) $response->getBody());