security: fix stored XSS in per-copy fields + harden two open redirects#267
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: 40 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 (3)
📝 WalkthroughWalkthroughIl PR centralizza la validazione dei redirect basati su ChangesHardening della sicurezza applicativa
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
🚥 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 |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/AutoriController.php`:
- Around line 403-406: Aggiorna la validazione nel flusso che assegna `$target`
in `AutoriController` per non considerare attendibile `$_SERVER['HTTP_HOST']`
nel confronto del Referer. Usa l’host canonico configurato, oppure
preferibilmente costruisci il redirect esclusivamente con `path` e `query`
estratti da `$referer`, ignorando sempre scheme e host esterni.
In `@tests/xss-copy-fields.spec.js`:
- Around line 106-129: Extend the DELETE-path assertions after the SweetAlert
opens to verify that the payload is present as escaped/inert text in the
SweetAlert body, not merely that no live img exists. Extend the EDIT-path checks
after opening the modal to read the relevant textarea associated with
openEditCopyModal and assert that it preserves the original payload text, while
retaining the existing XSS flag assertions.
- Around line 74-82: Update the setup block around the INSERT and copyId
assignment to retrieve the inserted row’s ID in the same dbQuery call as the
INSERT, rather than issuing a separate SELECT by numero_inventario. Preserve
assigning the returned ID to copyId so the afterAll cleanup reliably deletes the
created copy.
🪄 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
Run ID: d70711a3-b767-4d26-be4c-603852135170
📒 Files selected for processing (8)
app/Controllers/AutoriController.phpapp/Controllers/FrontendController.phpapp/Views/frontend/home-sections/latest_books_title.phpapp/Views/layout.phpapp/Views/libri/scheda_libro.phppublic/index.phptests/xss-copy-fields-encoding.unit.phptests/xss-copy-fields.spec.js
Code reviewBranch: Found 5 findings across all lanes:
Deep lane — correctness & security✓ Auto-fixable (4)
Cross-cutting group G1: F001 + F003 — Both stem from the hand-rolled referer validation in AutoriController::delete. Replace it with (and port-harden) the shared CopyController::safeReferer() helper: that single change removes the host-null open-redirect bypass (F003) and, once the helper's host comparison is made port-aware, restores same-origin referer preservation on non-standard-port deployments (F001). Fixing them separately risks re-introducing one while patching the other. Details and fix proposalsF001 — The same-host guard compares parse_url()'s host (never includes port) against $_SERVER['HTTP_HOST'] (DOES include a non-default port). On a deployment served on a non-standard port (the project's own dev/prod Apache on :8081, or any reverse-proxied port), a legitimate same-origin Referer 'https://host:8081/admin/authors' parses to host='host' while HTTP_HOST='host:8081', so $sameHost is always false and the user is silently bounced to bare /admin/authors instead of the referring page. Fail-safe but a functional regression of the intended referer preservation (the prior str_contains check had no port blind spot).File: Evidence:
Approach: Reuse/centralize CopyController::safeReferer() instead of the inline check, and make the host comparison port-aware (compare host+port, or strip the port from HTTP_HOST) so non-standard-port deployments preserve the referring page. Files to modify:
Verification:
Edge cases to preserve:
Latest fix attempt (fixrun_20260719011110manual): fixed and verified F003 — The guard blocks leading '//' (protocol-relative) but not backslash-prefixed forms ('/\evil.tld/admin/authors'). Such a value has no parse_url host (so $host===null makes $sameHost true) and its path contains '/admin/authors', so $target becomes the raw referer and 'Location: /\evil.tld/admin/authors' is emitted, which Chrome/Safari normalise backslash-to-slash into a protocol-relative redirect to evil.tld. Low real-world reachability (a browser will not send such a Referer) but the defensive check is incomplete relative to the '//' guard it pairs with.File: Evidence:
Approach: Reuse CopyController::safeReferer() instead of the inline guard; it accepts only a leading-'/' internal path or an exact same-host absolute URL, eliminating the host-null bypass (scheme-relative and backslash variants). Files to modify:
Verification:
Edge cases to preserve:
Latest fix attempt (fixrun_20260719011110manual): fixed and verified F006 — The force-HTTPS canonical-host hardening only takes effect when APP_CANONICAL_URL is set. When empty (the default on any existing install upgrading to this release without an admin adding the var), $canonicalUrl !== '' is false and $host silently falls back to the raw attacker-influenceable $_SERVER['HTTP_HOST'] — byte-for-byte the vulnerable behavior the PR's own comment says it fixes. Runs whenever force_https is enabled, so any deployment with HTTPS-forcing on but no canonical URL remains as exposed as before.File: Latest fix attempt (fixrun_20260719011110manual): fixed and verified F007 — Sibling sink with the same shape as the ones this PR hardened (inline onclick handler taking a staff/user-writable event title via json_encode()+htmlspecialchars()) but json_encode() here is called with NO flags (no JSON_HEX_TAG/APOS/QUOT/AMP), unlike the newly-patched scheda_libro.php. Not independently exploitable today (no manual quote-wrapping, so json_encode's own quote-escaping survives the htmlspecialchars->browser-HTML-decode round trip), but it is an inconsistent weaker instance of the exact pattern the PR hardens elsewhere and lacks JSON_HEX_TAG defense-in-depth against a future refactor.File: Latest fix attempt (fixrun_20260719011110manual): fixed and verified Fix runsRun
|
| Finding | Group | Outcome | phase_9_finding |
|---|---|---|---|
| F001 | FG-1 | ✓ fixed and verified | |
| F002 | FG-1 | ✓ fixed and verified | |
| F003 | FG-1 | ✓ fixed and verified | |
| F006 | FG-1 | ✓ fixed and verified | |
| F007 | FG-1 | ✓ fixed and verified |
🤖 Generated with Adam's Claude Code Review Command
…ings Follow-up to the XSS/redirect review of this PR (adamsreview F001/F002/F003, F006/F007 + CodeRabbit). Referer open-redirect (F001/F002/F003 + CodeRabbit): the inline same-host guard added to AutoriController::delete was unsound — it compared parse_url's host (no port) against HTTP_HOST (with port), breaking referer preservation on non-standard-port/reverse-proxied deployments (the project's own :8081), and it treated host-less referers as same-host, allowing open-redirect bypass via 'https:evil.tld/…' and '/\evil.tld/…'. Replace it (and CopyController's divergent safeReferer copy) with a single audited helper, App\Support\RefererGuard, that derives the redirect from the referer's PATH+query only — scheme and host are always discarded — so a spoofed Host/Referer can never leave the origin and there is no host/port comparison to get wrong. Cross-host and protocol-relative referers collapse to their harmless local path; '//', '/\' and scheme-relative forms are rejected to the fallback. index.php force-HTTPS comment (F006): reworded — the canonical-host preference only applies when APP_CANONICAL_URL is set; without it there is no trusted host at bootstrap and the request Host is used best-effort (documented). events/index.php (F007): add JSON_HEX_* flags to the confirmDelete onclick json_encode, matching the hardened scheda_libro sinks (defense in depth). Tests: referer-guard.unit.php (11 assertions — bypass vectors rejected, legit path+query preserved port-agnostically, mustContain gate). xss-copy-fields.spec.js strengthened per CodeRabbit: assert the copy code survives as inert escaped TEXT in the SweetAlert and the note round-trips into #edit-copy-note, and fetch the inserted copy id via LAST_INSERT_ID in the same connection.
|
Applied fixes for the review findings in 4c1300a:
The 3 CodeRabbit inline comments are addressed by the same commit and resolved. PHPStan L5 clean; |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
public/index.php (1)
280-296: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winNon usare
HTTP_HOSTcome fallback per il redirect HTTPS.Se
APP_CANONICAL_URLè assente o invalido, una richiesta conHost: evil.tldproduceLocation: https://evil.tld/.... Il redirect deve fallire in modo sicuro oppure usare esclusivamente un host configurato e validato.Possibile correzione
- $host = $_SERVER['HTTP_HOST'] ?? 'localhost'; $canonicalUrl = getenv('APP_CANONICAL_URL') ?: ($_ENV['APP_CANONICAL_URL'] ?? ''); - if ($canonicalUrl !== '') { - $canonicalParts = parse_url($canonicalUrl); - if (!empty($canonicalParts['host'])) { - $host = $canonicalParts['host'] - . (isset($canonicalParts['port']) ? ':' . (int) $canonicalParts['port'] : ''); - } + $canonicalParts = $canonicalUrl !== '' ? parse_url($canonicalUrl) : false; + if (!is_array($canonicalParts) || empty($canonicalParts['host'])) { + http_response_code(500); + exit('APP_CANONICAL_URL deve essere configurato per forzare HTTPS.'); } + $host = $canonicalParts['host'] + . (isset($canonicalParts['port']) ? ':' . (int) $canonicalParts['port'] : '');🤖 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 `@public/index.php` around lines 280 - 296, Remove the HTTP_HOST fallback from the HTTPS redirect logic around $host and APP_CANONICAL_URL. Require a configured, valid canonical host before issuing the redirect; when APP_CANONICAL_URL is absent or parse_url does not provide a valid host, fail safely instead of constructing a Location from request-controlled input. Preserve the existing canonical port handling for valid configuration.
🤖 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.
Outside diff comments:
In `@public/index.php`:
- Around line 280-296: Remove the HTTP_HOST fallback from the HTTPS redirect
logic around $host and APP_CANONICAL_URL. Require a configured, valid canonical
host before issuing the redirect; when APP_CANONICAL_URL is absent or parse_url
does not provide a valid host, fail safely instead of constructing a Location
from request-controlled input. Preserve the existing canonical port handling for
valid configuration.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b288abfa-4e5a-467c-be35-a4995d540dc3
📒 Files selected for processing (7)
app/Controllers/AutoriController.phpapp/Controllers/CopyController.phpapp/Support/RefererGuard.phpapp/Views/events/index.phppublic/index.phptests/referer-guard.unit.phptests/xss-copy-fields.spec.js
CodeRabbit (outside-diff) flagged the force-HTTPS redirect still building Location from the raw Host header when APP_CANONICAL_URL is unset — an open redirect on catch-all vhosts. Rather than hard-fail (500) when APP_CANONICAL_URL is missing — which would break the common case of force_https enabled via the admin toggle without the env var, and is inconsistent with the app-wide host trust model — delegate the target to HtmlHelper::getCurrentUrl(), the single audited resolver the rest of the app already uses for absolute URLs. It prefers APP_CANONICAL_URL, enforces the APP_TRUSTED_HOSTS whitelist, RFC-1123-validates the Host (falling back to localhost on a bad value, never the attacker's host), and strips control characters from the request URI; the scheme is then forced to https for the upgrade. This removes the divergent weaker inline copy and gives operators on a catch-all vhost the exact protection requested by setting APP_TRUSTED_HOSTS or APP_CANONICAL_URL.
|
@coderabbitai addressed the outside-diff Rather than a hard 500 when
and the scheme is then forced to Note: the other CodeRabbit comments in the same notification (locale duplicates, |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
git show --stat --oneline 505c56fb
git show --format=fuller --find-renames --find-copies 505c56fb -- public/index.php
fd -t f -i 'HtmlHelper.php' app
ast-grep outline public/index.php --items all
ast-grep outline app --items all --match 'HtmlHelper|getCurrentUrl'
rg -n -C 8 'getCurrentUrl\s*\(' public/index.php appLength of output: 14923 🏁 Script executed: #!/bin/bash
set -euo pipefail
ast-grep outline app/Support/HtmlHelper.php --items all --view expanded
sed -n '300,455p' app/Support/HtmlHelper.php
rg -n -C 5 'APP_CANONICAL_URL|APP_TRUSTED_HOSTS|HTTP_HOST|SERVER_NAME|REQUEST_URI' app/Support/HtmlHelper.php public/index.phpLength of output: 18425
In Host: evil.tldproduce ancora: Location: https://evil.tld/<request-uri>Il fallback a Serve un fallback a un’autorità trusted anche senza configurazione esplicita (oppure impedire |
Ten subprocess-isolated unit tests pinning the security properties of the public/index.php force-HTTPS redirect after it moved to HtmlHelper::getCurrentUrl(): legit host upgrades correctly (host/path/query/port preserved, scheme forced to https); spoofed Host is neutralised by APP_TRUSTED_HOSTS or APP_CANONICAL_URL; a malformed Host falls back to localhost (never the attacker); X-Forwarded-Host is ignored without a trusted proxy; a non-first whitelisted host is accepted; and raw CRLF in the request URI is stripped so the Location stays a single header with the host intact. Each scenario runs in a fresh 'php <file> --child' process so per-request $_SERVER/$_ENV and getBasePath()'s static cache never bleed across cases; no DB required.
CodeRabbit correctly pushed back on the previous getCurrentUrl() approach: in the zero-config case getBaseUrl() accepts any RFC-1123-valid Host, so on a catch-all vhost 'Host: evil.tld' still produced 'Location: https://evil.tld/…'. The localhost fallback only caught malformed hosts, not attacker-chosen valid domains. Add HtmlHelper::configuredTrustedHost() (APP_CANONICAL_URL, else the first APP_TRUSTED_HOSTS entry — both operator-configured, never the request Host) and HtmlHelper::forceHttpsRedirectTarget(), which returns the https target built from that trusted host + the control-char-stripped request path, or null when no trusted host is configured. public/index.php now redirects only when a target is returned; otherwise it fails safe — skips the HTTPS upgrade (serves the request over HTTP) and error_logs how to enforce it. The installer always writes APP_CANONICAL_URL, so real installs keep the redirect; only a broken/removed config hits the skip, which is exactly the 'prevent force_https without a trusted host' option CodeRabbit offered. Tests: rewrote tests/force-https-redirect.unit.php (10 subprocess-isolated cases) to pin the new contract — canonical/whitelist host used Host-independently, an attacker Host is ignored, a malformed canonical does not fall back to the Host, zero-config yields NO redirect, and the request path is CRLF-stripped and rooted.
|
@coderabbitai you're right — the previous The redirect target is now built only from an operator-configured trusted host, never the request Host:
This takes the second option you offered ("prevent force_https without a trusted host config"). It doesn't break real installs because the installer always writes Verified with 10 subprocess-isolated tests ( |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
git show --stat --oneline a6741149
fd -t f -i 'HtmlHelper.php' app
fd -t f -i 'Installer.php' app
fd -t f -i 'force-https-redirect.unit.php' tests
ast-grep outline public/index.php --items all
ast-grep outline app/Support/HtmlHelper.php --items all --view expanded
rg -n -C 12 'forceHttpsRedirectTarget\s*\(|configuredTrustedHost\s*\(|force_https|HTTP_HOST|APP_CANONICAL_URL|APP_TRUSTED_HOSTS' \
public/index.php app/Support/HtmlHelper.php app tests
rg -n -C 8 'detectCanonicalUrl\s*\(|APP_CANONICAL_URL' app/Installer.php app 2>/dev/null || trueLength of output: 110466
Ho verificato anche la copertura per Host spoofato, configurazione assente, canonical URL malformato e CRLF nella request URI. Finding risolto. 🐇 If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
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.Summary by CodeRabbit