diff --git a/CHANGELOG.md b/CHANGELOG.md index 564fcc4..80169a2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,13 @@ +# 0.4.3-dev + +- Serialize moderation, deletion and retention with a stable storage lock. +- Remove hidden pending residues of interrupted approvals during retention, including dry-run reporting. +- Write complete JSON to unique temporary files before atomic replacement; keep previous data on failure. +- Preserve published edits when recovering an interrupted approval and prevent deleted comments from resurfacing. +- Keep rate-limit counters intact on interrupted writes; reject empty or damaged existing files. +- Register daily comment retention with the Grav scheduler and support a non-mutating pruning preview. +- Add multiprocess and storage-failure regression tests. + # 0.4.2 ## 2026-09-23 diff --git a/README.md b/README.md index bfedf6c..4e1a1fe 100644 --- a/README.md +++ b/README.md @@ -81,3 +81,28 @@ only the IP or only the email does not bypass the limit. The rate-limit file is stored under the site's `user-data://void-comments/` directory and contains only SHA-256-derived keys and timestamps. Files written by version 0.1.x, which used an IP+email pair key, remain effective for that exact pair during the migration. + +## Storage and retention + +Moderation operations use a stable `.storage.lock` file. Rate limits use a +separate lock beside the JSON counters. Do not remove these locks while PHP +workers are running. Writes use unique temporary files, check complete writes +and flush before replacing JSON. Use local storage with working `flock` and +atomic rename semantics; distributed filesystems need separate validation. + +The approved file is the commit point for approval. Retrying an interrupted +approval removes stale pending data without replacing subsequent approved edits. +Readers suppress the stale pending copy; retention removes it without changing +the approved record, and includes the cleanup in pruning previews. Deleting an approved comment also +removes any pending copy left by an interrupted approval. + +The `void-comments-retention` scheduler job runs daily at 03:20 in Grav's +scheduler timezone. Configure the host to invoke Grav's scheduler regularly; +registering the job alone does not install an operating-system cron task. +It uses the existing policy: technical metadata expires after 7 days and +pending comments after 90 days. Approved comments have no automatic expiry. +`CommentStore::prune($now, true)` previews removals without changing records. +The same storage lock serializes retention and moderation. + +`composer test` includes worker-process concurrency, short-write, failed-write, +approval-recovery and retention-preview checks. No site records are required. diff --git a/blueprints.yaml b/blueprints.yaml index 1b21167..a6542f6 100644 --- a/blueprints.yaml +++ b/blueprints.yaml @@ -1,7 +1,7 @@ name: Void Comments slug: void-comments type: plugin -version: 0.4.2 +version: 0.4.3-dev description: Shared flat-file comments for Grav 2 with moderation, replies, rate limiting and CAPTCHA. icon: comments author: diff --git a/composer.json b/composer.json index b4824fb..9ff028e 100644 --- a/composer.json +++ b/composer.json @@ -2,7 +2,12 @@ "name": "voidlabs/grav-plugin-void-comments", "type": "grav-plugin", "description": "Shared flat-file comments for Grav 2 with moderation, replies and rate limiting.", - "keywords": ["grav", "plugin", "comments", "moderation"], + "keywords": [ + "grav", + "plugin", + "comments", + "moderation" + ], "homepage": "https://github.com/voidlabs/grav-plugin-void-comments", "license": "MIT", "authors": [ @@ -27,6 +32,9 @@ } }, "scripts": { - "test": "@php tests/run.php" + "test": [ + "@php tests/run.php", + "@php tests/storage.php" + ] } } diff --git a/src/AtomicJson.php b/src/AtomicJson.php new file mode 100644 index 0000000..389bd46 --- /dev/null +++ b/src/AtomicJson.php @@ -0,0 +1,62 @@ +file); - if (!is_dir($directory) && !mkdir($directory, 0770, true) && !is_dir($directory)) return false; - $handle = fopen($this->file, 'c+'); - if ($handle === false || !flock($handle, LOCK_EX)) { - if (is_resource($handle)) fclose($handle); - return false; - } try { - $raw = stream_get_contents($handle); - $data = $raw === '' ? [] : json_decode((string) $raw, true); - if (!is_array($data)) return false; - $address = strtolower(trim($address)); - $email = strtolower(trim($email)); - $legacyKey = hash('sha256', $address . "\0" . $email); - $keys = ['ip:' . hash('sha256', $address), 'email:' . hash('sha256', $email)]; - $threshold = $now - $this->window; - foreach ($data as $candidate => $times) { - $data[$candidate] = array_values(array_filter( - (array) $times, - static fn($time): bool => is_int($time) && $time > $threshold - )); - if ($data[$candidate] === []) unset($data[$candidate]); - } - // Files created by 0.1.x used a hash of IP + email. Keep active - // entries for that exact pair effective while new requests use - // independent IP and email counters. - foreach ($keys as $key) { - if (count($data[$legacyKey] ?? []) + count($data[$key] ?? []) >= $this->maximum) return false; - } - foreach ($keys as $key) $data[$key][] = $now; - $json = json_encode($data, JSON_THROW_ON_ERROR); - rewind($handle); - return ftruncate($handle, 0) - && fwrite($handle, $json) === strlen($json) - && fflush($handle); - } finally { - flock($handle, LOCK_UN); fclose($handle); + return AtomicJson::locked($this->file . '.lock', function () use ($address, $email, $now): bool { + $data = AtomicJson::read($this->file); + $address = strtolower(trim($address)); + $email = strtolower(trim($email)); + $legacyKey = hash('sha256', $address . "\0" . $email); + $keys = ['ip:' . hash('sha256', $address), 'email:' . hash('sha256', $email)]; + $threshold = $now - $this->window; + foreach ($data as $candidate => $times) { + $data[$candidate] = array_values(array_filter( + (array) $times, + static fn($time): bool => is_int($time) && $time > $threshold + )); + if ($data[$candidate] === []) unset($data[$candidate]); + } + // Files created by 0.1.x used a hash of IP + email. Keep active + // entries for that exact pair effective while new requests use + // independent IP and email counters. + foreach ($keys as $key) { + if (count($data[$legacyKey] ?? []) + count($data[$key] ?? []) >= $this->maximum) return false; + } + foreach ($keys as $key) $data[$key][] = $now; + AtomicJson::write($this->file, $data); + return true; + }); + } catch (\Throwable) { + // Missing files start new counters; damaged existing files fail closed. + return false; } } } diff --git a/src/CommentStore.php b/src/CommentStore.php index 61af504..eb60d67 100644 --- a/src/CommentStore.php +++ b/src/CommentStore.php @@ -4,11 +4,29 @@ namespace VoidLabs\Comments; +require_once __DIR__ . '/AtomicJson.php'; + final class CommentStore { + private bool $locked = false; + + private function transaction(callable $operation): mixed + { + if ($this->locked) return $operation(); + return AtomicJson::locked($this->directory . '/.storage.lock', function () use ($operation) { + $this->locked = true; + try { return $operation(); } finally { $this->locked = false; } + }); + } + public function __construct(private readonly string $directory) {} public function addPending(CommentRequest $request, string $address, ?\DateTimeImmutable $now = null): array + { + return $this->transaction(fn() => $this->addPendingLocked($request, $address, $now)); + } + + private function addPendingLocked(CommentRequest $request, string $address, ?\DateTimeImmutable $now = null): array { $now ??= new \DateTimeImmutable('now', new \DateTimeZone('UTC')); $id = bin2hex(random_bytes(16)); @@ -125,33 +143,58 @@ public function approvedParentForRoute(string $id, string $route): ?array } public function approve(string $id, ?\DateTimeImmutable $now = null): bool + { + return $this->transaction(fn() => $this->approveLocked($id, $now)); + } + + private function approveLocked(string $id, ?\DateTimeImmutable $now = null): bool { $record = $this->read('pending', $id); if ($record === null) return false; $now ??= new \DateTimeImmutable('now', new \DateTimeZone('UTC')); $record['moderated_at'] = $now->format(\DateTimeInterface::ATOM); unset($record['technical']); - $this->write('approved', $id, $record); + // Recover a previous approval interrupted after publishing the approved record. + // Never overwrite subsequent moderator edits with the stale pending copy. + if ($this->read('approved', $id) === null) $this->write('approved', $id, $record); $source = $this->filename('pending', $id); if (is_file($source) && !unlink($source)) throw new \RuntimeException('Impossibile rimuovere il commento pending.'); return true; } public function deletePending(string $id): bool + { + return $this->transaction(fn() => $this->deletePendingLocked($id)); + } + + private function deletePendingLocked(string $id): bool { $filename = $this->filename('pending', $id); return is_file($filename) && unlink($filename); } public function deleteApproved(string $id): bool + { + return $this->transaction(fn() => $this->deleteApprovedLocked($id)); + } + + private function deleteApprovedLocked(string $id): bool { $filename = $this->filename('approved', $id); + $pending = $this->filename('pending', $id); + if (is_file($filename) && is_file($pending) && !unlink($pending)) throw new \RuntimeException('Cannot remove interrupted approval.'); return is_file($filename) && unlink($filename); } public function update(string $status, string $id, array $changes): bool + { + return $this->transaction(fn() => $this->updateLocked($status, $id, $changes)); + } + + private function updateLocked(string $status, string $id, array $changes): bool { if (!in_array($status, ['pending', 'approved'], true)) throw new \InvalidArgumentException('Stato commento non valido.'); $record = $this->read($status, $id); + if ($status === 'pending' && is_file($this->filename('approved', $id))) return false; if ($record === null) return false; if (array_key_exists('author', $changes)) { $author = trim((string) $changes['author']); @@ -178,31 +221,48 @@ public function update(string $status, string $id, array $changes): bool return true; } - public function prune(?\DateTimeImmutable $now = null): array + public function prune(?\DateTimeImmutable $now = null, bool $dryRun = false): array + { + return $this->transaction(fn() => $this->pruneLocked($now, $dryRun)); + } + + private function pruneLocked(?\DateTimeImmutable $now = null, bool $dryRun = false): array { $now ??= new \DateTimeImmutable('now', new \DateTimeZone('UTC')); $result = ['technical_removed' => 0, 'pending_deleted' => 0]; - foreach ($this->records('pending') as $record) { + foreach ($this->recordsLocked('pending', true) as $record) { $id = (string) ($record['id'] ?? ''); + // Readers hide these records; retention must still remove their private data. + if ($this->read('approved', $id) !== null) { + if ($dryRun || $this->deletePendingLocked($id)) ++$result['pending_deleted']; + continue; + } $created = \DateTimeImmutable::createFromFormat(\DateTimeInterface::ATOM, (string) ($record['created_at'] ?? '')); if ($created && $created <= $now->modify('-90 days')) { - if ($this->deletePending($id)) ++$result['pending_deleted']; + if ($dryRun || $this->deletePending($id)) ++$result['pending_deleted']; continue; } $expires = \DateTimeImmutable::createFromFormat(\DateTimeInterface::ATOM, (string) ($record['technical']['expires_at'] ?? '')); if (isset($record['technical']) && $expires && $expires <= $now) { - unset($record['technical']); $this->write('pending', $id, $record); ++$result['technical_removed']; + unset($record['technical']); if (!$dryRun) $this->write('pending', $id, $record); ++$result['technical_removed']; } } return $result; } private function records(string $status): array + { + return $this->transaction(fn() => $this->recordsLocked($status)); + } + + private function recordsLocked(string $status, bool $includeSuperseded = false): array { $directory = $this->directory . '/' . $status; if (!is_dir($directory)) return []; $records = []; foreach (glob($directory . '/*.json') ?: [] as $filename) { + // Publishing the approved file is the commit point, even after a crash. + if (!$includeSuperseded && $status === 'pending' && is_file($this->directory . '/approved/' . basename($filename))) continue; $record = json_decode((string) file_get_contents($filename), true); if (is_array($record)) { // La directory e' l'unica fonte dello stato. I record legacy @@ -230,11 +290,7 @@ private function write(string $status, string $id, array $record): void unset($record['status']); $directory = $this->directory . '/' . $status; if (!is_dir($directory) && !mkdir($directory, 0770, true) && !is_dir($directory)) throw new \RuntimeException('Impossibile creare lo storage dei commenti.'); - $target = $this->filename($status, $id); $temporary = $target . '.tmp-' . bin2hex(random_bytes(4)); - $json = json_encode($record, JSON_PRETTY_PRINT | JSON_UNESCAPED_UNICODE | JSON_UNESCAPED_SLASHES | JSON_THROW_ON_ERROR) . "\n"; - if (file_put_contents($temporary, $json, LOCK_EX) === false || !rename($temporary, $target)) { - @unlink($temporary); throw new \RuntimeException('Impossibile aggiornare il commento.'); - } + AtomicJson::write($this->filename($status, $id), $record); } private function filename(string $status, string $id): string diff --git a/tests/run.php b/tests/run.php index a86a826..64d2a6d 100644 --- a/tests/run.php +++ b/tests/run.php @@ -38,7 +38,7 @@ function remove_tree(string $directory): void if (!is_dir($directory)) { return; } - foreach (glob($directory . '/*') ?: [] as $path) { + foreach (array_merge(glob($directory . '/*') ?: [], glob($directory . '/.[!.]*') ?: []) as $path) { if (is_dir($path)) { remove_tree($path); } else { diff --git a/tests/storage.php b/tests/storage.php new file mode 100644 index 0000000..9bed9ba --- /dev/null +++ b/tests/storage.php @@ -0,0 +1,110 @@ + (new CommentRateLimiter($dir . '/limits.json', 3600, 7))->consume('127.0.0.1', 'test@example.test', 1000), + 'body' => $store->update('pending', $id, ['body' => 'Edited body']), + 'author' => $store->update('pending', $id, ['author' => 'Edited author']), + 'approve' => $store->approve($id), + 'prune' => $store->prune(new DateTimeImmutable('2030-01-01T00:00:00+00:00')), + }; + echo json_encode($result); + exit; +} +$checks = 0; +$assert = static function (bool $condition, string $message) use (&$checks): void { + ++$checks; + if (!$condition) throw new RuntimeException($message); +}; +$dir = sys_get_temp_dir() . '/void-storage-' . bin2hex(random_bytes(8)); +mkdir($dir, 0700); +$spawn = static function (string $kind, string $dir, string $id = ''): array { + $command = [PHP_BINARY]; + if ($ini = php_ini_loaded_file()) array_push($command, '-c', $ini); + array_push($command, __FILE__, 'worker', $kind, $dir, $id); + $process = proc_open($command, [0 => ['pipe', 'r'], 1 => ['pipe', 'w'], 2 => ['pipe', 'w']], $pipes); + if (!is_resource($process)) throw new RuntimeException('Cannot start worker.'); + fclose($pipes[0]); + return [$process, $pipes]; +}; +$join = static function (array $worker): mixed { + [$process, $pipes] = $worker; + $output = stream_get_contents($pipes[1]); $errors = stream_get_contents($pipes[2]); + fclose($pipes[1]); fclose($pipes[2]); + if (proc_close($process) !== 0 || $errors !== '') throw new RuntimeException($errors); + return json_decode($output, true, flags: JSON_THROW_ON_ERROR); +}; +try { + $file = $dir . '/atomic.json'; + $GLOBALS['write_chunk'] = 7; + AtomicJson::write($file, ['text' => str_repeat('abc', 200)]); + $assert(strlen(AtomicJson::read($file)['text']) === 600, 'Short writes truncated JSON.'); + $before = file_get_contents($file); $GLOBALS['fail_write'] = true; + try { AtomicJson::write($file, ['new' => true]); throw new LogicException('Failure was ignored.'); } + catch (RuntimeException) {} + $GLOBALS['fail_write'] = false; + $assert(file_get_contents($file) === $before && glob($file . '.tmp-*') === [], 'Failed write damaged original or leaked temp.'); + file_put_contents($dir . '/empty.json', ''); + $assert(!(new CommentRateLimiter($dir . '/empty.json'))->consume('a', 'b'), 'Empty limiter failed open.'); + $workers = []; + for ($i = 0; $i < 16; ++$i) $workers[] = $spawn('limit', $dir); + $assert(count(array_filter(array_map($join, $workers))) === 7, 'Concurrent rate limit exceeded or lost updates.'); + $store = new CommentStore($dir); + $request = CommentRequest::fromArray(['route' => '/article', 'name' => 'Author', 'email' => 'test@example.test', 'message' => 'Body'], '/article', 'article', ['article']); + $record = $store->addPending($request, '127.0.0.1'); $id = $record['id']; + $lock = fopen($dir . '/.storage.lock', 'c+b'); flock($lock, LOCK_EX); + $workers = [$spawn('author', $dir, $id), $spawn('body', $dir, $id)]; + usleep(200000); flock($lock, LOCK_UN); fclose($lock); + $assert(array_map($join, $workers) === [true, true], 'Concurrent edits failed.'); + $changed = $store->pending()[0]; + $assert($changed['author'] === 'Edited author' && $changed['body'] === 'Edited body', 'Concurrent edits lost a field.'); + $preview = $store->prune(new DateTimeImmutable('2030-01-01T00:00:00+00:00'), true); + $assert($preview['pending_deleted'] === 1 && count($store->pending()) === 1, 'Retention preview modified data.'); + // Model a crash between publishing approved and removing pending. + $approved = $changed; $approved['body'] = 'Published edit'; unset($approved['technical']); + AtomicJson::write($dir . '/approved/' . $id . '.json', $approved); + $assert($store->pending() === [], 'Interrupted approval exposed duplicate pending.'); + $assert($store->approve($id) && $store->approved()[0]['body'] === 'Published edit', 'Approval retry overwrote published edit.'); + AtomicJson::write($dir . '/pending/' . $id . '.json', $record); + $assert($store->deleteApproved($id) && !is_file($dir . '/pending/' . $id . '.json'), 'Delete left a resumable stale pending.'); + $record = $store->addPending($request, '127.0.0.1', new DateTimeImmutable('2020-01-01T00:00:00+00:00')); + $workers = [$spawn('body', $dir, $record['id']), $spawn('prune', $dir, $record['id'])]; + array_map($join, $workers); + $assert($store->pending() === [], 'Concurrent edit resurrected expired comment.'); + // Retention must see hidden pending copies left by an interrupted approval. + $residue = $store->addPending($request, '127.0.0.1'); + $published = $residue; unset($published['technical']); + $published['body'] = 'Keep the published version'; + AtomicJson::write($dir . '/approved/' . $residue['id'] . '.json', $published); + $pendingFile = $dir . '/pending/' . $residue['id'] . '.json'; + $preview = $store->prune(null, true); + $assert($preview['pending_deleted'] === 1 && is_file($pendingFile), 'Preview missed hidden approval residue or modified it.'); + $pruned = $store->prune(); + $assert($pruned['pending_deleted'] === 1 && !is_file($pendingFile), 'Retention left hidden pending data indefinitely.'); + $assert($store->approved()[0] === $published, 'Residue cleanup changed the approved record.'); + + echo "OK: {$checks} storage checks, including 20 worker processes.\n"; +} finally { + $files = new RecursiveIteratorIterator(new RecursiveDirectoryIterator($dir, FilesystemIterator::SKIP_DOTS), RecursiveIteratorIterator::CHILD_FIRST); + foreach ($files as $file) $file->isDir() ? rmdir($file->getPathname()) : unlink($file->getPathname()); + rmdir($dir); +} +} diff --git a/void-comments.php b/void-comments.php index d8e9adb..1fe845b 100644 --- a/void-comments.php +++ b/void-comments.php @@ -18,6 +18,7 @@ final class VoidCommentsPlugin extends Plugin public static function getSubscribedEvents(): array { return [ + 'onSchedulerInitialized' => ['onSchedulerInitialized', 0], 'onFormPageHeaderProcessed' => ['onFormPageHeaderProcessed', 10], 'onFormProcessed' => ['onFormProcessed', 10], 'onTwigSiteVariables' => ['onTwigSiteVariables', 0], @@ -26,6 +27,11 @@ public static function getSubscribedEvents(): array ]; } + public function onSchedulerInitialized(Event $event): void + { + $event['scheduler']->addFunction(fn() => $this->store()->prune(), [], 'void-comments-retention')->daily(3, 20); + } + public function autoload(): ClassLoader { $loader = new ClassLoader();