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

Fix R01-N11: whitelist column in AuditRepository::distinctColumn

The private helper interpolated $col into SQL
("SELECT DISTINCT {$col} FROM audit_log ORDER BY {$col} ASC"). Today's
two callers both pass literal strings so it was non-exploitable, but the
contract was implicit — any future caller wiring user input through the
helper would hand over a SQL-injection vector.

Add an explicit `in_array($col, ['action', 'entity_type'], true)` guard
at the top of distinctColumn(); throw InvalidArgumentException otherwise.
Cheap and ends the foot-gun. New tests/Repositories/AuditRepositoryTest
covers the happy paths for distinctActions / distinctEntityTypes plus
two reflection-based regression guards (rejects an unsupported known
column and rejects a SQL-injection attempt).

Tests: 163 / 417 (was 159 / 413).

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

+ 14 - 1
src/Repositories/AuditRepository.php

@@ -105,9 +105,22 @@ final class AuditRepository
         return $out;
     }
 
-    /** @return list<string> */
+    /**
+     * @param 'action'|'entity_type' $col
+     * @return list<string>
+     */
     private function distinctColumn(string $col): array
     {
+        // Whitelist guard: $col is interpolated into SQL below, so it must
+        // never come from user input. Both internal callers pass a literal,
+        // but enforce the contract at runtime so a future refactor can't
+        // turn this into an injection vector (R01-N11).
+        if (!in_array($col, ['action', 'entity_type'], true)) {
+            throw new \InvalidArgumentException(
+                "AuditRepository::distinctColumn: unsupported column '{$col}'"
+            );
+        }
+
         $stmt = $this->pdo->query(
             "SELECT DISTINCT {$col} FROM audit_log ORDER BY {$col} ASC"
         );

+ 67 - 0
tests/Repositories/AuditRepositoryTest.php

@@ -0,0 +1,67 @@
+<?php
+
+declare(strict_types=1);
+
+namespace App\Tests\Repositories;
+
+use App\Repositories\AuditRepository;
+use App\Services\AuditLogger;
+use App\Tests\TestCase;
+use InvalidArgumentException;
+
+/**
+ * R01-N11: AuditRepository::distinctColumn interpolates its argument into
+ * SQL. The method is private and both internal callers pass literals, but
+ * the runtime whitelist guard makes the contract explicit so a future
+ * refactor can't open an injection vector.
+ */
+final class AuditRepositoryTest extends TestCase
+{
+    public function testDistinctActionsReturnsSortedUniqueValues(): void
+    {
+        $pdo    = $this->makeDb();
+        $logger = new AuditLogger($pdo);
+        $logger->record('UPDATE', 'worker', 1, ['n' => 1], ['n' => 2]);
+        $logger->record('CREATE', 'worker', 2, null, ['n' => 1]);
+        $logger->record('UPDATE', 'sprint', 3, ['n' => 1], ['n' => 2]);
+
+        $repo = new AuditRepository($pdo);
+        $this->assertSame(['CREATE', 'UPDATE'], $repo->distinctActions());
+    }
+
+    public function testDistinctEntityTypesReturnsSortedUniqueValues(): void
+    {
+        $pdo    = $this->makeDb();
+        $logger = new AuditLogger($pdo);
+        $logger->record('UPDATE', 'worker', 1, ['n' => 1], ['n' => 2]);
+        $logger->record('UPDATE', 'sprint', 2, ['n' => 1], ['n' => 2]);
+        $logger->record('UPDATE', 'worker', 3, ['n' => 1], ['n' => 2]);
+
+        $repo = new AuditRepository($pdo);
+        $this->assertSame(['sprint', 'worker'], $repo->distinctEntityTypes());
+    }
+
+    public function testDistinctColumnRejectsUnknownColumnViaReflection(): void
+    {
+        $pdo  = $this->makeDb();
+        $repo = new AuditRepository($pdo);
+
+        $rm = new \ReflectionMethod($repo, 'distinctColumn');
+        $rm->setAccessible(true);
+
+        $this->expectException(InvalidArgumentException::class);
+        $rm->invoke($repo, 'user_email');
+    }
+
+    public function testDistinctColumnRejectsInjectionAttemptViaReflection(): void
+    {
+        $pdo  = $this->makeDb();
+        $repo = new AuditRepository($pdo);
+
+        $rm = new \ReflectionMethod($repo, 'distinctColumn');
+        $rm->setAccessible(true);
+
+        $this->expectException(InvalidArgumentException::class);
+        $rm->invoke($repo, 'action; DROP TABLE audit_log; --');
+    }
+}