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

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

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

app/Util/UrlComparison.php

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

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

tests/Util/UrlComparisonTest.php

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