Skip to content

Commit 01dc1e7

Browse files
ssddanbrownssddanbrown
committed
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.
1 parent 59bbf50 commit 01dc1e7

7 files changed

Lines changed: 218 additions & 16 deletions

File tree

‎app/App/Providers/ValidationRuleServiceProvider.php‎

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
namespace BookStack\App\Providers;
44

55
use BookStack\Uploads\ImageService;
6+
use BookStack\Util\UrlFilter;
67
use Illuminate\Support\Facades\Validator;
78
use Illuminate\Support\ServiceProvider;
89

@@ -21,10 +22,8 @@ public function boot(): void
2122

2223
Validator::extend('safe_url', function ($attribute, $value, $parameters, $validator) {
2324
$cleanLinkName = strtolower(trim($value));
24-
$isJs = str_starts_with($cleanLinkName, 'javascript:');
25-
$isData = str_starts_with($cleanLinkName, 'data:');
26-
27-
return !$isJs && !$isData;
25+
$filter = new UrlFilter($cleanLinkName);
26+
return $filter->isAllowed();
2827
});
2928
}
3029
}

‎app/Uploads/Attachment.php‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010
use BookStack\Users\Models\HasCreatorAndUpdater;
1111
use BookStack\Users\Models\OwnableInterface;
1212
use BookStack\Users\Models\User;
13+
use BookStack\Util\UrlFilter;
1314
use Illuminate\Database\Eloquent\Builder;
1415
use Illuminate\Database\Eloquent\Factories\HasFactory;
1516
use Illuminate\Database\Eloquent\Relations\BelongsTo;
@@ -71,7 +72,7 @@ public function jointPermissions(): HasMany
7172
public function getUrl($openInline = false): string
7273
{
7374
if ($this->external && !str_starts_with($this->path, 'http')) {
74-
return $this->path;
75+
return (new UrlFilter($this->path))->clean();
7576
}
7677

7778
return url('/attachments/' . $this->id . ($openInline ? '?open=true' : ''));

‎app/Uploads/Controllers/AttachmentController.php‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111
use BookStack\Permissions\Permission;
1212
use BookStack\Uploads\Attachment;
1313
use BookStack\Uploads\AttachmentService;
14+
use BookStack\Util\UrlFilter;
1415
use Exception;
1516
use Illuminate\Contracts\Filesystem\FileNotFoundException;
1617
use Illuminate\Http\Request;
@@ -221,7 +222,8 @@ public function get(Request $request, string $attachmentId)
221222
}
222223

223224
if ($attachment->external) {
224-
return redirect($attachment->path);
225+
$url = (new UrlFilter($attachment->path))->clean();
226+
return redirect($url);
225227
}
226228

227229
$fileName = $attachment->getFileName();

‎app/Util/HtmlPurifier/ConfiguredHtmlPurifier.php‎

Lines changed: 8 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44

55
use BookStack\App\AppVersion;
66
use BookStack\Util\HtmlPurifier\Filters\UriLimitFileProtocolToAnchors;
7+
use BookStack\Util\UrlFilter;
78
use HTMLPurifier;
89
use HTMLPurifier_Config;
910
use HTMLPurifier_DefinitionCache_Serializer;
@@ -84,16 +85,13 @@ protected function setConfig(HTMLPurifier_Config $config, string $cachePath): vo
8485
$config->set('Attr.ID.HTML5', true);
8586
$config->set('Output.FixInnerHTML', false);
8687
$config->set('URI.SafeIframeRegexp', '%^(http://|https://|//)%');
87-
$config->set('URI.AllowedSchemes', [
88-
'http' => true,
89-
'https' => true,
90-
'mailto' => true,
91-
'ftp' => true,
92-
'nntp' => true,
93-
'news' => true,
94-
'tel' => true,
95-
'file' => true,
96-
]);
88+
89+
$allowedSchemes = UrlFilter::getAllowedSchemes();
90+
$allowedSchemesSetting = [];
91+
foreach ($allowedSchemes as $scheme) {
92+
$allowedSchemesSetting[$scheme] = true;
93+
}
94+
$config->set('URI.AllowedSchemes', $allowedSchemesSetting);
9795

9896
// $config->set('Cache.DefinitionImpl', null); // Disable cache during testing
9997
}

‎app/Util/UrlFilter.php‎

Lines changed: 103 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,103 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
namespace BookStack\Util;
6+
7+
/**
8+
* Helps filter URLs to prevent use of undesired schemes.
9+
* Also parses and rebuilds the URL to ensure it's valid.
10+
*/
11+
class UrlFilter
12+
{
13+
protected static array $allowedSchemes = ['http', 'https', 'mailto', 'tel', 'file', 'ftp', 'nntp', 'news'];
14+
protected string $url;
15+
16+
public function __construct(string $url)
17+
{
18+
$this->url = trim($url);
19+
}
20+
21+
/**
22+
* Check if the URL is allowed to be generally used as a link
23+
* in the application. This does not assure the original URL string
24+
* provided is safe as-is. Ensure you use the clean method to produce
25+
* a URL that is considered safe to use.
26+
*/
27+
public function isAllowed(): bool
28+
{
29+
$urlParts = parse_url($this->url);
30+
if (!$urlParts) {
31+
return false;
32+
}
33+
34+
// Extra check to help avoid scenarios where non-standard characters are used in the scheme
35+
// to work around parse_url handling with URLs which may be interpreted by the browser differently.
36+
if (str_contains($this->url, ':') && !preg_match('/^[a-z]+:/i', $this->url)) {
37+
return false;
38+
}
39+
40+
if (isset($urlParts['scheme'])) {
41+
return in_array(strtolower($urlParts['scheme']), self::$allowedSchemes);
42+
}
43+
44+
return true;
45+
}
46+
47+
/**
48+
* Clean the URL to ensure it's valid and only uses the allowed schemes.
49+
* If the URL is not allowed, return a placeholder.
50+
*/
51+
public function clean(): string
52+
{
53+
if (!$this->isAllowed()) {
54+
return '#badlink';
55+
}
56+
57+
$urlParts = parse_url($this->url);
58+
if (!$urlParts) {
59+
return '#badlink';
60+
}
61+
62+
$url = '';
63+
64+
if (isset($urlParts['scheme']) || isset($urlParts['host'])) {
65+
$scheme = strtolower($urlParts['scheme'] ?? 'https');
66+
$url = $scheme . ':' . (isset($urlParts['host']) ? '//' : '');
67+
}
68+
69+
if (isset($urlParts['user']) || isset($urlParts['pass'])) {
70+
$url .= $urlParts['user'] ?? '';
71+
if (isset($urlParts['pass'])) {
72+
$url .= ':' . $urlParts['pass'];
73+
}
74+
$url .= '@';
75+
}
76+
77+
if (isset($urlParts['host'])) {
78+
$url .= $urlParts['host'];
79+
}
80+
if (isset($urlParts['port'])) {
81+
$url .= ':' . $urlParts['port'];
82+
}
83+
if (isset($urlParts['path'])) {
84+
$url .= $urlParts['path'];
85+
}
86+
if (isset($urlParts['query'])) {
87+
$url .= '?' . $urlParts['query'];
88+
}
89+
if (isset($urlParts['fragment'])) {
90+
$url .= '#' . $urlParts['fragment'];
91+
}
92+
93+
return $url;
94+
}
95+
96+
/**
97+
* Get schemes that are allowed to be used in content links.
98+
*/
99+
public static function getAllowedSchemes(): array
100+
{
101+
return self::$allowedSchemes;
102+
}
103+
}

‎tests/Uploads/AttachmentTest.php‎

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -315,6 +315,34 @@ public function test_data_and_js_links_cannot_be_attached_to_a_page()
315315
}
316316
}
317317

318+
public function test_existing_data_and_js_links_do_not_render_link()
319+
{
320+
$this->asAdmin();
321+
$page = $this->entities->page();
322+
$attachment = Attachment::factory()->create(['uploaded_to' => $page->id]);
323+
324+
$links = [
325+
'javascript:alert("bunny")',
326+
' javascript:alert("bunny")',
327+
'JavaScript:alert("bunny")',
328+
"\t\n\t\nJavaScript:alert(\"bunny\")",
329+
'data:text/html;bunny<a></a>',
330+
'Data:text/html;bunny<a></a>',
331+
'Data:text/html;bunny<a></a>',
332+
'donk\tscript:alert("bunny")',
333+
"donk\tscript:alert('bunny')",
334+
];
335+
336+
foreach ($links as $link) {
337+
$attachment->path = $link;
338+
$attachment->save();
339+
340+
$resp = $this->get($page->getUrl());
341+
$resp->assertDontSee('bunny', false);
342+
$resp->assertSee('#badlink', false);
343+
}
344+
}
345+
318346
public function test_attachment_delete_only_shows_with_permission()
319347
{
320348
$this->asAdmin();

‎tests/Util/UrlFilterTest.php‎

Lines changed: 71 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,71 @@
1+
<?php
2+
3+
namespace Tests\Util;
4+
5+
use BookStack\Util\UrlFilter;
6+
use Tests\TestCase;
7+
8+
class UrlFilterTest extends TestCase
9+
{
10+
public function test_it_finds_invalid_urls()
11+
{
12+
$urls = [
13+
'javascript:alert("bunny")',
14+
' javascript:alert("bunny")',
15+
'JavaScript:alert("bunny")',
16+
"\t\n\t\nJavaScript:alert(\"bunny\")",
17+
'data:text/html;bunny<a></a>',
18+
'Data:text/html;bunny<a></a>',
19+
'Data:text/html;bunny<a></a>',
20+
"http://example.com\0javascript:alert(1)",
21+
];
22+
23+
foreach ($urls as $url) {
24+
$filter = new UrlFilter($url);
25+
$this->assertFalse($filter->isAllowed(), "Failed to detect invalid url: {$url}");
26+
}
27+
}
28+
29+
public function test_clean()
30+
{
31+
$expectedOutputByInput = [
32+
'javascript:alert("bunny")' => '#badlink',
33+
' javascript:alert("bunny")' => '#badlink',
34+
'JavaScript:alert("bunny")' => '#badlink',
35+
"\t\n\t\nJavaScript:alert(\"bunny\")" => '#badlink',
36+
'data:text/html;bunny<a></a>' => '#badlink',
37+
'Data:text/html;bunny<a></a>' => '#badlink',
38+
'Data:text/html;bunny<a></a>' => '#badlink',
39+
"http://example.com\0javascript:alert(1)" => '#badlink',
40+
"Java\tScript:alert(\"bunny\")" => '#badlink',
41+
42+
'https://example.com' => 'https://example.com',
43+
'https://example.com/a/b' => 'https://example.com/a/b',
44+
'https://example.com/a/b?a=b#ab' => 'https://example.com/a/b?a=b#ab',
45+
'https://example.com/a/b?a=b#ab&c=d' => 'https://example.com/a/b?a=b#ab&c=d',
46+
'https://example.com:5050' => 'https://example.com:5050',
47+
'https://example.com:5050/a/b' => 'https://example.com:5050/a/b',
48+
'https://example.com:5050/a/b?a=b#ab' => 'https://example.com:5050/a/b?a=b#ab',
49+
'https://user@example.com:5011/a/b?a=b' => 'https://user@example.com:5011/a/b?a=b',
50+
'https://user:pass@example.com:5011/a/b?a=b' => 'https://user:pass@example.com:5011/a/b?a=b',
51+
52+
'//example.com' => 'https://example.com',
53+
'a/b/c' => 'a/b/c',
54+
'/a/b/c' => '/a/b/c',
55+
56+
'tel:123456789' => 'tel:123456789',
57+
'TEL:123456789' => 'tel:123456789',
58+
'maiLto:a@b.c' => 'mailto:a@b.c',
59+
'file://a/b/c' => 'file://a/b/c',
60+
'ftp://a/b/c' => 'ftp://a/b/c',
61+
'nntp://a/b/c' => 'nntp://a/b/c',
62+
'news:a/b/c' => 'news:a/b/c',
63+
];
64+
65+
foreach ($expectedOutputByInput as $input => $expected) {
66+
$filter = new UrlFilter($input);
67+
$output = $filter->clean();
68+
$this->assertEquals($expected, $output, "Failed to clean url: {$input}");
69+
}
70+
}
71+
}

0 commit comments

Comments
 (0)