# REVIEW_01 — Security & "fishy patterns" audit

Date: 2026-05-07
Scope: full repo at `main` (commit `3728106`), against SPEC.md as of the same
SHA. Focus is on real correctness, security, robustness, and maintainability
issues — not cosmetics, naming, or stylistic nits.

This document is meant to be referenced on each fix loop alongside SPEC.md.
Findings are ordered by severity (highest first) and each carries:

- **ID**: stable handle (`R01-Nxx`) — quote when committing a fix.
- **Severity**: CRITICAL / HIGH / MEDIUM / LOW.
- **Status**: `open` / `accepted-by-design` / `fixed-in-<sha>` (update as we
  resolve).
- **Where**: file paths + line ranges (current, may shift after edits).
- **What**: the issue.
- **Why it matters**: concrete attack / failure mode.
- **Suggested fix**: shape of the change, not a full diff.

When a finding is closed, set Status to `fixed-in-<sha>` and append a short
note. Do not delete entries — they're history.

---

## How to use this with /loop

1. Pick the lowest-numbered `open` finding whose severity ≥ MEDIUM.
2. Re-read SPEC.md §7 (audit) and §9 (phase log) to confirm scope.
3. Apply the fix, add/adjust tests, update SPEC.md if behaviour changes.
4. Commit; flip Status to `fixed-in-<sha>` and add the SHA + a one-line note.
5. Commit the doc update separately, same maintenance contract as §14 of
   SPEC.md.

---

## CRITICAL

### R01-N01 — Local-admin password compared in plaintext from `.env`
- **Severity**: CRITICAL (within the local-admin code path; effectively HIGH
  for the app overall because the path is opt-in).
- **Status**: fixed-in-`857df15` — `src/Auth/LocalAdmin.php` now reads
  `LOCAL_ADMIN_PASSWORD_HASH` and verifies with `password_verify()`.
  Per the operator's explicit choice this is a clean break: NO plaintext
  `LOCAL_ADMIN_PASSWORD` fallback. `LocalAdmin::isEnabled()` returns false
  until the hash is provided, so `/auth/local` 404s instead of silently
  re-using stale plaintext config. Email comparison stays timing-safe
  (`hash_equals`, trimmed). Hash recipe documented in `README.md` Quick
  setup step 3 and `doc/admin-manual.md` §3.5 (host-PHP and
  `docker run php:8.3-cli` one-liners). New `tests/Auth/LocalAdminTest.php`
  (9 cases, +13 assertions) covers the happy path, wrong-password /
  wrong-email rejection, and an explicit regression guard that the legacy
  `LOCAL_ADMIN_PASSWORD` env var alone does NOT enable the fallback.
- **Where**:
  - `src/Auth/LocalAdmin.php` lines 51-66 (`verify()`, `password()`)
  - `.env.example` lines 25-33 (the comment acknowledging the trade-off)
- **What**: The local-admin password is stored in the environment and
  compared verbatim with `hash_equals(self::password(), $password)`. There is
  no hashing at rest. Anyone who can read `.env` (compromised backup, leaked
  Docker image layer, mis-configured docroot, container exec, debugging
  session) gets immediate admin.
- **Why it matters**: A common ops mistake (e.g., publishing the image with
  the env baked in, or leaving `.env` world-readable on the host) becomes a
  full-app compromise. The app already supports OIDC, so this fallback is the
  weakest link.
- **Suggested fix**:
  - Store a bcrypt/argon2id hash in `LOCAL_ADMIN_PASSWORD_HASH` instead.
    Verify with `password_verify()`. Offer a one-liner in the admin manual to
    generate the hash.
  - Keep timing-safe email comparison.
  - Document a clear deprecation path for plaintext (`LOCAL_ADMIN_PASSWORD`
    accepted with a logged warning for one release, then removed).

---

## HIGH

### R01-N02 — Information disclosure to anonymous users on `/`
- **Severity**: HIGH (signal to anyone scanning the public homepage).
- **Status**: fixed-in-`7fd849b` — twig guard tightened from
  `currentUser is null or currentUser.isAdmin` to
  `currentUser is not null and currentUser.isAdmin`; new
  `TwigViewTest::testHomeForAnonymousUserHidesRuntimePanel` asserts the
  panel and its leaked strings are absent for `currentUser=null`. Closes
  R01-N31 as a side effect (the in-page `/healthz` hint lived inside the
  same `<details>`).
- **Where**: `views/home.twig` lines 112-138 — the "Runtime" `<details>`
  panel.
- **What**: When `currentUser is null`, the panel still renders. It exposes:
  PHP version (`PHP_VERSION`), `APP_ENV`, the absolute path of the SQLite
  file, the schema version, OIDC configured y/n, local-admin enabled y/n.
- **Why it matters**: This is reconnaissance gold for an attacker — PHP
  version maps to known CVEs; the SQLite path tells them the deployment
  layout; "local admin enabled" advertises a weaker auth surface to focus
  on.
- **Suggested fix**: Render the panel only when `currentUser is not null and
  currentUser.isAdmin`. Keep `/healthz` as the public liveness probe (text
  "ok").

### R01-N03 — First-user-is-admin bootstrap is a land-grab
- **Severity**: HIGH on first deploy, none after.
- **Status**: fixed-in-`f565c86` — went with the explicit env-bootstrap
  branch. New `src/Auth/BootstrapAdmin.php` exposes
  `isConfigured()` + `matches(oid, email)`, reading the new
  `BOOTSTRAP_ADMIN_OID` / `BOOTSTRAP_ADMIN_EMAIL` env vars
  (case-insensitive, trimmed, `hash_equals`). `AuthController::callback`
  now gates promotion on `users.countAdmins() === 0 &&
  BootstrapAdmin::matches($oid, $email)` — with both env vars blank the
  OIDC path NEVER auto-promotes (operators must seed via the local-admin
  fallback or by flipping `is_admin` in `app.sqlite`). Switching from
  `count() === 0` to `countAdmins() === 0` also closes the SQLite
  `DEFERRED`-tx race the original finding called out: even if two
  flows concurrently see the same count, the BootstrapAdmin match
  eliminates the "wrong user wins" outcome — only the configured
  principal is ever promoted. The local-admin path is unchanged: it
  is itself an explicit env-bootstrap (`LOCAL_ADMIN_EMAIL` +
  `LOCAL_ADMIN_PASSWORD_HASH`) and `forceAdmin: true` continues to
  keep the configured local user admin. Both paths' BOOTSTRAP_ADMIN
  audit rows now carry `via=oidc` / `via=local` so `/audit` can
  distinguish them. Operator docs: new `doc/admin-manual.md` §3.5
  "Nominating the first administrator (OIDC)" (old §3.5 → §3.6),
  README's quick-setup paragraph rewritten, `.env.example` gains
  the new env block. New `tests/Auth/BootstrapAdminTest.php` (8
  cases) pins the matcher: unconfigured, oid-only, email-only,
  both-set, case-insensitive trim, blank-incoming-fields-don't-match,
  whitespace-only-env-treated-as-unset, either-channel-wins.
  Tests: 184 / 502 (was 176 / 484).
- **Where**:
  - `src/Controllers/AuthController.php` lines 86-116 (OIDC) and 217-254
    (local).
  - `src/Repositories/UserRepository.php` lines 50-78.
- **What**: The first user to complete a sign-in becomes admin. The check is
  `users.count() === 0` inside the same transaction as the upsert, but
  SQLite default `BEGIN` is `DEFERRED`, so two concurrent first-time logins
  could both observe count=0 and both get promoted. More importantly: if the
  app is reachable on the public internet before the intended admin signs
  in, an attacker with a valid Entra account in the tenant (or the local
  admin password if that path is enabled) can win the race.
- **Why it matters**: A wrong-admin state is recoverable only by another
  admin promoting/demoting via `/users`, which is itself admin-only. If the
  attacker is the first user, the legit operator has no path back without
  database access.
- **Suggested fix**: One of —
  - Require an explicit env-bootstrap step (`BOOTSTRAP_ADMIN_OID` or
    `BOOTSTRAP_ADMIN_EMAIL`) and only auto-promote when the signing user
    matches.
  - Or: use `BEGIN IMMEDIATE` for the upsert tx to serialise the check
    (cheap on SQLite WAL).
  - Document the first-deploy procedure in `doc/admin-manual.md` regardless.

### R01-N04 — `SESSION_SECRET` env var is documented but unused
- **Severity**: HIGH (misleading documentation; not actively exploitable).
- **Status**: fixed-in-`296883c` — removed from `.env.example`,
  `SPEC.md` §8, `README.md`, and `doc/admin-manual.md` §3.3 (subsequent
  subsections renumbered). Took the "remove it" path called out in the
  Suggested-fix list. Existing deployments' `.env` files keep their dead
  `SESSION_SECRET=` line; harmless, can be deleted at any time.
- **Where**:
  - `.env` / `.env.example` line 11.
  - `src/Auth/SessionGuard.php` — no reference.
  - SPEC.md §8 — declares it as required.
- **What**: `SESSION_SECRET` is documented as the salt for "session cookie
  name / CSRF tokens" but nothing in the code reads it. CSRF tokens are
  sourced from `random_bytes(32)` (good); the session id is whatever PHP
  generates (good). The env var is dead.
- **Why it matters**: An operator who reads SPEC + `.env.example` will
  assume rotating `SESSION_SECRET` invalidates active sessions / CSRF
  tokens. It does not. False sense of security; rotations don't actually
  protect against the threats they imagine they're protecting against.
- **Suggested fix**: Either —
  - Remove it from `.env.example`, SPEC §8, and the admin manual; OR
  - Wire it in: feed it into `session_id()` derivation, or use it to
    HMAC-derive CSRF tokens so deploy-time rotation is meaningful.
  - The first option is the smaller change and matches what the code
    actually does.

### R01-N05 — No HTTPS enforcement; security-cookie flag conditional on env
- **Severity**: HIGH.
- **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://`.
  - `src/Auth/SessionGuard.php` lines 41-58 — `secure` cookie flag mirrors
    the same flag.
- **What**: The app neither redirects HTTP→HTTPS nor refuses to serve auth
  flows over HTTP. If `APP_BASE_URL` is set to an `http://` URL, the session
  cookie's `secure` flag is dropped and HSTS is not emitted. A reverse-proxy
  misconfig (e.g., terminating TLS at the proxy but not setting
  `X-Forwarded-Proto`) silently degrades to HTTP behaviour.
- **Why it matters**: Cookie theft over plain HTTP, downgrade attacks, no
  HSTS preload safety net.
- **Suggested fix**:
  - Trust `X-Forwarded-Proto` when behind a known proxy (config-driven trust
    list) and treat the request as HTTPS for cookie / HSTS purposes.
  - Optionally redirect HTTP → HTTPS in `public/index.php` when `APP_BASE_URL`
    is HTTPS.
  - Document the proxy contract in `doc/admin-manual.md`.

### R01-N06 — No rate-limiting on local-admin login
- **Severity**: HIGH (when the local-admin path is enabled).
- **Status**: fixed-in-`e295432` — new migration `005_auth_throttle.sql`
  + `AuthThrottleRepository` keyed on `(ip_address, email)`. `loginLocal`
  now checks `lockoutFor()` BEFORE `LocalAdmin::verify()` (so a slow
  `password_verify()` can't be turned into an oracle while locked); on
  failure it calls `recordFailure()`, on success it `clear()`s the bucket.
  Policy in the pure-static `computeLockout(attempts, now)`: 1-4 → no
  lock, 5-9 → +5 min, 10-19 → +30 min, 20+ → +1 hour. Counter rolls
  over after 15 minutes of no failures, so honest mistypes don't get
  punished forever. Email key is lowercased + trimmed so case variation
  doesn't dodge the bucket; IPs separate, so two attacker IPs don't
  share a lock. Throttle hits emit a `LOGIN_FAILED` audit row with
  `reason=local_admin_throttled_until_<iso>` and the form renders an
  amber "Too many failed attempts" notice when redirected with
  `?throttled=1`. New `tests/Repositories/AuthThrottleRepositoryTest.php`
  (13 cases) pins the threshold matrix, the 4:59 / 5:00 lockout
  boundary, the idle-window reset, IP / email bucketing, and
  `purgeExpired()`'s selectivity. Tests: 176 / 484 (was 163 / 417).
- **Where**: `src/Controllers/AuthController.php::loginLocal` lines 194-276.
- **What**: There is no per-IP / per-account throttle on `/auth/local`.
  Attackers can brute-force the password as fast as the server can respond.
  The audit log records `LOGIN_FAILED` rows (forensics value), but nothing
  blocks the attempt.
- **Why it matters**: Combined with R01-N01 (plaintext compare), an attacker
  who guesses or leaks part of the password can finish the job by brute
  force. The OIDC path has Entra's own throttling, so this is specifically a
  hole in the local-admin path.
- **Suggested fix**:
  - In-process counter keyed by `(ip, email)` with a short backoff window
    after N failures (e.g. 5 in 60 s → 30 s pause; 20 in 10 min → block IP
    for 1 h).
  - Persistent counter in a tiny table (`auth_throttle`) so a process restart
    doesn't reset it.

---

## MEDIUM

### R01-N07 — `X-Forwarded-For` ignored; audit IP is wrong behind proxies
- **Severity**: MEDIUM.
- **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
  record the proxy's IP, not the user's.
- **Why it matters**: Audit log loses one of its main forensic values.
  Combined with the lack of rate limiting (R01-N06), a per-IP counter would
  also be wrong.
- **Suggested fix**: Add a config-driven "trusted proxies" list (CIDR);
  when REMOTE_ADDR matches a trusted proxy, walk the rightmost untrusted IP
  from `X-Forwarded-For`. Default empty (no trust). Document in admin
  manual.

### R01-N08 — Long-lived sessions, no inactivity timeout, CSRF token never rotated
- **Severity**: MEDIUM.
- **Status**: fixed-in-`bc745cd` — `SessionGuard` gained a 30-minute idle
  window (`IDLE_TIMEOUT_SECONDS = 1800`) enforced inside `start()`. New
  pure-static helpers `isIdleExpired($lastActive, $now)` and
  `expireIdleSession(array &$session, $now)` carry the policy: on every
  `start()`, if `user_id` is set and `$now - last_active >= 1800`, the
  authenticated keys (`user_id`, `login_at`, `last_active`,
  `csrf_token`) are dropped and `session_regenerate_id(true)` rotates
  the id — the next gate sees an anonymous session and redirects to
  `/auth/login`. Foreign session state (the OIDC library's
  state/nonce/PKCE) is preserved so an in-flight bounce to Entra is
  not killed by a stale idle clock. `login()` now stamps
  `last_active = time()` and `unset()`s `csrf_token`, so a token a
  pre-login attacker may have captured from the public homepage
  form cannot be replayed against the now-authenticated session
  (the next `csrfToken()` call mints a fresh
  `bin2hex(random_bytes(32))`). Boundary is `>=` so a session
  exactly 1800s idle is expired. New `tests/Auth/SessionGuardTest.php`
  (9 cases) pins the constant, the boundary semantics
  (0 / 1s / 1799s / 1800s / 2h), the anonymous-session no-op, the
  `last_active`-missing seed-on-first-hit, the auth-key drop on
  idle with foreign-key survival, and the non-int `user_id`
  defence. Tests: 211 / 562 (was 202 / 533).
- **Where**: `src/Auth/SessionGuard.php` lines 27-60, 109-116.
- **What**: `session.gc_maxlifetime = 28800` (8h) and the session cookie has
  `lifetime=0` (browser-session). Once authenticated, a user stays signed in
  until the browser closes or 8h passes since last GC tick. The CSRF token
  is generated once and reused for the lifetime of the session.
- **Why it matters**: A stolen session cookie is valid for hours; a stolen
  CSRF token paired with it lets the attacker submit any mutation that the
  user could.
- **Suggested fix**:
  - Track `$_SESSION['login_at']` (already set!) and an idle timestamp; on
    each request, if `now - last_active > 30 min`, force re-auth.
  - Rotate the CSRF token on `login()` (after `session_regenerate_id`) so
    pre-login tokens can't be replayed.

### R01-N09 — Session cookie `SameSite=Lax`, not `Strict`
- **Severity**: MEDIUM.
- **Status**: accepted-by-design — `Lax` is required for the OIDC return.
  The original "Suggested fix" understated the cost: when Entra redirects
  the user back to `/auth/callback` it is a *cross-site initiated* top-
  level navigation (initiator origin is `login.microsoftonline.com`), so a
  `SameSite=Strict` session cookie would NOT be sent. The OIDC library
  (`jumbojett/openid-connect-php`) reads the `state` and PKCE
  `code_verifier` it stashed in `$_SESSION` BEFORE the bounce — without
  the cookie those lookups fail and the callback errors out. We therefore
  keep `Lax` (cookie sent on top-level cross-site GETs only — never on
  cross-site sub-resource requests, image GETs, or any non-GET method).
  The real CSRF defence is `SessionGuard::verifyCsrf()` (per-session
  token, header or `_csrf` form field, `hash_equals()`); the OIDC
  callback adds the library's `state`/PKCE check on top. A "split-cookie"
  approach (Lax cookie just for the OIDC handshake, Strict for the main
  session) was considered and rejected as not worth the complexity:
  defence-in-depth gain is small and we'd have to teach two cookies to
  every place that rotates session ids.
- **Where**: `src/Auth/SessionGuard.php` lines 41-77.
- **What**: `Lax` allows the cookie on top-level cross-site GETs (a click
  from a phishing page).
- **Suggested fix**: NONE. Decision recorded — see Status.

### R01-N10 — SQL string concatenation of integer parameters in repos
- **Severity**: MEDIUM (non-exploitable today, brittle by design).
- **Status**: fixed-in-`c1dbfc1` — all three `MAX(sort_order)` lookups
  now use prepared statements with `?` placeholders instead of inlining
  the route-cast `int` into the SQL. `TaskRepository::create` (was
  line 60) and `TaskRepository::moveToSprint` (was line 100) prepare
  `'SELECT COALESCE(MAX(sort_order), 0) FROM tasks WHERE sprint_id = ?'`
  and bind `$sprintId` / `$destSprintId`. `SprintWorkerRepository::add`
  (was line 56) does the same against `sprint_workers`. The contract is
  now explicit at the SQL boundary — a future caller passing an
  unvalidated string is still type-safe because the value travels as a
  bound parameter, never as a string fragment of the query. No new
  tests: the fix is a mechanical refactor, the existing repo tests
  exercise these paths under `tests/Cascade` and `tests/Controllers`.
  No SPEC behaviour change.
- **Where**:
  - `src/Repositories/TaskRepository.php` line 60 (`MAX(sort_order) FROM
    tasks WHERE sprint_id = ' . $sprintId`) and line 100 (`moveToSprint`).
  - `src/Repositories/SprintWorkerRepository.php` line 56
    (`MAX(sort_order) FROM sprint_workers WHERE sprint_id = ' . $sprintId`).
  - `src/Controllers/SprintController.php` lines 1029-1033 — uses
    placeholders, OK.
- **What**: Integer route params are cast to int before reaching the repo
  (Router → controllers), so today these statements are SQL-injection-safe.
  But the contract is *implicit*: if any future caller passes an unvalidated
  string, the repo accepts it. Any code review tool (PHPStan / Psalm) that
  doesn't fully resolve types will flag this — and rightly so.
- **Why it matters**: Injection becomes one accidental refactor away.
- **Suggested fix**: Bind every value with `?` placeholders; never
  concatenate. Mechanical change, low risk.

### R01-N11 — `AuditRepository::distinctColumn` interpolates a column name
- **Severity**: MEDIUM (non-exploitable today; same brittleness class as
  N10).
- **Status**: fixed-in-`4ae1817` — `distinctColumn()` now opens with an
  `in_array($col, ['action', 'entity_type'], true)` whitelist; anything
  else throws `\InvalidArgumentException` before the interpolated SQL is
  prepared. The phpdoc was also tightened to the literal-string union so
  static analysers carry the contract. New
  `tests/Repositories/AuditRepositoryTest.php` (4 cases) covers the happy
  paths for both supported columns and two reflection-based regression
  guards (rejects an unsupported known column like `user_email`, rejects
  a SQL-injection attempt). Tests: 163 / 417 (was 159 / 413).
- **Where**: `src/Repositories/AuditRepository.php` lines 109-122.
- **What**: `"SELECT DISTINCT {$col} FROM audit_log ORDER BY {$col} ASC"`
  interpolates `$col`. Today the only callers are `distinctActions()` and
  `distinctEntityTypes()` with literal strings, but the method is private
  with no enforced whitelist.
- **Suggested fix**: Add a whitelist guard:
  `if (!in_array($col, ['action','entity_type'], true)) throw …` at the top
  of `distinctColumn`. Cheap, ends the foot-gun.

### R01-N12 — Audit log `from_date`/`to_date` filters not validated
- **Severity**: MEDIUM (UX bug, mild forensic impact).
- **Status**: fixed-in-`1b28469` — `AuditController::index` now routes
  both raw inputs through a pure static `validateDateFilters($from, $to)`
  that strict-parses `Y-m-d` (`createFromFormat` + round-trip equality,
  same pattern as `SprintController::isIsoDate`) and splits the output
  into `from` / `to` (empty string when the input fails to parse so the
  filter is dropped instead of poisoning the query) and an `errors` map
  keyed by field name. The repo is fed the validated copy; the view
  echoes the user's raw input back into the date fields so they can fix
  the typo, tints the offending input amber, and shows an inline
  "Use the format YYYY-MM-DD" notice plus a banner. No session-flash
  plumbing — the form is GET-driven and the response is the form, so
  errors travel with the rendered page. New
  `tests/Controllers/AuditControllerTest.php` (16 cases) pins the
  matrix: empty / valid / one-side-bad / both-bad, lenient rollovers
  caught by the round-trip check (`2025-02-29`, `2026-13-01`,
  `2026-02-30`, `2026-1-1`), whitespace rejected, leap-year preserved
  verbatim, an injection-shaped string rejected. Suite: 227 / 590 (was
  211 / 562).
- **Where**:
  - `src/Repositories/AuditRepository.php` lines 149-156.
  - `src/Controllers/AuditController.php` lines 39-49.
- **What**: The filter values are concatenated as `"{$date}T00:00:00Z"` and
  bound. Any garbage string just sorts lexically against the ISO-8601
  `occurred_at` column — silently producing empty result sets without an
  error. Not an injection (parameters are bound), but a bad UX and a way to
  hide events from a casual auditor (e.g., admin types `2024/01/01` instead
  of `2024-01-01` and "sees" no rows).
- **Suggested fix**: Validate `Y-m-d` server-side; on parse failure, drop
  the filter and flash an error. Same `DateTimeImmutable::createFromFormat`
  pattern already used in `SprintController::isIsoDate`.

### R01-N13 — Output buffer can leak partial responses on error
- **Severity**: MEDIUM.
- **Status**: fixed-in-`d7dbfb5` — new `App\Http\FatalErrorHandler`
  registers BOTH `set_exception_handler` (catches uncaught throwables
  escaping `Router::dispatch()`) and `register_shutdown_function`
  (filtered on a fatal mask: `E_ERROR | E_PARSE | E_CORE_ERROR |
  E_COMPILE_ERROR | E_USER_ERROR | E_RECOVERABLE_ERROR`, so suppressed
  warnings/notices don't trigger). On fire, the handler drains every
  open output buffer (so a half-rendered Twig template can't leak in
  front of the 500 page), then emits a minimal `500 — Server error`
  HTML body with the SAME security headers a normal response carries
  — nosniff, X-Frame-Options DENY, Referrer-Policy, the strict CSP,
  and HSTS when the resolved scheme is HTTPS. Production hides the
  throwable class + message; non-production echoes them (HTML-escaped)
  for debugging. To prevent the happy/error paths from drifting on
  CSP edits, `public/index.php` now sources its security headers from
  `FatalErrorHandler::securityHeaders($isHttps)` — single source of
  truth. The handler is registered TWICE: once with `isHttps=false`
  immediately after autoload (so a fatal during migrations or service
  wiring still produces a clean 500), and again with the resolved
  HTTPS flag right before `Router::dispatch()`. New
  `tests/Http/FatalErrorHandlerTest.php` (7 cases) pins the contract
  — production hides detail, dev surfaces it (HTML-escaped against
  XSS payloads), HSTS rides along on HTTPS only, headers are skipped
  cleanly when `headers_sent()` returns true (mid-flight fatal),
  the buffer drain runs exactly once and BEFORE any header / body
  write, and `securityHeaders($isHttps)` matches the emitted set.
  Tests: 235 / 624 (was 227 / 590).
- **Where**: `public/index.php` lines 38, 248-254.
- **What**: `ob_start()` is called at the top, but the flush is in the
  happy path. On a fatal error mid-request (uncaught throwable that escapes
  `dispatch()`, OOM, etc.), PHP's `output_buffering` may flush whatever was
  in the buffer to the response — without the security headers added at
  lines 240-243.
- **Why it matters**: Stack traces, partial HTML, or anything `var_dump`
  could land in the buffer when `display_errors=1`. In production with
  `display_errors=0` the risk is small, but the headers (CSP, X-Frame, etc.)
  still won't be applied to whatever does land.
- **Suggested fix**:
  - Register a `set_exception_handler` and a `register_shutdown_function`
    that discards the buffer (`ob_clean()`) and emits a minimal 500 page
    with the security headers.
  - Or drop `ob_start()` and rely on `Response::send()` to be the single
    output point; use `headers_sent()` checks before writing anything.

### R01-N14 — XLSX import session blob held in `$_SESSION`
- **Severity**: MEDIUM (memory / DoS vector, not an injection).
- **Status**: fixed-in-`d7dbfb5` — went with the cap-only path.
  `ImportController::MAX_SESSION_PAYLOAD_BYTES = 2 * 1024 * 1024` (2 MiB)
  bounds the JSON-encoded preview blob; `upload()` encodes the
  `ParsedSheet[]::toArray()` result once, and a payload past the cap
  is rejected with `?error=too_large_payload` (new entry in
  `views/sprints/import_upload.twig`'s message map). The encoded form
  is reused as the size estimate via the new pure-static
  `encodedPayloadBytes(array $sheetsArr): int` helper — same encoding
  flags as `AuditLogger::encodeJson` (`JSON_UNESCAPED_UNICODE |
  JSON_UNESCAPED_SLASHES`). The session entry also gains a
  `payload_bytes` field so the abandoned-token audit row has a real
  number rather than `unknown`. The "stream-parse + temp file"
  alternative was considered and rejected — the cap is enough for
  the threat model (admin-only upload, 5 MB raw cap already in place,
  `www-data`-only session files), and the temp-file approach adds
  a disk-hygiene problem the cap dodges entirely.
- **R01-N14b** — IMPORT_PREVIEW_ABANDONED audit rows. Both pruning
  paths now record the abandonment: `pruneSessionImports()` (called
  on every fresh upload, drops aged-out tokens) and
  `loadSessionEntry()` (called by `preview()` / `commit()`, drops a
  token whose TTL elapsed mid-flow). The row is `action=
  IMPORT_PREVIEW_ABANDONED, entity_type=import_token, entity_id=NULL`
  with `after_json` carrying `{file_name, sheet_count, payload_bytes,
  age_seconds, created_at}` (UTC ISO-8601). The pure-static
  `abandonedAuditPayload(array $entry, int $now): array` builder is
  guarded against malformed entries (`created_at` missing → age
  clamps to 0 instead of leaking the unix-epoch offset; clock skew
  giving a "future" entry → age also clamps to 0). The
  `ImportController` constructor gained an `AuditLogger` dependency;
  wiring updated in `public/index.php`.
- **Tests**: `tests/Controllers/ImportControllerTest.php` grew from
  4 to 12 cases. New cases pin: `encodedPayloadBytes([])` is 2 (just
  `[]`); the helper agrees with a hand-rolled `json_encode + strlen`
  on a real-shaped payload; the cap constant is exactly 2 MiB
  (drift fence so a future bump must update REVIEW_01.md +
  the upload form's error message); a payload of cap+1 length is
  measurable as larger than the cap; the abandoned-audit payload
  has the right keys + types + a strict ISO timestamp; missing
  fields produce safe defaults; clock skew → age 0. Suite:
  242 / 673 (was 227 / 590).
- **Where**: `src/Controllers/ImportController.php` lines 102-115 (write),
  244-261 (read). Session storage path is on disk under
  `/var/www/data/sessions/`.
- **What**: The full parsed workbook (every cell value, status, weeks,
  workers, tasks for every sheet) is JSON-stashed in PHP's session file.
  Limit is 5 MB on the upload but parsed expansion is unbounded — a hostile
  XLSX with many tabs can cost much more in memory + on-disk session size.
- **Why it matters**: Disk pressure, slow session file IO, possible OOM
  during the second hop (`fromArray()` reconstruction). And on disk these
  files are readable by `www-data` only — but they linger 30 min by design.
- **Suggested fix**:
  - Cap the per-token payload size; refuse parses that exceed e.g. 2 MB
    serialised JSON.
  - Or stream-parse + temp-file (signed file path) instead of session blob.
  - Minimum: also log a `IMPORT_PREVIEW_ABANDONED` audit row when a token
    expires unused.

### R01-N15 — `target="_blank"` task URL anchor uses `rel="noopener"` only
- **Severity**: MEDIUM (Privacy; small).
- **Status**: fixed-in-`d16bff4` — the user-controlled task link in
  `views/sprints/_task_list.twig` now emits `rel="noopener noreferrer"`,
  so the `Referer` header no longer leaks the originating
  `/sprints/{id}` URL when an admin clicks an attacker-set `t.url`.
  Scope intentionally narrow: the `/sprints/{id}/present` anchor in
  `views/sprints/show.twig` was left as `rel="noopener"` because it is
  same-origin and the Referer leak the finding describes does not
  apply.
- **Where**: `views/sprints/_task_list.twig` line 213.
- **What**: `rel="noopener"` blocks `window.opener` access, but the
  `Referer` header still leaks the origin URL (`/sprints/{id}`) to the
  external link target.
- **Why it matters**: Sprint URLs are guessable IDs; leaking them via Referer
  to attacker-controlled URLs (an attacker-set `t.url`) reveals that
  internal sprint exists.
- **Suggested fix**: `rel="noopener noreferrer"`.

### R01-N16 — PhpSpreadsheet 3.4 — pin and watch CVE feed
- **Severity**: MEDIUM (advisory; depends on actual installed version).
- **Status**: fixed-in-`ef9b9b8` — went with the doc + tooling path, no
  CI hook (this repo has no CI today; adding GitHub Actions would
  introduce a new external dependency the operator hasn't asked for).
  New `bin/audit.sh` wraps `composer audit --locked --no-interaction`
  inside `sprint_planer_web-app:latest`, so the audit reflects the
  exact dependency tree the live container ships and not whatever the
  host PHP happens to have. Honours `SPRINT_PLANER_IMAGE` for
  non-default tags. Refuses cleanly when docker is missing or the
  image hasn't been built. Today's baseline: "No security
  vulnerability advisories found." `doc/admin-manual.md` §5.5 grew a
  "Composer dependency cadence" block — recommended rebuild rhythm
  (after every git pull, monthly minimum), the auditor invocation,
  and a copy-pasteable weekly-cron snippet that pipes a non-zero exit
  to `mail`. PhpSpreadsheet itself stays at the `^3.4` caret range —
  minor upgrades will land on each `docker compose build --no-cache`
  with no manifest change required. No new tests (script is operator
  tooling, not application code).

### R01-N17 — Login `state`/PKCE storage in session — verify lifetime
- **Severity**: MEDIUM (usability hazard; not a vulnerability).
- **Status**: accepted-by-design — confirmed by reading
  `vendor/jumbojett/openid-connect-php/src/OpenIDConnectClient.php`. The
  library uses fixed session keys `openid_connect_state`,
  `openid_connect_nonce`, `openid_connect_code_verifier` (lines
  1789-1866); the methods `setSessionKey()` / `getSessionKey()` /
  `unsetSessionKey()` are `protected` so a subclass *could* namespace
  them, but the namespace would then have to round-trip via a separate
  short-lived cookie (the OIDC `state` parameter is generated and read
  by the library itself, not by us). The realistic failure mode is
  small: the second tab clobbers the first tab's `state`, the first
  tab's callback hits the `if ($_REQUEST['state'] !== $this->getState())
  { throw 'Unable to determine state' }` guard at line 322, the user
  is bounced to `/?auth_error=1`, and they retry. That guard is the
  *correct* OIDC state-mismatch behaviour — it's what stops a CSRF on
  the OIDC callback. The "fix" (subclass + transient flow-id cookie,
  ~80 LoC + tests) trades real complexity for a one-line UX nit. Not
  worth it for a single-admin tool. `doc/admin-manual.md` §5.6
  "Tabbed sign-in note" documents the rule for operators: complete
  one sign-in at a time. No code change.
- **Where**: `src/Auth/OidcClient.php` + `src/Controllers/AuthController.php`
  callback.
- **What**: `jumbojett/openid-connect-php` stores its `state` and PKCE
  `code_verifier` in `$_SESSION`. `SessionGuard::login()` calls
  `session_regenerate_id(true)` *after* the callback completes — but the
  state/verifier are written before the redirect to Entra and read after the
  bounce back. If two browser tabs initiate auth flows simultaneously, the
  second overwrites the first's state. Not a direct vuln but a usability
  hazard.
- **Suggested fix**: NONE. Decision recorded — see Status.

### R01-N18 — Email-claim trust path during OIDC upsert
- **Severity**: MEDIUM.
- **Status**: fixed-in-`a0b717e` — new `App\Auth\OidcClaims::resolveEmail
  ($claims, $oid)` is the single decision point for the audit-trail
  email. The `preferred_username` fallback is removed entirely (the
  actual hazard the finding flagged: user-controlled in some Entra
  tenants). `claims.email` is honoured when present, EXCEPT when the
  issuer explicitly marks it `email_verified === false` — that case
  falls back to the documented immutable label `entra:<oid>` so the
  forensic trail still has a stable, non-spoofable actor. A *missing*
  `email_verified` flag is treated as "no negative signal" because
  Entra v2.0 work/school tokens do not emit it (the email there comes
  from the directory and is server-controlled); requiring strict
  `=== true` would flip every existing audit row to `entra:<oid>`
  which is an unnecessary regression for the threat model. The
  `oid`-or-`sub` check moved up so a missing identifier rejects the
  callback BEFORE the email resolver runs (the resolver assumes a
  non-empty oid for its fallback). Identity is still keyed by `oid`
  / `sub` and is unaffected. New `tests/Auth/OidcClaimsTest.php` (10
  cases) pins the matrix: verified-true / verified-absent /
  verified-false / preferred-username-fallback-rejected / blank-email
  / whitespace-trim / no-claims-at-all / non-bool-truthy
  email_verified / non-scalar email defence. Suite: 252 / 683 (was
  242 / 673).
- **Where**: `src/Controllers/AuthController.php` lines 75-77.
- **What**: `email = $claims->email ?? $claims->preferred_username`. In Entra,
  `preferred_username` is *not* guaranteed to be a verified email; in some
  tenant configurations it can be controlled by the user. We don't check
  `email_verified` either.
- **Why it matters**: If the audit log uses email as the human-readable
  actor identifier (it does — it's joined back via `user_email` in
  `audit_log`), a user can spoof their actor email. The `entra_oid` (or
  `sub`) is the immutable identifier and is correctly used as the lookup
  key — so the *user identity* is safe, but the *audit trail's display*
  isn't.
- **Suggested fix**:
  - Prefer `claims.email` only when `claims.email_verified === true`.
  - Otherwise fall back to a documented, immutable label like
    `entra:<oid>`.

### R01-N19 — Strict CSP has no `report-uri` / `report-to`
- **Severity**: MEDIUM (operational visibility).
- **Status**: fixed-in-`f59f368` — went with `report-uri` only (CSP
  Level 1/2 directive, still honoured by every current browser; CSP3
  `report-to` would have needed a `Reporting-Endpoints` response
  header on every page and a second JSON parser for the
  `application/reports+json` shape, neither cheap for the marginal
  gain). `App\Http\FatalErrorHandler::CSP` gained
  `report-uri /csp-report`. New public route `POST /csp-report`
  routes to `App\Controllers\CspReportController::report`, which
  body-caps at `MAX_BODY_BYTES = 16 * 1024` (real CSP reports are
  well under 1 KiB; the cap is generous enough to never reject
  legit reports, tight enough that a hostile client can't pump
  megabytes of garbage into `audit_log`). The legacy
  `{"csp-report": {...}}` envelope is unwrapped via the pure-static
  `extractReport(string): ?array` helper; bare `{...}` shapes are
  also accepted; top-level lists / scalars / non-JSON return null.
  The endpoint is anonymous: no session, no CSRF — a browser fires
  reports without session context most of the time, and a forged
  report only costs one audit row (bounded by the cap). Always
  responds 204 (no leaks about whether the report was accepted or
  ignored). Audit row shape: `action=CSP_VIOLATION,
  entity_type=csp_violation, entity_id=NULL,
  before_json=NULL, after_json=<inner report>, user_id=NULL,
  user_email=NULL, ip_address=<client>, user_agent=<browser>`.
  `AuditRepository::distinctEntityTypes()` already powers the
  `/audit` view's filter dropdown, so `csp_violation` shows up
  there automatically. `FatalErrorHandlerTest` CSP-line assertion
  grew a `report-uri` check (drift fence). New
  `tests/Controllers/CspReportControllerTest.php` (10 cases) pins
  the 204-always rule, the body cap, the JSON-object-only contract,
  the envelope unwrap, and the audit-row shape end-to-end against
  an in-memory DB. Tests: 266 / 721 (was 252 / 683).
- **Where**: `public/index.php` lines 227-237.
- **What**: CSP is strict but silent — operators won't notice if a future
  view inadvertently introduces an inline handler or new external host
  until users complain.
- **Suggested fix**: Add a `report-uri /csp-report` (or `report-to`)
  directive; route it to a thin endpoint that drops the JSON in the audit
  log under `entity_type='csp_violation'`. Cheap; high signal.

### R01-N20 — `Response::redirect()` accepts arbitrary location strings
- **Severity**: MEDIUM (no current open redirect; pattern is fragile).
- **Status**: fixed-in-`f1aa924` — went with both bullet points
  from the original Suggested-fix list. `Response::redirect($location)`
  now refuses anything that is not a single `/` followed by something
  other than `/`, and also refuses CR/LF/NUL (header injection).
  Protocol-relative URLs (`//evil.example.com/x`) — the dangerous
  shape that *looks* like a path — are caught by the second test
  (without it `str_starts_with($location, '/')` would happily wave
  them through). The HTTP→HTTPS canonical redirect in
  `public/index.php` (the one place that needs to leave the origin)
  moves to the new `Response::external($url, $status)` helper, which
  restricts the scheme to `http`/`https` and requires a non-empty
  host so a stray `javascript:` / `data:` payload cannot wind up in
  the `Location:` header. Both helpers throw
  `\InvalidArgumentException` on misuse — loud in dev/test, harmless
  under the existing `FatalErrorHandler` safety net in production.
  Pure-static `isPathOnly()` and `isExternalUrl()` back the contract
  for direct unit tests. All 56 existing callers were audited:
  every one passes a `/`-prefixed literal or a `'/'`-prefixed
  template, and dynamic suffixes are integer IDs / tokens / `urlencode()`
  output, so none regress under the new guard. New
  `tests/Http/ResponseTest.php` (15 cases, 36 assertions). Tests:
  302 / 791 (was 266 / 721).
- **Where**: `src/Http/Response.php` lines 54-57.
- **What**: All current callers pass path-only or literal strings, so no
  open-redirect today. But the helper has no shape check: a future caller
  could pass `?next=` from user input and redirect off-origin.
- **Suggested fix**: Either —
  - Document the contract ("path-only; never user input"), and add an
    `assert()` or runtime check that the value starts with `/` or matches
    `APP_BASE_URL`.
  - Provide a separate `Response::external($url)` for the rare cases that
    actually need to leave the origin.

### R01-N21 — Trusted Twig auto-escape is the only XSS guard
- **Severity**: MEDIUM (audit recommendation).
- **Status**: fixed-in-`00bcf73` — went with the audit's two-test
  recommendation. New `tests/Http/TwigAutoescapeTest.php` (3 cases,
  23 assertions). *Behaviour pin:* renders a known XSS payload
  through a synthetic Twig template using the same `View` env the
  controllers use and asserts `<script>` does NOT survive the
  render (`&lt;script&gt;` does), so a future flip of `View`'s
  `autoescape` setting from `'html'` to anything else fails loudly.
  A second case pins attribute-context double-quote escaping
  (`title="…"` → `&quot;`). *Static guard:* walks every `.twig`
  file under `views/` and fails if any line carries
  `|\s*(raw|safe)` or `{%\s*autoescape`. Verified end-to-end by
  temporarily appending `{{ "x"|raw }}` to `home.twig` — the test
  reported the exact `path:line` of the offence with a remediation
  hint. No production code changed; the guards live entirely in
  the test suite. Today's scan: 0 offences across all `.twig`
  files. Tests: 305 / 814 (was 302 / 791). The audit's optional
  CI grep step is subsumed by this PHPUnit case (we already run
  the suite on every change), so no separate grep stanza needed.
- **Where**: `src/Http/View.php` line 30 (`autoescape => 'html'`); various
  views.
- **What**: All user-supplied strings (sprint name, task title, task
  description, task url, worker names, audit JSON) are rendered through
  Twig with `autoescape: html`. Spot-checks confirm no `|raw`, `|safe`, or
  unsafe `attr` filters are used (one place uses `|json_encode|e('html_attr')`
  for the `data-sprint-choices` payload — correct usage).
- **Why it matters**: One careless future template that uses `|raw` on a
  user field opens stored XSS in a place where it's particularly painful
  (the audit viewer renders attacker-controlled content).
- **Suggested fix**:
  - Pin `autoescape` config in a CI test that loads `View` and asserts the
    flag.
  - Add a static-analysis grep step in CI: `grep -rn '|raw' views/` must
    return nothing (or only allow-listed lines).

### R01-N22 — Migrator runs at request time, swallows stale cache risk
- **Severity**: MEDIUM (operational hazard).
- **Status**: fixed-in-`43b2fc9` — went with both bullet points from
  the audit's Suggested-fix list. Migrations are now applied by the
  new `bin/migrate.php` CLI, called from `bin/docker-entrypoint.sh`
  before `exec apache2-foreground`, so a failed migration aborts
  container start (`docker logs` carries the stderr) instead of
  leaking into the request path. The web request path only calls
  the new pure-read `Migrator::pendingFiles()` (which diffs files on
  disk against `schema_version` without applying SQL) and emits
  `503 Service Unavailable` + `Retry-After: 30` + a structured
  `error_log` line when anything is pending. Bare-metal deploys that
  skip Docker run the CLI manually as part of the release procedure;
  `doc/admin-manual.md` §5.5 documents both. The 503 path is
  intentionally loud — the alternative (auto-migrating in a request
  that may then crash mid-DDL) is exactly the partial-schema failure
  mode the finding flagged. New `tests/Db/MigratorTest.php` (6
  cases) pins `pendingFiles()` against the real `migrations/`
  folder, virgin-DB / post-migrate / drift-after-deploy paths, the
  ignore-non-`NNN_*.sql` rule, and the `schema_version`-creation
  side-effect. Tests: 311 / 823 (was 305 / 814).
- **Where**:
  - `public/index.php` lines 75-104 (check + 503).
  - `src/Db/Migrator.php` — `pendingFiles()` added; `migrate()`
    unchanged.
  - `bin/migrate.php` (new), `bin/docker-entrypoint.sh` (new).
  - `Dockerfile` — `ENTRYPOINT` + `CMD`.
- **What**: Migrations ran on every request as `pdo->exec($sql)` (multi-
  statement). On a fresh deploy, the *first* request after the new code
  ships did the migration; if that request crashed mid-migration (timeout,
  OOM), partial state could persist. SQLite + the migrator's `BEGIN..COMMIT`
  protects most cases, but `ALTER TABLE … ADD COLUMN` is a DDL that's
  conditionally inside a tx — see SQLite caveats.
- **Why it matters**: A partially-applied migration leaves the schema in a
  state where the new code disagrees with the DB and every subsequent
  request 500s.
- **Suggested fix**:
  - Move migration to a deploy-time step (entrypoint script that runs
    `php bin/migrate.php` before starting Apache).
  - Keep the request-time check as a safety net, but log loud + refuse to
    serve when versions disagree.

### R01-N23 — `users` table not deleted; emails not redacted on demote
- **Severity**: MEDIUM (privacy / GDPR-ish).
- **Status**: fixed-in-`22a3840` — went with the audit's
  Suggested-fix plan exactly: nullable `users.tombstoned_at` column
  (migration 006) plus a soft erasure flow surfaced as an explicit
  admin action. `User::publicEmail()` / `publicDisplayName()` return
  the documented `(former user)` label when tombstoned; live views
  (Users page, anywhere a `User` object surfaces) consult those
  helpers, while `email` / `displayName` on the snapshot — and
  therefore everything that flows into the `audit_log.user_email`
  column — keep the historical value verbatim. The trail is the
  authoritative record; erasure regulations permit retention for
  legal / security purposes (documented in admin-manual §5.1.1
  with that stance spelled out for the operator). New
  `tombstone()` controller action routed at
  `POST /users/{id}/tombstone` with a hidden `action`
  (`tombstone` / `restore`) field; pure-static `tombstoneGuardrail()`
  rejects self-tombstone, blocks tombstoning the last remaining
  admin (because tombstone forces `is_admin=0`), and short-circuits
  no-ops. A successful sign-in (OIDC or local-admin / forceAdmin)
  clears `tombstoned_at` — the marker is a display redaction, not an
  access control; if the org actually wants to revoke access the
  OIDC tenant is the right place. Audit action labels: TOMBSTONE
  and RESTORE on `entity_type=user`, picked up automatically by the
  `/audit` filter dropdown via `distinctActions()`. Tests:
  325 / 853 (was 311 / 823). UserRepositoryTest grew by five cases
  (auto-untombstone on both sign-in paths, force-demote, restore-
  doesn't-promote, public-label assertion). UserControllerTest grew
  by eight (tombstoneGuardrail matrix).
- **Where**:
  - `migrations/006_users_tombstoned.sql` (new column).
  - `src/Domain/User.php` (snapshot field + display helpers).
  - `src/Repositories/UserRepository.php` (`setTombstoned()` + auto-
    clear in `upsertFromOidc`).
  - `src/Controllers/UserController.php` lines 16-25 (header
    rewritten) + new `tombstone()` action and
    `tombstoneGuardrail()`.
  - `views/users/index.twig`, `views/audit/index.twig` (display).
  - `doc/admin-manual.md` §5.1.1, `SPEC.md` §7.
- **What**: A user demoted from admin (or who simply leaves the org) kept
  their `email` and `display_name` in the `users` table forever; the audit
  log's `user_email` column also retained it. There was no redact / tombstone
  flow.
- **Why it matters**: Privacy regulations may require name/email erasure
  on request. The current model can't honour that without breaking audit.
- **Suggested fix**:
  - Add a `users.tombstoned_at` column; when set, hide name/email from UI
    and replace with `(former user)`. Audit rows keep the old email
    verbatim — the audit log is the authoritative trail and erasure laws
    typically permit retention for legal/security purposes.
  - Document the policy in `doc/admin-manual.md`.

### R01-N24 — No request-body size limit on JSON endpoints
- **Severity**: MEDIUM.
- **Status**: fixed-in-`821122d` — went with both bullet points
  from the audit's Suggested-fix list. `Request::MAX_BODY_BYTES =
  1 MiB`; `Request::fromGlobals()` now checks `Content-Length`
  before reading `php://input`, so an oversized body is never
  loaded into PHP memory in the first place (with a post-read
  length check as defence-in-depth for clients that under-report
  or omit the header). The decision rides through to the new
  `bodyTooLarge` constructor flag, which `public/index.php`
  consults BEFORE the HTTPS redirect — wasting a TLS round trip
  on something we'd reject anyway is a worse experience than a
  fast 413. The 413 ships as the standard
  `{ok: false, error: { code: 'request_too_large', … }}`
  envelope; HTML form clients (which would already be in
  pathological territory at >1 MiB) see the JSON body in the
  browser, an acceptable trade-off vs. plumbing a separate
  HTML branch. Per-batch cap: `MAX_BATCH_ITEMS = 5000` on both
  `SprintController` and `TaskController`. The four list-bodied
  endpoints — `updateWeekCells`, `updateAssignments`,
  `updateAssignmentsStatus`, and tasks `reorder` — reject past
  the cap with `too_many_items` 413. Real workloads sit
  comfortably under: a sprint has ≤26 weeks (schema cap) and a
  realistic worker count, so the wall is well above any genuine
  UI flow but dodges the "1 MiB body packed with tens of
  thousands of tiny cells" path that would still keep one
  transaction long-running. New `RequestTest` cases (4) pin the
  constant, the constructor flag default + wiring, and the
  `json()`-returns-null contract when `bodyTooLarge` is set.
  `SprintControllerTest` + new `TaskControllerTest` pin
  `MAX_BATCH_ITEMS = 5000` and the cross-controller drift fence
  (the two caps must stay in lockstep). Tests: 331 / 860 (was
  325 / 853).
- **Where**:
  - `src/Http/Request.php` — `MAX_BODY_BYTES`, `bodyTooLarge`
    flag, `fromGlobals()` short-circuit.
  - `public/index.php` — pre-dispatch 413.
  - `src/Controllers/SprintController.php`,
    `src/Controllers/TaskController.php` — `MAX_BATCH_ITEMS` +
    per-endpoint guards.
- **What**: `file_get_contents('php://input')` read whatever PHP's
  `post_max_size` allowed (default 8 MB). `json_decode` with depth=64 is
  reasonable, but a 7 MB JSON list with 1M entries was parsed before
  any controller-level cap.
- **Why it matters**: Any admin can DoS the server by repeatedly POST'ing
  large bodies. Insider-only, but easy to do accidentally with a misbehaving
  client.
- **Suggested fix**:
  - Read `Content-Length` first; refuse `>1 MB` with 413.
  - Cap list lengths in batch endpoints (e.g. 5000 cells per
    `/sprints/{id}/week-cells`).

---

## LOW

### R01-N25 — No `X-Permitted-Cross-Domain-Policies` header
- **Severity**: LOW.
- **Status**: fixed-in-`f6ce13f` —
  `FatalErrorHandler::securityHeaders()` (the single source of truth
  shared by the happy path and the 500 fallback per R01-N13) now emits
  `X-Permitted-Cross-Domain-Policies: none` unconditionally — on HTTP
  and HTTPS alike, unlike HSTS. The drift fence in
  `FatalErrorHandlerTest` grew two assertions: the production-500 emit
  carries the header, and the helper map carries it on both
  `securityHeaders(true)` and `securityHeaders(false)`. The optional
  `Permissions-Policy` directive from the Suggested-fix list was left
  out — the app does not use any of the gated browser features
  (geolocation, camera, mic, USB, …) so the directive would be a no-op
  with maintenance cost; revisit if a future feature legitimately
  needs one of those APIs and we want an explicit allow-list. Tests:
  333 / 868 (was 331 / 860).
- **Where**: `public/index.php` lines 240-243.
- **What**: Adobe Flash / Acrobat is dead, but defence-in-depth headers are
  cheap.
- **Suggested fix**: Add `X-Permitted-Cross-Domain-Policies: none`.
  Optional: `Permissions-Policy` to disable geolocation / camera / etc.

### R01-N26 — `home.twig` "deleted" flash relies on query string truthiness
- **Severity**: LOW.
- **Status**: fixed-in-`1f11117` — went with the audit's
  Suggested-fix exactly: `SprintController::delete` stashes the
  deleted sprint's name in `$_SESSION['flash_deleted_sprint_name']`
  (matching the existing `flash_*` convention from
  `ImportController`) and redirects to a bare `/`. The home handler
  in `public/index.php` reads the key — `SessionGuard::currentUser`
  has already started the session by that point — and `unset()`s it
  on render, so a manual page refresh cannot re-show the chip and a
  hand-crafted URL cannot trigger it at all. The view's `{% if
  deletedSprintName|default('') != '' %}` guard is unchanged; only
  the data source moved. New `TwigViewTest` cases (2): chip renders
  when the flash is set (and the name is HTML-escaped, drift fence
  for R01-N21's autoescape pin), chip stays absent when the flash
  is empty. Tests: 333 / 868 (was 331 / 860).
- **Where**: `views/home.twig` lines 11-15; `public/index.php` line 150;
  `src/Controllers/SprintController.php::delete` line 1114.
- **What**: After delete, the user is redirected to `/?deleted=<urlencoded
  name>`. Anyone can manually craft a URL with `?deleted=foo` and see the
  green "Sprint foo was deleted" chip — no actual deletion happened. Twig
  auto-escape protects against XSS, so this is purely a UX/spoofing nit.
- **Suggested fix**: Use a one-shot session flash (`$_SESSION['flash_*']`,
  the import controller already does this), unset on render.

### R01-N27 — Session lifetime / GC is filesystem-only on container restart
- **Severity**: LOW.
- **Status**: fixed-in-`32d03fc` — went with a backgrounded loop in
  `bin/docker-entrypoint.sh` rather than installing cron (cron would
  be a new external dependency for a 4-line shell loop). After
  migrations succeed, the entrypoint spawns a child process that
  sleeps `${SESSION_GC_INTERVAL_SECONDS}` (default 3600 = 1h), then
  runs `find ${SESSION_PATH} -mindepth 1 -type f -mmin
  +${SESSION_GC_MAX_AGE_MINUTES} -delete` (default 480 minutes = 8h,
  matching `gc_maxlifetime` on `SessionGuard::start()`). The loop is
  a child of the entrypoint's PID 1, so `docker stop` propagates
  SIGTERM and reaps it together with Apache; no orphaned process on
  container restart. `find` is a coreutils binary already in the
  `php:8.3-apache` base image, so no new package install. Both knobs
  are env-var overridable
  (`SESSION_GC_MAX_AGE_MINUTES`,`SESSION_GC_INTERVAL_SECONDS`) for
  operators who want a tighter sweep on a public deployment.
  `doc/admin-manual.md` §5.7 documents the loop, the env vars, and a
  copy-pasteable system-cron snippet for bare-metal deployments that
  don't run the container's entrypoint. No new tests — the change is
  shell-script + docs, exercised by `docker compose up`. Existing
  PHPUnit suite still 333 / 868.
- **Where**: `Dockerfile` line 57; `src/Auth/SessionGuard.php` lines 33-39.
- **What**: Sessions are stored on a mounted volume. PHP's GC depends on
  request traffic to fire — a low-traffic deployment may keep stale session
  files for days past the 8h `gc_maxlifetime`. Not a session-validity issue
  (the session contains `login_at` + cookie expiry, see R01-N08), but a
  storage hygiene one.
- **Suggested fix**: Cron `find /var/www/data/sessions -mmin +480 -delete`
  in the container, or accept it as low-impact.

### R01-N28 — `SettingsController::update` builds `$changed[]` but never uses it
- **Severity**: LOW (dead code).
- **Status**: fixed-in-`216c15d` — went with the "delete the variable"
  branch of the Suggested-fix list. The `$changed = []` declaration and
  the trailing `$changed[] = $key` accumulator were both removed; per-key
  audit rows already capture what changed via
  `AuditLogger::recordForRequest()` (and `AppSettingsRepository::set()`'s
  no-op short-circuit ensures no audit row fires when before/after match),
  so the redirect doesn't need a count. The `?updated=N`-style alternative
  was rejected as scope creep — `?updated=1` plus the audit log is
  sufficient signal for an operator. No new tests: the change is a pure
  dead-code removal with no behavioural delta. Suite still 333 / 868.
- **Where**: `src/Controllers/SettingsController.php` lines 95-127.
- **What**: `$changed[]` is appended to but the variable is never read.
  Audit happens on a per-key basis already; the local accumulator is dead.
- **Suggested fix**: Delete the variable, or use it to short-circuit the
  redirect (e.g. `?updated=N` instead of always `?updated=1`).

### R01-N29 — `Migrator::apply` reads file via `file_get_contents` without size cap
- **Severity**: LOW.
- **Status**: accepted-by-design — took the audit's own
  "None required; document the convention" disposition. The trust model
  is now spelled out in a phpdoc on `Migrator::apply()`: migration files
  live under `migrations/`, are committed code reviewed in PRs, and
  therefore share the trust boundary with the rest of `src/`. After
  R01-N22 the `apply()` path runs at deploy time inside
  `bin/migrate.php` (Docker entrypoint), not at request time, so an
  oversized file aborts the container start with a loud `RuntimeException`
  rather than OOM-ing a live request. If `migrations/` ever accepts
  non-reviewed input (e.g. a future `gh-pr-deploys`-style workflow that
  lands files there from outside the review pipeline) the comment calls
  out the revisit point. No SPEC change. No test change — the contract
  pinned by `MigratorTest` (R01-N22) covers the read path end-to-end.
- **Where**: `src/Db/Migrator.php` lines 101-125.
- **What**: A migration file copied accidentally as a multi-GB blob would
  OOM the request. Minor — migrations are committed code, not user-supplied.
- **Suggested fix**: None required; document the convention.

### R01-N30 — `import_upload.twig` reveals colour-mapping rules to anyone with the link
- **Severity**: LOW (info disclosure; admin-gated).
- **Status**: accepted-by-design — confirmed with `grep` that the only
  route emitting `import_upload.twig` is `GET /sprints/import` in
  `public/index.php`, which gates on `SessionGuard::requireAdmin($users)`
  before the view renders. An anonymous request 302s to `/auth/login`
  with a flash, never reaching the colour-mapping block. The mapping
  itself documents the *Tool_Sprint Planning* workbook's own colour
  convention — common knowledge inside the team that owns the workbook
  — so even if the gate were ever weakened, the disclosure cost is
  near zero. The audit's own Suggested fix is "None"; recording that
  here as the closing disposition. No code change.
- **Where**: `views/sprints/import_upload.twig` lines 42-52.
- **What**: Page is admin-gated (`SessionGuard::requireAdmin`) so this is
  not exposed publicly. Listed for completeness.
- **Suggested fix**: None.

### R01-N31 — `home.twig` link to `/healthz` is a tiny info-disclosure
- **Severity**: LOW.
- **Status**: fixed-in-`7fd849b` — folded into the R01-N02 fix; the
  `<a href="/healthz">` line lives inside the now admin-only Runtime
  panel, so anonymous users no longer see it.
- **Where**: `views/home.twig` line 135.
- **What**: A textual hint to the existence of `/healthz`. Liveness probes
  are normally undocumented to discourage scanning; the route returns
  literal "ok" anyway, so the disclosure cost is tiny. Combined with R01-N02
  the fix is the same: gate the runtime panel.
- **Suggested fix**: Falls out of R01-N02.

### R01-N32 — Sprint settings exposes the to-be-deleted name in plain HTML
- **Severity**: LOW.
- **Status**: accepted-by-design — confirmed by re-reading the code at
  `src/Controllers/SprintController.php::delete` (lines 1001-1023): the
  `confirm_name` server-side check runs AFTER `requireAdmin` and
  `verifyCsrf`, comparing `trim($req->postString('confirm_name'))`
  against the persisted `$sprint->name` and 302'ing back to the settings
  page with `?error=name_mismatch` on any divergence. The
  `data-confirm-name` value in `views/sprints/settings.twig` line 181 is
  consumed only by the in-page JS to keep the submit button disabled
  until the typed value matches — a UX nicety, not a guard. An attacker
  who can read the settings page is already authenticated as admin (the
  view itself is admin-gated by `requireAdmin` in `public/index.php`)
  and can read the sprint name from `<title>` / `<h1>` regardless. No
  code change. The audit's own Suggested fix is "None"; recording the
  closing disposition here matches the convention used for N29/N30.

### R01-N33 — `data-csrf` attribute reads CSRF token in HTML
- **Severity**: LOW.
- **Status**: accepted-by-design — re-confirmed by tracing the consumers:
  `public/assets/js/app.js` line 65 (htmx `htmx:configRequest` hook),
  `public/assets/js/sprint-settings.js` line 31, and
  `public/assets/js/sprint-planner.js` line 114 all read `data-csrf` and
  attach it as `X-CSRF-Token` on `fetch` / htmx requests. The token is
  per-session (rotated on `SessionGuard::login()` per R01-N08), 256
  bits of entropy via `bin2hex(random_bytes(32))`, and the strict CSP
  (`script-src 'self'`, no `unsafe-inline` / `unsafe-eval`,
  `frame-ancestors 'none'`) means no foreign script can read the
  attribute from the rendered DOM. Removing the attribute would force
  inline-script injection of the token (forbidden by CSP) or a separate
  fetch endpoint that returns the token (a worse pattern — the cookie
  already authenticates that fetch, so the endpoint adds no defence).
  The audit's own Suggested fix is "None. Confirmed acceptable.";
  recording the closing disposition here matches the convention used
  for N29/N30. No code change.

### R01-N34 — `Migrator` discovery regex permits unintended filenames
- **Severity**: LOW.
- **Status**: accepted-by-design — same trust-boundary disposition as
  R01-N29: the `migrations/` directory is committed source code,
  reviewed in PRs, and shares the trust boundary with the rest of
  `src/`. After R01-N22 the discovery and apply path runs at deploy
  time inside `bin/migrate.php` (called by `bin/docker-entrypoint.sh`)
  rather than on a live request, so even a hostile file would only
  fire on container start where the operator sees the
  `RuntimeException` in `docker logs`. The current six committed
  files (`001_init.sql` through `006_users_tombstoned.sql`) all match
  the regex by intention; tightening the pattern to a strict
  allow-list would force a code edit on every legitimate migration.
  Filesystem write access on the deploy host is already arbitrary
  code execution by other means (PHP source under `src/`, Apache
  config, the entrypoint script itself). Revisit only if
  `migrations/` ever accepts non-reviewed input — same revisit
  point R01-N29 already calls out. The audit's own Suggested fix is
  "None required (server filesystem is trusted)"; recording the
  closing disposition here matches the convention used for N29/N30.
  No code change.

---

## Things I checked and consider OK

These are not findings; recorded so we don't re-investigate later:

- **PDO + prepared statements**: every dynamic value goes through `?`
  placeholders except the integer-cast cases noted in R01-N10.
- **CSRF coverage**: every mutation route I traced calls `verifyCsrf` (or
  the JSON variant). The audit hits 18/18 mutations per Phase 7.
- **Twig auto-escape**: enabled (`html` context); no `|raw` usage in
  `views/`.
- **CSP**: strict (`script-src 'self'`, `style-src 'self'`, no
  `unsafe-eval` / `unsafe-inline`); Alpine CSP build, vendored htmx /
  Sortable. `frame-ancestors 'none'`. `form-action` allows
  `login.microsoftonline.com` (needed for OIDC).
- **`X-Frame-Options`**, **`X-Content-Type-Options`**, **`Referrer-Policy`**
  all set per request.
- **Foreign keys**: `PRAGMA foreign_keys = ON` set on every PDO
  connection.
- **Transactions**: every audited mutation runs inside an explicit `BEGIN…
  COMMIT` and rolls back on throw.
- **Cascade audit integrity** (Phase 8): all five paths
  (`sprint→tasks→task_assignments`, `sprint→sprint_workers→
  sprint_worker_days`, `sprint→sprint_workers→task_assignments`,
  `sprint→sprint_weeks→sprint_worker_days`, plus Phase 22 `tasks(linked_
  task_id)→SET NULL`) snapshot before the parent delete fires.
- **Local-admin email comparison**: timing-safe via `hash_equals`.
- **CSRF token entropy**: `bin2hex(random_bytes(32))` — 256 bits.
- **Cookie flags**: `HttpOnly`, `SameSite=Lax`, `Secure` (when behind
  HTTPS, see R01-N05). No need to mark non-essential cookies.
- **Upload file validation** (Phase 20): extension regex + ZIP magic-byte
  check + `is_uploaded_file` + 5 MB cap. Reasonable.
- **JSON envelope**: `{ok, data|error}` per spec §7; consistent across
  endpoints.
- **Strict types** + PSR-12: enforced; PHP 8.3 readonly properties.
- **Audit no-op rule**: canonical-JSON equal before/after produces no row,
  per spec §7.
- **Number-input validation**: `CapacityCalculator::isHalfStep` and the
  per-cell range checks are tight.
- **No `eval`, no `exec`, no `system`**: confirmed by grep.

---

## Suggested ordering for the fix loops

A reasonable cadence (do not treat as binding):

1. ~~**R01-N02** (homepage info disclosure)~~ — fixed in `7fd849b`. Also
   closes R01-N31.
2. ~~**R01-N04** (`SESSION_SECRET` doc cleanup)~~ — fixed in `296883c`.
3. ~~**R01-N15** (`rel="noopener noreferrer"`)~~ — fixed in `d16bff4`.
4. ~~**R01-N11** (audit column whitelist)~~ — fixed in `4ae1817`.
5. ~~**R01-N01** (local-admin password hashing)~~ — fixed in `857df15`
   (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~~ —
   fixed in `a2e77ea`.
9. ~~**R01-N08** (idle session timeout + CSRF rotation)~~ — fixed in
   `bc745cd`.
10. ~~**R01-N10** (bind-not-concat sweep)~~ — fixed in `c1dbfc1`.
11. ~~**R01-N12** (date filter validation)~~ — fixed in `1b28469`.
12. ~~**R01-N09** (cookie SameSite)~~ — accepted-by-design; Lax is
    required for the OIDC return navigation, see Status block.
13. ~~**R01-N13** (fatal-error safety net)~~ — fixed in `d7dbfb5`.
14. ~~**R01-N14** (XLSX session cap + abandoned-token audit)~~ —
    fixed in `d7dbfb5`.
15. ~~**R01-N18** (drop preferred_username fallback, honour
    email_verified)~~ — fixed in `a0b717e`.
16. ~~**R01-N16** (composer-audit helper + admin-manual cadence)~~ —
    fixed in `ef9b9b8`.
17. ~~**R01-N17** (concurrent-tab OIDC state)~~ — accepted-by-design;
    library uses fixed session keys, the realistic failure mode is a
    one-line UX nit, see Status block.
18. ~~**R01-N19** (CSP `report-uri` + audit endpoint)~~ — fixed in
    `f59f368`.
19. ~~**R01-N20** (Response::redirect contract)~~ — fixed in
    `f1aa924`.
20. ~~**R01-N21** (Twig autoescape pin + static |raw guard)~~ —
    fixed in `00bcf73`.
21. ~~**R01-N25** (X-Permitted-Cross-Domain-Policies header)~~ —
    fixed in `f6ce13f`.
22. ~~**R01-N26** (one-shot session flash for the post-delete chip)~~ —
    fixed in `1f11117`.
23. ~~**R01-N27** (entrypoint session-file GC loop)~~ —
    fixed in `32d03fc`.
24. ~~**R01-N28** (drop dead `$changed[]` in SettingsController)~~ —
    fixed in `216c15d`.
25. ~~**R01-N29** (Migrator file size — document trust model)~~ —
    accepted-by-design; phpdoc on `Migrator::apply()`.
26. ~~**R01-N30** (import_upload colour-mapping disclosure)~~ —
    accepted-by-design; admin-gated, listed for completeness.
27. ~~**R01-N32** (sprint name in `data-confirm-name`)~~ —
    accepted-by-design; server-side `confirm_name` check is the
    real defence, JS guard is UX.
28. ~~**R01-N33** (`data-csrf` attribute)~~ — accepted-by-design;
    standard pattern, strict CSP prevents foreign-origin reads.
29. ~~**R01-N34** (Migrator regex permits filenames)~~ —
    accepted-by-design; `migrations/` is committed code, post-N22
    apply runs at deploy time only.

All findings now resolved (`fixed-in-<sha>` or `accepted-by-design`).

Each fix should ship as its own commit per SPEC.md §14, with a follow-up
SPEC update if behaviour or config surface changes.