1
0
Эх сурвалжийг харах

Fix R01-N12: validate audit date filters server-side

The /audit viewer concatenated `T00:00:00Z` / `T23:59:59Z` onto the raw
`from_date` / `to_date` query inputs and bound the result against the
ISO-8601 `occurred_at` column. Garbage in (e.g. `2024/01/01` instead of
`2024-01-01`) silently produced empty result sets — a UX bug and a way
to hide events from a casual auditor.

`AuditController::index` now routes both inputs through a new pure
static `validateDateFilters($from, $to)` helper that strict-parses
`Y-m-d` (createFromFormat + round-trip equality, same pattern as
`SprintController::isIsoDate`) and returns:
  - the value for the repo (empty string when the input fails to parse,
    so the filter is dropped instead of poisoning the query)
  - a `from`/`to` error map keyed by field name

The view echoes the user's raw input back into the date fields so they
can fix their typo, tints the offending input amber, and shows an inline
"Use the format YYYY-MM-DD" notice plus a banner. No flash session
plumbing — the form is GET-driven and the response is the form, so
errors travel with the rendered page.

Tests: new `tests/Controllers/AuditControllerTest.php` (16 cases) pins
the matrix — empty / valid / one-side-bad / both-bad, lenient rollovers
caught by the round-trip check (`2025-02-29`, `2026-13-01`, `2026-02-30`,
`2026-1-1`), whitespace rejected, leap-year preserved verbatim, and an
injection-shaped string rejected. Suite: 227 / 590 (was 211 / 562).

Closes R01-N12 from `doc/REVIEW_01.md`.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
chiappa 2 өдөр өмнө
parent
commit
1b28469a4e

+ 56 - 4
src/Controllers/AuditController.php

@@ -10,6 +10,7 @@ use App\Http\Response;
 use App\Http\View;
 use App\Repositories\AuditRepository;
 use App\Repositories\UserRepository;
+use DateTimeImmutable;
 
 /**
  * /audit — admin-only read-only viewer.
@@ -36,23 +37,34 @@ final class AuditController
             return $actor;
         }
 
+        $rawFrom = $req->queryString('from_date');
+        $rawTo   = $req->queryString('to_date');
+        $dates   = self::validateDateFilters($rawFrom, $rawTo);
+
+        // Form inputs echo what the user typed (so they can fix a typo);
+        // the repo only sees the values that parsed cleanly as Y-m-d.
         $filters = [
             'user_email'  => $req->queryString('user_email'),
             'action'      => $req->queryString('action'),
             'entity_type' => $req->queryString('entity_type'),
             'entity_id'   => $req->queryString('entity_id'),
-            'from_date'   => $req->queryString('from_date'),
-            'to_date'     => $req->queryString('to_date'),
+            'from_date'   => $rawFrom,
+            'to_date'     => $rawTo,
         ];
 
-        $page = (int) ($req->queryString('page') ?: '1');
-        $result = $this->audit->findPaged($filters, $page, self::PAGE_SIZE);
+        $effectiveFilters = $filters;
+        $effectiveFilters['from_date'] = $dates['from'];
+        $effectiveFilters['to_date']   = $dates['to'];
+
+        $page   = (int) ($req->queryString('page') ?: '1');
+        $result = $this->audit->findPaged($effectiveFilters, $page, self::PAGE_SIZE);
 
         return Response::html($this->view->render('audit/index', [
             'title'       => 'Audit log',
             'currentUser' => $actor,
             'csrfToken'   => SessionGuard::csrfToken(),
             'filters'     => $filters,
+            'dateErrors'  => $dates['errors'],
             'page'        => $result['page'],
             'pages'       => $result['pages'],
             'pageSize'    => $result['pageSize'],
@@ -63,4 +75,44 @@ final class AuditController
             'users'       => $this->audit->distinctUserEmails(),
         ]));
     }
+
+    /**
+     * Pure helper: parse the raw `from_date` / `to_date` query inputs,
+     * drop anything that is not a strict `Y-m-d`, and report which fields
+     * the user got wrong. The repo concatenates `T00:00:00Z` / `T23:59:59Z`
+     * onto whatever it gets, so any garbage that reaches it just sorts
+     * lexically against `occurred_at` and silently hides rows (R01-N12).
+     *
+     * @return array{from: string, to: string, errors: array<string,string>}
+     */
+    public static function validateDateFilters(string $from, string $to): array
+    {
+        $errors = [];
+
+        $effFrom = '';
+        if ($from !== '') {
+            if (self::isIsoDate($from)) {
+                $effFrom = $from;
+            } else {
+                $errors['from_date'] = 'Use the format YYYY-MM-DD.';
+            }
+        }
+
+        $effTo = '';
+        if ($to !== '') {
+            if (self::isIsoDate($to)) {
+                $effTo = $to;
+            } else {
+                $errors['to_date'] = 'Use the format YYYY-MM-DD.';
+            }
+        }
+
+        return ['from' => $effFrom, 'to' => $effTo, 'errors' => $errors];
+    }
+
+    private static function isIsoDate(string $s): bool
+    {
+        $d = DateTimeImmutable::createFromFormat('Y-m-d', $s);
+        return $d !== false && $d->format('Y-m-d') === $s;
+    }
 }

+ 91 - 0
tests/Controllers/AuditControllerTest.php

@@ -0,0 +1,91 @@
+<?php
+
+declare(strict_types=1);
+
+namespace App\Tests\Controllers;
+
+use App\Controllers\AuditController;
+use PHPUnit\Framework\Attributes\DataProvider;
+use PHPUnit\Framework\TestCase;
+
+/**
+ * R01-N12: pin AuditController::validateDateFilters. The repo concatenates
+ * "T00:00:00Z" / "T23:59:59Z" onto whatever string it gets and binds the
+ * result against the ISO-8601 `occurred_at` column — garbage in silently
+ * hides rows. The controller is the gate; this test pins which inputs
+ * pass through and which get dropped + flagged.
+ */
+final class AuditControllerTest extends TestCase
+{
+    /**
+     * @return list<array{string,string,string,string,list<string>,string}>
+     */
+    public static function dateCases(): array
+    {
+        // [from, to, expectedFrom, expectedTo, expectedErrorKeys, label]
+        return [
+            ['',           '',           '',           '',           [],                          'no filters → no errors'],
+            ['2026-01-31', '',           '2026-01-31', '',           [],                          'from-only valid'],
+            ['',           '2026-12-31', '',           '2026-12-31', [],                          'to-only valid'],
+            ['2026-01-01', '2026-12-31', '2026-01-01', '2026-12-31', [],                          'both valid'],
+            ['2024/01/01', '',           '',           '',           ['from_date'],               'slashes → from dropped, error reported'],
+            ['',           '2024/01/01', '',           '',           ['to_date'],                 'slashes → to dropped, error reported'],
+            ['2024/01/01', '2024/12/31', '',           '',           ['from_date', 'to_date'],    'both garbage → both dropped, both errors'],
+            ['2026-01-31', 'tomorrow',   '2026-01-31', '',           ['to_date'],                 'one-side invalid → only that side errors, the other passes'],
+            ['2026-13-01', '',           '',           '',           ['from_date'],               'impossible month → rejected'],
+            ['2026-02-30', '',           '',           '',           ['from_date'],               'impossible day for month → rejected (strict round-trip)'],
+            ['2026-1-1',   '',           '',           '',           ['from_date'],               'unpadded → rejected (strict round-trip)'],
+            [' 2026-01-01', '',          '',           '',           ['from_date'],               'leading space → rejected'],
+            ['2026-01-01 ', '',          '',           '',           ['from_date'],               'trailing space → rejected'],
+            ["'; DROP TABLE audit_log; --", '', '',    '',           ['from_date'],               'injection-shaped string → rejected'],
+        ];
+    }
+
+    /** @param list<string> $expectedErrorKeys */
+    #[DataProvider('dateCases')]
+    public function testValidateDateFilters(
+        string $from,
+        string $to,
+        string $expectedFrom,
+        string $expectedTo,
+        array  $expectedErrorKeys,
+        string $label,
+    ): void {
+        $out = AuditController::validateDateFilters($from, $to);
+
+        $this->assertSame($expectedFrom, $out['from'], "{$label}: from");
+        $this->assertSame($expectedTo,   $out['to'],   "{$label}: to");
+        $this->assertSame(
+            $expectedErrorKeys,
+            array_keys($out['errors']),
+            "{$label}: error keys",
+        );
+
+        // Error messages are non-empty when present (keeps the contract for
+        // the view, which renders {{ dateErrors.from_date }} verbatim).
+        foreach ($out['errors'] as $key => $msg) {
+            $this->assertNotSame('', $msg, "{$label}: error message for {$key} is non-empty");
+        }
+    }
+
+    public function testValidatorPreservesValidValuesVerbatim(): void
+    {
+        // Boundary that the repo concatenates onto: a leap day. Confirms
+        // we don't normalise or reformat — the repo gets exactly what the
+        // user typed (after gating).
+        $out = AuditController::validateDateFilters('2024-02-29', '2024-02-29');
+        $this->assertSame('2024-02-29', $out['from']);
+        $this->assertSame('2024-02-29', $out['to']);
+        $this->assertSame([], $out['errors']);
+    }
+
+    public function testValidatorRejectsNonLeapFebTwentyNine(): void
+    {
+        // 2025 is not a leap year. createFromFormat is lenient and silently
+        // rolls over to 2025-03-01; the round-trip equality check we use
+        // is what catches it.
+        $out = AuditController::validateDateFilters('2025-02-29', '');
+        $this->assertSame('', $out['from']);
+        $this->assertArrayHasKey('from_date', $out['errors']);
+    }
+}

+ 14 - 2
views/audit/index.twig

@@ -74,16 +74,28 @@
             <span class="text-xs text-slate-600 dark:text-slate-400">From date</span>
             <input name="from_date" type="date"
                    value="{{ filters.from_date }}"
-                   class="mt-1 block w-full rounded border border-slate-300 px-2 py-1.5 text-sm focus:outline-none focus:ring-2 focus:ring-slate-400 dark:bg-slate-800 dark:border-slate-600 dark:text-slate-100 dark:focus:ring-slate-500">
+                   class="mt-1 block w-full rounded border {{ dateErrors.from_date is defined ? 'border-amber-400 dark:border-amber-500' : 'border-slate-300 dark:border-slate-600' }} px-2 py-1.5 text-sm focus:outline-none focus:ring-2 focus:ring-slate-400 dark:bg-slate-800 dark:text-slate-100 dark:focus:ring-slate-500">
+            {% if dateErrors.from_date is defined %}
+                <span class="block mt-1 text-xs text-amber-700 dark:text-amber-300">{{ dateErrors.from_date }}</span>
+            {% endif %}
         </label>
 
         <label class="block">
             <span class="text-xs text-slate-600 dark:text-slate-400">To date</span>
             <input name="to_date" type="date"
                    value="{{ filters.to_date }}"
-                   class="mt-1 block w-full rounded border border-slate-300 px-2 py-1.5 text-sm focus:outline-none focus:ring-2 focus:ring-slate-400 dark:bg-slate-800 dark:border-slate-600 dark:text-slate-100 dark:focus:ring-slate-500">
+                   class="mt-1 block w-full rounded border {{ dateErrors.to_date is defined ? 'border-amber-400 dark:border-amber-500' : 'border-slate-300 dark:border-slate-600' }} px-2 py-1.5 text-sm focus:outline-none focus:ring-2 focus:ring-slate-400 dark:bg-slate-800 dark:text-slate-100 dark:focus:ring-slate-500">
+            {% if dateErrors.to_date is defined %}
+                <span class="block mt-1 text-xs text-amber-700 dark:text-amber-300">{{ dateErrors.to_date }}</span>
+            {% endif %}
         </label>
 
+        {% if dateErrors is not empty %}
+            <div class="md:col-span-6 -mt-1 rounded border border-amber-300 bg-amber-50 px-3 py-2 text-xs text-amber-800 dark:border-amber-600 dark:bg-amber-900/30 dark:text-amber-200">
+                Invalid date filter ignored. Use the YYYY-MM-DD format (e.g. 2026-01-31).
+            </div>
+        {% endif %}
+
         <div class="md:col-span-6 flex items-center justify-end gap-2">
             {% if anyFilter %}
                 <a href="/audit" class="text-sm text-slate-600 hover:underline dark:text-slate-400 dark:hover:text-slate-200">Clear</a>