doc/REVIEW_01.md: initial security + fishy-pattern audit

Full-repo review against SPEC.md @ 3728106. 34 findings (R01-N01..N34)
ordered by severity, plus a "checked & OK" section and a 12-step
suggested fix queue so /loop iterations can pick the next item.

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

doc/REVIEW_01.md

@@ -0,0 +1,640 @@
+# 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**: open — partially documented as by-design in SPEC §8 / .env.example.
+- **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**: open.
+- **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**: open — documented in SPEC §8 as intended.
+- **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**: open.
+- **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**: open.
+- **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**: open.
+- **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**: open.
+- **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**: open.
+- **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**: open — debatable; depends on threat model.
+- **Where**: `src/Auth/SessionGuard.php` lines 47, 56-58.
+- **What**: `Lax` allows the cookie on top-level cross-site GETs (a click
+  from a phishing page). The OIDC redirect flow doesn't need cross-site GET
+  cookies (it's a top-level navigation back to the app's own origin).
+- **Suggested fix**: Set `SameSite=Strict`. Validate the OIDC callback
+  flow still works (the library's state/PKCE check is what actually defends
+  here; `Strict` is defence-in-depth).
+
+### R01-N10 — SQL string concatenation of integer parameters in repos
+- **Severity**: MEDIUM (non-exploitable today, brittle by design).
+- **Status**: open.
+- **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**: open.
+- **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**: open.
+- **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**: open.
+- **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**: open.
+- **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**: open.
+- **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**: open.
+- **Where**: `composer.json` line 12 (`^3.4`), `composer.lock` (not read).
+- **What**: PhpSpreadsheet has a long history of XML-related vulns
+  (XXE etc.) when reading XLSX. Version 3.4 disables XXE by default but the
+  caret range allows minor upgrades, which is good — but operators may not
+  rebuild promptly.
+- **Why it matters**: Importer is admin-only, but admins can still feed a
+  hostile workbook (insider risk, compromised email, etc.).
+- **Suggested fix**:
+  - Document upgrade cadence (Dockerfile rebuild) in admin manual.
+  - Keep `setReadDataOnly(false)` (already correct — needed for fill colour
+    parsing) but explicitly call `LIBXML_NOENT | LIBXML_DTDLOAD` to off if
+    we ever reach to lower-level XML APIs.
+  - Run `composer audit` in CI.
+
+### R01-N17 — Login `state`/PKCE storage in session — verify lifetime
+- **Severity**: MEDIUM (compliance; needs runtime check).
+- **Status**: open.
+- **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**: Confirm the library's per-flow state isolation; if it
+  stores under fixed keys, file an upstream issue or wrap with a per-flow
+  random suffix.
+
+### R01-N18 — Email-claim trust path during OIDC upsert
+- **Severity**: MEDIUM.
+- **Status**: open.
+- **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**: open.
+- **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**: open.
+- **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**: open.
+- **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**: open.
+- **Where**: `public/index.php` lines 73-85; `src/Db/Migrator.php`.
+- **What**: Migrations run on every request as `pdo->exec($sql)` (multi-
+  statement). On a fresh deploy, the *first* request after the new code
+  ships does the migration; if that request crashes mid-migration (timeout,
+  OOM), partial state may 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**: open — documented in `UserController` header comment as
+  intentional ("users are never deleted; they'd orphan audit rows").
+- **Where**: `src/Controllers/UserController.php` lines 16-25.
+- **What**: A user demoted from admin (or who simply leaves the org) keeps
+  their `email` and `display_name` in the `users` table forever; the audit
+  log's `user_email` column also retains it. There is 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**: open.
+- **Where**: `src/Http/Request.php` lines 52-62, 84-101.
+- **What**: `file_get_contents('php://input')` reads whatever PHP's
+  `post_max_size` allows (default 8 MB). `json_decode` with depth=64 is
+  reasonable, but a 7 MB JSON list with 1M entries will be 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**: open.
+- **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**: open.
+- **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**: open.
+- **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**: open.
+- **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**: open.
+- **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**: open.
+- **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**: open.
+- **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**: open.
+- **Where**: `views/sprints/settings.twig` line 181 (`data-confirm-name=…`).
+- **What**: The expected confirmation string (the sprint's own name) is
+  emitted in a `data-` attribute so the JS can compare against typed input.
+  An attacker who can read the page already has the name from the title
+  bar — but the JS guard is bypassable trivially. The server-side check at
+  `SprintController::delete` lines 1001-1004 is the real defence.
+- **Suggested fix**: None (server check is authoritative, JS is UX). The
+  spec already calls this out as defence-in-depth.
+
+### R01-N33 — `data-csrf` attribute reads CSRF token in HTML
+- **Severity**: LOW.
+- **Status**: open — necessary for AJAX usage.
+- **Where**: `views/sprints/show.twig` line 9, `present.twig` line 7,
+  `settings.twig` line 9.
+- **What**: The CSRF token is rendered into a `data-csrf` attribute so
+  vanilla JS (`fetch` calls) can attach it as `X-CSRF-Token`. This is the
+  standard pattern; not a leak (the token is per-session and JS-readable
+  by design).
+- **Suggested fix**: None. Confirmed acceptable.
+
+### R01-N34 — `Migrator` discovery regex permits unintended filenames
+- **Severity**: LOW.
+- **Status**: open.
+- **Where**: `src/Db/Migrator.php` line 91.
+- **What**: `^(\d{3,})_[A-Za-z0-9_\-]+\.sql$` — accepts e.g.
+  `001_evil_inject.sql` if dropped into `migrations/`. That is, a bad actor
+  with write access to `migrations/` can run arbitrary SQL on next request.
+  This is an "if attacker has filesystem access, game over" scenario, but
+  worth noting that the migration runner does no signature / allow-list
+  check.
+- **Suggested fix**: None required (server filesystem is trusted). If this
+  ever changes (e.g. a `gh-pr-deploys` workflow that lets non-admins land
+  files in `migrations/`), reconsider.
+
+---
+
+## 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) — 10-line edit, big perception
+   win.
+2. **R01-N04** (`SESSION_SECRET` doc cleanup) — pure docs.
+3. **R01-N15** (`rel="noopener noreferrer"`) — one-character fix.
+4. **R01-N11** (audit column whitelist) — defensive, trivial.
+5. **R01-N01** (local-admin password hashing) — biggest single security
+   improvement; touches `LocalAdmin` + admin manual.
+6. **R01-N06** (login throttling) — tiny new repo + integration.
+7. **R01-N03** (first-user bootstrap hardening).
+8. **R01-N07** + **R01-N05** (proxy / HTTPS handling) — paired.
+9. **R01-N08** (idle session timeout + CSRF rotation).
+10. **R01-N10** (bind-not-concat sweep) — mechanical.
+11. **R01-N12** (date filter validation) — UX bug.
+12. The rest as time permits.
+
+Each fix should ship as its own commit per SPEC.md §14, with a follow-up
+SPEC update if behaviour or config surface changes.