From ad716d7cc3d5eae8fdf3efe02e243dc1776dd437 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 sanitizing the untrusted urls and whitelisting their protocol. --- app/Helpers/Bbcode.php | 7 +- app/Helpers/Linkify.php | 9 +- app/Http/Livewire/BbcodeInput.php | 5 +- app/Http/Resources/ChatMessageResource.php | 12 +- app/Models/Article.php | 9 -- app/Models/Comment.php | 9 -- app/Models/Message.php | 9 -- app/Models/Note.php | 9 -- app/Models/Playlist.php | 9 -- app/Models/Post.php | 9 -- app/Models/PrivateMessage.php | 9 -- app/Models/TicketNote.php | 9 -- app/Models/Torrent.php | 9 -- app/Models/TorrentRequest.php | 9 -- app/Models/User.php | 17 --- composer.json | 1 - ..._085223_htmlspecialchars_decode_bbcode.php | 133 ++++++++++++++++++ phpstan-baseline.neon | 5 - 18 files changed, 152 insertions(+), 127 deletions(-) create mode 100644 database/migrations/2024_07_03_085223_htmlspecialchars_decode_bbcode.php diff --git a/app/Helpers/Bbcode.php b/app/Helpers/Bbcode.php index fae28105e..2a2ba1bae 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 = str_replace('[hr]', '
    ', $source); @@ -320,7 +323,7 @@ class Bbcode $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/Helpers/Linkify.php b/app/Helpers/Linkify.php index d1a9293a2..0c69d4cc2 100644 --- a/app/Helpers/Linkify.php +++ b/app/Helpers/Linkify.php @@ -27,7 +27,14 @@ class Linkify $validator = new Validator( false, // bool - if should use top level domain to match urls without scheme [], // string[] - array of blacklisted schemes - [], // string[] - array of whitelisted schemes + [ + 'http', + 'https', + 'irc', + 'ftp', + 'sftp', + 'magnet', + ], // string[] - array of whitelisted schemes true // bool - if should match emails (if match by TLD set to "false" - will match only "mailto" urls) ); diff --git a/app/Http/Livewire/BbcodeInput.php b/app/Http/Livewire/BbcodeInput.php index 63a95655d..08b15c74b 100644 --- a/app/Http/Livewire/BbcodeInput.php +++ b/app/Http/Livewire/BbcodeInput.php @@ -18,7 +18,6 @@ namespace App\Http\Livewire; use App\Helpers\Bbcode; use Livewire\Component; -use voku\helper\AntiXSS; class BbcodeInput extends Component { @@ -39,13 +38,13 @@ class BbcodeInput extends Component $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/Http/Resources/ChatMessageResource.php b/app/Http/Resources/ChatMessageResource.php index 526ebfdb6..fb6048383 100644 --- a/app/Http/Resources/ChatMessageResource.php +++ b/app/Http/Resources/ChatMessageResource.php @@ -19,7 +19,6 @@ namespace App\Http\Resources; use App\Helpers\Bbcode; use hdvinnie\LaravelJoyPixels\LaravelJoyPixels; use Illuminate\Http\Resources\Json\JsonResource; -use voku\helper\AntiXSS; /** * @mixin \App\Models\Message @@ -33,17 +32,14 @@ class ChatMessageResource extends JsonResource */ public function toArray($request): array { - $emojiOne = app()->make(LaravelJoyPixels::class); + $emojiOne = new LaravelJoyPixels(); $bbcode = new Bbcode(); + $logger = $bbcode->parse($this->message); + $logger = $emojiOne->toImage($logger); if ($this->user_id == 1) { - $logger = $bbcode->parse('
    '.$this->message.'
    '); - $logger = $emojiOne->toImage($logger); $logger = str_replace('a href="/#', 'a trigger="bot" class="chatTrigger" href="/#', $logger); - } else { - $logger = $bbcode->parse('
    '.$this->message.'
    '); - $logger = $emojiOne->toImage($logger); } return [ @@ -52,7 +48,7 @@ class ChatMessageResource extends JsonResource 'user' => new ChatUserResource($this->whenLoaded('user')), 'receiver' => new ChatUserResource($this->whenLoaded('receiver')), 'chatroom' => new ChatRoomResource($this->whenLoaded('chatroom')), - 'message' => (new AntiXSS())->xss_clean($logger), + 'message' => $logger, 'created_at' => $this->created_at->toIso8601String(), 'updated_at' => $this->updated_at->toIso8601String(), ]; diff --git a/app/Models/Article.php b/app/Models/Article.php index b6d200a88..1423f235a 100644 --- a/app/Models/Article.php +++ b/app/Models/Article.php @@ -21,7 +21,6 @@ use App\Helpers\Linkify; use App\Traits\Auditable; use Illuminate\Database\Eloquent\Factories\HasFactory; use Illuminate\Database\Eloquent\Model; -use voku\helper\AntiXSS; /** * App\Models\Article. @@ -69,14 +68,6 @@ class Article extends Model 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/Comment.php b/app/Models/Comment.php index a5b98fed5..b4ac4b419 100644 --- a/app/Models/Comment.php +++ b/app/Models/Comment.php @@ -23,7 +23,6 @@ use App\Traits\Auditable; use Illuminate\Database\Eloquent\Builder; use Illuminate\Database\Eloquent\Factories\HasFactory; use Illuminate\Database\Eloquent\Model; -use voku\helper\AntiXSS; /** * App\Models\Comment. @@ -111,14 +110,6 @@ class Comment extends Model $builder->whereNull('parent_id'); } - /** - * 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 138f7608c..b78a1481b 100644 --- a/app/Models/Message.php +++ b/app/Models/Message.php @@ -19,7 +19,6 @@ namespace App\Models; use App\Helpers\Bbcode; use Illuminate\Database\Eloquent\Factories\HasFactory; use Illuminate\Database\Eloquent\Model; -use voku\helper\AntiXSS; /** * App\Models\Message. @@ -91,14 +90,6 @@ class Message extends Model 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/Note.php b/app/Models/Note.php index 196389ae2..6ab03f11b 100644 --- a/app/Models/Note.php +++ b/app/Models/Note.php @@ -20,7 +20,6 @@ use App\Helpers\Linkify; use App\Traits\Auditable; use Illuminate\Database\Eloquent\Factories\HasFactory; use Illuminate\Database\Eloquent\Model; -use voku\helper\AntiXSS; /** * App\Models\Note. @@ -79,14 +78,6 @@ class Note extends Model ]); } - /** - * Set Message After It's Been Purified. - */ - public function setMessageAttribute(?string $value): void - { - $this->attributes['message'] = $value === null ? null : htmlspecialchars((new AntiXSS())->xss_clean($value), ENT_NOQUOTES); - } - /** * Parse Message And Return Valid HTML. */ diff --git a/app/Models/Playlist.php b/app/Models/Playlist.php index b5d7c8ef3..9348ecc26 100644 --- a/app/Models/Playlist.php +++ b/app/Models/Playlist.php @@ -21,7 +21,6 @@ use App\Helpers\Linkify; use App\Traits\Auditable; use Illuminate\Database\Eloquent\Factories\HasFactory; use Illuminate\Database\Eloquent\Model; -use voku\helper\AntiXSS; /** * App\Models\Playlist. @@ -78,14 +77,6 @@ class Playlist extends Model 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 b8685614d..1fde47741 100644 --- a/app/Models/Post.php +++ b/app/Models/Post.php @@ -21,7 +21,6 @@ use App\Helpers\Linkify; use App\Traits\Auditable; use Illuminate\Database\Eloquent\Factories\HasFactory; use Illuminate\Database\Eloquent\Model; -use voku\helper\AntiXSS; /** * App\Models\Post. @@ -156,14 +155,6 @@ class Post extends Model ); } - /** - * 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 ddcc14a6a..ece0f05ed 100644 --- a/app/Models/PrivateMessage.php +++ b/app/Models/PrivateMessage.php @@ -20,7 +20,6 @@ use App\Helpers\Bbcode; use App\Helpers\Linkify; use Illuminate\Database\Eloquent\Factories\HasFactory; use Illuminate\Database\Eloquent\Model; -use voku\helper\AntiXSS; /** * App\Models\PrivateMessage. @@ -64,14 +63,6 @@ class PrivateMessage extends Model 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/TicketNote.php b/app/Models/TicketNote.php index 07a8c2e15..6014cf38c 100644 --- a/app/Models/TicketNote.php +++ b/app/Models/TicketNote.php @@ -18,7 +18,6 @@ namespace App\Models; use App\Helpers\Linkify; use Illuminate\Database\Eloquent\Model; -use voku\helper\AntiXSS; /** * App\Models\TicketNote. @@ -59,14 +58,6 @@ class TicketNote extends Model return $this->belongsTo(Ticket::class); } - /** - * Set Message After It's Been Purified. - */ - public function setMessageAttribute(?string $value): void - { - $this->attributes['message'] = $value === null ? null : htmlspecialchars((new AntiXSS())->xss_clean($value), ENT_NOQUOTES); - } - /** * Parse Message And Return Valid HTML. */ diff --git a/app/Models/Torrent.php b/app/Models/Torrent.php index 0230b3dfa..f2eb1694f 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 Laravel\Scout\Searchable; -use voku\helper\AntiXSS; /** * App\Models\Torrent. @@ -751,14 +750,6 @@ class Torrent extends Model return $this->hasOne(TorrentTrump::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 269f189c2..4eda9aae1 100644 --- a/app/Models/TorrentRequest.php +++ b/app/Models/TorrentRequest.php @@ -21,7 +21,6 @@ use App\Helpers\Linkify; use App\Traits\Auditable; use Illuminate\Database\Eloquent\Factories\HasFactory; use Illuminate\Database\Eloquent\Model; -use voku\helper\AntiXSS; /** * App\Models\TorrentRequest. @@ -215,14 +214,6 @@ class TorrentRequest extends Model 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 3d3c24c0c..293b0e8dc 100644 --- a/app/Models/User.php +++ b/app/Models/User.php @@ -28,7 +28,6 @@ use Illuminate\Database\Eloquent\SoftDeletes; use Illuminate\Foundation\Auth\User as Authenticatable; use Illuminate\Notifications\Notifiable; use Laravel\Fortify\TwoFactorAuthenticatable; -use voku\helper\AntiXSS; /** * App\Models\User. @@ -1181,14 +1180,6 @@ class User extends Authenticatable implements MustVerifyEmail 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. */ @@ -1199,14 +1190,6 @@ class User extends Authenticatable implements MustVerifyEmail 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 7afe942aa..6a194b4d1 100644 --- a/composer.json +++ b/composer.json @@ -41,7 +41,6 @@ "spatie/ssl-certificate": "^2.6.8", "symfony/dom-crawler": "^6.4.16", "theodorejb/polycast": "dev-master", - "voku/anti-xss": "^4.1.42", "vstelmakh/url-highlight": "^3.1.1" }, "require-dev": { diff --git a/database/migrations/2024_07_03_085223_htmlspecialchars_decode_bbcode.php b/database/migrations/2024_07_03_085223_htmlspecialchars_decode_bbcode.php new file mode 100644 index 000000000..d20148117 --- /dev/null +++ b/database/migrations/2024_07_03_085223_htmlspecialchars_decode_bbcode.php @@ -0,0 +1,133 @@ +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('comments') + ->lazyById() + ->each(function (object $comment): void { + /** @var object{id: int, content: string} $comment */ + DB::table('comments') + ->where('id', '=', $comment->id) + ->update([ + 'content' => htmlspecialchars_decode($comment->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('user_notes') + ->lazyById() + ->each(function (object $userNote): void { + /** @var object{id: int, message: string} $userNote */ + DB::table('user_notes') + ->where('id', '=', $userNote->id) + ->update([ + 'message' => htmlspecialchars_decode($userNote->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('ticket_notes') + ->lazyById() + ->each(function (object $ticketNote): void { + /** @var object{id: int, message: string} $ticketNote */ + DB::table('ticket_notes') + ->where('id', '=', $ticketNote->id) + ->update([ + 'message' => htmlspecialchars_decode($ticketNote->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), + ]); + }); + } +}; diff --git a/phpstan-baseline.neon b/phpstan-baseline.neon index 9fac5d078..459ac683b 100644 --- a/phpstan-baseline.neon +++ b/phpstan-baseline.neon @@ -470,11 +470,6 @@ parameters: count: 1 path: app/Http/Resources/ChatMessageResource.php - - - message: "#^Unable to resolve the template type TXssCleanInput in call to method voku\\\\helper\\\\AntiXSS\\:\\:xss_clean\\(\\)$#" - count: 1 - path: app/Http/Resources/ChatMessageResource.php - - message: "#^Method App\\\\Http\\\\Resources\\\\ChatRoomResource\\:\\:toArray\\(\\) return type has no value type specified in iterable type array\\.$#" count: 1