fix: tighten /internal/* gate to loopback by default (SEC_REVIEW F25)

Caddy and InternalNetworkMiddleware no longer trust the entire RFC1918
universe — the default allowlist is loopback only. Caddy's
trusted_proxies likewise default to loopback so a docker-bridge
neighbour can no longer forge REMOTE_ADDR via X-Forwarded-For. The
sidecar scheduler joins the api's network namespace
(network_mode: "service:api") so its tick calls land on 127.0.0.1
without weakening the gate. Operators with broader topologies opt in
via INTERNAL_CIDR_ALLOWLIST / TRUSTED_PROXIES env vars.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

.env.example

@@ -40,6 +40,22 @@ SCORE_REPORT_HARD_CUTOFF_DAYS=365
 
 # Internal jobs
 INTERNAL_JOB_TOKEN=                       # 32-byte hex
+# Comma- or whitespace-separated CIDR list of sources allowed to reach
+# /internal/*. Empty (the default) means loopback-only (127.0.0.1/32 +
+# ::1/128). The bundled `compose.scheduler.yml` shares the api's network
+# namespace, so its calls hit loopback and need no extra entries.
+# Production topologies that genuinely need extra sources (host cron on
+# a private bridge, etc.) list them here AND mirror them into the api
+# Caddyfile's @internal matcher.
+INTERNAL_CIDR_ALLOWLIST=
+# Comma- or whitespace-separated CIDR list of trusted reverse-proxy IPs
+# whose `X-Forwarded-For` header Caddy should honour for REMOTE_ADDR
+# rewriting. Default (loopback-only) means no XFF rewriting from any
+# real client. Set to your reverse proxy's CIDR (e.g. "10.0.0.5/32") if
+# the api sits behind one — required for accurate audit-log source IPs.
+# SEC_REVIEW F25: never include broad RFC1918 ranges in deployments
+# where untrusted neighbours can reach the api on the same docker bridge.
+TRUSTED_PROXIES=
 JOB_RECOMPUTE_MAX_RUNTIME_SECONDS=240
 JOB_RECOMPUTE_MAX_ROWS_PER_TICK=5000
 JOB_AUDIT_RETENTION_DAYS=180

SPEC.md

@@ -322,18 +322,29 @@ Bound endpoints in `Caddyfile`:
 ```caddy
 @internal {
     path /internal/*
-    remote_ip 127.0.0.1/32 ::1/128 172.16.0.0/12 10.0.0.0/8 192.168.0.0/16
+    remote_ip 127.0.0.1/32 ::1/128
 }
 handle @internal {
     php
 }
 @external_internal_blocked {
     path /internal/*
-    not remote_ip 127.0.0.1/32 ::1/128 172.16.0.0/12 10.0.0.0/8 192.168.0.0/16
+    not remote_ip 127.0.0.1/32 ::1/128
 }
 respond @external_internal_blocked 404
 ```
 
+The default loopback-only matcher (and the matching default in
+`InternalNetworkMiddleware`) is the post-F25 hardening: trusting the
+entire RFC1918 universe meant any docker-bridge neighbour could reach
+`:8081` from a private IP and forge `REMOTE_ADDR` via XFF. The bundled
+`compose.scheduler.yml` joins the api's network namespace
+(`network_mode: "service:api"`) so its tick calls land on `127.0.0.1`;
+host-cron / systemd timer deployments target `localhost`. Production
+topologies that genuinely need extra source CIDRs list them in the
+`INTERNAL_CIDR_ALLOWLIST` env var (PHP) AND mirror them into the
+Caddyfile matcher above.
+
 The OpenAPI document includes Public and Admin groups. Auth and Internal endpoints are documented separately in `doc/auth-flows.md` (they are not part of the public contract — frontends call Admin endpoints).
 
 ### CORS
@@ -709,6 +720,11 @@ services:
     build: { context: ./scheduler }
     environment:
       INTERNAL_JOB_TOKEN: ${INTERNAL_JOB_TOKEN}
+    # SEC_REVIEW F25: share the api's network namespace so the crontab
+    # can hit /internal/jobs/tick over loopback. The api's /internal/*
+    # gate is restricted to 127.0.0.1/::1 on both Caddy and PHP layers
+    # to keep docker-bridge neighbours out.
+    network_mode: "service:api"
     read_only: true
     tmpfs:
       - /run:mode=0755
@@ -723,7 +739,7 @@ services:
 
 `scheduler/scheduler.crontab` (baked into the image):
 ```cron
-* * * * * curl -sf -m 280 -X POST -H "Authorization: Bearer $INTERNAL_JOB_TOKEN" http://api:8081/internal/jobs/tick > /dev/null
+* * * * * curl -sf -m 280 -X POST -H "Authorization: Bearer $INTERNAL_JOB_TOKEN" http://localhost:8081/internal/jobs/tick > /dev/null
 ```
 
 Started with: `docker compose -f docker-compose.yml -f compose.scheduler.yml up -d`.

api/config/settings.php

@@ -44,6 +44,7 @@ return [
     ],
     'ui_service_token' => getenv('UI_SERVICE_TOKEN') ?: '',
     'internal_job_token' => getenv('INTERNAL_JOB_TOKEN') ?: '',
+    'internal_cidr_allowlist' => (string) (getenv('INTERNAL_CIDR_ALLOWLIST') ?: ''),
     'ui_origin' => getenv('UI_ORIGIN') ?: 'http://localhost:8080',
     'oidc_default_role' => $oidcDefaultRole,
     'score_hard_cutoff_days' => (int) (getenv('SCORE_REPORT_HARD_CUTOFF_DAYS') ?: 365),

api/docker/Caddyfile

@@ -5,8 +5,21 @@
     order php_server before file_server
     auto_https off
     admin off
+    # SEC_REVIEW F25: trust ONLY loopback as an XFF rewriter by default.
+    # The previous `trusted_proxies static private_ranges` honoured
+    # X-Forwarded-For from any RFC1918 peer. Combined with the wide
+    # /internal/* CIDR gate, a neighbouring container on the same docker
+    # bridge could forge `REMOTE_ADDR=127.0.0.1` via XFF and pass the
+    # network check. The bundled docker-compose stack has no real proxy
+    # in front of the api, so loopback-only is the correct default.
+    #
+    # Production deployments behind a real reverse proxy override the
+    # `TRUSTED_PROXIES` env var to that proxy's CIDR — for example:
+    #     TRUSTED_PROXIES="10.0.0.5/32"
+    # The default keeps `remote_ip` matchers below evaluating against the
+    # real TCP peer regardless of any XFF header.
     servers {
-        trusted_proxies static private_ranges
+        trusted_proxies static {$TRUSTED_PROXIES:127.0.0.1/32 ::1/128}
     }
 }
 
@@ -43,13 +56,19 @@
     @not_docs not path /api/docs /api/v1/openapi.yaml
     header @not_docs Content-Security-Policy "default-src 'none'; frame-ancestors 'none'; base-uri 'none'"
 
-    # Internal jobs API: only callable from loopback / RFC1918.
-    # The PHP layer also enforces this (InternalNetworkMiddleware) — Caddy
-    # is the first line of defence for production deployments where the
-    # api is reachable from the public internet.
+    # Internal jobs API: only callable from loopback by default
+    # (SEC_REVIEW F25). The bundled sidecar scheduler joins the api's
+    # network namespace via `network_mode: "service:api"`, so its calls
+    # arrive on 127.0.0.1. Host-cron Option A in the SPEC also targets
+    # localhost. Production topologies that genuinely need wider
+    # reachability set `INTERNAL_CIDR_ALLOWLIST` for the PHP middleware
+    # AND mirror the same CIDR list in this matcher (operators must
+    # patch the Caddyfile or override via a custom config).
+    # The PHP layer also enforces this (InternalNetworkMiddleware) —
+    # Caddy is the first line of defence.
     @internal {
         path /internal/*
-        remote_ip 127.0.0.1/32 ::1/128 172.16.0.0/12 10.0.0.0/8 192.168.0.0/16
+        remote_ip 127.0.0.1/32 ::1/128
     }
     handle @internal {
         php_server
@@ -57,7 +76,7 @@
 
     @external_internal_blocked {
         path /internal/*
-        not remote_ip 127.0.0.1/32 ::1/128 172.16.0.0/12 10.0.0.0/8 192.168.0.0/16
+        not remote_ip 127.0.0.1/32 ::1/128
     }
     respond @external_internal_blocked 404
 

api/src/App/Container.php

@@ -130,6 +130,7 @@ final class Container
             'settings.score_hard_cutoff_days' => (int) ($settings['score_hard_cutoff_days'] ?? 365),
             'settings.rate_limit_per_second' => (int) ($settings['rate_limit_per_second'] ?? 60),
             'settings.internal_job_token' => (string) ($settings['internal_job_token'] ?? ''),
+            'settings.internal_cidr_allowlist' => (string) ($settings['internal_cidr_allowlist'] ?? ''),
             'settings.job_recompute_max_runtime_seconds' => (int) ($settings['job_recompute_max_runtime_seconds'] ?? 240),
             'settings.job_recompute_max_rows_per_tick' => (int) ($settings['job_recompute_max_rows_per_tick'] ?? 5000),
             'settings.job_audit_retention_days' => (int) ($settings['job_audit_retention_days'] ?? 180),
@@ -392,7 +393,14 @@ final class Container
                 return $registry;
             }),
             JobsController::class => autowire(),
-            InternalNetworkMiddleware::class => autowire(),
+            InternalNetworkMiddleware::class => factory(static function (ContainerInterface $c): InternalNetworkMiddleware {
+                /** @var ResponseFactoryInterface $rf */
+                $rf = $c->get(ResponseFactoryInterface::class);
+                /** @var string $raw */
+                $raw = $c->get('settings.internal_cidr_allowlist');
+
+                return new InternalNetworkMiddleware($rf, InternalNetworkMiddleware::parseCidrList($raw));
+            }),
             InternalTokenMiddleware::class => factory(static function (ContainerInterface $c): InternalTokenMiddleware {
                 /** @var ResponseFactoryInterface $rf */
                 $rf = $c->get(ResponseFactoryInterface::class);

api/src/Infrastructure/Http/Middleware/InternalNetworkMiddleware.php

@@ -5,6 +5,7 @@ declare(strict_types=1);
 namespace App\Infrastructure\Http\Middleware;
 
 use App\Domain\Ip\Cidr;
+use App\Domain\Ip\InvalidCidrException;
 use App\Domain\Ip\InvalidIpException;
 use App\Domain\Ip\IpAddress;
 use Psr\Http\Message\ResponseFactoryInterface;
@@ -15,36 +16,84 @@ use Psr\Http\Server\RequestHandlerInterface;
 
 /**
  * Belt-and-suspenders network gate for `/internal/*`. Caddyfile already
- * 404s public-IP sources before the request reaches PHP, but this
- * middleware enforces the same rule when Slim is invoked directly (tests,
- * misconfigurations, future HTTP servers).
+ * 404s out-of-policy sources before the request reaches PHP, but this
+ * middleware enforces the same rule when Slim is invoked directly
+ * (tests, misconfigurations, future HTTP servers).
  *
  * Returns `404` rather than `403` so external observers can't tell the
- * endpoint exists. RFC1918 + loopback are allowed; everything else is
- * silently dropped.
+ * endpoint exists.
+ *
+ * SEC_REVIEW F25: defaults to loopback only (`127.0.0.1/32` + `::1/128`).
+ * Trusting the entire RFC1918 universe meant a neighbour container on a
+ * shared docker bridge could reach `:8081` from a 172.16/12 source and —
+ * combined with the wide `trusted_proxies private_ranges` Caddy default
+ * — forge `REMOTE_ADDR=127.0.0.1` via `X-Forwarded-For`. The bundled
+ * sidecar scheduler (`compose.scheduler.yml`) now joins the api's
+ * network namespace (`network_mode: "service:api"`) so its calls land
+ * on `127.0.0.1`. Production topologies that genuinely need extra
+ * source CIDRs (e.g. a host-cron VM with a private bridge) opt in via
+ * the `INTERNAL_CIDR_ALLOWLIST` env var.
  */
 final class InternalNetworkMiddleware implements MiddlewareInterface
 {
     /** @var list<string> */
-    private const ALLOWED_CIDRS = [
+    public const DEFAULT_ALLOWED_CIDRS = [
         '127.0.0.1/32',
         '::1/128',
-        '10.0.0.0/8',
-        '172.16.0.0/12',
-        '192.168.0.0/16',
     ];
 
     /** @var list<Cidr> */
     private array $cidrs;
 
-    public function __construct(private readonly ResponseFactoryInterface $responseFactory)
-    {
+    /**
+     * @param list<string>|null $allowedCidrs explicit allowlist (each entry is a
+     *                                        CIDR string parsed by `Cidr::fromString`).
+     *                                        `null` or `[]` falls back to
+     *                                        {@see DEFAULT_ALLOWED_CIDRS}. Invalid
+     *                                        entries throw at construction time so a
+     *                                        misconfigured deploy fails to boot rather
+     *                                        than silently widening the gate.
+     */
+    public function __construct(
+        private readonly ResponseFactoryInterface $responseFactory,
+        ?array $allowedCidrs = null,
+    ) {
+        $list = ($allowedCidrs === null || $allowedCidrs === []) ? self::DEFAULT_ALLOWED_CIDRS : $allowedCidrs;
         $this->cidrs = array_map(
             static fn (string $c): Cidr => Cidr::fromString($c),
-            self::ALLOWED_CIDRS,
+            $list,
         );
     }
 
+    /**
+     * Parses a comma- or whitespace-separated CIDR list (typically
+     * `INTERNAL_CIDR_ALLOWLIST` from env). Returns the validated list of
+     * CIDR strings ready to feed into the constructor; empty input
+     * returns an empty list so the constructor falls back to the
+     * loopback default. Invalid CIDRs throw — fail-closed.
+     *
+     * @return list<string>
+     * @throws InvalidCidrException
+     */
+    public static function parseCidrList(string $raw): array
+    {
+        if (trim($raw) === '') {
+            return [];
+        }
+        $parts = preg_split('/[\s,]+/', trim($raw)) ?: [];
+        $out = [];
+        foreach ($parts as $p) {
+            if ($p === '') {
+                continue;
+            }
+            // Validate by constructing — discards the Cidr but raises on bad input.
+            Cidr::fromString($p);
+            $out[] = $p;
+        }
+
+        return $out;
+    }
+
     public function process(ServerRequestInterface $request, RequestHandlerInterface $handler): ResponseInterface
     {
         $remote = self::remoteAddr($request);

api/tests/Unit/Http/InternalNetworkMiddlewareTest.php

@@ -4,6 +4,7 @@ declare(strict_types=1);
 
 namespace App\Tests\Unit\Http;
 
+use App\Domain\Ip\InvalidCidrException;
 use App\Infrastructure\Http\Middleware\InternalNetworkMiddleware;
 use PHPUnit\Framework\Attributes\DataProvider;
 use PHPUnit\Framework\TestCase;
@@ -14,25 +15,29 @@ use Slim\Psr7\Factory\ResponseFactory;
 use Slim\Psr7\Factory\ServerRequestFactory;
 
 /**
- * Network gate must let RFC1918 + loopback through, 404 everything else,
- * and never leak via 403 (which would tell attackers the endpoint exists).
- * The handler is stubbed to a marker response so we can confirm whether
- * the middleware short-circuited or passed through.
+ * Network gate must let only loopback through by default (SEC_REVIEW F25),
+ * 404 everything else, and never leak via 403 (which would tell attackers
+ * the endpoint exists). The handler is stubbed to a marker response so we
+ * can confirm whether the middleware short-circuited or passed through.
+ *
+ * RFC1918 is no longer in the default allowlist: an operator who needs
+ * non-loopback sources must opt in via `INTERNAL_CIDR_ALLOWLIST`.
  */
 final class InternalNetworkMiddlewareTest extends TestCase
 {
     /**
      * @return iterable<string, array{string, bool}>
      */
-    public static function addressProvider(): iterable
+    public static function defaultAddressProvider(): iterable
     {
         yield 'loopback v4' => ['127.0.0.1', true];
         yield 'loopback v6' => ['::1', true];
-        yield 'rfc1918 10/8' => ['10.5.6.7', true];
-        yield 'rfc1918 172.16/12' => ['172.16.42.1', true];
-        yield 'rfc1918 172.31/12 (boundary)' => ['172.31.255.255', true];
+        // RFC1918 sources are no longer allowed by default — F25.
+        yield 'rfc1918 10/8 rejected by default' => ['10.5.6.7', false];
+        yield 'rfc1918 172.16/12 rejected by default' => ['172.16.42.1', false];
+        yield 'rfc1918 172.31/12 (boundary) rejected by default' => ['172.31.255.255', false];
         yield 'just outside 172.16/12' => ['172.32.0.1', false];
-        yield 'rfc1918 192.168/16' => ['192.168.1.1', true];
+        yield 'rfc1918 192.168/16 rejected by default' => ['192.168.1.1', false];
         yield 'public 1.1.1.1' => ['1.1.1.1', false];
         yield 'public v4' => ['203.0.113.4', false];
         yield 'public v6' => ['2001:db8::1', false];
@@ -40,36 +45,93 @@ final class InternalNetworkMiddlewareTest extends TestCase
         yield 'empty' => ['', false];
     }
 
-    #[DataProvider('addressProvider')]
-    public function testNetworkGate(string $remoteAddr, bool $shouldPass): void
+    #[DataProvider('defaultAddressProvider')]
+    public function testDefaultLoopbackOnlyGate(string $remoteAddr, bool $shouldPass): void
     {
-        $middleware = new InternalNetworkMiddleware(new ResponseFactory());
+        $this->assertGate(new InternalNetworkMiddleware(new ResponseFactory()), $remoteAddr, $shouldPass);
+    }
+
+    public function testNullAllowedCidrsFallsBackToLoopbackDefault(): void
+    {
+        $middleware = new InternalNetworkMiddleware(new ResponseFactory(), null);
+        $this->assertGate($middleware, '127.0.0.1', true);
+        $this->assertGate($middleware, '10.0.0.1', false);
+    }
+
+    public function testEmptyAllowedCidrsFallsBackToLoopbackDefault(): void
+    {
+        $middleware = new InternalNetworkMiddleware(new ResponseFactory(), []);
+        $this->assertGate($middleware, '::1', true);
+        $this->assertGate($middleware, '192.168.1.1', false);
+    }
+
+    public function testCustomAllowlistAdmitsConfiguredSourcesOnly(): void
+    {
+        // Operator opt-in: extend allowlist to a single bridge IP.
+        $middleware = new InternalNetworkMiddleware(
+            new ResponseFactory(),
+            ['127.0.0.1/32', '::1/128', '172.20.0.5/32'],
+        );
+        $this->assertGate($middleware, '127.0.0.1', true);
+        $this->assertGate($middleware, '::1', true);
+        $this->assertGate($middleware, '172.20.0.5', true);
+        // Still narrow — 172.20.0.6 is one off and rejected.
+        $this->assertGate($middleware, '172.20.0.6', false);
+        // The wider 10/8 block was not configured.
+        $this->assertGate($middleware, '10.0.0.1', false);
+    }
+
+    public function testInvalidCidrInConstructorFailsClosed(): void
+    {
+        $this->expectException(\Throwable::class);
+        new InternalNetworkMiddleware(new ResponseFactory(), ['not-a-cidr']);
+    }
 
+    public function testParseCidrListAcceptsMixedSeparators(): void
+    {
+        self::assertSame(
+            ['127.0.0.1/32', '::1/128', '10.0.0.0/8'],
+            InternalNetworkMiddleware::parseCidrList('127.0.0.1/32, ::1/128 10.0.0.0/8'),
+        );
+    }
+
+    public function testParseCidrListReturnsEmptyForBlank(): void
+    {
+        self::assertSame([], InternalNetworkMiddleware::parseCidrList(''));
+        self::assertSame([], InternalNetworkMiddleware::parseCidrList('   '));
+    }
+
+    public function testParseCidrListThrowsOnInvalidEntry(): void
+    {
+        $this->expectException(InvalidCidrException::class);
+        InternalNetworkMiddleware::parseCidrList('127.0.0.1/32, garbage');
+    }
+
+    private function assertGate(InternalNetworkMiddleware $middleware, string $remoteAddr, bool $shouldPass): void
+    {
         $req = (new ServerRequestFactory())->createServerRequest(
             'POST',
             '/internal/jobs/tick',
             ['REMOTE_ADDR' => $remoteAddr],
         );
-
         $passthrough = new class () implements RequestHandlerInterface {
             public bool $reached = false;
 
             public function handle(ServerRequestInterface $request): ResponseInterface
             {
                 $this->reached = true;
-                $factory = new ResponseFactory();
 
-                return $factory->createResponse(204);
+                return (new ResponseFactory())->createResponse(204);
             }
         };
 
         $response = $middleware->process($req, $passthrough);
 
         if ($shouldPass) {
-            self::assertSame(204, $response->getStatusCode());
+            self::assertSame(204, $response->getStatusCode(), $remoteAddr . ' should be allowed');
             self::assertTrue($passthrough->reached);
         } else {
-            self::assertSame(404, $response->getStatusCode());
+            self::assertSame(404, $response->getStatusCode(), $remoteAddr . ' should be denied');
             self::assertFalse($passthrough->reached, 'handler must not see disallowed sources');
         }
     }

compose.scheduler.yml

@@ -4,6 +4,14 @@ services:
     build: { context: ./scheduler }
     environment:
       INTERNAL_JOB_TOKEN: ${INTERNAL_JOB_TOKEN}
+    # SEC_REVIEW F25: share the api container's network namespace so the
+    # baked crontab can hit /internal/jobs/tick over loopback. The api's
+    # /internal/* gate is restricted to 127.0.0.1/::1 (Caddy + PHP), so
+    # the previous `api:8081` URL from a sibling docker-bridge peer
+    # would now 404 — and that's the goal: a malicious neighbour on the
+    # same bridge can no longer reach /internal/*. Side effect: this
+    # service has no DNS / its own ports, but it needs neither.
+    network_mode: "service:api"
     # SEC_REVIEW F22: dependencies (curl, tini, ca-certificates) are now
     # baked into the image at build time with pinned versions, against a
     # digest-pinned alpine base. The previous `image: alpine:3` +

scheduler/scheduler.crontab

@@ -7,4 +7,11 @@
 #
 # -m 280 caps the request below the 1-minute cadence so we never queue
 # overlapping ticks.
-* * * * * curl -sf -m 280 -X POST -H "Authorization: Bearer $INTERNAL_JOB_TOKEN" http://api:8081/internal/jobs/tick > /dev/null
+#
+# SEC_REVIEW F25: target localhost — the scheduler service uses
+# `network_mode: "service:api"` so it shares the api container's network
+# namespace and reaches FrankenPHP via loopback. The api's /internal/*
+# gate is now loopback-only on both Caddy and PHP layers; reaching it
+# from a sibling docker-bridge peer (the previous `http://api:8081`
+# routing) would 404.
+* * * * * curl -sf -m 280 -X POST -H "Authorization: Bearer $INTERNAL_JOB_TOKEN" http://localhost:8081/internal/jobs/tick > /dev/null