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}"); + } + } +}