Security hardening + OPAC fixes (v0.7.42)

README.md

@@ -37,6 +37,34 @@ Pinakes is a self-hosted, full-featured ILS for schools, municipalities, and pri
 
 ---
 
+## What's New in v0.7.42
+
+A security-hardening and bug-fix release. It closes a set of access-control,
+SSRF, open-redirect and data-exposure issues found in a full security audit,
+and fixes three OPAC/CMS regressions.
+
+### Security
+- **Access control on the internal APIs** — `GET /api/editori` and `GET /api/libri` were reachable without a session and returned every column, exposing publisher PII (email, phone, tax code, referent contacts) and internal book fields (purchase price, private notes, inventory numbers). They now require the admin session middleware, matching how they are already documented.
+- **Borrower privacy on the book page** — a non-admin patron could open `/admin/books/{id}` and read every borrower's name, email and loan history. Borrower identity is now gated to admin/staff only (the reservations block already was).
+- **Privilege boundary on updates** — a `staff` user could upload and install an update package, or trigger a full update/downgrade, over the application files (a route to code execution). Those actions now require an explicit `admin` role, like the backup/restore actions.
+- **Public search API** no longer leaks the current borrower's name/email; it returns availability and due dates only.
+- **Session-role enforcement** — admin routes now re-validate the user against the database each request, so demoting or suspending a user takes effect immediately instead of at session expiry.
+- **Secrets** — the settings page no longer shows API keys or the reCAPTCHA secret to the `staff` role; API keys are accepted only via the `X-API-Key` / `Authorization: Bearer` header (never a `?api_key=` query string, which leaks into access logs).
+- **Hardening** — fixed an open redirect after login and on the language switch (`/\` backslash bypass), a path-traversal on language-file upload, SSRF in the image proxy (redirect + DNS-rebinding) and the installer DB test, plaintext SMTP password storage in the installer, CSV/formula injection in two exports, reset-link Host-header poisoning, a login timing side channel, and rate-limit bypass via forged forwarding headers.
+
+### Fixes
+- **Book subtitle now shows on the public book page** ([#293](https://github.com/fabiodalez-dev/Pinakes/issues/293)) — the `sottotitolo` was never rendered in the OPAC; it now appears under the title.
+- **Hero background image upload under a strict CSP** ([#292](https://github.com/fabiodalez-dev/Pinakes/issues/292)) — the uploader fetched a `blob:` URL that a strict `connect-src` policy blocks, so the image never reached the form. It now uses the file object directly and works regardless of the site's CSP.
+- **Related-books count on high-resolution screens** — the related strip packed 6+ cards on high-res laptops; it now caps at 4 and adapts down to 3/2/1 as the viewport narrows, with the mobile swipe carousel unchanged.
+
+### Database Changes
+- `migrate_0.7.42.sql` — bumps the `da_DK` translation key count (6607 → 6611) for installs already on 0.7.41, after this release added 4 UI strings. Idempotent.
+
+### Upgrade Notes
+- Back up your database before updating (the in-app updater does this automatically).
+
+---
+
 ## What's New in v0.7.41
 
 A maintenance release bundling seven merged pull requests: a new interface

app/Controllers/Admin/LanguagesController.php

@@ -99,6 +99,16 @@ public function store(Request $request, Response $response, \mysqli $db, array $
             $errors[] = __("Il nome nativo è obbligatorio (es. Italiano, English)");
         }
 
+        // Bail on an invalid locale code BEFORE reaching the upload handler:
+        // the code becomes part of the target file path, so an invalid code
+        // must never be used to build/write a translation file (path traversal).
+        if (empty($data['code']) || !I18n::isValidLocaleCode($data['code'])) {
+            $_SESSION['flash_error'] = implode('<br>', $errors);
+            return $response
+                ->withHeader('Location', '/admin/languages/create')
+                ->withStatus(302);
+        }
+
         // Handle translation file upload
         $translationFile = null;
         if (isset($_FILES['translation_json']) && $_FILES['translation_json']['error'] === UPLOAD_ERR_OK) {
@@ -559,6 +569,13 @@ private function processTranslationUpload(array $uploadedFile, string $code, boo
             return ['success' => false, 'error' => __("Errore nel caricamento del file JSON")];
         }
 
+        // The locale code becomes part of the target file path — reject any
+        // code that is not a strict xx_YY locale BEFORE touching the filesystem
+        // (prevents arbitrary .json write via "../" traversal).
+        if (!I18n::isValidLocaleCode($code)) {
+            return ['success' => false, 'error' => __("Codice lingua non valido")];
+        }
+
         $extension = strtolower(pathinfo($uploadedFile['name'] ?? '', PATHINFO_EXTENSION));
         $mimeType = strtolower($uploadedFile['type'] ?? '');
 
@@ -580,6 +597,17 @@ private function processTranslationUpload(array $uploadedFile, string $code, boo
         $sanitized = $this->sanitizeTranslations($decoded);
         $targetPath = $this->getLocaleFilePath($code);
 
+        // Defense in depth: the resolved file must live directly inside the
+        // locale directory. realpath() can't resolve a not-yet-created file, so
+        // validate the DIRNAME against the locale base and the basename shape.
+        $localeDir = realpath(__DIR__ . '/../../../locale');
+        $targetDir = realpath(dirname($targetPath));
+        if ($localeDir === false || $targetDir === false
+            || $targetDir !== $localeDir
+            || basename($targetPath) !== $code . '.json') {
+            return ['success' => false, 'error' => __("Percorso file non valido")];
+        }
+
         if ($backupExisting && file_exists($targetPath)) {
             copy($targetPath, $targetPath . '.backup.' . time());
         }

app/Controllers/AuthController.php

@@ -92,7 +92,15 @@ public function login(Request $request, Response $response, mysqli $db): Respons
             // security measure. By always running password_verify() even for non-existent users,
             // attackers cannot enumerate valid emails by measuring response times.
             // See OWASP: https://cheatsheetseries.owasp.org/cheatsheets/Authentication_Cheat_Sheet.html
-            $dummyHash = '$2y$12$PXZb520pM93TmNGnoJy2TuhssLxu4XversvqtKZ4B7xrm0sAldZE6'; // @codingStandardsIgnoreLine
+            //
+            // The dummy MUST use the SAME algorithm+cost as real passwords
+            // (PASSWORD_DEFAULT), otherwise password_verify() against it takes a
+            // measurably different time than against a real hash and leaks
+            // account existence via timing (CWE-208). Computed once per process.
+            static $dummyHash = null;
+            if ($dummyHash === null) {
+                $dummyHash = password_hash('', PASSWORD_DEFAULT);
+            }
             $hashToCheck = (string) ($row['password'] ?? $dummyHash);
 
             // Plugin hook: Custom login validation (e.g., reCAPTCHA, 2FA)
@@ -175,9 +183,11 @@ public function login(Request $request, Response $response, mysqli $db): Respons
 
                 return $response->withHeader('Location', $redirectUrl)->withStatus(302);
             }
-
-            // Ensure hash verification still happens when credentials invalid
-            password_verify($password, $dummyHash);
+            // NOTE: exactly ONE password_verify() runs above on every path — the
+            // real hash on a hit, $dummyHash on a miss — so both paths do
+            // identical work. Do NOT add a second password_verify() here: it
+            // would make the miss/wrong-password path slower than a valid-hash
+            // check and reintroduce the timing oracle (CWE-208).
         }
 
         // Failed: redirect back with invalid credentials error
@@ -220,7 +230,13 @@ private function sanitizeReturnUrl(?string $url): ?string
         if ($clean === '' || !str_starts_with($clean, '/')) {
             return null;
         }
-        if (str_starts_with($clean, '//')) {
+        // Reject protocol-relative ('//') and backslash-authority ('/\') forms:
+        // a browser normalises the '\' to '/', turning '/\evil.com' into
+        // '//evil.com' → a cross-origin open redirect (CWE-601). Query params
+        // are already URL-decoded here, so a '%5C' payload arrives as a raw '\'
+        // and is caught too. This is the single choke point for the post-login
+        // redirect (both loginForm() and login() route return_url through here).
+        if (preg_match('#^/[\\\\/]#', $clean)) {
             return null;
         }
         return $clean;

app/Controllers/CollocazioneController.php

@@ -405,7 +405,7 @@ public function exportCSV(Request $request, Response $response, mysqli $db): Res
 
         // Generate CSV output
         $output = fopen('php://temp', 'r+');
-        $writer = Csv::writerToStream($output, ';');
+        $writer = Csv::writerToStream($output, ';', "'");
         $writer->insertAll($csv);
         rewind($output);
         $csvContent = stream_get_contents($output);

app/Controllers/LanguageController.php

@@ -69,6 +69,16 @@ private function sanitizeRedirect($redirect): string
             return '/';
         }
 
-        return $redirect[0] === '/' ? $redirect : '/';
+        if ($redirect[0] !== '/') {
+            return '/';
+        }
+
+        // Reject backslash- or double-slash-prefixed paths (e.g. "/\evil.com")
+        // that browsers normalise into a cross-origin redirect.
+        if (preg_match('#^/[\\\\/]#', $redirect)) {
+            return '/';
+        }
+
+        return $redirect;
     }
 }

app/Controllers/LibraryThingImportController.php

@@ -2141,7 +2141,7 @@ public function exportToLibraryThing(Request $request, Response $response, \mysq
 
         // LibraryThing headers (TSV format). Field encoding (RFC 4180 quoting
         // of tab/newline/quote, doubled quotes) is handled by league/csv.
-        $writer = Csv::writerToStream($stream, "\t");
+        $writer = Csv::writerToStream($stream, "\t", "'");
         $headers = $this->getLibraryThingHeaders();
         $writer->insertOne($headers);
 

app/Controllers/LibriController.php

@@ -427,42 +427,66 @@ public function show(Request $request, Response $response, mysqli $db, int $id):
         $copie = $copyRepo->getByBookId($id);
         $copySchedule = $copyRepo->getScheduleByBookId($id);
 
-        // Get loan history for this book
-        $loanHistoryQuery = "
-            SELECT
-                p.id,
-                p.data_prestito,
-                p.data_scadenza,
-                p.data_restituzione,
-                p.stato,
-                p.renewals,
-                p.note,
-                u.nome as utente_nome,
-                u.cognome as utente_cognome,
-                u.email as utente_email,
-                u.id as utente_id,
-                staff.nome as staff_nome,
-                staff.cognome as staff_cognome
-            FROM prestiti p
-            LEFT JOIN utenti u ON p.utente_id = u.id
-            LEFT JOIN utenti staff ON p.processed_by = staff.id
-            WHERE p.libro_id = ?
-            ORDER BY p.data_prestito DESC
-        ";
-        $stmt = $db->prepare($loanHistoryQuery);
-        $stmt->bind_param('i', $id);
-        $stmt->execute();
-        $result = $stmt->get_result();
+        // Borrower identity (name/email/reading history) is GDPR-sensitive and must
+        // only be exposed to admin/staff. Standard/premium patrons can reach this
+        // page (route allows them) but must see availability WITHOUT any borrower PII.
+        $currentUserRole = $_SESSION['user']['tipo_utente'] ?? '';
+        $isAdminOrStaff = \in_array($currentUserRole, ['admin', 'staff'], true);
+
+        // Redact borrower PII from the active loan for non-admin/staff viewers,
+        // preserving the loan-existence/date fields needed for availability display.
+        if (!$isAdminOrStaff && !empty($activeLoan)) {
+            foreach (['utente_nome', 'utente_cognome', 'utente_email', 'utente_id'] as $piiField) {
+                unset($activeLoan[$piiField]);
+            }
+        }
+
+        // Redact per-copy current-borrower PII for non-admin/staff viewers.
+        if (!$isAdminOrStaff) {
+            foreach ($copie as $ci => $copia) {
+                foreach (['utente_nome', 'utente_cognome', 'utente_email', 'utente_id'] as $piiField) {
+                    unset($copie[$ci][$piiField]);
+                }
+            }
+        }
+
+        // Get loan history for this book — contains borrower PII, so only fetch it
+        // for admin/staff. Non-admin viewers get an empty history (block hidden).
         $loanHistory = [];
-        while ($row = $result->fetch_assoc()) {
-            $loanHistory[] = $row;
+        if ($isAdminOrStaff) {
+            $loanHistoryQuery = "
+                SELECT
+                    p.id,
+                    p.data_prestito,
+                    p.data_scadenza,
+                    p.data_restituzione,
+                    p.stato,
+                    p.renewals,
+                    p.note,
+                    u.nome as utente_nome,
+                    u.cognome as utente_cognome,
+                    u.email as utente_email,
+                    u.id as utente_id,
+                    staff.nome as staff_nome,
+                    staff.cognome as staff_cognome
+                FROM prestiti p
+                LEFT JOIN utenti u ON p.utente_id = u.id
+                LEFT JOIN utenti staff ON p.processed_by = staff.id
+                WHERE p.libro_id = ?
+                ORDER BY p.data_prestito DESC
+            ";
+            $stmt = $db->prepare($loanHistoryQuery);
+            $stmt->bind_param('i', $id);
+            $stmt->execute();
+            $result = $stmt->get_result();
+            while ($row = $result->fetch_assoc()) {
+                $loanHistory[] = $row;
+            }
+            $stmt->close();
         }
-        $stmt->close();
 
         // Active reservations for this book (admin/staff only - contains PII)
         $activeReservations = [];
-        $currentUserRole = $_SESSION['user']['tipo_utente'] ?? '';
-        $isAdminOrStaff = \in_array($currentUserRole, ['admin', 'staff'], true);
 
         if ($isAdminOrStaff) {
             $resStmt = $db->prepare("