diff --git a/cloud/src/Auth/AuthController.php b/cloud/src/Auth/AuthController.php index 7d464b9..4044f5b 100644 --- a/cloud/src/Auth/AuthController.php +++ b/cloud/src/Auth/AuthController.php @@ -3,7 +3,7 @@ declare(strict_types=1); namespace App\Auth; use App\Http\{Request, Response}; -use App\Security\Password; +use App\Security\{Password, Csrf}; use App\Repository\{UserRepository, AuditRepository, RateLimiterRepository}; final class AuthController @@ -28,6 +28,14 @@ final class AuthController return Response::json(['error' => 'rate_limited'], 429); } + // Login CSRF: the GET /login render seeds $_SESSION['csrf'] and embeds it as a hidden + // field; the POST must echo it back. Without this an attacker can silently log a victim + // into the attacker's account (session fixation via forged login). Checked before the + // user lookup so a forged request never triggers the (expensive) Argon2id verify. + if (!Csrf::check((string)$sess->get('csrf'), (string)$req->post('csrf'))) { + return Response::json(['error' => 'csrf'], 403); + } + $username = (string)$req->post('username'); $password = (string)$req->post('password'); $u = $this->users->byUsername($username); diff --git a/cloud/src/Dashboard/ZoneConfigController.php b/cloud/src/Dashboard/ZoneConfigController.php index 9eff256..54d34ab 100644 --- a/cloud/src/Dashboard/ZoneConfigController.php +++ b/cloud/src/Dashboard/ZoneConfigController.php @@ -41,13 +41,19 @@ final class ZoneConfigController if (!in_array($level, self::LEVELS, true) || !is_array($polygon) || !Polygon::validate($polygon)) { return Response::json(['error' => 'invalid_zone'], 400); } - $zoneId = $this->zones->save([ - 'zone_id' => $b['zone_id'] ?? null, - 'device_id' => (string)($b['device_id'] ?? ''), - 'camera_id' => (string)($b['camera_id'] ?? ''), - 'level' => $level, - 'polygon_json' => json_encode($polygon), - ]); + try { + $zoneId = $this->zones->save([ + 'zone_id' => $b['zone_id'] ?? null, + 'device_id' => (string)($b['device_id'] ?? ''), + 'camera_id' => (string)($b['camera_id'] ?? ''), + 'level' => $level, + 'polygon_json' => json_encode($polygon), + ]); + } catch (\RuntimeException $e) { + // Editing a zone with a device_id/camera_id that does not own it (or a zone deleted + // concurrently) is a CLIENT error, not a server fault — return 400, not a 500. + return Response::json(['error' => 'invalid_zone'], 400); + } $this->audit->append((string)$this->sess->get('username'), 'zone_save', "zone:$zoneId", [], ['level' => $level]); return Response::json(['zone_id' => $zoneId], 201); } diff --git a/cloud/tests/AuthControllerTest.php b/cloud/tests/AuthControllerTest.php index 8a39225..e21d132 100644 --- a/cloud/tests/AuthControllerTest.php +++ b/cloud/tests/AuthControllerTest.php @@ -15,12 +15,16 @@ final class AuthControllerTest extends TestCase 'role' => 'operator', 'site_scope' => null, 'failed_attempts' => 0, 'locked_until' => 0, ]]); } + private function sess(): ArraySessionStore { + // Model the real flow: GET /login already seeded the session csrf token before POST. + $s = new ArraySessionStore(); $s->set('csrf', 'tok'); return $s; + } private function req(string $u, string $p): Request { - return new Request('POST', '/login', [], '', [], ['username' => $u, 'password' => $p]); + return new Request('POST', '/login', [], '', [], ['username' => $u, 'password' => $p, 'csrf' => 'tok']); } public function testLoginSuccessSetsSession(): void { $c = new AuthController($this->users(), new InMemoryAuditRepository(), new InMemoryRateLimiter()); - $sess = new ArraySessionStore(); + $sess = $this->sess(); $res = $c->login($this->req('alice', 'pw'), $sess, now: 1000, clientIp: '1.2.3.4'); $this->assertSame(200, $res->status()); $this->assertSame(1, $sess->get('user_id')); @@ -28,39 +32,39 @@ final class AuthControllerTest extends TestCase } public function testBadPasswordIs401(): void { $c = new AuthController($this->users(), new InMemoryAuditRepository(), new InMemoryRateLimiter()); - $this->assertSame(401, $c->login($this->req('alice', 'nope'), new ArraySessionStore(), now: 1000, clientIp: '1.2.3.4')->status()); + $this->assertSame(401, $c->login($this->req('alice', 'nope'), $this->sess(), now: 1000, clientIp: '1.2.3.4')->status()); } public function testLockoutAfterMaxAttempts(): void { $repo = $this->users(); $c = new AuthController($repo, new InMemoryAuditRepository(), new InMemoryRateLimiter(), maxAttempts: 3, lockSeconds: 900); - for ($i = 0; $i < 3; $i++) $c->login($this->req('alice', 'bad'), new ArraySessionStore(), now: 1000, clientIp: '1.2.3.4'); + for ($i = 0; $i < 3; $i++) $c->login($this->req('alice', 'bad'), $this->sess(), now: 1000, clientIp: '1.2.3.4'); // now locked, even correct password is 423 - $this->assertSame(423, $c->login($this->req('alice', 'pw'), new ArraySessionStore(), now: 1000, clientIp: '1.2.3.4')->status()); + $this->assertSame(423, $c->login($this->req('alice', 'pw'), $this->sess(), now: 1000, clientIp: '1.2.3.4')->status()); } public function testExpiredLockResetsAttempts(): void { $repo = $this->users(); $c = new AuthController($repo, new InMemoryAuditRepository(), new InMemoryRateLimiter(), maxAttempts: 3, lockSeconds: 900); // lock the account with 3 bad tries at now=1000 - for ($i = 0; $i < 3; $i++) $c->login($this->req('alice', 'bad'), new ArraySessionStore(), now: 1000, clientIp: '1.2.3.4'); + for ($i = 0; $i < 3; $i++) $c->login($this->req('alice', 'bad'), $this->sess(), now: 1000, clientIp: '1.2.3.4'); // locked_until is now 1000 + 900 = 1900, verify it's locked - $this->assertSame(423, $c->login($this->req('alice', 'pw'), new ArraySessionStore(), now: 1000, clientIp: '1.2.3.4')->status()); + $this->assertSame(423, $c->login($this->req('alice', 'pw'), $this->sess(), now: 1000, clientIp: '1.2.3.4')->status()); // now at now=2000 (after lock window expires at 1900), login with correct password should succeed - $sess = new ArraySessionStore(); + $sess = $this->sess(); $res = $c->login($this->req('alice', 'pw'), $sess, now: 2000, clientIp: '1.2.3.4'); $this->assertSame(200, $res->status()); $this->assertSame(1, $sess->get('user_id')); } public function testUnknownUsernameReturns401GenericBody(): void { $c = new AuthController($this->users(), new InMemoryAuditRepository(), new InMemoryRateLimiter()); - $res = $c->login($this->req('nobody', 'whatever'), new ArraySessionStore(), now: 1000, clientIp: '1.2.3.4'); + $res = $c->login($this->req('nobody', 'whatever'), $this->sess(), now: 1000, clientIp: '1.2.3.4'); $this->assertSame(401, $res->status()); // identical generic body to the wrong-password case (no user-enumeration leak) - $wrong = $c->login($this->req('alice', 'nope'), new ArraySessionStore(), now: 1000, clientIp: '1.2.3.4'); + $wrong = $c->login($this->req('alice', 'nope'), $this->sess(), now: 1000, clientIp: '1.2.3.4'); $this->assertSame($wrong->body(), $res->body()); } public function testLogoutClearsSession(): void { $c = new AuthController($this->users(), new InMemoryAuditRepository(), new InMemoryRateLimiter()); - $sess = new ArraySessionStore(); + $sess = $this->sess(); $c->login($this->req('alice', 'pw'), $sess, now: 1000, clientIp: '1.2.3.4'); $this->assertSame(1, $sess->get('user_id')); $res = $c->logout($sess); @@ -72,8 +76,22 @@ final class AuthControllerTest extends TestCase // throttled before the (expensive) Argon2id verify runs repeatedly. $c = new AuthController($this->users(), new InMemoryAuditRepository(), new InMemoryRateLimiter(), loginIpLimitPerMin: 2); $ip = '9.9.9.9'; - $this->assertSame(200, $c->login($this->req('alice', 'pw'), new ArraySessionStore(), now: 1000, clientIp: $ip)->status()); - $this->assertSame(200, $c->login($this->req('alice', 'pw'), new ArraySessionStore(), now: 1000, clientIp: $ip)->status()); - $this->assertSame(429, $c->login($this->req('alice', 'pw'), new ArraySessionStore(), now: 1000, clientIp: $ip)->status()); + $this->assertSame(200, $c->login($this->req('alice', 'pw'), $this->sess(), now: 1000, clientIp: $ip)->status()); + $this->assertSame(200, $c->login($this->req('alice', 'pw'), $this->sess(), now: 1000, clientIp: $ip)->status()); + $this->assertSame(429, $c->login($this->req('alice', 'pw'), $this->sess(), now: 1000, clientIp: $ip)->status()); + } + public function testLoginRejectsMissingOrWrongCsrf(): void { + // Login CSRF: a forged cross-site POST would carry no (or a wrong) csrf field and must be + // rejected 403 BEFORE any credential check — even with an otherwise-valid password. + $c = new AuthController($this->users(), new InMemoryAuditRepository(), new InMemoryRateLimiter()); + // missing csrf field in the body + $missing = new Request('POST', '/login', [], '', [], ['username' => 'alice', 'password' => 'pw']); + $this->assertSame(403, $c->login($missing, $this->sess(), now: 1000, clientIp: '1.2.3.4')->status()); + // wrong csrf token + $wrong = new Request('POST', '/login', [], '', [], ['username' => 'alice', 'password' => 'pw', 'csrf' => 'nope']); + $this->assertSame(403, $c->login($wrong, $this->sess(), now: 1000, clientIp: '1.2.3.4')->status()); + // session that never seeded a token (empty expected) also rejects, even with a matching-looking field + $noTok = new ArraySessionStore(); + $this->assertSame(403, $c->login($this->req('alice', 'pw'), $noTok, now: 1000, clientIp: '1.2.3.4')->status()); } } diff --git a/cloud/tests/ZoneConfigTest.php b/cloud/tests/ZoneConfigTest.php index 0141dfa..30959a0 100644 --- a/cloud/tests/ZoneConfigTest.php +++ b/cloud/tests/ZoneConfigTest.php @@ -33,6 +33,20 @@ final class ZoneConfigTest extends TestCase $this->assertSame(201, $res->status()); $this->assertCount(1, $repo->forCamera('cam-1')); } + public function testSaveOwnershipMismatchIs400NotError(): void { + // The repo throws RuntimeException when the zone isn't owned by the given device/camera. + // The controller must turn that client error into 400, not let it become a 500. + $throwing = new class implements \App\Repository\ZoneRepository { + public function forDevice(string $d): array { return []; } + public function forCamera(string $c): array { return []; } + public function save(array $z): string { throw new \RuntimeException('not owned'); } + public function delete(string $id): bool { return true; } + }; + $c = new ZoneConfigController($throwing, new InMemoryAuditRepository(), $this->authed('operator')); + $res = $c->save($this->saveReq(['zone_id' => 'z1', 'device_id' => 'pi-2', 'camera_id' => 'cam-1', + 'level' => 'restricted', 'polygon' => [[0, 0], [9, 0], [9, 9]]])); + $this->assertSame(400, $res->status()); + } public function testSaveRejectsBadLevelOrPolygon(): void { $c = new ZoneConfigController(new InMemoryZoneRepository(), new InMemoryAuditRepository(), $this->authed('operator')); $this->assertSame(400, $c->save($this->saveReq(['camera_id' => 'cam-1', 'level' => 'BOOM', 'polygon' => [[0,0],[9,0],[9,9]]]))->status()); diff --git a/docs/delivery-audit.md b/docs/delivery-audit.md index 50f4823..e510147 100644 --- a/docs/delivery-audit.md +++ b/docs/delivery-audit.md @@ -161,14 +161,20 @@ misconfig/degradation fail LOUD. DONE — dispatch rawurldecodes captured params; BasePathTest locks space + Korean cases. - [ ] **P4-5 [M]** `cloud/src/Dashboard/EventListController.php:17` — event list hard-capped at newest 100, no pagination/date filter; older events unreachable. **Fix:** cursor pagination. -- [ ] **P4-6 [M]** `cloud/src/Repository/Pdo/PdoZoneRepository.php:86` — editing a zone with a +- [x] **P4-6 [M]** `cloud/src/Repository/Pdo/PdoZoneRepository.php:86` — editing a zone with a changed device_id/camera_id throws an uncaught RuntimeException → 500. **Fix:** 400 with a clear error. + DONE — ZoneConfigController::save wraps `$this->zones->save()` in try/catch(\RuntimeException) + → `invalid_zone` 400; test `testSaveOwnershipMismatchIs400NotError` with a throwing repo double. - [ ] **P4-7 [L]** `cloud/src/Ingest/IngestController.php:64` — snapshot write failure is swallowed (201 with evidence lost); file-write/DB-insert non-atomic → orphan JPEGs. **Fix:** order write→insert, log failures, reconcile orphans. -- [ ] **P4-8 [L]** `cloud/src/Auth/AuthController.php:31` — login form emits a CSRF token that +- [x] **P4-8 [L]** `cloud/src/Auth/AuthController.php:31` — login form emits a CSRF token that `login` never checks. **Fix:** verify it (or drop it). + DONE — verified (completed the half-built defense, not dropped). login() checks + `Csrf::check($sess->get('csrf'), $req->post('csrf'))` → 403 before the user lookup, closing the + login-CSRF / session-fixation hole. Tests updated to seed the session+body token; added + `testLoginRejectsMissingOrWrongCsrf`. - [x] **P4-9 [L]** `cloud/src/Auth/AuthController.php:71` — logout calls `regenerate()` after `session_destroy()` → E_WARNING, old cookie not replaced. **Fix:** reorder. DONE — logout() rotates the id then wipes (was regenerate-after-destroy). @@ -209,6 +215,11 @@ misconfig/degradation fail LOUD. (updated as each item lands — newest first) +- **2026-07-17 — P4-6 + P4-8 landed (cloud error/auth hardening).** ZoneConfig ownership-mismatch + now 400 not 500 (P4-6); login CSRF token now verified — closes login-CSRF/session-fixation + (P4-8). phpunit 88 green (4 skipped: PDO). Remaining solo: P4-5 (pagination), P4-7 (atomic + snapshot), P5-3 (HEF in bundle), P5-4 (pin+hash+SBOM). + - **2026-07-17 — 17 items landed, all with tests, full suite green.** ALL 4 BLOCKERS cleared (P1-2, P2-1, P2-2, P5-1) + all Phase-1 silent-failure (P1-1,3,4,5,6,7,8,10,11) + Phase-2 HIGH correctness (P2-3,4,5). Remaining: P1-9 (deferred to cloud batch), P2-6..11 (mostly threshold