starred/BookStack
1
0
Fork 0
mirror of https://github.com/BookStackApp/BookStack.git synced 2026-07-15 21:31:36 +03:00
Code Issues 784 Packages Projects Releases 25 Wiki Activity

Merge branch 'sec_jun26' into development

Browse Source
This commit is contained in:
Dan Brown
2026-07-02 09:38:17 +01:00
parent a213175004 caeea658d1
commit b87789dde6
17 changed files with 386 additions and 34 deletions
Download Patch File Download Diff File Expand all files Collapse all files

4
app/Access/Controllers/LoginController.php
View File

@@ -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;
}

11
app/Activity/Controllers/CommentController.php
View File

@@ -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);

7
app/App/Providers/ValidationRuleServiceProvider.php
View File

@@ -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();
});
}
}

9
app/Console/Commands/InstallModuleCommand.php
View File

@@ -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?");

3
app/Uploads/Attachment.php
View File

@@ -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' : ''));

4
app/Uploads/Controllers/AttachmentController.php
View File

@@ -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();

22
app/Util/HtmlPurifier/ConfiguredHtmlPurifier.php
View File

@@ -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

2
app/Util/HtmlPurifier/Filters/UriLimitFileProtocolToAnchors.php
View File

@@ -51,5 +51,3 @@ class UriLimitFileProtocolToAnchors extends HTMLPurifier_URIFilter
return false;
}
}
// vim: et sw=4 sts=4

26
app/Util/HtmlPurifier/SrcsetAttrDef.php Normal file
View File

@@ -0,0 +1,26 @@
<?php
namespace BookStack\Util\HtmlPurifier;
use HTMLPurifier_AttrDef;
/**
* Custom attribute definition to filter out potentially dangerous
* values from the srcset attribute.
*/
class SrcsetAttrDef extends HTMLPurifier_AttrDef
{
public function validate($string, $config, $context)
{
$lower = strtolower($string);
$nonAllowed = ['javascript:', 'vbscript:', 'data:', 'file:'];
foreach ($nonAllowed as $nonAllowedString) {
if (str_contains($lower, $nonAllowedString)) {
return false;
}
}
return $string;
}
}

36
app/Util/UrlComparison.php Normal file
View File

@@ -0,0 +1,36 @@
<?php
namespace BookStack\Util;
class UrlComparison
{
public function __construct(
protected readonly string $a,
protected readonly string $b,
) {
}
/**
* Check if the two URLs have the same origin.
*/
public function originsMatch(): bool
{
$aParts = parse_url($this->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);
}
}

103
app/Util/UrlFilter.php Normal file
View File

@@ -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;
}
}

17
tests/Activity/CommentStoreTest.php
View File

@@ -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();

14
tests/Auth/AuthTest.php
View File

@@ -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()

3
tests/Entity/PageContentFilteringTest.php
View File

@@ -469,6 +469,9 @@ HTML;
'<object data="file://link/to/file" id="bkmrk-file-object"></object>' => '<object id="bkmrk-file-object">',
'<div id="bkmrk-file-img"><img src="file://link/to/file" alt="My local image"></div>' => '<div id="bkmrk-file-img"></div>',
'<div id="bkmrk-file-img"><img srcset="file://link/to/file" alt="My local image"></div>' => '<div id="bkmrk-file-img"></div>',
'<div id="bkmrk-file-img"><img src="http://example.com" srcset="file://link/to/file 2x" alt="My local image"></div>' => '<div id="bkmrk-file-img"><img src="http://example.com" alt="My local image"></div>',
'<div id="bkmrk-file-img"><picture><source srcset="file://link/to/file"/><img src="http://example.com" alt="cat"/></picture></div>' => '<div id="bkmrk-file-img"><picture><source><img src="http://example.com" alt="cat"></picture></div>',
'<div id="bkmrk-link"><link rel="preload" as="image" imagesizes="50vw" imagesrcset="file://cat.com, bg-wide.png 800w" /></div>' => '<div id="bkmrk-link"></div>',
];
config()->set('app.content_filtering', 'a');

28
tests/Uploads/AttachmentTest.php
View File

@@ -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();

60
tests/Util/UrlComparisonTest.php Normal file
View File

@@ -0,0 +1,60 @@
<?php
namespace Tests\Util;
use BookStack\Util\UrlComparison;
use PHPUnit\Framework\TestCase;
class UrlComparisonTest extends TestCase
{
public function test_origins_match()
{
$good = [
['http://localhost/a/b', 'http://localhost#cat'],
['example.com/a/b', 'example.com?cat=dog'],
['https://example.com:5050/a/b', 'https://example.com:5050/a/b'],
];
$bad = [
['http://localhost/a/b', 'http://localh0st#cat'],
['https://example.com/a/b', 'http://example.com?cat=dog'],
['https://example.com:5051/a/b', 'https://example.com:5050/a/b'],
];
foreach ($good as [$a, $b]) {
$comparison = new UrlComparison($a, $b);
$this->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());
}
}
}

71
tests/Util/UrlFilterTest.php Normal file
View File

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