Apply custom CSS to auth pages (safely) + role-specific contributor help#273
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (45)
📒 Files selected for processing (13)
📝 WalkthroughWalkthroughIl CSS personalizzato viene centralizzato in una partial sanitizzata e incluso nelle pagine auth e frontend. Il form libri ora mostra help distinti per ruolo contributore, con traduzioni in italiano, inglese, tedesco e francese. Sono aggiunti test automatici per entrambe le integrazioni. ChangesCSS personalizzato condiviso
Help localizzati per contributore
Verifiche
Estimated code review effort: 2 (Semplice) | ~10 minuti Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
|
btw, what's the real issue with the So how this stored-XSS can even be exploited? And what's the difference of Or am I miss something? |
|
Fair point, and you're mostly right. Under the current model this isn't a privilege-escalation XSS: Where it matters concretely, and specifically because of this PR: Before #273, both If the partial emitted the CSS unsanitized, a Difference from "Essential JavaScript": that field is an explicit, labeled, cookie-category-aware opt-in to run JS, scoped to the frontend layout. The CSS field is labeled CSS and now renders on more surfaces including pre-auth; sanitizing keeps enabling this feature from silently turning the CSS box into a JS channel on pages the JS box was never wired into. So: agreed it's not cross-user XSS — but for a field that now renders pre-auth I'd rather it can only ever style. Shout if you see a hole in that. |
…ies) Five advisories were published against guzzlehttp/guzzle <7.15.1 on 2026-07-20, turning the "composer audit (known CVEs)" CI step red on main and every open PR: - CVE-2026-59883 — cookie disclosure/injection via IP-address domains - Host-only cookie scope not preserved - URI fragments disclosed in redirect Referer headers - Unbounded response cookies (DoS risk) - Proxy-Authorization headers can leak to origin servers All fixed in 7.15.1. The root constraint is already ^7.8, so only the lock + vendor tree change (no composer.json edit). Pulled promises 2.5.0→2.5.1 and psr7 2.12.1→2.13.0 as required transitive upgrades. Verified: composer audit --no-dev clean, autoloader carries no phpstan (0), platform_check still >= 80200, Guzzle client instantiates, PHPStan level 5 full-tree clean.
…ontributor help (#237) Custom CSS on auth pages (fixes #262, closes #265) -------------------------------------------------- The admin-configured advanced.custom_header_css only applied to the app chrome, so it never reached the login/register/forgot/reset pages — a user who wanted to hide the #255 registration fields via CSS couldn't. Extract a shared partial app/Views/auth/partials/custom-css.php, include it on all five auth views, and refactor frontend/layout.php to use it (removing its inline duplicate). Reimplements the intent of PR #265 by @Himura2la SAFELY: that PR emitted the CSS through ContentSanitizer::normalizeExternalAssets() (fonts-only), which does NOT strip a </style><script>… breakout — merging it would have reintroduced the stored-XSS fixed earlier, now on UNAUTHENTICATED pages. The partial uses ContentSanitizer::sanitizeCustomCss() (the same render-time sanitizer as frontend/layout.php), which strips <style>/<script> and HTML comment markers. Verified live: an injected </style><script> payload renders as inert CSS text (window flag undefined, title unchanged, 0 script tags) while legitimate CSS still applies. Role-specific contributor help text (#237, @HansUwe52) ------------------------------------------------------ Illustrator/Translator/Curator/Colorist fields showed the same generic "search for an existing author" hint. Give each its own role-named help string, translated across all four locales (it/en/de/fr). Guard: tests/auth-custom-css-and-contributor-help.unit.php asserts the partial uses the safe sanitizer, every view includes it, and all four locales carry the role-help keys.
e792a81 to
1129375
Compare
|
@fabiodalez-dev thanks for the detailed explanation, but what I don't understand yet is "why is it a problem to allow custom js on unauthenticated pages?" If an instance admin uses the XSS for injecting custom js into its pages bypassing cookie-category-aware consent, then it's purely their own responsibility for breaking GDPR on his instance via a dirty hack. They also can modify source code to get the same result, it would be just a little bit harder... Am I still missing something? Should you really take responsibility for admin's misuse of your interface? |
|
You're not missing anything — you're right. For a trusted admin (the only one who can write either field, who can already run JS via the Custom JS box or by editing source), this isn't a privilege boundary being crossed. There's no untrusted actor and no victim whose trust is violated: an admin serving their own script on their own pages is exactly what the Custom JS feature does by design. So it isn't a security vulnerability, and calling it "XSS" earlier was too strong — my bad. What actually justifies Whether the app should sanitize custom CSS at all — "it's the admin's instance, let them shoot their own foot" — is a fair position, but that's a question about the existing frontend behavior, not this PR. #273 just keeps the two paths consistent. If you think the sanitization should be dropped as theater against a non-threat, open an issue and I'll weigh it on its own merits — I'd rather change both paths together than have them disagree. So: agreed, it's not my job to police an admin's self-misuse, and this isn't guarding against that. It's only keeping one render path from behaving differently than the one already shipped. |
2 participants
Closes #262. Addresses #237. Supersedes #265.
Custom CSS on auth pages (#262 / #265)
advanced.custom_header_cssonly applied to the app chrome, so it never reached the login/register/forgot/reset pages — a user who wants to hide the #255 registration fields via CSS couldn't (#262). This adds a shared partialapp/Views/auth/partials/custom-css.php, includes it on all five auth views, and refactorsfrontend/layout.phpto use it (removing its inline duplicate).This reimplements the intent of #265 (by @Himura2la) safely. #265 emitted the CSS through
ContentSanitizer::normalizeExternalAssets()(fonts-only), which does not strip a</style><script>…breakout — merging it would have reintroduced the stored-XSS that was fixed earlier, and this time on unauthenticated pages. The partial usesContentSanitizer::sanitizeCustomCss()(the same render-time sanitizerfrontend/layout.phpalready uses), which strips<style>/<script>and HTML comment markers.Verified live: an injected
</style><script>payload renders as inert CSS text (window flagundefined, page title unchanged, 0 script tags in<head>), while legitimate CSS still applies on/registerand/accedi. The existingcustom-css-injection.spec.jsstill passes.Role-specific contributor help (#237, @HansUwe52)
Illustrator/Translator/Curator/Colorist showed the same generic "search for an existing author" hint. Each now has its own role-named help string, translated across all four locales (it/en/de/fr).
Tests
tests/auth-custom-css-and-contributor-help.unit.php(17 assertions) locks in the safe sanitizer, the partial inclusion on every view, and the role-help keys across all four locales.Summary by CodeRabbit
Nuove funzionalità
Miglioramenti