From fa4893d06401a84dd7f0a5a80e432d4171bdfc14 Mon Sep 17 00:00:00 2001 From: Snider Date: Thu, 29 Jan 2026 12:34:35 +0000 Subject: [PATCH] fix(security): require HTMLPurifier for XSS sanitisation The previous getSanitisedContent() method fell back to strip_tags() when HTMLPurifier was unavailable. This fallback was insecure as strip_tags() does not sanitise attributes, allowing XSS via onclick, onerror, and javascript: URLs. Changes: - Created Services/HtmlSanitiser.php using HTMLPurifier as the sole sanitiser - Added ezyang/htmlpurifier as a required dependency in composer.json - Added boot-time validation that throws RuntimeException if missing - Removed insecure strip_tags() fallback from ContentItem model - Added 30+ unit tests covering XSS attack vectors Closes SEC-002 from TODO.md Co-Authored-By: Claude Opus 4.5 --- Boot.php | 21 ++ Models/ContentItem.php | 15 +- Services/HtmlSanitiser.php | 151 +++++++++++ TODO.md | 318 +++++++++++++++++++++++ composer.json | 3 +- docs/architecture.md | 422 +++++++++++++++++++++++++++++++ docs/security.md | 389 ++++++++++++++++++++++++++++ tests/Unit/HtmlSanitiserTest.php | 322 +++++++++++++++++++++++ 8 files changed, 1631 insertions(+), 10 deletions(-) create mode 100644 Services/HtmlSanitiser.php create mode 100644 TODO.md create mode 100644 docs/architecture.md create mode 100644 docs/security.md create mode 100644 tests/Unit/HtmlSanitiserTest.php diff --git a/Boot.php b/Boot.php index 80a6173..cd2f02d 100644 --- a/Boot.php +++ b/Boot.php @@ -12,6 +12,8 @@ use Illuminate\Cache\RateLimiting\Limit; use Illuminate\Http\Request; use Illuminate\Support\Facades\RateLimiter; use Illuminate\Support\ServiceProvider; +use Core\Mod\Content\Services\HtmlSanitiser; +use RuntimeException; /** * Content Module Boot @@ -38,12 +40,31 @@ class Boot extends ServiceProvider public function register(): void { $this->mergeConfigFrom(__DIR__.'/config.php', 'content'); + + // Register HtmlSanitiser as a singleton for performance + $this->app->singleton(HtmlSanitiser::class); } public function boot(): void { $this->loadMigrationsFrom(__DIR__.'/Migrations'); $this->configureRateLimiting(); + $this->validateSecurityDependencies(); + } + + /** + * Validate that security-critical dependencies are available. + * + * @throws RuntimeException If HTMLPurifier is not installed + */ + protected function validateSecurityDependencies(): void + { + if (! HtmlSanitiser::isAvailable()) { + throw new RuntimeException( + 'core-content requires HTMLPurifier for secure HTML sanitisation. '. + 'Install it with: composer require ezyang/htmlpurifier' + ); + } } /** diff --git a/Models/ContentItem.php b/Models/ContentItem.php index 551fae3..9effd29 100644 --- a/Models/ContentItem.php +++ b/Models/ContentItem.php @@ -16,6 +16,7 @@ use Illuminate\Database\Eloquent\Relations\HasMany; use Illuminate\Database\Eloquent\SoftDeletes; use Core\Mod\Content\Enums\ContentType; use Core\Mod\Content\Observers\ContentItemObserver; +use Core\Mod\Content\Services\HtmlSanitiser; #[ObservedBy([ContentItemObserver::class])] class ContentItem extends Model @@ -330,6 +331,10 @@ class ContentItem extends Model * * Uses HTMLPurifier to remove XSS vectors while preserving * safe HTML elements like paragraphs, headings, lists, etc. + * + * SECURITY: This method uses HTMLPurifier which is a required dependency. + * Never fall back to strip_tags() as it does not sanitise attributes + * (e.g., onclick, onerror) which can still execute JavaScript. */ public function getSanitisedContent(): string { @@ -339,15 +344,7 @@ class ContentItem extends Model return ''; } - // Use the StaticPageSanitiser if available - if (class_exists(\Mod\Bio\Services\StaticPageSanitiser::class)) { - return app(\Mod\Bio\Services\StaticPageSanitiser::class)->sanitiseHtml($content); - } - - // Fallback: basic sanitisation using strip_tags with allowed tags - $allowedTags = '