sprint_planer_web/doc/REVIEW_01.md

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: open.
• Where: src/Auth/SessionGuard.php lines 27-60, 109-116.
• What: session.gc_maxlifetime = 28800 (8h) and the session cookie has lifetime=0 (browser-session). Once authenticated, a user stays signed in until the browser closes or 8h passes since last GC tick. The CSRF token is generated once and reused for the lifetime of the session.
• Why it matters: A stolen session cookie is valid for hours; a stolen CSRF token paired with it lets the attacker submit any mutation that the user could.
• Suggested fix:
  • Track $_SESSION['login_at'] (already set!) and an idle timestamp; on each request, if now - last_active > 30 min, force re-auth.
  • Rotate the CSRF token on login() (after session_regenerate_id) so pre-login tokens can't be replayed.

R01-N09 — Session cookie SameSite=Lax, not Strict

• Severity: MEDIUM.
• Status: open — debatable; depends on threat model.
• Where: src/Auth/SessionGuard.php lines 47, 56-58.
• What: Lax allows the cookie on top-level cross-site GETs (a click from a phishing page). The OIDC redirect flow doesn't need cross-site GET cookies (it's a top-level navigation back to the app's own origin).
• Suggested fix: Set SameSite=Strict. Validate the OIDC callback flow still works (the library's state/PKCE check is what actually defends here; Strict is defence-in-depth).

R01-N10 — SQL string concatenation of integer parameters in repos

• Severity: MEDIUM (non-exploitable today, brittle by design).
• Status: open.
• Where:
  • src/Repositories/TaskRepository.php line 60 (MAX(sort_order) FROM tasks WHERE sprint_id = ' . $sprintId) and line 100 (moveToSprint).
  • src/Repositories/SprintWorkerRepository.php line 56 (MAX(sort_order) FROM sprint_workers WHERE sprint_id = ' . $sprintId).
  • src/Controllers/SprintController.php lines 1029-1033 — uses placeholders, OK.
• What: Integer route params are cast to int before reaching the repo (Router → controllers), so today these statements are SQL-injection-safe. But the contract is implicit: if any future caller passes an unvalidated string, the repo accepts it. Any code review tool (PHPStan / Psalm) that doesn't fully resolve types will flag this — and rightly so.
• Why it matters: Injection becomes one accidental refactor away.
• Suggested fix: Bind every value with ? placeholders; never concatenate. Mechanical change, low risk.

R01-N11 — AuditRepository::distinctColumn interpolates a column name

• Severity: MEDIUM (non-exploitable today; same brittleness class as N10).
• Status: 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:<oid>.

R01-N19 — Strict CSP has no report-uri / report-to

• Severity: MEDIUM (operational visibility).
• Status: open.
• Where: public/index.php lines 227-237.
• What: CSP is strict but silent — operators won't notice if a future view inadvertently introduces an inline handler or new external host until users complain.
• Suggested fix: Add a report-uri /csp-report (or report-to) directive; route it to a thin endpoint that drops the JSON in the audit log under entity_type='csp_violation'. Cheap; high signal.

R01-N20 — Response::redirect() accepts arbitrary location strings

• Severity: MEDIUM (no current open redirect; pattern is fragile).
• Status: open.
• Where: src/Http/Response.php lines 54-57.
• What: All current callers pass path-only or literal strings, so no open-redirect today. But the helper has no shape check: a future caller could pass ?next= from user input and redirect off-origin.
• Suggested fix: Either —
  • Document the contract ("path-only; never user input"), and add an assert() or runtime check that the value starts with / or matches APP_BASE_URL.
  • Provide a separate Response::external($url) for the rare cases that actually need to leave the origin.

R01-N21 — Trusted Twig auto-escape is the only XSS guard

• Severity: MEDIUM (audit recommendation).
• Status: open.
• Where: src/Http/View.php line 30 (autoescape => 'html'); various views.
• What: All user-supplied strings (sprint name, task title, task description, task url, worker names, audit JSON) are rendered through Twig with autoescape: html. Spot-checks confirm no |raw, |safe, or unsafe attr filters are used (one place uses |json_encode|e('html_attr') for the data-sprint-choices payload — correct usage).
• Why it matters: One careless future template that uses |raw on a user field opens stored XSS in a place where it's particularly painful (the audit viewer renders attacker-controlled content).
• Suggested fix:
  • Pin autoescape config in a CI test that loads View and asserts the flag.
  • Add a static-analysis grep step in CI: grep -rn '|raw' views/ must return nothing (or only allow-listed lines).

R01-N22 — Migrator runs at request time, swallows stale cache risk

• Severity: MEDIUM (operational hazard).
• Status: open.
• Where: public/index.php lines 73-85; src/Db/Migrator.php.
• What: Migrations run on every request as pdo->exec($sql) (multi- statement). On a fresh deploy, the first request after the new code ships does the migration; if that request crashes mid-migration (timeout, OOM), partial state may persist. SQLite + the migrator's BEGIN..COMMIT protects most cases, but ALTER TABLE … ADD COLUMN is a DDL that's conditionally inside a tx — see SQLite caveats.
• Why it matters: A partially-applied migration leaves the schema in a state where the new code disagrees with the DB and every subsequent request 500s.
• Suggested fix:
  • Move migration to a deploy-time step (entrypoint script that runs php bin/migrate.php before starting Apache).
  • Keep the request-time check as a safety net, but log loud + refuse to serve when versions disagree.

R01-N23 — users table not deleted; emails not redacted on demote

• Severity: MEDIUM (privacy / GDPR-ish).
• Status: open — documented in UserController header comment as intentional ("users are never deleted; they'd orphan audit rows").
• Where: src/Controllers/UserController.php lines 16-25.
• What: A user demoted from admin (or who simply leaves the org) keeps their email and display_name in the users table forever; the audit log's user_email column also retains it. There is no redact / tombstone flow.
• Why it matters: Privacy regulations may require name/email erasure on request. The current model can't honour that without breaking audit.
• Suggested fix:
  • Add a users.tombstoned_at column; when set, hide name/email from UI and replace with (former user). Audit rows keep the old email verbatim — the audit log is the authoritative trail and erasure laws typically permit retention for legal/security purposes.
  • Document the policy in doc/admin-manual.md.

R01-N24 — No request-body size limit on JSON endpoints

• Severity: MEDIUM.
• Status: open.
• Where: src/Http/Request.php lines 52-62, 84-101.
• What: file_get_contents('php://input') reads whatever PHP's post_max_size allows (default 8 MB). json_decode with depth=64 is reasonable, but a 7 MB JSON list with 1M entries will be parsed before any controller-level cap.
• Why it matters: Any admin can DoS the server by repeatedly POST'ing large bodies. Insider-only, but easy to do accidentally with a misbehaving client.
• Suggested fix:
  • Read Content-Length first; refuse >1 MB with 413.
  • Cap list lengths in batch endpoints (e.g. 5000 cells per /sprints/{id}/week-cells).

---

LOW

R01-N25 — No X-Permitted-Cross-Domain-Policies header

• Severity: LOW.
• Status: open.
• Where: public/index.php lines 240-243.
• What: Adobe Flash / Acrobat is dead, but defence-in-depth headers are cheap.
• Suggested fix: Add X-Permitted-Cross-Domain-Policies: none. Optional: Permissions-Policy to disable geolocation / camera / etc.

R01-N26 — home.twig "deleted" flash relies on query string truthiness

• Severity: LOW.
• Status: open.
• Where: views/home.twig lines 11-15; public/index.php line 150; src/Controllers/SprintController.php::delete line 1114.
• What: After delete, the user is redirected to /?deleted=<urlencoded name>. Anyone can manually craft a URL with ?deleted=foo and see the green "Sprint foo was deleted" chip — no actual deletion happened. Twig auto-escape protects against XSS, so this is purely a UX/spoofing nit.
• Suggested fix: Use a one-shot session flash ($_SESSION['flash_*'], the import controller already does this), unset on render.

R01-N27 — Session lifetime / GC is filesystem-only on container restart

• Severity: LOW.
• Status: open.
• Where: Dockerfile line 57; src/Auth/SessionGuard.php lines 33-39.
• What: Sessions are stored on a mounted volume. PHP's GC depends on request traffic to fire — a low-traffic deployment may keep stale session files for days past the 8h gc_maxlifetime. Not a session-validity issue (the session contains login_at + cookie expiry, see R01-N08), but a storage hygiene one.
• Suggested fix: Cron find /var/www/data/sessions -mmin +480 -delete in the container, or accept it as low-impact.

R01-N28 — SettingsController::update builds $changed[] but never uses it

• Severity: LOW (dead code).
• Status: open.
• Where: src/Controllers/SettingsController.php lines 95-127.
• What: $changed[] is appended to but the variable is never read. Audit happens on a per-key basis already; the local accumulator is dead.
• Suggested fix: Delete the variable, or use it to short-circuit the redirect (e.g. ?updated=N instead of always ?updated=1).

R01-N29 — Migrator::apply reads file via file_get_contents without size cap

• Severity: LOW.
• Status: open.
• Where: src/Db/Migrator.php lines 101-125.
• What: A migration file copied accidentally as a multi-GB blob would OOM the request. Minor — migrations are committed code, not user-supplied.
• Suggested fix: None required; document the convention.

R01-N30 — import_upload.twig reveals colour-mapping rules to anyone with the link

• Severity: LOW (info disclosure; admin-gated).
• Status: open.
• Where: views/sprints/import_upload.twig lines 42-52.
• What: Page is admin-gated (SessionGuard::requireAdmin) so this is not exposed publicly. Listed for completeness.
• Suggested fix: None.

R01-N31 — home.twig link to /healthz is a tiny info-disclosure

• Severity: LOW.
• Status: 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: 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).
10. R01-N10 (bind-not-concat sweep) — mechanical.
11. R01-N12 (date filter validation) — UX bug.
12. The rest as time permits.

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