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:`).
HDInnovations · Jan 5, 2024 · 994edc7 · 994edc7
1 parent f91c1b2
commit 994edc7
Show file tree

Hide file tree

Showing 16 changed files with 133 additions and 412 deletions.
diff --git a/app/Helpers/Bbcode.php b/app/Helpers/Bbcode.php
@@ -148,19 +148,12 @@ class Bbcode
             'block'       => true,
         ],
         'namedquote' => [
-            'openBbcode'  => '/^\[quote=([^<>"]*?)\]/i',
+            'openBbcode'  => '/^\[quote=(.*?)\]/i',
             'closeBbcode' => '[/quote]',
             'openHtml'    => '<blockquote><i class="fas fa-quote-left"></i> <cite>Quoting $1:</cite><p>',
             'closeHtml'   => '</p></blockquote>',
             'block'       => true,
         ],
-        'namedlink' => [
-            'openBbcode'  => '/^\[url=(.*?)\]/i',
-            'closeBbcode' => '[/url]',
-            'openHtml'    => '<a href="$1">',
-            'closeHtml'   => '</a>',
-            'block'       => false,
-        ],
         'orderedlistnumerical' => [
             'openBbcode'  => '/^\[list=1\]/i',
             'closeBbcode' => '[/list]',
@@ -280,26 +273,34 @@ 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('[*]', '<li>', (string) $source);
+        $source = str_replace('[*]', '<li>', $source);
         $source = preg_replace_callback(
             '/\[url](.*?)\[\/url]/i',
-            static fn ($matches) => '<a href="'.htmlspecialchars($matches[1], ENT_QUOTES | ENT_HTML5).'">'.htmlspecialchars($matches[1], ENT_QUOTES | ENT_HTML5).'</a>',
+            fn ($matches) => '<a href="'.$this->sanitizeUrl($matches[1]).'">'.$this->sanitizeUrl($matches[1]).'</a>',
+            $source
+        );
+        $source = preg_replace_callback(
+            '/\[url=(.*?)](.*?)\[\/url]/i',
+            fn ($matches) => '<a href="'.$this->sanitizeUrl($matches[1]).'">'.$matches[2].'</a>',
             $source
         );
         $source = preg_replace_callback(
             '/\[img](.*?)\[\/img]/i',
-            static fn ($matches) => '<img src="'.htmlspecialchars($matches[1], ENT_QUOTES | ENT_HTML5).'" loading="lazy" class="img-responsive" style="display: inline !important;">',
+            fn ($matches) => '<img src="'.$this->sanitizeUrl($matches[1]).'" loading="lazy" class="img-responsive" style="display: inline !important;">',
             $source
         );
         $source = preg_replace_callback(
             '/\[img width=(\d+)](.*?)\[\/img]/i',
-            static fn ($matches) => '<img src="'.htmlspecialchars($matches[2], ENT_QUOTES | ENT_HTML5).'" loading="lazy" width="'.$matches[1].'px">',
+            fn ($matches) => '<img src="'.$this->sanitizeUrl($matches[2]).'" loading="lazy" width="'.$matches[1].'px">',
             $source
         );
         $source = preg_replace_callback(
             '/\[img=(\d+)(?:x\d+)?](.*?)\[\/img]/i',
-            static fn ($matches) => '<img src="'.htmlspecialchars($matches[2], ENT_QUOTES | ENT_HTML5).'" loading="lazy" width="'.$matches[1].'px">',
+            fn ($matches) => '<img src="'.$this->sanitizeUrl($matches[2]).'" loading="lazy" width="'.$matches[1].'px">',
             $source
         );
 
@@ -316,7 +317,7 @@ public function parse(?string $source, bool $replaceLineBreaks = true): string
             $source
         );
         $source = preg_replace_callback(
-            '/\[video="youtube"]([a-z0-9_-]{11})\[\/video]/i',
+            '/\[video=&quot;youtube&quot;]([a-z0-9_-]{11})\[\/video]/i',
             static fn ($matches) => '<iframe width="560" height="315" src="https://www.youtube-nocookie.com/embed/'.$matches[1].'?rel=0" allow="autoplay; encrypted-media" allowfullscreen></iframe>',
             $source
         );
@@ -328,7 +329,7 @@ public function parse(?string $source, bool $replaceLineBreaks = true): string
             static function ($matches) {
                 $comparates = preg_split('/\s*,\s*/', $matches[1]);
                 $urls = preg_split('/\s*(?:,|\s)\s*/', $matches[2]);
-                $validatedUrls = collect($urls)->filter(fn ($url) => filter_var($url, FILTER_VALIDATE_URL));
+                $validatedUrls = collect($urls)->map(fn ($url) => $this->sanitizeUrl($url));
                 $chunkedUrls = $validatedUrls->chunk(\count($comparates));
                 $html = view('partials.comparison', ['comparates' => $comparates, 'urls' => $chunkedUrls])->render();
                 $html = preg_replace('/\s+/', ' ', $html);
@@ -462,4 +463,26 @@ private function handleBlockElementSpacing(string &$source, int &$index, int $ta
             $index -= 1;
         }
     }
+
+    private function sanitizeUrl(string $url): string
+    {
+        // Do NOT add `javascript`, `data` or `vbscript` here
+        // or else you will allow an XSS vulnerability!
+        $protocolWhitelist = [
+            'http',
+            'https',
+            'irc',
+            'ftp',
+            'sftp',
+            'magnet',
+        ];
+
+        if (str_starts_with($url, '/')) {
+            $url = config('app.url').$url;
+        } elseif (!\in_array(parse_url($url, PHP_URL_SCHEME), $protocolWhitelist)) {
+            $url = 'https://'.$url;
+        }
+
+        return filter_var($url, FILTER_VALIDATE_URL) ?: 'Broken link';
+    }
 }
diff --git a/app/Http/Livewire/BbcodeInput.php b/app/Http/Livewire/BbcodeInput.php
@@ -35,7 +35,7 @@ final public function mount($name, $label, $required = false, $content = null):
         $this->name = $name;
         $this->label = $label;
         $this->isRequired = $required;
-        $this->contentBbcode = $content === null ? (old($name) ?? '') : htmlspecialchars_decode((string) $content);
+        $this->contentBbcode = $content ?? old($name) ?? '';
     }
 
     final public function updatedIsPreviewEnabled(): void

diff --git a/app/Http/Livewire/Comment.php b/app/Http/Livewire/Comment.php
@@ -31,7 +31,6 @@
 use App\Repositories\ChatRepository;
 use Illuminate\Support\Facades\Notification;
 use Livewire\Component;
-use voku\helper\AntiXSS;
 
 class Comment extends Component
 {
@@ -94,7 +93,7 @@ final public function updatedIsEditing($isEditing): void
     final public function editComment(): void
     {
         if (auth()->id() == $this->comment->user_id || auth()->user()->group->is_modo) {
-            $this->comment->update((new AntiXSS())->xss_clean($this->editState));
+            $this->comment->update($this->editState);
             $this->isEditing = false;
         } else {
             $this->dispatchBrowserEvent('error', ['type' => 'error',  'message' => 'Permission Denied!']);
@@ -130,7 +129,7 @@ final public function postReply(): void
             'replyState.content' => 'required',
         ]);
 
-        $reply = $this->comment->children()->make((new AntiXSS())->xss_clean($this->replyState));
+        $reply = $this->comment->children()->make($this->replyState);
         $reply->user()->associate(auth()->user());
         $reply->commentable()->associate($this->comment->commentable);
         $reply->anon = $this->anon;

diff --git a/app/Http/Livewire/Comments.php b/app/Http/Livewire/Comments.php
@@ -33,7 +33,6 @@
 use Illuminate\Support\Facades\Notification;
 use Livewire\Component;
 use Livewire\WithPagination;
-use voku\helper\AntiXSS;
 
 class Comments extends Component
 {
@@ -105,7 +104,7 @@ final public function postComment(): void
             'newCommentState.content' => 'required',
         ]);
 
-        $comment = $this->model->comments()->make((new AntiXSS())->xss_clean($this->newCommentState));
+        $comment = $this->model->comments()->make($this->newCommentState);
         $comment->user()->associate($this->user);
         $comment->anon = $this->anon;
         $comment->save();

diff --git a/app/Models/Article.php b/app/Models/Article.php
@@ -18,7 +18,6 @@
 use App\Traits\Auditable;
 use Illuminate\Database\Eloquent\Factories\HasFactory;
 use Illuminate\Database\Eloquent\Model;
-use voku\helper\AntiXSS;
 
 class Article extends Model
 {
@@ -82,14 +81,6 @@ public function getBrief(int $length = 20, bool $ellipses = true, bool $stripHtm
         return $trimmedText;
     }
 
-    /**
-     * Set The Articles Content After Its Been Purified.
-     */
-    public function setContentAttribute(?string $value): void
-    {
-        $this->attributes['content'] = 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
@@ -16,7 +16,6 @@
 use App\Helpers\Bbcode;
 use Illuminate\Database\Eloquent\Factories\HasFactory;
 use Illuminate\Database\Eloquent\Model;
-use voku\helper\AntiXSS;
 
 class Message extends Model
 {
@@ -75,14 +74,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
@@ -18,7 +18,6 @@
 use App\Traits\Auditable;
 use Illuminate\Database\Eloquent\Factories\HasFactory;
 use Illuminate\Database\Eloquent\Model;
-use voku\helper\AntiXSS;
 
 class Playlist extends Model
 {
@@ -58,14 +57,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'] = 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
@@ -18,7 +18,6 @@
 use App\Traits\Auditable;
 use Illuminate\Database\Eloquent\Factories\HasFactory;
 use Illuminate\Database\Eloquent\Model;
-use voku\helper\AntiXSS;
 
 class Post extends Model
 {
@@ -104,14 +103,6 @@ public function authorTopics(): \Illuminate\Database\Eloquent\Relations\HasMany
         return $this->hasMany(Topic::class, 'first_post_user_id', 'user_id');
     }
 
-    /**
-     * Set The Posts Content After Its Been Purified.
-     */
-    public function setContentAttribute(?string $value): void
-    {
-        $this->attributes['content'] = 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
@@ -17,7 +17,6 @@
 use App\Helpers\Linkify;
 use Illuminate\Database\Eloquent\Factories\HasFactory;
 use Illuminate\Database\Eloquent\Model;
-use voku\helper\AntiXSS;
 
 class PrivateMessage extends Model
 {
@@ -76,14 +75,6 @@ public function replyRecursive(): \Illuminate\Database\Eloquent\Relations\HasOne
         return $this->reply()->with('replyRecursive');
     }
 
-    /**
-     * 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
@@ -25,7 +25,6 @@
 use App\Traits\TorrentFilter;
 use Illuminate\Database\Eloquent\Factories\HasFactory;
 use Illuminate\Database\Eloquent\Model;
-use voku\helper\AntiXSS;
 
 /**
  * @property string $info_hash
@@ -318,14 +317,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'] = 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
@@ -19,7 +19,6 @@
 use App\Traits\TorrentFilter;
 use Illuminate\Database\Eloquent\Factories\HasFactory;
 use Illuminate\Database\Eloquent\Model;
-use voku\helper\AntiXSS;
 
 class TorrentRequest extends Model
 {
@@ -159,14 +158,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'] = 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
@@ -25,7 +25,6 @@
 use Illuminate\Foundation\Auth\User as Authenticatable;
 use Illuminate\Notifications\Notifiable;
 use Laravel\Fortify\TwoFactorAuthenticatable;
-use voku\helper\AntiXSS;
 
 class User extends Authenticatable implements MustVerifyEmail
 {
@@ -956,14 +955,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'] = htmlspecialchars((new AntiXSS())->xss_clean($value), ENT_NOQUOTES);
-    }
-
     /**
      * Returns the HTML of the user's signature.
      */
@@ -974,14 +965,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'] = htmlspecialchars((new AntiXSS())->xss_clean($value), ENT_NOQUOTES);
-    }
-
     /**
      * Parse About Me And Return Valid HTML.
      */

diff --git a/composer.json b/composer.json
@@ -36,7 +36,6 @@
         "spatie/ssl-certificate": "^2.6.2",
         "symfony/dom-crawler": "^6.4.0",
         "theodorejb/polycast": "dev-master",
-        "voku/anti-xss": "^4.1.42",
         "vstelmakh/url-highlight": "^3.0.3"
     },
     "require-dev": {