From 01dc1e71c5574ba36dabb55d9b2a3e4a234df2cc Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Tue, 30 Jun 2026 23:50:11 +0100 Subject: [PATCH] Attachments: Added more extensive URL filtering Added a central URLFilter class to check & clean URLs used for attachments, which is also used for validation, and by the purifier to standardise protocols (and to make protocol config easier in future). Thanks to mfk25 for reporting. --- .../ValidationRuleServiceProvider.php | 7 +- app/Uploads/Attachment.php | 3 +- .../Controllers/AttachmentController.php | 4 +- .../HtmlPurifier/ConfiguredHtmlPurifier.php | 18 ++- app/Util/UrlFilter.php | 103 ++++++++++++++++++ tests/Uploads/AttachmentTest.php | 28 +++++ tests/Util/UrlFilterTest.php | 71 ++++++++++++ 7 files changed, 218 insertions(+), 16 deletions(-) create mode 100644 app/Util/UrlFilter.php create mode 100644 tests/Util/UrlFilterTest.php diff --git a/app/App/Providers/ValidationRuleServiceProvider.php b/app/App/Providers/ValidationRuleServiceProvider.php index 1adc1ebd8..fc0302630 100644 --- a/app/App/Providers/ValidationRuleServiceProvider.php +++ b/app/App/Providers/ValidationRuleServiceProvider.php @@ -3,6 +3,7 @@ namespace BookStack\App\Providers; use BookStack\Uploads\ImageService; +use BookStack\Util\UrlFilter; use Illuminate\Support\Facades\Validator; use Illuminate\Support\ServiceProvider; @@ -21,10 +22,8 @@ class ValidationRuleServiceProvider extends ServiceProvider Validator::extend('safe_url', function ($attribute, $value, $parameters, $validator) { $cleanLinkName = strtolower(trim($value)); - $isJs = str_starts_with($cleanLinkName, 'javascript:'); - $isData = str_starts_with($cleanLinkName, 'data:'); - - return !$isJs && !$isData; + $filter = new UrlFilter($cleanLinkName); + return $filter->isAllowed(); }); } } diff --git a/app/Uploads/Attachment.php b/app/Uploads/Attachment.php index 05227243a..1619bebbc 100644 --- a/app/Uploads/Attachment.php +++ b/app/Uploads/Attachment.php @@ -10,6 +10,7 @@ use BookStack\Permissions\PermissionApplicator; use BookStack\Users\Models\HasCreatorAndUpdater; use BookStack\Users\Models\OwnableInterface; use BookStack\Users\Models\User; +use BookStack\Util\UrlFilter; use Illuminate\Database\Eloquent\Builder; use Illuminate\Database\Eloquent\Factories\HasFactory; use Illuminate\Database\Eloquent\Relations\BelongsTo; @@ -71,7 +72,7 @@ class Attachment extends Model implements OwnableInterface public function getUrl($openInline = false): string { if ($this->external && !str_starts_with($this->path, 'http')) { - return $this->path; + return (new UrlFilter($this->path))->clean(); } return url('/attachments/' . $this->id . ($openInline ? '?open=true' : '')); diff --git a/app/Uploads/Controllers/AttachmentController.php b/app/Uploads/Controllers/AttachmentController.php index bdd96d6ad..aa9e0e291 100644 --- a/app/Uploads/Controllers/AttachmentController.php +++ b/app/Uploads/Controllers/AttachmentController.php @@ -11,6 +11,7 @@ use BookStack\Http\Controller; use BookStack\Permissions\Permission; use BookStack\Uploads\Attachment; use BookStack\Uploads\AttachmentService; +use BookStack\Util\UrlFilter; use Exception; use Illuminate\Contracts\Filesystem\FileNotFoundException; use Illuminate\Http\Request; @@ -221,7 +222,8 @@ class AttachmentController extends Controller } if ($attachment->external) { - return redirect($attachment->path); + $url = (new UrlFilter($attachment->path))->clean(); + return redirect($url); } $fileName = $attachment->getFileName(); diff --git a/app/Util/HtmlPurifier/ConfiguredHtmlPurifier.php b/app/Util/HtmlPurifier/ConfiguredHtmlPurifier.php index 173935903..221db6ffc 100644 --- a/app/Util/HtmlPurifier/ConfiguredHtmlPurifier.php +++ b/app/Util/HtmlPurifier/ConfiguredHtmlPurifier.php @@ -4,6 +4,7 @@ namespace BookStack\Util\HtmlPurifier; use BookStack\App\AppVersion; use BookStack\Util\HtmlPurifier\Filters\UriLimitFileProtocolToAnchors; +use BookStack\Util\UrlFilter; use HTMLPurifier; use HTMLPurifier_Config; use HTMLPurifier_DefinitionCache_Serializer; @@ -84,16 +85,13 @@ class ConfiguredHtmlPurifier $config->set('Attr.ID.HTML5', true); $config->set('Output.FixInnerHTML', false); $config->set('URI.SafeIframeRegexp', '%^(http://|https://|//)%'); - $config->set('URI.AllowedSchemes', [ - 'http' => true, - 'https' => true, - 'mailto' => true, - 'ftp' => true, - 'nntp' => true, - 'news' => true, - 'tel' => true, - 'file' => true, - ]); + + $allowedSchemes = UrlFilter::getAllowedSchemes(); + $allowedSchemesSetting = []; + foreach ($allowedSchemes as $scheme) { + $allowedSchemesSetting[$scheme] = true; + } + $config->set('URI.AllowedSchemes', $allowedSchemesSetting); // $config->set('Cache.DefinitionImpl', null); // Disable cache during testing } diff --git a/app/Util/UrlFilter.php b/app/Util/UrlFilter.php new file mode 100644 index 000000000..a3c65d03e --- /dev/null +++ b/app/Util/UrlFilter.php @@ -0,0 +1,103 @@ +url = trim($url); + } + + /** + * Check if the URL is allowed to be generally used as a link + * in the application. This does not assure the original URL string + * provided is safe as-is. Ensure you use the clean method to produce + * a URL that is considered safe to use. + */ + public function isAllowed(): bool + { + $urlParts = parse_url($this->url); + if (!$urlParts) { + return false; + } + + // Extra check to help avoid scenarios where non-standard characters are used in the scheme + // to work around parse_url handling with URLs which may be interpreted by the browser differently. + if (str_contains($this->url, ':') && !preg_match('/^[a-z]+:/i', $this->url)) { + return false; + } + + if (isset($urlParts['scheme'])) { + return in_array(strtolower($urlParts['scheme']), self::$allowedSchemes); + } + + return true; + } + + /** + * Clean the URL to ensure it's valid and only uses the allowed schemes. + * If the URL is not allowed, return a placeholder. + */ + public function clean(): string + { + if (!$this->isAllowed()) { + return '#badlink'; + } + + $urlParts = parse_url($this->url); + if (!$urlParts) { + return '#badlink'; + } + + $url = ''; + + if (isset($urlParts['scheme']) || isset($urlParts['host'])) { + $scheme = strtolower($urlParts['scheme'] ?? 'https'); + $url = $scheme . ':' . (isset($urlParts['host']) ? '//' : ''); + } + + if (isset($urlParts['user']) || isset($urlParts['pass'])) { + $url .= $urlParts['user'] ?? ''; + if (isset($urlParts['pass'])) { + $url .= ':' . $urlParts['pass']; + } + $url .= '@'; + } + + if (isset($urlParts['host'])) { + $url .= $urlParts['host']; + } + if (isset($urlParts['port'])) { + $url .= ':' . $urlParts['port']; + } + if (isset($urlParts['path'])) { + $url .= $urlParts['path']; + } + if (isset($urlParts['query'])) { + $url .= '?' . $urlParts['query']; + } + if (isset($urlParts['fragment'])) { + $url .= '#' . $urlParts['fragment']; + } + + return $url; + } + + /** + * Get schemes that are allowed to be used in content links. + */ + public static function getAllowedSchemes(): array + { + return self::$allowedSchemes; + } +} diff --git a/tests/Uploads/AttachmentTest.php b/tests/Uploads/AttachmentTest.php index 22e4f4610..3f8a2aaf1 100644 --- a/tests/Uploads/AttachmentTest.php +++ b/tests/Uploads/AttachmentTest.php @@ -315,6 +315,34 @@ class AttachmentTest extends TestCase } } + public function test_existing_data_and_js_links_do_not_render_link() + { + $this->asAdmin(); + $page = $this->entities->page(); + $attachment = Attachment::factory()->create(['uploaded_to' => $page->id]); + + $links = [ + 'javascript:alert("bunny")', + ' javascript:alert("bunny")', + 'JavaScript:alert("bunny")', + "\t\n\t\nJavaScript:alert(\"bunny\")", + 'data:text/html;bunny', + 'Data:text/html;bunny', + 'Data:text/html;bunny', + 'donk\tscript:alert("bunny")', + "donk\tscript:alert('bunny')", + ]; + + foreach ($links as $link) { + $attachment->path = $link; + $attachment->save(); + + $resp = $this->get($page->getUrl()); + $resp->assertDontSee('bunny', false); + $resp->assertSee('#badlink', false); + } + } + public function test_attachment_delete_only_shows_with_permission() { $this->asAdmin(); diff --git a/tests/Util/UrlFilterTest.php b/tests/Util/UrlFilterTest.php new file mode 100644 index 000000000..8811b46d9 --- /dev/null +++ b/tests/Util/UrlFilterTest.php @@ -0,0 +1,71 @@ +', + 'Data:text/html;bunny', + 'Data:text/html;bunny', + "http://example.com\0javascript:alert(1)", + ]; + + foreach ($urls as $url) { + $filter = new UrlFilter($url); + $this->assertFalse($filter->isAllowed(), "Failed to detect invalid url: {$url}"); + } + } + + public function test_clean() + { + $expectedOutputByInput = [ + 'javascript:alert("bunny")' => '#badlink', + ' javascript:alert("bunny")' => '#badlink', + 'JavaScript:alert("bunny")' => '#badlink', + "\t\n\t\nJavaScript:alert(\"bunny\")" => '#badlink', + 'data:text/html;bunny' => '#badlink', + 'Data:text/html;bunny' => '#badlink', + 'Data:text/html;bunny' => '#badlink', + "http://example.com\0javascript:alert(1)" => '#badlink', + "Java\tScript:alert(\"bunny\")" => '#badlink', + + 'https://example.com' => 'https://example.com', + 'https://example.com/a/b' => 'https://example.com/a/b', + 'https://example.com/a/b?a=b#ab' => 'https://example.com/a/b?a=b#ab', + 'https://example.com/a/b?a=b#ab&c=d' => 'https://example.com/a/b?a=b#ab&c=d', + 'https://example.com:5050' => 'https://example.com:5050', + 'https://example.com:5050/a/b' => 'https://example.com:5050/a/b', + 'https://example.com:5050/a/b?a=b#ab' => 'https://example.com:5050/a/b?a=b#ab', + 'https://user@example.com:5011/a/b?a=b' => 'https://user@example.com:5011/a/b?a=b', + 'https://user:pass@example.com:5011/a/b?a=b' => 'https://user:pass@example.com:5011/a/b?a=b', + + '//example.com' => 'https://example.com', + 'a/b/c' => 'a/b/c', + '/a/b/c' => '/a/b/c', + + 'tel:123456789' => 'tel:123456789', + 'TEL:123456789' => 'tel:123456789', + 'maiLto:a@b.c' => 'mailto:a@b.c', + 'file://a/b/c' => 'file://a/b/c', + 'ftp://a/b/c' => 'ftp://a/b/c', + 'nntp://a/b/c' => 'nntp://a/b/c', + 'news:a/b/c' => 'news:a/b/c', + ]; + + foreach ($expectedOutputByInput as $input => $expected) { + $filter = new UrlFilter($input); + $output = $filter->clean(); + $this->assertEquals($expected, $output, "Failed to clean url: {$input}"); + } + } +}