security: fix stored XSS in per-copy fields + harden two open redirects#267
security: fix stored XSS in per-copy fields + harden two open redirects#267fabiodalez-dev wants to merge 1 commit into
Conversation
Full-app XSS review. One real stored XSS plus two low-severity open redirects and three defence-in-depth encoding gaps. Stored XSS (high) — app/Views/libri/scheda_libro.php. The staff-writable per-copy fields numero_inventario/note/stato (copy tracking #238) were emitted into inline onclick handlers with only htmlspecialchars(ENT_QUOTES) — the wrong encoding for a JS string inside an HTML attribute: the browser HTML-decodes the attribute before the JS parser runs, so a copy code like ');alert(document.domain)// broke out of the string and executed in another admin's browser on click of the copy edit/delete button (a staff->admin vector, issue #27). numero_inventario was additionally dropped raw into a SweetAlert html: template. Fix: json_encode() the values (JSON_HEX_* flags) for the onclick handlers and escapeHtml() the SweetAlert body. Open redirects (low): - AutoriController::delete used str_contains(referer, '/admin/authors'), which accepts https://evil.tld/admin/authors. Now validates same-host and path before bouncing back, else falls back to url('/admin/authors'). - public/index.php force-HTTPS redirect reflected the raw Host header. Now prefers the APP_CANONICAL_URL host when configured. Defence in depth (low, not currently reachable): add JSON_HEX_TAG to the home JSON-LD (FrontendController) and to the notification 'Apri' label (layout.php); escape the bare route_path() href in the latest-books CTA. Tests: xss-copy-fields-encoding.unit.php (30 assertions — the exact view encoding is inert for every breakout payload) and xss-copy-fields.spec.js (seeds a copy with a live payload, drives the real book-detail page: no onclick breakout, no SweetAlert <img> injection, no dialog).
|
Warning Review limit reached
Next review available in: 6 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 Run ID: 📒 Files selected for processing (8)
✨ 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 |
1 participant
Full-app XSS review (server views · client JS/DOM · controllers/headers). One real stored XSS, two low-severity open redirects, and three defence-in-depth encoding gaps. The rest of the output surface was verified safe.
🔴 High — Stored XSS in per-copy fields (
app/Views/libri/scheda_libro.php)The staff-writable per-copy fields
numero_inventario/note/stato(copy tracking #238) were rendered into inlineonclickhandlers with onlyhtmlspecialchars(ENT_QUOTES). That is the wrong encoding for a JS string inside an HTML attribute: the browser HTML-decodes the attribute ('→') before the JS parser runs, so a copy code like');alert(document.domain)//broke out of the string and executed in another admin's browser when they clicked the copy edit/delete button — a staff→admin persistence/escalation vector (issue #27).numero_inventariowas additionally dropped raw into a SweetAlerthtml:template.Fix:
json_encode()the values withJSON_HEX_TAG|JSON_HEX_APOS|JSON_HEX_QUOT|JSON_HEX_AMPfor the twoonclickhandlers, andescapeHtml()the SweetAlert body. The same value was already escaped correctly elsewhere in the file — this was a localized oversight, not a systemic gap.🟡 Low — Open redirects (no header injection — PSR-7 blocks CRLF)
AutoriController::deleteusedstr_contains($referer, '/admin/authors'), which acceptshttps://evil.tld/admin/authors. Now validates same-host + path before bouncing back, else falls back tourl('/admin/authors').public/index.phpforce-HTTPS redirect reflected the rawHostheader (and exited before the canonical-host enforcement). Now prefers theAPP_CANONICAL_URLhost when configured.🔵 Defence in depth (low, not currently reachable)
FrontendController) and the notification "Apri" label (layout.php) now includeJSON_HEX_TAG.route_path()in the latest-books CTA href is nowhtmlspecialchars()-wrapped (convention).Tests
tests/xss-copy-fields-encoding.unit.php— 30 assertions: the exact view encoding renders inert for every breakout payload (quote/tag/attr/backslash/unicode).tests/xss-copy-fields.spec.js— E2E: seeds a copy carrying a live payload into the DB, opens the real book-detail page, clicks delete (SweetAlert) and edit — noonclickbreakout, no injected<img>, no dialog; structural check confirms the json-encoded handler.Verification: PHPStan level 5 full-tree clean, unit 30/30, E2E 1/1.
Verified safe (broad coverage)
Header injection structurally impossible (PSR-7 rejects
\r\n); RSS/Atom/sitemap/ICS/OAI all escaped; JSON alwaysapplication/json;Content-Dispositionfilenames sanitized; file serving realpath-contained +nosniff. Views: all otheronX=/JSON-LD/script contexts use(int)orjson_encode(JSON_HEX_TAG)+htmlspecialchars; rich text sanitized at save viaHtmlHelper::sanitizeHtml(); meta/OG tagshtmlspecialchars. Client JS: unified search, DataTables, autocomplete, file preview, other SweetAlerthtml:all escape viaescapeHtml/textContent/createElement.