Home Esplora Aiuto
Accedi
chiappa
/
sprint_planer_web
mirror da https://git.snix.ch/chiappa/sprint_planer_web.git
1
0
Forka 0
File Problemi 0
Albero (Tree): 4f5c0013e9
Rami (Branch) Tag
sprint_planer_w...
/
doc
/
REVIEW_01.md

REVIEW_01.md 69 KB
Cronologia Originale

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.