# 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**: open — debatable; depends on threat model. - **Where**: `src/Auth/SessionGuard.php` lines 47, 56-58. - **What**: `Lax` allows the cookie on top-level cross-site GETs (a click from a phishing page). The OIDC redirect flow doesn't need cross-site GET cookies (it's a top-level navigation back to the app's own origin). - **Suggested fix**: Set `SameSite=Strict`. Validate the OIDC callback flow still works (the library's state/PKCE check is what actually defends here; `Strict` is defence-in-depth). ### R01-N10 — SQL string concatenation of integer parameters in repos - **Severity**: MEDIUM (non-exploitable today, brittle by design). - **Status**: 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**: open. - **Where**: - `src/Repositories/AuditRepository.php` lines 149-156. - `src/Controllers/AuditController.php` lines 39-49. - **What**: The filter values are concatenated as `"{$date}T00:00:00Z"` and bound. Any garbage string just sorts lexically against the ISO-8601 `occurred_at` column — silently producing empty result sets without an error. Not an injection (parameters are bound), but a bad UX and a way to hide events from a casual auditor (e.g., admin types `2024/01/01` instead of `2024-01-01` and "sees" no rows). - **Suggested fix**: Validate `Y-m-d` server-side; on parse failure, drop the filter and flash an error. Same `DateTimeImmutable::createFromFormat` pattern already used in `SprintController::isIsoDate`. ### R01-N13 — Output buffer can leak partial responses on error - **Severity**: MEDIUM. - **Status**: open. - **Where**: `public/index.php` lines 38, 248-254. - **What**: `ob_start()` is called at the top, but the flush is in the happy path. On a fatal error mid-request (uncaught throwable that escapes `dispatch()`, OOM, etc.), PHP's `output_buffering` may flush whatever was in the buffer to the response — without the security headers added at lines 240-243. - **Why it matters**: Stack traces, partial HTML, or anything `var_dump` could land in the buffer when `display_errors=1`. In production with `display_errors=0` the risk is small, but the headers (CSP, X-Frame, etc.) still won't be applied to whatever does land. - **Suggested fix**: - Register a `set_exception_handler` and a `register_shutdown_function` that discards the buffer (`ob_clean()`) and emits a minimal 500 page with the security headers. - Or drop `ob_start()` and rely on `Response::send()` to be the single output point; use `headers_sent()` checks before writing anything. ### R01-N14 — XLSX import session blob held in `$_SESSION` - **Severity**: MEDIUM (memory / DoS vector, not an injection). - **Status**: open. - **Where**: `src/Controllers/ImportController.php` lines 102-115 (write), 244-261 (read). Session storage path is on disk under `/var/www/data/sessions/`. - **What**: The full parsed workbook (every cell value, status, weeks, workers, tasks for every sheet) is JSON-stashed in PHP's session file. Limit is 5 MB on the upload but parsed expansion is unbounded — a hostile XLSX with many tabs can cost much more in memory + on-disk session size. - **Why it matters**: Disk pressure, slow session file IO, possible OOM during the second hop (`fromArray()` reconstruction). And on disk these files are readable by `www-data` only — but they linger 30 min by design. - **Suggested fix**: - Cap the per-token payload size; refuse parses that exceed e.g. 2 MB serialised JSON. - Or stream-parse + temp-file (signed file path) instead of session blob. - Minimum: also log a `IMPORT_PREVIEW_ABANDONED` audit row when a token expires unused. ### R01-N15 — `target="_blank"` task URL anchor uses `rel="noopener"` only - **Severity**: MEDIUM (Privacy; small). - **Status**: 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**: open. - **Where**: `composer.json` line 12 (`^3.4`), `composer.lock` (not read). - **What**: PhpSpreadsheet has a long history of XML-related vulns (XXE etc.) when reading XLSX. Version 3.4 disables XXE by default but the caret range allows minor upgrades, which is good — but operators may not rebuild promptly. - **Why it matters**: Importer is admin-only, but admins can still feed a hostile workbook (insider risk, compromised email, etc.). - **Suggested fix**: - Document upgrade cadence (Dockerfile rebuild) in admin manual. - Keep `setReadDataOnly(false)` (already correct — needed for fill colour parsing) but explicitly call `LIBXML_NOENT | LIBXML_DTDLOAD` to off if we ever reach to lower-level XML APIs. - Run `composer audit` in CI. ### R01-N17 — Login `state`/PKCE storage in session — verify lifetime - **Severity**: MEDIUM (compliance; needs runtime check). - **Status**: open. - **Where**: `src/Auth/OidcClient.php` + `src/Controllers/AuthController.php` callback. - **What**: `jumbojett/openid-connect-php` stores its `state` and PKCE `code_verifier` in `$_SESSION`. `SessionGuard::login()` calls `session_regenerate_id(true)` *after* the callback completes — but the state/verifier are written before the redirect to Entra and read after the bounce back. If two browser tabs initiate auth flows simultaneously, the second overwrites the first's state. Not a direct vuln but a usability hazard. - **Suggested fix**: Confirm the library's per-flow state isolation; if it stores under fixed keys, file an upstream issue or wrap with a per-flow random suffix. ### R01-N18 — Email-claim trust path during OIDC upsert - **Severity**: MEDIUM. - **Status**: open. - **Where**: `src/Controllers/AuthController.php` lines 75-77. - **What**: `email = $claims->email ?? $claims->preferred_username`. In Entra, `preferred_username` is *not* guaranteed to be a verified email; in some tenant configurations it can be controlled by the user. We don't check `email_verified` either. - **Why it matters**: If the audit log uses email as the human-readable actor identifier (it does — it's joined back via `user_email` in `audit_log`), a user can spoof their actor email. The `entra_oid` (or `sub`) is the immutable identifier and is correctly used as the lookup key — so the *user identity* is safe, but the *audit trail's display* isn't. - **Suggested fix**: - Prefer `claims.email` only when `claims.email_verified === true`. - Otherwise fall back to a documented, immutable label like `entra:`. ### 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) — UX bug. 12. The rest as time permits. Each fix should ship as its own commit per SPEC.md §14, with a follow-up SPEC update if behaviour or config surface changes.