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 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:`).
  • Loading branch information
Roardom committed Mar 1, 2024
1 parent 1cd3251 commit 8ab3967
Show file tree
Hide file tree
Showing 15 changed files with 108 additions and 160 deletions.
9 changes: 6 additions & 3 deletions app/Helpers/Bbcode.php
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,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 @@ -275,8 +275,11 @@ 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',
fn ($matches) => '<a href="'.$this->sanitizeUrl($matches[1]).'">'.$this->sanitizeUrl($matches[1]).'</a>',
Expand Down Expand Up @@ -316,7 +319,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
2 changes: 1 addition & 1 deletion app/Http/Livewire/BbcodeInput.php
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,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
Expand Down
5 changes: 2 additions & 3 deletions app/Http/Livewire/Comment.php
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@
use App\Traits\CastLivewireProperties;
use Illuminate\Support\Facades\Notification;
use Livewire\Component;
use voku\helper\AntiXSS;

class Comment extends Component
{
Expand Down Expand Up @@ -102,7 +101,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!']);
Expand Down Expand Up @@ -138,7 +137,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;
Expand Down
3 changes: 1 addition & 2 deletions app/Http/Livewire/Comments.php
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,6 @@
use Illuminate\Support\Facades\Notification;
use Livewire\Component;
use Livewire\WithPagination;
use voku\helper\AntiXSS;

class Comments extends Component
{
Expand Down Expand Up @@ -113,7 +112,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();
Expand Down
9 changes: 0 additions & 9 deletions app/Models/Article.php
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,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 @@ -64,14 +63,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'] = 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 @@ -16,7 +16,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 @@ -87,14 +86,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 @@ -18,7 +18,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 @@ -73,14 +72,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.
*/
Expand Down
9 changes: 0 additions & 9 deletions app/Models/Post.php
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,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 @@ -146,14 +145,6 @@ public function scopeAuthorized(
);
}

/**
* 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.
*/
Expand Down
9 changes: 0 additions & 9 deletions app/Models/PrivateMessage.php
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,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 @@ -89,14 +88,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.
*/
Expand Down
9 changes: 0 additions & 9 deletions app/Models/Torrent.php
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@
use App\Traits\TorrentFilter;
use Illuminate\Database\Eloquent\Factories\HasFactory;
use Illuminate\Database\Eloquent\Model;
use voku\helper\AntiXSS;

/**
* App\Models\Torrent.
Expand Down Expand Up @@ -388,14 +387,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.
*/
Expand Down
9 changes: 0 additions & 9 deletions app/Models/TorrentRequest.php
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,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 @@ -187,14 +186,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.
*/
Expand Down
17 changes: 0 additions & 17 deletions app/Models/User.php
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,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 @@ -1037,14 +1036,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.
*/
Expand All @@ -1055,14 +1046,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.
*/
Expand Down
1 change: 0 additions & 1 deletion composer.json
Original file line number Diff line number Diff line change
Expand Up @@ -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": {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,98 @@
<?php

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('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),
]);
});
}
};
Loading

0 comments on commit 8ab3967

Please sign in to comment.