chore: Release v0.1.5 #12
Conversation
# Changelog - Security Audit and Fixes ## Backend & UI - Fixed hardcoded Italian notification messages in backend dropdown - Added English translations for all in-app notifications (user registration, reservations, contact messages, overdue loans) - Fixed missing English translations for CSV import interface - Corrected CSV author separator to match export format (semicolon instead of pipe) ## Email & Activation - Restored email functionality with universal domain validation - Removed hardcoded domain whitelist causing email delivery failures - Fixed installer email address configuration - Corrected environment variable handling for canonical URL in activation links ## Installer - Fixed installer deletion form action preventing 404 errors - Corrected installer deletion button path - Updated installer instructions to document both CSV separators (semicolon and pipe) ## Security - Implemented RFC-compliant hostname validation with regex pattern - Replaced brittle whitelist system with universal validation - Added emergency production domain support ## Configuration - Fixed APP_CANONICAL_URL environment variable handling - Corrected .env configuration for email link generation - Improved environment variable parsing and validation --- ## Upgrade Notice If you're upgrading from a previous version and experiencing issues with email activation links or redirects: 1. Navigate to: /admin/maintenance/integrity-report 2. The system will detect the missing APP_CANONICAL_URL configuration 3. Click "Apply Fix" to automatically configure your canonical URL 4. Alternatively, manually add to your .env file: APP_CANONICAL_URL=https://yourdomain.com
WalkthroughAdds canonical URL configuration, validation, and an admin endpoint to set APP_CANONICAL_URL; centralizes URL handling via HtmlHelper; hardens LibriController (SSRF/DoS/path/mime protections, locks, streaming CSVs); adds disk-backed rate limiting for scrapers; introduces IDOR checks in UsersController; updates installer, views, and multiple controllers for safer URL/translation handling. Changes
Sequence Diagram(s)sequenceDiagram
participant Admin as Admin User
participant UI as Integrity Report UI
participant Server as MaintenanceController
participant Env as .env File
Admin->>UI: Click "Apply Fix" for canonical URL
UI->>Server: POST /admin/maintenance/apply-config-fix {issue_type, fix_value}
activate Server
Server->>Server: Validate CSRF and issue_type
Server->>Server: Validate fix_value is a URL
Server->>Env: Read .env
Server->>Env: Update or append APP_CANONICAL_URL=<fix_value>
Env-->>Server: Write success/failure
Server-->>UI: {success:true/false, message}
deactivate Server
UI->>UI: Show result and reload on success
sequenceDiagram
participant Client as Client
participant Scrape as ScrapeController
participant Rate as RateLimiter (disk)
participant API as External API
Client->>Scrape: Request fallback data
Scrape->>Rate: checkRateLimit(apiName, max)
Rate->>Rate: Load/prune timestamps (storage/rate_limits/*.json) with file lock
alt within limit
Rate-->>Scrape: allowed
Scrape->>API: Fetch external data
API-->>Scrape: Response
Scrape-->>Client: Data
else limit exceeded
Rate-->>Scrape: denied
Scrape-->>Client: Error (rate limited)
end
sequenceDiagram
participant Caller as API Client
participant Route as /api/libri/{id}/increase-copies
participant Libri as LibriController
participant DB as Database
participant Integrity as DataIntegrity
Caller->>Route: POST quantity=N
activate Libri
Libri->>DB: SELECT numero_inventario FROM libri WHERE id=...
Libri->>DB: UPDATE libri.copie_totali += N
loop for each new copy
Libri->>DB: CopyRepository::create(bookId, numeroInventario, status, note)
end
Libri->>Integrity: recalculateBookAvailability(bookId)
Integrity->>DB: Recompute copie_disponibili and UPDATE libri
Libri->>DB: SELECT copie_disponibili
Libri-->>Caller: {copie_disponibili: value}
deactivate Libri
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas requiring extra attention:
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
app/Views/frontend/catalog.php (1)
34-34: Inconsistent URL construction bypasses validation.Line 34 still uses manual URL construction with
$_SERVER['HTTPS']and$_SERVER['HTTP_HOST'], while lines 21-22 useHtmlHelper::getBaseUrl(). This creates inconsistency and bypasses the host validation that HtmlHelper provides.Apply this diff to use consistent URL handling:
"url" => $seoCanonical, "isPartOf" => [ "@type" => "Library", "name" => __("Biblioteca Digitale"), - "url" => (isset($_SERVER['HTTPS']) ? 'https' : 'http') . '://' . $_SERVER['HTTP_HOST'] . '/' + "url" => HtmlHelper::getBaseUrl() . '/' ], "potentialAction" => [app/Views/editori/crea_editore.php (1)
147-209: Unescaped translations in JS (Swal/alert) break scripts and weaken safetyUsing
<?= __("...") ?>inside single-quoted JS strings leads to invalid JS when translations contain'(many of these do), and increases injection surface. This code will currently fail to parse in Italian.Use
json_encodeto emit safe JS string literals and also switch the remaining JS-side__()to server-side translations for consistency:- if (window.Swal) { - Swal.fire({ - icon: 'error', - title: '<?= __("Campo Obbligatorio") ?>', - text: '<?= __("Il nome dell\'editore è obbligatorio.") ?>' - }); - } else { - alert('<?= __("Il nome dell\'editore è obbligatorio.") ?>'); - } + if (window.Swal) { + Swal.fire({ + icon: 'error', + title: <?= json_encode(__('Campo Obbligatorio')) ?>, + text: <?= json_encode(__('Il nome dell\'editore è obbligatorio.')) ?> + }); + } else { + alert(<?= json_encode(__('Il nome dell\'editore è obbligatorio.')) ?>); + } - if (sitoWeb && !isValidURL(sitoWeb)) { + if (sitoWeb && !isValidURL(sitoWeb)) { if (window.Swal) { Swal.fire({ icon: 'error', - title: '<?= __("URL Non Valido") ?>', - text: '<?= __("Il sito web deve essere un URL valido (es. https://www.esempio.com).") ?>' + title: <?= json_encode(__('URL Non Valido')) ?>, + text: <?= json_encode(__('Il sito web deve essere un URL valido (es. https://www.esempio.com).')) ?> }); } else { - alert('<?= __("Il sito web deve essere un URL valido.") ?>'); + alert(<?= json_encode(__('Il sito web deve essere un URL valido.')) ?>); } return; } - if (email && !isValidEmail(email)) { + if (email && !isValidEmail(email)) { if (window.Swal) { Swal.fire({ icon: 'error', - title: '<?= __("Email Non Valida") ?>', - text: '<?= __("L\'indirizzo email deve essere valido.") ?>' + title: <?= json_encode(__('Email Non Valida')) ?>, + text: <?= json_encode(__('L\'indirizzo email deve essere valido.')) ?> }); } else { - alert('<?= __("L\'indirizzo email deve essere valido.") ?>'); + alert(<?= json_encode(__('L\'indirizzo email deve essere valido.')) ?>); } return; } - if (window.Swal) { - const result = await Swal.fire({ - title: '<?= __("Conferma Salvataggio") ?>', - text: __('Sei sicuro di voler salvare l\'editore "%s"?').replace('%s', nome), + if (window.Swal) { + const result = await Swal.fire({ + title: <?= json_encode(__('Conferma Salvataggio')) ?>, + text: <?= json_encode(__('Sei sicuro di voler salvare l\'editore "%s"?')) ?>.replace('%s', nome), icon: 'question', showCancelButton: true, - confirmButtonText: '<?= __("Sì, Salva") ?>', - cancelButtonText: '<?= __("Annulla") ?>', + confirmButtonText: <?= json_encode(__('Sì, Salva')) ?>, + cancelButtonText: <?= json_encode(__('Annulla')) ?>, reverseButtons: true }); if (result.isConfirmed) { Swal.fire({ - title: '<?= __("Salvataggio in corso...") ?>', - text: '<?= __("Attendere prego") ?>', + title: <?= json_encode(__('Salvataggio in corso...')) ?>, + text: <?= json_encode(__('Attendere prego')) ?>, allowOutsideClick: false, showConfirmButton: false, willOpen: () => { Swal.showLoading(); } }); form.submit(); }app/Views/autori/crea_autore.php (1)
139-188: Unescaped translations in author form JS cause syntax errors and should usejson_encodeThe Swal/alert strings embed
<?= __("...") ?>inside single-quoted JS literals. With Italian texts likeIl nome dell'autore è obbligatorio.andLa data di nascita deve essere precedente..., this produces invalid JS and can’t work reliably across locales.Use
json_encodeto output safe JS strings and align confirmation text with server-side translations:- if (!nome) { - if (window.Swal) { - Swal.fire({ - icon: 'error', - title: '<?= __("Campo Obbligatorio") ?>', - text: '<?= __("Il nome dell\'autore è obbligatorio.") ?>' - }); - } else { - alert('<?= __("Il nome dell\'autore è obbligatorio.") ?>'); - } + if (!nome) { + if (window.Swal) { + Swal.fire({ + icon: 'error', + title: <?= json_encode(__('Campo Obbligatorio')) ?>, + text: <?= json_encode(__('Il nome dell\'autore è obbligatorio.')) ?> + }); + } else { + alert(<?= json_encode(__('Il nome dell\'autore è obbligatorio.')) ?>); + } return; } - if (dataNascita && dataMorte) { - if (new Date(dataNascita) >= new Date(dataMorte)) { - if (window.Swal) { - Swal.fire({ - icon: 'error', - title: '<?= __("Date Non Valide") ?>', - text: '<?= __("La data di nascita deve essere precedente alla data di morte.") ?>' - }); - } else { - alert('<?= __("La data di nascita deve essere precedente alla data di morte.") ?>'); - } + if (dataNascita && dataMorte) { + if (new Date(dataNascita) >= new Date(dataMorte)) { + if (window.Swal) { + Swal.fire({ + icon: 'error', + title: <?= json_encode(__('Date Non Valide')) ?>, + text: <?= json_encode(__('La data di nascita deve essere precedente alla data di morte.')) ?> + }); + } else { + alert(<?= json_encode(__('La data di nascita deve essere precedente alla data di morte.')) ?>); + } return; } } - if (window.Swal) { - const result = await Swal.fire({ - title: '<?= __("Conferma Salvataggio") ?>', - text: __('Sei sicuro di voler salvare l\'autore "%s"?').replace('%s', nome), + if (window.Swal) { + const result = await Swal.fire({ + title: <?= json_encode(__('Conferma Salvataggio')) ?>, + text: <?= json_encode(__('Sei sicuro di voler salvare l\'autore "%s"?')) ?>.replace('%s', nome), icon: 'question', showCancelButton: true, - confirmButtonText: '<?= __("Sì, Salva") ?>', - cancelButtonText: '<?= __("Annulla") ?>', + confirmButtonText: <?= json_encode(__('Sì, Salva')) ?>, + cancelButtonText: <?= json_encode(__('Annulla')) ?>, reverseButtons: true }); if (result.isConfirmed) { Swal.fire({ - title: '<?= __("Salvataggio in corso...") ?>', - text: '<?= __("Attendere prego") ?>', + title: <?= json_encode(__('Salvataggio in corso...')) ?>, + text: <?= json_encode(__('Attendere prego')) ?>, allowOutsideClick: false, showConfirmButton: false, willOpen: () => { Swal.showLoading(); } }); form.submit(); }app/Controllers/MaintenanceController.php (1)
6-9: Fix Exception handling by importing or fully-qualifying across all catch blocksThe issue is confirmed. The file has no
use Exception;import and is in theApp\Controllersnamespace. This means the three unqualifiedcatch (Exception $e)blocks at lines 55, 84, and 132 targetApp\Controllers\Exceptioninstead of the global\Exception, preventing proper error handling.The newer
applyConfigFixmethod (line 184) already correctly usescatch (\Exception $e), making the inconsistency clear.Recommend either:
- Adding
use Exception;to the imports (lines 6-9), or- Updating all three catch blocks to use
\Exceptionfor consistency withapplyConfigFixLocations requiring fixes:
- Line 55:
catch (Exception $e)infixIntegrityIssues- Line 84:
catch (Exception $e)inrecalculateAvailability- Line 132:
catch (Exception $e)inperformMaintenanceapp/Views/frontend/book-detail.php (1)
2255-2278: Fix JS fallback alerts to use the JS__()helper instead of raw PHP translationsIn the non-SweetAlert fallback at lines 2272, 2274, and 2277 you embed translated strings directly into JS single-quoted literals using PHP interpolation:
alert('<?= __("Prenotazione effettuata per ") ?>' + date); alert('<?= __("Errore: ") ?>' + (result.message || '<?= __("Impossibile creare la prenotazione") ?>')); alert('<?= __("Errore nella prenotazione") ?>');The JS
__()helper is already defined globally (lines 1937–1940) and used throughout the SweetAlert path (lines 2023+). Use it consistently in the fallback path instead:- alert('<?= __("Prenotazione effettuata per ") ?>' + date); + alert(__('Prenotazione effettuata per ') + date); } else { - alert('<?= __("Errore: ") ?>' + (result.message || '<?= __("Impossibile creare la prenotazione") ?>')); + alert(__('Errore: ') + (result.message || __('Impossibile creare la prenotazione'))); } } catch(_) { - alert('<?= __("Errore nella prenotazione") ?>'); + alert(__('Impossibile creare la prenotazione')); }This avoids JS string-escaping pitfalls and aligns with the rest of the codebase.
♻️ Duplicate comments (1)
app/Controllers/ScrapeController.php (1)
306-310: Open Library rate-limit guard mirrors Google Books logicThis block mirrors the Google Books guard and looks consistent. Any refactor to centralize or configure the per-API limits (mentioned in the previous comment) would apply here as well to keep things DRY.
🧹 Nitpick comments (20)
app/Controllers/ScrapeController.php (2)
168-172: Good placement of Google Books rate-limit guard; consider centralizing the limit valueThe early
checkRateLimit('google_books', 10)guard is correctly placed before any HTTP call and logs + bails out cleanly when exceeded. SincecheckRateLimitalready has a default of10, you might simplify by either:
- Relying on the default (
$this->checkRateLimit('google_books')), or- Moving the per-API limits into a small config/constant map to avoid magic numbers in multiple locations.
386-434: HardencheckRateLimitagainst malformed JSON and I/O failures, and align with existing rate-limit patternsThe overall approach (per‑API JSON file, 60‑second sliding window, write with
LOCK_EX) is reasonable, but a couple of robustness tweaks would help:
- Defensive structure checks for decoded JSON
If the JSON file is empty/corrupted or doesn’t contain a
callskey,$data['calls']will be undefined andarray_filterwill operate on a non‑array, which can explode at runtime in PHP 8+. Normal operation will always write the expected shape, but hardening against edge cases is cheap.You can ensure the structure before filtering and also merge with defaults instead of overwriting:
- // Load existing rate limit data - $data = ['calls' => [], 'last_cleanup' => $now]; + // Load existing rate limit data (with sane defaults) + $data = ['calls' => [], 'last_cleanup' => $now]; if (file_exists($rateLimitFile)) { $json = file_get_contents($rateLimitFile); if ($json !== false) { $decoded = json_decode($json, true); if (is_array($decoded)) { - $data = $decoded; + // Merge, so missing keys in file fall back to defaults + $data = array_merge($data, $decoded); } } } - // Remove calls older than 60 seconds - $data['calls'] = array_filter($data['calls'], fn($timestamp) => ($now - $timestamp) < 60); + // Ensure 'calls' is always an array, even if JSON was malformed + if (!isset($data['calls']) || !is_array($data['calls'])) { + $data['calls'] = []; + } + + // Remove calls older than 60 seconds + $data['calls'] = array_filter( + $data['calls'], + static fn($timestamp): bool => is_int($timestamp) && ($now - $timestamp) < 60 + ); @@ - // Save with lock - file_put_contents($rateLimitFile, json_encode($data), LOCK_EX); + // Save with lock; log on failure so silent degradation is visible + if (file_put_contents($rateLimitFile, json_encode($data), LOCK_EX) === false) { + error_log("[ScrapeController] Failed to persist rate limit file for $apiName at $rateLimitFile"); + }
- Optional: reduce lost updates under concurrency
Right now, two PHP workers can both read the file before either writes and each decide the limit is not exceeded, slightly under‑counting traffic in a high‑concurrency spike. For this use case (preventing bans) that’s probably acceptable, but if you want stricter guarantees you could refactor to use
fopen+flockfor the full read‑modify‑write cycle.
- Optional: unify rate‑limiting strategies
There’s already a DB‑backed
checkRateLimitinapp/Controllers/PasswordController.phpfor per‑identifier actions. Long‑term, you might consider a small shared service or helper that abstracts rate‑limit storage (DB vs filesystem) so controllers don’t grow ad‑hoc implementations with slightly different semantics.installer/steps/step6.php (1)
34-63: Direct DB persistence for email settings is correct; consider reusingsaveSettingThe new flow (load env, get PDO, bulk
INSERT ... ON DUPLICATE KEY UPDATE) correctly fixes the installer bootstrap issue and uses prepared statements safely. You could optionally delegate toInstaller::saveSetting()in a loop to avoid duplicating the insert logic / key names in multiple places, but the current implementation is functionally sound.installer/classes/Installer.php (1)
927-964:updateEnvVariableimplementation is correct; consider stripping newlines from valuesThe read/replace/append logic with
preg_quoteand a multiline^KEY=pattern is sound for typical.envlayouts. To make it a bit more robust against unexpected input and prevent accidental line breaking in.env, you might defensively strip CR/LF from$valuebefore building$replacement:- public function updateEnvVariable(string $key, string $value): bool + public function updateEnvVariable(string $key, string $value): bool { $envPath = $this->baseDir . '/.env'; @@ - // Escape special regex characters in key + // Normalize value to a single line + $value = str_replace(["\r", "\n"], '', $value); + + // Escape special regex characters in key $escapedKey = preg_quote($key, '/');app/Controllers/CsvImportController.php (1)
460-465: Updated authors separator logic aligns import with exportSwitching to
;as the primary authors separator with a|fallback keeps imports compatible with older CSVs while matching the current export format. Since this behavior is now more flexible than the comment ingenerateExampleCsv()(which only mentions|), you may optionally update that docblock to mention both;and|to avoid confusion for operators preparing files manually.app/Controllers/UsersController.php (1)
469-478: Stronger delete authorization is good; drop unused$currentUserIdRequiring
adminfor all deletions (and explicitly disallowing staff, including self‑deletion) is a sensible hardening for preserving audit trails.The
$currentUserIdvariable initialized on Line 470 is not used and is flagged by PHPMD. You can safely remove it:- $currentUserRole = (string)($_SESSION['user']['tipo_utente'] ?? ''); - $currentUserId = (int)($_SESSION['user']['id'] ?? 0); + $currentUserRole = (string)($_SESSION['user']['tipo_utente'] ?? '');app/Controllers/PasswordController.php (1)
188-210: Canonical URL–first base URL strategy is solid; consider constraining schemesPreferring
APP_CANONICAL_URL(when set) and otherwise falling back to a regex-validatedHTTP_HOST(with a localhost fallback) is a strong defense against Host header injection and gives predictable URLs for emails.One small hardening you might add is to restrict
APP_CANONICAL_URLtohttp/httpsschemes before accepting it, sinceFILTER_VALIDATE_URLwill also consider non-HTTP schemes valid. For example:- if ($canonicalUrl !== false) { - $canonicalUrl = trim((string)$canonicalUrl); - if ($canonicalUrl !== '' && filter_var($canonicalUrl, FILTER_VALIDATE_URL)) { - return rtrim($canonicalUrl, '/'); - } - } + if ($canonicalUrl !== false) { + $canonicalUrl = trim((string)$canonicalUrl); + if ($canonicalUrl !== '' && filter_var($canonicalUrl, FILTER_VALIDATE_URL)) { + $scheme = parse_url($canonicalUrl, PHP_URL_SCHEME) ?: ''; + if (in_array(strtolower($scheme), ['http', 'https'], true)) { + return rtrim($canonicalUrl, '/'); + } + } + }This keeps behavior unchanged for normal configs while avoiding odd schemes in case of misconfiguration.
app/Views/auth/register.php (1)
332-347: Escape translated strings when embedding into JavaScriptEmbedding
<?= __("...") ?>directly inside single-quoted JS strings will break if a translation ever contains', backslashes, or newlines, and weakens JS/XSS safety.Prefer emitting a JS string literal via
json_encode, e.g.:- if (password.value !== confirmPassword.value) { - e.preventDefault(); - alert('<?= __("Le password non coincidono!") ?>'); + if (password.value !== confirmPassword.value) { + e.preventDefault(); + alert(<?= json_encode(__('Le password non coincidono!')) ?>); confirmPassword.focus(); return false; } - if (password.value.length < 8) { - e.preventDefault(); - alert('<?= __("La password deve essere lunga almeno 8 caratteri!") ?>'); + if (password.value.length < 8) { + e.preventDefault(); + alert(<?= json_encode(__('La password deve essere lunga almeno 8 caratteri!')) ?>); password.focus(); return false; }app/Support/EmailService.php (1)
472-496: Canonical URL handling looks solid; consider restricting schemes to http/httpsThe new
getBaseUrl()logic correctly:
- Prefers a non-empty, valid APP_CANONICAL_URL.
- Otherwise falls back to a validated
HTTP_HOST, with a regex that mitigates Host header injection and a localhost safety fallback.To tighten this further, you may want to explicitly restrict APP_CANONICAL_URL to
http/httpsschemes instead of any URL accepted byFILTER_VALIDATE_URL, e.g.:- $canonicalUrl = getenv('APP_CANONICAL_URL'); - if ($canonicalUrl !== false) { - $canonicalUrl = trim((string)$canonicalUrl); - if ($canonicalUrl !== '' && filter_var($canonicalUrl, FILTER_VALIDATE_URL)) { - return rtrim($canonicalUrl, '/'); - } - } + $canonicalUrl = getenv('APP_CANONICAL_URL'); + if ($canonicalUrl !== false) { + $canonicalUrl = trim((string) $canonicalUrl); + if ( + $canonicalUrl !== '' && + preg_match('#^https?://#i', $canonicalUrl) && + filter_var($canonicalUrl, FILTER_VALIDATE_URL) + ) { + return rtrim($canonicalUrl, '/'); + } + }This keeps behavior the same for intended values while preventing accidental non-web schemes from being used in email links.
app/Views/frontend/book-detail.php (1)
2-2: Good move centralizing canonical/base URLs; align remaining schema usageUsing
HtmlHelper::getCurrentUrl()andHtmlHelper::getBaseUrl()for canonical URL, OG image, breadcrumbs, and SEO variables is a solid hardening against Host header issues and keeps URL computation consistent across views.For full consistency and to avoid re‑introducing manual host handling in one corner, consider also switching the
Organizationschema"url"field (Line 205) to useHtmlHelper::getBaseUrl()instead of rebuilding from$_SERVER['HTTPS']/$_SERVER['HTTP_HOST'].Also applies to: 84-89, 100-113, 1894-1899
installer/steps/step5.php (1)
12-37: Canonical URL handling looks solid; consider slightly hardening.envparsingThe new
canonical_urlfield is correctly sanitized, validated only when non‑empty, and written back to.envviaupdateEnvVariable('APP_CANONICAL_URL', …)with trailing slashes trimmed. The default value logic that pre‑loads from.envand repopulates POSTed values is also sound and keeps the UX smooth.Minor optional tweak: when extracting
APP_CANONICAL_URLfrom.envwithpreg_match('/^APP_CANONICAL_URL=(.*)$/m', $envContent, $matches)you may want to strip surrounding quotes or inline comments (e.g.
# note) before prefilling the input, so the installer shows a clean bare URL even if.envhas been hand‑edited. Not a blocker for this PR.Also applies to: 67-76, 96-102
app/Support/DataIntegrity.php (1)
282-311: Canonical URL integrity checks are helpful; consider reusing shared host validationThe new step 8 in
verifyDataConsistency()gives administrators clear diagnostics and fix suggestions forAPP_CANONICAL_URL(missing/empty/invalid), anddetectCurrentCanonicalUrl()does a good job inferring scheme/host/port across typical proxy and non‑proxy setups.For consistency and defense‑in‑depth, you might want to align
detectCurrentCanonicalUrl()’s host handling with the hostname validation you already use elsewhere (e.g. inNotificationService::getBaseUrl()or a centralHtmlHelper). That would avoid suggesting a malformed host infix_suggestionif the environment ever exposes an oddHTTP_HOST/X_FORWARDED_HOSTvalue, and it keeps all canonical URL logic following the same rules.Also applies to: 315-360
app/Controllers/MaintenanceController.php (1)
141-215: applyConfigFix endpoint is well-scoped; consider reusing shared env update logicThe new
applyConfigFix()endpoint correctly:
- Validates CSRF via
X-CSRF-Token.- Restricts
issue_typeto the three canonical URL issues your integrity report emits.- Validates
fix_valueas a URL before touching.env.- Replaces or appends the
APP_CANONICAL_URL=line and returns localized JSON responses.Two small points to consider:
- You currently ignore
issue_typebeyond validation; if different issue types ever need distinct behavior (e.g. clearing vs setting values), you’ll need a switch here.- The
.envupdate logic largely duplicatesInstaller::updateEnvVariable(). If practical, extracting a shared helper for env mutations would reduce the chance of future divergence between installer and maintenance flows.Functionally this looks good as-is.
app/Support/NotificationService.php (2)
731-754: Base URL resolution is robust; consider centralizing with other helpersThe updated
getBaseUrl()correctly:
- Prefers a validated
APP_CANONICAL_URLfrom the environment (ideal for production + CLI).- Falls back to
HTTP_HOSTwith a reasonably strict hostname+port regex.- Uses
localhostas a safe final fallback when the host format is invalid.This substantially improves URL safety compared to a raw
$_SERVER['HTTP_HOST']concat.Given you now have similar logic in other places (e.g. HtmlHelper, DataIntegrity), you might eventually want to centralize base URL resolution in a single helper to avoid drift and keep all components using identical rules.
944-952: In‑app notification titles/messages now localized; extend pattern over timeWrapping the admin in‑app notification titles and messages in
__()/sprintf()for:
- new contact messages,
- new user registrations,
- new reservations,
- overdue loans,
brings these UI elements into the translation system, matching the broader i18n work in this PR.
As a follow‑up (not required for this release), you could gradually migrate remaining hard‑coded Italian strings in this service (e.g. some loan warning texts) to use the same pattern for full language coverage.
Also applies to: 958-966, 972-980, 986-994
app/Controllers/LibriController.php (5)
279-285: Copy count bounds instore()Clamping
copie_totaliinto[1, 9999]prevents pathological values and keeps later loops safe. If you ever need symmetric behavior inupdate(), consider applying similar bounds there for consistency, but it is not strictly required.
318-330: Advisory locking around duplicate check instore()is mostly correct (minor double‑release)The advisory lock key derivation and use of
GET_LOCK(..., 10)make duplicate check + insert effectively atomic for identical identifiers, and the 503 JSON response on timeout is clear. Note that you callRELEASE_LOCKboth in the duplicate path and again in thefinallyblock; the latter is sufficient and the extra release just adds an unnecessary query (MySQL will just return0). You could safely drop the innerRELEASE_LOCKcalls.Also applies to: 332-355, 357-401, 560-565
761-781: Advisory locking inupdate()mirrorsstore()(same minor double‑release)The update‑time duplicate protection (excluding current
id) underGET_LOCKis consistent with creation. As instore(), you can rely solely on thefinallyblock to release the lock; the explicitRELEASE_LOCKbefore early returns is redundant but harmless.Also applies to: 783-817, 1004-1009
1856-1862: CSV export streaming viaphp://tempis a solid scalability and security improvementSwitching to
php://temp/maxmemory:...with row‑by‑row writes, UTF‑8 BOM, proper escaping around the semicolon delimiter, periodicgc_collect_cycles(), and hardened response headers (nosniff,DENY, strict CSP) significantly improves memory behavior and mitigates client‑side content‑sniffing risks. Note thatmaxmemorylimits in‑memory buffering, not total CSV size; the comment could be clarified if you want to avoid confusion.Also applies to: 1888-1889, 1891-1935, 1941-1950
1953-1989: Cover sync and scraping flow are reasonable, but consider reusinghandleCoverUrl()
syncCovers()+scrapeBookCover()add a rate‑limited bulk cover sync path with retries, which is good for operational robustness. Currently, the scrapedimageURL is written directly tocopertina_urlwithout going throughhandleCoverUrl()orisUrlAllowed(). If you ever change scraping sources or want consistent validation/canonicalization (e.g., auto‑local download for certain hosts), consider funnelling this path throughhandleCoverUrl()as well.Also applies to: 2007-2014, 2030-2088
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (34)
.env.example(2 hunks).gitignore(2 hunks)app/Controllers/CsvImportController.php(1 hunks)app/Controllers/FrontendController.php(1 hunks)app/Controllers/LibriController.php(18 hunks)app/Controllers/MaintenanceController.php(4 hunks)app/Controllers/PasswordController.php(8 hunks)app/Controllers/ReservationManager.php(1 hunks)app/Controllers/ScrapeController.php(3 hunks)app/Controllers/UsersController.php(2 hunks)app/Routes/web.php(3 hunks)app/Support/DataIntegrity.php(1 hunks)app/Support/EmailService.php(1 hunks)app/Support/HtmlHelper.php(1 hunks)app/Support/NotificationService.php(5 hunks)app/Views/admin/csv_import.php(1 hunks)app/Views/admin/integrity_report.php(6 hunks)app/Views/auth/register.php(1 hunks)app/Views/autori/crea_autore.php(4 hunks)app/Views/editori/crea_editore.php(5 hunks)app/Views/frontend/book-detail.php(4 hunks)app/Views/frontend/catalog-grid.php(2 hunks)app/Views/frontend/catalog.php(2 hunks)app/Views/frontend/cookies-page.php(1 hunks)app/Views/frontend/home-books-grid.php(2 hunks)app/Views/frontend/home-sections/genre_carousel.php(2 hunks)app/Views/frontend/layout.php(2 hunks)app/Views/frontend/privacy-page.php(1 hunks)app/Views/libri/partials/book_form.php(1 hunks)installer/classes/Installer.php(1 hunks)installer/steps/step5.php(4 hunks)installer/steps/step6.php(1 hunks)installer/steps/step7.php(3 hunks)locale/en_US.json(18 hunks)
🧰 Additional context used
🧬 Code graph analysis (9)
installer/steps/step6.php (1)
installer/classes/Installer.php (1)
getDatabaseConnection(157-207)
app/Views/frontend/home-sections/genre_carousel.php (4)
app/Support/HtmlHelper.php (2)
HtmlHelper(6-267)getBaseUrl(208-234)app/Controllers/ReservationManager.php (1)
getBaseUrl(187-210)app/Support/EmailService.php (1)
getBaseUrl(472-496)app/Support/NotificationService.php (1)
getBaseUrl(731-754)
app/Views/frontend/home-books-grid.php (1)
app/Support/HtmlHelper.php (2)
HtmlHelper(6-267)getBaseUrl(208-234)
app/Views/frontend/catalog.php (1)
app/Support/HtmlHelper.php (2)
HtmlHelper(6-267)getBaseUrl(208-234)
app/Views/frontend/cookies-page.php (1)
app/Support/HtmlHelper.php (2)
HtmlHelper(6-267)sanitizeHtml(69-200)
app/Controllers/ScrapeController.php (1)
app/Controllers/PasswordController.php (1)
checkRateLimit(216-254)
app/Views/frontend/privacy-page.php (1)
app/Support/HtmlHelper.php (2)
HtmlHelper(6-267)sanitizeHtml(69-200)
app/Views/frontend/layout.php (1)
app/Support/HtmlHelper.php (3)
HtmlHelper(6-267)absoluteUrl(258-266)getCurrentUrl(241-250)
app/Views/frontend/catalog-grid.php (4)
app/Support/HtmlHelper.php (2)
HtmlHelper(6-267)getBaseUrl(208-234)app/Controllers/ReservationManager.php (1)
getBaseUrl(187-210)app/Support/EmailService.php (1)
getBaseUrl(472-496)app/Support/NotificationService.php (1)
getBaseUrl(731-754)
🪛 PHPMD (2.15.0)
app/Controllers/LibriController.php
802-802: Avoid unused local variables such as '$editori'. (undefined)
(UnusedLocalVariable)
803-803: Avoid unused local variables such as '$autori'. (undefined)
(UnusedLocalVariable)
806-806: Avoid unused local variables such as '$scaffali'. (undefined)
(UnusedLocalVariable)
807-807: Avoid unused local variables such as '$generi'. (undefined)
(UnusedLocalVariable)
808-808: Avoid unused local variables such as '$sottogeneri'. (undefined)
(UnusedLocalVariable)
809-809: Avoid unused local variables such as '$error_message'. (undefined)
(UnusedLocalVariable)
811-811: Avoid unused local variables such as '$libroView'. (undefined)
(UnusedLocalVariable)
812-812: Avoid unused local variables such as '$content'. (undefined)
(UnusedLocalVariable)
1116-1116: Avoid unused parameters such as '$resource'. (undefined)
(UnusedFormalParameter)
1116-1116: Avoid unused parameters such as '$download_size'. (undefined)
(UnusedFormalParameter)
1116-1116: Avoid unused parameters such as '$upload_size'. (undefined)
(UnusedFormalParameter)
1116-1116: Avoid unused parameters such as '$uploaded'. (undefined)
(UnusedFormalParameter)
app/Controllers/UsersController.php
469-469: Avoid unused local variables such as '$currentUserId'. (undefined)
(UnusedLocalVariable)
🔇 Additional comments (37)
.gitignore (1)
37-38: New ignore patterns look goodIgnoring temporary debug/test scripts and fix.md is consistent with keeping maintenance artifacts out of version control.
Also applies to: 176-176
app/Controllers/FrontendController.php (1)
128-147: ParameterizedINclause for genre IDs is a solid hardeningUsing
array_unique(array_map('intval', ...)), dynamic?placeholders, andbind_paramwith ani...itype string correctly eliminates interpolation risk for the genre carousel query while preserving behavior. The prepare‑failureerror_log+continuefallback is also reasonable here.app/Controllers/CsvImportController.php (1)
284-305: CSV injection regex fix is correct and saferEscaping the hyphen in the character class (
/^[=+\-@].*/) avoids unintentionally matching a broad+–@range and now targets only=,+,-, and@as intended. This tightens the CSV‑injection guard without changing behavior for legitimate fields.Also applies to: 823-833
app/Controllers/UsersController.php (1)
261-270: IDOR protection on user update is appropriateUsing the session’s user ID/role and blocking staff from updating any user other than themselves (
$currentUserRole === 'staff' && $currentUserId !== $id) closes the obvious IDOR hole while still allowing admins to manage all accounts. This pairs well with the existing role‑whitelisting logic that prevents staff from escalating toadmin.app/Controllers/PasswordController.php (1)
32-40: Route-based redirects and token handling look consistent and safeRouting all redirects through
RouteTranslator::route(...)keeps URLs consistent across locales, and the various?error=.../?sent=1query strings are built in a straightforward way. For the reset flow, sanitizing the token to strip CR/LF before including it in theLocationheader and passing it viaurlencodein the query string is a good safeguard against header-splitting issues.Also applies to: 46-48, 66-69, 92-93, 100-102, 116-118, 133-135, 143-145, 147-154, 171-181
app/Views/frontend/catalog-grid.php (1)
2-2: LGTM! Centralized URL handling improves security.The migration to
HtmlHelper::getBaseUrl()eliminates direct reliance on$_SERVER['HTTPS']and$_SERVER['HTTP_HOST'], which reduces Host header injection risks. The helper provides consistent validation and canonical URL support across the codebase.Also applies to: 28-29
app/Views/frontend/cookies-page.php (1)
70-70: LGTM! XSS protection added.Wrapping
$pageContentwithHtmlHelper::sanitizeHtml()adds important XSS protection while preserving safe HTML formatting. The whitelist-based approach is a security best practice.app/Views/frontend/privacy-page.php (1)
69-69: LGTM! Consistent XSS protection.Same sanitization pattern as
cookies-page.php, providing XSS protection for user-generated content.app/Views/frontend/catalog.php (1)
2-3: LGTM! Centralized URL handling.The migration to
HtmlHelper::getBaseUrl()for canonical and image URLs is consistent with the broader URL normalization effort.Also applies to: 21-22
app/Views/frontend/home-books-grid.php (1)
2-3: LGTM! Consistent URL handling.Same pattern as
catalog-grid.php, providing centralized and validated URL construction for book cover images.Also applies to: 28-29
app/Controllers/ReservationManager.php (1)
187-210: LGTM! Improved security and environment-driven configuration.The refactored
getBaseUrl()method properly prioritizesAPP_CANONICAL_URLfrom the environment, then falls back to validatedHTTP_HOST. The hostname validation regex at line 204 prevents Host header injection attacks, and the localhost fallback provides a safe default.app/Views/frontend/home-sections/genre_carousel.php (1)
2-3: LGTM! Consistent URL handling.Same centralized URL handling pattern as other view files, providing validated base URL construction for book cover images.
Also applies to: 137-138
app/Views/admin/csv_import.php (1)
227-228: LGTM! Documentation updated to reflect dual separator support.The documentation now correctly shows that authors can be separated by both semicolon (
;) and pipe (|), with the example demonstrating the semicolon format. This aligns with the backend CSV import changes and improves import/export consistency..env.example (1)
17-23: Canonical URL documentation aligns with runtime behaviorThe expanded comments for
APP_CANONICAL_URLand the reordered production checklist accurately reflect the new canonical URL handling (including email links and CLI/cron scenarios) and match theEmailService::getBaseUrl()behavior.No changes needed here.
Also applies to: 70-76
app/Controllers/MaintenanceController.php (1)
30-63: CSRF error localization is consistent; JSON responses remain correctUpdating CSRF failure messages in the maintenance endpoints to use
__('Token CSRF non valido')keeps responses user‑friendly and consistent with the rest of the app, while still returning proper 403 JSON payloads.No functional concerns here.
Also applies to: 65-91, 93-139
installer/steps/step7.php (2)
134-136: LGTM: Clearer warning message.The consolidated warning text is more concise and easier to understand.
205-205: LGTM: Query-based routing is more flexible.The form action change from a hardcoded path to query parameters is a good refactoring that aligns with the handler logic on line 219.
app/Views/admin/integrity_report.php (2)
348-395: Good addition of config fix UI workflow.The
applyConfigFixfunction provides a clean user experience for applying canonical URL fixes. The flow is well-structured with confirmation, loading state, and success/error handling.
107-109: Review comment is incorrect — fix_suggestion format is standardized and regex extraction is robust.The concern about format inconsistency and regex fragility is not valid. The
detectCurrentCanonicalUrl()method (DataIntegrity.php, lines 318-359) has a return type ofstringand always returns a non-empty URL string (defaulting to at least "http://localhost"). All three fix_suggestion cases (missing, empty, or invalid canonical URL) consistently use the format"...APP_CANONICAL_URL={$currentUrl}", where$currentUrlis guaranteed to be populated. The regex pattern/APP_CANONICAL_URL=(.+)$/will reliably extract the URL portion in all scenarios.Likely an incorrect or invalid review comment.
app/Views/frontend/layout.php (2)
50-56: Excellent centralization of URL handling.Delegating to
HtmlHelper::absoluteUrl()ensures consistent and secure URL generation across the application. This prevents potential host header injection vulnerabilities by using a centralized, validated base URL.
81-86: LGTM: Consistent use of HtmlHelper for current URL.Using
HtmlHelper::getCurrentUrl()for canonical and hreflang tags ensures consistent URL resolution with proper security sanitization.app/Support/HtmlHelper.php (3)
69-200: Excellent HTML sanitization implementation.The whitelist-based approach is secure and comprehensive. Key strengths:
- Removes dangerous tags and event handlers
- Validates URL schemes in href/src
- Auto-adds security attributes for external links
- Handles attributes per-tag basis
208-234: Robust base URL resolution with security validation.The method correctly prioritizes
APP_CANONICAL_URLand includes fallback validation. The hostname regex on line 226 follows RFC standards for domain validation.One consideration: the regex doesn't support internationalized domain names (IDN). If your application needs to support non-ASCII domains, you may need to handle punycode encoding. Verify if this is required for your use case.
241-250: LGTM: Proper sanitization of REQUEST_URI.The method correctly sanitizes
REQUEST_URIby removing non-printable characters before concatenation, preventing potential injection attacks.app/Routes/web.php (2)
1191-1194: LGTM: Properly secured maintenance route.The new config fix route is correctly protected with CSRF middleware and admin authentication, and integrates with the UI additions in
integrity_report.php.
1534-1546: Verify inventory number generation handles edge cases.The inventory number generation logic constructs sequential copy numbers (e.g.,
LIB-123-C2). Ensure this handles cases where:
- Copies are deleted and re-added (gaps in sequence)
- Multiple concurrent requests try to add copies simultaneously
- The base
numero_inventariois missing or invalidConsider using a database sequence or auto-increment for copy numbers to avoid race conditions.
app/Controllers/LibriController.php (9)
14-39: Centralized storage/cover path helpers look goodUsing
getStoragePath(),getCoversUploadPath()andgetCoversUrlPath()to centralize paths improves maintainability and reduces path traversal risk when combined with the later realpath checks. No issues spotted here.
40-69: Log rotation and debug logging are safe and conservativeThe rotation logic (size- and age-based) and the development-only debug logging with extra sanitization and
LOCK_EXare reasonable; they avoid leaking sensitive fields and prevent trivial race conditions on log writes. Only minor note: rotation is not atomic under high concurrency, but for dev/debug logs this is acceptable.Also applies to: 71-97
257-271: ISBN/EAN sanitization with length pre-checks is solidPre‑bounding input length per field and then stripping spaces/dashes before further processing is a good ReDoS‑resistant pattern. The logic is consistent between
store()andupdate()and keeps identifiers normalized.Also applies to: 616-630
713-742: Cover removal path hardening is correctThe new logic that restricts deletions to
/uploads/copertine/viarealpath()and a checkedbaseDireffectively blocks path traversal and accidental deletions outside the cover directory while still logging blocked attempts for diagnostics. This is a good security improvement.
1012-1064: SSRF protection via domain whitelist + IP range check is soundCombining
isUrlAllowed()(domain whitelist and localhost/private IP blocking) with DNS resolution plusFILTER_FLAG_NO_PRIV_RANGE | FILTER_FLAG_NO_RES_RANGEinhandleCoverUrl()gives good defense against SSRF and DNS rebinding to internal targets. Behavior for unresolved hosts (treated as invalid) is also acceptable.
1156-1177: Cover file write & URL generation use the new helpers correctlyBoth remote (
handleCoverUrl) and uploaded (handleCoverUpload) covers now usegetCoversUploadPath()andgetCoversUrlPath(), create the directory if needed and write files with controlled names before updatingcopertina_url. This keeps filesystem and URL paths in sync and avoids trusting user‑supplied filenames or MIME types.Also applies to: 1218-1237
1252-1288: URL whitelist inisUrlAllowed()is conservative but correctAllowing only explicit known hosts and local
/uploadspaths, while rejecting localhost/private IPs, is a strong security stance. The simple regex on$hostmay occasionally block odd but valid hostnames that start with private‑range octets, but that trade‑off is acceptable given the threat model.
1299-1307: MIME and extension helpers cover modern formats and block SVGThe explicit early SVG detection (even when served as text/XML) plus magic‑byte checks for JPEG/PNG/GIF/WebP/AVIF/BMP is a good balance between safety and format support.
getExtensionFromMagicBytes()is consistent withisValidImageMimeType()so files are saved with matching, trusted extensions on upload.Also applies to: 1331-1343, 1379-1391
1066-1155: Code is sound; PHPMD optional suggestion not applicableThe cURL HEAD + streamed GET with 2MB cap is a solid DoS mitigation. PHPMD is not configured in this repository, so the optional suggestion to rename unused progress callback parameters (e.g.,
$_resource,$_download_size) is not currently applicable. The parameters are required by cURL'sPROGRESSFUNCTIONsignature and cannot be removed; they can only be renamed if PHPMD is introduced later. The code is correct as-is.locale/en_US.json (2)
4-37: Notification and label translations keep placeholders consistentThe updated English strings for overdue loans, reservations, multi‑author hints, IDs, canonical URL validation, notifications (new reservation/user/contact/overdue loan), and CSV author‑separator help text all preserve the original placeholder structure (
%s,%d) and read naturally. No issues detected with parameter order or meaning.Also applies to: 28-37, 247-248, 565-566, 615-615, 1114-1114, 1385-1385, 1760-1762, 1769-1772, 1912-1912, 1972-1972, 2524-2524, 2839-2840
291-293: Data‑integrity and configuration/maintenance messages are coherent and alignedNew/updated strings around integrity reports, configuration issues (canonical URL presence/validity), automatic fix application, and the confirmation prompts/buttons (“Apply Fix”, “Yes, fix/run/apply”) are consistent with each other and with the backend maintenance/integrity flows. Wording is clear for both Italian and English variants and placeholders like
%s/%dare preserved correctly.Also applies to: 2991-3027, 3055-3071
| // Calculate new total | ||
| $currentCopieTotali = (int) $book['copie_totali']; | ||
| $newCopieTotali = $currentCopieTotali + $copiesToAdd; | ||
|
|
||
| // Update copie_totali counter in libri table | ||
| $stmt = $db->prepare('UPDATE libri SET copie_totali = ? WHERE id = ?'); | ||
| $stmt->bind_param('ii', $newCopieTotali, $bookId); | ||
| $stmt->execute(); | ||
| $stmt->close(); | ||
|
|
||
| // Create physical copies in copie table | ||
| $copyRepo = new \App\Models\CopyRepository($db); | ||
| $baseInventario = !empty($book['numero_inventario']) | ||
| ? $book['numero_inventario'] | ||
| : "LIB-{$bookId}"; | ||
|
|
||
| // Start from current total + 1 for new copies | ||
| for ($i = 1; $i <= $copiesToAdd; $i++) { | ||
| $copyNumber = $currentCopieTotali + $i; | ||
| $numeroInventario = $newCopieTotali > 1 | ||
| ? "{$baseInventario}-C{$copyNumber}" | ||
| : $baseInventario; | ||
|
|
||
| $note = "Copia {$copyNumber} di {$newCopieTotali}"; | ||
| $copyRepo->create($bookId, $numeroInventario, 'disponibile', $note); | ||
| } | ||
|
|
||
| // Recalculate availability using DataIntegrity | ||
| $integrity = new \App\Support\DataIntegrity($db); | ||
| $integrity->recalculateBookAvailability($bookId); |
There was a problem hiding this comment.
🧩 Analysis chain
Improved inventory tracking with physical copy creation.
The refactored logic now creates physical copy records instead of just incrementing counters, which provides better inventory tracking. The use of CopyRepository::create() and DataIntegrity::recalculateBookAvailability() properly separates concerns.
Consider wrapping the copy creation loop and counter updates in a database transaction to ensure atomicity. If any copy creation fails, the copie_totali counter should be rolled back to maintain consistency.
🏁 Script executed:
#!/bin/bash
# Check if CopyRepository::create includes transaction handling
ast-grep --pattern $'class CopyRepository {
$$$
public function create($$$) {
$$$
}
$$$
}'Length of output: 0
🏁 Script executed:
#!/bin/bash
# Find CopyRepository file
fd -e php CopyRepositoryLength of output: 29
🏁 Script executed:
#!/bin/bash
# Search for CopyRepository class definition
rg -n "class CopyRepository" --type=phpLength of output: 52
🏁 Script executed:
#!/bin/bash
# Search for create method in CopyRepository context
rg -n "public function create" --type=php -A 10 -B 2Length of output: 62537
🏁 Script executed:
#!/bin/bash
# Extract the actual code at lines 1522-1551 in web.php
sed -n '1522,1551p' app/Routes/web.phpLength of output: 1261
🏁 Script executed:
#!/bin/bash
# Search for transaction handling around copy creation in web.php
rg -n "begin_transaction|commit|rollback|START TRANSACTION" app/Routes/web.php -B 5 -A 5Length of output: 0
🏁 Script executed:
#!/bin/bash
# Search for try-catch blocks in the increase copies section
rg -n "try\s*\{" app/Routes/web.php -B 2 -A 10 | head -100Length of output: 609
Transaction wrapping required for atomicity—copy creation must succeed or roll back the counter update.
The code updates copie_totali before creating physical copies in the loop. If any copy creation fails mid-loop, the counter remains incremented while the database contains fewer copies than recorded, breaking inventory integrity. Wrap both the counter update and copy creation in a database transaction to ensure atomicity.
🤖 Prompt for AI Agents
In app/Routes/web.php around lines 1522 to 1551, the update to copie_totali and
the loop creating copie are not atomic; wrap the counter update and all copy
creations in a single database transaction so either all changes succeed or none
are applied: begin a transaction before updating copie_totali, perform the
UPDATE and then create each copy inside the same transaction, and if any create
fails roll back and return/throw the error; on success commit the transaction
and then run the DataIntegrity recalculation (or include it inside the
transaction if appropriate), and ensure proper try/catch handling to always
rollback on exceptions and to close any resources.
| <p class="text-gray-700 mb-1"><strong>${__('Titolo:')}</strong> ${existingBook.title}</p> | ||
| ${existingBook.isbn13 ? `<p class="text-gray-700 mb-1"><strong>${__('ISBN-13:')}</strong> ${existingBook.isbn13}</p>` : ''} | ||
| ${existingBook.ean ? `<p class="text-gray-700 mb-1"><strong>${__('EAN:')}</strong> ${existingBook.ean}</p>` : ''} | ||
| ${existingBook.location ? `<p class="text-gray-700 mb-1"><strong>${__('Collocazione:')}</strong> <span class="inline-flex items-center px-2 py-1 bg-blue-100 text-blue-800 rounded-md text-sm"><i class="fas fa-map-marker-alt mr-1"></i>${existingBook.location}</span></p>` : `<p class="text-gray-700 mb-1"><strong>${__('Collocazione:')}</strong> <span class="text-gray-400">${__('Non specificata')}</span></p>`} |
There was a problem hiding this comment.
Critical XSS vulnerability: Escape user-controlled data before inserting into HTML.
The existingBook.location (and other fields like title, isbn13, ean on lines 1982-1984) are interpolated directly into HTML without escaping. If this data contains malicious script tags or HTML, it could execute in the user's browser.
Create a JavaScript HTML escaping utility and apply it to all user-controlled data:
// Add this utility function near the top of the script section
function escapeHtml(unsafe) {
if (!unsafe) return '';
return unsafe
.toString()
.replace(/&/g, "&")
.replace(/</g, "<")
.replace(/>/g, ">")
.replace(/"/g, """)
.replace(/'/g, "'");
}Then apply it in the handleDuplicateBook function:
<p class="mb-4">${__('Esiste già un libro con lo stesso identificatore (ISBN/EAN).')}</p>
<div class="bg-gray-100 p-4 rounded-lg mb-4 text-left">
<p class="font-semibold mb-2"><i class="fas fa-book mr-2"></i>${__('Libro Esistente:')}</p>
<p class="text-gray-700 mb-1"><strong>${__('ID:')}</strong> #${existingBook.id}</p>
- <p class="text-gray-700 mb-1"><strong>${__('Titolo:')}</strong> ${existingBook.title}</p>
- ${existingBook.isbn13 ? `<p class="text-gray-700 mb-1"><strong>${__('ISBN-13:')}</strong> ${existingBook.isbn13}</p>` : ''}
- ${existingBook.ean ? `<p class="text-gray-700 mb-1"><strong>${__('EAN:')}</strong> ${existingBook.ean}</p>` : ''}
- ${existingBook.location ? `<p class="text-gray-700 mb-1"><strong>${__('Collocazione:')}</strong> <span class="inline-flex items-center px-2 py-1 bg-blue-100 text-blue-800 rounded-md text-sm"><i class="fas fa-map-marker-alt mr-1"></i>${existingBook.location}</span></p>` : `<p class="text-gray-700 mb-1"><strong>${__('Collocazione:')}</strong> <span class="text-gray-400">${__('Non specificata')}</span></p>`}
+ <p class="text-gray-700 mb-1"><strong>${__('Titolo:')}</strong> ${escapeHtml(existingBook.title)}</p>
+ ${existingBook.isbn13 ? `<p class="text-gray-700 mb-1"><strong>${__('ISBN-13:')}</strong> ${escapeHtml(existingBook.isbn13)}</p>` : ''}
+ ${existingBook.ean ? `<p class="text-gray-700 mb-1"><strong>${__('EAN:')}</strong> ${escapeHtml(existingBook.ean)}</p>` : ''}
+ ${existingBook.location ? `<p class="text-gray-700 mb-1"><strong>${__('Collocazione:')}</strong> <span class="inline-flex items-center px-2 py-1 bg-blue-100 text-blue-800 rounded-md text-sm"><i class="fas fa-map-marker-alt mr-1"></i>${escapeHtml(existingBook.location)}</span></p>` : `<p class="text-gray-700 mb-1"><strong>${__('Collocazione:')}</strong> <span class="text-gray-400">${__('Non specificata')}</span></p>`}
</div>🤖 Prompt for AI Agents
In app/Views/libri/partials/book_form.php around line 1985, user-controlled
fields like existingBook.location (and nearby title, isbn13, ean at ~1982-1984)
are injected into HTML without escaping, creating an XSS risk; add a small JS
utility function escapeHtml(unsafe) near the top of the script (return empty
string for falsy, convert to string, and replace &, <, >, ", ') and then wrap
all interpolations of existingBook properties with escapeHtml(...) in the
handleDuplicateBook rendering (including the span contents and any fallback "Non
specificata" rendering) so untrusted data is HTML-escaped before insertion,
preserving existing layout and null/undefined handling.
- Fixed JSON body parsing in applyConfigFix endpoint (was causing 400 errors) - Fixed port number handling in getBaseUrl() (was breaking CSS/images on dev servers) - Both fixes are critical for production functionality
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
app/Controllers/MaintenanceController.php (3)
185-211: Consider atomic write pattern for additional reliability.While file locking addresses race conditions, using an atomic write pattern (write to temporary file, then rename) would further protect against file corruption if the write operation is interrupted by disk full, process termination, or system crash.
Example atomic write pattern:
// After generating $newContent $tempPath = $envPath . '.tmp'; if (file_put_contents($tempPath, $newContent) === false) { @unlink($tempPath); throw new \Exception(__("Impossibile scrivere nel file .env")); } if (!rename($tempPath, $envPath)) { @unlink($tempPath); throw new \Exception(__("Impossibile aggiornare il file .env")); }
213-216: Inform users that application restart may be required.After successfully updating the
.envfile, changes toAPP_CANONICAL_URLwon't take effect until the application's environment is reloaded. Consider adding a note in the success message to guide users to restart the application or clear caches if needed.Example message enhancement:
$response->getBody()->write(json_encode([ 'success' => true, - 'message' => __("Configurazione aggiornata con successo!") + 'message' => __("Configurazione aggiornata con successo!") . ' ' . + __("Potrebbe essere necessario riavviare l'applicazione.") ]));
187-187: Consider using a constant for the .env path.The hardcoded relative path
__DIR__ . '/../../.env'assumes a specific directory structure. While this works, using a constant or configuration value would improve maintainability if the project structure changes.Example:
// In a config file or bootstrap define('ENV_PATH', __DIR__ . '/../../.env'); // In the controller $envPath = ENV_PATH;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/Controllers/MaintenanceController.php(4 hunks)app/Support/HtmlHelper.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/Support/HtmlHelper.php
🔇 Additional comments (1)
app/Controllers/MaintenanceController.php (1)
33-33: LGTM: Consistent CSRF error localization.The CSRF validation error messages have been properly localized using the translation function across all maintenance endpoints.
Also applies to: 68-68, 96-96, 147-147
| // Valida URL | ||
| if (!filter_var($fixValue, FILTER_VALIDATE_URL)) { | ||
| $response->getBody()->write(json_encode([ | ||
| 'success' => false, | ||
| 'message' => __("L'URL fornito non è valido") | ||
| ])); | ||
| return $response->withStatus(400)->withHeader('Content-Type', 'application/json'); | ||
| } |
There was a problem hiding this comment.
Add scheme validation to restrict to HTTP/HTTPS only.
The current URL validation using FILTER_VALIDATE_URL accepts any valid URL scheme (including file://, javascript://, data://, ftp://, etc.). Since APP_CANONICAL_URL is used for generating web links (emails, activation links per PR context), it should be restricted to http:// or https:// schemes only.
Apply this diff to add scheme validation:
- // Valida URL
- if (!filter_var($fixValue, FILTER_VALIDATE_URL)) {
+ // Valida URL e schema
+ if (!filter_var($fixValue, FILTER_VALIDATE_URL) ||
+ !preg_match('/^https?:\/\//i', $fixValue)) {
$response->getBody()->write(json_encode([
'success' => false,
- 'message' => __("L'URL fornito non è valido")
+ 'message' => __("L'URL deve iniziare con http:// o https://")
]));
return $response->withStatus(400)->withHeader('Content-Type', 'application/json');
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Valida URL | |
| if (!filter_var($fixValue, FILTER_VALIDATE_URL)) { | |
| $response->getBody()->write(json_encode([ | |
| 'success' => false, | |
| 'message' => __("L'URL fornito non è valido") | |
| ])); | |
| return $response->withStatus(400)->withHeader('Content-Type', 'application/json'); | |
| } | |
| // Valida URL e schema | |
| if (!filter_var($fixValue, FILTER_VALIDATE_URL) || | |
| !preg_match('/^https?:\/\//i', $fixValue)) { | |
| $response->getBody()->write(json_encode([ | |
| 'success' => false, | |
| 'message' => __("L'URL deve iniziare con http:// o https://") | |
| ])); | |
| return $response->withStatus(400)->withHeader('Content-Type', 'application/json'); | |
| } |
🤖 Prompt for AI Agents
In app/Controllers/MaintenanceController.php around lines 176 to 183, the URL
validation only uses FILTER_VALIDATE_URL and thus allows non-HTTP schemes;
update the check to also extract the URL scheme (e.g., with parse_url($fixValue,
PHP_URL_SCHEME)) and reject the value unless the scheme is exactly 'http' or
'https' (return the same 400 JSON response). Keep the existing error message and
response flow but add this additional scheme check immediately after the
FILTER_VALIDATE_URL validation.
| try { | ||
| // Aggiorna il file .env | ||
| $envPath = __DIR__ . '/../../.env'; | ||
| if (!file_exists($envPath)) { | ||
| throw new \Exception(__("File .env non trovato")); | ||
| } | ||
|
|
||
| $envContent = file_get_contents($envPath); | ||
| if ($envContent === false) { | ||
| throw new \Exception(__("Impossibile leggere il file .env")); | ||
| } | ||
|
|
||
| // Cerca e sostituisci APP_CANONICAL_URL | ||
| $pattern = '/^APP_CANONICAL_URL=.*$/m'; | ||
| $replacement = 'APP_CANONICAL_URL=' . $fixValue; | ||
|
|
||
| if (preg_match($pattern, $envContent)) { | ||
| // Esiste già, sostituisci | ||
| $newContent = preg_replace($pattern, $replacement, $envContent); | ||
| } else { | ||
| // Non esiste, aggiungi alla fine | ||
| $newContent = rtrim($envContent) . "\n" . $replacement . "\n"; | ||
| } | ||
|
|
||
| if (file_put_contents($envPath, $newContent) === false) { | ||
| throw new \Exception(__("Impossibile scrivere nel file .env")); | ||
| } |
There was a problem hiding this comment.
Add file locking to prevent race conditions.
The read-modify-write sequence on .env lacks file locking, which could lead to race conditions if multiple administrators trigger this endpoint concurrently. One request could overwrite another's changes or corrupt the file.
Apply this diff to add file locking:
try {
// Aggiorna il file .env
$envPath = __DIR__ . '/../../.env';
if (!file_exists($envPath)) {
throw new \Exception(__("File .env non trovato"));
}
- $envContent = file_get_contents($envPath);
- if ($envContent === false) {
+ $fp = fopen($envPath, 'r+');
+ if ($fp === false) {
throw new \Exception(__("Impossibile leggere il file .env"));
}
+
+ if (!flock($fp, LOCK_EX)) {
+ fclose($fp);
+ throw new \Exception(__("Impossibile ottenere il lock sul file .env"));
+ }
+
+ $envContent = stream_get_contents($fp);
+ if ($envContent === false) {
+ flock($fp, LOCK_UN);
+ fclose($fp);
+ throw new \Exception(__("Impossibile leggere il file .env"));
+ }
// Cerca e sostituisci APP_CANONICAL_URL
$pattern = '/^APP_CANONICAL_URL=.*$/m';
$replacement = 'APP_CANONICAL_URL=' . $fixValue;
if (preg_match($pattern, $envContent)) {
// Esiste già, sostituisci
$newContent = preg_replace($pattern, $replacement, $envContent);
} else {
// Non esiste, aggiungi alla fine
$newContent = rtrim($envContent) . "\n" . $replacement . "\n";
}
- if (file_put_contents($envPath, $newContent) === false) {
+ rewind($fp);
+ ftruncate($fp, 0);
+ if (fwrite($fp, $newContent) === false) {
+ flock($fp, LOCK_UN);
+ fclose($fp);
throw new \Exception(__("Impossibile scrivere nel file .env"));
}
+
+ flock($fp, LOCK_UN);
+ fclose($fp);
$response->getBody()->write(json_encode([
'success' => true,🤖 Prompt for AI Agents
In app/Controllers/MaintenanceController.php around lines 185 to 211, the
read-modify-write of the .env file has no file locking which can cause race
conditions; open the .env with fopen in read-write mode, acquire an exclusive
flock before reading, perform the pattern check and build the new content while
holding the lock, then rewind/truncate, write the new content, fflush and
release the lock before closing the handle; ensure you check for
fopen/flock/file write failures and throw the same Exceptions on failure so the
endpoint remains robust.
Fixed 11 instances of nested PHP tags (<?= '<?= __() ?>' ?>) that were
rendering literal PHP code in form placeholders instead of translated text.
Files changed:
- app/Views/editori/crea_editore.php (9 fixes)
- app/Views/autori/crea_autore.php (2 fixes)
Pattern corrected:
FROM: placeholder="<?= '<?= __("Text") ?>' ?>"
TO: placeholder="<?= __("Text") ?>"
Impact: Form placeholders now display properly translated text instead
of raw PHP code, improving UX for both Italian and English users.
Related to security audit - no security impact, cosmetic i18n bug only.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/Views/autori/crea_autore.php(4 hunks)app/Views/editori/crea_editore.php(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/Views/autori/crea_autore.php
🔇 Additional comments (1)
app/Views/editori/crea_editore.php (1)
53-107: Placeholder translations now work correctly!The placeholder attributes have been fixed from the previous review. All placeholders now use the correct syntax
placeholder="<?= __("...") ?>"which will properly render translated text instead of displaying raw PHP tags to users.
Fixed 2 UI issues:
1. Biography placeholder showing 'Enter $1' instead of proper text
- File: app/Views/autori/crea_autore.php (line 97)
- Changed from: __('Inserisci $1')
- Changed to: __('Inserisci una breve biografia dell'autore...')
2. 404 error for digital-library.css on pages where plugin is not needed
- File: storage/plugins/digital-library/DigitalLibraryPlugin.php (line 321-325)
- Added file_exists() check before loading CSS
- Only loads CSS if file exists in public directory
- Prevents console errors on non-digital-library pages
Impact:
- ✅ Better UX: Clear placeholder text for biography field
- ✅ Clean console: No more 404 errors for missing CSS
- ✅ Plugin compatibility: CSS loads only when file exists
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
storage/plugins/digital-library/DigitalLibraryPlugin.php (1)
324-324: Consider using htmlspecialchars for consistency.While the hardcoded URL poses no XSS risk, lines 319 and 329 use
htmlspecialcharsfor similar asset URLs. Applying the same practice here would maintain consistency and protect against future refactoring that might introduce dynamic URLs.- echo '<link rel="stylesheet" href="/plugins/digital-library/assets/css/digital-library.css">' . "\n"; + echo '<link rel="stylesheet" href="' . htmlspecialchars('/plugins/digital-library/assets/css/digital-library.css', ENT_QUOTES, 'UTF-8') . '">' . "\n";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/Views/autori/crea_autore.php(6 hunks)app/Views/editori/crea_editore.php(5 hunks)storage/plugins/digital-library/DigitalLibraryPlugin.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- app/Views/autori/crea_autore.php
- app/Views/editori/crea_editore.php
🔇 Additional comments (1)
storage/plugins/digital-library/DigitalLibraryPlugin.php (1)
322-324: Verify path mismatch between file_exists check and route handler.The code checks for the CSS file in the
public/plugins/directory (line 322), but theserveAssetroute handler (lines 478-487) serves assets fromstorage/plugins/digital-library/assets/. This means:
- If the CSS exists only in
storage/plugins/digital-library/assets/css/(the typical plugin location), thefile_existscheck will fail and the CSS won't be loaded, even though the route could successfully serve it.- The CSS will only load if it has been explicitly copied/deployed to the
public/directory.Is this intentional (requiring assets to be "published" before use), or should the check verify the file location that the route actually serves from?
Consider aligning the check with the actual serving location:
- // Digital Library CSS - only load if file exists in public directory - $pluginCssPath = __DIR__ . '/../../../public/plugins/digital-library/assets/css/digital-library.css'; + // Digital Library CSS - only load if file exists in plugin directory + $pluginCssPath = __DIR__ . '/assets/css/digital-library.css'; if (file_exists($pluginCssPath)) { echo '<link rel="stylesheet" href="/plugins/digital-library/assets/css/digital-library.css">' . "\n"; }
The file_exists() check was looking in public/plugins/ but the route handler serves assets from storage/plugins/. This caused the CSS to not load even when the file existed in the correct location. Now checks __DIR__ . '/assets/css/digital-library.css' which matches where serveAsset() actually serves files from (line 478).
chore: Release v0.1.5 # Changelog - Security Audit and Fixes ## Backend & UI - Fixed hardcoded Italian notification messages in backend dropdown - Added English translations for all in-app notifications (user registration, reservations, contact messages, overdue loans) - Fixed missing English translations for CSV import interface - Corrected CSV author separator to match export format (semicolon instead of pipe) ## Email & Activation - Restored email functionality with universal domain validation - Removed hardcoded domain whitelist causing email delivery failures - Fixed installer email address configuration - Corrected environment variable handling for canonical URL in activation links ## Installer - Fixed installer deletion form action preventing 404 errors - Corrected installer deletion button path - Updated installer instructions to document both CSV separators (semicolon and pipe) ## Security - Implemented RFC-compliant hostname validation with regex pattern - Replaced brittle whitelist system with universal validation - Added emergency production domain support ## Configuration - Fixed APP_CANONICAL_URL environment variable handling - Corrected .env configuration for email link generation - Improved environment variable parsing and validation --- ## Upgrade Notice If you're upgrading from a previous version and experiencing issues with email activation links or redirects: 1. Navigate to: /admin/maintenance/integrity-report 2. The system will detect the missing APP_CANONICAL_URL configuration 3. Click "Apply Fix" to automatically configure your canonical URL 4. Alternatively, manually add to your .env file: APP_CANONICAL_URL=https://yourdomain.com
Changelog - Security Audit and Fixes
Backend & UI
Email & Activation
Installer
Security
Configuration
Upgrade Notice
If you're upgrading from a previous version and experiencing issues with email activation links or redirects:
Summary by CodeRabbit
New Features
Bug Fixes
Security & Performance
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.