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.
2026-07-15 21:31:36 +03:00 · 2026-06-30 23:50:11 +01:00
parent 59bbf504cf
commit 01dc1e71c5
7 changed files with 218 additions and 16 deletions
--- 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();
        });
    }
 }
--- 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' : ''));
--- 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();
--- 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
    }
--- a/app/Util/UrlFilter.php
+++ b/app/Util/UrlFilter.php
@@ -0,0 +1,103 @@
+<?php
+
+declare(strict_types=1);
+
+namespace BookStack\Util;
+
+/**
+ * Helps filter URLs to prevent use of undesired schemes.
+ * Also parses and rebuilds the URL to ensure it's valid.
+ */
+class UrlFilter
+{
+    protected static array $allowedSchemes = ['http', 'https', 'mailto', 'tel', 'file', 'ftp', 'nntp', 'news'];
+    protected string $url;
+
+    public function __construct(string $url)
+    {
+        $this->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;
+    }
+}
--- 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<a></a>',
+            'Data:text/html;bunny<a></a>',
+            'Data:text/html;bunny<a></a>',
+            '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();
--- a/tests/Util/UrlFilterTest.php
+++ b/tests/Util/UrlFilterTest.php
@@ -0,0 +1,71 @@
+<?php
+
+namespace Tests\Util;
+
+use BookStack\Util\UrlFilter;
+use Tests\TestCase;
+
+class UrlFilterTest extends TestCase
+{
+    public function test_it_finds_invalid_urls()
+    {
+        $urls = [
+            'javascript:alert("bunny")',
+            ' javascript:alert("bunny")',
+            'JavaScript:alert("bunny")',
+            "\t\n\t\nJavaScript:alert(\"bunny\")",
+            'data:text/html;bunny<a></a>',
+            'Data:text/html;bunny<a></a>',
+            'Data:text/html;bunny<a></a>',
+            "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<a></a>' => '#badlink',
+            'Data:text/html;bunny<a></a>' => '#badlink',
+            'Data:text/html;bunny<a></a>' => '#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}");
+        }
+    }
+}