Security hardening + OPAC fixes (v0.7.42)#294
Open
fabiodalez-dev wants to merge 4 commits into
Open
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.
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (37)
✨ Finishing Touches📝 Generate docstrings
🧪 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 triggered.
|
Owner
Author
|
@coderabbitai full review |
Action performedFull review triggered. 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. |
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