Fix R01-N24: 1 MiB body cap + 5000-item batch cap on JSON endpoints

The default `post_max_size = 8M` is too generous for an admin-only
internal tool — a misbehaving client (or a hostile insider) could
repeatedly POST multi-megabyte JSON and pin the server. Two layers of
defence: a request-level body cap that short-circuits before any
controller runs, and a per-batch list cap on the JSON endpoints that
loop over user-supplied lists.

- src/Http/Request.php: `MAX_BODY_BYTES = 1 MiB`. `fromGlobals()`
  now consults `Content-Length` first — when oversized, the body is
  not even read into PHP memory — and falls back to a post-read
  length check for clients that under-report or omit the header.
  The new `bodyTooLarge` constructor flag carries the decision to
  the front controller; existing callers keep working (default
  false, json() naturally returns null when rawBody is blank).
- public/index.php: short-circuit oversized requests with a 413
  + `Response::err('request_too_large', …)` BEFORE the HTTPS
  redirect, so a TLS hop doesn't burn round-trips on something
  we'd reject anyway. The output buffer is drained the same way
  the happy path does.
- src/Controllers/SprintController.php + TaskController.php:
  `MAX_BATCH_ITEMS = 5000` constant. `updateWeekCells`,
  `updateAssignments`, `updateAssignmentsStatus`, and tasks
  `reorder` reject lists past the cap with a
  `too_many_items` 413 envelope. The cap is comfortably above any
  real workload (max worker count × 26-week limit = well under
  5000) but stops a 1 MiB body packed with thousands of tiny cells
  from running a long upsert loop inside one transaction.
- tests/Http/RequestTest.php gains 4 cases pinning the constant
  (drift fence — bumping it changes a published HTTP contract),
  the constructor flag default and explicit wiring, and the
  json()-returns-null contract when bodyTooLarge is set.
- tests/Controllers/SprintControllerTest.php +
  tests/Controllers/TaskControllerTest.php (new) pin the
  MAX_BATCH_ITEMS constant on both controllers and assert they
  have not drifted apart from each other.

Tests: 331 / 860 (was 325 / 853).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

public/index.php

@@ -259,6 +259,27 @@ $router->post('/settings',                            $settingsCtrl->update(...)
 // ---------------------------------------------------------------------------
 $request = Request::fromGlobals();
 
+// R01-N24: refuse to dispatch oversized request bodies. The cap (1 MiB,
+// `Request::MAX_BODY_BYTES`) is well above any legitimate JSON payload the
+// app expects — the largest batch endpoint
+// (`PATCH /sprints/{id}/week-cells`) is bounded at 5000 cells in the
+// controller, comfortably under that. Bodies are short-circuited inside
+// `Request::fromGlobals()` so the buffer is never held in PHP memory.
+// `Response::err` is the canonical JSON envelope; HTML form clients will
+// see the JSON in the browser, which is acceptable for a degenerate
+// >1 MiB form POST.
+if ($request->bodyTooLarge) {
+    Response::err(
+        'request_too_large',
+        'Request body exceeds the ' . (Request::MAX_BODY_BYTES >> 10) . ' KiB cap',
+        413,
+    )->send();
+    if (ob_get_level() > 0) {
+        @ob_end_flush();
+    }
+    exit;
+}
+
 // R01-N05: when `APP_BASE_URL` declares HTTPS, refuse to serve sensitive
 // flows over plain HTTP — redirect the user to the canonical scheme before
 // any controller, session, or auth logic runs. `/healthz` is exempt so

src/Controllers/SprintController.php

@@ -29,6 +29,18 @@ use Throwable;
 
 final class SprintController
 {
+    /**
+     * R01-N24: defence-in-depth cap on the batch JSON endpoints. The 1 MiB
+     * `Request::MAX_BODY_BYTES` already gates the wire format; this cap
+     * stops an attacker (or a buggy client) from packing tens of thousands
+     * of small cells inside a still-under-1MiB body and pinning the DB on
+     * a long upsert loop. Real workloads are nowhere near this limit:
+     * `n_weeks` is capped at 26 in the schema and a sprint never carries
+     * tens of admins-on-its-back, so 5000 cells in one PATCH is already
+     * comfortably more than any genuine UI flow needs.
+     */
+    public const MAX_BATCH_ITEMS = 5000;
+
     public function __construct(
         private readonly PDO                       $pdo,
         private readonly UserRepository            $users,
@@ -793,6 +805,13 @@ final class SprintController
         if ($body === []) {
             return Response::ok(['applied' => 0, 'noop' => 0, 'per_worker' => new \stdClass()]);
         }
+        if (count($body) > self::MAX_BATCH_ITEMS) {
+            return Response::err(
+                'too_many_items',
+                'cell list exceeds ' . self::MAX_BATCH_ITEMS . '-item cap',
+                413,
+            );
+        }
 
         // Cross-check every cell belongs to this sprint.
         $validSw = array_column(

src/Controllers/TaskController.php

@@ -31,6 +31,13 @@ use Throwable;
  */
 final class TaskController
 {
+    /**
+     * R01-N24: see SprintController::MAX_BATCH_ITEMS. Same rationale, same
+     * cap; reorder / assignments / status share the cell-style payload
+     * shape and need the same defence-in-depth bound.
+     */
+    public const MAX_BATCH_ITEMS = 5000;
+
     public function __construct(
         private readonly PDO                       $pdo,
         private readonly UserRepository            $users,
@@ -267,6 +274,13 @@ final class TaskController
         if (!is_array($body) || !array_is_list($body)) {
             return Response::err('validation', 'body must be a list of {task_id, sort_order}', 422);
         }
+        if (count($body) > self::MAX_BATCH_ITEMS) {
+            return Response::err(
+                'too_many_items',
+                'reorder list exceeds ' . self::MAX_BATCH_ITEMS . '-item cap',
+                413,
+            );
+        }
 
         $ordering  = [];
         $seenOrder = [];
@@ -327,6 +341,13 @@ final class TaskController
         if ($body === []) {
             return Response::ok(['applied' => 0, 'noop' => 0]);
         }
+        if (count($body) > self::MAX_BATCH_ITEMS) {
+            return Response::err(
+                'too_many_items',
+                'assignment list exceeds ' . self::MAX_BATCH_ITEMS . '-item cap',
+                413,
+            );
+        }
 
         // Cross-check sprint worker IDs belong to the task's sprint.
         $validSw = [];
@@ -430,6 +451,13 @@ final class TaskController
         if ($body === []) {
             return Response::ok(['applied' => 0, 'noop' => 0]);
         }
+        if (count($body) > self::MAX_BATCH_ITEMS) {
+            return Response::err(
+                'too_many_items',
+                'status list exceeds ' . self::MAX_BATCH_ITEMS . '-item cap',
+                413,
+            );
+        }
 
         $validSw = [];
         foreach ($this->sprintWorkers->allForSprint($task->sprintId) as $sw) {

src/Http/Request.php

@@ -6,6 +6,16 @@ namespace App\Http;
 
 final class Request
 {
+    /**
+     * R01-N24: hard cap on request body size. PHP's default
+     * `post_max_size` is 8 MiB; this stricter app-level cap kicks in
+     * before any controller logic so a runaway client cannot pump the
+     * server with multi-megabyte JSON. Any non-safe request whose
+     * body (or declared `Content-Length`) exceeds this cap is rejected
+     * with 413 by the front controller before dispatch.
+     */
+    public const MAX_BODY_BYTES = 1024 * 1024; // 1 MiB
+
     /**
      * @param array<string,mixed>  $query   raw $_GET (may contain arrays for ?a[]=…)
      * @param array<string,mixed>  $post    raw $_POST
@@ -20,6 +30,7 @@ final class Request
         public readonly string $rawBody,
         public readonly array  $headers,
         public readonly array  $server,
+        public readonly bool   $bodyTooLarge = false,
     ) {
     }
 
@@ -49,16 +60,35 @@ final class Request
             $headers['content-length'] = (string) $_SERVER['CONTENT_LENGTH'];
         }
 
-        $raw = (string) (file_get_contents('php://input') ?: '');
+        // R01-N24: short-circuit oversized requests BEFORE reading the body
+        // into memory. The Content-Length check is the primary gate; the
+        // post-read length check is a defence-in-depth fallback for clients
+        // that under-report (or omit) the header.
+        $bodyTooLarge = false;
+        $declaredLen  = isset($_SERVER['CONTENT_LENGTH']) && is_scalar($_SERVER['CONTENT_LENGTH'])
+            ? (int) $_SERVER['CONTENT_LENGTH']
+            : 0;
+
+        if ($declaredLen > self::MAX_BODY_BYTES) {
+            $bodyTooLarge = true;
+            $raw          = '';
+        } else {
+            $raw = (string) (file_get_contents('php://input') ?: '');
+            if (strlen($raw) > self::MAX_BODY_BYTES) {
+                $bodyTooLarge = true;
+                $raw          = '';
+            }
+        }
 
         return new self(
-            method:  $method,
-            path:    $path,
-            query:   $_GET,
-            post:    $_POST,
-            rawBody: $raw,
-            headers: $headers,
-            server:  $_SERVER,
+            method:       $method,
+            path:         $path,
+            query:        $_GET,
+            post:         $_POST,
+            rawBody:      $raw,
+            headers:      $headers,
+            server:       $_SERVER,
+            bodyTooLarge: $bodyTooLarge,
         );
     }
 

tests/Controllers/SprintControllerTest.php

@@ -42,4 +42,16 @@ final class SprintControllerTest extends TestCase
         $r->setAccessible(true);
         return $r->invoke(null, ...$args);
     }
+
+    /**
+     * R01-N24: the per-batch list cap is a drift-sensitive number — bumping
+     * it has security implications (memory + DB pressure under attacker
+     * load) and operator implications (existing tooling that batches near
+     * the cap may break). A future refactor that tweaks it must update
+     * REVIEW_01.md §R01-N24 in the same commit.
+     */
+    public function testMaxBatchItemsConstant(): void
+    {
+        $this->assertSame(5000, SprintController::MAX_BATCH_ITEMS);
+    }
 }

tests/Controllers/TaskControllerTest.php

@@ -0,0 +1,27 @@
+<?php
+
+declare(strict_types=1);
+
+namespace App\Tests\Controllers;
+
+use App\Controllers\TaskController;
+use PHPUnit\Framework\TestCase;
+
+/**
+ * R01-N24 drift fence: TaskController shares the SprintController batch
+ * cap. A bump in the constant has memory / DB-pressure implications under
+ * attacker load — REVIEW_01.md §R01-N24 must move with it.
+ */
+final class TaskControllerTest extends TestCase
+{
+    public function testMaxBatchItemsConstantMatchesSprintController(): void
+    {
+        $this->assertSame(5000, TaskController::MAX_BATCH_ITEMS);
+        // The two controllers cap independently but the audit suggested
+        // a single bound; assert they have not drifted apart.
+        $this->assertSame(
+            \App\Controllers\SprintController::MAX_BATCH_ITEMS,
+            TaskController::MAX_BATCH_ITEMS,
+        );
+    }
+}

tests/Http/RequestTest.php

@@ -90,4 +90,62 @@ final class RequestTest extends TestCase
         $req = $this->makeRequest(['HTTPS' => 'off']);
         self::assertFalse($req->isHttps());
     }
+
+    // ------------------------------------------------------------------
+    // R01-N24: body size cap
+    // ------------------------------------------------------------------
+
+    public function testMaxBodyBytesCapIsExactlyOneMebibyte(): void
+    {
+        // Drift fence — bumping this changes a published HTTP contract
+        // (clients that rely on the 413 boundary). Update REVIEW_01.md
+        // §R01-N24, public/index.php's error message, and any per-cap
+        // operator docs alongside the value here.
+        self::assertSame(1024 * 1024, Request::MAX_BODY_BYTES);
+    }
+
+    public function testBodyTooLargeFlagDefaultsToFalse(): void
+    {
+        $req = $this->makeRequest([]);
+        self::assertFalse($req->bodyTooLarge);
+    }
+
+    public function testBodyTooLargeFlagWiresThroughConstructor(): void
+    {
+        // The front controller (`public/index.php`) reads this property to
+        // emit a 413 before dispatch. A future refactor must not lose the
+        // wiring or the cap silently disappears.
+        $req = new Request(
+            method:       'POST',
+            path:         '/sprints/1/week-cells',
+            query:        [],
+            post:         [],
+            rawBody:      '',
+            headers:      [],
+            server:       [],
+            bodyTooLarge: true,
+        );
+        self::assertTrue($req->bodyTooLarge);
+    }
+
+    public function testJsonReturnsNullWhenBodyWasOversized(): void
+    {
+        // `fromGlobals()` blanks `rawBody` once it decides the request was
+        // oversized, so the existing `json()` parser naturally returns null.
+        // Pin that downstream-safety contract — controllers that fall back
+        // to `?? []` continue to work; the front-controller 413 has
+        // already replied so this branch should never run in production,
+        // but defending against a misuse path is cheap.
+        $req = new Request(
+            method:       'POST',
+            path:         '/x',
+            query:        [],
+            post:         [],
+            rawBody:      '',
+            headers:      ['content-type' => 'application/json'],
+            server:       [],
+            bodyTooLarge: true,
+        );
+        self::assertNull($req->json());
+    }
 }