From 2017c0b6809570bbbc75dec512f0f7419f55668c Mon Sep 17 00:00:00 2001 From: Roardom Date: Sat, 4 Nov 2023 10:20:35 +0000 Subject: [PATCH] update: remove XSS cleaner and remove XSS vulnerabilities We've been mostly relying on the 3rd party xss cleaner to make sure user submitted content is clean. This PR fixes up any leftover holes in the bbcode parser that allow xss vulnerabilities, and as a result, the 3rd party library isn't needed anymore. It cleans responsibly by first, running `htmlspecialchars()` over the content, followed by validating any untrusted input used inside html attributes. It validates urls by returning them as redirects relying on the browser not supporting executable protocols (like `javascript:`). --- app/Helpers/Bbcode.php | 7 +- app/Http/Livewire/BbcodeInput.php | 5 +- app/Models/Article.php | 9 -- app/Models/Message.php | 9 -- app/Models/Playlist.php | 9 -- app/Models/Post.php | 9 -- app/Models/PrivateMessage.php | 9 -- app/Models/Torrent.php | 9 -- app/Models/TorrentRequest.php | 9 -- app/Models/User.php | 17 --- composer.json | 1 - ..._094036_htmlspecialchars_decode_bbcode.php | 100 ++++++++++++++++++ 12 files changed, 107 insertions(+), 86 deletions(-) create mode 100644 database/migrations/2023_11_04_094036_htmlspecialchars_decode_bbcode.php diff --git a/app/Helpers/Bbcode.php b/app/Helpers/Bbcode.php index 03701513a8..e8d2b1245d 100755 --- a/app/Helpers/Bbcode.php +++ b/app/Helpers/Bbcode.php @@ -153,7 +153,7 @@ class Bbcode 'block' => true, ], 'namedquote' => [ - 'openBbcode' => '/^\[quote=([^<>"]*?)\]/i', + 'openBbcode' => '/^\[quote=(.*?)\]/i', 'closeBbcode' => '[/quote]', 'openHtml' => '
Quoting $1:

', 'closeHtml' => '

', @@ -278,6 +278,9 @@ class Bbcode */ public function parse(?string $source, bool $replaceLineBreaks = true): string { + $source ??= ''; + $source = htmlspecialchars($source, ENT_QUOTES | ENT_SUBSTITUTE | ENT_HTML5, 'UTF-8'); + // Replace all void elements since they don't have closing tags $source = str_replace('[*]', '
  • ', (string) $source); $source = preg_replace_callback( @@ -319,7 +322,7 @@ public function parse(?string $source, bool $replaceLineBreaks = true): string $source ?? '' ); $source = preg_replace_callback( - '/\[video="youtube"]([a-z0-9_-]{11})\[\/video]/i', + '/\[video="youtube"]([a-z0-9_-]{11})\[\/video]/i', static fn ($matches) => '', $source ?? '' ); diff --git a/app/Http/Livewire/BbcodeInput.php b/app/Http/Livewire/BbcodeInput.php index 63a95655dd..08b15c74bc 100644 --- a/app/Http/Livewire/BbcodeInput.php +++ b/app/Http/Livewire/BbcodeInput.php @@ -18,7 +18,6 @@ use App\Helpers\Bbcode; use Livewire\Component; -use voku\helper\AntiXSS; class BbcodeInput extends Component { @@ -39,13 +38,13 @@ final public function mount(string $name, string $label, bool $required = false, $this->name = $name; $this->label = $label; $this->isRequired = $required; - $this->contentBbcode = $content === null ? (old($name) ?? '') : htmlspecialchars_decode($content); + $this->contentBbcode = $content ?? old($name) ?? ''; } final public function updatedIsPreviewEnabled(): void { if ($this->isPreviewEnabled) { - $this->contentHtml = (new Bbcode())->parse(htmlspecialchars((new AntiXSS())->xss_clean($this->contentBbcode), ENT_NOQUOTES)); + $this->contentHtml = (new Bbcode())->parse($this->contentBbcode); } } diff --git a/app/Models/Article.php b/app/Models/Article.php index 6e9219adac..7e78c0d2e0 100644 --- a/app/Models/Article.php +++ b/app/Models/Article.php @@ -21,7 +21,6 @@ use App\Traits\Auditable; use Illuminate\Database\Eloquent\Factories\HasFactory; use Illuminate\Database\Eloquent\Model; -use voku\helper\AntiXSS; /** * App\Models\Article. @@ -67,14 +66,6 @@ public function comments(): \Illuminate\Database\Eloquent\Relations\MorphMany return $this->morphMany(Comment::class, 'commentable'); } - /** - * Set The Articles Content After Its Been Purified. - */ - public function setContentAttribute(?string $value): void - { - $this->attributes['content'] = $value === null ? null : htmlspecialchars((new AntiXSS())->xss_clean($value), ENT_NOQUOTES); - } - /** * Parse Content And Return Valid HTML. */ diff --git a/app/Models/Message.php b/app/Models/Message.php index 6ea989cbc0..d7b0a779f7 100644 --- a/app/Models/Message.php +++ b/app/Models/Message.php @@ -19,7 +19,6 @@ use App\Helpers\Bbcode; use Illuminate\Database\Eloquent\Factories\HasFactory; use Illuminate\Database\Eloquent\Model; -use voku\helper\AntiXSS; /** * App\Models\Message. @@ -90,14 +89,6 @@ public function chatroom(): \Illuminate\Database\Eloquent\Relations\BelongsTo return $this->belongsTo(Chatroom::class); } - /** - * Set The Chat Message After Its Been Purified. - */ - public function setMessageAttribute(string $value): void - { - $this->attributes['message'] = htmlspecialchars((new AntiXSS())->xss_clean($value), ENT_NOQUOTES); - } - /** * Parse Content And Return Valid HTML. */ diff --git a/app/Models/Playlist.php b/app/Models/Playlist.php index f1def38904..c87251f9a2 100644 --- a/app/Models/Playlist.php +++ b/app/Models/Playlist.php @@ -21,7 +21,6 @@ use App\Traits\Auditable; use Illuminate\Database\Eloquent\Factories\HasFactory; use Illuminate\Database\Eloquent\Model; -use voku\helper\AntiXSS; /** * App\Models\Playlist. @@ -76,14 +75,6 @@ public function comments(): \Illuminate\Database\Eloquent\Relations\MorphMany return $this->morphMany(Comment::class, 'commentable'); } - /** - * Set The Playlists Description After It's Been Purified. - */ - public function setDescriptionAttribute(?string $value): void - { - $this->attributes['description'] = $value === null ? null : htmlspecialchars((new AntiXSS())->xss_clean($value), ENT_NOQUOTES); - } - /** * Parse Description And Return Valid HTML. */ diff --git a/app/Models/Post.php b/app/Models/Post.php index 6f3cbee02c..073a3ecdff 100644 --- a/app/Models/Post.php +++ b/app/Models/Post.php @@ -21,7 +21,6 @@ use App\Traits\Auditable; use Illuminate\Database\Eloquent\Factories\HasFactory; use Illuminate\Database\Eloquent\Model; -use voku\helper\AntiXSS; /** * App\Models\Post. @@ -154,14 +153,6 @@ public function scopeAuthorized( ); } - /** - * Set The Posts Content After Its Been Purified. - */ - public function setContentAttribute(?string $value): void - { - $this->attributes['content'] = $value === null ? null : htmlspecialchars((new AntiXSS())->xss_clean($value), ENT_NOQUOTES); - } - /** * Parse Content And Return Valid HTML. */ diff --git a/app/Models/PrivateMessage.php b/app/Models/PrivateMessage.php index 2cb09b0ca4..5aba0df11b 100644 --- a/app/Models/PrivateMessage.php +++ b/app/Models/PrivateMessage.php @@ -20,7 +20,6 @@ use App\Helpers\Linkify; use Illuminate\Database\Eloquent\Factories\HasFactory; use Illuminate\Database\Eloquent\Model; -use voku\helper\AntiXSS; /** * App\Models\PrivateMessage. @@ -63,14 +62,6 @@ public function conversation(): \Illuminate\Database\Eloquent\Relations\BelongsT return $this->belongsTo(Conversation::class); } - /** - * Set The PM Message After Its Been Purified. - */ - public function setMessageAttribute(string $value): void - { - $this->attributes['message'] = htmlspecialchars((new AntiXSS())->xss_clean($value), ENT_NOQUOTES); - } - /** * Parse Content And Return Valid HTML. */ diff --git a/app/Models/Torrent.php b/app/Models/Torrent.php index d27fce5cd7..dcbac3fdfc 100644 --- a/app/Models/Torrent.php +++ b/app/Models/Torrent.php @@ -29,7 +29,6 @@ use Illuminate\Database\Eloquent\Factories\HasFactory; use Illuminate\Database\Eloquent\Model; use Illuminate\Database\Eloquent\SoftDeletes; -use voku\helper\AntiXSS; /** * App\Models\Torrent. @@ -399,14 +398,6 @@ public function resurrections(): \Illuminate\Database\Eloquent\Relations\HasMany return $this->hasMany(Resurrection::class); } - /** - * Set The Torrents Description After Its Been Purified. - */ - public function setDescriptionAttribute(?string $value): void - { - $this->attributes['description'] = $value === null ? null : htmlspecialchars((new AntiXSS())->xss_clean($value), ENT_NOQUOTES); - } - /** * Parse Description And Return Valid HTML. */ diff --git a/app/Models/TorrentRequest.php b/app/Models/TorrentRequest.php index 02cfb6c52e..7290ba9789 100644 --- a/app/Models/TorrentRequest.php +++ b/app/Models/TorrentRequest.php @@ -22,7 +22,6 @@ use App\Traits\TorrentFilter; use Illuminate\Database\Eloquent\Factories\HasFactory; use Illuminate\Database\Eloquent\Model; -use voku\helper\AntiXSS; /** * App\Models\TorrentRequest. @@ -195,14 +194,6 @@ public function claim(): \Illuminate\Database\Eloquent\Relations\HasOne return $this->hasOne(TorrentRequestClaim::class, 'request_id'); } - /** - * Set The Requests Description After Its Been Purified. - */ - public function setDescriptionAttribute(?string $value): void - { - $this->attributes['description'] = $value === null ? null : htmlspecialchars((new AntiXSS())->xss_clean($value), ENT_NOQUOTES); - } - /** * Parse Description And Return Valid HTML. */ diff --git a/app/Models/User.php b/app/Models/User.php index ce4893bf77..124b5fa086 100644 --- a/app/Models/User.php +++ b/app/Models/User.php @@ -28,7 +28,6 @@ use Illuminate\Foundation\Auth\User as Authenticatable; use Illuminate\Notifications\Notifiable; use Laravel\Fortify\TwoFactorAuthenticatable; -use voku\helper\AntiXSS; /** * App\Models\User. @@ -1114,14 +1113,6 @@ public function getFormattedBufferAttribute(): string return StringHelper::formatBytes($bytes); } - /** - * Set The Users Signature After It's Been Purified. - */ - public function setSignatureAttribute(?string $value): void - { - $this->attributes['signature'] = $value === null ? null : htmlspecialchars((new AntiXSS())->xss_clean($value), ENT_NOQUOTES); - } - /** * Returns the HTML of the user's signature. */ @@ -1132,14 +1123,6 @@ public function getSignatureHtmlAttribute(): string return (new Linkify())->linky($bbcode->parse($this->signature)); } - /** - * Set The Users About Me After It's Been Purified. - */ - public function setAboutAttribute(?string $value): void - { - $this->attributes['about'] = $value === null ? null : htmlspecialchars((new AntiXSS())->xss_clean($value), ENT_NOQUOTES); - } - /** * Parse About Me And Return Valid HTML. */ diff --git a/composer.json b/composer.json index 52aa894688..7c7eb09f43 100644 --- a/composer.json +++ b/composer.json @@ -38,7 +38,6 @@ "spatie/ssl-certificate": "^2.6.6", "symfony/dom-crawler": "^6.4.8", "theodorejb/polycast": "dev-master", - "voku/anti-xss": "^4.1.42", "vstelmakh/url-highlight": "^3.0.3" }, "require-dev": { diff --git a/database/migrations/2023_11_04_094036_htmlspecialchars_decode_bbcode.php b/database/migrations/2023_11_04_094036_htmlspecialchars_decode_bbcode.php new file mode 100644 index 0000000000..7c6898767a --- /dev/null +++ b/database/migrations/2023_11_04_094036_htmlspecialchars_decode_bbcode.php @@ -0,0 +1,100 @@ +lazyById() + ->each(function (object $article): void { + /** @var object{id: int, content: string} $article */ + DB::table('articles') + ->where('id', '=', $article->id) + ->update([ + 'content' => htmlspecialchars_decode($article->content), + ]); + }); + + DB::table('messages') + ->lazyById() + ->each(function (object $message): void { + /** @var object{id: int, message: string} $message */ + DB::table('messages') + ->where('id', '=', $message->id) + ->update([ + 'message' => htmlspecialchars_decode($message->message), + ]); + }); + + DB::table('playlists') + ->lazyById() + ->each(function (object $playlist): void { + /** @var object{id: int, description: string} $playlist */ + DB::table('playlists') + ->where('id', '=', $playlist->id) + ->update([ + 'description' => htmlspecialchars_decode($playlist->description), + ]); + }); + + DB::table('posts') + ->lazyById() + ->each(function (object $post): void { + /** @var object{id: int, content: string} $post */ + DB::table('posts') + ->where('id', '=', $post->id) + ->update([ + 'content' => htmlspecialchars_decode($post->content), + ]); + }); + + DB::table('private_messages') + ->lazyById() + ->each(function (object $privateMessage): void { + /** @var object{id: int, message: string} $privateMessage */ + DB::table('private_messages') + ->where('id', '=', $privateMessage->id) + ->update([ + 'message' => htmlspecialchars_decode($privateMessage->message), + ]); + }); + + DB::table('torrents') + ->lazyById() + ->each(function (object $torrent): void { + /** @var object{id: int, description: string} $torrent */ + DB::table('torrents') + ->where('id', '=', $torrent->id) + ->update([ + 'description' => htmlspecialchars_decode($torrent->description), + ]); + }); + + DB::table('requests') + ->lazyById() + ->each(function (object $request): void { + /** @var object{id: int, description: string} $request */ + DB::table('requests') + ->where('id', '=', $request->id) + ->update([ + 'description' => htmlspecialchars_decode($request->description), + ]); + }); + + DB::table('users') + ->lazyById() + ->each(function (object $user): void { + /** @var object{id: int, about: string, signature: string} $user */ + DB::table('users') + ->where('id', '=', $user->id) + ->update([ + 'about' => htmlspecialchars_decode($user->about), + 'signature' => htmlspecialchars_decode($user->signature), + ]); + }); + } +};