diff --git a/app/Access/Controllers/LoginController.php b/app/Access/Controllers/LoginController.php index 4694f22e4..fece3d880 100644 --- a/app/Access/Controllers/LoginController.php +++ b/app/Access/Controllers/LoginController.php @@ -8,6 +8,7 @@ use BookStack\Exceptions\LoginAttemptEmailNeededException; use BookStack\Exceptions\LoginAttemptException; use BookStack\Facades\Activity; use BookStack\Http\Controller; +use BookStack\Util\UrlComparison; use Illuminate\Http\RedirectResponse; use Illuminate\Http\Request; use Illuminate\Validation\ValidationException; @@ -186,7 +187,8 @@ class LoginController extends Controller { // Store the previous location for redirect after login $previous = url()->previous(''); - $isPreviousFromInstance = str_starts_with($previous, url('/')); + $comparison = new UrlComparison($previous, url('/')); + $isPreviousFromInstance = $comparison->originsMatch() && $comparison->pathsOverlap(); if (!$previous || !setting('app-public') || !$isPreviousFromInstance) { return; } diff --git a/app/Activity/Controllers/CommentController.php b/app/Activity/Controllers/CommentController.php index f61a2c8df..8474d9eb1 100644 --- a/app/Activity/Controllers/CommentController.php +++ b/app/Activity/Controllers/CommentController.php @@ -59,8 +59,7 @@ class CommentController extends Controller 'html' => ['required', 'string'], ]); - $comment = $this->commentRepo->getById($commentId); - $this->checkOwnablePermission(Permission::PageView, $comment->entity); + $comment = $this->commentRepo->getVisibleById($commentId); $this->checkOwnablePermission(Permission::CommentUpdate, $comment); $comment = $this->commentRepo->update($comment, $input['html']); @@ -76,8 +75,7 @@ class CommentController extends Controller */ public function archive(int $id) { - $comment = $this->commentRepo->getById($id); - $this->checkOwnablePermission(Permission::PageView, $comment->entity); + $comment = $this->commentRepo->getVisibleById($id); if (!userCan(Permission::CommentUpdate, $comment) && !userCan(Permission::CommentDelete, $comment)) { $this->showPermissionError(); } @@ -96,8 +94,7 @@ class CommentController extends Controller */ public function unarchive(int $id) { - $comment = $this->commentRepo->getById($id); - $this->checkOwnablePermission(Permission::PageView, $comment->entity); + $comment = $this->commentRepo->getVisibleById($id); if (!userCan(Permission::CommentUpdate, $comment) && !userCan(Permission::CommentDelete, $comment)) { $this->showPermissionError(); } @@ -116,7 +113,7 @@ class CommentController extends Controller */ public function destroy(int $id) { - $comment = $this->commentRepo->getById($id); + $comment = $this->commentRepo->getVisibleById($id); $this->checkOwnablePermission(Permission::CommentDelete, $comment); $this->commentRepo->delete($comment); 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/Console/Commands/InstallModuleCommand.php b/app/Console/Commands/InstallModuleCommand.php index 114bfb105..8e7747455 100644 --- a/app/Console/Commands/InstallModuleCommand.php +++ b/app/Console/Commands/InstallModuleCommand.php @@ -7,6 +7,7 @@ use BookStack\Theming\ThemeModule; use BookStack\Theming\ThemeModuleException; use BookStack\Theming\ThemeModuleManager; use BookStack\Theming\ThemeModuleZip; +use BookStack\Util\UrlComparison; use GuzzleHttp\Psr7\Request; use Illuminate\Console\Command; use Illuminate\Support\Str; @@ -199,7 +200,6 @@ class InstallModuleCommand extends Command { $httpRequests = app()->make(HttpRequestService::class); $client = $httpRequests->buildClient(30, ['stream' => true]); - $originalUrl = parse_url($location); $currentLocation = $location; $maxRedirects = 3; $redirectCount = 0; @@ -212,12 +212,11 @@ class InstallModuleCommand extends Command if ($statusCode >= 300 && $statusCode < 400 && $redirectCount < $maxRedirects) { $redirectLocation = $resp->getHeaderLine('Location'); if ($redirectLocation) { - $redirectUrl = parse_url($redirectLocation); - $redirectOriginMatches = ($originalUrl['host'] ?? '') === ($redirectUrl['host'] ?? '') - && ($originalUrl['scheme'] ?? '') === ($redirectUrl['scheme'] ?? '') - && ($originalUrl['port'] ?? '') === ($redirectUrl['port'] ?? ''); + $comparison = new UrlComparison($location, $redirectLocation); + $redirectOriginMatches = $comparison->originsMatch(); if (!$redirectOriginMatches) { + $redirectUrl = parse_url($redirectLocation); $redirectOrigin = ($redirectUrl['scheme'] ?? '') . '://' . ($redirectUrl['host'] ?? '') . (isset($redirectUrl['port']) ? ':' . $redirectUrl['port'] : ''); $this->info("The download URL is redirecting to a different site: {$redirectOrigin}"); $shouldContinue = $this->confirm("Do you trust downloading the module from this site?"); 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 87ed5add2..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 } @@ -156,6 +154,10 @@ class ConfiguredHtmlPurifier // Allow mention-ids on links $definition->addAttribute('a', 'data-mention-user-id', 'Number'); + + // Set up custom handler for srcset to limit accepted types + $definition->addAttribute('img', 'srcset', new SrcsetAttrDef()); + $definition->addAttribute('source', 'srcset', new SrcsetAttrDef()); } protected function configureUriDefinition(HTMLPurifier_URIDefinition $definition): void diff --git a/app/Util/HtmlPurifier/Filters/UriLimitFileProtocolToAnchors.php b/app/Util/HtmlPurifier/Filters/UriLimitFileProtocolToAnchors.php index 19ca9cc82..bf259e083 100644 --- a/app/Util/HtmlPurifier/Filters/UriLimitFileProtocolToAnchors.php +++ b/app/Util/HtmlPurifier/Filters/UriLimitFileProtocolToAnchors.php @@ -51,5 +51,3 @@ class UriLimitFileProtocolToAnchors extends HTMLPurifier_URIFilter return false; } } - -// vim: et sw=4 sts=4 diff --git a/app/Util/HtmlPurifier/SrcsetAttrDef.php b/app/Util/HtmlPurifier/SrcsetAttrDef.php new file mode 100644 index 000000000..3b8417a55 --- /dev/null +++ b/app/Util/HtmlPurifier/SrcsetAttrDef.php @@ -0,0 +1,26 @@ +a); + $bParts = parse_url($this->b); + + return $aParts['host'] === $bParts['host'] + && $aParts['scheme'] === $bParts['scheme'] + && $aParts['port'] === $bParts['port']; + } + + /** + * Check if there's some overlap between the two URLs' paths. + */ + public function pathsOverlap(): bool + { + $aPath = parse_url($this->a, PHP_URL_PATH) ?? ''; + $bPath = parse_url($this->b, PHP_URL_PATH) ?? ''; + + return str_starts_with($aPath, $bPath) || str_starts_with($bPath, $aPath); + } +} 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/Activity/CommentStoreTest.php b/tests/Activity/CommentStoreTest.php index 2296f91a9..04624e4d5 100644 --- a/tests/Activity/CommentStoreTest.php +++ b/tests/Activity/CommentStoreTest.php @@ -105,6 +105,23 @@ class CommentStoreTest extends TestCase $this->assertActivityExists(ActivityType::COMMENT_DELETE); } + public function test_comment_delete_requires_view_permission_to_page() + { + $editor = $this->users->editor(); + $this->permissions->grantUserRolePermissions($editor, ['comment-delete-all']); + $page = $this->entities->page(); + $this->actingAs($editor); + + $commentData = Comment::factory()->make(); + $this->postJson("/comment/$page->id", $commentData->getAttributes()); + $comment = $page->comments()->first(); + $this->permissions->disableEntityInheritedPermissions($page); + + $resp = $this->deleteJson("/comment/$comment->id"); + $resp->assertStatus(404); + $this->assertDatabaseHas('comments', ['id' => $comment->id]); + } + public function test_comment_archive_and_unarchive() { $this->asAdmin(); diff --git a/tests/Auth/AuthTest.php b/tests/Auth/AuthTest.php index bffd8bbdb..b42f7cb40 100644 --- a/tests/Auth/AuthTest.php +++ b/tests/Auth/AuthTest.php @@ -70,10 +70,18 @@ class AuthTest extends TestCase config()->set('app.url', 'http://localhost'); $this->setSettings(['app-public' => true]); - $this->get('/login', ['referer' => 'https://example.com']); - $login = $this->post('/login', ['email' => 'admin@admin.com', 'password' => 'password']); + $testCases = [ + 'https://example.com', + 'http://localhost.example.com', + 'http://localhost:ab@example.com', + ]; - $login->assertRedirect('http://localhost'); + foreach ($testCases as $testCase) { + $this->get('/login', ['Referer' => $testCase]); + $login = $this->post('/login', ['email' => 'admin@admin.com', 'password' => 'password']); + $login->assertRedirect('http://localhost'); + auth()->logout(); + } } public function test_login_intended_redirect_does_not_factor_mfa_routes() diff --git a/tests/Entity/PageContentFilteringTest.php b/tests/Entity/PageContentFilteringTest.php index 0ebbd9a0e..5cb898e05 100644 --- a/tests/Entity/PageContentFilteringTest.php +++ b/tests/Entity/PageContentFilteringTest.php @@ -469,6 +469,9 @@ HTML; '' => '', '
My local image
' => '
', '
My local image
' => '
', + '
My local image
' => '
My local image
', + '
cat
' => '
cat
', + '' => '', ]; config()->set('app.content_filtering', 'a'); 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/UrlComparisonTest.php b/tests/Util/UrlComparisonTest.php new file mode 100644 index 000000000..af20011fa --- /dev/null +++ b/tests/Util/UrlComparisonTest.php @@ -0,0 +1,60 @@ +assertTrue($comparison->originsMatch()); + } + + foreach ($bad as [$a, $b]) { + $comparison = new UrlComparison($a, $b); + $this->assertFalse($comparison->originsMatch()); + } + } + + public function test_paths_overlap() + { + $good = [ + ['https://example.com', 'https://example.com/a/b/c'], + ['https://example.com/', 'https://example.com/a/b/c'], + ['https://example.com/a/b', 'https://example.com/a/b/c'], + ['https://example.com/a/b/c', 'https://example.com/a'], + ['http://donk.com/a/b/c?a=b#cat', 'https://example.com:5005/a/b#hello'], + ]; + + $bad = [ + ['https://example.com/a/c', 'https://example.com/a/b/c/d'], + ['https://example.com/a/c', 'https://example.com/d/a/c'], + ]; + + foreach ($good as [$a, $b]) { + $comparison = new UrlComparison($a, $b); + $this->assertTrue($comparison->pathsOverlap()); + } + + foreach ($bad as [$a, $b]) { + $comparison = new UrlComparison($a, $b); + $this->assertFalse($comparison->pathsOverlap()); + } + } +} 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}"); + } + } +}