Security hardening + OPAC fixes (v0.7.42)#294
Merged
Conversation
…orage Six findings from a multi-agent security scan of app/, public/ and installer/, each confirmed against the code and covered by a regression test (tests/security-scan-findings.unit.php, 12/12): - F1 (CWE-807, MEDIUM): RateLimitMiddleware keyed the login/forgot/reset throttle on client-supplied X-Forwarded-For without checking the peer, so a rotating header minted unlimited buckets and defeated the only brute-force control. It now honors forwarding headers only when REMOTE_ADDR is a configured trusted proxy (reuses HtmlHelper::isRemoteAddrTrustedProxy, made public); default Apache-direct keys on the real peer. - F6 (CWE-807): same trusted-proxy gate applied to RememberMeService::getClientIP so the security audit log / session UI cannot be fed a forged IP. - F3 (CWE-862): private mode allow-listed /feed.xml, /sitemap and /llms.txt, leaking book titles/authors/descriptions and enumerating every catalog URL to logged-out visitors. Removed from ALLOWED_PREFIXES (still fully public when private mode is off). - F5 (CWE-918): /api/plugins/proxy-image followed redirects and re-resolved DNS after its own public-IP check (TOCTOU rebinding). Now FOLLOWLOCATION=false (3xx refused), protocol pinned to HTTPS, and curl pinned via CURLOPT_RESOLVE to the exact IPs already validated as public. - F2 (CWE-312): the installer wrote smtp_password to system_settings in cleartext, bypassing the app's AES-256-GCM at-rest encryption. It now encrypts with the same scheme inline (round-trips through SettingsEncryption::decrypt); aborts the step if no key is available rather than persisting plaintext. - F4 (CWE-918): the installer DB connection test echoed the raw PDO error, an error-based oracle for internal port scanning on the unauthenticated test-connection path. Detail now goes to the server log; the client gets a generic, non-oracle message. phpstan level 5 clean; full unit battery 67/67.
…trict CSP Three OPAC/CMS regressions reported on 2026-07-25/26: - Related books grid packed 6+ cards on high-resolution laptops. The auto-fill minmax(170px) floor let a wide grid fit too many columns. Each column is now at least a quarter of the row (or 200px), so the grid caps at 4 columns on wide screens and degrades to 3/2/1 as the viewport narrows; the <=480px swipe carousel is unchanged. Verified in a real browser at 1512/1100/834/640/390px -> 4/4/3/2/carousel. - The book subtitle (sottotitolo) was never rendered on the public book page even when present (#293). Added it under the title, escaped the same way, shown only when non-empty. Verified it now displays. - Hero background upload failed under a strict connect-src CSP (#292): the file-added handler did fetch(URL.createObjectURL(file.data)), and blob: is not always allowed by connect-src, so the image never reached the form. Uppy already provides a File in file.data — use it directly (mirrors settings/index.php's logo uploader); only fall back to fetching a preview URL when file.data is not a File. No blob: fetch on the normal path, so it works regardless of the site's CSP.
Findings from a multi-agent security audit of app/, public/ and installer/,
each confirmed against the code and fixed under the project's conventions
(soft-delete, escaping, \Throwable, admin re-checks). HIGH first:
- F1/F5 (CWE-862): GET /api/editori and /api/libri were unauthenticated and
returned SELECT e.*/l.* — publisher PII (email, phone, tax code, referent
contacts) and internal book fields (price, private notes, inventory numbers).
Both routes now require AdminAuthMiddleware, matching the sibling admin API
routes and the API docs.
- F2 (CWE-862): a standard/premium patron could read every borrower's name,
email and loan history via /admin/books/{id}. Borrower PII is now not fetched
for non-admin/staff and gated behind $isAdminOrStaff in the view (defaults to
false so an unset flag can never leak PII).
- F3/F7 (CWE-269): a staff user could upload+install an update package or run a
full update over the app files (path to RCE). installManualUpdate/uploadUpdate/
performUpdate now require an inline admin re-check, like backup/restore.
- F8 (CWE-613): admin routes re-validate tipo_utente + stato against the DB each
request (cached per request), so demotion/suspension takes effect immediately.
- F9 (CWE-863): the public search API no longer returns the borrower's name/
email — availability and due date only.
- F11 (CWE-522): settings no longer expose API keys or the reCAPTCHA secret to
staff; F12 (CWE-598): API keys accepted only via X-API-Key / Authorization
header, never a ?api_key= query string.
- F4/F10 (CWE-601): open redirect after login and on the language switch — the
`/\` backslash-authority bypass of the `//` guard is now rejected.
- F6 (CWE-22): path traversal in language-file upload — the locale code is
validated and the resolved path confined before any file write.
- SSRF: image proxy no longer follows redirects and pins curl to the validated
public IPs (DNS-rebinding); installer DB test no longer echoes the raw PDO
error (network oracle). F13: reset links fail closed on the Host header.
F14: login dummy hash matches PASSWORD_DEFAULT cost (timing). F15/F16: CSV/TSV
exports escape formula-leading cells. F1(prev)/F6(prev): rate-limit + audit IP
only trust forwarding headers behind a configured proxy.
Release prep for v0.7.42:
- 4 new UI strings added to all 5 locales; da_DK key count 6607 -> 6611.
- migrate_0.7.42.sql bumps existing 0.7.41 installs to the new da_DK count
(idempotent); migrate_0.7.41.sql updated in lockstep with the locale file.
- New behavioural migration test (tests/migration-0.7.42.unit.php, 5/5).
- version.json -> 0.7.42; README "What's New" section; docs/api.MD updated.
phpstan level 5 clean; full unit battery 68/68; code-quality 15/15.
…d, redirect Two Opus reviewers (correctness + regression) audited the audit fixes and agreed on one real regression the F11 fix introduced, plus two minor residuals: - F11 regression (was HIGH): the reCAPTCHA secret input is rendered disabled for staff, and disabled inputs are not submitted — so updateContactSettings read the absent field as '' and persisted it, silently WIPING the admin-configured secret and fail-opening reCAPTCHA on the public contact form. Now the secret key is admin-only in the save handler: for non-admins it is dropped from the settings map, so neither a normal staff save nor a forged POST can overwrite the stored value. - F3/F7 residual (LOW): saveToken() (persists the GitHub update token) lacked the inline admin re-check its sibling update actions have — added. - PrestitiController::sanitizeRedirect (LOW): only blocked '//', not the '/\' backslash-authority bypass; aligned with the AuthController/LanguageController sanitizers (findings F4/F10). Admin+CSRF-gated, so not externally exploitable, fixed for consistency. Everything else in the audit was verified complete by both reviewers. phpstan level 5 clean.
|
Warning Review limit reached
Next review available in: 3 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughLa release 0.7.42 introduce hardening su autorizzazioni, privacy, redirect, API key, proxy, rate limiting e segreti. Aggiorna inoltre installer, migrazione danese, test di regressione, interfacce libro/homepage, documentazione e versione. ChangesHardening e release 0.7.42
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
Owner
Author
|
@coderabbitai full review |
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 56 minutes. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/Controllers/LibriController.php (1)
425-428: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winRiduci lo stesso PII anche su
$copySchedule.
getScheduleByBookId()restituisceutente_id,utente_nome,utente_cognomeeutente_email; il calendario costruisce gli eventi da$copySchedule, non solo da$copie. Applica lo stesso bloccounset()anche per $copySchedule quando$isAdminOrStaffè falso.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/Controllers/LibriController.php` around lines 425 - 428, Applica a $copySchedule lo stesso blocco di rimozione dei dati personali già usato per $copie quando $isAdminOrStaff è falso. Nel flusso del controller, rimuovi da ogni elemento di $copySchedule i campi utente_id, utente_nome, utente_cognome e utente_email prima che il calendario costruisca gli eventi.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/Controllers/PasswordController.php`:
- Around line 100-107: Replace the error_log call in the EmailService fallback
within the password email flow with SecureLogger::error(), preserving the
failure context and exception details while using the centralized redaction and
retention behavior.
In `@app/Middleware/AdminAuthMiddleware.php`:
- Around line 81-90: La revalidazione del ruolo deve aggiornare la sessione e i
controlli successivi devono usare il ruolo ri-validato. In
app/Middleware/AdminAuthMiddleware.php alle righe 81-90, modifica
revalidateRole() affinché sincronizzi $_SESSION['user']['tipo_utente'] e stato
con i valori DB prima di restituire true; in
app/Controllers/UpdateController.php alle righe 75-79, 498-502, 591-595 e
660-665, mantieni i controlli di performUpdate, uploadUpdate,
installManualUpdate e saveToken, facendoli dipendere dal ruolo aggiornato
anziché dallo snapshot di login.
- Around line 168-263: In tutte le route che istanziano AdminAuthMiddleware,
passa la connessione mysqli condivisa dal container tramite
$app->getContainer()->get('db'). Aggiungi o aggiorna il costruttore di
AdminAuthMiddleware per ricevere e conservare questa dipendenza, lasciando
getDb() esclusivamente come fallback quando la connessione iniettata non è
disponibile.
In `@app/Routes/web.php`:
- Around line 2821-2849: Apply the same validated-IP DNS pinning to the
`/proxy/cover` curl block: collect the validated IPv4 and IPv6 addresses, reject
an empty collection, build the host/port/IP resolve entry, and pass it through
`CURLOPT_RESOLVE` alongside the existing HTTPS restriction. Update the
`/proxy/cover` curl configuration rather than the already protected block shown
by the diff.
In `@app/Views/settings/contacts-tab.php`:
- Around line 143-148: Update the recaptcha_secret_key input in the admin
contacts view to use the view’s required direct attribute escaping instead of
HtmlHelper::e($recaptchaSecret), while preserving the existing password field
value and surrounding markup.
---
Outside diff comments:
In `@app/Controllers/LibriController.php`:
- Around line 425-428: Applica a $copySchedule lo stesso blocco di rimozione dei
dati personali già usato per $copie quando $isAdminOrStaff è falso. Nel flusso
del controller, rimuovi da ogni elemento di $copySchedule i campi utente_id,
utente_nome, utente_cognome e utente_email prima che il calendario costruisca
gli eventi.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: bad50b98-6bc5-44de-bbf2-c12b78e6639d
📒 Files selected for processing (37)
README.mdapp/Controllers/Admin/LanguagesController.phpapp/Controllers/AuthController.phpapp/Controllers/CollocazioneController.phpapp/Controllers/LanguageController.phpapp/Controllers/LibraryThingImportController.phpapp/Controllers/LibriController.phpapp/Controllers/PasswordController.phpapp/Controllers/PrestitiController.phpapp/Controllers/PublicApiController.phpapp/Controllers/SettingsController.phpapp/Controllers/UpdateController.phpapp/Middleware/AdminAuthMiddleware.phpapp/Middleware/ApiKeyMiddleware.phpapp/Middleware/PrivateModeMiddleware.phpapp/Middleware/RateLimitMiddleware.phpapp/Routes/web.phpapp/Support/HtmlHelper.phpapp/Support/RememberMeService.phpapp/Views/cms/edit-home.phpapp/Views/frontend/book-detail.phpapp/Views/libri/scheda_libro.phpapp/Views/settings/advanced-tab.phpapp/Views/settings/contacts-tab.phpdocs/api.MDinstaller/classes/Validator.phpinstaller/database/migrations/migrate_0.7.41.sqlinstaller/database/migrations/migrate_0.7.42.sqlinstaller/steps/step6.phplocale/da_DK.jsonlocale/de_DE.jsonlocale/en_US.jsonlocale/fr_FR.jsonlocale/it_IT.jsontests/migration-0.7.42.unit.phptests/security-scan-findings.unit.phpversion.json
CodeRabbit's full review surfaced a real gap the Opus reviewers missed, plus three quick wins: - AdminAuthMiddleware: revalidateRole() read tipo_utente/stato from the DB but discarded them, leaving $_SESSION['user']['tipo_utente'] at the stale login-time value. Since the middleware admits both admin and staff, a demoted admin (admin -> staff) passed the set-membership check AND kept 'admin' in the session, so every downstream inline 'admin'-only guard (update install/perform, saveToken, reCAPTCHA secret) still granted the destructive op. Now the fresh DB role/status is written back to the session, so demotion takes full effect immediately. - /proxy/cover: applied the same CURLOPT_RESOLVE IP-pinning as /api/plugins/proxy-image so the cover proxy isn't vulnerable to the DNS- rebinding second-lookup (a whitelisted-but-compromised host resolving to an internal IP). - PasswordController: the EmailService-fallback catch used error_log(); the PHPMailer message can embed the recipient address, so use SecureLogger::error for centralised redaction. - contacts-tab.php: replaced HtmlHelper::e() with htmlspecialchars(ENT_QUOTES) on the reCAPTCHA secret value, per the project's no-HtmlHelper-in-views rule. phpstan level 5 full-tree clean; unit battery 68/68.
extra-features Test 6 (profile phone edit) and Test 9 (admin creates user) predate #255, which made address required-by-config on the user forms. On an install where address is required they left it empty, so the form never submitted — Test 6 read a NULL phone and Test 9 stayed on /create. Fill address (guarded on field presence) so both specs pass regardless of the install's required-field config. Test-only; the product itself persists phone+address correctly when all required fields are filled. (Re-applied cleanly on the current specs; the old test/user-form-address- required-255 branch was based on a stale main and carried vendor/ noise, so it was not merged.)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Release-prep branch for v0.7.42 — a security-hardening + bug-fix release. It closes a set of access-control, SSRF, open-redirect and data-exposure issues found in a full security audit of
app/,public/andinstaller/, and fixes three OPAC/CMS regressions reported by users.Security (audit findings)
High
GET /api/editoriandGET /api/libriwere reachable without a session and returned every column, exposing publisher PII (email, phone, tax code, referent contacts) and internal book fields (price, private notes, inventory numbers). Now behind the admin-session middleware, matching how the API docs already describe them./admin/books/{id}and read every borrower's name, email and loan history. Borrower identity is now gated to admin/staff only.staffuser could upload+install an update package or trigger a full update over the app files (a route to code execution). Those actions (andsaveToken) now require an explicitadminre-check.Medium/Low — public search API no longer leaks borrower name/email; admin routes re-validate role+status against the DB each request (immediate demotion/suspension); settings no longer expose API keys / reCAPTCHA secret to staff; API keys accepted only via header, never
?api_key=; open redirect after login and on the language switch (/\backslash bypass) fixed in all sanitizers; path-traversal on language-file upload; SSRF in the image proxy (redirect + DNS-rebinding) and the installer DB test; plaintext SMTP password in the installer; CSV/formula injection in two exports; reset-link Host-header poisoning; login timing side channel; rate-limit bypass via forged forwarding headers.Two independent Opus reviewers audited the fixes; the one real regression they found (a staff save wiping the admin reCAPTCHA secret) and two minor residuals are fixed in the final commit.
Bug fixes
connect-srcCSP (Unable to upload Hero Background Image #292) — no longer fetches ablob:URL.Database
migrate_0.7.42.sql— bumps theda_DKtranslation key count (6607 → 6611) for installs already on 0.7.41 (idempotent). New behavioural migration test.Testing
migration-0.7.42andsecurity-scan-findingstests);code-quality15/15. The related-books and subtitle fixes were verified in a real browser at 5 viewport widths. Full E2E suite + reinstall regression to run before release.Summary by CodeRabbit
Nuove funzionalità
Correzioni
Documentazione