From 746c91316c9d93d0e71089a66323521e2c6c9bba Mon Sep 17 00:00:00 2001 From: fabiodalez-dev Date: Wed, 19 Aug 2026 15:27:26 +0200 Subject: [PATCH 01/16] fix(release): harden plugin updates and package sessions --- .distignore | 2 + .rsync-filter | 3 + app/Support/PluginManager.php | 501 ++++++++++++++++-- bin/build-release.sh | 10 + scripts/ci-verify-release.sh | 4 + tests/code-quality.spec.js | 6 + tests/helpers/plugin-zip-update-bootstrap.php | 49 ++ tests/plugin-manager.unit.php | 10 + ...gin-zip-update-all-bundled.integration.php | 7 +- tests/plugin-zip-update.integration.php | 221 +++++++- 10 files changed, 754 insertions(+), 59 deletions(-) create mode 100644 tests/helpers/plugin-zip-update-bootstrap.php diff --git a/.distignore b/.distignore index eabbe4425..b6da0486d 100644 --- a/.distignore +++ b/.distignore @@ -75,6 +75,7 @@ fix-*.php # Storage/uploads (exclude user data, keep directory structure) storage/logs/* storage/cache/* +storage/sessions/* storage/backups/* storage/uploads/* storage/tmp/* @@ -87,6 +88,7 @@ cache/* # Keep essential files and directory structure !storage/logs/.gitkeep !storage/cache/.gitkeep +!storage/sessions/.gitkeep !storage/backups/.gitkeep !storage/backups/.htaccess !storage/uploads/.gitkeep diff --git a/.rsync-filter b/.rsync-filter index eb5de57f8..75fa4e507 100644 --- a/.rsync-filter +++ b/.rsync-filter @@ -34,6 +34,8 @@ + storage/logs/.gitkeep + storage/cache/ + storage/cache/.gitkeep ++ storage/sessions/ ++ storage/sessions/.gitkeep + storage/backups/ + storage/backups/.gitkeep + storage/backups/.htaccess @@ -254,6 +256,7 @@ # Storage user data (structure already included above) - /storage/logs/* - /storage/cache/* +- /storage/sessions/* - /storage/backups/* - /storage/tmp/* - /storage/calendar/* diff --git a/app/Support/PluginManager.php b/app/Support/PluginManager.php index 29c14feba..e19de37b1 100644 --- a/app/Support/PluginManager.php +++ b/app/Support/PluginManager.php @@ -15,6 +15,7 @@ class PluginManager { private const MAX_UPLOAD_BYTES = 104857600; // 100 MB + private const PENDING_UPDATE_PREFIX = '.pinakes-plugin-update-'; private mysqli $db; private string $pluginsDir; private string $uploadsDir; @@ -22,6 +23,9 @@ class PluginManager private ?string $cachedEncryptionKey = null; private bool $encryptionKeyResolved = false; + /** @var array Plugins rolled back after their replacement class was loaded. */ + private array $skipPluginIdsThisRequest = []; + /** * Per-process cache for {@see isActive()} lookups. * @@ -177,6 +181,9 @@ public function autoRegisterBundledPlugins(): int $stmt->close(); if ($row) { + if (isset($this->skipPluginIdsThisRequest[(int) $row['id']])) { + continue; + } // Manifest compatibility metadata is operational, not merely // descriptive. Keep it in sync even when a bundled plugin's // own version did not change in this core release. @@ -1260,8 +1267,12 @@ private function getPluginClassName(string $pluginName): string * identity. Settings, plugin_data, logs and hooks all refer to the existing * ID through foreign keys and are therefore intentionally left untouched. * - * Filesystem changes are rollback-safe: the old package is kept as a sibling - * backup until the metadata update succeeds. + * Active plugins have already been required by the time the admin upload + * endpoint runs, so PHP cannot safely instantiate the replacement class in + * this request. Keep the old package as a sibling backup and persist a + * pending-update marker. The next bootstrap runs the new onActivate() before + * hooks are loaded, then removes the backup; a lifecycle failure restores + * the old package, metadata and hook rows. * * @param array $existingPlugin * @param array $pluginMeta @@ -1280,13 +1291,6 @@ private function updatePluginFromStaging( return ['success' => false, 'message' => __('Plugin installato non valido.'), 'plugin_id' => null]; } - try { - $metadata = json_encode($pluginMeta['metadata'] ?? [], JSON_THROW_ON_ERROR); - } catch (\JsonException $e) { - $this->deleteDirectory($stagingPath); - return ['success' => false, 'message' => __('Metadati del plugin non validi.'), 'plugin_id' => null]; - } - $pluginPath = rtrim($pluginsBaseDir, DIRECTORY_SEPARATOR) . DIRECTORY_SEPARATOR . $directory; try { $backupPath = rtrim($pluginsBaseDir, DIRECTORY_SEPARATOR) @@ -1297,6 +1301,22 @@ private function updatePluginFromStaging( } $hasBackup = false; $newPackageInstalled = false; + $metadataUpdated = false; + $pendingMarkerPath = null; + $pendingMarkerHandle = null; + + try { + $oldPluginSnapshot = $this->pluginMetadataSnapshot($existingPlugin); + $newPluginSnapshot = $this->pluginMetadataSnapshot($pluginMeta); + $oldHooks = $this->snapshotPluginHooks($pluginId); + } catch (\Throwable $e) { + $this->deleteDirectory($stagingPath); + return [ + 'success' => false, + 'message' => __('Impossibile preparare lo stato dell\'aggiornamento del plugin.'), + 'plugin_id' => null, + ]; + } try { if (file_exists($pluginPath) && !is_dir($pluginPath)) { @@ -1315,44 +1335,32 @@ private function updatePluginFromStaging( } $newPackageInstalled = true; - $displayName = (string) $pluginMeta['display_name']; - $description = (string) ($pluginMeta['description'] ?? ''); - $version = (string) $pluginMeta['version']; - $author = (string) ($pluginMeta['author'] ?? ''); - $authorUrl = (string) ($pluginMeta['author_url'] ?? ''); - $pluginUrl = (string) ($pluginMeta['plugin_url'] ?? ''); - $mainFile = (string) $pluginMeta['main_file']; - $requiresPhp = (string) ($pluginMeta['requires_php'] ?? ''); - $requiresApp = (string) ($pluginMeta['requires_app'] ?? ''); - - $stmt = $this->db->prepare( - 'UPDATE plugins SET display_name = ?, description = ?, version = ?, author = ?, author_url = ?, ' - . 'plugin_url = ?, main_file = ?, requires_php = ?, requires_app = ?, metadata = ? WHERE id = ?' - ); - if ($stmt === false) { - throw new \RuntimeException('Impossibile aggiornare i metadati del plugin.'); - } - $stmt->bind_param( - 'ssssssssssi', - $displayName, - $description, - $version, - $author, - $authorUrl, - $pluginUrl, - $mainFile, - $requiresPhp, - $requiresApp, - $metadata, - $pluginId - ); - $updated = $stmt->execute(); - $stmt->close(); - if (!$updated) { - throw new \RuntimeException('Impossibile salvare i metadati aggiornati del plugin.'); + if ((int) ($existingPlugin['is_active'] ?? 0) === 1) { + $pendingMarkerPath = $this->pendingPluginUpdatePath($pluginId); + $pendingState = [ + 'plugin_id' => $pluginId, + 'plugin_name' => (string) ($existingPlugin['name'] ?? $pluginMeta['name']), + 'directory' => $directory, + 'backup_directory' => $hasBackup ? basename($backupPath) : null, + 'old_plugin' => $oldPluginSnapshot, + 'new_plugin' => $newPluginSnapshot, + 'old_hooks' => $oldHooks, + ]; + $pendingMarkerHandle = $this->createPendingPluginUpdateMarker( + $pendingMarkerPath, + $pendingState + ); } - if ($hasBackup && !$this->deleteDirectory($backupPath)) { + $this->applyPluginMetadataSnapshot($pluginId, $newPluginSnapshot); + $metadataUpdated = true; + + // Inactive plugins run onActivate() when the administrator enables + // them, so there is no lifecycle to defer and no backup to retain. + if ((int) ($existingPlugin['is_active'] ?? 0) !== 1 + && $hasBackup + && !$this->deleteDirectory($backupPath) + ) { SecureLogger::warning('[PluginManager] Updated plugin backup could not be removed', [ 'plugin' => $pluginMeta['name'], 'path' => $backupPath, @@ -1368,6 +1376,24 @@ private function updatePluginFromStaging( 'updated' => true, ]; } catch (\Throwable $e) { + if (is_resource($pendingMarkerHandle)) { + if (is_string($pendingMarkerPath)) { + @unlink($pendingMarkerPath); + } + flock($pendingMarkerHandle, LOCK_UN); + fclose($pendingMarkerHandle); + $pendingMarkerHandle = null; + } + if ($metadataUpdated) { + try { + $this->applyPluginMetadataSnapshot($pluginId, $oldPluginSnapshot); + } catch (\Throwable $rollbackError) { + SecureLogger::error('[PluginManager] Failed to restore plugin metadata after update rollback', [ + 'plugin' => $pluginMeta['name'], + 'error' => $rollbackError->getMessage(), + ]); + } + } if ($newPackageInstalled && is_dir($pluginPath)) { $this->deleteDirectory($pluginPath); } @@ -1389,6 +1415,151 @@ private function updatePluginFromStaging( 'message' => __('Errore durante l\'aggiornamento del plugin: %s', $e->getMessage()), 'plugin_id' => null, ]; + } finally { + if (is_resource($pendingMarkerHandle)) { + flock($pendingMarkerHandle, LOCK_UN); + fclose($pendingMarkerHandle); + } + } + } + + /** @return array */ + private function pluginMetadataSnapshot(array $plugin): array + { + $metadata = $plugin['metadata'] ?? []; + if (!is_array($metadata)) { + $decoded = json_decode((string) $metadata, true); + $metadata = is_array($decoded) ? $decoded : []; + } + + return [ + 'display_name' => (string) ($plugin['display_name'] ?? $plugin['name'] ?? ''), + 'description' => (string) ($plugin['description'] ?? ''), + 'version' => (string) ($plugin['version'] ?? ''), + 'author' => (string) ($plugin['author'] ?? ''), + 'author_url' => (string) ($plugin['author_url'] ?? ''), + 'plugin_url' => (string) ($plugin['plugin_url'] ?? ''), + 'main_file' => (string) ($plugin['main_file'] ?? ''), + 'requires_php' => (string) ($plugin['requires_php'] ?? ''), + 'requires_app' => (string) ($plugin['requires_app'] ?? ''), + 'metadata' => json_encode($metadata, JSON_THROW_ON_ERROR), + ]; + } + + /** @param array $snapshot */ + private function applyPluginMetadataSnapshot(int $pluginId, array $snapshot): void + { + $stmt = $this->db->prepare( + 'UPDATE plugins SET display_name = ?, description = ?, version = ?, author = ?, author_url = ?, ' + . 'plugin_url = ?, main_file = ?, requires_php = ?, requires_app = ?, metadata = ? WHERE id = ?' + ); + if ($stmt === false) { + throw new \RuntimeException('Impossibile aggiornare i metadati del plugin.'); + } + + $displayName = $snapshot['display_name']; + $description = $snapshot['description']; + $version = $snapshot['version']; + $author = $snapshot['author']; + $authorUrl = $snapshot['author_url']; + $pluginUrl = $snapshot['plugin_url']; + $mainFile = $snapshot['main_file']; + $requiresPhp = $snapshot['requires_php']; + $requiresApp = $snapshot['requires_app']; + $metadata = $snapshot['metadata']; + $stmt->bind_param( + 'ssssssssssi', + $displayName, + $description, + $version, + $author, + $authorUrl, + $pluginUrl, + $mainFile, + $requiresPhp, + $requiresApp, + $metadata, + $pluginId + ); + $updated = $stmt->execute(); + $stmt->close(); + if (!$updated) { + throw new \RuntimeException('Impossibile salvare i metadati aggiornati del plugin.'); + } + } + + /** + * @return list + */ + private function snapshotPluginHooks(int $pluginId): array + { + $stmt = $this->db->prepare( + 'SELECT hook_name, callback_class, callback_method, priority, is_active, created_at ' + . 'FROM plugin_hooks WHERE plugin_id = ? ORDER BY id ASC' + ); + if ($stmt === false) { + throw new \RuntimeException('Impossibile leggere gli hook del plugin.'); + } + $stmt->bind_param('i', $pluginId); + if (!$stmt->execute()) { + $stmt->close(); + throw new \RuntimeException('Impossibile leggere gli hook del plugin.'); + } + $result = $stmt->get_result(); + if ($result === false) { + $stmt->close(); + throw new \RuntimeException('Impossibile leggere gli hook del plugin.'); + } + + $hooks = []; + while ($row = $result->fetch_assoc()) { + $hooks[] = [ + 'hook_name' => (string) $row['hook_name'], + 'callback_class' => (string) $row['callback_class'], + 'callback_method' => (string) $row['callback_method'], + 'priority' => (int) $row['priority'], + 'is_active' => (int) $row['is_active'], + 'created_at' => (string) $row['created_at'], + ]; + } + $stmt->close(); + return $hooks; + } + + private function pendingPluginUpdatePath(int $pluginId): string + { + return $this->pluginsDir . DIRECTORY_SEPARATOR . self::PENDING_UPDATE_PREFIX . $pluginId . '.json'; + } + + /** + * Write and exclusively lock the marker before changing DB metadata. A + * concurrent bootstrap waits for the upload request to finish, then sees a + * complete package plus a complete rollback snapshot. + * + * @param array $state + * @return resource + */ + private function createPendingPluginUpdateMarker(string $path, array $state) + { + $handle = @fopen($path, 'x+b'); + if ($handle === false || !flock($handle, LOCK_EX)) { + if (is_resource($handle)) { + fclose($handle); + } + throw new \RuntimeException('Esiste già un aggiornamento pendente per questo plugin.'); + } + + try { + $json = json_encode($state, JSON_THROW_ON_ERROR | JSON_UNESCAPED_SLASHES); + if (fwrite($handle, $json) !== strlen($json) || !fflush($handle)) { + throw new \RuntimeException('Impossibile salvare lo stato dell\'aggiornamento del plugin.'); + } + return $handle; + } catch (\Throwable $e) { + @unlink($path); + flock($handle, LOCK_UN); + fclose($handle); + throw $e; } } @@ -1790,6 +1961,11 @@ public function log(?int $pluginId, string $level, string $message, array $conte */ public function loadActivePlugins(): void { + // A ZIP upload replaces files after active plugin classes have already + // been loaded for that request. Complete those upgrades now, in a fresh + // PHP request, before any active plugin or hook is instantiated. + $this->finalizePendingPluginUpdates(); + // Bundled-plugin registration and orphan cleanup are maintenance // operations: they scan the filesystem and issue several queries, yet // their outcome only changes after an update or an admin plugin action. @@ -1840,8 +2016,15 @@ public function loadActivePlugins(): void // current request or the following five minutes. $instances = []; foreach ($activePlugins as $plugin) { + $pluginId = (int) $plugin['id']; + if (isset($this->skipPluginIdsThisRequest[$pluginId])) { + // The failed replacement class remains defined until this PHP + // request ends. The old files are already restored and will be + // loaded normally by the next request. + continue; + } try { - $instances[(int) $plugin['id']] = $this->instantiatePlugin($plugin); + $instances[$pluginId] = $this->instantiatePlugin($plugin); } catch (\Throwable $e) { SecureLogger::error("[PluginManager] Failed to load plugin '{$plugin['name']}'", ['error' => $e->getMessage()]); } @@ -1885,6 +2068,232 @@ public function loadActivePlugins(): void $this->hookManager->setPluginsLoadedRuntime(); } + private function finalizePendingPluginUpdates(): void + { + $pattern = $this->pluginsDir . DIRECTORY_SEPARATOR . self::PENDING_UPDATE_PREFIX . '*.json'; + $markers = glob($pattern) ?: []; + sort($markers); + + foreach ($markers as $markerPath) { + $state = null; + $handle = @fopen($markerPath, 'r+b'); + if ($handle === false) { + continue; + } + if (!flock($handle, LOCK_EX)) { + fclose($handle); + continue; + } + + try { + // Another request may have completed and unlinked this marker + // while this request was waiting for its lock. + if (!is_file($markerPath)) { + continue; + } + rewind($handle); + $raw = stream_get_contents($handle); + $state = is_string($raw) ? json_decode($raw, true, 512, JSON_THROW_ON_ERROR) : null; + if (!is_array($state)) { + throw new \RuntimeException('Stato di aggiornamento non valido.'); + } + + $this->finalizePendingPluginUpdate($state); + @unlink($markerPath); + } catch (\Throwable $e) { + if (is_array($state)) { + $this->rollbackPendingPluginUpdate($state, $e); + @unlink($markerPath); + } else { + SecureLogger::error('[PluginManager] Invalid pending plugin update marker', [ + 'path' => $markerPath, + 'error' => $e->getMessage(), + ]); + } + } finally { + flock($handle, LOCK_UN); + fclose($handle); + } + } + } + + /** @param array $state */ + private function finalizePendingPluginUpdate(array $state): void + { + $pluginId = (int) ($state['plugin_id'] ?? 0); + $directory = (string) ($state['directory'] ?? ''); + $newPlugin = $state['new_plugin'] ?? null; + if ($pluginId <= 0 || !$this->isSafePluginDirectoryName($directory) || !is_array($newPlugin)) { + throw new \RuntimeException('Stato di aggiornamento incompleto.'); + } + + $plugin = $this->getPlugin($pluginId); + if ($plugin === null || (string) ($plugin['path'] ?? '') !== $directory) { + throw new \RuntimeException('Il plugin aggiornato non è più registrato.'); + } + + // A crash between promoting the package and updating its DB row is + // recoverable because the marker contains the complete new metadata. + $this->applyPluginMetadataSnapshot($pluginId, $newPlugin); + $plugin = $this->getPlugin($pluginId); + if ($plugin === null) { + throw new \RuntimeException('Il plugin aggiornato non è più disponibile.'); + } + + if ((int) ($plugin['is_active'] ?? 0) !== 1) { + self::clearPluginCache(); + $this->deletePendingPluginBackup($state); + return; + } + + $instance = $this->instantiatePlugin($plugin); + if (method_exists($instance, 'onActivate')) { + $instance->onActivate(); + } + + self::clearPluginCache(); + SecureLogger::info('[PluginManager] Pending plugin update lifecycle completed', [ + 'plugin' => (string) ($state['plugin_name'] ?? ''), + 'plugin_id' => $pluginId, + 'version' => (string) ($newPlugin['version'] ?? ''), + ]); + // Destructive cleanup is last: any earlier exception can still restore + // the package from this backup. + $this->deletePendingPluginBackup($state); + } + + /** @param array $state */ + private function deletePendingPluginBackup(array $state): void + { + $directory = (string) ($state['directory'] ?? ''); + $backupDirectory = $state['backup_directory'] ?? null; + if (!is_string($backupDirectory) || $backupDirectory === '') { + return; + } + if (!$this->isSafePluginDirectoryName($directory) + || !preg_match('/^\.' . preg_quote($directory, '/') . '\.backup-[a-f0-9]{16}$/D', $backupDirectory) + ) { + throw new \RuntimeException('Percorso di backup del plugin non valido.'); + } + + $backupPath = $this->pluginsDir . DIRECTORY_SEPARATOR . $backupDirectory; + if (is_dir($backupPath) && !$this->deleteDirectory($backupPath)) { + SecureLogger::warning('[PluginManager] Completed plugin update backup could not be removed', [ + 'path' => $backupPath, + ]); + } + } + + /** @param array $state */ + private function rollbackPendingPluginUpdate(array $state, \Throwable $cause): void + { + $pluginId = (int) ($state['plugin_id'] ?? 0); + $directory = (string) ($state['directory'] ?? ''); + $backupDirectory = $state['backup_directory'] ?? null; + $oldPlugin = $state['old_plugin'] ?? null; + $oldHooks = $state['old_hooks'] ?? null; + $restored = false; + + if ($pluginId > 0 + && $this->isSafePluginDirectoryName($directory) + && is_string($backupDirectory) + && preg_match('/^\.' . preg_quote($directory, '/') . '\.backup-[a-f0-9]{16}$/D', $backupDirectory) + && is_array($oldPlugin) + && is_array($oldHooks) + ) { + $pluginPath = $this->pluginsDir . DIRECTORY_SEPARATOR . $directory; + $backupPath = $this->pluginsDir . DIRECTORY_SEPARATOR . $backupDirectory; + if (is_dir($backupPath)) { + if (is_dir($pluginPath)) { + $this->deleteDirectory($pluginPath); + } + if (rename($backupPath, $pluginPath)) { + try { + $this->applyPluginMetadataSnapshot($pluginId, $oldPlugin); + $this->restorePluginHooks($pluginId, $oldHooks); + $this->skipPluginIdsThisRequest[$pluginId] = true; + $restored = true; + } catch (\Throwable $rollbackError) { + SecureLogger::error('[PluginManager] Pending plugin DB rollback failed', [ + 'plugin_id' => $pluginId, + 'error' => $rollbackError->getMessage(), + ]); + } + } + } + } + + if (!$restored && $pluginId > 0) { + // With no trustworthy old package, fail closed: a broken active + // plugin must not be required on every request. + $stmt = $this->db->prepare('UPDATE plugins SET is_active = 0, activated_at = NULL WHERE id = ?'); + if ($stmt !== false) { + $stmt->bind_param('i', $pluginId); + $stmt->execute(); + $stmt->close(); + } + } + + self::clearPluginCache(); + SecureLogger::error('[PluginManager] Plugin lifecycle failed after ZIP update; rollback applied', [ + 'plugin' => (string) ($state['plugin_name'] ?? ''), + 'plugin_id' => $pluginId, + 'restored' => $restored, + 'error' => $cause->getMessage(), + ]); + } + + /** @param list> $hooks */ + private function restorePluginHooks(int $pluginId, array $hooks): void + { + $delete = $this->db->prepare('DELETE FROM plugin_hooks WHERE plugin_id = ?'); + if ($delete === false) { + throw new \RuntimeException('Impossibile ripristinare gli hook del plugin.'); + } + $delete->bind_param('i', $pluginId); + if (!$delete->execute()) { + $delete->close(); + throw new \RuntimeException('Impossibile ripristinare gli hook del plugin.'); + } + $delete->close(); + + if ($hooks === []) { + return; + } + $insert = $this->db->prepare( + 'INSERT INTO plugin_hooks ' + . '(plugin_id, hook_name, callback_class, callback_method, priority, is_active, created_at) ' + . 'VALUES (?, ?, ?, ?, ?, ?, ?)' + ); + if ($insert === false) { + throw new \RuntimeException('Impossibile ripristinare gli hook del plugin.'); + } + + foreach ($hooks as $hook) { + $hookName = (string) ($hook['hook_name'] ?? ''); + $callbackClass = (string) ($hook['callback_class'] ?? ''); + $callbackMethod = (string) ($hook['callback_method'] ?? ''); + $priority = (int) ($hook['priority'] ?? 10); + $isActive = (int) ($hook['is_active'] ?? 1); + $createdAt = (string) ($hook['created_at'] ?? date('Y-m-d H:i:s')); + $insert->bind_param( + 'isssiis', + $pluginId, + $hookName, + $callbackClass, + $callbackMethod, + $priority, + $isActive, + $createdAt + ); + if (!$insert->execute()) { + $insert->close(); + throw new \RuntimeException('Impossibile ripristinare gli hook del plugin.'); + } + } + $insert->close(); + } + /** * Invalidate the cross-request plugin caches. Must be called by every * plugin lifecycle mutation (install/activate/deactivate/uninstall). diff --git a/bin/build-release.sh b/bin/build-release.sh index 88380cced..536b7c328 100644 --- a/bin/build-release.sh +++ b/bin/build-release.sh @@ -243,6 +243,16 @@ verify_package_contents() { has_errors=true fi + # PHP session files may contain authenticated user data and CSRF tokens. + # Ship the writable directory only, never any runtime session payload. + local unexpected_session + unexpected_session=$(find "$package_dir/storage/sessions" -type f \ + ! -name '.gitkeep' -print -quit 2>/dev/null || true) + if [ -n "$unexpected_session" ]; then + log_error "Package contains runtime session data: ${unexpected_session#"$package_dir/"}" + has_errors=true + fi + # Files that MUST be in the package local required_files=( "public/index.php" diff --git a/scripts/ci-verify-release.sh b/scripts/ci-verify-release.sh index 03f103457..8de79a9d6 100755 --- a/scripts/ci-verify-release.sh +++ b/scripts/ci-verify-release.sh @@ -82,6 +82,10 @@ if find "$package_dir" -type f \( -name '*.pem' -o -name '*.key' -o -name 'id_rs echo "release contains a private-key or registry-credential file" >&2 exit 1 fi +if find "$package_dir/storage/sessions" -type f ! -name '.gitkeep' -print -quit 2>/dev/null | grep -q .; then + echo "release contains runtime session data" >&2 + exit 1 +fi echo "Checking release runtime and metadata" version="$(php -r 'echo json_decode(file_get_contents($argv[1]), true, 512, JSON_THROW_ON_ERROR)["version"];' "$package_dir/version.json")" diff --git a/tests/code-quality.spec.js b/tests/code-quality.spec.js index 8e2faf91c..0fa8d1ed4 100644 --- a/tests/code-quality.spec.js +++ b/tests/code-quality.spec.js @@ -209,6 +209,12 @@ test.describe.serial('Code Quality — 15 static analysis tests', () => { .not.toMatch(/REMOTE_PLUGIN_COUNT[^\n]*-ge\s+\d+/); expect(fs.readFileSync(path.join(ROOT, 'bin', 'build-release.sh'), 'utf-8')) .toContain('BundledPlugins::LIST declares ${#bundled_plugins[@]}'); + const releaseFilter = fs.readFileSync(path.join(ROOT, '.rsync-filter'), 'utf-8'); + const archiveVerifier = fs.readFileSync(path.join(ROOT, 'scripts', 'ci-verify-release.sh'), 'utf-8'); + expect(releaseFilter, 'runtime PHP sessions must never enter a release package') + .toContain('- /storage/sessions/*'); + expect(archiveVerifier, 'archive verification must reject leaked runtime sessions') + .toContain('release contains runtime session data'); }); // ── 3. Plugin ensureSchema() called from onActivate() ───────────────────── diff --git a/tests/helpers/plugin-zip-update-bootstrap.php b/tests/helpers/plugin-zip-update-bootstrap.php new file mode 100644 index 000000000..e8aecaacd --- /dev/null +++ b/tests/helpers/plugin-zip-update-bootstrap.php @@ -0,0 +1,49 @@ += 2 && ($value[0] === '"' || $value[0] === "'") && $value[-1] === $value[0]) { + $value = substr($value, 1, -1); + } + $values[trim($key)] = $value; + } + return $values; +} + +$env = pzub_env(__DIR__ . '/../../.env'); +$socket = getenv('E2E_DB_SOCKET') ?: ($env['DB_SOCKET'] ?? ''); +$user = getenv('E2E_DB_USER') ?: ($env['DB_USER'] ?? ''); +$password = getenv('E2E_DB_PASS') ?: ($env['DB_PASS'] ?? ($env['DB_PASSWORD'] ?? '')); +$database = getenv('E2E_DB_NAME') ?: ($env['DB_NAME'] ?? ''); + +mysqli_report(MYSQLI_REPORT_OFF); +$db = (is_string($socket) && $socket !== '' && file_exists($socket)) + ? @new mysqli(null, $user, $password, $database, 0, $socket) + : @new mysqli($env['DB_HOST'] ?? '127.0.0.1', $user, $password, $database, (int) ($env['DB_PORT'] ?? 3306)); +if ($db->connect_errno !== 0) { + fwrite(STDERR, "Database not reachable during plugin update bootstrap.\n"); + exit(2); +} + +try { + $manager = new \App\Support\PluginManager($db, new \App\Support\HookManager($db)); + $manager->loadActivePlugins(); +} finally { + $db->close(); +} diff --git a/tests/plugin-manager.unit.php b/tests/plugin-manager.unit.php index 5cf44a8c3..d7f25f24e 100644 --- a/tests/plugin-manager.unit.php +++ b/tests/plugin-manager.unit.php @@ -45,6 +45,16 @@ $check($source !== false && str_contains($source, 'UPDATE plugins SET display_name'), 'update persists new manifest metadata'); $check($source !== false && str_contains($source, "'updated' => true"), 'update response identifies a successful update'); $check($source !== false && str_contains($source, 'isSafePluginFilePath'), 'manifest main_file is validated against traversal'); +$check($source !== false && str_contains($source, '$this->finalizePendingPluginUpdates();'), 'fresh bootstrap finalizes active ZIP updates'); +$check($source !== false && str_contains($source, '$instance->onActivate();'), 'fresh bootstrap runs the replacement lifecycle'); +$check($source !== false && str_contains($source, '$this->restorePluginHooks($pluginId, $oldHooks);'), 'failed replacement lifecycle restores prior hook rows'); +$check($source !== false && str_contains($source, '$this->skipPluginIdsThisRequest[$pluginId] = true;'), 'rolled-back replacement class is skipped for the rest of its PHP request'); +$finalizeAt = $source !== false ? strpos($source, '$this->finalizePendingPluginUpdates();') : false; +$maintenanceAt = $source !== false ? strpos($source, '$maintenanceKey =', $finalizeAt === false ? 0 : $finalizeAt) : false; +$check( + $finalizeAt !== false && $maintenanceAt !== false && $finalizeAt < $maintenanceAt, + 'pending lifecycle completes before maintenance and active hook loading' +); echo "\n================================\n"; echo "Passed: $passed Failed: $failed\n"; diff --git a/tests/plugin-zip-update-all-bundled.integration.php b/tests/plugin-zip-update-all-bundled.integration.php index ddc8e0e37..b1f100dbb 100644 --- a/tests/plugin-zip-update-all-bundled.integration.php +++ b/tests/plugin-zip-update-all-bundled.integration.php @@ -15,9 +15,9 @@ * 4. restores the directory byte-for-byte and the plugins-row version, so a * developer/CI database and working tree are left exactly as found. * - * The update path swaps directories and rewrites metadata; it does not - * instantiate the plugin or run ensureSchema, so this is safe to run for every - * plugin in one process. + * Active-plugin lifecycle is intentionally deferred to a fresh bootstrap. This + * file only checks package compatibility for every bundled plugin; the + * disposable plugin integration test exercises lifecycle success and rollback. * * Run: php tests/plugin-zip-update-all-bundled.integration.php */ @@ -205,6 +205,7 @@ function pzua_zip_plugin(string $pluginDir, string $slug, string $bumpedVersion) foreach (glob($pluginsDir . '/.' . $slug . '.staging-*') ?: [] as $stray) { pzua_rmdir($stray); } + @unlink($pluginsDir . '/.pinakes-plugin-update-' . $pluginId . '.json'); } } diff --git a/tests/plugin-zip-update.integration.php b/tests/plugin-zip-update.integration.php index 23e80d1d8..36f7d99aa 100644 --- a/tests/plugin-zip-update.integration.php +++ b/tests/plugin-zip-update.integration.php @@ -45,7 +45,14 @@ function pzu_delete_directory(string $directory): void } /** @return string ZIP path */ -function pzu_create_zip(string $slug, string $className, string $version, string $displayName): string +function pzu_create_zip( + string $slug, + string $className, + string $version, + string $displayName, + string $lifecycle, + string $tableName +): string { $zipPath = tempnam(sys_get_temp_dir(), 'pinakes-plugin-update-'); if ($zipPath === false) { @@ -63,15 +70,136 @@ function pzu_create_zip(string $slug, string $className, string $version, string 'requires_php' => '8.2', 'metadata' => ['test_package' => true], ], JSON_THROW_ON_ERROR); - $wrapper = "pluginId === null) { + throw new RuntimeException('plugin id missing'); + } + if (!$this->db->query('CREATE TABLE IF NOT EXISTS `{{TABLE}}` (`id` INT NOT NULL PRIMARY KEY) ENGINE=InnoDB')) { + throw new RuntimeException('schema update failed'); + } + $delete = $this->db->prepare('DELETE FROM plugin_hooks WHERE plugin_id = ?'); + $delete->bind_param('i', $this->pluginId); + if (!$delete->execute()) { + throw new RuntimeException('hook cleanup failed'); + } + $delete->close(); + $hookName = 'test.update.v2'; + $callbackClass = self::class; + $callbackMethod = 'handleUpdated'; + $priority = 7; + $insert = $this->db->prepare( + 'INSERT INTO plugin_hooks (plugin_id, hook_name, callback_class, callback_method, priority, is_active) ' + . 'VALUES (?, ?, ?, ?, ?, 1)' + ); + $insert->bind_param('isssi', $this->pluginId, $hookName, $callbackClass, $callbackMethod, $priority); + if (!$insert->execute()) { + throw new RuntimeException('hook registration failed'); + } + $insert->close(); + } + + public function handleUpdated(): void + { + } +PHP; + } elseif ($lifecycle === 'failure') { + $lifecycleMethod = <<<'PHP' + public function onActivate(): void + { + if ($this->pluginId === null) { + throw new RuntimeException('plugin id missing'); + } + $hookName = 'test.update.partial'; + $callbackClass = self::class; + $callbackMethod = 'handlePartial'; + $priority = 3; + $insert = $this->db->prepare( + 'INSERT INTO plugin_hooks (plugin_id, hook_name, callback_class, callback_method, priority, is_active) ' + . 'VALUES (?, ?, ?, ?, ?, 1)' + ); + $insert->bind_param('isssi', $this->pluginId, $hookName, $callbackClass, $callbackMethod, $priority); + $insert->execute(); + $insert->close(); + throw new RuntimeException('intentional lifecycle failure'); + } + + public function handlePartial(): void + { + } +PHP; + } else { + $lifecycleMethod = <<<'PHP' + public function handleLegacy(): void + { + } +PHP; + } + + $wrapperTemplate = <<<'PHP' +db = $db; + } + + public function setPluginId(int $pluginId): void + { + $this->pluginId = $pluginId; + } + +{{LIFECYCLE}} +} +PHP; + $wrapper = strtr($wrapperTemplate, [ + '{{CLASS}}' => $className, + '{{TABLE}}' => $tableName, + '{{LIFECYCLE}}' => str_replace('{{TABLE}}', $tableName, $lifecycleMethod), + ]); $zip->addFromString($slug . '/plugin.json', $manifest); $zip->addFromString($slug . '/wrapper.php', $wrapper); $zip->close(); return $zipPath; } +function pzu_run_fresh_bootstrap(): void +{ + $process = proc_open( + [PHP_BINARY, __DIR__ . '/helpers/plugin-zip-update-bootstrap.php'], + [ + 0 => ['pipe', 'r'], + 1 => ['pipe', 'w'], + 2 => ['pipe', 'w'], + ], + $pipes, + dirname(__DIR__) + ); + if (!is_resource($process)) { + throw new RuntimeException('Unable to start fresh plugin bootstrap process.'); + } + fclose($pipes[0]); + $stdout = stream_get_contents($pipes[1]); + $stderr = stream_get_contents($pipes[2]); + fclose($pipes[1]); + fclose($pipes[2]); + $exitCode = proc_close($process); + if ($exitCode !== 0) { + throw new RuntimeException( + 'Fresh plugin bootstrap failed: ' . trim((string) $stdout . "\n" . (string) $stderr) + ); + } +} + $env = pzu_env(__DIR__ . '/../.env'); $socket = getenv('E2E_DB_SOCKET') ?: ($env['DB_SOCKET'] ?? ''); $user = getenv('E2E_DB_USER') ?: ($env['DB_USER'] ?? ''); @@ -96,13 +224,16 @@ function pzu_create_zip(string $slug, string $className, string $version, string $className = 'PluginZipUpdate' . ucfirst($suffix) . 'Plugin'; $pluginsDir = dirname(__DIR__) . '/storage/plugins'; $pluginDir = $pluginsDir . '/' . $slug; +$pendingMarker = $pluginsDir . '/.pinakes-plugin-update-'; +$tableName = 'plugin_zip_update_' . $suffix; $zipV1 = null; $zipV2 = null; +$zipV3 = null; $pluginId = 0; try { $manager = new \App\Support\PluginManager($db, new \App\Support\HookManager($db)); - $zipV1 = pzu_create_zip($slug, $className, '1.0.0', 'Disposable plugin v1'); + $zipV1 = pzu_create_zip($slug, $className, '1.0.0', 'Disposable plugin v1', 'none', $tableName); $firstInstall = $manager->installFromZip($zipV1); if (($firstInstall['success'] ?? false) !== true) { throw new RuntimeException('Initial install failed: ' . ($firstInstall['message'] ?? 'unknown error')); @@ -115,9 +246,9 @@ function pzu_create_zip(string $slug, string $className, string $version, string $db->query("UPDATE plugins SET is_active = 1 WHERE id = {$pluginId}"); $db->query("INSERT INTO plugin_settings (plugin_id, setting_key, setting_value, autoload) VALUES ({$pluginId}, 'kept_setting', 'kept value', 1)"); $db->query("INSERT INTO plugin_data (plugin_id, data_key, data_value, data_type) VALUES ({$pluginId}, 'kept_data', 'kept value', 'string')"); - $db->query("INSERT INTO plugin_hooks (plugin_id, hook_name, callback_class, callback_method, priority, is_active) VALUES ({$pluginId}, 'test.update', '{$className}', 'handleUpdate', 10, 1)"); + $db->query("INSERT INTO plugin_hooks (plugin_id, hook_name, callback_class, callback_method, priority, is_active) VALUES ({$pluginId}, 'test.update.legacy', '{$className}', 'handleLegacy', 10, 1)"); - $zipV2 = pzu_create_zip($slug, $className, '1.1.0', 'Disposable plugin v2'); + $zipV2 = pzu_create_zip($slug, $className, '1.1.0', 'Disposable plugin v2', 'success', $tableName); $update = $manager->installFromZip($zipV2); if (($update['success'] ?? false) !== true || ($update['updated'] ?? false) !== true) { throw new RuntimeException('ZIP update failed: ' . ($update['message'] ?? 'unknown error')); @@ -129,29 +260,99 @@ function pzu_create_zip(string $slug, string $className, string $version, string $row = $db->query("SELECT version, display_name, is_active FROM plugins WHERE id = {$pluginId}")->fetch_assoc(); $settings = (int) $db->query("SELECT COUNT(*) FROM plugin_settings WHERE plugin_id = {$pluginId} AND setting_key = 'kept_setting'")->fetch_row()[0]; $data = (int) $db->query("SELECT COUNT(*) FROM plugin_data WHERE plugin_id = {$pluginId} AND data_key = 'kept_data'")->fetch_row()[0]; - $hooks = (int) $db->query("SELECT COUNT(*) FROM plugin_hooks WHERE plugin_id = {$pluginId} AND hook_name = 'test.update'")->fetch_row()[0]; + $legacyHooksBeforeBootstrap = (int) $db->query("SELECT COUNT(*) FROM plugin_hooks WHERE plugin_id = {$pluginId} AND hook_name = 'test.update.legacy'")->fetch_row()[0]; if (!is_array($row) || $row['version'] !== '1.1.0' || $row['display_name'] !== 'Disposable plugin v2') { throw new RuntimeException('ZIP update did not persist the replacement manifest.'); } - if ((int) $row['is_active'] !== 1 || $settings !== 1 || $data !== 1 || $hooks !== 1) { + if ((int) $row['is_active'] !== 1 || $settings !== 1 || $data !== 1 || $legacyHooksBeforeBootstrap !== 1) { throw new RuntimeException('ZIP update did not preserve plugin state and related data.'); } if (!is_file($pluginDir . '/wrapper.php')) { throw new RuntimeException('ZIP update did not promote the replacement package.'); } - echo "PASS: existing plugin ZIP update keeps ID, active state, settings, data and hooks\n"; + $pendingMarker .= $pluginId . '.json'; + if (!is_file($pendingMarker)) { + throw new RuntimeException('Active ZIP update did not persist a deferred lifecycle marker.'); + } + $schemaBeforeBootstrap = (int) $db->query( + "SELECT COUNT(*) FROM information_schema.TABLES WHERE TABLE_SCHEMA = DATABASE() AND TABLE_NAME = '{$tableName}'" + )->fetch_row()[0]; + if ($schemaBeforeBootstrap !== 0) { + throw new RuntimeException('Replacement lifecycle ran in the process that still owns the old class.'); + } + + pzu_run_fresh_bootstrap(); + + $schemaAfterBootstrap = (int) $db->query( + "SELECT COUNT(*) FROM information_schema.TABLES WHERE TABLE_SCHEMA = DATABASE() AND TABLE_NAME = '{$tableName}'" + )->fetch_row()[0]; + $legacyHooks = (int) $db->query("SELECT COUNT(*) FROM plugin_hooks WHERE plugin_id = {$pluginId} AND hook_name = 'test.update.legacy'")->fetch_row()[0]; + $updatedHooks = (int) $db->query("SELECT COUNT(*) FROM plugin_hooks WHERE plugin_id = {$pluginId} AND hook_name = 'test.update.v2' AND callback_method = 'handleUpdated'")->fetch_row()[0]; + if ($schemaAfterBootstrap !== 1 || $legacyHooks !== 0 || $updatedHooks !== 1) { + throw new RuntimeException('Fresh bootstrap did not apply replacement schema and hook lifecycle.'); + } + if (is_file($pendingMarker) || (glob($pluginsDir . '/.' . $slug . '.backup-*') ?: []) !== []) { + throw new RuntimeException('Successful replacement lifecycle left update recovery files behind.'); + } + + // A later update whose lifecycle fails must restore the v2 files, metadata + // and hook rows instead of leaving a half-upgraded active plugin behind. + $zipV3 = pzu_create_zip($slug, $className, '1.2.0', 'Disposable plugin v3', 'failure', $tableName); + $failingUpdate = $manager->installFromZip($zipV3); + if (($failingUpdate['success'] ?? false) !== true || !is_file($pendingMarker)) { + throw new RuntimeException('Unable to stage the failing lifecycle rollback case.'); + } + pzu_run_fresh_bootstrap(); + + $rolledBack = $db->query("SELECT version, display_name, is_active FROM plugins WHERE id = {$pluginId}")->fetch_assoc(); + $updatedHooksAfterRollback = (int) $db->query("SELECT COUNT(*) FROM plugin_hooks WHERE plugin_id = {$pluginId} AND hook_name = 'test.update.v2' AND callback_method = 'handleUpdated' AND is_active = 1")->fetch_row()[0]; + $partialHooks = (int) $db->query("SELECT COUNT(*) FROM plugin_hooks WHERE plugin_id = {$pluginId} AND hook_name = 'test.update.partial'")->fetch_row()[0]; + $restoredSource = (string) file_get_contents($pluginDir . '/wrapper.php'); + if (!is_array($rolledBack) + || $rolledBack['version'] !== '1.1.0' + || $rolledBack['display_name'] !== 'Disposable plugin v2' + || (int) $rolledBack['is_active'] !== 1 + || $updatedHooksAfterRollback !== 1 + || $partialHooks !== 0 + || !str_contains($restoredSource, 'handleUpdated') + || str_contains($restoredSource, 'intentional lifecycle failure') + ) { + throw new RuntimeException('Failed replacement lifecycle did not restore package, metadata and hooks.'); + } + // The request that loaded the broken v3 class must skip the rolled-back + // plugin. A second fresh request can now load v2 and keep its hook active. + pzu_run_fresh_bootstrap(); + $activeHooksOnNextRequest = (int) $db->query("SELECT COUNT(*) FROM plugin_hooks WHERE plugin_id = {$pluginId} AND hook_name = 'test.update.v2' AND callback_method = 'handleUpdated' AND is_active = 1")->fetch_row()[0]; + if ($activeHooksOnNextRequest !== 1) { + throw new RuntimeException('Next request did not load the restored plugin class and hook cleanly.'); + } + if (is_file($pendingMarker) || (glob($pluginsDir . '/.' . $slug . '.backup-*') ?: []) !== []) { + throw new RuntimeException('Lifecycle rollback left update recovery files behind.'); + } + + echo "PASS: ZIP update defers lifecycle to a fresh process and rolls back package, metadata and hooks on failure\n"; } finally { if ($pluginId > 0) { $db->query("DELETE FROM plugins WHERE id = {$pluginId}"); } + $db->query("DROP TABLE IF EXISTS `{$tableName}`"); pzu_delete_directory($pluginDir); + if ($pluginId > 0) { + @unlink($pluginsDir . '/.pinakes-plugin-update-' . $pluginId . '.json'); + } + foreach (glob($pluginsDir . '/.' . $slug . '.backup-*') ?: [] as $backup) { + pzu_delete_directory($backup); + } if (is_string($zipV1)) { @unlink($zipV1); } if (is_string($zipV2)) { @unlink($zipV2); } + if (is_string($zipV3)) { + @unlink($zipV3); + } $db->close(); } From 92898b95ba7dde722c1b95e22d511cfa60228a0e Mon Sep 17 00:00:00 2001 From: fabiodalez-dev Date: Wed, 19 Aug 2026 15:55:34 +0200 Subject: [PATCH 02/16] fix(plugins): retire unreadable update markers and harden verifiers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address review findings on the deferred plugin-update lifecycle: - finalizePendingPluginUpdates(): an unreadable marker ($state null) was only logged, never removed. createPendingPluginUpdateMarker() opens with fopen('x+b'), so the leftover marker made every future update of that plugin fail with 'already pending' — a permanent brick until manual FS cleanup. Retire the corrupted file under a non-.json name for diagnosis, unlink if the rename fails. - deletePendingPluginBackup(): stop throwing on an unsafe backup path. It runs after the update is applied and (re)activated; a throw propagated into the finalize catch and rolled back a committed update. Log and keep the orphaned backup instead. - build-release.sh + ci-verify-release.sh: enforce the same storage/sessions tree in both verifiers — require .gitkeep and reject every other entry (files, symlinks, stray dirs), not just plain files. - plugin-manager.unit.php: two assertions could not fail (onActivate string is shared with activatePlugin; strpos offset made the order check tautological). Anchor onActivate inside finalizePendingPluginUpdate and compare real positions; add guards for the two fixes above. --- app/Support/PluginManager.php | 19 ++++++++++++++++++- bin/build-release.sh | 11 ++++++++--- scripts/ci-verify-release.sh | 5 +++-- tests/plugin-manager.unit.php | 26 ++++++++++++++++++++++++-- 4 files changed, 53 insertions(+), 8 deletions(-) diff --git a/app/Support/PluginManager.php b/app/Support/PluginManager.php index e19de37b1..035a852d5 100644 --- a/app/Support/PluginManager.php +++ b/app/Support/PluginManager.php @@ -2109,6 +2109,15 @@ private function finalizePendingPluginUpdates(): void 'path' => $markerPath, 'error' => $e->getMessage(), ]); + // An unreadable marker holds no state to roll back, but it + // must not brick future updates: createPendingPluginUpdateMarker() + // opens with fopen('x+b'), so a leftover marker makes every + // later update of this plugin fail with "already pending". + // Keep the corrupted file for diagnosis under a name that no + // longer matches the *.json marker glob; unlink if that fails. + if (!@rename($markerPath, $markerPath . '.invalid-' . time())) { + @unlink($markerPath); + } } } finally { flock($handle, LOCK_UN); @@ -2173,7 +2182,15 @@ private function deletePendingPluginBackup(array $state): void if (!$this->isSafePluginDirectoryName($directory) || !preg_match('/^\.' . preg_quote($directory, '/') . '\.backup-[a-f0-9]{16}$/D', $backupDirectory) ) { - throw new \RuntimeException('Percorso di backup del plugin non valido.'); + // Best-effort cleanup only: this runs AFTER the update is already + // applied and (re)activated. Throwing here would propagate into + // finalizePendingPluginUpdates()'s catch and roll back a committed + // update — a far worse outcome than an orphaned backup directory. + SecureLogger::warning('[PluginManager] Unsafe backup path on completed update; leaving backup in place', [ + 'directory' => $directory, + 'backup_directory' => $backupDirectory, + ]); + return; } $backupPath = $this->pluginsDir . DIRECTORY_SEPARATOR . $backupDirectory; diff --git a/bin/build-release.sh b/bin/build-release.sh index 536b7c328..6a4eeb3e5 100644 --- a/bin/build-release.sh +++ b/bin/build-release.sh @@ -244,12 +244,17 @@ verify_package_contents() { fi # PHP session files may contain authenticated user data and CSRF tokens. - # Ship the writable directory only, never any runtime session payload. + # Ship the writable directory as exactly {.gitkeep}: require the placeholder + # and reject every other entry — session payloads, symlinks, and stray dirs. + if [ ! -f "$package_dir/storage/sessions/.gitkeep" ]; then + log_error "Package missing storage/sessions/.gitkeep" + has_errors=true + fi local unexpected_session - unexpected_session=$(find "$package_dir/storage/sessions" -type f \ + unexpected_session=$(find "$package_dir/storage/sessions" -mindepth 1 \ ! -name '.gitkeep' -print -quit 2>/dev/null || true) if [ -n "$unexpected_session" ]; then - log_error "Package contains runtime session data: ${unexpected_session#"$package_dir/"}" + log_error "Package contains unexpected storage/sessions entry: ${unexpected_session#"$package_dir/"}" has_errors=true fi diff --git a/scripts/ci-verify-release.sh b/scripts/ci-verify-release.sh index 8de79a9d6..20fb40d0b 100755 --- a/scripts/ci-verify-release.sh +++ b/scripts/ci-verify-release.sh @@ -82,8 +82,9 @@ if find "$package_dir" -type f \( -name '*.pem' -o -name '*.key' -o -name 'id_rs echo "release contains a private-key or registry-credential file" >&2 exit 1 fi -if find "$package_dir/storage/sessions" -type f ! -name '.gitkeep' -print -quit 2>/dev/null | grep -q .; then - echo "release contains runtime session data" >&2 +[ -f "$package_dir/storage/sessions/.gitkeep" ] || { echo "release missing storage/sessions/.gitkeep" >&2; exit 1; } +if find "$package_dir/storage/sessions" -mindepth 1 ! -name '.gitkeep' -print -quit 2>/dev/null | grep -q .; then + echo "release contains unexpected storage/sessions entry" >&2 exit 1 fi diff --git a/tests/plugin-manager.unit.php b/tests/plugin-manager.unit.php index d7f25f24e..f179fa4c8 100644 --- a/tests/plugin-manager.unit.php +++ b/tests/plugin-manager.unit.php @@ -46,16 +46,38 @@ $check($source !== false && str_contains($source, "'updated' => true"), 'update response identifies a successful update'); $check($source !== false && str_contains($source, 'isSafePluginFilePath'), 'manifest main_file is validated against traversal'); $check($source !== false && str_contains($source, '$this->finalizePendingPluginUpdates();'), 'fresh bootstrap finalizes active ZIP updates'); -$check($source !== false && str_contains($source, '$instance->onActivate();'), 'fresh bootstrap runs the replacement lifecycle'); +$finalizeMethodAt = $source !== false ? strpos($source, 'private function finalizePendingPluginUpdate(array $state)') : false; +$finalizeEndAt = $finalizeMethodAt !== false ? strpos($source, 'private function deletePendingPluginBackup', $finalizeMethodAt) : false; +$onActivateInFinalizeAt = $finalizeMethodAt !== false ? strpos($source, '$instance->onActivate();', $finalizeMethodAt) : false; +$check( + $onActivateInFinalizeAt !== false && $finalizeEndAt !== false && $onActivateInFinalizeAt < $finalizeEndAt, + 'fresh bootstrap runs the replacement lifecycle (onActivate inside finalizePendingPluginUpdate)' +); $check($source !== false && str_contains($source, '$this->restorePluginHooks($pluginId, $oldHooks);'), 'failed replacement lifecycle restores prior hook rows'); $check($source !== false && str_contains($source, '$this->skipPluginIdsThisRequest[$pluginId] = true;'), 'rolled-back replacement class is skipped for the rest of its PHP request'); $finalizeAt = $source !== false ? strpos($source, '$this->finalizePendingPluginUpdates();') : false; -$maintenanceAt = $source !== false ? strpos($source, '$maintenanceKey =', $finalizeAt === false ? 0 : $finalizeAt) : false; +$maintenanceAt = $source !== false ? strpos($source, '$maintenanceKey =') : false; $check( $finalizeAt !== false && $maintenanceAt !== false && $finalizeAt < $maintenanceAt, 'pending lifecycle completes before maintenance and active hook loading' ); +echo "\nPending marker resilience:\n"; +// An unreadable marker must be renamed/unlinked, else fopen('x+b') bricks every +// future update of that plugin with "already pending". +$check( + $source !== false && str_contains($source, "@rename(\$markerPath, \$markerPath . '.invalid-'"), + 'unreadable marker is retired so it cannot block future updates' +); +// Post-commit backup cleanup must never throw: a throw would propagate into the +// finalize catch and roll back an already-applied, already-activated update. +$check( + $source !== false + && str_contains($source, 'leaving backup in place') + && !str_contains($source, 'Percorso di backup del plugin non valido'), + 'completed-update backup cleanup logs instead of throwing' +); + echo "\n================================\n"; echo "Passed: $passed Failed: $failed\n"; exit($failed > 0 ? 1 : 0); From 226df2537ff525a6c231150f3f64aaf27a515773 Mon Sep 17 00:00:00 2001 From: fabiodalez-dev Date: Wed, 19 Aug 2026 16:11:11 +0200 Subject: [PATCH 03/16] docs(changelog): note active-plugin update lifecycle and verifier hardening The 0.7.62 entry covered the plugin ZIP-update feature (#358) but not the lifecycle correctness added while hardening it: an active plugin's update now runs its new onActivate()/ensureSchema() on the next request with rollback on failure, and the release verifiers reject any non-.gitkeep storage/sessions entry. Document both under 0.7.62 so the release notes are complete. --- CHANGELOG.md | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a621b497d..4100f1aa4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -40,7 +40,11 @@ Overdue-loan recalls (solleciti) and emailing the loan receipt (#360). - **Plugin ZIP updates (#358)**: uploading a plugin ZIP whose name matches an already-installed plugin now updates it in place — its id, settings, data and hooks are preserved and its files are swapped atomically — instead of failing - on the existing directory. Covered by contract and per-bundled-plugin + on the existing directory. Updating an already-active plugin now runs its new + lifecycle (`onActivate()`/`ensureSchema()`) on the next request via a + pending-update marker, rolling back package, metadata and hooks if the new + version fails to activate — so schema changes shipped in an update are applied + instead of being silently skipped. Covered by contract and per-bundled-plugin integration tests. ### Internal @@ -48,6 +52,10 @@ Overdue-loan recalls (solleciti) and emailing the loan receipt (#360). - CI: the OWASP ZAP baseline no longer fails on the ISBN/EAN-13 PII-disclosure false positive, allowlisted narrowly to 13-digit codes on bibliographic pages (#359). +- Release verifiers now require `storage/sessions/.gitkeep` and reject every + other entry there (files, symlinks, stray directories), and an unreadable + plugin-update marker is retired instead of permanently blocking future + updates of that plugin. ## [0.7.61] From bc4f05e89a05be14d81ce9d76378d8d210d52ed7 Mon Sep 17 00:00:00 2001 From: fabiodalez-dev Date: Wed, 19 Aug 2026 17:11:01 +0200 Subject: [PATCH 04/16] fix(release): ship storage/sessions/.gitkeep and stabilise plugin-update tests Full CI on the #364 delta surfaced three real problems the pre-merge run missed (its checks had run against the already-merged 57baf0d0 tree, not the delta): - storage/sessions/.gitkeep was never created, even though .rsync-filter and .distignore were configured to ship it and the release verifier now requires it. Create the empty placeholder (force-added past the global **/.gitkeep ignore, like the sibling storage/*/.gitkeep files) and mirror the per-dir un-ignore rules in .gitignore. The package now ships an empty writable storage/sessions/ with its placeholder, fixing the reproducible-release build and the packaged-app job. - ci-verify-release.sh kept the reject-any-non-.gitkeep hardening (which now also catches symlinks and stray dirs via -mindepth 1) but restored the original 'release contains runtime session data' message, which the static code-quality guard asserts verbatim; build-release.sh message restored to match. Fixes the Full E2E and browser-regression shards. - plugin-zip-update.integration.php: the parent process checked for recovery files (marker/backup) deleted by the child bootstrap process, but PHP's stat cache only clears on same-process FS calls, so is_file() returned a stale hit on Linux/PHP 8.2 (CI) though macOS/PHP 8.4 masked it. clearstatcache(true) after the child finishes. Fixes the static-quality integration run. --- .gitignore | 3 +++ bin/build-release.sh | 2 +- scripts/ci-verify-release.sh | 2 +- storage/sessions/.gitkeep | 0 tests/plugin-zip-update.integration.php | 7 +++++++ 5 files changed, 12 insertions(+), 2 deletions(-) create mode 100644 storage/sessions/.gitkeep diff --git a/.gitignore b/.gitignore index 9b13bb7c7..966a427a6 100644 --- a/.gitignore +++ b/.gitignore @@ -73,6 +73,9 @@ storage/* !storage/logs/ storage/logs/* !storage/logs/.gitkeep +!storage/sessions/ +storage/sessions/* +!storage/sessions/.gitkeep storage/*.log !storage/tmp/ storage/tmp/* diff --git a/bin/build-release.sh b/bin/build-release.sh index 6a4eeb3e5..04b37e2bb 100644 --- a/bin/build-release.sh +++ b/bin/build-release.sh @@ -254,7 +254,7 @@ verify_package_contents() { unexpected_session=$(find "$package_dir/storage/sessions" -mindepth 1 \ ! -name '.gitkeep' -print -quit 2>/dev/null || true) if [ -n "$unexpected_session" ]; then - log_error "Package contains unexpected storage/sessions entry: ${unexpected_session#"$package_dir/"}" + log_error "Package contains runtime session data: ${unexpected_session#"$package_dir/"}" has_errors=true fi diff --git a/scripts/ci-verify-release.sh b/scripts/ci-verify-release.sh index 20fb40d0b..31e5568aa 100755 --- a/scripts/ci-verify-release.sh +++ b/scripts/ci-verify-release.sh @@ -84,7 +84,7 @@ if find "$package_dir" -type f \( -name '*.pem' -o -name '*.key' -o -name 'id_rs fi [ -f "$package_dir/storage/sessions/.gitkeep" ] || { echo "release missing storage/sessions/.gitkeep" >&2; exit 1; } if find "$package_dir/storage/sessions" -mindepth 1 ! -name '.gitkeep' -print -quit 2>/dev/null | grep -q .; then - echo "release contains unexpected storage/sessions entry" >&2 + echo "release contains runtime session data" >&2 exit 1 fi diff --git a/storage/sessions/.gitkeep b/storage/sessions/.gitkeep new file mode 100644 index 000000000..e69de29bb diff --git a/tests/plugin-zip-update.integration.php b/tests/plugin-zip-update.integration.php index 36f7d99aa..b2555790a 100644 --- a/tests/plugin-zip-update.integration.php +++ b/tests/plugin-zip-update.integration.php @@ -198,6 +198,13 @@ function pzu_run_fresh_bootstrap(): void 'Fresh plugin bootstrap failed: ' . trim((string) $stdout . "\n" . (string) $stderr) ); } + + // The child process (finalizePendingPluginUpdates) deletes the marker and + // backup directory. PHP's stat cache is only invalidated by filesystem + // calls made in THIS process, so without an explicit clear the parent still + // sees the pre-bootstrap state for is_file()/is_dir() checks — a stale hit + // that surfaces on Linux/PHP 8.2 (CI) even though macOS/PHP 8.4 masks it. + clearstatcache(true); } $env = pzu_env(__DIR__ . '/../.env'); From bbaf1a0d3803c22c7225723f9c49badcf3183703 Mon Sep 17 00:00:00 2001 From: fabiodalez-dev Date: Wed, 19 Aug 2026 18:10:15 +0200 Subject: [PATCH 05/16] fix(plugins): address review findings on the deferred-update lifecycle Seven CodeRabbit findings on the #364 delta: - skipPluginIdsThisRequest is now static so the 'skip the rolled-back plugin for the rest of the request' invariant holds across every PluginManager instance in the process, not just the one that rolled back (the broken replacement class stays defined process-wide). - restorePluginHooks() wraps its DELETE + re-INSERT in a transaction (only when one is not already open, detected via @@autocommit, to avoid nesting) so a mid-loop INSERT failure cannot leave plugin_hooks with a partial set. - The finalize loop's unreadable-open / lock-failure paths now log instead of failing silently. They still retry rather than retiring the marker: retiring on a transient error would drop the deferred onActivate (the schema migration this mechanism exists to run); a permanent failure is now diagnosable. - Both release verifiers are fail-closed: a non-zero find status aborts instead of being read as 'no session data' (the || true / pipe-to-grep swallowed it). - PENDING_UPDATE_PREFIX is public so the integration tests reference it instead of duplicating the '.pinakes-plugin-update-' literal in three places. - The fresh-bootstrap helper routes child stderr to a file so a large stack trace can no longer deadlock the parent (stdout pipe full / child blocked on stderr). - The third-bootstrap assertion now checks the rolled-back plugin's state is unchanged by a later request (version/is_active/hooks), and its comment no longer claims to test the intra-process skip set, which a separate process cannot observe. --- app/Support/PluginManager.php | 136 ++++++++++++------ bin/build-release.sh | 9 +- scripts/ci-verify-release.sh | 8 +- tests/plugin-manager.unit.php | 2 +- ...gin-zip-update-all-bundled.integration.php | 2 +- tests/plugin-zip-update.integration.php | 37 +++-- 6 files changed, 132 insertions(+), 62 deletions(-) diff --git a/app/Support/PluginManager.php b/app/Support/PluginManager.php index 035a852d5..0dfc835e7 100644 --- a/app/Support/PluginManager.php +++ b/app/Support/PluginManager.php @@ -15,7 +15,8 @@ class PluginManager { private const MAX_UPLOAD_BYTES = 104857600; // 100 MB - private const PENDING_UPDATE_PREFIX = '.pinakes-plugin-update-'; + /** Filename prefix for deferred plugin-update markers; public so tests reference it instead of duplicating the literal. */ + public const PENDING_UPDATE_PREFIX = '.pinakes-plugin-update-'; private mysqli $db; private string $pluginsDir; private string $uploadsDir; @@ -23,8 +24,13 @@ class PluginManager private ?string $cachedEncryptionKey = null; private bool $encryptionKeyResolved = false; - /** @var array Plugins rolled back after their replacement class was loaded. */ - private array $skipPluginIdsThisRequest = []; + /** + * @var array Plugins rolled back after their replacement class was + * loaded. Static so the skip covers the whole PHP request even when a + * controller builds a second PluginManager instance: the broken replacement + * class stays defined process-wide, so every instance must skip it. + */ + private static array $skipPluginIdsThisRequest = []; /** * Per-process cache for {@see isActive()} lookups. @@ -181,7 +187,7 @@ public function autoRegisterBundledPlugins(): int $stmt->close(); if ($row) { - if (isset($this->skipPluginIdsThisRequest[(int) $row['id']])) { + if (isset(self::$skipPluginIdsThisRequest[(int) $row['id']])) { continue; } // Manifest compatibility metadata is operational, not merely @@ -2017,7 +2023,7 @@ public function loadActivePlugins(): void $instances = []; foreach ($activePlugins as $plugin) { $pluginId = (int) $plugin['id']; - if (isset($this->skipPluginIdsThisRequest[$pluginId])) { + if (isset(self::$skipPluginIdsThisRequest[$pluginId])) { // The failed replacement class remains defined until this PHP // request ends. The old files are already restored and will be // loaded normally by the next request. @@ -2078,9 +2084,23 @@ private function finalizePendingPluginUpdates(): void $state = null; $handle = @fopen($markerPath, 'r+b'); if ($handle === false) { + // Retry next request rather than retiring the marker: retiring on + // a transient open failure would silently drop the deferred + // onActivate (schema migration) this whole mechanism exists to + // run. Log so a genuinely permanent failure is diagnosable, not + // silent. + SecureLogger::warning('[PluginManager] Could not open pending plugin update marker; will retry next request', [ + 'path' => $markerPath, + ]); continue; } if (!flock($handle, LOCK_EX)) { + // LOCK_EX blocks on contention, so a false return is a real lock + // error, not another request finalizing. Same rationale as above: + // retry (don't drop the deferred lifecycle), but make it loud. + SecureLogger::warning('[PluginManager] Could not lock pending plugin update marker; will retry next request', [ + 'path' => $markerPath, + ]); fclose($handle); continue; } @@ -2228,7 +2248,7 @@ private function rollbackPendingPluginUpdate(array $state, \Throwable $cause): v try { $this->applyPluginMetadataSnapshot($pluginId, $oldPlugin); $this->restorePluginHooks($pluginId, $oldHooks); - $this->skipPluginIdsThisRequest[$pluginId] = true; + self::$skipPluginIdsThisRequest[$pluginId] = true; $restored = true; } catch (\Throwable $rollbackError) { SecureLogger::error('[PluginManager] Pending plugin DB rollback failed', [ @@ -2263,52 +2283,76 @@ private function rollbackPendingPluginUpdate(array $state, \Throwable $cause): v /** @param list> $hooks */ private function restorePluginHooks(int $pluginId, array $hooks): void { - $delete = $this->db->prepare('DELETE FROM plugin_hooks WHERE plugin_id = ?'); - if ($delete === false) { - throw new \RuntimeException('Impossibile ripristinare gli hook del plugin.'); + // DELETE + N INSERTs must be atomic: a mid-loop INSERT failure must not + // leave plugin_hooks with an arbitrary subset of the original rows. Only + // open a transaction when one is not already active (autocommit still + // on) — nesting begin_transaction() would implicitly commit the outer. + $ownTransaction = false; + $autocommitResult = $this->db->query('SELECT @@autocommit'); + if ($autocommitResult instanceof \mysqli_result) { + $autocommitRow = $autocommitResult->fetch_row(); + $autocommitResult->free(); + if ($autocommitRow !== null && (int) $autocommitRow[0] === 1) { + $ownTransaction = $this->db->begin_transaction(); + } } - $delete->bind_param('i', $pluginId); - if (!$delete->execute()) { + + try { + $delete = $this->db->prepare('DELETE FROM plugin_hooks WHERE plugin_id = ?'); + if ($delete === false) { + throw new \RuntimeException('Impossibile ripristinare gli hook del plugin.'); + } + $delete->bind_param('i', $pluginId); + if (!$delete->execute()) { + $delete->close(); + throw new \RuntimeException('Impossibile ripristinare gli hook del plugin.'); + } $delete->close(); - throw new \RuntimeException('Impossibile ripristinare gli hook del plugin.'); - } - $delete->close(); - if ($hooks === []) { - return; - } - $insert = $this->db->prepare( - 'INSERT INTO plugin_hooks ' - . '(plugin_id, hook_name, callback_class, callback_method, priority, is_active, created_at) ' - . 'VALUES (?, ?, ?, ?, ?, ?, ?)' - ); - if ($insert === false) { - throw new \RuntimeException('Impossibile ripristinare gli hook del plugin.'); - } - - foreach ($hooks as $hook) { - $hookName = (string) ($hook['hook_name'] ?? ''); - $callbackClass = (string) ($hook['callback_class'] ?? ''); - $callbackMethod = (string) ($hook['callback_method'] ?? ''); - $priority = (int) ($hook['priority'] ?? 10); - $isActive = (int) ($hook['is_active'] ?? 1); - $createdAt = (string) ($hook['created_at'] ?? date('Y-m-d H:i:s')); - $insert->bind_param( - 'isssiis', - $pluginId, - $hookName, - $callbackClass, - $callbackMethod, - $priority, - $isActive, - $createdAt - ); - if (!$insert->execute()) { + if ($hooks !== []) { + $insert = $this->db->prepare( + 'INSERT INTO plugin_hooks ' + . '(plugin_id, hook_name, callback_class, callback_method, priority, is_active, created_at) ' + . 'VALUES (?, ?, ?, ?, ?, ?, ?)' + ); + if ($insert === false) { + throw new \RuntimeException('Impossibile ripristinare gli hook del plugin.'); + } + + foreach ($hooks as $hook) { + $hookName = (string) ($hook['hook_name'] ?? ''); + $callbackClass = (string) ($hook['callback_class'] ?? ''); + $callbackMethod = (string) ($hook['callback_method'] ?? ''); + $priority = (int) ($hook['priority'] ?? 10); + $isActive = (int) ($hook['is_active'] ?? 1); + $createdAt = (string) ($hook['created_at'] ?? date('Y-m-d H:i:s')); + $insert->bind_param( + 'isssiis', + $pluginId, + $hookName, + $callbackClass, + $callbackMethod, + $priority, + $isActive, + $createdAt + ); + if (!$insert->execute()) { + $insert->close(); + throw new \RuntimeException('Impossibile ripristinare gli hook del plugin.'); + } + } $insert->close(); - throw new \RuntimeException('Impossibile ripristinare gli hook del plugin.'); } + + if ($ownTransaction) { + $this->db->commit(); + } + } catch (\Throwable $e) { + if ($ownTransaction) { + $this->db->rollback(); + } + throw $e; } - $insert->close(); } /** diff --git a/bin/build-release.sh b/bin/build-release.sh index 04b37e2bb..3fcc9df51 100644 --- a/bin/build-release.sh +++ b/bin/build-release.sh @@ -250,10 +250,13 @@ verify_package_contents() { log_error "Package missing storage/sessions/.gitkeep" has_errors=true fi + # Fail closed: a find error must never be read as "no session data". local unexpected_session - unexpected_session=$(find "$package_dir/storage/sessions" -mindepth 1 \ - ! -name '.gitkeep' -print -quit 2>/dev/null || true) - if [ -n "$unexpected_session" ]; then + if ! unexpected_session=$(find "$package_dir/storage/sessions" -mindepth 1 \ + ! -name '.gitkeep' -print -quit 2>/dev/null); then + log_error "Could not scan storage/sessions for leaked session data" + has_errors=true + elif [ -n "$unexpected_session" ]; then log_error "Package contains runtime session data: ${unexpected_session#"$package_dir/"}" has_errors=true fi diff --git a/scripts/ci-verify-release.sh b/scripts/ci-verify-release.sh index 31e5568aa..962b54d89 100755 --- a/scripts/ci-verify-release.sh +++ b/scripts/ci-verify-release.sh @@ -83,7 +83,13 @@ if find "$package_dir" -type f \( -name '*.pem' -o -name '*.key' -o -name 'id_rs exit 1 fi [ -f "$package_dir/storage/sessions/.gitkeep" ] || { echo "release missing storage/sessions/.gitkeep" >&2; exit 1; } -if find "$package_dir/storage/sessions" -mindepth 1 ! -name '.gitkeep' -print -quit 2>/dev/null | grep -q .; then +# Fail closed: a find error must never be read as "no session data" and let an +# unverified archive pass. Capture the scan so a non-zero find status aborts. +if ! session_scan="$(find "$package_dir/storage/sessions" -mindepth 1 ! -name '.gitkeep' -print -quit 2>/dev/null)"; then + echo "could not scan storage/sessions for leaked session data" >&2 + exit 1 +fi +if [ -n "$session_scan" ]; then echo "release contains runtime session data" >&2 exit 1 fi diff --git a/tests/plugin-manager.unit.php b/tests/plugin-manager.unit.php index f179fa4c8..a15885b9d 100644 --- a/tests/plugin-manager.unit.php +++ b/tests/plugin-manager.unit.php @@ -54,7 +54,7 @@ 'fresh bootstrap runs the replacement lifecycle (onActivate inside finalizePendingPluginUpdate)' ); $check($source !== false && str_contains($source, '$this->restorePluginHooks($pluginId, $oldHooks);'), 'failed replacement lifecycle restores prior hook rows'); -$check($source !== false && str_contains($source, '$this->skipPluginIdsThisRequest[$pluginId] = true;'), 'rolled-back replacement class is skipped for the rest of its PHP request'); +$check($source !== false && str_contains($source, 'self::$skipPluginIdsThisRequest[$pluginId] = true;'), 'rolled-back replacement class is skipped for the rest of its PHP request'); $finalizeAt = $source !== false ? strpos($source, '$this->finalizePendingPluginUpdates();') : false; $maintenanceAt = $source !== false ? strpos($source, '$maintenanceKey =') : false; $check( diff --git a/tests/plugin-zip-update-all-bundled.integration.php b/tests/plugin-zip-update-all-bundled.integration.php index b1f100dbb..3bd9f6756 100644 --- a/tests/plugin-zip-update-all-bundled.integration.php +++ b/tests/plugin-zip-update-all-bundled.integration.php @@ -205,7 +205,7 @@ function pzua_zip_plugin(string $pluginDir, string $slug, string $bumpedVersion) foreach (glob($pluginsDir . '/.' . $slug . '.staging-*') ?: [] as $stray) { pzua_rmdir($stray); } - @unlink($pluginsDir . '/.pinakes-plugin-update-' . $pluginId . '.json'); + @unlink($pluginsDir . '/' . \App\Support\PluginManager::PENDING_UPDATE_PREFIX . $pluginId . '.json'); } } diff --git a/tests/plugin-zip-update.integration.php b/tests/plugin-zip-update.integration.php index b2555790a..2ec8d6c92 100644 --- a/tests/plugin-zip-update.integration.php +++ b/tests/plugin-zip-update.integration.php @@ -174,28 +174,34 @@ public function setPluginId(int $pluginId): void function pzu_run_fresh_bootstrap(): void { + // Route stderr to a file, not a pipe: reading stdout to EOF then stderr can + // deadlock if the child fills the ~64 KB stderr pipe buffer first (child + // blocks writing stderr ↔ parent blocks reading stdout) — exactly the + // large-stack-trace case where the diagnostic matters most. + $stderrFile = tempnam(sys_get_temp_dir(), 'pzu_stderr_'); $process = proc_open( [PHP_BINARY, __DIR__ . '/helpers/plugin-zip-update-bootstrap.php'], [ 0 => ['pipe', 'r'], 1 => ['pipe', 'w'], - 2 => ['pipe', 'w'], + 2 => ['file', $stderrFile, 'w'], ], $pipes, dirname(__DIR__) ); if (!is_resource($process)) { + @unlink($stderrFile); throw new RuntimeException('Unable to start fresh plugin bootstrap process.'); } fclose($pipes[0]); $stdout = stream_get_contents($pipes[1]); - $stderr = stream_get_contents($pipes[2]); fclose($pipes[1]); - fclose($pipes[2]); $exitCode = proc_close($process); + $stderr = (string) @file_get_contents($stderrFile); + @unlink($stderrFile); if ($exitCode !== 0) { throw new RuntimeException( - 'Fresh plugin bootstrap failed: ' . trim((string) $stdout . "\n" . (string) $stderr) + 'Fresh plugin bootstrap failed: ' . trim((string) $stdout . "\n" . $stderr) ); } @@ -231,7 +237,7 @@ function pzu_run_fresh_bootstrap(): void $className = 'PluginZipUpdate' . ucfirst($suffix) . 'Plugin'; $pluginsDir = dirname(__DIR__) . '/storage/plugins'; $pluginDir = $pluginsDir . '/' . $slug; -$pendingMarker = $pluginsDir . '/.pinakes-plugin-update-'; +$pendingMarker = $pluginsDir . '/' . \App\Support\PluginManager::PENDING_UPDATE_PREFIX; $tableName = 'plugin_zip_update_' . $suffix; $zipV1 = null; $zipV2 = null; @@ -328,12 +334,23 @@ function pzu_run_fresh_bootstrap(): void ) { throw new RuntimeException('Failed replacement lifecycle did not restore package, metadata and hooks.'); } - // The request that loaded the broken v3 class must skip the rolled-back - // plugin. A second fresh request can now load v2 and keep its hook active. + // A later, unrelated request must not disturb the rolled-back plugin. With + // the marker already gone (the rollback unlinked it), this fresh process runs + // finalizePendingPluginUpdates() as a no-op: v2 stays active, its version is + // not bumped again, and its hook keeps serving. (Skipping the broken v3 class + // is an intra-process concern of the rollback request above — a separate + // process cannot and need not observe it.) + $stateBeforeNextRequest = $db->query("SELECT version, is_active FROM plugins WHERE id = {$pluginId}")->fetch_assoc(); pzu_run_fresh_bootstrap(); + $stateAfterNextRequest = $db->query("SELECT version, is_active FROM plugins WHERE id = {$pluginId}")->fetch_assoc(); $activeHooksOnNextRequest = (int) $db->query("SELECT COUNT(*) FROM plugin_hooks WHERE plugin_id = {$pluginId} AND hook_name = 'test.update.v2' AND callback_method = 'handleUpdated' AND is_active = 1")->fetch_row()[0]; - if ($activeHooksOnNextRequest !== 1) { - throw new RuntimeException('Next request did not load the restored plugin class and hook cleanly.'); + if (!is_array($stateAfterNextRequest) + || $stateAfterNextRequest != $stateBeforeNextRequest + || $stateAfterNextRequest['version'] !== '1.1.0' + || (int) $stateAfterNextRequest['is_active'] !== 1 + || $activeHooksOnNextRequest !== 1 + ) { + throw new RuntimeException('A later request re-triggered or disturbed the rolled-back plugin.'); } if (is_file($pendingMarker) || (glob($pluginsDir . '/.' . $slug . '.backup-*') ?: []) !== []) { throw new RuntimeException('Lifecycle rollback left update recovery files behind.'); @@ -347,7 +364,7 @@ function pzu_run_fresh_bootstrap(): void $db->query("DROP TABLE IF EXISTS `{$tableName}`"); pzu_delete_directory($pluginDir); if ($pluginId > 0) { - @unlink($pluginsDir . '/.pinakes-plugin-update-' . $pluginId . '.json'); + @unlink($pluginsDir . '/' . \App\Support\PluginManager::PENDING_UPDATE_PREFIX . $pluginId . '.json'); } foreach (glob($pluginsDir . '/.' . $slug . '.backup-*') ?: [] as $backup) { pzu_delete_directory($backup); From 044015072a6cf82f7402023b2047b238cda72b36 Mon Sep 17 00:00:00 2001 From: fabiodalez-dev Date: Wed, 19 Aug 2026 18:30:41 +0200 Subject: [PATCH 06/16] fix(plugins): unlink orphaned marker on lock failure and harden test cleanup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-ups on the same review round: - createPendingPluginUpdateMarker(): split the fopen('x+b') and flock() failure paths. fopen('x+b') creates the marker file; if the subsequent flock() fails, the empty marker was left on disk (the caller's catch cannot remove it — this method throws before returning the handle), permanently blocking every future update of that plugin with 'already pending'. Unlink it before throwing. - Test marker cleanup now globs '.json*' so retired markers renamed to .json.invalid- by finalizePendingPluginUpdates() are removed too, instead of an exact .json unlink that would leave them behind. - Clarify the third-bootstrap comment: the intra-process skip is already covered by the partialHooks === 0 assertion after the rollback; the third bootstrap only guarantees a later unrelated request leaves the rolled-back plugin undisturbed. --- app/Support/PluginManager.php | 17 ++++++++++++---- ...gin-zip-update-all-bundled.integration.php | 5 ++++- tests/plugin-zip-update.integration.php | 20 ++++++++++++------- 3 files changed, 30 insertions(+), 12 deletions(-) diff --git a/app/Support/PluginManager.php b/app/Support/PluginManager.php index 0dfc835e7..f0983a7fe 100644 --- a/app/Support/PluginManager.php +++ b/app/Support/PluginManager.php @@ -1548,12 +1548,21 @@ private function pendingPluginUpdatePath(int $pluginId): string private function createPendingPluginUpdateMarker(string $path, array $state) { $handle = @fopen($path, 'x+b'); - if ($handle === false || !flock($handle, LOCK_EX)) { - if (is_resource($handle)) { - fclose($handle); - } + if ($handle === false) { + // The exclusive create failed: a marker already exists (another + // pending update) or the directory is unwritable. Nothing to clean. throw new \RuntimeException('Esiste già un aggiornamento pendente per questo plugin.'); } + if (!flock($handle, LOCK_EX)) { + // fopen('x+b') just created this file. If it cannot be locked, remove + // the empty marker before throwing: leaving it would block every + // future update of this plugin (fopen('x+b') keeps failing with + // "already pending") and the caller's catch cannot clean it up + // because this method threw before returning the handle. + fclose($handle); + @unlink($path); + throw new \RuntimeException('Impossibile bloccare il marker di aggiornamento del plugin.'); + } try { $json = json_encode($state, JSON_THROW_ON_ERROR | JSON_UNESCAPED_SLASHES); diff --git a/tests/plugin-zip-update-all-bundled.integration.php b/tests/plugin-zip-update-all-bundled.integration.php index 3bd9f6756..90590a8a5 100644 --- a/tests/plugin-zip-update-all-bundled.integration.php +++ b/tests/plugin-zip-update-all-bundled.integration.php @@ -205,7 +205,10 @@ function pzua_zip_plugin(string $pluginDir, string $slug, string $bumpedVersion) foreach (glob($pluginsDir . '/.' . $slug . '.staging-*') ?: [] as $stray) { pzua_rmdir($stray); } - @unlink($pluginsDir . '/' . \App\Support\PluginManager::PENDING_UPDATE_PREFIX . $pluginId . '.json'); + // Glob so retired markers (.json.invalid-) are cleaned too. + foreach (glob($pluginsDir . '/' . \App\Support\PluginManager::PENDING_UPDATE_PREFIX . $pluginId . '.json*') ?: [] as $marker) { + @unlink($marker); + } } } diff --git a/tests/plugin-zip-update.integration.php b/tests/plugin-zip-update.integration.php index 2ec8d6c92..eab212d40 100644 --- a/tests/plugin-zip-update.integration.php +++ b/tests/plugin-zip-update.integration.php @@ -334,12 +334,14 @@ function pzu_run_fresh_bootstrap(): void ) { throw new RuntimeException('Failed replacement lifecycle did not restore package, metadata and hooks.'); } - // A later, unrelated request must not disturb the rolled-back plugin. With - // the marker already gone (the rollback unlinked it), this fresh process runs - // finalizePendingPluginUpdates() as a no-op: v2 stays active, its version is - // not bumped again, and its hook keeps serving. (Skipping the broken v3 class - // is an intra-process concern of the rollback request above — a separate - // process cannot and need not observe it.) + // The intra-process skip (the rollback request must not re-instantiate the + // broken v3 class) is already covered above: partialHooks === 0 asserts v3's + // artifacts were not (re)created in that same request. skipPluginIdsThisRequest + // lives in the rollback process, so a separate process cannot observe it. + // This third bootstrap adds the distinct guarantee that a later, unrelated + // request does not disturb the rolled-back plugin: with the marker gone, + // finalizePendingPluginUpdates() is a no-op — v2 stays active, its version is + // not bumped again, and its hook keeps serving. $stateBeforeNextRequest = $db->query("SELECT version, is_active FROM plugins WHERE id = {$pluginId}")->fetch_assoc(); pzu_run_fresh_bootstrap(); $stateAfterNextRequest = $db->query("SELECT version, is_active FROM plugins WHERE id = {$pluginId}")->fetch_assoc(); @@ -364,7 +366,11 @@ function pzu_run_fresh_bootstrap(): void $db->query("DROP TABLE IF EXISTS `{$tableName}`"); pzu_delete_directory($pluginDir); if ($pluginId > 0) { - @unlink($pluginsDir . '/' . \App\Support\PluginManager::PENDING_UPDATE_PREFIX . $pluginId . '.json'); + // Glob (not an exact .json unlink) so retired markers renamed to + // .json.invalid- by finalizePendingPluginUpdates() are cleaned too. + foreach (glob($pluginsDir . '/' . \App\Support\PluginManager::PENDING_UPDATE_PREFIX . $pluginId . '.json*') ?: [] as $marker) { + @unlink($marker); + } } foreach (glob($pluginsDir . '/.' . $slug . '.backup-*') ?: [] as $backup) { pzu_delete_directory($backup); From f9656b1a3b5c3af484d730299d87aa1e9c5f2fce Mon Sep 17 00:00:00 2001 From: fabiodalez-dev Date: Wed, 19 Aug 2026 19:02:00 +0200 Subject: [PATCH 07/16] fix(plugins,release): savepoint tx probe and reject symlinked session dir MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two review findings on the previous commit: - restorePluginHooks() detected an active transaction via @@autocommit, which is unreliable: MySQL keeps @@autocommit = 1 after begin_transaction(), so the guard could nest begin_transaction() and implicitly commit a caller's outer transaction. Probe with a savepoint instead (a SAVEPOINT outside a transaction is discarded by autocommit, so the RELEASE fails) — verified in/out of a transaction and after rollback. Inside a transaction the hook restore is now scoped to a SAVEPOINT/ROLLBACK TO SAVEPOINT; otherwise it owns a fresh transaction. Probe works under both mysqli exception and silent error modes. - Both release verifiers now reject a symlinked storage/sessions directory or .gitkeep before scanning: [ -d ]/[ -f ] follow symlinks, so a link could point outside the tree and slip session data past the check. Reject with [ -L ] first, allow only a real directory holding a real placeholder. A static guard in code-quality.spec.js pins the -L check in both verifiers. --- app/Support/PluginManager.php | 49 +++++++++++++++++++++++++---------- bin/build-release.sh | 32 +++++++++++++---------- scripts/ci-verify-release.sh | 12 ++++++++- tests/code-quality.spec.js | 7 +++++ 4 files changed, 73 insertions(+), 27 deletions(-) diff --git a/app/Support/PluginManager.php b/app/Support/PluginManager.php index f0983a7fe..5aef133e5 100644 --- a/app/Support/PluginManager.php +++ b/app/Support/PluginManager.php @@ -2293,17 +2293,17 @@ private function rollbackPendingPluginUpdate(array $state, \Throwable $cause): v private function restorePluginHooks(int $pluginId, array $hooks): void { // DELETE + N INSERTs must be atomic: a mid-loop INSERT failure must not - // leave plugin_hooks with an arbitrary subset of the original rows. Only - // open a transaction when one is not already active (autocommit still - // on) — nesting begin_transaction() would implicitly commit the outer. - $ownTransaction = false; - $autocommitResult = $this->db->query('SELECT @@autocommit'); - if ($autocommitResult instanceof \mysqli_result) { - $autocommitRow = $autocommitResult->fetch_row(); - $autocommitResult->free(); - if ($autocommitRow !== null && (int) $autocommitRow[0] === 1) { - $ownTransaction = $this->db->begin_transaction(); - } + // leave plugin_hooks with an arbitrary subset of the original rows. + // Detect an already-open transaction with a savepoint probe — NOT + // @@autocommit, which stays 1 after begin_transaction() and would let us + // nest begin_transaction() and implicitly commit the caller's outer + // transaction. Inside one, scope our work to a savepoint; otherwise own + // a fresh transaction. + $inTransaction = $this->hasActiveTransaction(); + if ($inTransaction) { + $this->db->query('SAVEPOINT pinakes_restore_hooks'); + } else { + $this->db->begin_transaction(); } try { @@ -2353,17 +2353,40 @@ private function restorePluginHooks(int $pluginId, array $hooks): void $insert->close(); } - if ($ownTransaction) { + if ($inTransaction) { + $this->db->query('RELEASE SAVEPOINT pinakes_restore_hooks'); + } else { $this->db->commit(); } } catch (\Throwable $e) { - if ($ownTransaction) { + if ($inTransaction) { + $this->db->query('ROLLBACK TO SAVEPOINT pinakes_restore_hooks'); + } else { $this->db->rollback(); } throw $e; } } + /** + * Whether a transaction is already open on the connection. @@autocommit is + * unreliable here — it stays 1 after begin_transaction() — so probe with a + * savepoint instead: outside a transaction the SAVEPOINT is a no-op that + * autocommit discards, so the following RELEASE cannot find it. Works under + * both mysqli exception and silent error modes. + */ + private function hasActiveTransaction(): bool + { + try { + if ($this->db->query('SAVEPOINT pinakes_tx_probe') === false) { + return false; + } + return $this->db->query('RELEASE SAVEPOINT pinakes_tx_probe') !== false; + } catch (\mysqli_sql_exception $e) { + return false; + } + } + /** * Invalidate the cross-request plugin caches. Must be called by every * plugin lifecycle mutation (install/activate/deactivate/uninstall). diff --git a/bin/build-release.sh b/bin/build-release.sh index 3fcc9df51..a660a1dfe 100644 --- a/bin/build-release.sh +++ b/bin/build-release.sh @@ -244,21 +244,27 @@ verify_package_contents() { fi # PHP session files may contain authenticated user data and CSRF tokens. - # Ship the writable directory as exactly {.gitkeep}: require the placeholder - # and reject every other entry — session payloads, symlinks, and stray dirs. - if [ ! -f "$package_dir/storage/sessions/.gitkeep" ]; then - log_error "Package missing storage/sessions/.gitkeep" + # Ship the writable directory as exactly {.gitkeep}: a REAL directory holding + # a REAL placeholder, and nothing else. Reject symlinks first (-d/-f follow + # them, so a symlinked dir/placeholder could point outside the tree and slip + # session data past the scan), then require the placeholder, then scan. + if [ -L "$package_dir/storage/sessions" ] || [ ! -d "$package_dir/storage/sessions" ]; then + log_error "storage/sessions is not a real directory" has_errors=true - fi - # Fail closed: a find error must never be read as "no session data". - local unexpected_session - if ! unexpected_session=$(find "$package_dir/storage/sessions" -mindepth 1 \ - ! -name '.gitkeep' -print -quit 2>/dev/null); then - log_error "Could not scan storage/sessions for leaked session data" - has_errors=true - elif [ -n "$unexpected_session" ]; then - log_error "Package contains runtime session data: ${unexpected_session#"$package_dir/"}" + elif [ -L "$package_dir/storage/sessions/.gitkeep" ] || [ ! -f "$package_dir/storage/sessions/.gitkeep" ]; then + log_error "storage/sessions/.gitkeep is missing or a symlink" has_errors=true + else + # Fail closed: a find error must never be read as "no session data". + local unexpected_session + if ! unexpected_session=$(find "$package_dir/storage/sessions" -mindepth 1 \ + ! -name '.gitkeep' -print -quit 2>/dev/null); then + log_error "Could not scan storage/sessions for leaked session data" + has_errors=true + elif [ -n "$unexpected_session" ]; then + log_error "Package contains runtime session data: ${unexpected_session#"$package_dir/"}" + has_errors=true + fi fi # Files that MUST be in the package diff --git a/scripts/ci-verify-release.sh b/scripts/ci-verify-release.sh index 962b54d89..e3ec5528f 100755 --- a/scripts/ci-verify-release.sh +++ b/scripts/ci-verify-release.sh @@ -82,7 +82,17 @@ if find "$package_dir" -type f \( -name '*.pem' -o -name '*.key' -o -name 'id_rs echo "release contains a private-key or registry-credential file" >&2 exit 1 fi -[ -f "$package_dir/storage/sessions/.gitkeep" ] || { echo "release missing storage/sessions/.gitkeep" >&2; exit 1; } +# storage/sessions must be a REAL directory holding a REAL .gitkeep. Reject +# symlinks first: -d/-f follow them, so a symlinked directory or placeholder +# could point outside the tree and slip session data past the scan. +if [ -L "$package_dir/storage/sessions" ] || [ ! -d "$package_dir/storage/sessions" ]; then + echo "release storage/sessions is not a real directory" >&2 + exit 1 +fi +if [ -L "$package_dir/storage/sessions/.gitkeep" ] || [ ! -f "$package_dir/storage/sessions/.gitkeep" ]; then + echo "release missing storage/sessions/.gitkeep (or it is a symlink)" >&2 + exit 1 +fi # Fail closed: a find error must never be read as "no session data" and let an # unverified archive pass. Capture the scan so a non-zero find status aborts. if ! session_scan="$(find "$package_dir/storage/sessions" -mindepth 1 ! -name '.gitkeep' -print -quit 2>/dev/null)"; then diff --git a/tests/code-quality.spec.js b/tests/code-quality.spec.js index 0fa8d1ed4..201a5e1d2 100644 --- a/tests/code-quality.spec.js +++ b/tests/code-quality.spec.js @@ -215,6 +215,13 @@ test.describe.serial('Code Quality — 15 static analysis tests', () => { .toContain('- /storage/sessions/*'); expect(archiveVerifier, 'archive verification must reject leaked runtime sessions') .toContain('release contains runtime session data'); + // Both verifiers must reject a symlinked sessions dir / placeholder + // (-d/-f follow symlinks, so a link could slip session data past the scan). + const releaseBuilder = fs.readFileSync(path.join(ROOT, 'bin', 'build-release.sh'), 'utf-8'); + expect(archiveVerifier, 'archive verification must reject a symlinked storage/sessions') + .toContain('-L "$package_dir/storage/sessions"'); + expect(releaseBuilder, 'release build must reject a symlinked storage/sessions') + .toContain('-L "$package_dir/storage/sessions"'); }); // ── 3. Plugin ensureSchema() called from onActivate() ───────────────────── From 80a69a412bc90cf9c1f3192cb3974fd257fcbf41 Mon Sep 17 00:00:00 2001 From: fabiodalez-dev Date: Wed, 19 Aug 2026 19:05:15 +0200 Subject: [PATCH 08/16] feat(loans): per-row quick recall action on the loans list (#360) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The single 'Invia Sollecito' action only existed on the loan detail page. Add a per-row bullhorn button on the loans list for loans already past their due date (strict data_scadenza < today, matching sendManualRecall's giorni_ritardo >= 1 guard), for both the SSR rows and the DataTables render so they cannot diverge. It reuses the existing single-recall endpoint (POST /admin/loans/{id}/recall) and copies the detail page's confirm/POST/report flow verbatim — CSRF token, the data.error||data.code SESSION_EXPIRED/CSRF_INVALID reload branch, and the success/failure SweetAlert. Button styling is copied verbatim from the sibling confirmPickup action (amber never co-occurs: pickup is for da_ritirare, recall for overdue in_corso/in_ritardo). No new i18n keys — the detail-page strings are reused and already present in all five locales. Note: 'Scaduto' rows are expired pickups (the book never left the library), so recall/extend intentionally do not apply there; overdue loans are the yellow 'In Ritardo' rows, which were already selectable for bulk recall. --- CHANGELOG.md | 5 +- app/Views/prestiti/index.php | 98 ++++++++++++++++++++++++++++++++++++ 2 files changed, 101 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4100f1aa4..caa28321e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,8 +14,9 @@ Overdue-loan recalls (solleciti) and emailing the loan receipt (#360). single overdue notification. Automatic recalls repeat at a configurable interval up to a configurable cap (Settings → Loans → "Solleciti automatici", off by default; sent by the notifications cron or on admin login). Staff can - also send a manual recall for one loan from the loan detail page, or for many - at once from the loans list via the bulk action bar — manual recalls ignore + also send a manual recall for one loan — from the loan detail page or a + per-row action on the loans list — or for many at once from the loans list + via the bulk action bar — manual recalls ignore the automatic schedule but share the same per-loan counter (`prestiti.recall_count` / `last_recall_at`, added by `migrate_0.7.62-rc.1.sql` and self-healed at runtime). New editable email diff --git a/app/Views/prestiti/index.php b/app/Views/prestiti/index.php index 586c1c81f..9ed259bfd 100644 --- a/app/Views/prestiti/index.php +++ b/app/Views/prestiti/index.php @@ -353,6 +353,11 @@ class="inline-flex items-center gap-2 whitespace-nowrap px-4 py-2 bg-gray-100 te + = 1). + $ssrRecallable = $ssrExtendable + && (string)($prestito['data_scadenza'] ?? '') !== '' + && (string)$prestito['data_scadenza'] < $applicationToday; ?> @@ -400,6 +405,11 @@ class="inline-flex items-center px-3 py-1.5 bg-red-600 hover:bg-red-500 text-whi + + + @@ -592,6 +602,14 @@ className: 'text-right', actions += ` `; + // #360: quick recall only for a loan already past its due date + // (strict <, matching sendManualRecall's giorni_ritardo >= 1) — + // same condition as the SSR row above. + if (row.data_scadenza && row.data_scadenza < applicationToday) { + actions += ``; + } } actions += ``; return actions; @@ -824,6 +842,86 @@ function applyChecked() { applyChecked(); })(); + // #360: per-row quick recall — same confirm/POST/report flow as the + // "Invia Sollecito" button on the loan detail page (dettagli_prestito.php). + // Delegated: DataTables redraws replace the buttons on every draw. + document.addEventListener('click', async function(e) { + const btn = e.target.closest ? e.target.closest('.loan-recall-btn') : null; + if (!btn || btn.disabled) return; + const loanId = parseInt(btn.dataset.loanId, 10); + if (!loanId) return; + const result = await Swal.fire({ + title: __('Inviare un sollecito per questo prestito?'), + text: __('L\'utente riceverà un\'email di sollecito per la restituzione.'), + icon: 'question', + showCancelButton: true, + confirmButtonText: __('Sì, invia'), + cancelButtonText: __('Annulla'), + confirmButtonColor: '#111827', + cancelButtonColor: '#6b7280' + }); + if (!result.isConfirmed) { + return; + } + btn.disabled = true; + try { + const response = await fetch(window.BASE_PATH + '/admin/loans/' + loanId + '/recall', { + method: 'POST', + credentials: 'same-origin', + headers: { + 'Content-Type': 'application/json', + 'X-CSRF-Token': document.querySelector('meta[name="csrf-token"]')?.getAttribute('content') || '' + }, + body: JSON.stringify({}) + }); + const data = await response.json(); + // The CSRF middleware rejects an expired session / bad token with + // { error, code } (not { message }); tell the user to reload + // instead of showing a generic "send failed", per the app-wide + // data.code===SESSION_EXPIRED|CSRF_INVALID convention. + if (data.error || data.code) { + Swal.fire({ + title: __('Errore'), + text: data.error || __('Invio del sollecito non riuscito.'), + icon: 'error', + confirmButtonText: __('OK'), + confirmButtonColor: '#111827' + }); + if (data.code === 'SESSION_EXPIRED' || data.code === 'CSRF_INVALID') { + setTimeout(() => window.location.reload(), 2000); + } + return; + } + if (data.success) { + Swal.fire({ + title: __('Sollecito inviato'), + text: data.message || '', + icon: 'success', + confirmButtonText: __('OK'), + confirmButtonColor: '#111827' + }); + } else { + Swal.fire({ + title: __('Errore'), + text: data.message || __('Invio del sollecito non riuscito.'), + icon: 'error', + confirmButtonText: __('OK'), + confirmButtonColor: '#111827' + }); + } + } catch (error) { + Swal.fire({ + title: __('Errore'), + text: __('Errore nella comunicazione con il server'), + icon: 'error', + confirmButtonText: __('OK'), + confirmButtonColor: '#111827' + }); + } finally { + btn.disabled = false; + } + }); + // Pending loan requests widget - Approve/Reject buttons document.querySelectorAll('.approve-btn').forEach(button => { button.addEventListener('click', function() { From f04132a957a5f76fcfb1d7fa6fd088765fbafd49 Mon Sep 17 00:00:00 2001 From: fabiodalez-dev Date: Thu, 20 Aug 2026 03:44:57 +0200 Subject: [PATCH 09/16] fix(plugins,release): unique per-invocation savepoints and full-path session allowlist Three CodeRabbit findings on the previous commit: - restorePluginHooks() and hasActiveTransaction() built fixed savepoint names (pinakes_restore_hooks / pinakes_tx_probe). MySQL DELETES an existing savepoint when SAVEPOINT reuses the same name, so a fixed name could silently clobber a caller's savepoint boundary. Generate a unique 'pinakes_sp_'.bin2hex(random_bytes(6)) per invocation and use it for SAVEPOINT / RELEASE SAVEPOINT / ROLLBACK TO SAVEPOINT; the probe always RELEASEs after a successful SAVEPOINT so it never leaves one behind inside a caller's transaction. Verified nested: a caller's outer savepoint survives. - Both release verifiers used '! -name .gitkeep', which allow-lists ANY file named .gitkeep at any depth. Switched to a full-path allowlist '! -path "$package_dir/storage/sessions/.gitkeep"' so only the exact top-level placeholder is permitted (a nested storage/sessions/x/.gitkeep is now rejected). - code-quality.spec.js pins the -L .gitkeep symlink check in both verifiers. --- app/Support/PluginManager.php | 27 ++++++++++++++++++++++----- bin/build-release.sh | 2 +- scripts/ci-verify-release.sh | 2 +- tests/code-quality.spec.js | 4 ++++ 4 files changed, 28 insertions(+), 7 deletions(-) diff --git a/app/Support/PluginManager.php b/app/Support/PluginManager.php index 5aef133e5..e8368cdc6 100644 --- a/app/Support/PluginManager.php +++ b/app/Support/PluginManager.php @@ -2300,8 +2300,13 @@ private function restorePluginHooks(int $pluginId, array $hooks): void // transaction. Inside one, scope our work to a savepoint; otherwise own // a fresh transaction. $inTransaction = $this->hasActiveTransaction(); + // SAVEPOINT reuses (deletes) an existing savepoint of the same name, + // so a fixed identifier could silently clobber a caller's savepoint + // boundary. Generate a unique identifier per invocation — savepoint + // names are identifiers, not bindable params, and hex is injection-safe. + $savepoint = 'pinakes_sp_' . bin2hex(random_bytes(6)); if ($inTransaction) { - $this->db->query('SAVEPOINT pinakes_restore_hooks'); + $this->db->query('SAVEPOINT ' . $savepoint); } else { $this->db->begin_transaction(); } @@ -2354,13 +2359,13 @@ private function restorePluginHooks(int $pluginId, array $hooks): void } if ($inTransaction) { - $this->db->query('RELEASE SAVEPOINT pinakes_restore_hooks'); + $this->db->query('RELEASE SAVEPOINT ' . $savepoint); } else { $this->db->commit(); } } catch (\Throwable $e) { if ($inTransaction) { - $this->db->query('ROLLBACK TO SAVEPOINT pinakes_restore_hooks'); + $this->db->query('ROLLBACK TO SAVEPOINT ' . $savepoint); } else { $this->db->rollback(); } @@ -2377,11 +2382,23 @@ private function restorePluginHooks(int $pluginId, array $hooks): void */ private function hasActiveTransaction(): bool { + // Unique per call: SAVEPOINT reuses (deletes) an existing savepoint of + // the same name, so a fixed probe name could clobber a caller's own + // savepoint boundary. Hex from random_bytes() is injection-safe. + $probe = 'pinakes_sp_' . bin2hex(random_bytes(6)); try { - if ($this->db->query('SAVEPOINT pinakes_tx_probe') === false) { + if ($this->db->query('SAVEPOINT ' . $probe) === false) { return false; } - return $this->db->query('RELEASE SAVEPOINT pinakes_tx_probe') !== false; + } catch (\mysqli_sql_exception $e) { + return false; + } + try { + // Always release the probe after a successful SAVEPOINT so it never + // lingers inside a caller's transaction. Outside one, autocommit + // already discarded it and the RELEASE fails — that failure IS the + // "no transaction" signal. + return $this->db->query('RELEASE SAVEPOINT ' . $probe) !== false; } catch (\mysqli_sql_exception $e) { return false; } diff --git a/bin/build-release.sh b/bin/build-release.sh index a660a1dfe..4c0ae32d0 100644 --- a/bin/build-release.sh +++ b/bin/build-release.sh @@ -258,7 +258,7 @@ verify_package_contents() { # Fail closed: a find error must never be read as "no session data". local unexpected_session if ! unexpected_session=$(find "$package_dir/storage/sessions" -mindepth 1 \ - ! -name '.gitkeep' -print -quit 2>/dev/null); then + ! -path "$package_dir/storage/sessions/.gitkeep" -print -quit 2>/dev/null); then log_error "Could not scan storage/sessions for leaked session data" has_errors=true elif [ -n "$unexpected_session" ]; then diff --git a/scripts/ci-verify-release.sh b/scripts/ci-verify-release.sh index e3ec5528f..03330c0ee 100755 --- a/scripts/ci-verify-release.sh +++ b/scripts/ci-verify-release.sh @@ -95,7 +95,7 @@ if [ -L "$package_dir/storage/sessions/.gitkeep" ] || [ ! -f "$package_dir/stora fi # Fail closed: a find error must never be read as "no session data" and let an # unverified archive pass. Capture the scan so a non-zero find status aborts. -if ! session_scan="$(find "$package_dir/storage/sessions" -mindepth 1 ! -name '.gitkeep' -print -quit 2>/dev/null)"; then +if ! session_scan="$(find "$package_dir/storage/sessions" -mindepth 1 ! -path "$package_dir/storage/sessions/.gitkeep" -print -quit 2>/dev/null)"; then echo "could not scan storage/sessions for leaked session data" >&2 exit 1 fi diff --git a/tests/code-quality.spec.js b/tests/code-quality.spec.js index 201a5e1d2..74227547a 100644 --- a/tests/code-quality.spec.js +++ b/tests/code-quality.spec.js @@ -222,6 +222,10 @@ test.describe.serial('Code Quality — 15 static analysis tests', () => { .toContain('-L "$package_dir/storage/sessions"'); expect(releaseBuilder, 'release build must reject a symlinked storage/sessions') .toContain('-L "$package_dir/storage/sessions"'); + expect(archiveVerifier, 'archive verification must reject a symlinked .gitkeep placeholder') + .toContain('-L "$package_dir/storage/sessions/.gitkeep"'); + expect(releaseBuilder, 'release build must reject a symlinked .gitkeep placeholder') + .toContain('-L "$package_dir/storage/sessions/.gitkeep"'); }); // ── 3. Plugin ensureSchema() called from onActivate() ───────────────────── From af369bc8e49bb0ec1107fbe85d6ab0541c8b2dee Mon Sep 17 00:00:00 2001 From: fabiodalez-dev Date: Thu, 20 Aug 2026 03:54:46 +0200 Subject: [PATCH 10/16] fix(plugins): create the deferred-update marker before promoting the package MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit createPendingPluginUpdateMarker was called AFTER rename(staging -> plugin), so between the promotion and the marker creation a concurrent loadActivePlugins() would find no marker and load the new files against the still-old metadata, hooks and schema. Move the marker creation before the promotion: it holds an exclusive lock for the rest of the request (released in the finally), so a concurrent finalizePendingPluginUpdates() blocks on it instead of loading a half-updated state. If the request dies before promotion, the marker's finalize throws on the missing main file (instantiatePlugin guards file_exists) and rolls back to the backup — it never activates the previous package against new state. --- app/Support/PluginManager.php | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/app/Support/PluginManager.php b/app/Support/PluginManager.php index e8368cdc6..ee0983995 100644 --- a/app/Support/PluginManager.php +++ b/app/Support/PluginManager.php @@ -1336,11 +1336,16 @@ private function updatePluginFromStaging( $hasBackup = true; } - if (!rename($stagingPath, $pluginPath)) { - throw new \RuntimeException('Impossibile sostituire i file del plugin.'); - } - $newPackageInstalled = true; - + // Create the deferred-update marker BEFORE the new package becomes + // visible, not after. It holds an exclusive lock for the rest of this + // request (released in the finally), so a concurrent bootstrap's + // finalizePendingPluginUpdates() blocks on it instead of loading the + // new files against the still-old metadata, hooks and schema — the + // window between promotion and marker creation is thereby closed. + // If this request dies before the promotion below, the marker's + // finalize cannot instantiate the not-yet-moved package + // (instantiatePlugin throws on the missing main file) and rolls back + // to the backup — it never activates a half-updated state. if ((int) ($existingPlugin['is_active'] ?? 0) === 1) { $pendingMarkerPath = $this->pendingPluginUpdatePath($pluginId); $pendingState = [ @@ -1358,6 +1363,11 @@ private function updatePluginFromStaging( ); } + if (!rename($stagingPath, $pluginPath)) { + throw new \RuntimeException('Impossibile sostituire i file del plugin.'); + } + $newPackageInstalled = true; + $this->applyPluginMetadataSnapshot($pluginId, $newPluginSnapshot); $metadataUpdated = true; From f131d5dd3f4ea214b133e61d080092083d0fb621 Mon Sep 17 00:00:00 2001 From: fabiodalez-dev Date: Thu, 20 Aug 2026 03:58:59 +0200 Subject: [PATCH 11/16] fix(loans): don't mark a book ready-for-pickup while a copy is still out (#366) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit MaintenanceService::activateScheduledLoans() promoted a 'prenotato' loan to 'da_ritirare' and emailed 'ready for pickup' purely on its date window, with no check that a copy is physically free. In the #366 sequence — a reservation scheduled right after a loan that then goes overdue and is never returned — runAll() activates the reservation (before it flips the predecessor overdue), so the book was announced ready while still out, and confirmPickup then refused. Guard the promotion inside the existing per-loan transaction, after the row locks and before the UPDATE + email: - book level: count occupying rows for the libro_id (active in_corso/in_ritardo/ da_ritirare, plus copy-holding pendente) excluding this row; if >= totalCopies, roll back and keep 'prenotato' — no state change, no email. No date predicate on active loans: an unreturned copy is out regardless of its contractual dates. - copy level: if the reservation pins a copia_id, that copy must be on the shelf ('disponibile'/'prenotato'); a 'prestato' copy still out blocks promotion even when another copy of the title is free. Multi-copy preserved (2 copies, 1 out -> promotes). New behavioural test pickup-ready-copy-free-366.unit.php (8 assertions, 4 fail without the guard). --- app/Support/MaintenanceService.php | 63 ++++++ locale/da_DK.json | 4 +- locale/de_DE.json | 4 +- locale/en_US.json | 4 +- locale/fr_FR.json | 4 +- locale/it_IT.json | 4 +- tests/pickup-ready-copy-free-366.unit.php | 253 ++++++++++++++++++++++ 7 files changed, 331 insertions(+), 5 deletions(-) create mode 100644 tests/pickup-ready-copy-free-366.unit.php diff --git a/app/Support/MaintenanceService.php b/app/Support/MaintenanceService.php index a9564eb38..1b6b94242 100644 --- a/app/Support/MaintenanceService.php +++ b/app/Support/MaintenanceService.php @@ -343,6 +343,8 @@ public function activateScheduledLoans(): int $activatedCount = 0; // Instantiate DataIntegrity once outside the loop to reduce overhead $integrity = new DataIntegrity($this->db); + // Capacity ceiling authority for the #366 copy-free guard below + $capacity = new \App\Services\CapacityService($this->db); // Get pickup expiry days from settings $settingsRepo = new SettingsRepository($this->db); @@ -385,6 +387,67 @@ public function activateScheduledLoans(): int } $loan = $lockedLoan; + // #366 guard: a reservation may only become 'da_ritirare' (and get + // the pickup-ready email) when a physical copy is genuinely free + // RIGHT NOW. The date window alone is not enough: the preceding + // loan may still be out — overdue included. 'in_corso'/'in_ritardo' + // rows are counted with NO date predicate because an unreturned + // copy is out regardless of its contractual dates (and + // updateOverdueLoans runs AFTER this sweep, so an overdue loan can + // still sit in 'in_corso' here). Sibling 'da_ritirare' pickups and + // copy-holding 'pendente' rows each pin a copy on the shelf too. + // Future 'prenotato' rows are NOT counted: they hold capacity for + // a later window, not a copy today. If nothing is free the + // reservation simply stays 'prenotato' — no state change, no email + // — and a later run promotes it once the copy actually comes back. + $occStmt = $this->db->prepare(" + SELECT COUNT(*) AS occupied + FROM prestiti + WHERE libro_id = ? + AND id <> ? + AND ( (attivo = 1 AND stato IN ('in_corso','in_ritardo','da_ritirare')) + OR (attivo = 0 AND stato = 'pendente' AND copia_id IS NOT NULL) ) + "); + $occStmt->bind_param('ii', $bookId, $loanId); + $occStmt->execute(); + $occRow = $occStmt->get_result()->fetch_assoc(); + $occStmt->close(); + $occupied = (int) ($occRow['occupied'] ?? 0); + + if ($occupied >= $capacity->totalCopies($bookId)) { + $this->db->rollback(); + SecureLogger::info(__('Attivazione prestito rinviata: nessuna copia libera'), [ + 'prestito_id' => $loanId, + 'libro_id' => $bookId, + 'occupied' => $occupied + ]); + continue; + } + + // Per-copy check (multi-copy titles): the reservation may be pinned + // to a specific copy that is still out on the previous loan even + // when another copy of the same title is free. Only the two + // on-shelf states may be promoted. + if (!empty($loan['copia_id'])) { + $copiaId = (int) $loan['copia_id']; + $copyStmt = $this->db->prepare('SELECT stato FROM copie WHERE id = ? FOR UPDATE'); + $copyStmt->bind_param('i', $copiaId); + $copyStmt->execute(); + $copyRow = $copyStmt->get_result()->fetch_assoc(); + $copyStmt->close(); + $copyState = $copyRow['stato'] ?? null; + if (!in_array($copyState, ['disponibile', 'prenotato'], true)) { + $this->db->rollback(); + SecureLogger::info(__('Attivazione prestito rinviata: copia assegnata non in sede'), [ + 'prestito_id' => $loanId, + 'libro_id' => $bookId, + 'copia_id' => $copiaId, + 'copia_stato' => $copyState + ]); + continue; + } + } + // Calculate pickup deadline dal "oggi" applicativo, cappata a // data_scadenza (L1): senza il cap un prestito con finestra corta // restava ritirabile (e la copia bloccata) oltre la fine del diff --git a/locale/da_DK.json b/locale/da_DK.json index f9e4ea3a6..0cf15f9ef 100644 --- a/locale/da_DK.json +++ b/locale/da_DK.json @@ -6873,5 +6873,7 @@ "Ultimo Sollecito:": "Seneste rykker:", "%s prestito selezionato riceverà il sollecito (solo se scaduto).": "%s valgt lån modtager rykkeren (kun hvis forfaldent).", "%s prestiti selezionati riceveranno il sollecito (solo quelli scaduti).": "%s valgte lån modtager rykkeren (kun de forfaldne).", - "Impossibile generare la ricevuta PDF.": "Kvitterings-PDF'en kunne ikke genereres." + "Impossibile generare la ricevuta PDF.": "Kvitterings-PDF'en kunne ikke genereres.", + "Attivazione prestito rinviata: nessuna copia libera": "Aktivering af lån udskudt: intet eksemplar ledigt", + "Attivazione prestito rinviata: copia assegnata non in sede": "Aktivering af lån udskudt: tildelt eksemplar ikke på biblioteket" } diff --git a/locale/de_DE.json b/locale/de_DE.json index 7f95c8cfb..716676e5e 100644 --- a/locale/de_DE.json +++ b/locale/de_DE.json @@ -6873,5 +6873,7 @@ "Ultimo Sollecito:": "Letzte Mahnung:", "%s prestito selezionato riceverà il sollecito (solo se scaduto).": "%s ausgewählte Ausleihe erhält die Mahnung (nur bei Überfälligkeit).", "%s prestiti selezionati riceveranno il sollecito (solo quelli scaduti).": "%s ausgewählte Ausleihen erhalten die Mahnung (nur die überfälligen).", - "Impossibile generare la ricevuta PDF.": "Die Quittungs-PDF konnte nicht erstellt werden." + "Impossibile generare la ricevuta PDF.": "Die Quittungs-PDF konnte nicht erstellt werden.", + "Attivazione prestito rinviata: nessuna copia libera": "Ausleihe-Aktivierung verschoben: kein Exemplar frei", + "Attivazione prestito rinviata: copia assegnata non in sede": "Ausleihe-Aktivierung verschoben: zugewiesenes Exemplar nicht in der Bibliothek" } diff --git a/locale/en_US.json b/locale/en_US.json index 05dd86655..d0d215ae9 100644 --- a/locale/en_US.json +++ b/locale/en_US.json @@ -6873,5 +6873,7 @@ "Ultimo Sollecito:": "Last Reminder:", "%s prestito selezionato riceverà il sollecito (solo se scaduto).": "%s selected loan will receive the reminder (only if overdue).", "%s prestiti selezionati riceveranno il sollecito (solo quelli scaduti).": "%s selected loans will receive the reminder (only the overdue ones).", - "Impossibile generare la ricevuta PDF.": "Could not generate the receipt PDF." + "Impossibile generare la ricevuta PDF.": "Could not generate the receipt PDF.", + "Attivazione prestito rinviata: nessuna copia libera": "Loan activation deferred: no copy free", + "Attivazione prestito rinviata: copia assegnata non in sede": "Loan activation deferred: assigned copy not in library" } diff --git a/locale/fr_FR.json b/locale/fr_FR.json index de268b543..595bc47c4 100644 --- a/locale/fr_FR.json +++ b/locale/fr_FR.json @@ -6873,5 +6873,7 @@ "Ultimo Sollecito:": "Dernier rappel :", "%s prestito selezionato riceverà il sollecito (solo se scaduto).": "%s prêt sélectionné recevra le rappel (uniquement s'il est en retard).", "%s prestiti selezionati riceveranno il sollecito (solo quelli scaduti).": "%s prêts sélectionnés recevront le rappel (uniquement ceux en retard).", - "Impossibile generare la ricevuta PDF.": "Impossible de générer le PDF du reçu." + "Impossibile generare la ricevuta PDF.": "Impossible de générer le PDF du reçu.", + "Attivazione prestito rinviata: nessuna copia libera": "Activation du prêt reportée : aucun exemplaire libre", + "Attivazione prestito rinviata: copia assegnata non in sede": "Activation du prêt reportée : exemplaire attribué absent de la bibliothèque" } diff --git a/locale/it_IT.json b/locale/it_IT.json index 7dc3e344f..0444f8a9d 100644 --- a/locale/it_IT.json +++ b/locale/it_IT.json @@ -6873,5 +6873,7 @@ "Ultimo Sollecito:": "Ultimo Sollecito:", "%s prestito selezionato riceverà il sollecito (solo se scaduto).": "%s prestito selezionato riceverà il sollecito (solo se scaduto).", "%s prestiti selezionati riceveranno il sollecito (solo quelli scaduti).": "%s prestiti selezionati riceveranno il sollecito (solo quelli scaduti).", - "Impossibile generare la ricevuta PDF.": "Impossibile generare la ricevuta PDF." + "Impossibile generare la ricevuta PDF.": "Impossibile generare la ricevuta PDF.", + "Attivazione prestito rinviata: nessuna copia libera": "Attivazione prestito rinviata: nessuna copia libera", + "Attivazione prestito rinviata: copia assegnata non in sede": "Attivazione prestito rinviata: copia assegnata non in sede" } diff --git a/tests/pickup-ready-copy-free-366.unit.php b/tests/pickup-ready-copy-free-366.unit.php new file mode 100644 index 000000000..2f9966b94 --- /dev/null +++ b/tests/pickup-ready-copy-free-366.unit.php @@ -0,0 +1,253 @@ + stays 'prenotato', no pickup_deadline + * (the pickup email only fires after a successful promotion commit, + * so no promotion == no email). + * 2. Same with predecessor flipped to 'in_ritardo' -> still 'prenotato'. + * 3. Predecessor returned -> next sweep promotes to 'da_ritirare' with a + * pickup_deadline. + * 4. Multi-copy: 2 copies, 1 out overdue -> the reservation on the free + * copy IS promoted (multi-copy behaviour preserved). + * 5. Multi-copy, reservation pinned to the copy that is still out -> stays + * 'prenotato' even though the book-level count has room; promotes after + * that copy is returned. + * + * Drives the real MaintenanceService::activateScheduledLoans() against the + * live DB. Touches only data it creates (titles ZZ_366_%, users zz366-%). + * + * Run: php tests/pickup-ready-copy-free-366.unit.php + */ + +use App\Support\MaintenanceService; + +$root = dirname(__DIR__); +require $root . '/vendor/autoload.php'; +mysqli_report(MYSQLI_REPORT_ERROR | MYSQLI_REPORT_STRICT); + +function prcfenv(string $path): array +{ + $env = []; + foreach (preg_split('/\r?\n/', (string) @file_get_contents($path)) as $line) { + $line = trim($line); + if ($line === '' || $line[0] === '#' || !str_contains($line, '=')) { + continue; + } + [$k, $v] = explode('=', $line, 2); + $k = trim($k); + $v = trim($v); + if (strlen($v) >= 2 && ($v[0] === '"' || $v[0] === "'") && $v[-1] === $v[0]) { + $v = substr($v, 1, -1); + } + $env[$k] = $v; + } + return $env; +} + +$env = prcfenv($root . '/.env'); +$dbName = $env['DB_NAME'] ?? ''; +$dbUser = $env['DB_USER'] ?? ''; +$dbPass = $env['DB_PASS'] ?? ($env['DB_PASSWORD'] ?? ''); +$socket = getenv('E2E_DB_SOCKET') ?: ($env['DB_SOCKET'] ?? '/opt/homebrew/var/mysql/mysql.sock'); +try { + $db = (is_string($socket) && $socket !== '' && file_exists($socket)) + ? new mysqli(null, $dbUser, $dbPass, $dbName, 0, $socket) + : new mysqli($env['DB_HOST'] ?? '127.0.0.1', $dbUser, $dbPass, $dbName, (int) ($env['DB_PORT'] ?? 3306)); +} catch (\Throwable $e) { + echo "SKIP: database not reachable (" . $e->getMessage() . ")\n"; + exit(0); +} +$db->set_charset('utf8mb4'); + +$TESTNO = 0; +$failed = 0; +function check(bool $cond, string $desc): void +{ + global $TESTNO, $failed; + $TESTNO++; + printf("[%02d] %s: %s\n", $TESTNO, $cond ? 'PASS' : 'FAIL', $desc); + if (!$cond) { + $failed++; + } +} + +// Per-run token: cleanup and assertions only ever touch this run's rows. +$RUN = bin2hex(random_bytes(6)); +$TITLE_PREFIX = "ZZ_366_{$RUN}_"; +$EMAIL_SUFFIX = "@example.invalid"; + +$today = \App\Support\DateHelper::today(); +$d = static fn (int $offsetDays): string => date('Y-m-d', strtotime($today . ($offsetDays >= 0 ? " +{$offsetDays} days" : ' ' . $offsetDays . ' days'))); + +/* -------------------------------- helpers -------------------------------- */ + +$mkUser = static function (string $tag) use ($db, $RUN, $EMAIL_SUFFIX): int { + $tessera = 'Z366' . strtoupper($tag) . substr($RUN, 0, 10); + $email = "zz366-{$tag}-{$RUN}{$EMAIL_SUFFIX}"; + $stmt = $db->prepare("INSERT INTO utenti (codice_tessera, nome, cognome, email, password, stato, tipo_utente, email_verificata) + VALUES (?, 'Test', ?, ?, 'x', 'attivo', 'standard', 1)"); + $cognome = "ZZ366 {$tag}"; + $stmt->bind_param('sss', $tessera, $cognome, $email); + $stmt->execute(); + $stmt->close(); + return (int) $db->insert_id; +}; + +$mkBook = static function (string $tag, int $copies) use ($db, $TITLE_PREFIX): array { + $title = $TITLE_PREFIX . $tag; + $stmt = $db->prepare("INSERT INTO libri (titolo, copie_totali, copie_disponibili, created_at, updated_at) + VALUES (?, ?, ?, NOW(), NOW())"); + $stmt->bind_param('sii', $title, $copies, $copies); + $stmt->execute(); + $stmt->close(); + $bookId = (int) $db->insert_id; + $copyIds = []; + for ($i = 1; $i <= $copies; $i++) { + $code = "ZZ366-{$bookId}-C{$i}"; + $stmt = $db->prepare("INSERT INTO copie (libro_id, numero_inventario, stato) VALUES (?, ?, 'disponibile')"); + $stmt->bind_param('is', $bookId, $code); + $stmt->execute(); + $stmt->close(); + $copyIds[] = (int) $db->insert_id; + } + return [$bookId, $copyIds]; +}; + +$mkLoan = static function (int $bookId, ?int $copiaId, int $userId, string $stato, string $from, string $to, int $attivo = 1) use ($db): int { + $stmt = $db->prepare("INSERT INTO prestiti (libro_id, copia_id, utente_id, data_prestito, data_scadenza, stato, origine, attivo) + VALUES (?, ?, ?, ?, ?, ?, 'diretto', ?)"); + $stmt->bind_param('iiisssi', $bookId, $copiaId, $userId, $from, $to, $stato, $attivo); + $stmt->execute(); + $stmt->close(); + return (int) $db->insert_id; +}; + +$setCopyState = static function (int $copiaId, string $stato) use ($db): void { + $stmt = $db->prepare("UPDATE copie SET stato = ? WHERE id = ?"); + $stmt->bind_param('si', $stato, $copiaId); + $stmt->execute(); + $stmt->close(); +}; + +$loanRow = static function (int $loanId) use ($db): array { + $stmt = $db->prepare("SELECT stato, pickup_deadline FROM prestiti WHERE id = ?"); + $stmt->bind_param('i', $loanId); + $stmt->execute(); + $row = $stmt->get_result()->fetch_assoc() ?: []; + $stmt->close(); + return $row; +}; + +$returnLoan = static function (int $loanId, ?int $copiaId) use ($db, $setCopyState, $today): void { + $stmt = $db->prepare("UPDATE prestiti SET stato = 'restituito', attivo = 0, data_restituzione = ? WHERE id = ?"); + $stmt->bind_param('si', $today, $loanId); + $stmt->execute(); + $stmt->close(); + if ($copiaId !== null) { + $setCopyState($copiaId, 'disponibile'); + } +}; + +/* -------------------------------- cleanup -------------------------------- */ +$cleanup = static function () use ($db, $TITLE_PREFIX, $RUN, $EMAIL_SUFFIX): void { + $like = $db->real_escape_string($TITLE_PREFIX) . '%'; + $db->query("DELETE p FROM prestiti p JOIN libri l ON l.id = p.libro_id WHERE l.titolo LIKE '{$like}'"); + $db->query("DELETE c FROM copie c JOIN libri l ON l.id = c.libro_id WHERE l.titolo LIKE '{$like}'"); + $db->query("DELETE FROM libri WHERE titolo LIKE '{$like}'"); + $emailLike = $db->real_escape_string("zz366-%-{$RUN}{$EMAIL_SUFFIX}"); + $db->query("DELETE FROM utenti WHERE email LIKE '{$emailLike}'"); +}; +$cleanup(); + +try { + $svc = new MaintenanceService($db); + $borrower = $mkUser('a'); + $reserver = $mkUser('b'); + + /* ---- Scenario 1: single copy, predecessor still out (issue #366) ---- */ + [$book1, $copies1] = $mkBook('one-copy', 1); + $c1 = $copies1[0]; + // Predecessor loan, picked up 30 days ago, due 5 days ago, NOT returned. + // Deliberately left in 'in_corso' (stale) — runAll() flips overdue loans + // to 'in_ritardo' only AFTER activateScheduledLoans, so this is exactly + // the state the sweep sees on a real run. + $prev1 = $mkLoan($book1, $c1, $borrower, 'in_corso', $d(-30), $d(-5)); + $setCopyState($c1, 'prestato'); + // Queued reservation right after the loan: window started 2 days ago. + // No copy pinned (queued behind the outstanding loan). + $res1 = $mkLoan($book1, null, $reserver, 'prenotato', $d(-2), $d(20)); + + $svc->activateScheduledLoans(); + $row = $loanRow($res1); + check(($row['stato'] ?? '') === 'prenotato', "1 copy + predecessor 'in_corso' past due: reservation stays 'prenotato' (no promotion, no pickup email)"); + check(($row['pickup_deadline'] ?? null) === null, 'blocked reservation gets no pickup_deadline'); + + // Flip the predecessor to 'in_ritardo' (what updateOverdueLoans does) and + // sweep again: still no free copy, still no promotion. + $db->query("UPDATE prestiti SET stato = 'in_ritardo' WHERE id = {$prev1}"); + $svc->activateScheduledLoans(); + $row = $loanRow($res1); + check(($row['stato'] ?? '') === 'prenotato', "predecessor 'in_ritardo': reservation still 'prenotato'"); + + // Return the predecessor: the very next sweep must promote. + $returnLoan($prev1, $c1); + $svc->activateScheduledLoans(); + $row = $loanRow($res1); + check(($row['stato'] ?? '') === 'da_ritirare', "predecessor returned: reservation promotes to 'da_ritirare'"); + check(!empty($row['pickup_deadline']), 'promoted reservation gets a pickup_deadline'); + + /* ---- Scenario 2: multi-copy, one out overdue, one free -> promote ---- */ + [$book2, $copies2] = $mkBook('two-copies', 2); + [$c2a, $c2b] = $copies2; + $prev2 = $mkLoan($book2, $c2a, $borrower, 'in_ritardo', $d(-30), $d(-5)); + $setCopyState($c2a, 'prestato'); + $res2 = $mkLoan($book2, $c2b, $reserver, 'prenotato', $d(-1), $d(20)); + $setCopyState($c2b, 'prenotato'); + + $svc->activateScheduledLoans(); + $row = $loanRow($res2); + check(($row['stato'] ?? '') === 'da_ritirare', '2 copies / 1 out overdue: reservation on the free copy IS promoted'); + + /* ---- Scenario 3: multi-copy but pinned to the copy still out -------- */ + [$book3, $copies3] = $mkBook('pinned-copy', 2); + [$c3a, $c3b] = $copies3; + $prev3 = $mkLoan($book3, $c3a, $borrower, 'in_corso', $d(-30), $d(-5)); + $setCopyState($c3a, 'prestato'); + // Reservation pinned to the very copy that is still out (non-overlapping + // window, so the DB trigger allows the row — the #366 blind spot). + $res3 = $mkLoan($book3, $c3a, $reserver, 'prenotato', $d(-1), $d(20)); + + $svc->activateScheduledLoans(); + $row = $loanRow($res3); + check(($row['stato'] ?? '') === 'prenotato', "reservation pinned to the out copy stays 'prenotato' even with book-level room"); + + $returnLoan($prev3, $c3a); + $setCopyState($c3a, 'prenotato'); // copy back on the shelf, held for the reservation + $svc->activateScheduledLoans(); + $row = $loanRow($res3); + check(($row['stato'] ?? '') === 'da_ritirare', "pinned copy returned: reservation promotes to 'da_ritirare'"); + +} catch (\Throwable $e) { + $cleanup(); + fwrite(STDERR, 'FAIL: ' . $e->getMessage() . "\n"); + exit(1); +} + +$cleanup(); +$db->close(); +echo "\n" . ($failed === 0 ? "ALL {$TESTNO} PASS\n" : "{$failed}/{$TESTNO} FAILED\n"); +exit($failed > 0 ? 1 : 0); From e18a9034a8307edb5efc6cc0cfecea666ce922ba Mon Sep 17 00:00:00 2001 From: fabiodalez-dev Date: Thu, 20 Aug 2026 11:17:04 +0200 Subject: [PATCH 12/16] fix(loans): lock-first ordering in circulation transactions to end MVCC stale-read races Under InnoDB REPEATABLE READ the read view is fixed at a transaction's first consistent read. Eight circulation transactions did begin_transaction() -> plain SELECT libro_id -> SELECT ... FROM libri FOR UPDATE, so the snapshot predated the book lock; once the lock was granted (exactly when a competitor held it), every later plain read still saw pre-lock state. Effects: a just-cancelled reservation was promoted and emailed 'book available', and the same copia_id could be committed to two loans (or trip the overlap trigger, aborting an unrelated return/cancel with a 500). - Move the id-resolution lookup before begin_transaction() in approveLoan, rejectLoan, cancelPickup, returnLoan (LoanApprovalController), processReturn (PrestitiController), close (LoanRepository), cancelLoan, cancelReservation (UserActionsController), so the first in-transaction statement is the locking libri FOR UPDATE read and the read view is created post-lock. Canonical lock order (libri -> prestiti -> copie) preserved; existing post-lock re-reads kept. - ReservationManager::processBookAvailability: read the queue head FOR UPDATE and claim the reservation with a state-guarded UPDATE (SET stato='completata' WHERE id=? AND stato='attiva', affected_rows checked) BEFORE creating the loan; 0 rows -> skip (no loan, no email). Compensating revert to 'attiva' if allocation then fails under an external transaction. - createLoanFromReservation: the copy-overlap re-check is now a locking read. New two-connection concurrency test mvcc-lockfirst-circulation.unit.php (30 checks): reproduces both races (they fail pre-fix), all pass post-fix. --- app/Controllers/LoanApprovalController.php | 83 +++-- app/Controllers/PrestitiController.php | 33 +- app/Controllers/ReservationManager.php | 59 +++- app/Controllers/UserActionsController.php | 94 ++--- app/Models/LoanRepository.php | 35 +- tests/mvcc-lockfirst-circulation.unit.php | 378 +++++++++++++++++++++ 6 files changed, 558 insertions(+), 124 deletions(-) create mode 100644 tests/mvcc-lockfirst-circulation.unit.php diff --git a/app/Controllers/LoanApprovalController.php b/app/Controllers/LoanApprovalController.php index 186ca6bb6..e3f831384 100644 --- a/app/Controllers/LoanApprovalController.php +++ b/app/Controllers/LoanApprovalController.php @@ -171,19 +171,19 @@ public function approveLoan(Request $request, Response $response, mysqli $db): R } try { - $db->begin_transaction(); - // ORDINE DI LOCK CANONICO (P3): la riga `libri` per prima, poi `prestiti`. - // Determiniamo il libro del prestito con una lettura NON bloccante, poi - // acquisiamo i lock nell'ordine libri -> prestiti come tutti gli altri - // entry point, evitando deadlock da lock-order inversion. + // Determiniamo il libro del prestito con una lettura NON bloccante PRIMA + // di aprire la transazione (lock-first, come update()/renew()): sotto + // REPEATABLE READ la read view nasce alla prima consistent read della + // transazione, e farla nascere prima del lock renderebbe ogni SELECT + // non bloccante successiva cieca ai commit concorrenti avvenuti mentre + // aspettavamo il lock del libro. $bookLookup = $db->prepare("SELECT libro_id FROM prestiti WHERE id = ? AND stato = 'pendente'"); $bookLookup->bind_param('i', $loanId); $bookLookup->execute(); $bookRow = $bookLookup->get_result()->fetch_assoc(); $bookLookup->close(); if (!$bookRow) { - $db->rollback(); $response->getBody()->write(json_encode([ 'success' => false, 'message' => __('Prestito non trovato o già processato') @@ -192,8 +192,11 @@ public function approveLoan(Request $request, Response $response, mysqli $db): R } $libroId = (int) $bookRow['libro_id']; + $db->begin_transaction(); + // Lock della riga `libri` PRIMA — serializza anche le approvazioni dello - // stesso libro (CONC-03). + // stesso libro (CONC-03) — ed è la PRIMA statement della transazione, + // così la read view viene creata solo a lock acquisito (post-competitor). $lockBookStmt = $db->prepare("SELECT id FROM libri WHERE id = ? AND deleted_at IS NULL FOR UPDATE"); $lockBookStmt->bind_param('i', $libroId); $lockBookStmt->execute(); @@ -595,29 +598,32 @@ public function rejectLoan(Request $request, Response $response, mysqli $db): Re return $response->withHeader('Content-Type', 'application/json')->withStatus(400); } + // Canonical lock order: resolve the book without locking, lock `libri` + // first, then lock the pending loan. DataIntegrity locks the same book + // during the availability recalculation; taking the loan first here + // inverted the order used by approval/return and could deadlock. + // Lock-first (MVCC): the lookup runs BEFORE begin_transaction() so the + // REPEATABLE READ view is created only after the book lock is acquired — + // a plain in-txn read here would freeze a pre-lock snapshot and blind + // every later non-locking SELECT to concurrent committed changes. + $lookup = $db->prepare("SELECT libro_id FROM prestiti WHERE id = ? AND stato = 'pendente'"); + $lookup->bind_param('i', $loanId); + $lookup->execute(); + $lookupRow = $lookup->get_result()->fetch_assoc(); + $lookup->close(); + if (!$lookupRow) { + $response->getBody()->write(json_encode([ + 'success' => false, + 'message' => __('Prestito non trovato o già processato') + ])); + return $response->withHeader('Content-Type', 'application/json')->withStatus(400); + } + $bookId = (int) $lookupRow['libro_id']; + // Start transaction for the atomic terminal transition + availability update. $db->begin_transaction(); try { - // Canonical lock order: resolve the book without locking, lock `libri` - // first, then lock the pending loan. DataIntegrity locks the same book - // during the availability recalculation; taking the loan first here - // inverted the order used by approval/return and could deadlock. - $lookup = $db->prepare("SELECT libro_id FROM prestiti WHERE id = ? AND stato = 'pendente'"); - $lookup->bind_param('i', $loanId); - $lookup->execute(); - $lookupRow = $lookup->get_result()->fetch_assoc(); - $lookup->close(); - if (!$lookupRow) { - $db->rollback(); - $response->getBody()->write(json_encode([ - 'success' => false, - 'message' => __('Prestito non trovato o già processato') - ])); - return $response->withHeader('Content-Type', 'application/json')->withStatus(400); - } - $bookId = (int) $lookupRow['libro_id']; - // NIENTE filtro deleted_at qui né nella JOIN sottostante (eccezione // deliberata al soft-delete invariant): rifiutare una richiesta pendente // deve funzionare ANCHE se il libro è stato soft-eliminato nel frattempo — @@ -978,19 +984,21 @@ public function cancelPickup(Request $request, Response $response, mysqli $db): } try { - $db->begin_transaction(); $today = DateHelper::today(); // ORDINE DI LOCK CANONICO (P3): la riga `libri` per prima, poi `prestiti`. - // Lettura NON bloccante del libro, poi lock nell'ordine libri -> prestiti - // come approveLoan/store/renew (M2, niente lock-order inversion). + // Lettura NON bloccante del libro PRIMA di begin_transaction() (lock-first, + // MVCC): la read view REPEATABLE READ nasce alla prima consistent read in + // transazione — se nascesse qui, prima del lock, le SELECT non bloccanti + // successive non vedrebbero i commit concorrenti avvenuti durante l'attesa + // del lock. Poi lock nell'ordine libri -> prestiti come approveLoan/store/ + // renew (M2, niente lock-order inversion). $bookLookup = $db->prepare("SELECT libro_id FROM prestiti WHERE id = ?"); $bookLookup->bind_param('i', $loanId); $bookLookup->execute(); $bookRow = $bookLookup->get_result()->fetch_assoc(); $bookLookup->close(); if (!$bookRow) { - $db->rollback(); $response->getBody()->write(json_encode([ 'success' => false, 'message' => __('Prestito non trovato o non cancellabile') @@ -999,6 +1007,8 @@ public function cancelPickup(Request $request, Response $response, mysqli $db): } $libroId = (int) $bookRow['libro_id']; + $db->begin_transaction(); + // Lock della riga `libri` SENZA filtro deleted_at: l'annullamento di un // ritiro deve sempre poter procedere anche su libro soft-deleted (vedi // LoanRepository::close), altrimenti prestito e copia resterebbero @@ -1152,18 +1162,19 @@ public function returnLoan(Request $request, Response $response, mysqli $db): Re } try { - $db->begin_transaction(); - // ORDINE DI LOCK CANONICO (P3): la riga `libri` per prima, poi `prestiti`. - // Lettura NON bloccante del libro, poi lock nell'ordine libri -> prestiti - // come approveLoan/store/renew (M2, niente lock-order inversion). + // Lettura NON bloccante del libro PRIMA di begin_transaction() (lock-first, + // MVCC): la read view REPEATABLE READ nasce alla prima consistent read in + // transazione — anticiparla al pre-lock renderebbe le SELECT non bloccanti + // successive (promozione coda, capacity gate) cieche ai commit concorrenti + // avvenuti durante l'attesa del lock. Poi lock nell'ordine libri -> + // prestiti come approveLoan/store/renew (M2, niente lock-order inversion). $bookLookup = $db->prepare("SELECT libro_id FROM prestiti WHERE id = ?"); $bookLookup->bind_param('i', $loanId); $bookLookup->execute(); $bookRow = $bookLookup->get_result()->fetch_assoc(); $bookLookup->close(); if (!$bookRow) { - $db->rollback(); $response->getBody()->write(json_encode([ 'success' => false, 'message' => __('Prestito non trovato o non restituibile') @@ -1172,6 +1183,8 @@ public function returnLoan(Request $request, Response $response, mysqli $db): Re } $libroId = (int) $bookRow['libro_id']; + $db->begin_transaction(); + // Lock della riga `libri` SENZA filtro deleted_at: la RESTITUZIONE deve // sempre poter procedere anche su libro soft-deleted (vedi il commento in // LoanRepository::close), altrimenti prestito e copia resterebbero diff --git a/app/Controllers/PrestitiController.php b/app/Controllers/PrestitiController.php index 534c03ca3..0d320140c 100644 --- a/app/Controllers/PrestitiController.php +++ b/app/Controllers/PrestitiController.php @@ -1174,24 +1174,27 @@ public function processReturn(Request $request, Response $response, mysqli $db, // UTC) un rientro poco dopo mezzanotte verrebbe datato al giorno prima. $data_restituzione = \App\Support\DateHelper::today(); + // ORDINE DI LOCK CANONICO (P3): determina il libro del prestito con una + // lettura NON bloccante PRIMA di begin_transaction() (lock-first, MVCC: + // la read view REPEATABLE READ nasce alla prima consistent read in + // transazione — anticiparla al pre-lock renderebbe le SELECT non bloccanti + // successive cieche ai commit concorrenti avvenuti durante l'attesa del + // lock), poi blocca la riga `libri` PRIMA e infine quella del prestito — + // stesso ordine di store/approveLoan/renew/close per evitare deadlock con + // le approvazioni concorrenti sullo stesso libro. + $lookup = $db->prepare('SELECT libro_id FROM prestiti WHERE id = ?'); + $lookup->bind_param('i', $id); + $lookup->execute(); + $lrow = $lookup->get_result()->fetch_assoc(); + $lookup->close(); + if (!$lrow) { + return $response->withHeader('Location', url('/admin/loans/returned/' . $id) . '?error=not_returnable')->withStatus(302); + } + $libro_id = (int) $lrow['libro_id']; + // Avvia transazione $db->begin_transaction(); try { - // ORDINE DI LOCK CANONICO (P3): determina il libro del prestito con una - // lettura NON bloccante, poi blocca la riga `libri` PRIMA e infine quella - // del prestito — stesso ordine di store/approveLoan/renew/close per - // evitare deadlock con le approvazioni concorrenti sullo stesso libro. - $lookup = $db->prepare('SELECT libro_id FROM prestiti WHERE id = ?'); - $lookup->bind_param('i', $id); - $lookup->execute(); - $lrow = $lookup->get_result()->fetch_assoc(); - $lookup->close(); - if (!$lrow) { - $db->rollback(); - return $response->withHeader('Location', url('/admin/loans/returned/' . $id) . '?error=not_returnable')->withStatus(302); - } - $libro_id = (int) $lrow['libro_id']; - // Lock della riga `libri`. NIENTE filtro deleted_at (come in // LoanRepository::close()): la restituzione deve sempre poter procedere // anche su libro soft-deleted — la regola soft-delete governa diff --git a/app/Controllers/ReservationManager.php b/app/Controllers/ReservationManager.php index d3b5dba4f..9d4bb4add 100644 --- a/app/Controllers/ReservationManager.php +++ b/app/Controllers/ReservationManager.php @@ -182,8 +182,12 @@ public function processBookAvailability($bookId) // Get the next date-eligible reservation in queue // Only process reservations where start date <= today (ready to convert to loan) - // Note: Book-level lock above serializes all processing for this book, - // so we don't need row-level lock on prenotazioni here + // FOR UPDATE (locking/current read): the book-level lock serializes + // writers that follow the canonical order, but a CALLER's REPEATABLE + // READ snapshot can predate that lock — a plain read here would then + // still see a reservation that a competitor cancelled and committed + // while we waited for the book lock, and promote/email it anyway. + // The locking read always returns the latest committed row state. $stmt = $this->db->prepare(" SELECT r.*, u.email, u.nome, u.cognome FROM prenotazioni r @@ -192,6 +196,7 @@ public function processBookAvailability($bookId) AND " . \App\Support\LoanEligibility::promotableReservationWhere('r') . " ORDER BY r.queue_position ASC LIMIT 1 + FOR UPDATE "); $stmt->bind_param('iss', $bookId, $today, $today); $stmt->execute(); @@ -229,28 +234,49 @@ public function processBookAvailability($bookId) ? (int) $nextReservation['queue_position'] : null; if ($this->isDateRangeAvailable($bookId, $startDate, $endDate, (int) $nextReservation['id'], $headQueuePos)) { + // Claim the reservation FIRST with a state-guarded UPDATE. + // The `AND stato = 'attiva'` guard + affected_rows check is the + // last line of defense: if a competitor cancelled/changed this + // reservation and committed in the meantime (0 rows touched), + // we must NOT create the loan nor queue the "book available" + // email — an unguarded UPDATE here resurrected a cancelled + // reservation to 'completata' and emailed the user. + $claim = $this->db->prepare("UPDATE prenotazioni SET stato = 'completata' WHERE id = ? AND stato = 'attiva'"); + $claim->bind_param('i', $nextReservation['id']); + $claim->execute(); + $claimed = $claim->affected_rows; + $claim->close(); + if ($claimed !== 1) { + // Nothing mutated: skip this reservation entirely. + $this->commitIfOwned($ownTransaction); + return false; + } + // Create the loan - check return value to handle race conditions // Note: createLoanFromReservation() handles its own transaction internally // when called standalone, but here we're already in a transaction $loanCreated = $this->createLoanFromReservation($nextReservation); if ($loanCreated === false) { - // Race condition detected - loan creation failed + // Race condition detected - loan creation failed. Restore + // the claim first: with an EXTERNAL transaction we cannot + // roll back the owner's work, and leaving the row + // 'completata' without its loan would be committed by the + // caller (reservation lost). We still hold the row lock, + // so this compensating UPDATE cannot race. + $unclaim = $this->db->prepare("UPDATE prenotazioni SET stato = 'attiva' WHERE id = ? AND stato = 'completata'"); + $unclaim->bind_param('i', $nextReservation['id']); + $unclaim->execute(); + $unclaim->close(); $this->rollbackIfOwned($ownTransaction); return false; } - // Mark reservation as completed - $stmt = $this->db->prepare("UPDATE prenotazioni SET stato = 'completata' WHERE id = ?"); - $stmt->bind_param('i', $nextReservation['id']); - $stmt->execute(); - $stmt->close(); - - // BUG9/D4 double-subtraction fix: createLoanFromReservation() already - // recalc'd availability, but the source reservation was still 'attiva' - // then — so the new pendente+copy loan AND the waitlist slot were both - // counted. Recalc again now that the reservation is 'completata', so the - // commitment is counted exactly once. + // BUG9/D4 double-subtraction fix: the reservation is claimed + // 'completata' BEFORE createLoanFromReservation() recalcs, so + // the new pendente+copy loan and the waitlist slot are never + // both counted. Recalc once more after the allocation settles + // so the commitment is counted exactly once. $integrity = new \App\Support\DataIntegrity($this->db); if (!$integrity->recalculateBookAvailability($bookId, true)) { throw new \RuntimeException('Failed to recalculate availability after reservation promotion.'); @@ -409,6 +435,10 @@ private function createLoanFromReservation($reservation) $lockCopyStmt->execute(); $lockCopyStmt->close(); + // FOR UPDATE (locking/current read) like approveLoan's final overlap + // check: with a plain read a caller whose REPEATABLE READ snapshot + // predates the book lock would miss a loan a competitor just + // committed on this copy and double-commit the same copia_id. $overlapCopyStmt = $this->db->prepare(" SELECT 1 FROM prestiti WHERE copia_id = ? @@ -418,6 +448,7 @@ private function createLoanFromReservation($reservation) OR (stato = 'pendente' AND copia_id IS NOT NULL) -- pending conversion holds this copy (#157, model A-refined) ) LIMIT 1 + FOR UPDATE "); $overlapCopyStmt->bind_param('iss', $copyId, $endDate, $startDate); $overlapCopyStmt->execute(); diff --git a/app/Controllers/UserActionsController.php b/app/Controllers/UserActionsController.php index 197fdd344..463fb6dd3 100644 --- a/app/Controllers/UserActionsController.php +++ b/app/Controllers/UserActionsController.php @@ -131,35 +131,38 @@ public function cancelLoan(Request $request, Response $response, mysqli $db): Re } $uid = (int) $user['id']; - $db->begin_transaction(); + // ORDINE DI LOCK CANONICO (P3, M2): risolvi libro_id con una lettura + // NON bloccante PRIMA di begin_transaction() (lock-first, MVCC: la read + // view REPEATABLE READ nasce alla prima consistent read in transazione — + // anticiparla al pre-lock renderebbe le SELECT non bloccanti successive, + // promozione coda inclusa, cieche ai commit concorrenti avvenuti durante + // l'attesa del lock), blocca la riga `libri` PRIMA e solo dopo il prestito + // — stesso pattern di cancelReservation qui sotto. Lockare prima la + // riga prestiti incrocerebbe i lock con i percorsi di creazione/ + // approvazione (che vanno libri -> prestiti) causando deadlock. + // Note: 'pendente' has attivo=0, 'prenotato' has attivo=1 + $lookupStmt = $db->prepare(" + SELECT libro_id + FROM prestiti + WHERE id = ? AND utente_id = ? AND ( + (attivo = 0 AND stato = 'pendente') + OR (attivo = 1 AND stato = 'prenotato') + ) + "); + $lookupStmt->bind_param('ii', $loanId, $uid); + $lookupStmt->execute(); + $lookupRow = $lookupStmt->get_result()->fetch_assoc(); + $lookupStmt->close(); - try { - // ORDINE DI LOCK CANONICO (P3, M2): risolvi libro_id con una lettura - // NON bloccante, blocca la riga `libri` PRIMA e solo dopo il prestito - // — stesso pattern di cancelReservation qui sotto. Lockare prima la - // riga prestiti incrocerebbe i lock con i percorsi di creazione/ - // approvazione (che vanno libri -> prestiti) causando deadlock. - // Note: 'pendente' has attivo=0, 'prenotato' has attivo=1 - $lookupStmt = $db->prepare(" - SELECT libro_id - FROM prestiti - WHERE id = ? AND utente_id = ? AND ( - (attivo = 0 AND stato = 'pendente') - OR (attivo = 1 AND stato = 'prenotato') - ) - "); - $lookupStmt->bind_param('ii', $loanId, $uid); - $lookupStmt->execute(); - $lookupRow = $lookupStmt->get_result()->fetch_assoc(); - $lookupStmt->close(); + if (!$lookupRow) { + return $response->withHeader('Location', RouteTranslator::route('reservations') . '?error=not_found')->withStatus(302); + } - if (!$lookupRow) { - $db->rollback(); - return $response->withHeader('Location', RouteTranslator::route('reservations') . '?error=not_found')->withStatus(302); - } + $libroId = (int) $lookupRow['libro_id']; - $libroId = (int) $lookupRow['libro_id']; + $db->begin_transaction(); + try { // Lock della riga libri per serializzare rilascio copia, promozione // coda e ricalcolo disponibilità con gli altri percorsi sullo stesso libro. // CI-SOFT-DELETE-EXEMPT: user cancellation must release existing circulation state for a deleted book. @@ -275,28 +278,31 @@ public function cancelReservation(Request $request, Response $response, mysqli $ } $uid = (int) $user['id']; - $db->begin_transaction(); + // CANONICAL LOCK ORDER (P3, L7): resolve libro_id with a NON-blocking + // read BEFORE begin_transaction() (lock-first, MVCC: the REPEATABLE READ + // view is created at the transaction's first consistent read — creating + // it pre-lock would blind every later non-locking SELECT, queue + // promotion included, to commits that landed while waiting for the book + // lock), then lock the `libri` row FIRST and only then the reservation — + // same order as LoanRepository::close, so this path never crosses + // locks with the create/approve paths (which go libri -> rows). + $lookupStmt = $db->prepare("SELECT libro_id FROM prenotazioni WHERE id = ? AND utente_id = ? AND stato = 'attiva'"); + $lookupStmt->bind_param('ii', $rid, $uid); + $lookupStmt->execute(); + $lookupRow = $lookupStmt->get_result()->fetch_assoc(); + $lookupStmt->close(); - try { - // CANONICAL LOCK ORDER (P3, L7): resolve libro_id with a NON-blocking - // read, lock the `libri` row FIRST and only then the reservation — - // same order as LoanRepository::close, so this path never crosses - // locks with the create/approve paths (which go libri -> rows). - $lookupStmt = $db->prepare("SELECT libro_id FROM prenotazioni WHERE id = ? AND utente_id = ? AND stato = 'attiva'"); - $lookupStmt->bind_param('ii', $rid, $uid); - $lookupStmt->execute(); - $lookupRow = $lookupStmt->get_result()->fetch_assoc(); - $lookupStmt->close(); - - if (!$lookupRow) { - // Check if it's actually a loan/active reservation (prestiti table) request instead? - // Sometimes frontend might send reservation_id for prestiti items if confusingly named - $db->rollback(); - return $response->withHeader('Location', RouteTranslator::route('reservations') . '?error=not_found')->withStatus(302); - } + if (!$lookupRow) { + // Check if it's actually a loan/active reservation (prestiti table) request instead? + // Sometimes frontend might send reservation_id for prestiti items if confusingly named + return $response->withHeader('Location', RouteTranslator::route('reservations') . '?error=not_found')->withStatus(302); + } - $libroId = (int) $lookupRow['libro_id']; + $libroId = (int) $lookupRow['libro_id']; + $db->begin_transaction(); + + try { // Lock the book row to serialize the queue reorder + availability // recalculation with other paths working on the same book's queue. // CI-SOFT-DELETE-EXEMPT: reservation cancellation must unblock a deleted book's existing queue. diff --git a/app/Models/LoanRepository.php b/app/Models/LoanRepository.php index c3720c15f..64a370b89 100644 --- a/app/Models/LoanRepository.php +++ b/app/Models/LoanRepository.php @@ -148,25 +148,28 @@ public function getActiveLoanByBook(int $bookId): ?array */ public function close(int $id): bool { - $this->db->begin_transaction(); + // ORDINE DI LOCK CANONICO (P3): determina il libro del prestito con una + // lettura NON bloccante PRIMA di begin_transaction() (lock-first, MVCC), + // poi blocca la riga `libri` PRIMA e infine quella del prestito — stesso + // ordine di store/approveLoan/renew per evitare deadlock. La lettura sta + // FUORI dalla transazione: sotto REPEATABLE READ la read view nasce alla + // prima consistent read in transazione, e farla nascere prima del lock + // renderebbe le SELECT non bloccanti successive (promozione coda, + // capacity gate) cieche ai commit concorrenti avvenuti durante l'attesa + // del lock — es. una prenotazione appena annullata verrebbe promossa. + $lookup = $this->db->prepare('SELECT libro_id FROM prestiti WHERE id=?'); + $lookup->bind_param('i', $id); + $lookup->execute(); + $lrow = $lookup->get_result()->fetch_assoc(); + $lookup->close(); + if (!$lrow) { + return false; + } + $bookId = (int) $lrow['libro_id']; - $bookId = null; + $this->db->begin_transaction(); try { - // ORDINE DI LOCK CANONICO (P3): determina il libro del prestito con una - // lettura NON bloccante, poi blocca la riga `libri` PRIMA e infine quella del - // prestito — stesso ordine di store/approveLoan/renew per evitare deadlock. - $lookup = $this->db->prepare('SELECT libro_id FROM prestiti WHERE id=?'); - $lookup->bind_param('i', $id); - $lookup->execute(); - $lrow = $lookup->get_result()->fetch_assoc(); - $lookup->close(); - if (!$lrow) { - $this->db->rollback(); - return false; - } - $bookId = (int) $lrow['libro_id']; - // Lock della riga `libri` per serializzare il ricalcolo della // disponibilità. NB: NIENTE filtro deleted_at qui (e nessun bail) — // la RESTITUZIONE di un prestito deve sempre poter procedere anche se diff --git a/tests/mvcc-lockfirst-circulation.unit.php b/tests/mvcc-lockfirst-circulation.unit.php new file mode 100644 index 000000000..fbc6afde8 --- /dev/null +++ b/tests/mvcc-lockfirst-circulation.unit.php @@ -0,0 +1,378 @@ += 2 && ($v[0] === '"' || $v[0] === "'") && $v[-1] === $v[0]) { + $v = substr($v, 1, -1); + } + $env[$k] = $v; + } + return $env; +} + +$env = mvccenv($root . '/.env'); +$dbName = getenv('E2E_DB_NAME') ?: ($env['DB_NAME'] ?? ''); +$dbUser = getenv('E2E_DB_USER') ?: ($env['DB_USER'] ?? ''); +$dbPass = getenv('E2E_DB_PASS') !== false && getenv('E2E_DB_PASS') !== '' ? getenv('E2E_DB_PASS') : ($env['DB_PASS'] ?? ($env['DB_PASSWORD'] ?? '')); +$socket = getenv('E2E_DB_SOCKET') ?: ($env['DB_SOCKET'] ?? '/opt/homebrew/var/mysql/mysql.sock'); + +$connect = static function () use ($dbName, $dbUser, $dbPass, $socket, $env): mysqli { + $db = (is_string($socket) && $socket !== '' && file_exists($socket)) + ? new mysqli(null, $dbUser, $dbPass, $dbName, 0, $socket) + : new mysqli($env['DB_HOST'] ?? '127.0.0.1', $dbUser, $dbPass, $dbName, (int) ($env['DB_PORT'] ?? 3306)); + $db->set_charset('utf8mb4'); + return $db; +}; + +try { + $connA = $connect(); // transactional actor (the vulnerable write path) + $connB = $connect(); // concurrent competitor (autocommit) +} catch (\Throwable $e) { + echo "SKIP: database not reachable (" . $e->getMessage() . ")\n"; + exit(0); +} + +$TESTNO = 0; +$failed = 0; +function check(bool $cond, string $desc): void +{ + global $TESTNO, $failed; + $TESTNO++; + printf("[%02d] %s: %s\n", $TESTNO, $cond ? 'PASS' : 'FAIL', $desc); + if (!$cond) { + $failed++; + } +} + +// Per-run token: cleanup and assertions only ever touch this run's rows. +$RUN = bin2hex(random_bytes(6)); +$TITLE_PREFIX = "ZZ_MVCC_{$RUN}_"; +$EMAIL_SUFFIX = "@example.invalid"; + +$today = \App\Support\DateHelper::today(); +$d = static fn (int $offsetDays): string => date('Y-m-d', strtotime($today . ($offsetDays >= 0 ? " +{$offsetDays} days" : ' ' . $offsetDays . ' days'))); + +/* -------------------------------- helpers -------------------------------- */ + +$mkUser = static function (string $tag) use ($connB, $RUN, $EMAIL_SUFFIX): int { + $tessera = 'ZMV' . strtoupper($tag) . substr($RUN, 0, 9); + $email = "zzmvcc-{$tag}-{$RUN}{$EMAIL_SUFFIX}"; + $stmt = $connB->prepare("INSERT INTO utenti (codice_tessera, nome, cognome, email, password, stato, tipo_utente, email_verificata) + VALUES (?, 'Test', ?, ?, 'x', 'attivo', 'standard', 1)"); + $cognome = "ZZMVCC {$tag}"; + $stmt->bind_param('sss', $tessera, $cognome, $email); + $stmt->execute(); + $stmt->close(); + return (int) $connB->insert_id; +}; + +$mkBook = static function (string $tag, int $copies) use ($connB, $TITLE_PREFIX): array { + $title = $TITLE_PREFIX . $tag; + $stmt = $connB->prepare("INSERT INTO libri (titolo, copie_totali, copie_disponibili, created_at, updated_at) + VALUES (?, ?, ?, NOW(), NOW())"); + $stmt->bind_param('sii', $title, $copies, $copies); + $stmt->execute(); + $stmt->close(); + $bookId = (int) $connB->insert_id; + $copyIds = []; + for ($i = 1; $i <= $copies; $i++) { + $code = "ZZMVCC-{$bookId}-C{$i}"; + $stmt = $connB->prepare("INSERT INTO copie (libro_id, numero_inventario, stato) VALUES (?, ?, 'disponibile')"); + $stmt->bind_param('is', $bookId, $code); + $stmt->execute(); + $stmt->close(); + $copyIds[] = (int) $connB->insert_id; + } + return [$bookId, $copyIds]; +}; + +$mkReservation = static function (int $bookId, int $userId, string $from, string $to, int $queuePos = 1) use ($connB): int { + $stmt = $connB->prepare("INSERT INTO prenotazioni (libro_id, utente_id, data_inizio_richiesta, data_fine_richiesta, queue_position, stato) + VALUES (?, ?, ?, ?, ?, 'attiva')"); + $stmt->bind_param('iissi', $bookId, $userId, $from, $to, $queuePos); + $stmt->execute(); + $stmt->close(); + return (int) $connB->insert_id; +}; + +$reservationState = static function (int $id) use ($connB): string { + $stmt = $connB->prepare("SELECT stato FROM prenotazioni WHERE id = ?"); + $stmt->bind_param('i', $id); + $stmt->execute(); + $row = $stmt->get_result()->fetch_assoc(); + $stmt->close(); + return (string) ($row['stato'] ?? ''); +}; + +$loanCountOnCopy = static function (int $copiaId) use ($connB): int { + $stmt = $connB->prepare("SELECT COUNT(*) AS n FROM prestiti WHERE copia_id = ? AND stato NOT IN ('restituito','annullato')"); + $stmt->bind_param('i', $copiaId); + $stmt->execute(); + $row = $stmt->get_result()->fetch_assoc(); + $stmt->close(); + return (int) ($row['n'] ?? 0); +}; + +$promoLoanCount = static function (int $bookId, int $userId) use ($connB): int { + $stmt = $connB->prepare("SELECT COUNT(*) AS n FROM prestiti WHERE libro_id = ? AND utente_id = ? AND origine = 'prenotazione'"); + $stmt->bind_param('ii', $bookId, $userId); + $stmt->execute(); + $row = $stmt->get_result()->fetch_assoc(); + $stmt->close(); + return (int) ($row['n'] ?? 0); +}; + +$deferredNotifications = static function (ReservationManager $rm): array { + $prop = new \ReflectionProperty(ReservationManager::class, 'deferredReservationNotifications'); + $prop->setAccessible(true); + return (array) $prop->getValue($rm); +}; + +/* -------------------------------- cleanup -------------------------------- */ +$cleanup = static function () use ($connB, $TITLE_PREFIX, $RUN, $EMAIL_SUFFIX): void { + $like = $connB->real_escape_string($TITLE_PREFIX) . '%'; + $connB->query("DELETE p FROM prestiti p JOIN libri l ON l.id = p.libro_id WHERE l.titolo LIKE '{$like}'"); + $connB->query("DELETE r FROM prenotazioni r JOIN libri l ON l.id = r.libro_id WHERE l.titolo LIKE '{$like}'"); + $connB->query("DELETE c FROM copie c JOIN libri l ON l.id = c.libro_id WHERE l.titolo LIKE '{$like}'"); + $connB->query("DELETE FROM libri WHERE titolo LIKE '{$like}'"); + $emailLike = $connB->real_escape_string("zzmvcc-%-{$RUN}{$EMAIL_SUFFIX}"); + $connB->query("DELETE FROM utenti WHERE email LIKE '{$emailLike}'"); +}; +$cleanup(); + +try { + $reserver = $mkUser('a'); + $competitor = $mkUser('b'); + + /* ===================================================================== + * RACE 1 — cancelled reservation must NOT be resurrected nor emailed. + * ===================================================================== */ + [$book1, $copies1] = $mkBook('race1', 1); + $res1 = $mkReservation($book1, $reserver, $d(-1), $d(14)); + + // Connection A: mirror the PRE-FIX transaction shape of the vulnerable + // callers (close/cancelLoan/...): open the txn and freeze the read view + // with a plain consistent read BEFORE acquiring the book lock. + $connA->begin_transaction(); + $freeze = $connA->prepare("SELECT libro_id FROM prestiti WHERE id = ?"); + $zero = 0; + $freeze->bind_param('i', $zero); + $freeze->execute(); + $freeze->get_result(); + $freeze->close(); + + // Connection B (the competitor): the user cancels the reservation — and + // COMMITS — while A is on its way to the book lock. + $connB->query("UPDATE prenotazioni SET stato = 'annullata' WHERE id = {$res1}"); + + // Connection A: acquire the book lock (first lock, as post-fix code does), + // then run the REAL promotion inside A's transaction. + $lock = $connA->prepare("SELECT id FROM libri WHERE id = ? FOR UPDATE"); + $lock->bind_param('i', $book1); + $lock->execute(); + $lock->get_result(); + $lock->close(); + + // Precondition of the race: A's PLAIN snapshot is genuinely stale — it + // still sees the cancelled reservation as 'attiva'. (True pre- and + // post-fix: this is InnoDB REPEATABLE READ, not the code under test.) + $staleStmt = $connA->prepare("SELECT stato FROM prenotazioni WHERE id = ?"); + $staleStmt->bind_param('i', $res1); + $staleStmt->execute(); + $staleRow = $staleStmt->get_result()->fetch_assoc(); + $staleStmt->close(); + check(($staleRow['stato'] ?? '') === 'attiva', "race 1 precondition: A's plain snapshot still sees the cancelled reservation as 'attiva' (stale MVCC view reproduced)"); + + $rm1 = new ReservationManager($connA); + $rm1->setExternalTransaction(true); + $promoted = $rm1->processBookAvailability($book1); + $connA->commit(); + + check($promoted === false, 'race 1: promotion reports false for the cancelled reservation'); + check($reservationState($res1) === 'annullata', "race 1: reservation stays 'annullata' (not resurrected to 'completata')"); + check($promoLoanCount($book1, $reserver) === 0, 'race 1: no loan created from the cancelled reservation'); + check(count($deferredNotifications($rm1)) === 0, 'race 1: no "book available" notification queued for the cancelled reservation'); + + /* ===================================================================== + * RACE 2 — the same copia_id must NOT be committed to two loans. + * ===================================================================== */ + [$book2, $copies2] = $mkBook('race2', 1); + $c2 = $copies2[0]; + $res2 = $mkReservation($book2, $reserver, $d(-1), $d(14)); + + // Connection A: freeze the read view pre-lock (pre-fix caller shape). + $connA->begin_transaction(); + $freeze = $connA->prepare("SELECT libro_id FROM prestiti WHERE id = ?"); + $freeze->bind_param('i', $zero); + $freeze->execute(); + $freeze->get_result(); + $freeze->close(); + + // Connection B: a competing pendente loan claims the only copy — committed. + $from2 = $d(0); + $to2 = $d(10); + $stmt = $connB->prepare("INSERT INTO prestiti (libro_id, copia_id, utente_id, data_prestito, data_scadenza, stato, origine, attivo) + VALUES (?, ?, ?, ?, ?, 'pendente', 'prenotazione', 0)"); + $stmt->bind_param('iiiss', $book2, $c2, $competitor, $from2, $to2); + $stmt->execute(); + $stmt->close(); + + // Connection A: book lock, then the real promotion. + $lock = $connA->prepare("SELECT id FROM libri WHERE id = ? FOR UPDATE"); + $lock->bind_param('i', $book2); + $lock->execute(); + $lock->get_result(); + $lock->close(); + + $rm2 = new ReservationManager($connA); + $rm2->setExternalTransaction(true); + $promoted2 = $rm2->processBookAvailability($book2); + $connA->commit(); + + check($promoted2 === false, 'race 2: promotion aborts when the copy was just committed to a competitor'); + check($loanCountOnCopy($c2) === 1, 'race 2: exactly ONE open loan holds the copy (no copia_id double-commit)'); + check($reservationState($res2) === 'attiva', "race 2: the unpromoted reservation is released back to 'attiva' (claim reverted)"); + check(count($deferredNotifications($rm2)) === 0, 'race 2: no notification queued for the aborted promotion'); + + /* ===================================================================== + * SOURCE INVARIANTS — lock-first ordering in all 8 transactions + + * ReservationManager guards. + * ===================================================================== */ + $readSource = static function (string $path) use ($root): string { + $source = file_get_contents($root . $path); + if ($source === false) { + throw new \RuntimeException("unreadable source: {$path}"); + } + return $source; + }; + $extractMethod = static function (string $source, string $signature): string { + $start = strpos($source, $signature); + if ($start === false) { + return ''; + } + $remaining = substr($source, $start + strlen($signature)); + if (!preg_match('/\n (?:public|protected|private) (?:const |function )/', $remaining, $m, PREG_OFFSET_CAPTURE)) { + return substr($source, $start); + } + return substr($source, $start, strlen($signature) + $m[0][1]); + }; + + $lockFirst = [ + ['/app/Controllers/LoanApprovalController.php', 'function approveLoan(', 'SELECT libro_id FROM prestiti'], + ['/app/Controllers/LoanApprovalController.php', 'function rejectLoan(', 'SELECT libro_id FROM prestiti'], + ['/app/Controllers/LoanApprovalController.php', 'function returnLoan(', 'SELECT libro_id FROM prestiti'], + ['/app/Controllers/LoanApprovalController.php', 'function cancelPickup(', 'SELECT libro_id FROM prestiti'], + ['/app/Controllers/PrestitiController.php', 'function processReturn(', 'SELECT libro_id FROM prestiti'], + ['/app/Models/LoanRepository.php', 'function close(', 'SELECT libro_id FROM prestiti'], + ['/app/Controllers/UserActionsController.php', 'function cancelLoan(', 'SELECT libro_id'], + ['/app/Controllers/UserActionsController.php', 'function cancelReservation(', 'SELECT libro_id FROM prenotazioni'], + ]; + foreach ($lockFirst as [$file, $sig, $lookupSql]) { + $body = $extractMethod($readSource($file), $sig); + $label = basename($file) . ' ' . trim($sig, '('); + // Match the actual CALL (`->begin_transaction(`), not comments that + // merely mention begin_transaction() while explaining the ordering. + $beginPos = strpos($body, '->begin_transaction('); + $lookupPos = strpos($body, $lookupSql); + $ok = $body !== '' && $beginPos !== false && $lookupPos !== false && $lookupPos < $beginPos; + check($ok, "lock-first: {$label} resolves the book id BEFORE begin_transaction()"); + + // The FIRST prepared statement inside the transaction must be the + // locking read of the book row (read view created post-lock). + $afterBegin = $beginPos !== false ? substr($body, $beginPos) : ''; + $firstSql = ''; + if (preg_match('/prepare\(\s*(["\'])(.*?)\1/s', $afterBegin, $m)) { + $firstSql = $m[2]; + } + $ok2 = str_contains($firstSql, 'FROM libri') && str_contains($firstSql, 'FOR UPDATE'); + check($ok2, "lock-first: {$label} first in-txn statement is `libri ... FOR UPDATE`"); + } + + $rmSource = $readSource('/app/Controllers/ReservationManager.php'); + $pba = $extractMethod($rmSource, 'function processBookAvailability('); + check((bool) preg_match('/FROM prenotazioni r\s+JOIN utenti u ON r\.utente_id = u\.id.*?LIMIT 1\s+FOR UPDATE/s', $pba), 'ReservationManager: queue read is a locking read (FOR UPDATE)'); + check(str_contains($pba, "SET stato = 'completata' WHERE id = ? AND stato = 'attiva'"), "ReservationManager: completata claim is guarded by AND stato = 'attiva'"); + check((bool) preg_match('/\$claimed\s*!==?\s*1|affected_rows/s', $pba) && str_contains($pba, 'affected_rows'), 'ReservationManager: completata claim checks affected_rows'); + // The claim must happen BEFORE the loan is created (match the actual + // call, not comments that mention the method name). + $claimPos = strpos($pba, "SET stato = 'completata' WHERE id = ? AND stato = 'attiva'"); + $createPos = strpos($pba, '$this->createLoanFromReservation('); + check($claimPos !== false && $createPos !== false && $claimPos < $createPos, 'ReservationManager: reservation is claimed before the loan is created'); + + $clfr = $extractMethod($rmSource, 'function createLoanFromReservation('); + check((bool) preg_match('/SELECT 1 FROM prestiti\s+WHERE copia_id = \?.*?LIMIT 1\s+FOR UPDATE/s', $clfr), 'ReservationManager: copy-overlap re-check is a locking read (FOR UPDATE)'); + +} catch (\Throwable $e) { + // Never leave A mid-transaction holding locks. + try { + $connA->rollback(); + } catch (\Throwable $ignored) { + } + $cleanup(); + fwrite(STDERR, 'FAIL: ' . $e->getMessage() . "\n" . $e->getTraceAsString() . "\n"); + exit(1); +} + +$cleanup(); +$connA->close(); +$connB->close(); +echo "\n" . ($failed === 0 ? "ALL {$TESTNO} PASS\n" : "{$failed}/{$TESTNO} FAILED\n"); +exit($failed > 0 ? 1 : 0); From bbab2a63edc111c07a59b2b781d65be183950271 Mon Sep 17 00:00:00 2001 From: fabiodalez-dev Date: Thu, 20 Aug 2026 11:28:16 +0200 Subject: [PATCH 13/16] fix(loans): harden the full #366 reschedule/overdue scenario end-to-end MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The committed single-step guard stopped promoting a book while a copy was out. Walking the exact issue-366 sequence (reserve after loan -> overdue -> reschedule + prolong -> overdue again -> not prolonged) exposed four residual gaps, now fixed: - PrestitiController::update() (P1): rescheduling an open prenotato/da_ritirare loan left stato/pickup_deadline stale, so checkExpiredPickups() culled a just-rescheduled valid loan (wrong 'pickup expired' email + lost loan) and shrinking data_scadenza below pickup_deadline made an unexpirable hold. Now demote to prenotato + clear the deadline when the new start is in the future, else re-derive the da_ritirare deadline from today and cap it at data_scadenza. - MaintenanceService::runAll() + CapacityService (P2): updateOverdueLoans() now runs FIRST, and holdingLoanIntervals() treats a date-overdue in_corso loan (data_scadenza < today) like in_ritardo — clamped open-ended — so an unflipped overdue loan no longer 'frees' capacity between cron runs. Single OR branch per row, no double-clamp. - PrestitiController::renew() (P3): the overdue gate is date-based too (in_corso AND data_scadenza < today), preventing a renewal from a stale past due date and the flag reset that re-armed a duplicate overdue email. - PrestitiController::renew() (P3): the capacity window starts at due+1, matching the #336 fix in bulkExtend/update(). New tests/issue-366-full-scenario.unit.php drives the real production paths over all six steps (1- and multi-copy); 34 assertions, 11 fail pre-fix. --- app/Controllers/PrestitiController.php | 64 +++- app/Services/CapacityService.php | 14 +- app/Support/MaintenanceService.php | 30 +- tests/issue-366-full-scenario.unit.php | 455 +++++++++++++++++++++++++ 4 files changed, 543 insertions(+), 20 deletions(-) create mode 100644 tests/issue-366-full-scenario.unit.php diff --git a/app/Controllers/PrestitiController.php b/app/Controllers/PrestitiController.php index 0d320140c..ddb8f31cc 100644 --- a/app/Controllers/PrestitiController.php +++ b/app/Controllers/PrestitiController.php @@ -824,7 +824,7 @@ public function update(Request $request, Response $response, mysqli $db, int $id // iniziale e questo lock le può aver cambiate, e la finestra "vecchia" // del check di capacità qui sotto deve basarsi sui valori realmente // salvati, non su quelli pre-transazione (CodeRabbit, PR #337). - $lockLoan = $db->prepare('SELECT attivo, data_restituzione, libro_id, copia_id, utente_id, data_prestito, data_scadenza FROM prestiti WHERE id=? FOR UPDATE'); + $lockLoan = $db->prepare('SELECT attivo, data_restituzione, libro_id, copia_id, utente_id, stato, data_prestito, data_scadenza FROM prestiti WHERE id=? FOR UPDATE'); $lockLoan->bind_param('i', $id); $lockLoan->execute(); $locked = $lockLoan->get_result()->fetch_assoc(); @@ -1045,6 +1045,47 @@ public function update(Request $request, Response $response, mysqli $db, int $id $resetRecall->close(); } + // #366 residual: rescheduling an open 'prenotato'/'da_ritirare' loan + // must keep the pickup lifecycle coherent with the NEW window — + // update() moved the dates but left stato/pickup_deadline untouched, + // so checkExpiredPickups() culled a just-rescheduled valid loan + // against its STALE deadline (wrong "pickup expired" email + lost + // loan), and shrinking data_scadenza below pickup_deadline left an + // unexpirable hold pinning the copy past its own loan window. + // Mirror approveLoan()/activateScheduledLoans(): + // - new start in the future -> back to 'prenotato', no deadline; + // - already 'da_ritirare' -> re-derive the deadline from today, + // capped at the new data_scadenza (L1); + // - 'prenotato' whose start has arrived is deliberately NOT + // promoted here: the maintenance sweep owns that transition, + // with the #366 copy-free guard and the pickup-ready email. + if (in_array((string) $locked['stato'], ['prenotato', 'da_ritirare'], true) + && ($newPrestito !== $oldPrestito || $newScadenza !== $oldScadenza)) { + $todayApp = \App\Support\DateHelper::today(); + if ($newPrestito > $todayApp) { + $pickupRecalc = $db->prepare( + "UPDATE prestiti SET stato = 'prenotato', pickup_deadline = NULL + WHERE id = ? AND attivo = 1 AND stato IN ('prenotato', 'da_ritirare')" + ); + $pickupRecalc->bind_param('i', $id); + $pickupRecalc->execute(); + $pickupRecalc->close(); + } elseif ((string) $locked['stato'] === 'da_ritirare') { + $pickupDays = (int) ((new \App\Models\SettingsRepository($db))->get('loans', 'pickup_expiry_days', '3') ?? 3); + $pickupDeadline = (new \DateTimeImmutable($todayApp))->modify('+' . $pickupDays . ' days')->format('Y-m-d'); + if ($pickupDeadline > $newScadenza) { + $pickupDeadline = $newScadenza; + } + $pickupRecalc = $db->prepare( + "UPDATE prestiti SET pickup_deadline = ? + WHERE id = ? AND attivo = 1 AND stato = 'da_ritirare'" + ); + $pickupRecalc->bind_param('si', $pickupDeadline, $id); + $pickupRecalc->execute(); + $pickupRecalc->close(); + } + } + // Ricalcola la disponibilità (M6c): spostare le date di un 'prenotato' // attraverso oggi cambia l'occupazione corrente e lascerebbe // copie_disponibili/libri.stato stantii fino al prossimo evento. @@ -1786,8 +1827,15 @@ public function renew(Request $request, Response $response, mysqli $db, int $id) return $response->withHeader('Location', url($errorUrl . $separator . 'error=loan_not_active'))->withStatus(302); } - // Check if loan is overdue - $isLate = ($loan['stato'] === 'in_ritardo'); + // Check if loan is overdue — by STATE or by DATE (#366 residual): a loan + // whose data_scadenza is already past but that the maintenance sweep has + // not yet flipped to 'in_ritardo' is just as overdue. Renewing it would + // compute the new due date from the stale past date AND reset + // warning_sent/overdue_notification_sent, re-arming a duplicate overdue + // email. Same refusal, same error code. + $todayApp = \App\Support\DateHelper::today(); + $isLate = ($loan['stato'] === 'in_ritardo') + || ($loan['stato'] === 'in_corso' && (string) $loan['data_scadenza'] < $todayApp); if ($isLate) { $errorUrl = $redirectTo ?? url('/admin/loans'); $separator = strpos($errorUrl, '?') === false ? '?' : '&'; @@ -1875,7 +1923,9 @@ public function renew(Request $request, Response $response, mysqli $db, int $id) $separator = strpos($errorUrl, '?') === false ? '?' : '&'; return $response->withHeader('Location', url($errorUrl . $separator . 'error=loan_not_active'))->withStatus(302); } - if ($lockedLoan['stato'] === 'in_ritardo') { + // State- OR date-based overdue, same predicate as the pre-lock check. + if ($lockedLoan['stato'] === 'in_ritardo' + || ($lockedLoan['stato'] === 'in_corso' && (string) $lockedLoan['data_scadenza'] < $todayApp)) { $db->rollback(); $errorUrl = $redirectTo ?? url('/admin/loans'); $separator = strpos($errorUrl, '?') === false ? '?' : '&'; @@ -1906,7 +1956,11 @@ public function renew(Request $request, Response $response, mysqli $db, int $id) // Extension is allowed if: // 1. No other reservations/loans overlap with the extension period, OR // 2. Another copy is available for those overlapping reservations - $extensionStart = $currentDueDate; // Extension starts from current due date + // #336 (same fix as bulkExtend/update): the claimed window starts the + // day AFTER the current due date — the due date itself is already + // held by this loan, so starting at $currentDueDate double-counted + // that day and bounced renewals over commitments that only touch it. + $extensionStart = date('Y-m-d', strtotime($currentDueDate . ' +1 day')); $extensionEnd = $proposedNewDueDate; $capacity = new \App\Services\CapacityService($db); diff --git a/app/Services/CapacityService.php b/app/Services/CapacityService.php index 5974b9270..6e8bfb2c2 100644 --- a/app/Services/CapacityService.php +++ b/app/Services/CapacityService.php @@ -201,19 +201,25 @@ private function holdingLoanIntervals(int $libroId, string $start, string $end, // date is in the past, but the physical copy remains out of the library: // clamp it to the requested window end instead of freeing capacity after // data_scadenza. This mirrors the DB trigger and the public calendar. + // #366 residual: a loan overdue BY DATE but not yet flipped by the + // maintenance sweep ('in_corso' with data_scadenza < today) is the very + // same unreturned copy — treat it exactly like 'in_ritardo' (one branch, + // one clamp, never both on the same row: the OR selects a single case). + $today = \App\Support\DateHelper::today(); + $openEnded = "(p.attivo = 1 AND (p.stato = 'in_ritardo' OR (p.stato = 'in_corso' AND p.data_scadenza < ?)))"; $sql = "SELECT GREATEST(p.data_prestito, ?) AS s, LEAST(CASE - WHEN p.attivo = 1 AND p.stato = 'in_ritardo' THEN ? + WHEN {$openEnded} THEN ? ELSE p.data_scadenza END, ?) AS e FROM prestiti p WHERE p.libro_id = ? AND p.data_prestito <= ? - AND (p.stato = 'in_ritardo' OR p.data_scadenza >= ?) + AND ({$openEnded} OR p.data_scadenza >= ?) AND ( (p.attivo = 1 AND p.stato IN ('prenotato','da_ritirare','in_corso','in_ritardo')) OR (p.attivo = 0 AND p.stato = 'pendente' AND p.copia_id IS NOT NULL) )"; - $types = 'sssiss'; - $params = [$start, $end, $end, $libroId, $end, $start]; + $types = 'ssssisss'; + $params = [$start, $today, $end, $end, $libroId, $end, $today, $start]; if ($excludePrestitoId !== null) { $sql .= ' AND p.id <> ?'; $types .= 'i'; diff --git a/app/Support/MaintenanceService.php b/app/Support/MaintenanceService.php index 1b6b94242..f7833ee06 100644 --- a/app/Support/MaintenanceService.php +++ b/app/Support/MaintenanceService.php @@ -131,7 +131,21 @@ public function runAll(): array $results['errors'][] = 'contributorBackfill: ' . $e->getMessage(); } - // Expire FIRST (BUG8/D13 ordering): cull dead-period reservations and + // Overdue flip FIRST (#366 residual): flip date-overdue 'in_corso' loans + // to 'in_ritardo' BEFORE expiries/activations/promotions, so every gate + // in this same pass that special-cases 'in_ritardo' (capacity clamps, + // overlap predicates, renew()'s state check) sees the truthful state. + // With the old order the flip ran LAST: on the day a reservation's start + // arrived its unreturned predecessor still sat in 'in_corso' with a past + // due date, defeating those gates for one whole pass. + try { + $results['overdue_loans_updated'] = $this->updateOverdueLoans(); + } catch (\Throwable $e) { + $results['errors'][] = 'updateOverdueLoans: ' . $e->getMessage(); + SecureLogger::error(__('MaintenanceService errore prestiti in ritardo'), ['error' => $e->getMessage()]); + } + + // Expire next (BUG8/D13 ordering): cull dead-period reservations and // unclaimed pickups before activating scheduled loans, so a reservation // whose window has already passed is never promoted to 'da_ritirare'. try { @@ -170,13 +184,6 @@ public function runAll(): array SecureLogger::error(__('MaintenanceService errore conversione prenotazioni'), ['error' => $e->getMessage()]); } - try { - $results['overdue_loans_updated'] = $this->updateOverdueLoans(); - } catch (\Throwable $e) { - $results['errors'][] = 'updateOverdueLoans: ' . $e->getMessage(); - SecureLogger::error(__('MaintenanceService errore prestiti in ritardo'), ['error' => $e->getMessage()]); - } - // Run automatic notifications try { $notificationResults = $this->runNotifications(); @@ -392,9 +399,10 @@ public function activateScheduledLoans(): int // RIGHT NOW. The date window alone is not enough: the preceding // loan may still be out — overdue included. 'in_corso'/'in_ritardo' // rows are counted with NO date predicate because an unreturned - // copy is out regardless of its contractual dates (and - // updateOverdueLoans runs AFTER this sweep, so an overdue loan can - // still sit in 'in_corso' here). Sibling 'da_ritirare' pickups and + // copy is out regardless of its contractual dates (runAll() now + // flips overdue loans BEFORE this sweep, but a standalone call or + // a mid-pass write can still leave one in 'in_corso' here — keep + // the state-agnostic count). Sibling 'da_ritirare' pickups and // copy-holding 'pendente' rows each pin a copy on the shelf too. // Future 'prenotato' rows are NOT counted: they hold capacity for // a later window, not a copy today. If nothing is free the diff --git a/tests/issue-366-full-scenario.unit.php b/tests/issue-366-full-scenario.unit.php new file mode 100644 index 000000000..fb00e5877 --- /dev/null +++ b/tests/issue-366-full-scenario.unit.php @@ -0,0 +1,455 @@ +set_charset('utf8mb4'); +} catch (\Throwable $e) { + fwrite(STDERR, "FAIL: database unreachable — mandatory for this test: {$e->getMessage()}\n"); + exit(1); +} + +$TESTNO = 0; +$failed = 0; +function check(bool $cond, string $desc): void +{ + global $TESTNO, $failed; + $TESTNO++; + printf("[%02d] %s: %s\n", $TESTNO, $cond ? 'PASS' : 'FAIL', $desc); + if (!$cond) { + $failed++; + } +} + +// Per-run token: cleanup and assertions only ever touch this run's rows. +$RUN = bin2hex(random_bytes(6)); +$TITLE_PREFIX = "ZZ_366FS_{$RUN}_"; +$EMAIL_DOMAIN = '@366fs.test.local'; + +$today = DateHelper::today(); +$d = static fn (int $offsetDays): string => date('Y-m-d', strtotime($today . ($offsetDays >= 0 ? " +{$offsetDays} days" : ' ' . $offsetDays . ' days'))); + +$settingsRow = static function (string $key, string $default) use ($db): string { + $stmt = $db->prepare("SELECT setting_value FROM system_settings WHERE category = 'loans' AND setting_key = ?"); + $stmt->bind_param('s', $key); + $stmt->execute(); + $row = $stmt->get_result()->fetch_assoc(); + $stmt->close(); + return $row !== null ? (string) $row['setting_value'] : $default; +}; +$pickupDays = max(0, (int) $settingsRow('pickup_expiry_days', '3')); +$loanDays = (int) $settingsRow('loan_duration_days', '30'); +if ($loanDays < 1) { + $loanDays = 30; +} + +/* -------------------------------- helpers -------------------------------- */ + +$userSeq = 0; +$mkUser = static function () use ($db, $RUN, $EMAIL_DOMAIN, &$userSeq): int { + $userSeq++; + $tessera = 'Z366F' . strtoupper(substr($RUN, 0, 8)) . $userSeq; + $email = "u{$userSeq}-{$RUN}{$EMAIL_DOMAIN}"; + $cognome = "ZZ366FS {$userSeq}"; + $stmt = $db->prepare("INSERT INTO utenti (codice_tessera, nome, cognome, email, password, stato, tipo_utente, email_verificata) + VALUES (?, 'Test', ?, ?, 'x', 'attivo', 'standard', 1)"); + $stmt->bind_param('sss', $tessera, $cognome, $email); + $stmt->execute(); + $stmt->close(); + return (int) $db->insert_id; +}; + +$bookSeq = 0; +$mkBook = static function (int $copies) use ($db, $TITLE_PREFIX, &$bookSeq): array { + $bookSeq++; + $title = $TITLE_PREFIX . $bookSeq; + $stmt = $db->prepare("INSERT INTO libri (titolo, copie_totali, copie_disponibili, created_at, updated_at) + VALUES (?, ?, ?, NOW(), NOW())"); + $stmt->bind_param('sii', $title, $copies, $copies); + $stmt->execute(); + $stmt->close(); + $bookId = (int) $db->insert_id; + $copyIds = []; + for ($i = 1; $i <= $copies; $i++) { + $code = "ZZ366FS-{$bookId}-C{$i}"; + $stmt = $db->prepare("INSERT INTO copie (libro_id, numero_inventario, stato) VALUES (?, ?, 'disponibile')"); + $stmt->bind_param('is', $bookId, $code); + $stmt->execute(); + $stmt->close(); + $copyIds[] = (int) $db->insert_id; + } + return [$bookId, $copyIds]; +}; + +$mkLoan = static function (int $bookId, ?int $copiaId, int $userId, string $stato, string $from, string $to, ?string $pickupDeadline = null, int $warned = 0) use ($db): int { + $stmt = $db->prepare("INSERT INTO prestiti (libro_id, copia_id, utente_id, data_prestito, data_scadenza, stato, origine, attivo, pickup_deadline, warning_sent, overdue_notification_sent) + VALUES (?, ?, ?, ?, ?, ?, 'diretto', 1, ?, ?, ?)"); + $stmt->bind_param('iiissssii', $bookId, $copiaId, $userId, $from, $to, $stato, $pickupDeadline, $warned, $warned); + $stmt->execute(); + $stmt->close(); + return (int) $db->insert_id; +}; + +$setCopyState = static function (int $copiaId, string $stato) use ($db): void { + $stmt = $db->prepare('UPDATE copie SET stato = ? WHERE id = ?'); + $stmt->bind_param('si', $stato, $copiaId); + $stmt->execute(); + $stmt->close(); +}; + +$loanRow = static function (int $loanId) use ($db): array { + $stmt = $db->prepare('SELECT stato, attivo, data_prestito, data_scadenza, pickup_deadline, warning_sent, overdue_notification_sent, renewals FROM prestiti WHERE id = ?'); + $stmt->bind_param('i', $loanId); + $stmt->execute(); + $row = $stmt->get_result()->fetch_assoc() ?: []; + $stmt->close(); + return $row; +}; + +// Fixture-only writes to rows this run created (simulating elapsed days). +$shiftLoan = static function (int $loanId, ?string $from, ?string $to) use ($db): void { + if ($from !== null) { + $stmt = $db->prepare('UPDATE prestiti SET data_prestito = ? WHERE id = ?'); + $stmt->bind_param('si', $from, $loanId); + $stmt->execute(); + $stmt->close(); + } + if ($to !== null) { + $stmt = $db->prepare('UPDATE prestiti SET data_scadenza = ? WHERE id = ?'); + $stmt->bind_param('si', $to, $loanId); + $stmt->execute(); + $stmt->close(); + } +}; + +$returnLoan = static function (int $loanId, ?int $copiaId, string $copyState = 'disponibile') use ($db, $setCopyState, $today): void { + $stmt = $db->prepare("UPDATE prestiti SET stato = 'restituito', attivo = 0, data_restituzione = ? WHERE id = ?"); + $stmt->bind_param('si', $today, $loanId); + $stmt->execute(); + $stmt->close(); + if ($copiaId !== null) { + $setCopyState($copiaId, $copyState); + } +}; + +// Real controller invocations (same harness as loan-bulk-extension-capacity). +$callUpdate = static function (int $loanId, int $userId, string $start, string $due) use ($db) { + // The controller authorizes from the session; processed_by must reference a + // real user because of the FK, so reuse the fixture borrower. + $_SESSION['user'] = ['tipo_utente' => 'admin', 'id' => $userId]; + $request = (new ServerRequestFactory()) + ->createServerRequest('POST', '/admin/loans/edit/' . $loanId) + ->withParsedBody(['utente_id' => $userId, 'data_prestito' => $start, 'data_scadenza' => $due]); + $response = (new ResponseFactory())->createResponse(); + return (new PrestitiController())->update($request, $response, $db, $loanId); +}; + +$callRenew = static function (int $loanId, int $userId) use ($db) { + $_SESSION['user'] = ['tipo_utente' => 'admin', 'id' => $userId]; + $request = (new ServerRequestFactory()) + ->createServerRequest('POST', '/admin/loans/renew/' . $loanId) + ->withParsedBody([]); + $response = (new ResponseFactory())->createResponse(); + return (new PrestitiController())->renew($request, $response, $db, $loanId); +}; + +$svc = new MaintenanceService($db); +// One maintenance pass, in runAll()'s (fixed) order, restricted to the loan +// lifecycle tasks — the full runAll() also fires notifications/ICS/hooks over +// the whole shared DB, which this test deliberately avoids. +$maintenancePass = static function () use ($svc): void { + $svc->updateOverdueLoans(); + $svc->checkExpiredReservations(); + $svc->checkExpiredPickups(); + $svc->activateScheduledLoans(); +}; + +/* -------------------------------- cleanup -------------------------------- */ +$cleanup = static function () use ($db, $TITLE_PREFIX, $RUN, $EMAIL_DOMAIN): void { + $like = $db->real_escape_string($TITLE_PREFIX) . '%'; + $db->query("DELETE r FROM prenotazioni r JOIN libri l ON l.id = r.libro_id WHERE l.titolo LIKE '{$like}'"); + $db->query("DELETE p FROM prestiti p JOIN libri l ON l.id = p.libro_id WHERE l.titolo LIKE '{$like}'"); + $db->query("DELETE c FROM copie c JOIN libri l ON l.id = c.libro_id WHERE l.titolo LIKE '{$like}'"); + $db->query("DELETE FROM libri WHERE titolo LIKE '{$like}'"); + $emailLike = $db->real_escape_string("u%-{$RUN}{$EMAIL_DOMAIN}"); + $db->query("DELETE FROM utenti WHERE email LIKE '{$emailLike}'"); +}; +$cleanup(); + +try { + /* ---- 0. runAll() ordering: the overdue flip must run FIRST ----------- */ + $maintenanceSrc = (string) file_get_contents($root . '/app/Support/MaintenanceService.php'); + $runAllStart = (int) strpos($maintenanceSrc, 'public function runAll'); + $body = substr($maintenanceSrc, $runAllStart); + $posOverdue = strpos($body, '$this->updateOverdueLoans()'); + $posExpired = strpos($body, '$this->checkExpiredReservations()'); + $posActivate = strpos($body, '$this->activateScheduledLoans()'); + check( + $posOverdue !== false && $posExpired !== false && $posActivate !== false + && $posOverdue < $posExpired && $posOverdue < $posActivate, + 'runAll() flips overdue loans BEFORE expiries and scheduled-loan activation' + ); + + /* ---- A. The exact #366 six-step sequence (single copy) --------------- */ + $borrower = $mkUser(); + $reserver = $mkUser(); + [$bookA, [$copyA]] = $mkBook(1); + + // Step 1-2: loan out since d-30, due d-5, NOT returned (overdue, not yet + // flipped — the state a real pass starts from); reservation right after it, + // start already arrived (d-4 .. d+10), no copy pinned. + $loanA = $mkLoan($bookA, $copyA, $borrower, 'in_corso', $d(-30), $d(-5)); + $setCopyState($copyA, 'prestato'); + $resA = $mkLoan($bookA, null, $reserver, 'prenotato', $d(-4), $d(10)); + + // Maintenance pass #1. + $maintenancePass(); + check(($loanRow($loanA)['stato'] ?? '') === 'in_ritardo', 'A: pass 1 flips the unreturned predecessor to in_ritardo (order fix)'); + $row = $loanRow($resA); + check(($row['stato'] ?? '') === 'prenotato' && ($row['pickup_deadline'] ?? null) === null, + 'A: reservation NOT announced ready while the copy is out (no promotion => no pickup email)'); + + // Step 3a: staff change the reservation (move it later, inside its window). + $resp = $callUpdate($resA, $reserver, $d(6), $d(10)); + check(!str_contains($resp->getHeaderLine('Location'), 'error='), 'A: staff reschedule of the queued reservation succeeds'); + $row = $loanRow($resA); + check(($row['stato'] ?? '') === 'prenotato' && ($row['pickup_deadline'] ?? null) === null, + 'A: rescheduled reservation stays prenotato with no pickup_deadline'); + + // Step 3b: renew() refuses the overdue predecessor; staff prolong via the + // edit form instead (the sanctioned path, which recomputes stato — #281). + $resp = $callRenew($loanA, $borrower); + check(str_contains($resp->getHeaderLine('Location'), 'error=loan_overdue'), 'A: renew() refuses the in_ritardo predecessor'); + $resp = $callUpdate($loanA, $borrower, $d(-30), $d(5)); + check(!str_contains($resp->getHeaderLine('Location'), 'error='), 'A: staff prolong the overdue loan via edit'); + $row = $loanRow($loanA); + check(($row['stato'] ?? '') === 'in_corso' && ($row['data_scadenza'] ?? '') === $d(5), 'A: prolonged loan is back in_corso with the new due date'); + + // Step 4: time passes — the patron overdues AGAIN (due date now past, stato + // still in_corso: the daily flip has not run yet) and the reservation's + // start date arrives. Fixture-only date shifts on this run's own rows. + $shiftLoan($loanA, null, $d(-2)); + $shiftLoan($resA, $d(-1), $d(10)); + + // Step 5-6: staff do NOT prolong. The next maintenance pass must NOT + // announce the book ready for pickup, and must NOT destroy the reservation. + $maintenancePass(); + check(($loanRow($loanA)['stato'] ?? '') === 'in_ritardo', 'A: pass 2 flips the re-overdue predecessor first'); + $row = $loanRow($resA); + check(($row['stato'] ?? '') === 'prenotato' && ($row['pickup_deadline'] ?? null) === null, + 'A: CORE #366 — reservation still NOT ready-for-pickup while the loan is out (no pickup email)'); + check((int) ($row['attivo'] ?? 0) === 1 && ($row['stato'] ?? '') !== 'scaduto', 'A: reservation not culled by the expiry crons'); + + // Step 7: the predecessor finally comes back — the reservation DOES promote. + $returnLoan($loanA, $copyA); + $maintenancePass(); + $row = $loanRow($resA); + check(($row['stato'] ?? '') === 'da_ritirare', 'A: after the return the reservation promotes to da_ritirare'); + check(!empty($row['pickup_deadline']) && $row['pickup_deadline'] <= $row['data_scadenza'], + 'A: promoted reservation has a pickup_deadline capped at its own due date'); + + /* ---- B. Reschedule of a da_ritirare loan vs the expiry cron ---------- */ + // B1: pickup window already blown (deadline yesterday); staff reschedule the + // loan to start in the future BEFORE the cron culls it. The edit must demote + // it to 'prenotato' and clear the stale deadline, so checkExpiredPickups() + // does not destroy a just-rescheduled valid loan (wrong "pickup expired" + // email — that mail only fires after a successful cull commit). + $userB = $mkUser(); + [$bookB, [$copyB]] = $mkBook(1); + $loanB = $mkLoan($bookB, $copyB, $userB, 'da_ritirare', $d(-6), $d(15), $d(-1)); + $setCopyState($copyB, 'prenotato'); + + $resp = $callUpdate($loanB, $userB, $d(4), $d(15)); + check(!str_contains($resp->getHeaderLine('Location'), 'error='), 'B1: rescheduling the da_ritirare loan to a future start succeeds'); + $row = $loanRow($loanB); + check(($row['stato'] ?? '') === 'prenotato', 'B1: future start demotes da_ritirare back to prenotato'); + check(($row['pickup_deadline'] ?? null) === null, 'B1: the stale pickup_deadline is cleared on demotion'); + + $svc->checkExpiredPickups(); + $svc->checkExpiredReservations(); + $row = $loanRow($loanB); + check(($row['stato'] ?? '') === 'prenotato' && (int) ($row['attivo'] ?? 0) === 1, + 'B1: expiry crons do NOT cull the rescheduled loan (no wrong pickup-expired email)'); + + // B2: staff shrink the due date below the current pickup_deadline. The + // deadline must be re-derived and capped at the new data_scadenza — + // otherwise the hold outlives its own loan window and nothing ever expires it. + $userB2 = $mkUser(); + [$bookB2, [$copyB2]] = $mkBook(1); + $loanB2 = $mkLoan($bookB2, $copyB2, $userB2, 'da_ritirare', $d(-2), $d(10), $d(3)); + $setCopyState($copyB2, 'prenotato'); + + $resp = $callUpdate($loanB2, $userB2, $d(-2), $d(1)); + check(!str_contains($resp->getHeaderLine('Location'), 'error='), 'B2: shrinking the due date of a da_ritirare loan succeeds'); + $row = $loanRow($loanB2); + $expectedDeadline = min($d($pickupDays), $d(1)); + check(($row['stato'] ?? '') === 'da_ritirare', 'B2: started window keeps the loan da_ritirare'); + check(($row['pickup_deadline'] ?? '') === $expectedDeadline && ($row['pickup_deadline'] ?? '') <= ($row['data_scadenza'] ?? ''), + 'B2: pickup_deadline re-derived and capped at the shrunk due date (no zombie hold)'); + + /* ---- C. renew(): date-based overdue gate + #336 window --------------- */ + // C1: overdue BY DATE but still 'in_corso' (flip not yet run). renew() must + // refuse — renewing from the stale past due date and resetting the + // notification flags would re-arm a duplicate overdue email. + $userC = $mkUser(); + [$bookC, [$copyC]] = $mkBook(1); + $loanC = $mkLoan($bookC, $copyC, $userC, 'in_corso', $d(-30), $d(-5), null, 1); + $setCopyState($copyC, 'prestato'); + + $resp = $callRenew($loanC, $userC); + check(str_contains($resp->getHeaderLine('Location'), 'error=loan_overdue'), 'C1: renew() refuses a loan overdue by date (stato still in_corso)'); + $row = $loanRow($loanC); + check(($row['data_scadenza'] ?? '') === $d(-5) && (int) ($row['renewals'] ?? -1) === 0, 'C1: refused renewal leaves the due date and renewal count unchanged'); + check((int) ($row['overdue_notification_sent'] ?? 0) === 1 && (int) ($row['warning_sent'] ?? 0) === 1, + 'C1: notification flags NOT reset (no duplicate overdue email re-armed)'); + + // C2: due TODAY, another user's reservation occupies ONLY the due day. The + // due day is already held by this loan (#336): the claimed window starts the + // day after, so the renewal must succeed. + $userC2 = $mkUser(); + $userC2b = $mkUser(); + [$bookC2, [$copyC2]] = $mkBook(1); + $loanC2 = $mkLoan($bookC2, $copyC2, $userC2, 'in_corso', $d(-10), $d(0)); + $setCopyState($copyC2, 'prestato'); + $stmt = $db->prepare("INSERT INTO prenotazioni (libro_id, utente_id, data_inizio_richiesta, data_fine_richiesta, data_scadenza_prenotazione, stato, queue_position) + VALUES (?, ?, ?, ?, ?, 'attiva', 1)"); + $sameDay = $d(0); + $stmt->bind_param('iisss', $bookC2, $userC2b, $sameDay, $sameDay, $sameDay); + $stmt->execute(); + $stmt->close(); + + $resp = $callRenew($loanC2, $userC2); + check(str_contains($resp->getHeaderLine('Location'), 'renewed=1'), + 'C2: renewal succeeds when the only overlapping commitment sits on the already-held due date (#336 window)'); + $row = $loanRow($loanC2); + check(($row['data_scadenza'] ?? '') === $d($loanDays), 'C2: renewed due date = current due + configured loan duration'); + + /* ---- D. Multi-copy variants ------------------------------------------ */ + // D1: 2 copies, one out overdue-not-flipped, reservation pinned to the free + // copy: the pass promotes it (multi-copy behaviour preserved). + $userD = $mkUser(); + $userD2 = $mkUser(); + [$bookD, [$copyDa, $copyDb]] = $mkBook(2); + $loanD = $mkLoan($bookD, $copyDa, $userD, 'in_corso', $d(-30), $d(-5)); + $setCopyState($copyDa, 'prestato'); + $resD = $mkLoan($bookD, $copyDb, $userD2, 'prenotato', $d(-1), $d(20)); + $setCopyState($copyDb, 'prenotato'); + + $maintenancePass(); + check(($loanRow($loanD)['stato'] ?? '') === 'in_ritardo', 'D1: overdue sibling flipped first'); + check(($loanRow($resD)['stato'] ?? '') === 'da_ritirare', 'D1: reservation on the genuinely free copy IS promoted'); + + // D2: 2 copies but the reservation is pinned to the copy still out. It must + // stay prenotato; a staff reschedule (future start) keeps it demoted; once + // the pinned copy returns and the window arrives, it promotes. + $userE = $mkUser(); + $userE2 = $mkUser(); + [$bookE, [$copyEa, $copyEb]] = $mkBook(2); + $loanE = $mkLoan($bookE, $copyEa, $userE, 'in_corso', $d(-30), $d(-5)); + $setCopyState($copyEa, 'prestato'); + $resE = $mkLoan($bookE, $copyEa, $userE2, 'prenotato', $d(-1), $d(20)); + + $svc->activateScheduledLoans(); + check(($loanRow($resE)['stato'] ?? '') === 'prenotato', 'D2: reservation pinned to the out copy stays prenotato despite book-level room'); + + $resp = $callUpdate($resE, $userE2, $d(2), $d(20)); + check(!str_contains($resp->getHeaderLine('Location'), 'error='), 'D2: rescheduling the pinned reservation succeeds'); + $row = $loanRow($resE); + check(($row['stato'] ?? '') === 'prenotato' && ($row['pickup_deadline'] ?? null) === null, 'D2: rescheduled pinned reservation stays prenotato, no deadline'); + + $returnLoan($loanE, $copyEa, 'prenotato'); // copy back on the shelf, held for the reservation + $shiftLoan($resE, $d(0), null); // time passes: its start arrives + $svc->activateScheduledLoans(); + $row = $loanRow($resE); + check(($row['stato'] ?? '') === 'da_ritirare' && !empty($row['pickup_deadline']), 'D2: pinned copy returned + window arrived => promotion'); + + /* ---- E. CapacityService: date-overdue in_corso occupies open-ended --- */ + $userF = $mkUser(); + [$bookF, [$copyF]] = $mkBook(1); + $loanF = $mkLoan($bookF, $copyF, $userF, 'in_corso', $d(-30), $d(-5)); + $setCopyState($copyF, 'prestato'); + + $capacity = new CapacityService($db); + check(!$capacity->hasFreeCapacity($bookF, $today, $d(7)), + 'E: a date-overdue in_corso loan occupies the future window (unreturned copy, open-ended clamp)'); + $db->query("UPDATE prestiti SET stato = 'in_ritardo' WHERE id = {$loanF}"); + check(!$capacity->hasFreeCapacity($bookF, $today, $d(7)), + 'E: identical verdict once flipped to in_ritardo (no drift between the two states)'); + $returnLoan($loanF, $copyF); + check($capacity->hasFreeCapacity($bookF, $today, $d(7)), + 'E: capacity frees once the copy is actually returned'); + +} catch (\Throwable $e) { + $cleanup(); + fwrite(STDERR, 'FAIL: ' . $e->getMessage() . "\n" . $e->getTraceAsString() . "\n"); + $db->close(); + exit(1); +} + +$cleanup(); +$db->close(); +echo "\n" . ($failed === 0 ? "ALL {$TESTNO} PASS\n" : "{$failed}/{$TESTNO} FAILED\n"); +exit($failed > 0 ? 1 : 0); From 36da9801deeac163ed91bf426a61a40fa0f219c6 Mon Sep 17 00:00:00 2001 From: fabiodalez-dev Date: Thu, 20 Aug 2026 11:53:30 +0200 Subject: [PATCH 14/16] fix(loans): close the remaining reservation/loan edge cases from the audit Seven verified findings from the circulation-system audit: - confirmPickup accepted a copy in state 'prestato' (still out), so a successor pickup on a copy whose overdue predecessor was not yet flipped committed a second active loan on one physical copy. Reject 'prestato' like the other non-lendable states. - Admin cancelReservation freed capacity but never promoted the queue (every sibling release path does). Added the setExternalTransaction + bounded processBookAvailability loop + post-commit flushDeferredNotifications. - sendLoanExpirationWarnings / sendOverdueLoanNotifications filtered deleted_at IS NULL, so archiving a book with an active overdue loan silenced its overdue notice AND therefore its automatic recalls (which require the overdue flag). Dropped the filter (chase-up mail, same rationale as recalls), marked CI-SOFT-DELETE-EXEMPT. - reservation_book_available could be sent twice (deferred flush racing the atomic retry sweep). sendReservationNotification now claims notifica_inviata=1 WHERE id=? AND notifica_inviata=0 before sending and reverts on failure; the sweep's external claim was removed to avoid a double-claim no-send loop. - store() did not cap pickup_deadline at data_scadenza (approveLoan and activateScheduledLoans do); bulkExtend and update()'s date extension skipped LoanEligibility::checkUser (renew already ran it) so a suspended borrower kept a book via bulk/reschedule. - Expiry audit notes used the process-TZ date instead of the app-TZ decision date (off-by-a-day near midnight). New tests/audit-fixes-p2-p4.unit.php (33 assertions, 20 fail pre-fix). --- app/Controllers/LoanApprovalController.php | 25 +- app/Controllers/PrestitiController.php | 29 +- app/Controllers/ReservationManager.php | 76 ++-- app/Support/MaintenanceService.php | 14 +- app/Support/NotificationService.php | 6 +- tests/audit-fixes-p2-p4.unit.php | 468 +++++++++++++++++++++ 6 files changed, 580 insertions(+), 38 deletions(-) create mode 100644 tests/audit-fixes-p2-p4.unit.php diff --git a/app/Controllers/LoanApprovalController.php b/app/Controllers/LoanApprovalController.php index e3f831384..0479fe004 100644 --- a/app/Controllers/LoanApprovalController.php +++ b/app/Controllers/LoanApprovalController.php @@ -915,7 +915,11 @@ public function confirmPickup(Request $request, Response $response, mysqli $db): $copyResult = $copyCheckStmt->get_result()->fetch_assoc(); $copyCheckStmt->close(); - $invalidStates = ['perso', 'danneggiato', 'manutenzione', 'in_restauro', 'in_trasferimento']; + // 'prestato' incluso (P2): una copia ancora 'prestato' è fuori con un + // ALTRO prestito aperto (es. predecessore in ritardo non ancora + // rientrato) — confermare il ritiro creerebbe due prestiti attivi + // sulla stessa copia fisica (double-issue). + $invalidStates = ['perso', 'danneggiato', 'manutenzione', 'in_restauro', 'in_trasferimento', 'prestato']; if (!$copyResult || in_array($copyResult['stato'], $invalidStates, true)) { // Fail closed: roll back the just-applied 'in_corso' update instead // of committing a loan over a missing/non-lendable copy (BUG7c/D12). @@ -1473,8 +1477,27 @@ public function cancelReservation(Request $request, Response $response, mysqli $ throw new \RuntimeException('Failed to recalculate book availability'); } + // Promote the waitlist: an admin-cancelled reservation frees capacity, + // and every sibling release path (user cancel, admin edit, reject, + // cancelPickup, return) immediately converts the next queued + // reservation — cancelReservation was the only one that left the + // freed capacity idle until the next maintenance run. + $reservationManager = new \App\Controllers\ReservationManager($db); + $reservationManager->setExternalTransaction(true); + for ($promoGuard = 0; $promoGuard < 1000 && $reservationManager->processBookAvailability($libroId); $promoGuard++) { + // keep promoting while freed capacity converts the next queued reservation + } + $db->commit(); + // Notifiche accodate durante la transazione esterna (P2): inviale ora + // che il commit è avvenuto, come fa MaintenanceService. + try { + $reservationManager->flushDeferredNotifications(); + } catch (\Throwable $flushError) { + \App\Support\SecureLogger::warning("[cancelReservation] Deferred notification flush failed: " . $flushError->getMessage()); + } + // Notifica all'utente DOPO il commit (M11): try/catch isolato, un errore // di invio non deve far fallire l'annullamento già committato. try { diff --git a/app/Controllers/PrestitiController.php b/app/Controllers/PrestitiController.php index ddb8f31cc..fa2079490 100644 --- a/app/Controllers/PrestitiController.php +++ b/app/Controllers/PrestitiController.php @@ -572,6 +572,15 @@ public function store(Request $request, Response $response, mysqli $db): Respons $settingsRepo = new \App\Models\SettingsRepository($db); $pickupDays = (int) ($settingsRepo->get('loans', 'pickup_expiry_days', '3') ?? 3); $pickupDeadline = date('Y-m-d', strtotime("{$today} +{$pickupDays} days")); + // Cap alla fine della finestra del prestito (L1), come + // approveLoan/activateScheduledLoans: una deadline oltre + // data_scadenza permetterebbe di confermare il ritiro di un + // prestito già scaduto e terrebbe la copia impegnata oltre + // la finestra. Confronto lessicografico sicuro: entrambe + // validate Y-m-d sopra. + if ($pickupDeadline > $data_scadenza) { + $pickupDeadline = $data_scadenza; + } } } else { // Future loan - user will pick up when loan period starts @@ -940,6 +949,16 @@ public function update(Request $request, Response $response, mysqli $db, int $id $oldPrestito = (string) $locked['data_prestito']; $oldScadenza = (string) $locked['data_scadenza']; if ($newPrestito !== $oldPrestito || $newScadenza !== $oldScadenza) { + // Idoneità del prestatario quando la scadenza viene ESTESA (stesso + // gate di renew(): l'estensione conferisce un beneficio). Se + // l'utente cambia l'idoneità è già stata verificata sopra (M6b). + if ($newScadenza > $oldScadenza && $newUserId === (int) $locked['utente_id']) { + $eligibilityError = \App\Support\LoanEligibility::checkUser($db, $newUserId); + if ($eligibilityError !== null) { + $db->rollback(); + return $response->withHeader('Location', url('/admin/loans') . '?error=' . $eligibilityError)->withStatus(302); + } + } // Y-m-d strings compare correctly lexicographically (validated // strict above); ±1 day via DateTimeImmutable, no TZ ambiguity. $dayBefore = static fn (string $ymd): string => (new \DateTimeImmutable($ymd))->modify('-1 day')->format('Y-m-d'); @@ -1646,7 +1665,7 @@ public function bulkExtend(Request $request, Response $response, mysqli $db): Re // Rows closed or moved out of an extendable state meanwhile are // intentionally ignored, matching the previous bulk semantics. $lockLoans = $db->prepare( - "SELECT id, libro_id, copia_id, data_prestito, data_scadenza + "SELECT id, libro_id, copia_id, utente_id, data_prestito, data_scadenza FROM prestiti WHERE id IN ($placeholders) AND attivo = 1 @@ -1686,6 +1705,14 @@ public function bulkExtend(Request $request, Response $response, mysqli $db): Re $extended = 0; foreach ($loans as $loan) { + // Idoneità del prestatario per OGNI prestito esteso (stesso gate + // di renew(): l'estensione conferisce un beneficio — un utente + // sospeso o con tessera scaduta non deve trattenere il libro via + // bulk). Salta SOLO questo prestito, il resto del lotto procede: + // l'inidoneità di un utente non è un conflitto di capacità. + if (\App\Support\LoanEligibility::checkUser($db, (int) $loan['utente_id']) !== null) { + continue; + } // null == a capacity/copy conflict: roll the WHOLE batch back so // no partial extension is committed (same all-or-nothing contract // the tests pin). A thrown error is handled by the outer catch. diff --git a/app/Controllers/ReservationManager.php b/app/Controllers/ReservationManager.php index 9d4bb4add..f3d097216 100644 --- a/app/Controllers/ReservationManager.php +++ b/app/Controllers/ReservationManager.php @@ -547,7 +547,32 @@ private function updateQueuePositions($bookId, int $completedPosition) */ private function sendReservationNotification(array $reservation): bool { + $claimed = false; + $revertClaim = function () use ($reservation): void { + $stmt = $this->db->prepare("UPDATE prenotazioni SET notifica_inviata = 0 WHERE id = ?"); + $stmt->bind_param('i', $reservation['id']); + $stmt->execute(); + $stmt->close(); + }; + try { + // Claim atomico PRIMA dell'invio (stesso pattern claim-then-send dello + // sweep): tra il commit del chiamante e il flush differito lo sweep + // retryUnsentReservationNotifications() (cron/login admin) può leggere + // la stessa riga completata+notifica_inviata=0 e inviare — senza claim + // l'utente riceveva l'email 'reservation_book_available' doppia. + // affected_rows=0 => un altro processo ha già preso in carico (o già + // inviato) questa notifica. Ripristinato a 0 su invio fallito, così la + // riga resta eleggibile per il run successivo. + $claimStmt = $this->db->prepare("UPDATE prenotazioni SET notifica_inviata = 1 WHERE id = ? AND notifica_inviata = 0"); + $claimStmt->bind_param('i', $reservation['id']); + $claimStmt->execute(); + $claimed = $claimStmt->affected_rows === 1; + $claimStmt->close(); + if (!$claimed) { + return false; + } + // Get book details $stmt = $this->db->prepare(" SELECT l.titolo, COALESCE(l.isbn13, l.isbn10, '') as isbn, @@ -565,6 +590,7 @@ private function sendReservationNotification(array $reservation): bool $stmt->close(); if (!$book) { + $revertClaim(); return false; } @@ -597,16 +623,13 @@ private function sendReservationNotification(array $reservation): bool $variables ); - // Only mark as notified if email was actually sent successfully: - // le righe 'completata' con notifica_inviata=0 vengono riprese da + // notifica_inviata è già a 1 dal claim atomico in testa: su invio + // fallito rilascia il claim, così la riga 'completata' con + // notifica_inviata=0 viene ripresa da // retryUnsentReservationNotifications() al run di manutenzione/cron // successivo (M4) — prima nessuno le rileggeva e l'email era persa. - if ($success) { - $stmt = $this->db->prepare("UPDATE prenotazioni SET notifica_inviata = 1 WHERE id = ?"); - $stmt->bind_param('i', $reservation['id']); - $stmt->execute(); - $stmt->close(); - } else { + if (!$success) { + $revertClaim(); \App\Support\SecureLogger::warning('ReservationManager: email send failed, will be retried by retryUnsentReservationNotifications() on next maintenance run', [ 'reservation_id' => (int) $reservation['id'], ]); @@ -615,6 +638,16 @@ private function sendReservationNotification(array $reservation): bool return $success; } catch (\Throwable $e) { + if ($claimed) { + try { + $revertClaim(); + } catch (\Throwable $revertError) { + \App\Support\SecureLogger::error('ReservationManager: failed to release notification claim', [ + 'reservation_id' => (int) $reservation['id'], + 'error' => $revertError->getMessage(), + ]); + } + } \App\Support\SecureLogger::error('ReservationManager: failed to send reservation notification', [ 'error' => $e->getMessage(), ]); @@ -668,22 +701,12 @@ public function retryUnsentReservationNotifications(int $limit = 20): int // Claim-then-send (stesso pattern di warning/overdue): i tre percorsi // che invocano questo sweep (cron automatic-notifications, cron // full-maintenance e runIfNeeded() da login admin) usano lock diversi - // e possono girare in overlap, quindi senza claim atomico due run - // selezionerebbero la stessa riga e l'utente riceverebbe l'email doppia. - $claimStmt = $this->db->prepare("UPDATE prenotazioni SET notifica_inviata = 1 WHERE id = ? AND notifica_inviata = 0"); - $revertStmt = $this->db->prepare("UPDATE prenotazioni SET notifica_inviata = 0 WHERE id = ?"); - + // e possono girare in overlap. Il claim atomico vive DENTRO + // sendReservationNotification() — così copre anche il flush differito + // post-commit, che prima inviava senza claim e in overlap con questo + // sweep raddoppiava l'email; su invio fallito è sempre lui a + // rilasciare il claim, così la riga resta eleggibile al run dopo. foreach ($reservations as $reservation) { - $reservationId = (int) $reservation['id']; - - // Claim atomico PRIMA dell'invio: se affected_rows è 0 un run - // concorrente ha già preso in carico questa riga. - $claimStmt->bind_param('i', $reservationId); - $claimStmt->execute(); - if ($claimStmt->affected_rows < 1) { - continue; - } - // Normalizza entrambi gli estremi come CapacityService: le righe // legacy possono avere start/fine NULL ma una scadenza valida. $reservation['data_inizio_richiesta'] = $reservation['data_inizio_richiesta'] @@ -695,15 +718,8 @@ public function retryUnsentReservationNotifications(int $limit = 20): int if ($this->sendReservationNotification($reservation)) { $sentCount++; - } else { - // Invio fallito: rilascia il claim così la riga resta - // eleggibile per il run successivo. - $revertStmt->bind_param('i', $reservationId); - $revertStmt->execute(); } } - $claimStmt->close(); - $revertStmt->close(); return $sentCount; } diff --git a/app/Support/MaintenanceService.php b/app/Support/MaintenanceService.php index f7833ee06..e5741cac4 100644 --- a/app/Support/MaintenanceService.php +++ b/app/Support/MaintenanceService.php @@ -710,8 +710,12 @@ public function checkExpiredReservations(): int } $copiaId = $lockedReservation['copia_id'] ? (int) $lockedReservation['copia_id'] : null; - // Build note suffix safely with bound parameter - $noteSuffix = "\n[System] " . __('Scaduta il') . ' ' . date('d/m/Y'); + // Build note suffix safely with bound parameter. Data nel fuso + // applicativo (P4): la decisione di scadenza usa $today + // (DateHelper::today()), mentre date('d/m/Y') userebbe la TZ del + // processo — a cavallo della mezzanotte la nota citava un giorno + // diverso da quello effettivamente deciso. + $noteSuffix = "\n[System] " . __('Scaduta il') . ' ' . implode('/', array_reverse(explode('-', $today))); // Mark as expired. Re-assert stato='prenotato' + check affected_rows // (D14): a concurrent confirmPickup/activateScheduledLoans may have @@ -888,8 +892,10 @@ public function checkExpiredPickups(): int } $copiaId = $lockedPickup['copia_id'] ? (int) $lockedPickup['copia_id'] : null; - // Build note suffix safely with bound parameter - $noteSuffix = "\n[System] " . __('Ritiro scaduto il') . ' ' . date('d/m/Y'); + // Build note suffix safely with bound parameter. Data nel fuso + // applicativo (P4), come sopra: stessa data della decisione + // basata su $today, non la TZ del processo. + $noteSuffix = "\n[System] " . __('Ritiro scaduto il') . ' ' . implode('/', array_reverse(explode('-', $today))); // Mark as expired with state guard (prevents TOCTOU with concurrent confirmPickup) $updateStmt = $this->db->prepare(" diff --git a/app/Support/NotificationService.php b/app/Support/NotificationService.php index 6ac994813..0d6667fca 100644 --- a/app/Support/NotificationService.php +++ b/app/Support/NotificationService.php @@ -474,12 +474,13 @@ public function sendLoanExpirationWarnings(): int { // warning horizon. An exact "today + X" match misses short loans created // inside that horizon (especially loans created with due date today). // Overdue loans remain handled separately below by the overdue workflow. + // CI-SOFT-DELETE-EXEMPT: the expiry warning must keep firing for active loans whose book was archived — the copy is still out and the borrower must be chased (same rationale as sendLoanRecalls). $stmt = $this->db->prepare(" SELECT p.id, p.data_scadenza, l.titolo as libro_titolo, CONCAT(u.nome, ' ', u.cognome) as utente_nome, u.email as utente_email, DATEDIFF(p.data_scadenza, ?) as giorni_rimasti FROM prestiti p - JOIN libri l ON p.libro_id = l.id AND l.deleted_at IS NULL + JOIN libri l ON p.libro_id = l.id JOIN utenti u ON p.utente_id = u.id WHERE p.stato = 'in_corso' AND p.attivo = 1 @@ -580,12 +581,13 @@ public function sendOverdueLoanNotifications(): int { $today = DateHelper::today(); // Get overdue loans + // CI-SOFT-DELETE-EXEMPT: the first overdue notice must keep firing for active loans whose book was archived — soft-deleting the book silenced this notice AND, since sendLoanRecalls() requires overdue_notification_sent=1, every automatic recall after it. $stmt = $this->db->prepare(" SELECT p.id, p.data_scadenza, l.titolo as libro_titolo, CONCAT(u.nome, ' ', u.cognome) as utente_nome, u.email as utente_email, DATEDIFF(?, p.data_scadenza) as giorni_ritardo FROM prestiti p - JOIN libri l ON p.libro_id = l.id AND l.deleted_at IS NULL + JOIN libri l ON p.libro_id = l.id JOIN utenti u ON p.utente_id = u.id WHERE p.stato IN ('in_corso', 'in_ritardo') AND p.attivo = 1 diff --git a/tests/audit-fixes-p2-p4.unit.php b/tests/audit-fixes-p2-p4.unit.php new file mode 100644 index 000000000..c448d9d05 --- /dev/null +++ b/tests/audit-fixes-p2-p4.unit.php @@ -0,0 +1,468 @@ +set_charset('utf8mb4'); +} catch (\Throwable $e) { + fwrite(STDERR, "FAIL: database unreachable — mandatory for this test: {$e->getMessage()}\n"); + exit(1); +} + +$TESTNO = 0; +$failed = 0; +function check(bool $cond, string $desc): void +{ + global $TESTNO, $failed; + $TESTNO++; + printf("[%02d] %s: %s\n", $TESTNO, $cond ? 'PASS' : 'FAIL', $desc); + if (!$cond) { + $failed++; + } +} + +// Per-run token: cleanup and assertions only ever touch this run's rows. +$RUN = bin2hex(random_bytes(6)); +$TITLE_PREFIX = "ZZ_AFIX_{$RUN}_"; +$EMAIL_DOMAIN = '@afix.test.local'; + +$today = DateHelper::today(); +$d = static fn (int $offsetDays): string => date('Y-m-d', strtotime($today . ($offsetDays >= 0 ? " +{$offsetDays} days" : ' ' . $offsetDays . ' days'))); + +$settingsRow = static function (string $key, string $default) use ($db): string { + $stmt = $db->prepare("SELECT setting_value FROM system_settings WHERE category = 'loans' AND setting_key = ?"); + $stmt->bind_param('s', $key); + $stmt->execute(); + $row = $stmt->get_result()->fetch_assoc(); + $stmt->close(); + return $row !== null ? (string) $row['setting_value'] : $default; +}; +$pickupDays = max(0, (int) $settingsRow('pickup_expiry_days', '3')); + +/* -------------------------------- helpers -------------------------------- */ + +$userSeq = 0; +$mkUser = static function (string $stato = 'attivo') use ($db, $RUN, $EMAIL_DOMAIN, &$userSeq): int { + $userSeq++; + $tessera = 'ZAFIX' . strtoupper(substr($RUN, 0, 7)) . $userSeq; + $email = "u{$userSeq}-{$RUN}{$EMAIL_DOMAIN}"; + $cognome = "ZZAFIX {$userSeq}"; + $stmt = $db->prepare("INSERT INTO utenti (codice_tessera, nome, cognome, email, password, stato, tipo_utente, email_verificata) + VALUES (?, 'Test', ?, ?, 'x', ?, 'standard', 1)"); + $stmt->bind_param('ssss', $tessera, $cognome, $email, $stato); + $stmt->execute(); + $stmt->close(); + return (int) $db->insert_id; +}; + +$bookSeq = 0; +$mkBook = static function (int $copies) use ($db, $TITLE_PREFIX, &$bookSeq): array { + $bookSeq++; + $title = $TITLE_PREFIX . $bookSeq; + $stmt = $db->prepare("INSERT INTO libri (titolo, copie_totali, copie_disponibili, created_at, updated_at) + VALUES (?, ?, ?, NOW(), NOW())"); + $stmt->bind_param('sii', $title, $copies, $copies); + $stmt->execute(); + $stmt->close(); + $bookId = (int) $db->insert_id; + $copyIds = []; + for ($i = 1; $i <= $copies; $i++) { + $code = "ZZAFIX-{$bookId}-C{$i}"; + $stmt = $db->prepare("INSERT INTO copie (libro_id, numero_inventario, stato) VALUES (?, ?, 'disponibile')"); + $stmt->bind_param('is', $bookId, $code); + $stmt->execute(); + $stmt->close(); + $copyIds[] = (int) $db->insert_id; + } + return [$bookId, $copyIds]; +}; + +$mkLoan = static function (int $bookId, ?int $copiaId, int $userId, string $stato, string $from, string $to, ?string $pickupDeadline = null) use ($db): int { + $stmt = $db->prepare("INSERT INTO prestiti (libro_id, copia_id, utente_id, data_prestito, data_scadenza, stato, origine, attivo, pickup_deadline, warning_sent, overdue_notification_sent) + VALUES (?, ?, ?, ?, ?, ?, 'diretto', 1, ?, 0, 0)"); + $stmt->bind_param('iiissss', $bookId, $copiaId, $userId, $from, $to, $stato, $pickupDeadline); + $stmt->execute(); + $stmt->close(); + return (int) $db->insert_id; +}; + +$setCopyState = static function (int $copiaId, string $stato) use ($db): void { + $stmt = $db->prepare('UPDATE copie SET stato = ? WHERE id = ?'); + $stmt->bind_param('si', $stato, $copiaId); + $stmt->execute(); + $stmt->close(); +}; + +$loanRow = static function (int $loanId) use ($db): array { + $stmt = $db->prepare('SELECT stato, attivo, data_prestito, data_scadenza, pickup_deadline, warning_sent, overdue_notification_sent, note FROM prestiti WHERE id = ?'); + $stmt->bind_param('i', $loanId); + $stmt->execute(); + $row = $stmt->get_result()->fetch_assoc() ?: []; + $stmt->close(); + return $row; +}; + +$reservationRow = static function (int $resId) use ($db): array { + $stmt = $db->prepare('SELECT stato, notifica_inviata, queue_position FROM prenotazioni WHERE id = ?'); + $stmt->bind_param('i', $resId); + $stmt->execute(); + $row = $stmt->get_result()->fetch_assoc() ?: []; + $stmt->close(); + return $row; +}; + +$mkQueueReservation = static function (int $bookId, int $userId, string $from, string $to, int $queuePos, int $notified = 0, string $stato = 'attiva') use ($db): int { + $stmt = $db->prepare("INSERT INTO prenotazioni (libro_id, utente_id, data_inizio_richiesta, data_fine_richiesta, data_scadenza_prenotazione, stato, queue_position, notifica_inviata) + VALUES (?, ?, ?, ?, ?, ?, ?, ?)"); + $stmt->bind_param('iisssssi', $bookId, $userId, $from, $to, $to, $stato, $queuePos, $notified); + $stmt->execute(); + $stmt->close(); + return (int) $db->insert_id; +}; + +$jsonBody = static function ($response): array { + return (array) json_decode((string) $response->getBody(), true); +}; + +// Whether email delivery actually works in this environment (locally the +// 'mail' driver hands off to sendmail and returns true; on CI it may not). +// Email-outcome-dependent assertions are gated on this probe. +$mailWorks = false; +try { + $probeEmail = new \App\Support\EmailService($db); + $mailWorks = $probeEmail->sendTemplate("probe-{$RUN}{$EMAIL_DOMAIN}", 'reservation_book_available', [ + 'utente_nome' => 'Probe', 'libro_titolo' => 'Probe', 'libro_autore' => 'Probe', + 'libro_isbn' => 'N/A', 'data_inizio' => '01-01-2026', 'data_fine' => '02-01-2026', + 'book_url' => 'http://localhost/', 'profile_url' => 'http://localhost/', + ], 'it_IT'); +} catch (\Throwable $e) { + $mailWorks = false; +} +echo 'mail delivery in this environment: ' . ($mailWorks ? 'WORKING' : 'unavailable (email-dependent assertions relaxed)') . "\n"; + +/* -------------------------------- cleanup -------------------------------- */ +$cleanup = static function () use ($db, $TITLE_PREFIX, $RUN, $EMAIL_DOMAIN): void { + $like = $db->real_escape_string($TITLE_PREFIX) . '%'; + $db->query("DELETE n FROM admin_notifications n JOIN prestiti p ON n.related_id = p.id JOIN libri l ON l.id = p.libro_id WHERE l.titolo LIKE '{$like}'"); + $db->query("DELETE r FROM prenotazioni r JOIN libri l ON l.id = r.libro_id WHERE l.titolo LIKE '{$like}'"); + $db->query("DELETE p FROM prestiti p JOIN libri l ON l.id = p.libro_id WHERE l.titolo LIKE '{$like}'"); + $db->query("DELETE c FROM copie c JOIN libri l ON l.id = c.libro_id WHERE l.titolo LIKE '{$like}'"); + $db->query("DELETE FROM libri WHERE titolo LIKE '{$like}'"); + $emailLike = $db->real_escape_string("u%-{$RUN}{$EMAIL_DOMAIN}"); + $db->query("DELETE FROM utenti WHERE email LIKE '{$emailLike}'"); +}; +$cleanup(); + +$processTz = date_default_timezone_get(); + +try { + /* =============== P2-1: confirmPickup refuses a copy still out ========= */ + $borrower1 = $mkUser(); + $picker1 = $mkUser(); + [$bookP1, [$copyP1]] = $mkBook(1); + // Predecessor still out and overdue (not yet flipped), copy 'prestato'. + $prevLoan = $mkLoan($bookP1, $copyP1, $borrower1, 'in_corso', $d(-30), $d(-5)); + $setCopyState($copyP1, 'prestato'); + // Successor already announced ready on the SAME copy (the #366-adjacent + // inconsistent state this guard must fail closed on). + $nextLoan = $mkLoan($bookP1, $copyP1, $picker1, 'da_ritirare', $d(-1), $d(10), $d(2)); + + $_SESSION['user'] = ['tipo_utente' => 'admin', 'id' => $picker1]; + $request = (new ServerRequestFactory()) + ->createServerRequest('POST', '/admin/loans/confirm-pickup') + ->withParsedBody(['loan_id' => $nextLoan]); + $resp = (new LoanApprovalController())->confirmPickup($request, (new ResponseFactory())->createResponse(), $db); + $body = $jsonBody($resp); + + check(($body['success'] ?? null) === false, 'P2-1: confirmPickup REFUSES pickup while the assigned copy is still prestato'); + check(($loanRow($nextLoan)['stato'] ?? '') === 'da_ritirare', 'P2-1: the successor loan stays da_ritirare (rolled back)'); + $copyState = (string) $db->query("SELECT stato FROM copie WHERE id = {$copyP1}")->fetch_row()[0]; + check($copyState === 'prestato', 'P2-1: the copy stays prestato (still held by the open predecessor)'); + $activeOnCopy = (int) $db->query("SELECT COUNT(*) FROM prestiti WHERE copia_id = {$copyP1} AND attivo = 1 AND stato IN ('in_corso','in_ritardo')")->fetch_row()[0]; + check($activeOnCopy === 1, 'P2-1: exactly ONE running loan on the physical copy (no double-issue)'); + + /* =============== P2-2: admin cancelReservation promotes the queue ===== */ + $resUser1 = $mkUser(); + $resUser2 = $mkUser(); + [$bookQ, [$copyQ]] = $mkBook(1); + // Two queued reservations over the same window on a 1-copy book: neither + // can promote while the other occupies the capacity unit. + $resQ1 = $mkQueueReservation($bookQ, $resUser1, $d(0), $d(10), 1); + $resQ2 = $mkQueueReservation($bookQ, $resUser2, $d(0), $d(10), 2); + + $_SESSION['user'] = ['tipo_utente' => 'admin', 'id' => $resUser1]; + $request = (new ServerRequestFactory()) + ->createServerRequest('POST', '/admin/reservations/cancel') + ->withParsedBody(['reservation_id' => $resQ1, 'reason' => 'test P2-2']); + $resp = (new LoanApprovalController())->cancelReservation($request, (new ResponseFactory())->createResponse(), $db); + $body = $jsonBody($resp); + + check(($body['success'] ?? null) === true, 'P2-2: admin cancelReservation succeeds'); + check(($reservationRow($resQ1)['stato'] ?? '') === 'annullata', 'P2-2: the cancelled reservation is annullata'); + check(($reservationRow($resQ2)['stato'] ?? '') === 'completata', 'P2-2: the next queued reservation is promoted IN the same request (not left for the cron)'); + // The promotion always creates the conversion loan as 'pendente' (attivo=0): + // it awaits the admin's confirmation of the physical pickup. + $promotedLoans = (int) $db->query("SELECT COUNT(*) FROM prestiti WHERE libro_id = {$bookQ} AND utente_id = {$resUser2} AND stato = 'pendente'")->fetch_row()[0]; + check($promotedLoans === 1, 'P2-2: the promotion created the successor conversion loan (pendente)'); + + /* =============== P2-3: chase-up senders on an archived book =========== */ + $overdueUser = $mkUser(); + [$bookArch, [$copyArch]] = $mkBook(1); + $archLoan = $mkLoan($bookArch, $copyArch, $overdueUser, 'in_corso', $d(-20), $d(-3)); + $setCopyState($copyArch, 'prestato'); + $warnUser = $mkUser(); + [$bookArch2, [$copyArch2]] = $mkBook(1); + $warnLoan = $mkLoan($bookArch2, $copyArch2, $warnUser, 'in_corso', $d(-10), $d(0)); + $setCopyState($copyArch2, 'prestato'); + // Archive both books while their loans are still out. + $db->query("UPDATE libri SET deleted_at = NOW() WHERE id IN ({$bookArch}, {$bookArch2})"); + + $notifications = new NotificationService($db); + $notifications->sendOverdueLoanNotifications(); + $row = $loanRow($archLoan); + // The claim flips stato regardless of the email outcome: pre-fix the loan + // was never selected at all and stayed 'in_corso'. + check(($row['stato'] ?? '') === 'in_ritardo', 'P2-3: the overdue notice still processes the loan of an ARCHIVED book (stato flipped by the claim)'); + if ($mailWorks) { + check((int) ($row['overdue_notification_sent'] ?? 0) === 1, 'P2-3: overdue_notification_sent=1 — sendLoanRecalls() is unblocked for the archived book'); + } + + $notifications->sendLoanExpirationWarnings(); + if ($mailWorks) { + check((int) ($loanRow($warnLoan)['warning_sent'] ?? 0) === 1, 'P2-3: the expiry warning still fires for the loan of an ARCHIVED book'); + } + + // Environment-independent guard: neither chase-up sender may filter on + // libri.deleted_at (same source-assertion style as the runAll() order check + // in issue-366-full-scenario). + $notifSrc = (string) file_get_contents($root . '/app/Support/NotificationService.php'); + $extractMethod = static function (string $src, string $needle): string { + $start = strpos($src, $needle); + if ($start === false) { + return ''; + } + $end = strpos($src, "\n public function ", $start + strlen($needle)); + return substr($src, $start, ($end === false ? strlen($src) : $end) - $start); + }; + check(!str_contains($extractMethod($notifSrc, 'public function sendLoanExpirationWarnings'), 'deleted_at'), + 'P2-3: sendLoanExpirationWarnings() has no deleted_at filter (source guard)'); + check(!str_contains($extractMethod($notifSrc, 'public function sendOverdueLoanNotifications'), 'deleted_at'), + 'P2-3: sendOverdueLoanNotifications() has no deleted_at filter (source guard)'); + + /* =============== P2-4: claim-before-send on the reservation email ===== */ + $notifUser = $mkUser(); + [$bookN, [$copyN]] = $mkBook(1); + $sendMethod = new \ReflectionMethod(ReservationManager::class, 'sendReservationNotification'); + $sendMethod->setAccessible(true); + $manager = new ReservationManager($db); + $userRow = $db->query("SELECT email, nome, cognome FROM utenti WHERE id = {$notifUser}")->fetch_assoc(); + $mkPayload = static fn (int $resId, int $bookId) => [ + 'id' => $resId, 'libro_id' => $bookId, + 'email' => $userRow['email'], 'nome' => $userRow['nome'], 'cognome' => $userRow['cognome'], + 'data_inizio_richiesta' => $d(0), 'data_fine_richiesta' => $d(10), + ]; + + // Race replay: the sweep has ALREADY claimed this row (notifica_inviata=1) + // between the caller's commit and the deferred flush. The flush's send must + // now be a no-op — pre-fix it sent the email again (returned true). + $resClaimed = $mkQueueReservation($bookN, $notifUser, $d(0), $d(10), 1, 1, 'completata'); + $sent = (bool) $sendMethod->invoke($manager, $mkPayload($resClaimed, $bookN)); + check($sent === false, 'P2-4: a reservation already claimed by the sweep is NOT sent again by the deferred flush'); + check((int) ($reservationRow($resClaimed)['notifica_inviata'] ?? 0) === 1, 'P2-4: the concurrent claim is left untouched'); + + if ($mailWorks) { + // Happy path: unclaimed row is claimed + sent exactly once. + $resFresh = $mkQueueReservation($bookN, $notifUser, $d(0), $d(10), 2, 0, 'completata'); + $sent = (bool) $sendMethod->invoke($manager, $mkPayload($resFresh, $bookN)); + check($sent === true, 'P2-4: an unclaimed reservation is claimed and sent'); + check((int) ($reservationRow($resFresh)['notifica_inviata'] ?? 0) === 1, 'P2-4: successful send keeps the claim (notifica_inviata=1)'); + } + + // Failure path: the send aborts (archived book) AFTER the claim — the claim + // must be released so the retry sweep still sees the row. + [$bookN2] = $mkBook(1); + $resFail = $mkQueueReservation($bookN2, $notifUser, $d(0), $d(10), 1, 0, 'completata'); + $db->query("UPDATE libri SET deleted_at = NOW() WHERE id = {$bookN2}"); + $sent = (bool) $sendMethod->invoke($manager, $mkPayload($resFail, $bookN2)); + check($sent === false && (int) ($reservationRow($resFail)['notifica_inviata'] ?? 1) === 0, + 'P2-4: a failed send releases the claim (row stays eligible for the retry sweep)'); + + /* =============== P3-1: store() caps pickup_deadline =================== */ + $storeAdmin = $mkUser(); + $storeUser = $mkUser(); + [$bookS] = $mkBook(1); + $_SESSION['user'] = ['tipo_utente' => 'admin', 'id' => $storeAdmin]; + $request = (new ServerRequestFactory()) + ->createServerRequest('POST', '/admin/loans/store') + ->withParsedBody([ + 'utente_id' => (string) $storeUser, + 'libro_id' => (string) $bookS, + 'data_prestito' => $d(0), + 'data_scadenza' => $d(1), + ]); + $resp = (new PrestitiController())->store($request, (new ResponseFactory())->createResponse(), $db); + check(str_contains($resp->getHeaderLine('Location'), 'created=1'), 'P3-1: direct admin loan (due tomorrow, no immediate delivery) is created'); + $storeLoanRow = $db->query("SELECT id, stato, pickup_deadline, data_scadenza FROM prestiti WHERE libro_id = {$bookS} AND utente_id = {$storeUser}")->fetch_assoc() ?: []; + check(($storeLoanRow['stato'] ?? '') === 'da_ritirare', 'P3-1: the loan is da_ritirare with a pickup deadline'); + $expectedDeadline = min($d($pickupDays), $d(1)); + check(($storeLoanRow['pickup_deadline'] ?? '') === $expectedDeadline && ($storeLoanRow['pickup_deadline'] ?? 'z') <= ($storeLoanRow['data_scadenza'] ?? ''), + "P3-1: pickup_deadline capped at data_scadenza ({$expectedDeadline}) like approveLoan/activateScheduledLoans"); + if ($pickupDays <= 1) { + echo " (note: loans.pickup_expiry_days={$pickupDays} makes the cap non-discriminating in this environment)\n"; + } + + /* =============== P3-2: eligibility on bulkExtend and update() ========= */ + $goodUser = $mkUser(); + $suspendedUser = $mkUser('sospeso'); + [$bookG, [$copyG]] = $mkBook(1); + [$bookX, [$copyX]] = $mkBook(1); + $goodLoan = $mkLoan($bookG, $copyG, $goodUser, 'in_corso', $d(-5), $d(5)); + $setCopyState($copyG, 'prestato'); + $suspLoan = $mkLoan($bookX, $copyX, $suspendedUser, 'in_corso', $d(-5), $d(5)); + $setCopyState($copyX, 'prestato'); + + $_SESSION['user'] = ['tipo_utente' => 'admin', 'id' => $goodUser]; + $request = (new ServerRequestFactory()) + ->createServerRequest('POST', '/admin/loans/bulk-extend') + ->withParsedBody(['ids' => [(string) $goodLoan, (string) $suspLoan], 'days' => '5']); + $resp = (new PrestitiController())->bulkExtend($request, (new ResponseFactory())->createResponse(), $db); + $location = $resp->getHeaderLine('Location'); + + check(str_contains($location, 'bulk_extended=1'), 'P3-2: bulkExtend extends only the eligible borrower\'s loan (1 of 2)'); + check(($loanRow($goodLoan)['data_scadenza'] ?? '') === $d(10), 'P3-2: eligible loan extended (+5 from its due date)'); + check(($loanRow($suspLoan)['data_scadenza'] ?? '') === $d(5), 'P3-2: SUSPENDED borrower\'s loan NOT extended by bulkExtend'); + + // update(): extending the due date for a suspended borrower must be refused + // (same benefit gate as renew()); shrinking it must still be allowed. + $request = (new ServerRequestFactory()) + ->createServerRequest('POST', '/admin/loans/edit/' . $suspLoan) + ->withParsedBody(['utente_id' => (string) $suspendedUser, 'data_prestito' => $d(-5), 'data_scadenza' => $d(12)]); + $resp = (new PrestitiController())->update($request, (new ResponseFactory())->createResponse(), $db, $suspLoan); + check(str_contains($resp->getHeaderLine('Location'), 'error=user_suspended'), 'P3-2: update() refuses a due-date EXTENSION for a suspended borrower'); + check(($loanRow($suspLoan)['data_scadenza'] ?? '') === $d(5), 'P3-2: refused extension leaves the due date unchanged'); + + $request = (new ServerRequestFactory()) + ->createServerRequest('POST', '/admin/loans/edit/' . $suspLoan) + ->withParsedBody(['utente_id' => (string) $suspendedUser, 'data_prestito' => $d(-5), 'data_scadenza' => $d(3)]); + $resp = (new PrestitiController())->update($request, (new ResponseFactory())->createResponse(), $db, $suspLoan); + check(!str_contains($resp->getHeaderLine('Location'), 'error='), 'P3-2: shrinking the due date for a suspended borrower is still allowed (no benefit conferred)'); + check(($loanRow($suspLoan)['data_scadenza'] ?? '') === $d(3), 'P3-2: shrunk due date applied'); + + /* =============== P4: audit note date in the app timezone ============== */ + // Force a process TZ whose current DATE differs from the app-TZ date; the + // two candidates span 26 hours, so at any instant at least one differs. + $driftTz = null; + foreach (['Pacific/Kiritimati', 'Etc/GMT+12'] as $tzName) { + if ((new \DateTime('now', new \DateTimeZone($tzName)))->format('Y-m-d') !== $today) { + $driftTz = $tzName; + break; + } + } + check($driftTz !== null, 'P4: found a process TZ whose date differs from the app-TZ date'); + + $expiredUser = $mkUser(); + [$bookE1, [$copyE1]] = $mkBook(1); + $resExpired = $mkLoan($bookE1, $copyE1, $expiredUser, 'prenotato', $d(-10), $d(-2)); + $setCopyState($copyE1, 'prenotato'); + [$bookE2, [$copyE2]] = $mkBook(1); + $pickupExpired = $mkLoan($bookE2, $copyE2, $expiredUser, 'da_ritirare', $d(-6), $d(10), $d(-1)); + $setCopyState($copyE2, 'prenotato'); + + $appNoteDate = implode('/', array_reverse(explode('-', $today))); + if ($driftTz !== null) { + date_default_timezone_set($driftTz); + try { + $procNoteDate = date('d/m/Y'); + $svc = new MaintenanceService($db); + $svc->checkExpiredReservations(); + $svc->checkExpiredPickups(); + } finally { + date_default_timezone_set($processTz); + } + + $row = $loanRow($resExpired); + check(($row['stato'] ?? '') === 'scaduto' && str_contains((string) $row['note'], $appNoteDate), + "P4: 'Scaduta il' audit note carries the app-TZ date ({$appNoteDate})"); + check(!str_contains((string) $row['note'], $procNoteDate), + "P4: 'Scaduta il' audit note does NOT carry the process-TZ date ({$procNoteDate})"); + + $row = $loanRow($pickupExpired); + check(($row['stato'] ?? '') === 'scaduto' && str_contains((string) $row['note'], $appNoteDate), + "P4: 'Ritiro scaduto il' audit note carries the app-TZ date ({$appNoteDate})"); + check(!str_contains((string) $row['note'], $procNoteDate), + "P4: 'Ritiro scaduto il' audit note does NOT carry the process-TZ date ({$procNoteDate})"); + } + +} catch (\Throwable $e) { + date_default_timezone_set($processTz); + $cleanup(); + fwrite(STDERR, 'FAIL: ' . $e->getMessage() . "\n" . $e->getTraceAsString() . "\n"); + $db->close(); + exit(1); +} + +$cleanup(); +$db->close(); +echo "\n" . ($failed === 0 ? "ALL {$TESTNO} PASS\n" : "{$failed}/{$TESTNO} FAILED\n"); +exit($failed > 0 ? 1 : 0); From 03b718c85203bf3f86b35c5b951c8b7664f28577 Mon Sep 17 00:00:00 2001 From: fabiodalez-dev Date: Thu, 20 Aug 2026 11:55:23 +0200 Subject: [PATCH 15/16] docs(changelog): document the reservation/loan edge-case fixes in 0.7.62 --- CHANGELOG.md | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index caa28321e..da0ff2fa3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -48,6 +48,38 @@ Overdue-loan recalls (solleciti) and emailing the loan receipt (#360). instead of being silently skipped. Covered by contract and per-bundled-plugin integration tests. +### Fixed + +- **Book announced "ready for pickup" while still on an overdue loan (#366)**: + a reservation scheduled right after a loan that then went overdue and was + never returned was promoted to `da_ritirare` on its date alone, emailing the + next patron a wrong "ready for pickup" notice while the book was still out. + Promotion is now gated on a copy being physically free (active loans below the + copy count; a pinned copy must be on the shelf). The full reported sequence is + covered too: rescheduling an open reservation/pickup no longer leaves a stale + `pickup_deadline` for the expiry sweep to cull a valid loan against, the + overdue flip now runs first in the maintenance pass so an unreturned overdue + loan keeps holding its copy, and `renew()` refuses a date-overdue loan. +- **Concurrent circulation actions no longer corrupt state**: eight + transactions (approve/return/reject/cancel loan and reservation) resolved the + loan id with a plain read before taking the book lock, so under REPEATABLE + READ their later reads were blind to a competitor that had just committed. + A just-cancelled reservation could be promoted and emailed, and one physical + copy could be committed to two loans. The lookups now run before the + transaction so the first locked read fixes the snapshot, and reservation + promotion claims the row with a state-guarded update. +- **Pickup confirmation** now refuses a copy that is still out on another loan + (`prestato`), preventing a double issue of the same physical copy. +- **Admin reservation cancellation** now promotes the next reservation in the + queue immediately, like every other path that frees a copy. +- **Overdue notices and automatic recalls** now fire for loans whose book has + been archived (soft-deleted) — the chase-up mail no longer filters those out. +- **The "reservation available" email** can no longer be sent twice when the + retry sweep races the request that promoted it. +- Admin direct loans cap the pickup deadline at the due date; bulk loan + extension and reschedules re-check borrower eligibility; expiry audit notes + use the same day the decision was made near midnight. + ### Internal - CI: the OWASP ZAP baseline no longer fails on the ISBN/EAN-13 PII-disclosure From 025e79c3b8fffde4021b102e988d55a4cfbc04cf Mon Sep 17 00:00:00 2001 From: fabiodalez-dev Date: Thu, 20 Aug 2026 12:33:20 +0200 Subject: [PATCH 16/16] fix(tests,release): address review findings on the audit-fix tests and verifiers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Both release verifiers now also reject a symlinked parent storage/ directory (-d/-f follow a symlinked parent, which could redirect the whole scan outside the package); a static guard in code-quality.spec.js pins the -L storage check. - pickup-ready-copy-free-366.unit.php read DB credentials only from .env and exit(0)'d ('SKIP') when the DB was unreachable, so it could silently no-op in CI. It now reads getenv(E2E_DB_*) first like the other three tests, and fails (exit 1) instead of skipping when CI_STRICT_TESTS=1. - issue-366-full-scenario.unit.php: the runAll()-ordering check cast strpos() to int, so a renamed signature would fold to offset 0 and pass spuriously — now asserts the method was found before checking the order. - Corrected the misleading 'touches only rows it creates' header on the three tests that drive GLOBAL maintenance sweeps (they mutate every matching row and must run against an isolated test DB, as CI does). --- bin/build-release.sh | 4 ++-- scripts/ci-verify-release.sh | 4 ++-- tests/audit-fixes-p2-p4.unit.php | 8 ++++++-- tests/code-quality.spec.js | 6 ++++++ tests/issue-366-full-scenario.unit.php | 10 +++++++--- tests/pickup-ready-copy-free-366.unit.php | 22 +++++++++++++++++----- 6 files changed, 40 insertions(+), 14 deletions(-) diff --git a/bin/build-release.sh b/bin/build-release.sh index 4c0ae32d0..57f744388 100644 --- a/bin/build-release.sh +++ b/bin/build-release.sh @@ -248,8 +248,8 @@ verify_package_contents() { # a REAL placeholder, and nothing else. Reject symlinks first (-d/-f follow # them, so a symlinked dir/placeholder could point outside the tree and slip # session data past the scan), then require the placeholder, then scan. - if [ -L "$package_dir/storage/sessions" ] || [ ! -d "$package_dir/storage/sessions" ]; then - log_error "storage/sessions is not a real directory" + if [ -L "$package_dir/storage" ] || [ -L "$package_dir/storage/sessions" ] || [ ! -d "$package_dir/storage/sessions" ]; then + log_error "storage or storage/sessions is not a real directory" has_errors=true elif [ -L "$package_dir/storage/sessions/.gitkeep" ] || [ ! -f "$package_dir/storage/sessions/.gitkeep" ]; then log_error "storage/sessions/.gitkeep is missing or a symlink" diff --git a/scripts/ci-verify-release.sh b/scripts/ci-verify-release.sh index 03330c0ee..48286132a 100755 --- a/scripts/ci-verify-release.sh +++ b/scripts/ci-verify-release.sh @@ -85,8 +85,8 @@ fi # storage/sessions must be a REAL directory holding a REAL .gitkeep. Reject # symlinks first: -d/-f follow them, so a symlinked directory or placeholder # could point outside the tree and slip session data past the scan. -if [ -L "$package_dir/storage/sessions" ] || [ ! -d "$package_dir/storage/sessions" ]; then - echo "release storage/sessions is not a real directory" >&2 +if [ -L "$package_dir/storage" ] || [ -L "$package_dir/storage/sessions" ] || [ ! -d "$package_dir/storage/sessions" ]; then + echo "release storage or storage/sessions is not a real directory" >&2 exit 1 fi if [ -L "$package_dir/storage/sessions/.gitkeep" ] || [ ! -f "$package_dir/storage/sessions/.gitkeep" ]; then diff --git a/tests/audit-fixes-p2-p4.unit.php b/tests/audit-fixes-p2-p4.unit.php index c448d9d05..1c832880c 100644 --- a/tests/audit-fixes-p2-p4.unit.php +++ b/tests/audit-fixes-p2-p4.unit.php @@ -26,8 +26,12 @@ * not from the process TZ. * * Drives the REAL production paths (LoanApprovalController, PrestitiController, - * ReservationManager, NotificationService, MaintenanceService). Touches only - * rows it creates (titles ZZ_AFIX_%, emails @afix.test.local) and cleans up. + * ReservationManager, NotificationService, MaintenanceService). It asserts only + * on rows it creates (titles ZZ_AFIX_%, emails @afix.test.local) and cleans them + * up, but the P2-3/P4 checks invoke GLOBAL maintenance senders/sweeps + * (sendOverdueLoanNotifications/sendLoanExpirationWarnings and the expiry + * culls), which touch every matching row — run against an isolated/dedicated + * test DB (as CI does), never a shared one. * * Run: php tests/audit-fixes-p2-p4.unit.php */ diff --git a/tests/code-quality.spec.js b/tests/code-quality.spec.js index 74227547a..a5ba48cf8 100644 --- a/tests/code-quality.spec.js +++ b/tests/code-quality.spec.js @@ -226,6 +226,12 @@ test.describe.serial('Code Quality — 15 static analysis tests', () => { .toContain('-L "$package_dir/storage/sessions/.gitkeep"'); expect(releaseBuilder, 'release build must reject a symlinked .gitkeep placeholder') .toContain('-L "$package_dir/storage/sessions/.gitkeep"'); + // The parent storage/ must be rejected too: -d/-f follow a symlinked + // parent, which could redirect the whole scan outside the package. + expect(archiveVerifier, 'archive verification must reject a symlinked parent storage/') + .toContain('-L "$package_dir/storage"'); + expect(releaseBuilder, 'release build must reject a symlinked parent storage/') + .toContain('-L "$package_dir/storage"'); }); // ── 3. Plugin ensureSchema() called from onActivate() ───────────────────── diff --git a/tests/issue-366-full-scenario.unit.php b/tests/issue-366-full-scenario.unit.php index fb00e5877..cc039b8e0 100644 --- a/tests/issue-366-full-scenario.unit.php +++ b/tests/issue-366-full-scenario.unit.php @@ -31,7 +31,10 @@ * Drives the REAL production paths: MaintenanceService::updateOverdueLoans/ * checkExpiredReservations/checkExpiredPickups/activateScheduledLoans, * PrestitiController::update()/renew(), CapacityService::hasFreeCapacity(). - * Touches only rows it creates (titles ZZ_366FS_%, emails @366fs.test.local). + * It asserts only on rows it creates (titles ZZ_366FS_%, emails + * @366fs.test.local) and cleans them up, but the maintenance sweeps above are + * GLOBAL (no per-book filter) and mutate every matching row — run against an + * isolated/dedicated test DB (as CI does), never a shared one. * * Run: php tests/issue-366-full-scenario.unit.php */ @@ -242,8 +245,9 @@ function check(bool $cond, string $desc): void try { /* ---- 0. runAll() ordering: the overdue flip must run FIRST ----------- */ $maintenanceSrc = (string) file_get_contents($root . '/app/Support/MaintenanceService.php'); - $runAllStart = (int) strpos($maintenanceSrc, 'public function runAll'); - $body = substr($maintenanceSrc, $runAllStart); + $runAllStart = strpos($maintenanceSrc, 'public function runAll'); + check($runAllStart !== false, 'runAll() is present in MaintenanceService (else the ordering check below is meaningless)'); + $body = $runAllStart !== false ? substr($maintenanceSrc, $runAllStart) : ''; $posOverdue = strpos($body, '$this->updateOverdueLoans()'); $posExpired = strpos($body, '$this->checkExpiredReservations()'); $posActivate = strpos($body, '$this->activateScheduledLoans()'); diff --git a/tests/pickup-ready-copy-free-366.unit.php b/tests/pickup-ready-copy-free-366.unit.php index 2f9966b94..c02b83a02 100644 --- a/tests/pickup-ready-copy-free-366.unit.php +++ b/tests/pickup-ready-copy-free-366.unit.php @@ -28,7 +28,10 @@ * that copy is returned. * * Drives the real MaintenanceService::activateScheduledLoans() against the - * live DB. Touches only data it creates (titles ZZ_366_%, users zz366-%). + * live DB. It asserts only on rows it creates (titles ZZ_366_%, users zz366-%) + * and cleans them up, but activateScheduledLoans() is a GLOBAL sweep with no + * per-book filter — it mutates every matching row in the database. Run this + * against an isolated/dedicated test DB (as CI does), never a shared one. * * Run: php tests/pickup-ready-copy-free-366.unit.php */ @@ -59,15 +62,24 @@ function prcfenv(string $path): array } $env = prcfenv($root . '/.env'); -$dbName = $env['DB_NAME'] ?? ''; -$dbUser = $env['DB_USER'] ?? ''; -$dbPass = $env['DB_PASS'] ?? ($env['DB_PASSWORD'] ?? ''); +// Read CI env vars first (the other tests in this PR do), so the test connects +// when CI supplies credentials via the environment rather than a written .env. +$dbName = getenv('E2E_DB_NAME') ?: ($env['DB_NAME'] ?? ''); +$dbUser = getenv('E2E_DB_USER') ?: ($env['DB_USER'] ?? ''); +$dbPass = getenv('E2E_DB_PASS') ?: ($env['DB_PASS'] ?? ($env['DB_PASSWORD'] ?? '')); +$dbHost = getenv('E2E_DB_HOST') ?: ($env['DB_HOST'] ?? '127.0.0.1'); $socket = getenv('E2E_DB_SOCKET') ?: ($env['DB_SOCKET'] ?? '/opt/homebrew/var/mysql/mysql.sock'); try { $db = (is_string($socket) && $socket !== '' && file_exists($socket)) ? new mysqli(null, $dbUser, $dbPass, $dbName, 0, $socket) - : new mysqli($env['DB_HOST'] ?? '127.0.0.1', $dbUser, $dbPass, $dbName, (int) ($env['DB_PORT'] ?? 3306)); + : new mysqli($dbHost, $dbUser, $dbPass, $dbName, (int) ($env['DB_PORT'] ?? 3306)); } catch (\Throwable $e) { + // Under CI_STRICT_TESTS a missing DB must FAIL, not silently skip: a test + // that exit(0)s on no-DB gives false confidence that it ran green in CI. + if (getenv('CI_STRICT_TESTS') === '1') { + fwrite(STDERR, "DB unreachable under CI_STRICT_TESTS: " . $e->getMessage() . "\n"); + exit(1); + } echo "SKIP: database not reachable (" . $e->getMessage() . ")\n"; exit(0); }