Browse Source

Fix R01-N13 + R01-N14: fatal-error safety net + XLSX session cap

R01-N13 — front-controller safety net.

`public/index.php` calls `ob_start()` at the top so a stray
warning/notice can't send headers before `Response::send()` runs, but the
flush is in the happy path. On a fatal error mid-request (uncaught
throwable that escapes `dispatch()`, OOM, parse error in a lazily-loaded
class), PHP's `output_buffering` may flush whatever was in the buffer —
without the security headers (CSP, HSTS, X-Frame, X-Content-Type-Options,
Referrer-Policy) that the regular path applies.

New `App\Http\FatalErrorHandler` registers BOTH `set_exception_handler`
(catches uncaught throwables) 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 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. 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()`.

The `emit()` function takes injectable header / body / drain / headers-
sent callables so tests can capture each side-effect without touching
real PHP `header()` / stdout / output buffers (PHPUnit owns its own
buffers and forbids us from closing them). New
`tests/Http/FatalErrorHandlerTest.php` (7 cases) pins:

  - production hides throwable detail; dev surfaces it (HTML-escaped
    against XSS payloads in error messages);
  - HSTS rides along on HTTPS only;
  - headers are skipped cleanly when `headers_sent()` returns true
    (mid-flight fatal — body must still append);
  - the buffer drain runs exactly once, BEFORE any header / body write
    (otherwise the partial render the buffer holds would emit first);
  - `securityHeaders($isHttps)` matches the emitted set including the
    OIDC `form-action` allowance for `login.microsoftonline.com`.

R01-N14 — XLSX import session cap + abandoned-token audit.

The full parsed workbook (every cell value, status, weeks, workers,
tasks for every sheet) was JSON-stashed in PHP's session file. Limit
was 5 MB on the upload but parsed expansion is unbounded — a hostile
XLSX with many tabs could blow session file size + memory during the
second-hop `fromArray()` reconstruction.

`ImportController::MAX_SESSION_PAYLOAD_BYTES = 2 * 1024 * 1024` (2 MiB)
bounds the JSON-encoded preview blob. `upload()` encodes the
`ParsedSheet[]::toArray()` result once (reused as the size estimate
via the new pure-static `encodedPayloadBytes(array): int` helper —
same encoding flags as `AuditLogger::encodeJson`); a payload past the
cap is rejected with `?error=too_large_payload` (new entry in the
upload form's message map) and nothing lands in the session. The
session entry also gains a `payload_bytes` field so the abandoned-
token audit row has a real number rather than `unknown`.

Both pruning paths now record the abandonment:
`pruneSessionImports()` (fresh-upload sweep, 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/Controllers/ImportControllerTest.php` grew from 4 to 12 cases:
the cap constant is exactly 2 MiB (drift fence); the helper agrees
with a hand-rolled `json_encode + strlen`; the abandoned-audit
payload has the right keys + types + a strict ISO timestamp; missing
fields produce safe defaults; clock skew → age 0.

R01-N09 — accepted-by-design (separate doc-only follow-up).

The original "Suggested fix: SameSite=Strict" understated the cost:
when Entra redirects the user back to /auth/callback it is a *cross-
site initiated* top-level navigation, so a `SameSite=Strict` session
cookie would NOT be sent. The OIDC library reads `state` and PKCE
`code_verifier` from `$_SESSION` BEFORE the bounce — without the
cookie those lookups fail. We keep `Lax`; the real CSRF defence is
`SessionGuard::verifyCsrf()` (per-session token, `hash_equals()`),
not the cookie attribute.

Verified:
  - `for f in $(git ls-files '*.php'); do php -l "$f"; done` → clean.
  - Full PHPUnit suite via the project's docker image:
    `docker run --rm -v "$PWD:/app" -w /app sprint_planer_web-app:latest
     sh -c "composer install --quiet && vendor/bin/phpunit --colors=never"`
    → OK (242 tests, 673 assertions). Was 227 / 590.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
chiappa 2 ngày trước cách đây
mục cha
commit
d7dbfb587e

+ 22 - 33
public/index.php

@@ -15,6 +15,7 @@ use App\Controllers\UserController;
 use App\Controllers\WorkerController;
 use App\Db\Connection;
 use App\Db\Migrator;
+use App\Http\FatalErrorHandler;
 use App\Http\Request;
 use App\Http\Response;
 use App\Http\Router;
@@ -70,6 +71,15 @@ if ($appEnv !== 'production') {
     ini_set('display_errors', '0');
 }
 
+// ---------------------------------------------------------------------------
+// R01-N13: install the fatal-error safety net AS EARLY AS POSSIBLE — before
+// migrations, before service wiring. An uncaught throwable from anywhere
+// below now produces a minimal 500 page with full security headers instead
+// of leaking whatever was buffered. We re-register later (with the resolved
+// $isHttps) to flip the HSTS bit, but having the handler installed up-front
+// covers fatals during bootstrap (e.g. broken migration, missing class).
+FatalErrorHandler::register($appEnv, false);
+
 // ---------------------------------------------------------------------------
 // Migrations — cheap no-op when already current
 // ---------------------------------------------------------------------------
@@ -125,7 +135,7 @@ $importCommit   = new SprintImporter(
     $tasks, $taskAssign, $workers, $audit,
 );
 $importCtrl     = new ImportController(
-    $pdo, $users, $sprints, $xlsxParser, $importCommit, $view,
+    $pdo, $users, $sprints, $xlsxParser, $importCommit, $view, $audit,
 );
 
 // ---------------------------------------------------------------------------
@@ -254,40 +264,19 @@ if ($baseIsHttps && $knownHttp && $request->path !== '/healthz') {
     exit;
 }
 
+// R01-N13: now that we know the resolved HTTPS posture, re-register the
+// fatal handler so a fatal mid-dispatch lands HSTS too. Cheap; just a
+// closure replacement.
+FatalErrorHandler::register($appEnv, $baseIsHttps);
+
 $response = $router->dispatch($request);
 
-// Apply security headers to every response (spec §9). Kept here (instead of
-// Response::send) so the policy is visible + editable in one place.
-// HSTS is emitted whenever the canonical base URL is HTTPS — sticking the
-// header on plain-HTTP responses is harmless (browsers ignore it from
-// non-secure contexts) and avoids a gap during the very first redirect.
-$isHttps = $baseIsHttps;
-
-// Strict CSP (Phase 11 + Phase 19). Tailwind is pre-compiled at image-build
-// time, jQuery / jQuery UI are gone, and Alpine (CSP build), htmx, and
-// SortableJS are vendored under /assets/js/vendor/ — so script-src and
-// style-src are 'self' only, no 'unsafe-eval', no 'unsafe-inline', no CDN
-// hosts. font-src keeps `data:` for the few inline data-URL glyphs.
-$csp = implode('; ', [
-    "default-src 'self'",
-    "script-src 'self'",
-    "style-src 'self'",
-    "img-src 'self' data:",
-    "font-src 'self' data:",
-    "connect-src 'self'",
-    "frame-ancestors 'none'",
-    "base-uri 'self'",
-    "form-action 'self' https://login.microsoftonline.com",
-]);
-
-$response
-    ->withHeader('X-Content-Type-Options', 'nosniff')
-    ->withHeader('X-Frame-Options', 'DENY')
-    ->withHeader('Referrer-Policy', 'strict-origin-when-cross-origin')
-    ->withHeader('Content-Security-Policy', $csp);
-
-if ($isHttps) {
-    $response->withHeader('Strict-Transport-Security', 'max-age=31536000; includeSubDomains');
+// Apply security headers to every response (spec §9). Sourced from the
+// FatalErrorHandler so the happy path and the 500-fallback share a single
+// CSP + header set — there's no way for a future edit to drift between
+// the two paths.
+foreach (FatalErrorHandler::securityHeaders($baseIsHttps) as $name => $value) {
+    $response->withHeader($name, $value);
 }
 
 $response->send();

+ 122 - 9
src/Controllers/ImportController.php

@@ -7,11 +7,13 @@ namespace App\Controllers;
 use App\Auth\SessionGuard;
 use App\Domain\Import\ImportResult;
 use App\Domain\Import\ParsedSheet;
+use App\Domain\User;
 use App\Http\Request;
 use App\Http\Response;
 use App\Http\View;
 use App\Repositories\SprintRepository;
 use App\Repositories\UserRepository;
+use App\Services\AuditLogger;
 use App\Services\Import\SprintImporter;
 use App\Services\Import\XlsxSprintImporter;
 use PDO;
@@ -36,6 +38,19 @@ final class ImportController
     private const TTL_SECONDS = 1800;
     private const MAX_FILE_BYTES = 5 * 1024 * 1024;
 
+    /**
+     * R01-N14: hard cap on the JSON-encoded preview blob we stash in
+     * `$_SESSION` between the upload and the commit step. The 5 MB
+     * upload cap (`MAX_FILE_BYTES`) bounds raw bytes coming in, but
+     * parsed XLSX expansion is unbounded — a hostile workbook with
+     * many tabs could blow the session file size. Two megabytes is
+     * roomy for a real planning workbook (the production
+     * `Tool_Sprint Planning` one rarely exceeds a few hundred KB
+     * serialised) and small enough that on-disk session IO stays
+     * snappy.
+     */
+    public const MAX_SESSION_PAYLOAD_BYTES = 2 * 1024 * 1024;
+
     public function __construct(
         private readonly PDO              $pdo,
         private readonly UserRepository   $users,
@@ -43,6 +58,7 @@ final class ImportController
         private readonly XlsxSprintImporter $parser,
         private readonly SprintImporter   $committer,
         private readonly View             $view,
+        private readonly AuditLogger      $audit,
     ) {
     }
 
@@ -99,17 +115,28 @@ final class ImportController
             return Response::redirect('/sprints/import?error=parse_failed');
         }
 
+        // R01-N14: enforce the per-token serialised cap. We encode once
+        // for the size check and stash the array form (cheap to read,
+        // no decode hop on preview/commit). A parse that explodes past
+        // the cap is rejected outright; nothing lands in the session.
+        $sheetsArr     = array_map(fn(ParsedSheet $s) => $s->toArray(), $sheets);
+        $payloadBytes  = self::encodedPayloadBytes($sheetsArr);
+        if ($payloadBytes > self::MAX_SESSION_PAYLOAD_BYTES) {
+            return Response::redirect('/sprints/import?error=too_large_payload');
+        }
+
         $token = bin2hex(random_bytes(16));
         SessionGuard::start();
         if (!isset($_SESSION[self::SESSION_KEY]) || !is_array($_SESSION[self::SESSION_KEY])) {
             $_SESSION[self::SESSION_KEY] = [];
         }
         $_SESSION[self::SESSION_KEY][$token] = [
-            'created_at' => time(),
-            'sheets'     => array_map(fn(ParsedSheet $s) => $s->toArray(), $sheets),
-            'file_name'  => basename($orig),
+            'created_at'    => time(),
+            'sheets'        => $sheetsArr,
+            'file_name'     => basename($orig),
+            'payload_bytes' => $payloadBytes,
         ];
-        $this->pruneSessionImports();
+        $this->pruneSessionImports($req, $actor);
 
         return Response::redirect('/sprints/import/' . $token);
     }
@@ -121,7 +148,7 @@ final class ImportController
             return $actor;
         }
         $token = (string) ($params['token'] ?? '');
-        $entry = $this->loadSessionEntry($token);
+        $entry = $this->loadSessionEntry($token, $req, $actor);
         if ($entry === null) {
             return Response::redirect('/sprints/import?error=expired');
         }
@@ -165,7 +192,7 @@ final class ImportController
             return Response::text('CSRF token invalid', 403);
         }
         $token = (string) ($params['token'] ?? '');
-        $entry = $this->loadSessionEntry($token);
+        $entry = $this->loadSessionEntry($token, $req, $actor);
         if ($entry === null) {
             return Response::redirect('/sprints/import?error=expired');
         }
@@ -241,7 +268,7 @@ final class ImportController
     // ------------------------------------------------------------------ utils
 
     /** @return array{sheets: list<array<string,mixed>>, file_name: string, created_at: int}|null */
-    private function loadSessionEntry(string $token): ?array
+    private function loadSessionEntry(string $token, Request $req, User $actor): ?array
     {
         if (!preg_match('/^[0-9a-f]{32}$/', $token)) {
             return null;
@@ -254,13 +281,19 @@ final class ImportController
         $entry = $bag[$token];
         $createdAt = (int) ($entry['created_at'] ?? 0);
         if ($createdAt + self::TTL_SECONDS < time()) {
+            // R01-N14: emit IMPORT_PREVIEW_ABANDONED for the token the
+            // user just tried to use. They'll be redirected to the
+            // upload form with `?error=expired`; the audit row makes the
+            // expiry visible in `/audit` instead of disappearing
+            // silently.
+            $this->recordAbandonedImport($entry, $req, $actor);
             unset($_SESSION[self::SESSION_KEY][$token]);
             return null;
         }
         return $entry;
     }
 
-    private function pruneSessionImports(): void
+    private function pruneSessionImports(Request $req, User $actor): void
     {
         SessionGuard::start();
         $bag = $_SESSION[self::SESSION_KEY] ?? [];
@@ -269,12 +302,42 @@ final class ImportController
         }
         $cutoff = time() - self::TTL_SECONDS;
         foreach ($bag as $tok => $row) {
-            if (!is_array($row) || (int) ($row['created_at'] ?? 0) < $cutoff) {
+            if (!is_array($row)) {
+                unset($_SESSION[self::SESSION_KEY][$tok]);
+                continue;
+            }
+            if ((int) ($row['created_at'] ?? 0) < $cutoff) {
+                // R01-N14: token aged out without a commit. Same audit
+                // row as in `loadSessionEntry`.
+                $this->recordAbandonedImport($row, $req, $actor);
                 unset($_SESSION[self::SESSION_KEY][$tok]);
             }
         }
     }
 
+    /**
+     * R01-N14: write an IMPORT_PREVIEW_ABANDONED audit row for a
+     * preview token that expired before being committed. `entity_type`
+     * is `import_token` because no DB row backs the preview blob; the
+     * row carries enough metadata (file name, age, sheet count,
+     * payload size) for an admin reviewing `/audit` to reconstruct
+     * what was abandoned.
+     *
+     * @param array<string,mixed> $entry
+     */
+    private function recordAbandonedImport(array $entry, Request $req, User $actor): void
+    {
+        $this->audit->recordForRequest(
+            action:     'IMPORT_PREVIEW_ABANDONED',
+            entityType: 'import_token',
+            entityId:   null,
+            before:     null,
+            after:      self::abandonedAuditPayload($entry, time()),
+            req:        $req,
+            actor:      $actor,
+        );
+    }
+
     /** @return list<array{id:int,name:string,startDate:string,endDate:string}> */
     private function emptySprintCandidates(): array
     {
@@ -360,6 +423,56 @@ final class ImportController
         return implode(' ', $parts);
     }
 
+    /**
+     * R01-N14: serialised-byte estimate for a `ParsedSheet[]::toArray()`
+     * payload. Pure, so the cap can be unit-tested without a real
+     * upload + session round-trip.
+     *
+     * Encoding mirrors `AuditLogger::encodeJson`: UTF-8 plain, no
+     * escaping of slashes — what the session bag *would* serialise to
+     * if we were JSON-encoding it. PHP's session serialiser is
+     * different (PHP-serialized format) but the JSON byte length is a
+     * reliable proxy and matches the contract recorded in REVIEW_01.
+     *
+     * @param array<int,array<string,mixed>> $sheetsArr
+     */
+    public static function encodedPayloadBytes(array $sheetsArr): int
+    {
+        $json = json_encode(
+            $sheetsArr,
+            JSON_UNESCAPED_UNICODE | JSON_UNESCAPED_SLASHES,
+        );
+        return $json === false ? 0 : strlen($json);
+    }
+
+    /**
+     * R01-N14: payload for the IMPORT_PREVIEW_ABANDONED audit row.
+     * Keeps just enough metadata to reconstruct what was lost without
+     * persisting any user task content.
+     *
+     * @param  array<string,mixed> $entry  the session entry as stashed
+     *                                     by `upload()`
+     * @return array<string,mixed>
+     */
+    public static function abandonedAuditPayload(array $entry, int $now): array
+    {
+        $createdAt = (int) ($entry['created_at'] ?? 0);
+        $sheets    = (array) ($entry['sheets'] ?? []);
+        // Missing `created_at` reads as 0; without this guard the
+        // computed age would be the full unix-epoch offset (~50 yr),
+        // which is alarming noise in `/audit` for what is just a
+        // malformed entry. Same shape clamps clock-skew "future" rows
+        // to 0 instead of negative.
+        $age       = $createdAt > 0 ? max(0, $now - $createdAt) : 0;
+        return [
+            'file_name'     => (string) ($entry['file_name'] ?? ''),
+            'sheet_count'   => count($sheets),
+            'payload_bytes' => (int) ($entry['payload_bytes'] ?? 0),
+            'age_seconds'   => $age,
+            'created_at'    => $createdAt > 0 ? gmdate('Y-m-d\TH:i:s\Z', $createdAt) : '',
+        ];
+    }
+
     private static function looksLikeXlsx(string $origName, string $tmpPath): bool
     {
         if (!preg_match('/\.xlsx$/i', $origName)) {

+ 197 - 0
src/Http/FatalErrorHandler.php

@@ -0,0 +1,197 @@
+<?php
+
+declare(strict_types=1);
+
+namespace App\Http;
+
+use ErrorException;
+use Throwable;
+
+/**
+ * R01-N13: front-controller safety net.
+ *
+ * `public/index.php` calls `ob_start()` at the very top so a stray
+ * warning/notice cannot send headers before `Response::send()` runs.
+ * The flush, however, sits in the happy path: a fatal error mid-request
+ * (uncaught throwable, OOM, parse error in a lazily-loaded class) would
+ * otherwise let PHP flush whatever was in the buffer to the response —
+ * without the security headers (CSP, HSTS, X-Frame, etc.) the regular
+ * path applies.
+ *
+ * Registering this handler:
+ *
+ *   - replaces an uncaught throwable with a minimal `500 Server error`
+ *     HTML page, drained of any partial render that was buffered;
+ *   - applies the same security headers a normal response would carry
+ *     (so an error path is no weaker than the happy path);
+ *   - in non-production environments the throwable's class + message is
+ *     surfaced into the page; production gets a generic message.
+ *
+ * The class is pure-static and `emit()` accepts injectable header / body
+ * sinks so tests can capture both without touching real PHP `header()` /
+ * stdout state.
+ */
+final class FatalErrorHandler
+{
+    /**
+     * Strict CSP shared with the happy-path response in `public/index.php`.
+     * Tailwind is pre-compiled at image-build time; jQuery / jQuery UI are
+     * gone; Alpine (CSP build), htmx, and SortableJS are vendored under
+     * `/assets/js/vendor/`. So `script-src` and `style-src` are `'self'`
+     * only. `form-action` permits the OIDC redirect to Entra.
+     */
+    public const CSP =
+        "default-src 'self'; "
+        . "script-src 'self'; "
+        . "style-src 'self'; "
+        . "img-src 'self' data:; "
+        . "font-src 'self' data:; "
+        . "connect-src 'self'; "
+        . "frame-ancestors 'none'; "
+        . "base-uri 'self'; "
+        . "form-action 'self' https://login.microsoftonline.com";
+
+    /**
+     * Bit mask for "fatal in the sense that the request can no longer
+     * complete normally". User errors and recoverable type errors are
+     * included so a runtime `trigger_error(E_USER_ERROR)` from a
+     * controller goes through the same minimal-500 path instead of
+     * leaking a partial render.
+     */
+    private const FATAL_MASK = E_ERROR
+        | E_PARSE
+        | E_CORE_ERROR
+        | E_COMPILE_ERROR
+        | E_USER_ERROR
+        | E_RECOVERABLE_ERROR;
+
+    public static function register(string $appEnv, bool $isHttps): void
+    {
+        set_exception_handler(static function (Throwable $e) use ($appEnv, $isHttps): void {
+            self::emit($e, $appEnv, $isHttps);
+        });
+        register_shutdown_function(static function () use ($appEnv, $isHttps): void {
+            $last = error_get_last();
+            if ($last === null) {
+                return;
+            }
+            $type = (int) ($last['type'] ?? 0);
+            if (($type & self::FATAL_MASK) === 0) {
+                return;
+            }
+            $err = new ErrorException(
+                (string) ($last['message'] ?? 'fatal error'),
+                0,
+                $type,
+                (string) ($last['file'] ?? ''),
+                (int) ($last['line'] ?? 0),
+            );
+            self::emit($err, $appEnv, $isHttps);
+        });
+    }
+
+    /**
+     * Discard buffered output and write a minimal 500 page with the same
+     * security headers a regular response would carry.
+     *
+     * The four callable sinks are dependency-injection seams for tests:
+     * pass closures that record into arrays / no-op the drain instead of
+     * touching real PHP `header()` / stdout / output buffers. In
+     * production all are null and the function uses real PHP I/O.
+     *
+     * @param ?callable(string,bool):void $emitHeader    ($name_with_value, $replace)
+     * @param ?callable(string):void      $emitBody      ($body)
+     * @param ?callable():bool            $headersSent
+     * @param ?callable():void            $drainBuffers
+     */
+    public static function emit(
+        Throwable $error,
+        string $appEnv,
+        bool $isHttps,
+        ?callable $emitHeader = null,
+        ?callable $emitBody = null,
+        ?callable $headersSent = null,
+        ?callable $drainBuffers = null,
+    ): void {
+        $drainBuffers = $drainBuffers ?? static function (): void {
+            // Drain every level of buffered output so a partial render
+            // doesn't leak in front of (or interleave with) the 500
+            // page. ob_end_clean returns false at the bottom of the
+            // stack — break out then so we don't busy-loop.
+            while (ob_get_level() > 0) {
+                if (@ob_end_clean() === false) {
+                    break;
+                }
+            }
+        };
+        $drainBuffers();
+
+        $headersSent = $headersSent ?? static fn(): bool => headers_sent();
+        $emitHeader  = $emitHeader  ?? static function (string $line, bool $replace): void {
+            header($line, $replace);
+        };
+        $emitBody    = $emitBody    ?? static function (string $body): void {
+            echo $body;
+        };
+
+        if (!$headersSent()) {
+            // http_response_code via header() so the test sink sees it too.
+            $emitHeader('HTTP/1.1 500 Internal Server Error', true);
+            $emitHeader('Content-Type: text/html; charset=utf-8', true);
+            foreach (self::securityHeaders($isHttps) as $name => $value) {
+                $emitHeader($name . ': ' . $value, true);
+            }
+            // Belt-and-braces: also bump the response code via the
+            // dedicated PHP function. The header() call already covers
+            // most SAPIs; this is harmless when redundant.
+            @http_response_code(500);
+        }
+
+        $emitBody(self::renderBody($error, $appEnv));
+    }
+
+    /**
+     * Headers applied to every response (happy path AND fatal). Returned
+     * as a name=>value map so `public/index.php` can spread them onto a
+     * `Response` and the fatal path can write them with `header()`.
+     *
+     * @return array<string,string>
+     */
+    public static function securityHeaders(bool $isHttps): array
+    {
+        $h = [
+            'X-Content-Type-Options'  => 'nosniff',
+            'X-Frame-Options'         => 'DENY',
+            'Referrer-Policy'         => 'strict-origin-when-cross-origin',
+            'Content-Security-Policy' => self::CSP,
+        ];
+        if ($isHttps) {
+            $h['Strict-Transport-Security'] = 'max-age=31536000; includeSubDomains';
+        }
+        return $h;
+    }
+
+    /**
+     * Minimal HTML body. In production the class+message is dropped on
+     * the floor — operators have the audit log + the PHP error log and
+     * the user gets nothing exploitable. In dev / staging the throwable
+     * class + message is surfaced so a stack trace is one click away.
+     */
+    public static function renderBody(Throwable $error, string $appEnv): string
+    {
+        $detail = '';
+        if ($appEnv !== 'production') {
+            $payload = get_class($error) . ': ' . $error->getMessage();
+            $detail  = '<pre>' . htmlspecialchars($payload, ENT_QUOTES, 'UTF-8') . '</pre>';
+        }
+        return "<!doctype html>\n"
+            . "<html lang=\"en\"><head><meta charset=\"utf-8\">"
+            . "<title>500 — Server error</title></head>"
+            . "<body style=\"font-family:system-ui,sans-serif;max-width:40rem;margin:3rem auto;padding:0 1rem;\">"
+            . "<h1>500 — Server error</h1>"
+            . "<p>The application encountered a fatal error while processing your request. "
+            . "The incident has been logged.</p>"
+            . $detail
+            . "</body></html>\n";
+    }
+}

+ 108 - 0
tests/Controllers/ImportControllerTest.php

@@ -60,6 +60,114 @@ final class ImportControllerTest extends TestCase
         $this->assertSame('unknown', self::call('uploadErrorCode', 9999));
     }
 
+    // ---------------------------------------------------------------------
+    // R01-N14: per-token serialised-payload cap + abandoned-token audit
+    // ---------------------------------------------------------------------
+
+    public function testEncodedPayloadBytesIsZeroForEmpty(): void
+    {
+        $this->assertSame(2, ImportController::encodedPayloadBytes([]), 'JSON for [] is two bytes');
+    }
+
+    public function testEncodedPayloadBytesGrowsWithContent(): void
+    {
+        $small = ImportController::encodedPayloadBytes([['sheetName' => 'A', 'tasks' => []]]);
+        $big   = ImportController::encodedPayloadBytes([
+            ['sheetName' => 'A', 'tasks' => array_fill(0, 100, ['title' => str_repeat('x', 50)])],
+        ]);
+        $this->assertGreaterThan($small, $big);
+    }
+
+    public function testEncodedPayloadBytesMatchesJsonStrlen(): void
+    {
+        // The cap is "JSON byte length"; the helper must agree with what
+        // a manual json_encode() of the same payload reports — otherwise
+        // the contract documented in REVIEW_01.md drifts.
+        $sheetsArr = [
+            ['sheetName' => 'Sprint A', 'tasks' => [['title' => 'Älphå', 'days' => 1.5]]],
+            ['sheetName' => 'Sprint B', 'tasks' => []],
+        ];
+        $expected = strlen((string) json_encode(
+            $sheetsArr,
+            JSON_UNESCAPED_UNICODE | JSON_UNESCAPED_SLASHES,
+        ));
+        $this->assertSame($expected, ImportController::encodedPayloadBytes($sheetsArr));
+    }
+
+    public function testCapConstantMatchesReviewN14(): void
+    {
+        // Pin the cap at exactly 2 MiB. The number is documented in
+        // REVIEW_01.md and the upload-error message; if it changes, both
+        // pieces of documentation need to be edited too.
+        $this->assertSame(
+            2 * 1024 * 1024,
+            ImportController::MAX_SESSION_PAYLOAD_BYTES,
+            'cap drift: bumping this constant requires updating REVIEW_01.md + import_upload.twig',
+        );
+    }
+
+    public function testEncodedPayloadCanCrossTheCap(): void
+    {
+        // Sanity: a deliberately-large payload exceeds the cap. We feed
+        // the helper a payload just past the threshold to prove the
+        // arithmetic agrees in the direction the controller relies on.
+        $blob   = str_repeat('A', ImportController::MAX_SESSION_PAYLOAD_BYTES);
+        $sheets = [['sheetName' => 'big', 'data' => $blob]];
+        $this->assertGreaterThan(
+            ImportController::MAX_SESSION_PAYLOAD_BYTES,
+            ImportController::encodedPayloadBytes($sheets),
+        );
+    }
+
+    public function testAbandonedAuditPayloadShapeAndContents(): void
+    {
+        $entry = [
+            'created_at'    => 1_700_000_000,  // 2023-11-14T22:13:20Z
+            'file_name'     => 'planning.xlsx',
+            'payload_bytes' => 1234,
+            'sheets'        => [
+                ['sheetName' => 'A'],
+                ['sheetName' => 'B'],
+                ['sheetName' => 'C'],
+            ],
+        ];
+        $now = 1_700_000_300; // 5 minutes later
+
+        $payload = ImportController::abandonedAuditPayload($entry, $now);
+
+        $this->assertSame('planning.xlsx',         $payload['file_name']);
+        $this->assertSame(3,                       $payload['sheet_count']);
+        $this->assertSame(1234,                    $payload['payload_bytes']);
+        $this->assertSame(300,                     $payload['age_seconds']);
+        $this->assertSame('2023-11-14T22:13:20Z',  $payload['created_at']);
+    }
+
+    public function testAbandonedAuditPayloadHandlesMissingFields(): void
+    {
+        // Old or malformed session entries (e.g. from a session created
+        // before the payload_bytes field landed): the helper must not
+        // throw and must fall back to safe defaults rather than leaking
+        // PHP nulls into an audit row.
+        $payload = ImportController::abandonedAuditPayload([], 1_700_000_000);
+        $this->assertSame('',  $payload['file_name']);
+        $this->assertSame(0,   $payload['sheet_count']);
+        $this->assertSame(0,   $payload['payload_bytes']);
+        $this->assertSame(0,   $payload['age_seconds'], 'created_at=0 → age clamped to 0, not negative');
+        $this->assertSame('',  $payload['created_at'],   'no spoofed timestamp when created_at is missing');
+    }
+
+    public function testAbandonedAuditAgeNeverNegative(): void
+    {
+        // Defensive: clock skew or a session entry from "the future"
+        // (e.g. a system clock that just rolled back) must not produce
+        // a negative age — the audit row would render badly in /audit.
+        $payload = ImportController::abandonedAuditPayload(
+            ['created_at' => 2_000_000_000, 'sheets' => []],
+            1_999_999_900,
+        );
+        $this->assertSame(0, $payload['age_seconds']);
+    }
+
     /**
      * Reflectively call a private static helper on ImportController so we
      * don't need to expand its public surface for testability.

+ 241 - 0
tests/Http/FatalErrorHandlerTest.php

@@ -0,0 +1,241 @@
+<?php
+
+declare(strict_types=1);
+
+namespace App\Tests\Http;
+
+use App\Http\FatalErrorHandler;
+use App\Tests\TestCase;
+use RuntimeException;
+
+/**
+ * R01-N13: pin the front-controller safety net.
+ *
+ * The handler's two side-effecting concerns — emitting headers and
+ * echoing the body — are both injected as callables, so we can capture
+ * their arguments without touching real PHP `header()` or stdout.
+ */
+final class FatalErrorHandlerTest extends TestCase
+{
+    /** @var list<string> */
+    private array $headers;
+    private string $body;
+    private bool $headersSent;
+    private int $drainCalls;
+
+    protected function setUp(): void
+    {
+        $this->headers     = [];
+        $this->body        = '';
+        $this->headersSent = false;
+        $this->drainCalls  = 0;
+    }
+
+    public function testEmitWritesMinimal500WithSecurityHeadersInProduction(): void
+    {
+        FatalErrorHandler::emit(
+            new RuntimeException('database is on fire'),
+            'production',
+            isHttps: true,
+            emitHeader: $this->headerSink(),
+            emitBody:   $this->bodySink(),
+            headersSent: fn(): bool => $this->headersSent,
+            drainBuffers: $this->drainSink(),
+        );
+
+        self::assertSame(1, $this->drainCalls, 'emit must drain buffers exactly once');
+
+        self::assertContains('HTTP/1.1 500 Internal Server Error', $this->headers);
+        self::assertContains('Content-Type: text/html; charset=utf-8', $this->headers);
+        self::assertContains('X-Content-Type-Options: nosniff', $this->headers);
+        self::assertContains('X-Frame-Options: DENY', $this->headers);
+        self::assertContains('Referrer-Policy: strict-origin-when-cross-origin', $this->headers);
+        self::assertContains(
+            'Strict-Transport-Security: max-age=31536000; includeSubDomains',
+            $this->headers,
+            'HSTS must ride along when isHttps=true',
+        );
+        $cspLine = $this->findHeader('Content-Security-Policy');
+        self::assertNotNull($cspLine);
+        self::assertStringContainsString("default-src 'self'",  $cspLine);
+        self::assertStringContainsString("script-src 'self'",   $cspLine);
+        self::assertStringContainsString("frame-ancestors 'none'", $cspLine);
+        self::assertStringContainsString(
+            "form-action 'self' https://login.microsoftonline.com",
+            $cspLine,
+            'OIDC redirect target must remain reachable from the error page',
+        );
+
+        self::assertStringContainsString('500 — Server error', $this->body);
+        self::assertStringNotContainsString(
+            'database is on fire',
+            $this->body,
+            'production must NOT leak the throwable message',
+        );
+        self::assertStringNotContainsString(
+            'RuntimeException',
+            $this->body,
+            'production must NOT leak the throwable class',
+        );
+    }
+
+    public function testEmitLeaksThrowableDetailInDevelopment(): void
+    {
+        FatalErrorHandler::emit(
+            new RuntimeException('PDOException: no such table'),
+            'development',
+            isHttps: false,
+            emitHeader: $this->headerSink(),
+            emitBody:   $this->bodySink(),
+            headersSent: fn(): bool => $this->headersSent,
+            drainBuffers: $this->drainSink(),
+        );
+
+        self::assertStringContainsString('500 — Server error', $this->body);
+        self::assertStringContainsString('RuntimeException',   $this->body);
+        self::assertStringContainsString('no such table',      $this->body);
+    }
+
+    public function testEmitOmitsHstsWhenNotHttps(): void
+    {
+        FatalErrorHandler::emit(
+            new RuntimeException('boom'),
+            'production',
+            isHttps: false,
+            emitHeader: $this->headerSink(),
+            emitBody:   $this->bodySink(),
+            headersSent: fn(): bool => $this->headersSent,
+            drainBuffers: $this->drainSink(),
+        );
+
+        foreach ($this->headers as $h) {
+            self::assertStringStartsNotWith(
+                'Strict-Transport-Security:',
+                $h,
+                'HSTS must not be emitted on plain HTTP',
+            );
+        }
+    }
+
+    public function testEmitSkipsHeadersWhenAlreadySent(): void
+    {
+        // Mid-flight fatal: PHP has already written status + some headers.
+        // We must NOT try to set new ones (PHP would warn) — but the body
+        // append should still happen so users see *something*.
+        $this->headersSent = true;
+
+        FatalErrorHandler::emit(
+            new RuntimeException('mid-flight'),
+            'production',
+            isHttps: true,
+            emitHeader: $this->headerSink(),
+            emitBody:   $this->bodySink(),
+            headersSent: fn(): bool => $this->headersSent,
+            drainBuffers: $this->drainSink(),
+        );
+
+        self::assertSame([], $this->headers);
+        self::assertStringContainsString('500 — Server error', $this->body);
+        self::assertSame(
+            1,
+            $this->drainCalls,
+            'buffer must still be drained even when headers are already sent — partial output is dangerous',
+        );
+    }
+
+    public function testEmitInvokesDrainBeforeWriting(): void
+    {
+        // Pin the drain → write ordering: if the drain ran AFTER the
+        // body sink, the partial render the buffer holds would be
+        // emitted first, in front of (or interleaved with) our 500
+        // page. Capture the call order via a closure that records the
+        // sequence and assert drain comes first.
+        $order = [];
+        FatalErrorHandler::emit(
+            new RuntimeException('boom'),
+            'production',
+            isHttps: false,
+            emitHeader: function (string $line, bool $replace) use (&$order): void {
+                $order[] = 'header';
+            },
+            emitBody: function (string $body) use (&$order): void {
+                $order[] = 'body';
+            },
+            headersSent: fn(): bool => false,
+            drainBuffers: function () use (&$order): void {
+                $order[] = 'drain';
+            },
+        );
+
+        self::assertSame('drain', $order[0] ?? null, 'buffer drain must precede any header / body write');
+        self::assertContains('body', $order);
+    }
+
+    public function testRenderBodyEscapesThrowableMessageInDev(): void
+    {
+        $body = FatalErrorHandler::renderBody(
+            new RuntimeException('<script>alert("xss")</script>'),
+            'development',
+        );
+        self::assertStringNotContainsString(
+            '<script>alert("xss")</script>',
+            $body,
+            'dev message must be HTML-escaped before display',
+        );
+        self::assertStringContainsString('&lt;script&gt;', $body);
+    }
+
+    public function testSecurityHeadersHelperMatchesEmittedSet(): void
+    {
+        $h = FatalErrorHandler::securityHeaders(true);
+        self::assertSame('nosniff',                                $h['X-Content-Type-Options']);
+        self::assertSame('DENY',                                   $h['X-Frame-Options']);
+        self::assertSame('strict-origin-when-cross-origin',        $h['Referrer-Policy']);
+        self::assertSame('max-age=31536000; includeSubDomains',    $h['Strict-Transport-Security']);
+        self::assertArrayHasKey('Content-Security-Policy', $h);
+
+        $hPlain = FatalErrorHandler::securityHeaders(false);
+        self::assertArrayNotHasKey('Strict-Transport-Security', $hPlain);
+    }
+
+    /** @return callable(string,bool):void */
+    private function headerSink(): callable
+    {
+        return function (string $line, bool $replace): void {
+            $this->headers[] = $line;
+        };
+    }
+
+    /** @return callable(string):void */
+    private function bodySink(): callable
+    {
+        return function (string $body): void {
+            $this->body .= $body;
+        };
+    }
+
+    /**
+     * No-op drain that just counts calls. Lets us assert "drain was
+     * invoked exactly once" without disturbing PHPUnit's own output
+     * buffers (which it owns and forbids us from closing).
+     *
+     * @return callable():void
+     */
+    private function drainSink(): callable
+    {
+        return function (): void {
+            $this->drainCalls++;
+        };
+    }
+
+    private function findHeader(string $name): ?string
+    {
+        $needle = strtolower($name) . ':';
+        foreach ($this->headers as $h) {
+            if (str_starts_with(strtolower($h), $needle)) {
+                return $h;
+            }
+        }
+        return null;
+    }
+}

+ 12 - 11
views/sprints/import_upload.twig

@@ -1,17 +1,18 @@
 {% extends "layout.twig" %}
 
 {% set errorMessages = {
-    'no_file':         'No file was uploaded.',
-    'too_big':         'The file is larger than 5 MB.',
-    'partial':         'The upload was interrupted. Try again.',
-    'upload_invalid':  'Upload validation failed.',
-    'not_xlsx':        'That doesn’t look like an .xlsx file.',
-    'parse_failed':    'Could not parse the workbook. Open it in Excel to confirm it’s not corrupted.',
-    'expired':         'Your previous import session expired. Please upload again.',
-    'nothing_selected':'No sheets were selected to import.',
-    'server':          'Server upload error.',
-    'size':            'File size is invalid.',
-    'unknown':         'Upload failed.',
+    'no_file':           'No file was uploaded.',
+    'too_big':           'The file is larger than 5 MB.',
+    'partial':           'The upload was interrupted. Try again.',
+    'upload_invalid':    'Upload validation failed.',
+    'not_xlsx':          'That doesn’t look like an .xlsx file.',
+    'parse_failed':      'Could not parse the workbook. Open it in Excel to confirm it’s not corrupted.',
+    'too_large_payload': 'The parsed workbook is too large for a preview session (limit: 2 MB). Split sheets or shorten task lists and try again.',
+    'expired':           'Your previous import session expired. Please upload again.',
+    'nothing_selected':  'No sheets were selected to import.',
+    'server':            'Server upload error.',
+    'size':              'File size is invalid.',
+    'unknown':           'Upload failed.',
 } %}
 
 {% block content %}