Переглянути джерело

Docs: mark R01-N05 / R01-N07 fixed, refresh SPEC §8 / §9 / §11 / §13

REVIEW_01 entries get the post-fix status with the real SHA, the suggested-
ordering checklist marks step 8 (proxy / HTTPS handling) done, and SPEC.md
gains a Shipped entry under §9 documenting the new trusted-proxy contract,
the new TRUSTED_PROXIES line in §8, the bumped 202 / 533 test count in §11,
and a new git-history line in §13.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
chiappa 2 днів тому
батько
коміт
97018093e0
2 змінених файлів з 92 додано та 4 видалено
  1. 47 1
      SPEC.md
  2. 45 3
      doc/REVIEW_01.md

+ 47 - 1
SPEC.md

@@ -280,6 +280,13 @@ DB_PATH=/var/www/data/app.sqlite
 SESSION_PATH=/var/www/data/sessions
 APP_ENV=production
 
+# Reverse-proxy trust (R01-N05 / R01-N07). Comma-separated CIDRs of the
+# proxies in front of the app. When `REMOTE_ADDR` matches one of these the
+# app walks `X-Forwarded-For` for the originating client IP (audit log,
+# login throttle bucket) and honours `X-Forwarded-Proto: https` for
+# Secure-cookie / HSTS / HTTP→HTTPS-redirect decisions. Blank ⇒ no trust.
+TRUSTED_PROXIES=
+
 # Optional explicit OIDC bootstrap (R01-N03). When the `users` table has no
 # admin and the signing user matches one of these (case-insensitive,
 # timing-safe), they are promoted on sign-in. Either or both may be set;
@@ -1232,6 +1239,44 @@ is gone — see `src/Auth/BootstrapAdmin.php`.
       §3.6. Tests: 184 / 502 (was 176 / 484). Seventh fix from
       `doc/REVIEW_01.md`.
 
+- [x] **R01-N05 + R01-N07 — Trusted-proxy aware HTTPS detection &
+      client IP** (`a2e77ea`). Behind a reverse proxy the app used
+      to record the proxy's address as the audit IP and to silently
+      drop the session cookie's `Secure` flag whenever `APP_BASE_URL`
+      was `http://` (and never to redirect mistyped HTTP traffic to
+      HTTPS). New `src/Http/TrustedProxies.php` owns the policy: a
+      comma-separated `TRUSTED_PROXIES=` env var lists the CIDRs of
+      hops that may speak `X-Forwarded-For` / `X-Forwarded-Proto` on
+      behalf of the user, and `clientIp()` walks the XFF chain
+      rightmost-to-leftmost to return the first hop that is not in any
+      configured CIDR. Bare addresses without `/n` are normalised to
+      host masks. With the env var blank — the default — both
+      forwarded headers are ignored, so a hostile direct client can't
+      lie its way into a different audit IP or HTTPS posture.
+      `Request::ip()` now goes through the helper (so the audit
+      pipeline and the R01-N06 throttle bucket key both fix
+      themselves), and a new `Request::isHttps()` exposes the same
+      decision to the rest of the app. `SessionGuard::start()` marks
+      the session cookie `Secure` when *either* `APP_BASE_URL` is
+      HTTPS or the live request is effectively HTTPS, so a TLS-
+      terminated proxy hop no longer downgrades the cookie.
+      `public/index.php` adds a one-shot HTTP→HTTPS redirect (308)
+      when `APP_BASE_URL` is HTTPS and the request is provably HTTP —
+      either there is no `TRUSTED_PROXIES` configured at all, or a
+      trusted proxy explicitly reported `X-Forwarded-Proto: http`. We
+      deliberately do NOT redirect when the proxy stays silent, to
+      avoid an infinite-loop with TLS-terminating proxies that forgot
+      to forward the scheme; `/healthz` is exempt outright. Operator
+      docs: new `.env.example` block, new admin-manual §3.5 "Reverse
+      proxy and HTTPS" (old §3.5 → §3.6, old §3.6 → §3.7) with an
+      Nginx snippet showing the required `proxy_set_header` lines.
+      New `tests/Http/TrustedProxiesTest.php` (14 cases) covers
+      direct-client / single-hop / multi-hop / IPv6 / typo / port-
+      stripping / `X-Forwarded-Proto` trust gating; new
+      `tests/Http/RequestTest.php` (4 cases) wires the env into
+      `Request::ip()` / `isHttps()`. Tests: 202 / 533 (was 184 / 502).
+      Eighth fix from `doc/REVIEW_01.md`.
+
 - [x] **New sprint form: drop weeks input + task list row hover**
       (`3728106`). The `/sprints/new` form no longer collects an
       `n_weeks` value — the week count is derived from `start_date` /
@@ -1301,7 +1346,7 @@ for f in $(git ls-files '*.php'); do php -l "$f" | tail -1 | sed "s|^|$f: |"; do
 Run the test suite:
 ```bash
 vendor/bin/phpunit
-# → OK (184 tests, 502 assertions)
+# → OK (202 tests, 533 assertions)
 ```
 
 The Phase 20 parser tests need `ext-dom`, `ext-zip`, `ext-xmlreader`,
@@ -1348,6 +1393,7 @@ before acting — nothing here is load-bearing once it grows stale.
 ## 13. Git history (as of this writing)
 
 ```
+a2e77ea Fix R01-N05 + R01-N07: trusted-proxy aware HTTPS + client IP
 f565c86 Fix R01-N03: explicit env-bootstrap for the first OIDC admin
 2b8f167 Docs: mark R01-N06 fixed, refresh SPEC §3 / §4 / §7 / §9 / §11 / §13
 e295432 Fix R01-N06: throttle local-admin login by (ip, email)

+ 45 - 3
doc/REVIEW_01.md

@@ -175,7 +175,28 @@ note. Do not delete entries — they're history.
 
 ### R01-N05 — No HTTPS enforcement; security-cookie flag conditional on env
 - **Severity**: HIGH.
-- **Status**: open.
+- **Status**: fixed-in-`a2e77ea` — paired with R01-N07. New
+  `src/Http/TrustedProxies.php` reads `TRUSTED_PROXIES` (comma-separated
+  CIDRs); `Request::isHttps()` honours `X-Forwarded-Proto: https` only when
+  `REMOTE_ADDR` is in a trusted CIDR (so an off-net attacker cannot lie
+  their way into Secure-cookie / HSTS behaviour). `SessionGuard::start()`
+  now marks the cookie `Secure` when *either* `APP_BASE_URL` is HTTPS *or*
+  the live request is effectively HTTPS — a TLS-terminating proxy that
+  forwards over plain HTTP no longer downgrades the cookie. Multi-hop
+  `X-Forwarded-Proto` lists ("https, http") read leftmost-first per the
+  RFC 7239 convention. `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, to avoid an infinite-loop with TLS-terminating proxies that
+  forgot to forward the header. `/healthz` is exempt outright. Operator
+  docs: new `.env.example` `TRUSTED_PROXIES=` block, new admin-manual
+  §3.5 "Reverse proxy and HTTPS" with the required Nginx
+  `proxy_set_header` lines (old §3.5/§3.6 shifted to §3.6/§3.7). New
+  `tests/Http/TrustedProxiesTest.php` (14 cases) and `RequestTest.php`
+  (4 cases) cover proxy trust gating, IPv6 CIDRs, port stripping, and
+  the multi-hop XFP list. Tests: 202 / 533 (was 184 / 502).
 - **Where**:
   - `public/index.php` lines 220, 245-247 — HSTS only when `APP_BASE_URL`
     starts with `https://`.
@@ -236,7 +257,27 @@ note. Do not delete entries — they're history.
 
 ### R01-N07 — `X-Forwarded-For` ignored; audit IP is wrong behind proxies
 - **Severity**: MEDIUM.
-- **Status**: open.
+- **Status**: fixed-in-`a2e77ea` — paired with R01-N05. The new
+  `TrustedProxies::clientIp()` walks `X-Forwarded-For` rightmost-to-
+  leftmost (with `REMOTE_ADDR` logically appended) and returns the first
+  hop that is not in any configured CIDR. When the immediate peer is not
+  itself a trusted proxy the header is ignored entirely — so a hostile
+  off-net client cannot forge an audit IP. `Request::ip()` now goes
+  through this helper, which means the audit pipeline (`AuditLogger::
+  audit()`) and the R01-N06 throttle bucket both pick up the real client
+  address with no further changes. IPv4-with-port (`1.2.3.4:51234`) and
+  bracketed-IPv6 (`[::1]:443`) hops are normalised by stripping the port.
+  Configured via the new `TRUSTED_PROXIES=` env var (comma-separated
+  CIDRs; bare IPs treated as `/32` or `/128`); blank by default so no
+  trust is granted to anyone unless the operator opts in. Documented in
+  admin-manual §3.5 with an Nginx snippet showing the required
+  `proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for` line.
+  Pinned by `tests/Http/TrustedProxiesTest.php` (14 cases): direct-
+  client return, single trusted hop, multi-hop walk, all-trusted edge
+  case, untrusted-remote-ignores-XFF, port stripping, IPv6 CIDR, host-
+  mask shorthand, bad-CIDR typo (must not widen trust), env parser
+  comma + whitespace, env blank → no trust, plus three `Request`-level
+  wiring checks. Tests: 202 / 533 (was 184 / 502).
 - **Where**: `src/Http/Request.php::ip()` lines 103-107.
 - **What**: `Request::ip()` returns `$_SERVER['REMOTE_ADDR']` only. When
   deployed behind any reverse proxy / load balancer, every audit row will
@@ -709,7 +750,8 @@ A reasonable cadence (do not treat as binding):
    (hash-only, no plaintext fallback per operator decision).
 6. ~~**R01-N06** (login throttling)~~ — fixed in `e295432`.
 7. ~~**R01-N03** (first-user bootstrap hardening)~~ — fixed in `f565c86`.
-8. **R01-N07** + **R01-N05** (proxy / HTTPS handling) — paired.
+8. ~~**R01-N07** + **R01-N05** (proxy / HTTPS handling) — paired~~ —
+   fixed in `a2e77ea`.
 9. **R01-N08** (idle session timeout + CSRF rotation).
 10. **R01-N10** (bind-not-concat sweep) — mechanical.
 11. **R01-N12** (date filter validation) — UX bug.