Skip to content

Commit

Permalink
update: remove XSS cleaner and remove XSS vulnerabilities
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
Roardom committed Jul 3, 2024
1 parent ddea582 commit 011d07e
Show file tree
Hide file tree
Showing 13 changed files with 118 additions and 95 deletions.
7 changes: 5 additions & 2 deletions app/Helpers/Bbcode.php
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,7 @@ 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>',
Expand Down Expand Up @@ -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('[*]', '<li>', (string) $source);
$source = preg_replace_callback(
Expand Down Expand Up @@ -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=&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 ?? ''
);
Expand Down
5 changes: 2 additions & 3 deletions app/Http/Livewire/BbcodeInput.php
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@

use App\Helpers\Bbcode;
use Livewire\Component;
use voku\helper\AntiXSS;

class BbcodeInput extends Component
{
Expand All @@ -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);
}
}

Expand Down
9 changes: 0 additions & 9 deletions app/Models/Article.php
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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.
*/
Expand Down
9 changes: 0 additions & 9 deletions app/Models/Comment.php
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@
use Illuminate\Database\Eloquent\Builder;
use Illuminate\Database\Eloquent\Factories\HasFactory;
use Illuminate\Database\Eloquent\Model;
use voku\helper\AntiXSS;

/**
* App\Models\Comment.
Expand Down Expand Up @@ -109,14 +108,6 @@ public function scopeParent(Builder $builder): void
$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.
*/
Expand Down
9 changes: 0 additions & 9 deletions app/Models/Message.php
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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.
*/
Expand Down
9 changes: 0 additions & 9 deletions app/Models/Playlist.php
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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.
*/
Expand Down
9 changes: 0 additions & 9 deletions app/Models/Post.php
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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.
*/
Expand Down
9 changes: 0 additions & 9 deletions app/Models/PrivateMessage.php
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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.
*/
Expand Down
9 changes: 0 additions & 9 deletions app/Models/Torrent.php
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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.
*/
Expand Down
9 changes: 0 additions & 9 deletions app/Models/TorrentRequest.php
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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.
*/
Expand Down
17 changes: 0 additions & 17 deletions app/Models/User.php
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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.
*/
Expand All @@ -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.
*/
Expand Down
1 change: 0 additions & 1 deletion composer.json
Original file line number Diff line number Diff line change
Expand Up @@ -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": {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,111 @@
<?php

declare(strict_types=1);

use Illuminate\Database\Migrations\Migration;
use Illuminate\Support\Facades\DB;

return new class () extends Migration {
public function up(): void
{
DB::table('articles')
->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('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),
]);
});
}
};

0 comments on commit 011d07e

Please sign in to comment.