From 99070f5e87cc432e3cc3cd3573ee124da2c99fc9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=EC=9C=A0=EC=B0=BD=EC=9A=B1?= Date: Fri, 17 Jul 2026 10:08:28 +0900 Subject: [PATCH] fix(cloud): don't swallow snapshot failures, clean orphan JPEGs (P4-7) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The ingest snapshot loop silently `continue`d on re-encode and file-write failures (evidence lost with a 201), and a DB-index throw after the file landed left an orphan JPEG on disk with no row — or bubbled into a 500 that discarded an already-stored event. Extracts storeSnapshot(): re-encode → write file → index, in that order so bytes exist before a row points at them. Every failure is error_log'd instead of swallowed; a DB-index failure after the write @unlinks the orphan file. A snapshot problem no longer fails the ingest — the event is already persisted, so the 201 stands. Test: testSnapshotIndexFailureCleansOrphanFileButKeepsEvent uses a throwing snapshot repo and asserts 201, the event is stored, and no *.jpg is left in the snapshot dir. phpunit: 90 passing (4 skipped: PDO-backed). Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_019Xmn8KZWqiQUguEMRtn6rW --- cloud/src/Ingest/IngestController.php | 33 +++++++++++++++++++++++---- cloud/tests/IngestControllerTest.php | 20 ++++++++++++++++ docs/delivery-audit.md | 7 +++++- 3 files changed, 54 insertions(+), 6 deletions(-) diff --git a/cloud/src/Ingest/IngestController.php b/cloud/src/Ingest/IngestController.php index 113dd6c..cc80e8d 100644 --- a/cloud/src/Ingest/IngestController.php +++ b/cloud/src/Ingest/IngestController.php @@ -68,11 +68,7 @@ final class IngestController $eventId = $this->events->insert($v['clean']); foreach (($payload['snapshots'] ?? []) as $b64) { - $img = ImageProcessor::reencode((string)$b64); - if (!$img['ok']) continue; - $path = rtrim($this->cfg->snapshotDir(), '/\\') . '/' . $img['uuid'] . '.jpg'; - if (file_put_contents($path, $img['bytes']) === false) continue; - $this->snapshots->add($eventId, $img['uuid'], $img['hash']); + $this->storeSnapshot($eventId, (string)$b64); } // PIPA retention: purge worker snapshots older than the configured window. Runs rarely // (the gate) and off the critical path — the event is already stored. @@ -82,4 +78,31 @@ final class IngestController } return Response::json(['event_id' => $eventId], 201); } + + /** + * Persist one snapshot: re-encode → write file → index in the DB, in that order so the + * bytes exist on disk before a row can point at them. Failures are logged (never silently + * swallowed) and, critically, a DB-index failure AFTER the file lands deletes the orphan + * JPEG so disk never accumulates evidence with no index row. A snapshot problem never fails + * the ingest — the event itself is already stored and the 201 stands. + */ + private function storeSnapshot(int $eventId, string $b64): void + { + $img = ImageProcessor::reencode($b64); + if (!$img['ok']) { + error_log("ingest: snapshot re-encode failed for event $eventId (malformed or oversized)"); + return; + } + $path = rtrim($this->cfg->snapshotDir(), '/\\') . '/' . $img['uuid'] . '.jpg'; + if (file_put_contents($path, $img['bytes']) === false) { + error_log("ingest: snapshot file write failed for event $eventId at $path"); + return; + } + try { + $this->snapshots->add($eventId, $img['uuid'], $img['hash']); + } catch (\Throwable $e) { + @unlink($path); + error_log("ingest: snapshot index failed for event $eventId, removed orphan {$img['uuid']}: " . $e->getMessage()); + } + } } diff --git a/cloud/tests/IngestControllerTest.php b/cloud/tests/IngestControllerTest.php index 321a31f..0d72b3f 100644 --- a/cloud/tests/IngestControllerTest.php +++ b/cloud/tests/IngestControllerTest.php @@ -84,6 +84,26 @@ final class IngestControllerTest extends TestCase $this->assertArrayHasKey($recent, $snaps->created, 'a 1-day-old snapshot must survive'); } + public function testSnapshotIndexFailureCleansOrphanFileButKeepsEvent(): void { + // The file lands, then the DB index throws. The event must still be stored (201) and the + // orphan JPEG must be removed — never left on disk with no row pointing at it. + $cfg = Config::fromArray(['snapshot_dir' => $this->dir, 'timestamp_window' => 300, 'rate_limit_per_min' => 60]); + $throwingSnaps = new class implements \App\Repository\SnapshotRepository { + public function add(int $e, string $u, string $h): void { throw new \RuntimeException('db down'); } + public function forEvent(int $e): array { return []; } + public function pathByUuid(string $u): ?string { return null; } + public function purgeOlderThan(int $c): int { return 0; } + }; + $events = new InMemoryEventRepository(); + $c = new IngestController($cfg, new InMemoryDeviceRepository(['pi-1' => 'sekret']), + new InMemoryNonceRepository(), new InMemoryRateLimiter(), $events, $throwingSnaps); + $res = $c->handle($this->signedReq('sekret', $this->payload()), now: 1000, clientIp: '1.2.3.4'); + $this->assertSame(201, $res->status()); // a snapshot failure never discards the event + $this->assertCount(1, $events->rows); + $this->assertSame([], glob($this->dir . '/*.jpg'), + 'orphan snapshot file must be removed when the DB index write fails'); + } + public function testIpFloodRateLimited(): void { // Attacker without a valid device secret floods from one IP; the pre-auth // IP limiter must trip BEFORE any HMAC/device-secret work, protecting diff --git a/docs/delivery-audit.md b/docs/delivery-audit.md index 1182384..cd23834 100644 --- a/docs/delivery-audit.md +++ b/docs/delivery-audit.md @@ -171,9 +171,14 @@ misconfig/degradation fail LOUD. 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 +- [x] **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. + DONE — extracted `storeSnapshot()`: re-encode→write→index in order; every failure (re-encode, + file write, DB index) now `error_log`s instead of a silent `continue`; a DB-index failure after + the file lands `@unlink`s the orphan. A snapshot problem no longer fails the ingest — the event + is already stored, 201 stands. Test `testSnapshotIndexFailureCleansOrphanFileButKeepsEvent` + (throwing repo double → asserts 201, event stored, no leftover *.jpg). - [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