# 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-` (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-` 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-` 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 `
`). - **Where**: `views/home.twig` lines 112-138 — the "Runtime" `
` 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_` 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:` 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:` 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:`. ### 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=`. 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**: fixed-in-`7fd849b` — folded into the R01-N02 fix; the `` 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**: 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)~~ — 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. 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.