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/.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/.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/CHANGELOG.md b/CHANGELOG.md index a621b497d..da0ff2fa3 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 @@ -40,14 +41,54 @@ 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. +### 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 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] diff --git a/app/Controllers/LoanApprovalController.php b/app/Controllers/LoanApprovalController.php index 186ca6bb6..0479fe004 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 — @@ -909,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). @@ -978,19 +988,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 +1011,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 +1166,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 +1187,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 @@ -1460,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 534c03ca3..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 @@ -824,7 +833,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(); @@ -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'); @@ -1045,6 +1064,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. @@ -1174,24 +1234,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 @@ -1602,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 @@ -1642,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. @@ -1783,8 +1854,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 ? '?' : '&'; @@ -1872,7 +1950,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 ? '?' : '&'; @@ -1903,7 +1983,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/Controllers/ReservationManager.php b/app/Controllers/ReservationManager.php index d3b5dba4f..f3d097216 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(); @@ -516,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, @@ -534,6 +590,7 @@ private function sendReservationNotification(array $reservation): bool $stmt->close(); if (!$book) { + $revertClaim(); return false; } @@ -566,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'], ]); @@ -584,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(), ]); @@ -637,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'] @@ -664,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/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/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 a9564eb38..e5741cac4 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(); @@ -343,6 +350,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 +394,68 @@ 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 (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 + // 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 @@ -639,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 @@ -817,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/app/Support/PluginManager.php b/app/Support/PluginManager.php index 29c14feba..ee0983995 100644 --- a/app/Support/PluginManager.php +++ b/app/Support/PluginManager.php @@ -15,6 +15,8 @@ class PluginManager { private const MAX_UPLOAD_BYTES = 104857600; // 100 MB + /** 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; @@ -22,6 +24,14 @@ class PluginManager private ?string $cachedEncryptionKey = null; private bool $encryptionKeyResolved = false; + /** + * @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. * @@ -177,6 +187,9 @@ public function autoRegisterBundledPlugins(): int $stmt->close(); if ($row) { + if (isset(self::$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 +1273,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 +1297,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 +1307,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)) { @@ -1310,49 +1336,47 @@ private function updatePluginFromStaging( $hasBackup = 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 = [ + '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 (!rename($stagingPath, $pluginPath)) { throw new \RuntimeException('Impossibile sostituire i file del plugin.'); } $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.'); - } + $this->applyPluginMetadataSnapshot($pluginId, $newPluginSnapshot); + $metadataUpdated = true; - if ($hasBackup && !$this->deleteDirectory($backupPath)) { + // 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 +1392,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 +1431,160 @@ 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) { + // 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); + 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 +1986,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 +2041,15 @@ public function loadActivePlugins(): void // current request or the following five minutes. $instances = []; foreach ($activePlugins as $plugin) { + $pluginId = (int) $plugin['id']; + 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. + 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 +2093,327 @@ 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) { + // 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; + } + + 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(), + ]); + // 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); + 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) + ) { + // 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; + 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); + self::$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 + N INSERTs must be atomic: a mid-loop INSERT failure must not + // 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(); + // 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 ' . $savepoint); + } else { + $this->db->begin_transaction(); + } + + 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(); + + 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(); + } + + if ($inTransaction) { + $this->db->query('RELEASE SAVEPOINT ' . $savepoint); + } else { + $this->db->commit(); + } + } catch (\Throwable $e) { + if ($inTransaction) { + $this->db->query('ROLLBACK TO SAVEPOINT ' . $savepoint); + } 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 + { + // 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 ' . $probe) === false) { + return 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; + } + } + /** * Invalidate the cross-request plugin caches. Must be called by every * plugin lifecycle mutation (install/activate/deactivate/uninstall). 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() { diff --git a/bin/build-release.sh b/bin/build-release.sh index 88380cced..57f744388 100644 --- a/bin/build-release.sh +++ b/bin/build-release.sh @@ -243,6 +243,30 @@ verify_package_contents() { has_errors=true fi + # PHP session files may contain authenticated user data and CSRF tokens. + # 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" ] || [ -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" + 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 \ + ! -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 + log_error "Package contains runtime session data: ${unexpected_session#"$package_dir/"}" + has_errors=true + fi + fi + # Files that MUST be in the package local required_files=( "public/index.php" 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/scripts/ci-verify-release.sh b/scripts/ci-verify-release.sh index 03f103457..48286132a 100755 --- a/scripts/ci-verify-release.sh +++ b/scripts/ci-verify-release.sh @@ -82,6 +82,27 @@ 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 +# 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" ] || [ -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 + 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 ! -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 +if [ -n "$session_scan" ]; 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/storage/sessions/.gitkeep b/storage/sessions/.gitkeep new file mode 100644 index 000000000..e69de29bb diff --git a/tests/audit-fixes-p2-p4.unit.php b/tests/audit-fixes-p2-p4.unit.php new file mode 100644 index 000000000..1c832880c --- /dev/null +++ b/tests/audit-fixes-p2-p4.unit.php @@ -0,0 +1,472 @@ +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); diff --git a/tests/code-quality.spec.js b/tests/code-quality.spec.js index 8e2faf91c..a5ba48cf8 100644 --- a/tests/code-quality.spec.js +++ b/tests/code-quality.spec.js @@ -209,6 +209,29 @@ 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'); + // 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"'); + 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"'); + // 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/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/issue-366-full-scenario.unit.php b/tests/issue-366-full-scenario.unit.php new file mode 100644 index 000000000..cc039b8e0 --- /dev/null +++ b/tests/issue-366-full-scenario.unit.php @@ -0,0 +1,459 @@ +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 = 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()'); + 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); 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); 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..c02b83a02 --- /dev/null +++ b/tests/pickup-ready-copy-free-366.unit.php @@ -0,0 +1,265 @@ + 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. 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 + */ + +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'); +// 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($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); +} +$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); diff --git a/tests/plugin-manager.unit.php b/tests/plugin-manager.unit.php index 5cf44a8c3..a15885b9d 100644 --- a/tests/plugin-manager.unit.php +++ b/tests/plugin-manager.unit.php @@ -45,6 +45,38 @@ $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'); +$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, '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( + $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"; diff --git a/tests/plugin-zip-update-all-bundled.integration.php b/tests/plugin-zip-update-all-bundled.integration.php index ddc8e0e37..90590a8a5 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,10 @@ function pzua_zip_plugin(string $pluginDir, string $slug, string $bumpedVersion) foreach (glob($pluginsDir . '/.' . $slug . '.staging-*') ?: [] as $stray) { pzua_rmdir($stray); } + // 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 23e80d1d8..eab212d40 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,149 @@ 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 +{ + // 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 => ['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]); + fclose($pipes[1]); + $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" . $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'); $socket = getenv('E2E_DB_SOCKET') ?: ($env['DB_SOCKET'] ?? ''); $user = getenv('E2E_DB_USER') ?: ($env['DB_USER'] ?? ''); @@ -96,13 +237,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 . '/' . \App\Support\PluginManager::PENDING_UPDATE_PREFIX; +$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 +259,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 +273,116 @@ 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 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(); + $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 (!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.'); + } + + 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) { + // 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); + } if (is_string($zipV1)) { @unlink($zipV1); } if (is_string($zipV2)) { @unlink($zipV2); } + if (is_string($zipV3)) { + @unlink($zipV3); + } $db->close(); }