Browse Source

Fix R01-N05 + R01-N07: trusted-proxy aware HTTPS + client IP

These two findings are paired and ship together because they share the
same configuration surface — `TRUSTED_PROXIES`, the comma-separated CIDR
list of reverse proxies allowed to speak `X-Forwarded-For` /
`X-Forwarded-Proto` on behalf of the user.

Before this change:

  - `Request::ip()` returned `REMOTE_ADDR` verbatim, so behind any
    reverse proxy / load balancer every audit row recorded the proxy's
    address instead of the user's. The R01-N06 throttle bucket was
    keyed on the same (wrong) value.
  - `SessionGuard::start()` derived the cookie `Secure` flag purely
    from `APP_BASE_URL` starting with `https://`. A TLS-terminating
    proxy that forwarded over plain HTTP, or an `APP_BASE_URL` typo,
    silently downgraded the cookie.
  - `public/index.php` neither redirected HTTP→HTTPS nor refused to
    serve auth flows over HTTP — a downgrade attack had no fence.

New `src/Http/TrustedProxies.php` owns the policy:

  - `fromEnv()` reads `TRUSTED_PROXIES`, comma-separated CIDRs (bare
    addresses are normalised to `/32` for IPv4 / `/128` for IPv6).
    Blank ⇒ no trust at all.
  - `clientIp(\$server)` walks `X-Forwarded-For` rightmost-to-leftmost
    (with `REMOTE_ADDR` logically appended) and returns the first hop
    that is not in any configured CIDR. If `REMOTE_ADDR` itself is not
    trusted the header is ignored — so a hostile direct client cannot
    forge an audit IP. Hops with an attached port (`1.2.3.4:51234`,
    `[::1]:443`) are normalised by stripping the port.
  - `isHttps(\$server)` honours server-side TLS unconditionally
    (`HTTPS=on`, `REQUEST_SCHEME=https`, port 443) and additionally
    accepts `X-Forwarded-Proto: https` from a trusted peer. RFC-style
    multi-hop lists ("https, http") read leftmost-first — what the
    outermost proxy actually saw.

Wiring:

  - `src/Http/Request.php`: `ip()` now goes through the helper, so the
    audit pipeline (`AuditLogger::audit`) and the R01-N06 throttle
    bucket key both fix themselves with no controller changes. New
    `isHttps()` exposes the same decision to the rest of the app.
  - `src/Auth/SessionGuard.php::start()`: marks the cookie `Secure`
    when EITHER `APP_BASE_URL` is HTTPS OR the live request is
    effectively HTTPS. A TLS-terminated proxy hop no longer silently
    downgrades the cookie.
  - `public/index.php`: adds a 308 HTTP→HTTPS redirect when
    `APP_BASE_URL` is HTTPS AND the request is *provably* HTTP — i.e.
    no `TRUSTED_PROXIES` configured at all (so `REMOTE_ADDR` is the
    user) or a trusted proxy explicitly reported `X-Forwarded-Proto:
    http`. The redirect is deliberately suppressed when the proxy is
    silent about the scheme: a TLS-terminating proxy that forgot to
    forward the header would otherwise infinite-loop. `/healthz` is
    exempt outright. HSTS continues to be emitted whenever
    `APP_BASE_URL` is HTTPS.

Untrusted forwarded headers are ignored across the board, so the
default — \`TRUSTED_PROXIES=\` blank — is the safest posture and an
operator must opt in by listing their proxy CIDRs.

Operator docs:

  - `.env.example`: new \`TRUSTED_PROXIES=\` block with rationale and
    examples for IPv4 / IPv6 CIDRs and bare-host shorthand.
  - `doc/admin-manual.md`: new §3.5 "Reverse proxy and HTTPS" with
    the proxy contract (forward XFF, forward XFP, sit inside a listed
    CIDR), explanation of the redirect's loop-safe predicate, and an
    Nginx \`proxy_set_header\` snippet. Old §3.5 / §3.6 → §3.6 / §3.7;
    cross-references in §5.4 + the troubleshooting table updated.

Tests: 202 / 533 (was 184 / 502).

  - `tests/Http/TrustedProxiesTest.php` (14 cases): direct-client
    return; trusted single-hop XFF lookup; multi-hop walk past two
    trusted internal proxies; all-hops-trusted falls back to leftmost;
    untrusted REMOTE_ADDR ignores XFF entirely; port-stripping;
    IPv6 CIDR; bare-IP host-mask shorthand; typo'd CIDR doesn't widen
    trust; \`fromEnv()\` parser handles whitespace + commas; blank env
    grants no trust; \`isHttps()\` direct TLS variants; XFP trust
    gated on REMOTE_ADDR being trusted; multi-hop XFP list reads
    leftmost.
  - `tests/Http/RequestTest.php` (4 cases): wiring smoke tests that
    `Request::ip()` and `Request::isHttps()` actually consult
    `TRUSTED_PROXIES`.

Eighth fix from \`doc/REVIEW_01.md\`.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
chiappa 2 days ago
parent
commit
a2e77ea22a

+ 14 - 0
.env.example

@@ -18,6 +18,20 @@ SESSION_PATH=/var/www/data/sessions
 # 'production' disables verbose error output. Anything else is treated as dev.
 APP_ENV=production
 
+# ---------------------------------------------------------------------------
+# Reverse-proxy trust (R01-N05 / R01-N07). Comma-separated list of CIDRs of
+# the proxies in front of the app. When the immediate peer (`REMOTE_ADDR`)
+# matches one of these:
+#   * `X-Forwarded-For` is walked to find the real client IP — used for the
+#     audit log and the local-admin login throttle bucket;
+#   * `X-Forwarded-Proto: https` is honoured for cookie `Secure` / HSTS
+#     decisions, so a TLS-terminating proxy can mark requests as HTTPS.
+# Leave blank when the app is exposed directly with no reverse proxy. Examples:
+#   TRUSTED_PROXIES=10.0.0.0/8,192.168.0.0/16
+#   TRUSTED_PROXIES=172.16.0.5,2001:db8::/32
+# ---------------------------------------------------------------------------
+TRUSTED_PROXIES=
+
 # ---------------------------------------------------------------------------
 # OIDC bootstrap admin (optional) — nominate the very first administrator up
 # front, so a public-facing first deploy can't be land-grabbed by another

+ 64 - 7
doc/admin-manual.md

@@ -107,7 +107,64 @@ APP_ENV=production
 `production` silences verbose PHP errors. Any other value (e.g. `dev`)
 turns them on — useful when troubleshooting in a non-public install.
 
-### 3.5 Nominating the first administrator (OIDC)
+### 3.5 Reverse proxy and HTTPS
+
+```
+TRUSTED_PROXIES=10.0.0.0/8,192.168.0.0/16
+```
+
+Set this when the application is exposed through a reverse proxy or load
+balancer (Nginx, Traefik, Caddy, an AWS ALB, etc.). Provide a
+comma-separated list of CIDRs that contains every hop between the public
+internet and the PHP container. Bare addresses without `/n` are treated as
+host masks (`/32` for IPv4, `/128` for IPv6).
+
+When the immediate peer matches one of those CIDRs the application will:
+
+- walk `X-Forwarded-For` rightmost-to-leftmost to pick the originating
+  client IP — this is what lands in the audit log and the local-admin
+  login-throttle bucket (R01-N06 / R01-N07); and
+- honour `X-Forwarded-Proto: https` so the session cookie keeps its
+  `Secure` flag and the response carries `Strict-Transport-Security`
+  even when the proxy → container link is plain HTTP (R01-N05).
+
+Leave the variable blank when the container is exposed directly. With no
+trusted proxies configured the application ignores both forwarded headers,
+so a hostile client cannot lie its way into a different audit IP or into
+HTTPS-only behaviour.
+
+**Proxy contract.** Whichever proxy you put in front of the app must:
+
+1. Forward the original client IP in `X-Forwarded-For` (most reverse
+   proxies do this by default, but verify);
+2. Forward the original scheme in `X-Forwarded-Proto` — this is the
+   single signal the application uses to decide whether the user-facing
+   request was over TLS;
+3. Be reachable by the PHP container at an address that falls inside one
+   of the CIDRs you listed in `TRUSTED_PROXIES`.
+
+If `APP_BASE_URL` is `https://…` and the application detects that the
+live request is genuinely over HTTP (either no proxy at all, or a trusted
+proxy that explicitly reports `X-Forwarded-Proto: http`), it issues a
+permanent redirect to the canonical HTTPS URL. `/healthz` is exempt so
+liveness probes keep working over either scheme. The redirect is
+suppressed when the proxy is silent about the scheme — there is no safe
+way to distinguish "real HTTP request" from "TLS-terminating proxy that
+forgot to set the header", and a redirect in the latter case would
+infinite-loop.
+
+A typical Nginx snippet:
+
+```nginx
+location / {
+    proxy_pass         http://sprint_planer:80;
+    proxy_set_header   Host              $host;
+    proxy_set_header   X-Forwarded-For   $proxy_add_x_forwarded_for;
+    proxy_set_header   X-Forwarded-Proto $scheme;
+}
+```
+
+### 3.6 Nominating the first administrator (OIDC)
 
 Historically the first user to complete sign-in via *any* path was promoted
 to administrator. On a public-facing first deploy that is a land-grab risk
@@ -133,7 +190,7 @@ Either / both / neither may be set:
 - **Both set**: the signing user is promoted on a match against either field.
 - **One set**: only that channel matters.
 - **Neither set**: the OIDC path will *never* auto-promote. In that case
-  bootstrap the first administrator via the local-admin fallback (§3.6) or
+  bootstrap the first administrator via the local-admin fallback (§3.7) or
   by setting `users.is_admin = 1` directly in `app.sqlite`.
 
 Auto-promotion additionally requires that no administrator already exists
@@ -141,11 +198,11 @@ Auto-promotion additionally requires that no administrator already exists
 promotions go through the **Users** page (§5.1). The promotion is recorded
 in the audit log as `BOOTSTRAP_ADMIN` with `via=oidc`.
 
-The local-admin fallback (§3.6) is itself an explicit env-bootstrap and
+The local-admin fallback (§3.7) is itself an explicit env-bootstrap and
 does not require the variables above — its `BOOTSTRAP_ADMIN` audit row is
 tagged `via=local`.
 
-### 3.6 Local admin fallback (optional)
+### 3.7 Local admin fallback (optional)
 
 ```
 LOCAL_ADMIN_EMAIL=admin@example.com
@@ -199,7 +256,7 @@ with `via=local` is also recorded.
 
 For OIDC-only deployments — where you don't want the local fallback at
 all — leave `LOCAL_ADMIN_EMAIL` / `LOCAL_ADMIN_PASSWORD_HASH` blank and
-nominate the first admin via the OIDC bootstrap (§3.5).
+nominate the first admin via the OIDC bootstrap (§3.6).
 
 To disable the fallback later, blank both variables and restart.
 
@@ -354,7 +411,7 @@ docker compose up -d
 ```
 
 Migrations run again on the next request. The first administrator is
-re-seeded as described in §3.5 (OIDC bootstrap) or §3.6 (local-admin
+re-seeded as described in §3.6 (OIDC bootstrap) or §3.7 (local-admin
 fallback) — there is no longer an unconditional "first sign-in becomes
 admin" path.
 
@@ -377,7 +434,7 @@ before pulling.
 | Symptom | Likely cause | Fix |
 |---|---|---|
 | `/auth/login` returns "redirect URI mismatch" | The redirect URI registered in Entra does not equal `{APP_BASE_URL}/auth/callback`. | Update either `APP_BASE_URL` in `.env` or the Entra app registration. |
-| `/auth/local` returns 404 | `LOCAL_ADMIN_EMAIL` or `LOCAL_ADMIN_PASSWORD_HASH` is blank. | Set both (see §3.6 for the hash recipe), then `docker compose restart`. |
+| `/auth/local` returns 404 | `LOCAL_ADMIN_EMAIL` or `LOCAL_ADMIN_PASSWORD_HASH` is blank. | Set both (see §3.7 for the hash recipe), then `docker compose restart`. |
 | Sign-in succeeds but every page shows "not authorised" | The user has no `is_admin` flag and is trying to access an admin-only page. | Promote them via Users (logged in as another admin). |
 | Container restarts in a loop | Most often a malformed `.env` line or a permission problem on `./data`. | `docker compose logs` will show the PHP fatal. Check that `./data` is writable by the `www-data` user inside the container (uid 33 on the Apache image). |
 | CSS looks broken / unstyled | The Node build stage was skipped or used a stale layer. | `docker compose build --no-cache` and restart. |

+ 44 - 2
public/index.php

@@ -18,6 +18,7 @@ use App\Db\Migrator;
 use App\Http\Request;
 use App\Http\Response;
 use App\Http\Router;
+use App\Http\TrustedProxies;
 use App\Http\View;
 use App\Repositories\AppSettingsRepository;
 use App\Repositories\AuditRepository;
@@ -214,12 +215,53 @@ $router->post('/settings',                            $settingsCtrl->update(...)
 // ---------------------------------------------------------------------------
 // Dispatch
 // ---------------------------------------------------------------------------
-$request  = Request::fromGlobals();
+$request = Request::fromGlobals();
+
+// R01-N05: when `APP_BASE_URL` declares HTTPS, refuse to serve sensitive
+// flows over plain HTTP — redirect the user to the canonical scheme before
+// any controller, session, or auth logic runs. `/healthz` is exempt so
+// liveness probes continue to work over either scheme. The decision uses
+// the trusted-proxy helper so a TLS-terminating reverse proxy can pass
+// `X-Forwarded-Proto: https` and the app will treat the request as secure.
+$baseUrl       = (string) (getenv('APP_BASE_URL') ?: '');
+$baseIsHttps   = str_starts_with($baseUrl, 'https://');
+$proxies       = TrustedProxies::fromEnv();
+$requestIsHttps = $proxies->isHttps($_SERVER);
+
+// Only redirect when we can be SURE the live request is genuinely HTTP —
+// otherwise a TLS proxy that forgot to set `X-Forwarded-Proto` would loop
+// forever (proxy talks HTTPS to user, talks HTTP to us, we redirect, …).
+// Sure cases:
+//   * no `TRUSTED_PROXIES` configured → REMOTE_ADDR is the user, so the
+//     server-side scheme is authoritative;
+//   * `TRUSTED_PROXIES` configured AND REMOTE_ADDR is a trusted proxy AND
+//     it explicitly told us `X-Forwarded-Proto: http`.
+$xfpRaw  = (string) ($_SERVER['HTTP_X_FORWARDED_PROTO'] ?? '');
+$xfp     = strtolower(trim(strtok($xfpRaw, ',') ?: ''));
+$remote  = (string) ($_SERVER['REMOTE_ADDR'] ?? '');
+$noProxy = getenv('TRUSTED_PROXIES') === false || trim((string) getenv('TRUSTED_PROXIES')) === '';
+$knownHttp = !$requestIsHttps && (
+    $noProxy
+    || ($remote !== '' && $proxies->isTrusted($remote) && $xfp === 'http')
+);
+
+if ($baseIsHttps && $knownHttp && $request->path !== '/healthz') {
+    $target = rtrim($baseUrl, '/') . ($_SERVER['REQUEST_URI'] ?? '/');
+    Response::redirect($target, 308)->send();
+    if (ob_get_level() > 0) {
+        @ob_end_flush();
+    }
+    exit;
+}
+
 $response = $router->dispatch($request);
 
 // Apply security headers to every response (spec §9). Kept here (instead of
 // Response::send) so the policy is visible + editable in one place.
-$isHttps = str_starts_with((string) (getenv('APP_BASE_URL') ?: ''), 'https://');
+// HSTS is emitted whenever the canonical base URL is HTTPS — sticking the
+// header on plain-HTTP responses is harmless (browsers ignore it from
+// non-secure contexts) and avoids a gap during the very first redirect.
+$isHttps = $baseIsHttps;
 
 // Strict CSP (Phase 11 + Phase 19). Tailwind is pre-compiled at image-build
 // time, jQuery / jQuery UI are gone, and Alpine (CSP build), htmx, and

+ 10 - 1
src/Auth/SessionGuard.php

@@ -7,6 +7,7 @@ namespace App\Auth;
 use App\Domain\User;
 use App\Http\Request;
 use App\Http\Response;
+use App\Http\TrustedProxies;
 use App\Repositories\UserRepository;
 
 /**
@@ -38,8 +39,16 @@ final class SessionGuard
             session_save_path($savePath);
         }
 
+        // R01-N05: mark the cookie Secure when *either* the configured base
+        // URL is HTTPS *or* the live request is effectively HTTPS (which
+        // can include `X-Forwarded-Proto: https` from a trusted proxy —
+        // see TrustedProxies). This way an operator who forgets to set
+        // APP_BASE_URL still gets Secure cookies as soon as TLS is in
+        // play, and the cookie isn't dropped to plain text just because
+        // a single internal probe arrived over HTTP.
         $baseUrl = (string) (getenv('APP_BASE_URL') ?: '');
-        $secure  = str_starts_with($baseUrl, 'https://');
+        $secure  = str_starts_with($baseUrl, 'https://')
+            || TrustedProxies::fromEnv()->isHttps($_SERVER);
 
         ini_set('session.use_strict_mode', '1');
         ini_set('session.use_only_cookies', '1');

+ 14 - 2
src/Http/Request.php

@@ -100,10 +100,22 @@ final class Request
         return is_array($data) ? $data : null;
     }
 
+    /**
+     * The originating client IP, honouring `X-Forwarded-For` only when the
+     * direct peer is in a configured `TRUSTED_PROXIES` CIDR (R01-N07).
+     */
     public function ip(): string
     {
-        $v = $this->server['REMOTE_ADDR'] ?? '';
-        return is_scalar($v) ? (string) $v : '';
+        return TrustedProxies::fromEnv()->clientIp($this->server);
+    }
+
+    /**
+     * True iff the request is effectively over HTTPS, after consulting the
+     * trusted-proxy `X-Forwarded-Proto` header (R01-N05).
+     */
+    public function isHttps(): bool
+    {
+        return TrustedProxies::fromEnv()->isHttps($this->server);
     }
 
     public function userAgent(): string

+ 249 - 0
src/Http/TrustedProxies.php

@@ -0,0 +1,249 @@
+<?php
+
+declare(strict_types=1);
+
+namespace App\Http;
+
+/**
+ * Trusted-proxy helper for `X-Forwarded-For` and `X-Forwarded-Proto`
+ * (R01-N05 / R01-N07).
+ *
+ * Operators populate `TRUSTED_PROXIES` in `.env` with a comma-separated list
+ * of CIDRs (e.g. `10.0.0.0/8, 192.168.0.0/16, 2001:db8::/32`). Bare IPs
+ * without `/n` are treated as `/32` (IPv4) or `/128` (IPv6). With the env
+ * var blank — the default — the helper trusts no proxy and returns
+ * `REMOTE_ADDR` verbatim.
+ *
+ * Two consumers:
+ *
+ *   - `clientIp(array $server)` walks rightmost-to-leftmost through
+ *     `X-Forwarded-For` (with `REMOTE_ADDR` appended logically), returning
+ *     the first hop that is not a trusted proxy. This is the IP that goes
+ *     into the audit log and the local-admin throttle bucket (R01-N07).
+ *
+ *   - `isHttps(array $server)` recognises a request as HTTPS when either
+ *     the server-side scheme is already TLS (`HTTPS=on`, port 443, etc.)
+ *     OR `X-Forwarded-Proto: https` arrived from a trusted proxy
+ *     (R01-N05).
+ *
+ * Untrusted forwarded headers are ignored; an attacker can't lie their way
+ * into a different audit IP or HTTPS posture by hand-crafting request
+ * headers, because their transport-layer `REMOTE_ADDR` will not match a
+ * configured CIDR.
+ */
+final class TrustedProxies
+{
+    /** @var array<int, array{0:string,1:int}> list of [networkBin, prefixBits] */
+    private array $cidrs;
+
+    /**
+     * @param string[] $cidrs CIDR strings such as "10.0.0.0/8" or "127.0.0.1".
+     *                        Invalid entries are silently skipped — operators
+     *                        get the secure default (no trust) on a typo.
+     */
+    public function __construct(array $cidrs)
+    {
+        $parsed = [];
+        foreach ($cidrs as $raw) {
+            $entry = self::parseCidr((string) $raw);
+            if ($entry !== null) {
+                $parsed[] = $entry;
+            }
+        }
+        $this->cidrs = $parsed;
+    }
+
+    /** Build from `TRUSTED_PROXIES`. Empty / unset → trusts nothing. */
+    public static function fromEnv(): self
+    {
+        $raw = getenv('TRUSTED_PROXIES');
+        if (!is_string($raw) || trim($raw) === '') {
+            return new self([]);
+        }
+        $parts = array_filter(
+            array_map('trim', explode(',', $raw)),
+            static fn(string $p): bool => $p !== '',
+        );
+        return new self(array_values($parts));
+    }
+
+    /** True iff `$ip` falls inside any configured CIDR. */
+    public function isTrusted(string $ip): bool
+    {
+        $bin = @inet_pton($ip);
+        if ($bin === false) {
+            return false;
+        }
+        foreach ($this->cidrs as [$rangeBin, $bits]) {
+            if (strlen($bin) !== strlen($rangeBin)) {
+                continue; // family mismatch (v4 vs v6)
+            }
+            if (self::matchPrefix($bin, $rangeBin, $bits)) {
+                return true;
+            }
+        }
+        return false;
+    }
+
+    /**
+     * Resolve the originating client IP for a request.
+     *
+     * Algorithm (RFC 7239 / X-Forwarded-For convention):
+     *   1. If `REMOTE_ADDR` itself is not in a trusted CIDR → return it as-is.
+     *      The forwarded header cannot be trusted from an arbitrary client.
+     *   2. Otherwise build the chain `[XFF entries…, REMOTE_ADDR]` and walk
+     *      from right (closest to us) to left (closest to the client), skipping
+     *      hops that are still trusted. The first non-trusted hop is the
+     *      client.
+     *   3. If every hop is trusted (e.g. an internal-only deployment) we fall
+     *      back to the leftmost XFF entry; that's the most informative value
+     *      we have.
+     *
+     * @param array<string,mixed> $server $_SERVER-shaped map.
+     */
+    public function clientIp(array $server): string
+    {
+        $remote = self::scalar($server, 'REMOTE_ADDR');
+        if ($remote === '') {
+            return '';
+        }
+        if (!$this->isTrusted($remote)) {
+            return $remote;
+        }
+
+        $xffRaw = self::scalar($server, 'HTTP_X_FORWARDED_FOR');
+        if ($xffRaw === '') {
+            return $remote;
+        }
+
+        $hops = array_values(array_filter(
+            array_map(
+                static fn(string $hop): string => self::stripPort(trim($hop)),
+                explode(',', $xffRaw),
+            ),
+            static fn(string $hop): bool => $hop !== '',
+        ));
+        if ($hops === []) {
+            return $remote;
+        }
+
+        // Walk rightmost-to-leftmost; skip trusted hops, return first
+        // untrusted one.
+        for ($i = count($hops) - 1; $i >= 0; $i--) {
+            $hop = $hops[$i];
+            if (!$this->isTrusted($hop)) {
+                return $hop;
+            }
+        }
+
+        // Every hop is trusted — return leftmost as the "deepest" guess.
+        return $hops[0];
+    }
+
+    /**
+     * True iff the request is effectively over HTTPS.
+     *
+     * Server-side TLS (`HTTPS=on`, port 443, `REQUEST_SCHEME=https`) is
+     * always trusted. `X-Forwarded-Proto: https` is only honoured when the
+     * immediate `REMOTE_ADDR` is in a trusted CIDR — otherwise an off-net
+     * attacker could lie their way into Secure-cookie + HSTS behaviour
+     * during a downgrade attack.
+     *
+     * @param array<string,mixed> $server $_SERVER-shaped map.
+     */
+    public function isHttps(array $server): bool
+    {
+        $https = strtolower(self::scalar($server, 'HTTPS'));
+        if ($https !== '' && $https !== 'off') {
+            return true;
+        }
+        $scheme = strtolower(self::scalar($server, 'REQUEST_SCHEME'));
+        if ($scheme === 'https') {
+            return true;
+        }
+        $port = self::scalar($server, 'SERVER_PORT');
+        if ($port === '443') {
+            return true;
+        }
+
+        $remote = self::scalar($server, 'REMOTE_ADDR');
+        if ($remote === '' || !$this->isTrusted($remote)) {
+            return false;
+        }
+        $proto = strtolower(trim(self::scalar($server, 'HTTP_X_FORWARDED_PROTO')));
+        if ($proto === '') {
+            return false;
+        }
+        // Some proxies (HAProxy, AWS ALB) emit a comma-separated list when
+        // there is more than one hop. Take the leftmost entry — the one set
+        // by the outermost proxy that actually saw the client.
+        $first = strtolower(trim(strtok($proto, ',') ?: ''));
+        return $first === 'https';
+    }
+
+    /**
+     * @return array{0:string,1:int}|null parsed CIDR or null when invalid.
+     */
+    private static function parseCidr(string $raw): ?array
+    {
+        if ($raw === '') {
+            return null;
+        }
+        $slash = strpos($raw, '/');
+        $addr  = $slash === false ? $raw : substr($raw, 0, $slash);
+        $bin   = @inet_pton($addr);
+        if ($bin === false) {
+            return null;
+        }
+        $maxBits = strlen($bin) * 8; // 32 for v4, 128 for v6
+        if ($slash === false) {
+            return [$bin, $maxBits];
+        }
+        $bitsRaw = substr($raw, $slash + 1);
+        if ($bitsRaw === '' || !ctype_digit($bitsRaw)) {
+            return null;
+        }
+        $bits = (int) $bitsRaw;
+        if ($bits < 0 || $bits > $maxBits) {
+            return null;
+        }
+        return [$bin, $bits];
+    }
+
+    /** Compare the leftmost `$bits` bits of two `inet_pton` byte strings. */
+    private static function matchPrefix(string $a, string $b, int $bits): bool
+    {
+        $fullBytes = intdiv($bits, 8);
+        if ($fullBytes > 0 && substr($a, 0, $fullBytes) !== substr($b, 0, $fullBytes)) {
+            return false;
+        }
+        $remaining = $bits % 8;
+        if ($remaining === 0) {
+            return true;
+        }
+        $mask = (~((1 << (8 - $remaining)) - 1)) & 0xFF;
+        return (ord($a[$fullBytes]) & $mask) === (ord($b[$fullBytes]) & $mask);
+    }
+
+    private static function stripPort(string $hop): string
+    {
+        if ($hop === '' || $hop[0] === '[') {
+            // bracketed IPv6: "[::1]:443" → "::1"
+            $end = strpos($hop, ']');
+            return $end === false ? $hop : substr($hop, 1, $end - 1);
+        }
+        // IPv4 with port: "1.2.3.4:5678" → "1.2.3.4". Bare IPv6 has multiple
+        // colons and no brackets; leave it alone.
+        if (substr_count($hop, ':') === 1) {
+            return substr($hop, 0, (int) strpos($hop, ':'));
+        }
+        return $hop;
+    }
+
+    /** @param array<string,mixed> $server */
+    private static function scalar(array $server, string $key): string
+    {
+        $v = $server[$key] ?? '';
+        return is_scalar($v) ? (string) $v : '';
+    }
+}

+ 93 - 0
tests/Http/RequestTest.php

@@ -0,0 +1,93 @@
+<?php
+
+declare(strict_types=1);
+
+namespace App\Tests\Http;
+
+use App\Http\Request;
+use App\Tests\TestCase;
+
+/**
+ * Smoke tests that `Request::ip()` and `Request::isHttps()` actually consult
+ * `TRUSTED_PROXIES` (R01-N05 / R01-N07). The detail-level CIDR / XFF logic
+ * is exercised in `TrustedProxiesTest`; here we only check the wiring.
+ */
+final class RequestTest extends TestCase
+{
+    /**
+     * @param array<string,mixed> $server
+     */
+    private function makeRequest(array $server): Request
+    {
+        return new Request(
+            method:  'GET',
+            path:    '/',
+            query:   [],
+            post:    [],
+            rawBody: '',
+            headers: [],
+            server:  $server,
+        );
+    }
+
+    public function testIpReturnsRemoteAddrWhenNoTrustedProxiesConfigured(): void
+    {
+        $prev = getenv('TRUSTED_PROXIES');
+        try {
+            putenv('TRUSTED_PROXIES');
+            $req = $this->makeRequest([
+                'REMOTE_ADDR'           => '203.0.113.42',
+                'HTTP_X_FORWARDED_FOR'  => '198.51.100.7',
+            ]);
+            self::assertSame('203.0.113.42', $req->ip());
+        } finally {
+            $prev === false ? putenv('TRUSTED_PROXIES') : putenv('TRUSTED_PROXIES=' . $prev);
+        }
+    }
+
+    public function testIpHonoursXffWhenRemoteIsTrusted(): void
+    {
+        $prev = getenv('TRUSTED_PROXIES');
+        try {
+            putenv('TRUSTED_PROXIES=10.0.0.0/8');
+            $req = $this->makeRequest([
+                'REMOTE_ADDR'           => '10.0.0.1',
+                'HTTP_X_FORWARDED_FOR'  => '198.51.100.7',
+            ]);
+            self::assertSame('198.51.100.7', $req->ip());
+        } finally {
+            $prev === false ? putenv('TRUSTED_PROXIES') : putenv('TRUSTED_PROXIES=' . $prev);
+        }
+    }
+
+    public function testIsHttpsHonoursXfpOnlyFromTrustedProxy(): void
+    {
+        $prev = getenv('TRUSTED_PROXIES');
+        try {
+            putenv('TRUSTED_PROXIES=10.0.0.0/8');
+
+            $trustedReq = $this->makeRequest([
+                'REMOTE_ADDR'             => '10.0.0.1',
+                'HTTP_X_FORWARDED_PROTO'  => 'https',
+            ]);
+            self::assertTrue($trustedReq->isHttps());
+
+            $untrustedReq = $this->makeRequest([
+                'REMOTE_ADDR'             => '203.0.113.5',
+                'HTTP_X_FORWARDED_PROTO'  => 'https',
+            ]);
+            self::assertFalse($untrustedReq->isHttps());
+        } finally {
+            $prev === false ? putenv('TRUSTED_PROXIES') : putenv('TRUSTED_PROXIES=' . $prev);
+        }
+    }
+
+    public function testIsHttpsRecognisesDirectTls(): void
+    {
+        $req = $this->makeRequest(['HTTPS' => 'on']);
+        self::assertTrue($req->isHttps());
+
+        $req = $this->makeRequest(['HTTPS' => 'off']);
+        self::assertFalse($req->isHttps());
+    }
+}

+ 162 - 0
tests/Http/TrustedProxiesTest.php

@@ -0,0 +1,162 @@
+<?php
+
+declare(strict_types=1);
+
+namespace App\Tests\Http;
+
+use App\Http\TrustedProxies;
+use App\Tests\TestCase;
+
+/**
+ * Pins the contract for R01-N05 + R01-N07: the proxy-aware client-IP and
+ * HTTPS detector. The class is pure (no DB/session) so we exercise it with
+ * plain `$_SERVER`-shaped arrays.
+ */
+final class TrustedProxiesTest extends TestCase
+{
+    public function testEmptyConfigReturnsRemoteAddrVerbatim(): void
+    {
+        $tp = new TrustedProxies([]);
+        self::assertSame('203.0.113.5', $tp->clientIp([
+            'REMOTE_ADDR'           => '203.0.113.5',
+            'HTTP_X_FORWARDED_FOR'  => '198.51.100.7, 192.0.2.1',
+        ]));
+    }
+
+    public function testTrustsXffWhenRemoteIsTrusted(): void
+    {
+        $tp = new TrustedProxies(['10.0.0.0/8']);
+        self::assertSame('203.0.113.5', $tp->clientIp([
+            'REMOTE_ADDR'           => '10.5.5.5',
+            'HTTP_X_FORWARDED_FOR'  => '203.0.113.5',
+        ]));
+    }
+
+    public function testWalksRightmostUntrustedHop(): void
+    {
+        // Two trusted internal hops in front of the real client.
+        $tp = new TrustedProxies(['10.0.0.0/8', '192.168.0.0/16']);
+        self::assertSame('203.0.113.5', $tp->clientIp([
+            'REMOTE_ADDR'           => '10.0.0.1',
+            'HTTP_X_FORWARDED_FOR'  => '203.0.113.5, 192.168.1.1, 10.0.0.2',
+        ]));
+    }
+
+    public function testFallsBackToLeftmostWhenAllHopsTrusted(): void
+    {
+        $tp = new TrustedProxies(['10.0.0.0/8']);
+        self::assertSame('10.0.0.7', $tp->clientIp([
+            'REMOTE_ADDR'           => '10.0.0.1',
+            'HTTP_X_FORWARDED_FOR'  => '10.0.0.7, 10.0.0.2',
+        ]));
+    }
+
+    public function testIgnoresXffWhenRemoteUntrusted(): void
+    {
+        // Attacker hits the app directly and forges X-Forwarded-For. We
+        // must NOT believe the header.
+        $tp = new TrustedProxies(['10.0.0.0/8']);
+        self::assertSame('203.0.113.42', $tp->clientIp([
+            'REMOTE_ADDR'           => '203.0.113.42',
+            'HTTP_X_FORWARDED_FOR'  => '127.0.0.1, 10.0.0.1',
+        ]));
+    }
+
+    public function testStripsPortFromXffHops(): void
+    {
+        $tp = new TrustedProxies(['10.0.0.0/8']);
+        self::assertSame('203.0.113.5', $tp->clientIp([
+            'REMOTE_ADDR'           => '10.0.0.1',
+            'HTTP_X_FORWARDED_FOR'  => '203.0.113.5:51234',
+        ]));
+    }
+
+    public function testIpv6Cidr(): void
+    {
+        $tp = new TrustedProxies(['2001:db8::/32']);
+        self::assertSame('2001:db8::1', $tp->clientIp([
+            'REMOTE_ADDR'           => '2001:db8::a',
+            'HTTP_X_FORWARDED_FOR'  => '2001:db8::1',
+        ]));
+    }
+
+    public function testBareIpTreatedAsHostMask(): void
+    {
+        $tp = new TrustedProxies(['127.0.0.1']);
+        self::assertTrue($tp->isTrusted('127.0.0.1'));
+        self::assertFalse($tp->isTrusted('127.0.0.2'));
+    }
+
+    public function testInvalidEntriesAreSkippedNotTrusted(): void
+    {
+        // Typos must not silently widen trust.
+        $tp = new TrustedProxies(['nonsense', '10.0.0.0/40', '10.0.0.0/8']);
+        self::assertTrue($tp->isTrusted('10.1.2.3'));
+        self::assertFalse($tp->isTrusted('203.0.113.5'));
+    }
+
+    public function testFromEnvParsesCommaList(): void
+    {
+        $prev = getenv('TRUSTED_PROXIES');
+        try {
+            putenv('TRUSTED_PROXIES=10.0.0.0/8 , 192.168.0.0/16');
+            $tp = TrustedProxies::fromEnv();
+            self::assertTrue($tp->isTrusted('10.5.5.5'));
+            self::assertTrue($tp->isTrusted('192.168.0.1'));
+            self::assertFalse($tp->isTrusted('172.16.0.1'));
+        } finally {
+            $prev === false ? putenv('TRUSTED_PROXIES') : putenv('TRUSTED_PROXIES=' . $prev);
+        }
+    }
+
+    public function testFromEnvWithBlankReturnsNoTrust(): void
+    {
+        $prev = getenv('TRUSTED_PROXIES');
+        try {
+            putenv('TRUSTED_PROXIES=   ');
+            $tp = TrustedProxies::fromEnv();
+            self::assertFalse($tp->isTrusted('10.5.5.5'));
+            self::assertFalse($tp->isTrusted('127.0.0.1'));
+        } finally {
+            $prev === false ? putenv('TRUSTED_PROXIES') : putenv('TRUSTED_PROXIES=' . $prev);
+        }
+    }
+
+    public function testIsHttpsDirectTls(): void
+    {
+        $tp = new TrustedProxies([]);
+        self::assertTrue($tp->isHttps(['HTTPS' => 'on']));
+        self::assertTrue($tp->isHttps(['REQUEST_SCHEME' => 'https']));
+        self::assertTrue($tp->isHttps(['SERVER_PORT' => '443']));
+        self::assertFalse($tp->isHttps(['HTTPS' => 'off']));
+        self::assertFalse($tp->isHttps([]));
+    }
+
+    public function testIsHttpsTrustsXfpFromTrustedProxyOnly(): void
+    {
+        $tp = new TrustedProxies(['10.0.0.0/8']);
+        self::assertTrue($tp->isHttps([
+            'REMOTE_ADDR'             => '10.0.0.1',
+            'HTTP_X_FORWARDED_PROTO'  => 'https',
+        ]));
+        // Same header but from an untrusted peer — must NOT be honoured.
+        self::assertFalse($tp->isHttps([
+            'REMOTE_ADDR'             => '203.0.113.5',
+            'HTTP_X_FORWARDED_PROTO'  => 'https',
+        ]));
+    }
+
+    public function testIsHttpsHandlesMultiHopXfpList(): void
+    {
+        // RFC-style "https, http" → leftmost is what the user actually saw.
+        $tp = new TrustedProxies(['10.0.0.0/8']);
+        self::assertTrue($tp->isHttps([
+            'REMOTE_ADDR'             => '10.0.0.1',
+            'HTTP_X_FORWARDED_PROTO'  => 'https, http',
+        ]));
+        self::assertFalse($tp->isHttps([
+            'REMOTE_ADDR'             => '10.0.0.1',
+            'HTTP_X_FORWARDED_PROTO'  => 'http, https',
+        ]));
+    }
+}