From caeea658d117580f801ef55aa450aec2f16d785b Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Wed, 1 Jul 2026 10:45:59 +0100 Subject: [PATCH] Access: Hardened usage of referring URLs via login Adds a more substantial URL check, via a new class which is shared and used in other parts of the app for consistency. Thanks to mfk25 for reporting. --- app/Access/Controllers/LoginController.php | 4 +- app/Console/Commands/InstallModuleCommand.php | 9 ++- app/Util/UrlComparison.php | 36 +++++++++++ tests/Auth/AuthTest.php | 14 ++++- tests/Util/UrlComparisonTest.php | 60 +++++++++++++++++++ 5 files changed, 114 insertions(+), 9 deletions(-) create mode 100644 app/Util/UrlComparison.php create mode 100644 tests/Util/UrlComparisonTest.php 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/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/Util/UrlComparison.php b/app/Util/UrlComparison.php new file mode 100644 index 000000000..66984918d --- /dev/null +++ b/app/Util/UrlComparison.php @@ -0,0 +1,36 @@ +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/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/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()); + } + } +}