Просмотр исходного кода

Fix R01-N19: CSP report-uri + audit endpoint

The strict CSP in `App\Http\FatalErrorHandler::CSP` was silent — operators
wouldn't notice a future view inadvertently introducing an inline handler
or a new external host until users complained. The CSP now carries
`report-uri /csp-report`, and the new public `POST /csp-report` route
(`App\Controllers\CspReportController`) writes one audit row per
browser-fired report.

Anonymous: no session, no CSRF — this is a browser mechanism, not a user
action; a forged report only costs one audit row, bounded by the 16 KiB
body cap (`MAX_BODY_BYTES`). Bodies that fail to decode as a JSON object,
or that exceed the cap, get a 204 with no row written so the endpoint
reveals nothing. The legacy `{"csp-report": {...}}` envelope is unwrapped
before the inner object lands in `after_json`.

We did not add `report-to` (CSP Level 3) — it would need a
`Reporting-Endpoints` response header on every page and a second JSON
parser for the `application/reports+json` shape, neither cheap, and
every current browser still honours `report-uri`.

Audit row: `action=CSP_VIOLATION, entity_type=csp_violation,
entity_id=NULL, after_json=<inner report>, ip/ua=<browser>`. The
`/audit` view's filter dropdown already feeds from
`AuditRepository::distinctEntityTypes()` so `csp_violation` shows up
there automatically.

Tests: 266 / 721 (was 252 / 683). New
`tests/Controllers/CspReportControllerTest.php` (10 cases) pins the
204-always rule, the body cap, the JSON-object-only contract, the
envelope unwrap, and the audit-row shape end-to-end against an
in-memory DB. `FatalErrorHandlerTest` CSP-line assertion grew a
`report-uri` check (drift fence).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
chiappa 2 дней назад
Родитель
Сommit
f59f368765

+ 39 - 4
SPEC.md

@@ -205,6 +205,7 @@ Pages (HTML):
 |--------|-----------------------------|----------------|
 | GET    | `/`                         | any (anon → sign-in CTA) |
 | GET    | `/healthz`                  | —              |
+| POST   | `/csp-report`               | — (browser-fired, R01-N19 — no CSRF; 16 KiB body cap; audits as `CSP_VIOLATION`/`csp_violation`; always 204) |
 | GET    | `/auth/login`               | —              |
 | GET    | `/auth/callback`            | —              |
 | GET    | `/auth/local`               | — (404 if disabled) |
@@ -268,12 +269,19 @@ is called inside the same transaction as the DB change. Controllers prefer
     - `TaskController::delete()` — task → task_assignments
     - `SprintController::removeWorker()` — sprint_worker → sprint_worker_days + task_assignments
     - `SprintController::replaceWeeks()` — sprint_week → sprint_worker_days (on shrink)
-- Non-mutation events (LOGIN, LOGOUT, LOGIN_FAILED, BOOTSTRAP_ADMIN) → always
-  one row. R01-N06 adds a second `LOGIN_FAILED` reason on the local-admin
-  path: `local_admin_throttled_until_<iso>` is written when the
+- Non-mutation events (LOGIN, LOGOUT, LOGIN_FAILED, BOOTSTRAP_ADMIN,
+  IMPORT_PREVIEW_ABANDONED, CSP_VIOLATION) → always one row. R01-N06
+  adds a second `LOGIN_FAILED` reason on the local-admin path:
+  `local_admin_throttled_until_<iso>` is written when the
   `(ip, email)` bucket is currently locked, separate from the existing
   `local_admin_credential_mismatch` row written when the password
-  itself was wrong.
+  itself was wrong. R01-N19 adds `CSP_VIOLATION` /
+  `entity_type=csp_violation` rows: the browser POSTs the JSON report
+  body to `/csp-report`, the controller unwraps the standard
+  `{"csp-report": {...}}` envelope, and the inner object lands in
+  `after_json`. `entity_id` is null; `user_id` / `user_email` are null
+  (browser reports rarely carry session context); IP / UA come from
+  the request. Body cap: 16 KiB.
 
 ## 8. Env (.env.example)
 
@@ -1467,6 +1475,33 @@ is gone — see `src/Auth/BootstrapAdmin.php`.
       Tests: 227 / 590 (was 211 / 562). Eleventh fix from
       `doc/REVIEW_01.md`.
 
+- [x] **R01-N19 — CSP `report-uri /csp-report` + audit endpoint**
+      The strict CSP in `App\Http\FatalErrorHandler::CSP` was silent —
+      operators wouldn't notice a future view inadvertently introducing
+      an inline handler or a new external host until users complained.
+      The CSP now carries `report-uri /csp-report`, and the new public
+      `POST /csp-report` route (`App\Controllers\CspReportController`)
+      writes one audit row per browser-fired report. Anonymous: no
+      session, no CSRF — this is a browser mechanism, not a user
+      action; a forged report only costs one audit row, bounded by the
+      16 KiB body cap (`MAX_BODY_BYTES`). Bodies that fail to decode as
+      a JSON object, or that exceed the cap, get a 204 with no row
+      written so the endpoint reveals nothing. The legacy
+      `{"csp-report": {...}}` envelope is unwrapped before the inner
+      object lands in `after_json`. We are not adding `report-to` (CSP
+      Level 3) until the legacy form proves insufficient — `report-to`
+      requires a `Reporting-Endpoints` response header on every page
+      and a second JSON parser for the `application/reports+json`
+      shape, neither cheap. The audit listing surfaces the new
+      `csp_violation` entity-type filter automatically (the `/audit`
+      view's filter dropdown is wired through
+      `AuditRepository::distinctEntityTypes()`). The
+      `FatalErrorHandlerTest` CSP-line assertion grew a `report-uri`
+      check to fence drift on edits to either the directive or the
+      route. New `tests/Controllers/CspReportControllerTest.php` (10
+      cases). Tests: 266 / 721 (was 252 / 683). Sixteenth fix from
+      `doc/REVIEW_01.md`.
+
 - [x] **New sprint form: drop weeks input + task list row hover**
       (`3728106`). The `/sprints/new` form no longer collects an
       `n_weeks` value — the week count is derived from `start_date` /

+ 7 - 0
public/index.php

@@ -7,6 +7,7 @@ use App\Auth\OidcClient;
 use App\Auth\SessionGuard;
 use App\Controllers\AuditController;
 use App\Controllers\AuthController;
+use App\Controllers\CspReportController;
 use App\Controllers\ImportController;
 use App\Controllers\SettingsController;
 use App\Controllers\SprintController;
@@ -137,6 +138,7 @@ $importCommit   = new SprintImporter(
 $importCtrl     = new ImportController(
     $pdo, $users, $sprints, $xlsxParser, $importCommit, $view, $audit,
 );
+$cspReportCtrl  = new CspReportController($audit);
 
 // ---------------------------------------------------------------------------
 // Routing
@@ -168,6 +170,11 @@ $router->get('/', function (Request $req) use ($view, $pdo, $users, $sprints, $a
 
 $router->get('/healthz', fn() => Response::text('ok'));
 
+// R01-N19: browser-fired CSP violation reports. Public POST (no auth, no
+// CSRF — see CspReportController). Body capped at 16 KiB; one audit row
+// per accepted report.
+$router->post('/csp-report', $cspReportCtrl->report(...));
+
 $router->get('/auth/login',     $auth->login(...));
 $router->get('/auth/callback',  $auth->callback(...));
 $router->post('/auth/logout',   $auth->logout(...));

+ 104 - 0
src/Controllers/CspReportController.php

@@ -0,0 +1,104 @@
+<?php
+
+declare(strict_types=1);
+
+namespace App\Controllers;
+
+use App\Http\Request;
+use App\Http\Response;
+use App\Services\AuditLogger;
+
+/**
+ * R01-N19: thin endpoint for browser-initiated CSP violation reports.
+ *
+ * The strict CSP in `App\Http\FatalErrorHandler::CSP` carries a
+ * `report-uri /csp-report` directive, so any browser that blocks an
+ * inline-style / inline-script / off-host fetch sends a JSON POST here.
+ * The report shape is the legacy `report-uri` envelope:
+ *
+ *     POST /csp-report  Content-Type: application/csp-report
+ *     {"csp-report": {"document-uri":"...","violated-directive":"...", ...}}
+ *
+ * We unwrap the inner `csp-report` (if present) and write a single audit
+ * row with `action=CSP_VIOLATION`, `entity_type=csp_violation`. The audit
+ * row's `after_json` is the report; `entity_id` is null. Always 204.
+ *
+ * Anonymous: the endpoint does NOT call any `SessionGuard` gate because
+ * the browser fires reports without the session cookie context most of
+ * the time. CSRF is intentionally NOT enforced — this is a browser
+ * mechanism, not a user action; a forged report only costs one audit row,
+ * bounded by the body cap below.
+ *
+ * Body cap: 16 KiB. Real CSP reports are well under 1 KiB; the cap is
+ * generous enough that legitimate reports always fit, tight enough that
+ * a hostile client cannot pump megabytes of garbage into `audit_log`.
+ */
+final class CspReportController
+{
+    public const MAX_BODY_BYTES = 16 * 1024;
+
+    public function __construct(private readonly AuditLogger $audit)
+    {
+    }
+
+    public function report(Request $req): Response
+    {
+        // Always answer 204 — a browser sending a report doesn't expect
+        // anything actionable back, and we don't want to leak whether
+        // the report was accepted vs. ignored.
+        $reply = Response::text('', 204);
+
+        if (strlen($req->rawBody) === 0 || strlen($req->rawBody) > self::MAX_BODY_BYTES) {
+            return $reply;
+        }
+
+        $report = self::extractReport($req->rawBody);
+        if ($report === null) {
+            return $reply;
+        }
+
+        $this->audit->record(
+            action:     'CSP_VIOLATION',
+            entityType: 'csp_violation',
+            entityId:   null,
+            before:     null,
+            after:      $report,
+            userId:     null,
+            userEmail:  null,
+            ipAddress:  $req->ip(),
+            userAgent:  $req->userAgent(),
+        );
+
+        return $reply;
+    }
+
+    /**
+     * Decode the request body and return the inner CSP report payload.
+     * Returns null if the body is not a JSON object or is structurally
+     * unusable. Accepts both the bare `{...}` and the standard
+     * `{"csp-report": {...}}` envelopes; in the latter case the inner
+     * object is what gets recorded.
+     *
+     * @return array<string,mixed>|null
+     */
+    public static function extractReport(string $rawBody): ?array
+    {
+        try {
+            /** @var mixed $decoded */
+            $decoded = json_decode($rawBody, true, 8, JSON_THROW_ON_ERROR);
+        } catch (\JsonException) {
+            return null;
+        }
+        // Top-level JSON must be an object (assoc array). A bare list
+        // `[…]` decodes to a list-shaped array, which is not what any
+        // CSP report shape uses.
+        if (!is_array($decoded) || array_is_list($decoded)) {
+            return null;
+        }
+
+        if (isset($decoded['csp-report']) && is_array($decoded['csp-report'])) {
+            return $decoded['csp-report'];
+        }
+        return $decoded;
+    }
+}

+ 10 - 1
src/Http/FatalErrorHandler.php

@@ -39,6 +39,14 @@ final class FatalErrorHandler
      * 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.
+     *
+     * R01-N19: `report-uri /csp-report` routes browser-side violations
+     * to `App\Controllers\CspReportController`, which audits each one as
+     * `CSP_VIOLATION` / `csp_violation`. Report-uri is the legacy
+     * directive but still honoured by every current browser; we are not
+     * adding `report-to` (which would need a `Reporting-Endpoints`
+     * header on every response and a second JSON parser path) until
+     * the legacy form proves insufficient.
      */
     public const CSP =
         "default-src 'self'; "
@@ -49,7 +57,8 @@ final class FatalErrorHandler
         . "connect-src 'self'; "
         . "frame-ancestors 'none'; "
         . "base-uri 'self'; "
-        . "form-action 'self' https://login.microsoftonline.com";
+        . "form-action 'self' https://login.microsoftonline.com; "
+        . "report-uri /csp-report";
 
     /**
      * Bit mask for "fatal in the sense that the request can no longer

+ 233 - 0
tests/Controllers/CspReportControllerTest.php

@@ -0,0 +1,233 @@
+<?php
+
+declare(strict_types=1);
+
+namespace App\Tests\Controllers;
+
+use App\Controllers\CspReportController;
+use App\Http\Request;
+use App\Services\AuditLogger;
+use App\Tests\TestCase;
+use PHPUnit\Framework\Attributes\DataProvider;
+use ReflectionClass;
+
+/**
+ * R01-N19: pin the contract of CspReportController.
+ *
+ * The controller has two halves:
+ *
+ *   - `extractReport()` — pure decoder: bytes → array | null. Easy to
+ *     pin with a data-providery sweep over good and adversarial inputs.
+ *   - `report()` — IO half: takes a Request, writes one audit row, hands
+ *     back a 204. Validated end-to-end against a real (in-memory) DB so
+ *     we catch shape drift in the audit insert at the same time.
+ */
+final class CspReportControllerTest extends TestCase
+{
+    public function testExtractReportUnwrapsCspReportEnvelope(): void
+    {
+        $body = json_encode(['csp-report' => [
+            'document-uri'      => 'https://app.example.com/sprints/1',
+            'violated-directive' => "script-src 'self'",
+            'blocked-uri'       => 'https://attacker.example/x.js',
+        ]], JSON_THROW_ON_ERROR);
+
+        $out = CspReportController::extractReport($body);
+
+        self::assertIsArray($out);
+        self::assertSame('https://app.example.com/sprints/1', $out['document-uri']);
+        self::assertSame("script-src 'self'", $out['violated-directive']);
+        self::assertArrayNotHasKey('csp-report', $out, 'inner object is unwrapped, not nested');
+    }
+
+    public function testExtractReportAcceptsBareJsonObject(): void
+    {
+        $body = '{"document-uri":"https://x.example/","violated-directive":"img-src"}';
+        $out  = CspReportController::extractReport($body);
+
+        self::assertIsArray($out);
+        self::assertSame('https://x.example/', $out['document-uri']);
+    }
+
+    /**
+     * @return list<array{0:string,1:string}>
+     */
+    public static function malformedBodies(): array
+    {
+        return [
+            ['',                                             'empty body'],
+            ['not json at all',                              'free text'],
+            ['"a string, not an object"',                    'JSON string scalar'],
+            ['42',                                           'JSON number scalar'],
+            ['null',                                         'JSON null'],
+            ['[1,2,3]',                                      'JSON array (top-level non-object)'],
+            ['{"csp-report":"not an object"}',               'csp-report not an array → fall through to outer object'],
+        ];
+    }
+
+    #[DataProvider('malformedBodies')]
+    public function testExtractReportRejectsNonObjectAndMalformedBodies(string $body, string $label): void
+    {
+        $out = CspReportController::extractReport($body);
+
+        // The csp-report-not-an-object case falls through to the outer object,
+        // which IS an object — so we accept that one as a non-null array.
+        if ($body === '{"csp-report":"not an object"}') {
+            self::assertIsArray($out, "{$label}: outer object survives unwrap miss");
+            self::assertSame('not an object', $out['csp-report']);
+            return;
+        }
+
+        self::assertNull($out, "{$label}: should be unparseable");
+    }
+
+    public function testReportWritesAuditRowAnd204(): void
+    {
+        $pdo    = $this->makeDb();
+        $audit  = new AuditLogger($pdo);
+        $ctrl   = new CspReportController($audit);
+
+        $report = json_encode(['csp-report' => [
+            'document-uri'        => 'https://app.example.com/audit',
+            'referrer'            => '',
+            'violated-directive'  => "script-src 'self'",
+            'effective-directive' => "script-src",
+            'original-policy'     => "default-src 'self'; report-uri /csp-report",
+            'disposition'         => 'enforce',
+            'blocked-uri'         => 'inline',
+            'line-number'         => 12,
+            'source-file'         => 'https://app.example.com/audit',
+            'status-code'         => 200,
+            'script-sample'       => '',
+        ]], JSON_THROW_ON_ERROR);
+
+        $req = $this->makeRequest('POST', '/csp-report', $report, [
+            'content-type' => 'application/csp-report',
+            'user-agent'   => 'Mozilla/5.0 (TestRunner)',
+        ], '203.0.113.10');
+
+        $resp = $ctrl->report($req);
+
+        self::assertSame(204, $resp->status);
+        self::assertSame('', $resp->body);
+
+        $rows = $pdo->query(
+            'SELECT action, entity_type, entity_id, before_json, after_json,
+                    user_id, user_email, ip_address, user_agent
+               FROM audit_log'
+        )->fetchAll();
+
+        self::assertCount(1, $rows, 'exactly one audit row written');
+        $row = $rows[0];
+        self::assertSame('CSP_VIOLATION', $row['action']);
+        self::assertSame('csp_violation', $row['entity_type']);
+        self::assertNull($row['entity_id']);
+        self::assertNull($row['before_json']);
+        self::assertNull($row['user_id']);
+        self::assertNull($row['user_email']);
+        self::assertSame('203.0.113.10', $row['ip_address']);
+        self::assertSame('Mozilla/5.0 (TestRunner)', $row['user_agent']);
+
+        $after = json_decode((string) $row['after_json'], true);
+        self::assertIsArray($after);
+        self::assertSame('https://app.example.com/audit', $after['document-uri']);
+        self::assertSame("script-src 'self'", $after['violated-directive']);
+        self::assertArrayNotHasKey('csp-report', $after);
+    }
+
+    public function testReportWithEmptyBodyWritesNoAuditRow(): void
+    {
+        $pdo   = $this->makeDb();
+        $audit = new AuditLogger($pdo);
+        $ctrl  = new CspReportController($audit);
+
+        $req  = $this->makeRequest('POST', '/csp-report', '', [], '198.51.100.1');
+        $resp = $ctrl->report($req);
+
+        self::assertSame(204, $resp->status);
+        self::assertSame(
+            0,
+            (int) $pdo->query('SELECT COUNT(*) FROM audit_log')->fetchColumn(),
+            'empty body → no row',
+        );
+    }
+
+    public function testReportRejectsBodyOverCap(): void
+    {
+        $pdo   = $this->makeDb();
+        $audit = new AuditLogger($pdo);
+        $ctrl  = new CspReportController($audit);
+
+        // Build a JSON object whose serialized form exceeds the 16 KiB cap.
+        $padding = str_repeat('a', CspReportController::MAX_BODY_BYTES + 1);
+        $body    = json_encode(['csp-report' => ['document-uri' => $padding]], JSON_THROW_ON_ERROR);
+        self::assertGreaterThan(CspReportController::MAX_BODY_BYTES, strlen($body));
+
+        $req  = $this->makeRequest('POST', '/csp-report', $body, [
+            'content-type' => 'application/csp-report',
+        ], '198.51.100.2');
+        $resp = $ctrl->report($req);
+
+        self::assertSame(204, $resp->status, 'still 204 — endpoint reveals nothing');
+        self::assertSame(
+            0,
+            (int) $pdo->query('SELECT COUNT(*) FROM audit_log')->fetchColumn(),
+            'oversized body → no row',
+        );
+    }
+
+    public function testReportRejectsNonJsonBody(): void
+    {
+        $pdo   = $this->makeDb();
+        $audit = new AuditLogger($pdo);
+        $ctrl  = new CspReportController($audit);
+
+        $req  = $this->makeRequest('POST', '/csp-report', 'plain text junk', [
+            'content-type' => 'text/plain',
+        ], '198.51.100.3');
+        $resp = $ctrl->report($req);
+
+        self::assertSame(204, $resp->status);
+        self::assertSame(
+            0,
+            (int) $pdo->query('SELECT COUNT(*) FROM audit_log')->fetchColumn(),
+        );
+    }
+
+    public function testMaxBodyBytesIsExactly16KiB(): void
+    {
+        // Drift fence: a future bump must update REVIEW_01.md and the
+        // controller phpdoc together with the constant.
+        self::assertSame(16 * 1024, CspReportController::MAX_BODY_BYTES);
+    }
+
+    /**
+     * @param array<string,string> $headers lower-cased names
+     */
+    private function makeRequest(
+        string $method,
+        string $path,
+        string $rawBody,
+        array $headers = [],
+        string $remoteAddr = '127.0.0.1',
+    ): Request {
+        // The Request constructor is `public readonly` — we hand-build it
+        // via reflection so we don't have to mutate $_SERVER / $_POST.
+        $server = [
+            'REMOTE_ADDR'    => $remoteAddr,
+            'REQUEST_METHOD' => $method,
+            'REQUEST_URI'    => $path,
+        ];
+
+        $r = (new ReflectionClass(Request::class))->newInstanceArgs([
+            'method'  => $method,
+            'path'    => $path,
+            'query'   => [],
+            'post'    => [],
+            'rawBody' => $rawBody,
+            'headers' => $headers,
+            'server'  => $server,
+        ]);
+        return $r;
+    }
+}

+ 5 - 0
tests/Http/FatalErrorHandlerTest.php

@@ -65,6 +65,11 @@ final class FatalErrorHandlerTest extends TestCase
             $cspLine,
             'OIDC redirect target must remain reachable from the error page',
         );
+        self::assertStringContainsString(
+            'report-uri /csp-report',
+            $cspLine,
+            'R01-N19: browsers must know where to send CSP violation reports',
+        );
 
         self::assertStringContainsString('500 — Server error', $this->body);
         self::assertStringNotContainsString(