Updated version and assets for release v25.11.5

For #5925
Added tests to cover.
Extracted existing test to place with similiar sessions tests

.env.example

@@ -26,6 +26,13 @@ DB_DATABASE=database_database
 DB_USERNAME=database_username
 DB_PASSWORD=database_user_password
 
+# Storage system to use
+# By default files are stored on the local filesystem, with images being placed in
+# public web space so they can be efficiently served directly by the web-server.
+# For other options with different security levels & considerations, refer to:
+# https://www.bookstackapp.com/docs/admin/upload-config/
+STORAGE_TYPE=local
+
 # Mail system to use
 # Can be 'smtp' or 'sendmail'
 MAIL_DRIVER=smtp

app/Access/Controllers/OidcController.php

@@ -9,11 +9,9 @@ use Illuminate\Http\Request;
 
 class OidcController extends Controller
 {
-    protected OidcService $oidcService;
-
-    public function __construct(OidcService $oidcService)
-    {
-        $this->oidcService = $oidcService;
+    public function __construct(
+        protected OidcService $oidcService
+    ) {
         $this->middleware('guard:oidc');
     }
 
@@ -30,7 +28,7 @@ class OidcController extends Controller
             return redirect('/login');
         }
 
-        session()->flash('oidc_state', $loginDetails['state']);
+        session()->put('oidc_state', time() . ':' . $loginDetails['state']);
 
         return redirect($loginDetails['url']);
     }
@@ -41,10 +39,16 @@ class OidcController extends Controller
      */
     public function callback(Request $request)
     {
-        $storedState = session()->pull('oidc_state');
         $responseState = $request->query('state');
+        $splitState =  explode(':', session()->pull('oidc_state', ':'), 2);
+        if (count($splitState) !== 2) {
+            $splitState = [null, null];
+        }
 
-        if ($storedState !== $responseState) {
+        [$storedStateTime, $storedState] = $splitState;
+        $threeMinutesAgo = time() - 3 * 60;
+
+        if (!$storedState || $storedState !== $responseState || intval($storedStateTime) < $threeMinutesAgo) {
             $this->showErrorNotification(trans('errors.oidc_fail_authed', ['system' => config('oidc.name')]));
 
             return redirect('/login');
@@ -62,7 +66,7 @@ class OidcController extends Controller
     }
 
     /**
-     * Log the user out then start the OIDC RP-initiated logout process.
+     * Log the user out, then start the OIDC RP-initiated logout process.
      */
     public function logout()
     {

app/Activity/Models/Comment.php

@@ -41,7 +41,19 @@ class Comment extends Model implements Loggable, OwnableInterface
      */
     public function entity(): MorphTo
     {
-        return $this->morphTo('commentable');
+        // We specifically define null here to avoid the different name (commentable)
+        // being used by Laravel eager loading instead of the method name, which it was doing
+        // in some scenarios like when deserialized when going through the queue system.
+        // So we instead specify the type and id column names to use.
+        // Related to:
+        // https://github.com/laravel/framework/pull/24815
+        // https://github.com/laravel/framework/issues/27342
+        // https://github.com/laravel/framework/issues/47953
+        // (and probably more)
+
+        // Ultimately, we could just align the method name to 'commentable' but that would be a potential
+        // breaking change and not really worthwhile in a patch due to the risk of creating extra problems.
+        return $this->morphTo(null, 'commentable_type', 'commentable_id');
     }
 
     /**

app/Activity/Notifications/Handlers/BaseNotificationHandler.php

@@ -20,6 +20,7 @@ abstract class BaseNotificationHandler implements NotificationHandler
     {
         $users = User::query()->whereIn('id', array_unique($userIds))->get();
 
+        /** @var User $user */
         foreach ($users as $user) {
             // Prevent sending to the user that initiated the activity
             if ($user->id === $initiator->id) {

app/Http/Middleware/StartSessionExtended.php

@@ -14,7 +14,10 @@ use Illuminate\Session\Middleware\StartSession as Middleware;
 class StartSessionExtended extends Middleware
 {
     protected static array $pathPrefixesExcludedFromHistory = [
-        'uploads/images/'
+        'uploads/images/',
+        'dist/',
+        'manifest.json',
+        'opensearch.xml',
     ];
 
     /**

app/Uploads/ImageService.php

@@ -264,7 +264,7 @@ class ImageService
             return false;
         }
 
-        if ($this->storage->usingSecureImages() && user()->isGuest()) {
+        if ($this->blockedBySecureImages()) {
             return false;
         }
 
@@ -280,13 +280,24 @@ class ImageService
             return false;
         }
 
-        if ($this->storage->usingSecureImages() && user()->isGuest()) {
+        if ($this->blockedBySecureImages()) {
             return false;
         }
 
         return $this->imageFileExists($image->path, $image->type);
     }
 
+    /**
+     * Check if the current user should be blocked from accessing images based on if secure images are enabled
+     * and if public access is enabled for the application.
+     */
+    protected function blockedBySecureImages(): bool
+    {
+        $enforced = $this->storage->usingSecureImages() && !setting('app-public');
+
+        return $enforced && user()->isGuest();
+    }
+
     /**
      * Check if the given image path exists for the given image type and that it is likely an image file.
      */

app/Uploads/ImageStorage.php

@@ -74,7 +74,7 @@ class ImageStorage
             return 'local';
         }
 
-        // Rename local_secure options to get our image specific storage driver which
+        // Rename local_secure options to get our image-specific storage driver, which
         // is scoped to the relevant image directories.
         if ($localSecureInUse) {
             return 'local_secure_images';

dev/checksums/vendor

@@ -1 +1 @@
-a75aa1a640d312e5e6a52f63b121daf5bca1e4ad11aaf9a162c8f91e8e2e00ed
+f685179ac59c80d2b635c1df33974830616d7e055fc390be1a17056443644e4a

tests/Api/ApiDocsTest.php

@@ -22,7 +22,7 @@ class ApiDocsTest extends TestCase
         $resp->assertStatus(200);
         $resp->assertSee(url('/api/docs.json'));
         $resp->assertSee('Show a JSON view of the API docs data.');
-        $resp->assertHeader('Content-Type', 'text/html; charset=UTF-8');
+        $resp->assertHeader('Content-Type', 'text/html; charset=utf-8');
     }
 
     public function test_docs_json_endpoint_returns_json()

tests/Auth/OidcTest.php

@@ -138,7 +138,7 @@ class OidcTest extends TestCase
     {
         // Start auth
         $this->post('/oidc/login');
-        $state = session()->get('oidc_state');
+        $state = explode(':', session()->get('oidc_state'), 2)[1];
 
         $transactions = $this->mockHttpClient([$this->getMockAuthorizationResponse([
             'email' => 'benny@example.com',
@@ -190,6 +190,35 @@ class OidcTest extends TestCase
         $this->assertSessionError('Login using SingleSignOn-Testing failed, system did not provide successful authorization');
     }
 
+    public function test_callback_works_even_if_other_request_made_by_session()
+    {
+        $this->mockHttpClient([$this->getMockAuthorizationResponse([
+            'email' => 'benny@example.com',
+            'sub'   => 'benny1010101',
+        ])]);
+
+        $this->post('/oidc/login');
+        $state = explode(':', session()->get('oidc_state'), 2)[1];
+
+        $this->get('/');
+
+        $resp = $this->get("/oidc/callback?code=SplxlOBeZQQYbYS6WxSbIA&state={$state}");
+        $resp->assertRedirect('/');
+    }
+
+    public function test_callback_fails_if_state_timestamp_is_too_old()
+    {
+        $this->post('/oidc/login');
+        $state = explode(':', session()->get('oidc_state'), 2)[1];
+        session()->put('oidc_state', (time() - 60 * 4) . ':' . $state);
+
+        $this->get('/');
+
+        $resp = $this->get("/oidc/callback?code=SplxlOBeZQQYbYS6WxSbIA&state={$state}");
+        $resp->assertRedirect('/login');
+        $this->assertSessionError('Login using SingleSignOn-Testing failed, system did not provide successful authorization');
+    }
+
     public function test_dump_user_details_option_outputs_as_expected()
     {
         config()->set('oidc.dump_user_details', true);
@@ -797,7 +826,7 @@ class OidcTest extends TestCase
     {
         // Start auth
         $resp = $this->post('/oidc/login');
-        $state = session()->get('oidc_state');
+        $state = explode(':', session()->get('oidc_state'), 2)[1];
 
         $pkceCode = session()->get('oidc_pkce_code');
         $this->assertGreaterThan(30, strlen($pkceCode));
@@ -825,7 +854,7 @@ class OidcTest extends TestCase
     {
         config()->set('oidc.display_name_claims', 'first_name|last_name');
         $this->post('/oidc/login');
-        $state = session()->get('oidc_state');
+        $state = explode(':', session()->get('oidc_state'), 2)[1];
 
         $client = $this->mockHttpClient([
             $this->getMockAuthorizationResponse(['name' => null]),
@@ -973,7 +1002,7 @@ class OidcTest extends TestCase
         ]);
 
         $this->post('/oidc/login');
-        $state = session()->get('oidc_state');
+        $state = explode(':', session()->get('oidc_state'), 2)[1];
         $client = $this->mockHttpClient([$this->getMockAuthorizationResponse([
             'groups' => [],
         ])]);
@@ -999,7 +1028,7 @@ class OidcTest extends TestCase
     protected function runLogin($claimOverrides = [], $additionalHttpResponses = []): TestResponse
     {
         $this->post('/oidc/login');
-        $state = session()->get('oidc_state');
+        $state = explode(':', session()->get('oidc_state'), 2)[1] ?? '';
         $this->mockHttpClient([$this->getMockAuthorizationResponse($claimOverrides), ...$additionalHttpResponses]);
 
         return $this->get('/oidc/callback?code=SplxlOBeZQQYbYS6WxSbIA&state=' . $state);

tests/Auth/Saml2Test.php

@@ -36,7 +36,7 @@ class Saml2Test extends TestCase
     public function test_metadata_endpoint_displays_xml_as_expected()
     {
         $req = $this->get('/saml2/metadata');
-        $req->assertHeader('Content-Type', 'text/xml; charset=UTF-8');
+        $req->assertHeader('Content-Type', 'text/xml; charset=utf-8');
         $req->assertSee('md:EntityDescriptor');
         $req->assertSee(url('/saml2/acs'));
     }
@@ -51,7 +51,7 @@ class Saml2Test extends TestCase
 
         $req = $this->get('/saml2/metadata');
         $req->assertOk();
-        $req->assertHeader('Content-Type', 'text/xml; charset=UTF-8');
+        $req->assertHeader('Content-Type', 'text/xml; charset=utf-8');
         $req->assertSee('md:EntityDescriptor');
     }
 

tests/SessionTest.php

@@ -0,0 +1,53 @@
+<?php
+
+namespace Tests;
+
+class SessionTest extends TestCase
+{
+    public function test_secure_images_not_tracked_in_session_history()
+    {
+        config()->set('filesystems.images', 'local_secure');
+        $this->asEditor();
+        $page = $this->entities->page();
+        $result = $this->files->uploadGalleryImageToPage($this, $page);
+        $expectedPath = storage_path($result['path']);
+        $this->assertFileExists($expectedPath);
+
+        $this->get('/books');
+        $this->assertEquals(url('/books'), session()->previousUrl());
+
+        $resp = $this->get($result['path']);
+        $resp->assertOk();
+        $resp->assertHeader('Content-Type', 'image/png');
+
+        $this->assertEquals(url('/books'), session()->previousUrl());
+
+        if (file_exists($expectedPath)) {
+            unlink($expectedPath);
+        }
+    }
+
+    public function test_pwa_manifest_is_not_tracked_in_session_history()
+    {
+        $this->asEditor()->get('/books');
+        $this->get('/manifest.json');
+
+        $this->assertEquals(url('/books'), session()->previousUrl());
+    }
+
+    public function test_dist_dir_access_is_not_tracked_in_session_history()
+    {
+        $this->asEditor()->get('/books');
+        $this->get('/dist/sub/hello.txt');
+
+        $this->assertEquals(url('/books'), session()->previousUrl());
+    }
+
+    public function test_opensearch_is_not_tracked_in_session_history()
+    {
+        $this->asEditor()->get('/books');
+        $this->get('/opensearch.xml');
+
+        $this->assertEquals(url('/books'), session()->previousUrl());
+    }
+}

tests/ThemeTest.php

@@ -478,7 +478,7 @@ END;
 
             $resp = $this->asAdmin()->get("/theme/{$themeFolderName}/file.txt");
             $resp->assertStreamedContent($text);
-            $resp->assertHeader('Content-Type', 'text/plain; charset=UTF-8');
+            $resp->assertHeader('Content-Type', 'text/plain; charset=utf-8');
             $resp->assertHeader('Cache-Control', 'max-age=86400, private');
 
             $resp = $this->asAdmin()->get("/theme/{$themeFolderName}/image.png");
@@ -487,7 +487,7 @@ END;
 
             $resp = $this->asAdmin()->get("/theme/{$themeFolderName}/file.css");
             $resp->assertStreamedContent($css);
-            $resp->assertHeader('Content-Type', 'text/css; charset=UTF-8');
+            $resp->assertHeader('Content-Type', 'text/css; charset=utf-8');
             $resp->assertHeader('Cache-Control', 'max-age=86400, private');
         });
     }

tests/Uploads/AttachmentTest.php

@@ -323,7 +323,7 @@ class AttachmentTest extends TestCase
 
         $attachmentGet = $this->get($attachment->getUrl(true));
         // http-foundation/Response does some 'fixing' of responses to add charsets to text responses.
-        $attachmentGet->assertHeader('Content-Type', 'text/plain; charset=UTF-8');
+        $attachmentGet->assertHeader('Content-Type', 'text/plain; charset=utf-8');
         $attachmentGet->assertHeader('Content-Disposition', 'inline; filename="upload_test_file.txt"');
         $attachmentGet->assertHeader('X-Content-Type-Options', 'nosniff');
 
@@ -339,7 +339,7 @@ class AttachmentTest extends TestCase
 
         $attachmentGet = $this->get($attachment->getUrl(true));
         // http-foundation/Response does some 'fixing' of responses to add charsets to text responses.
-        $attachmentGet->assertHeader('Content-Type', 'text/plain; charset=UTF-8');
+        $attachmentGet->assertHeader('Content-Type', 'text/plain; charset=utf-8');
         $attachmentGet->assertHeader('Content-Disposition', 'inline; filename="test_file.html"');
 
         $this->files->deleteAllAttachmentFiles();

tests/Uploads/ImageTest.php

@@ -5,6 +5,8 @@ namespace Tests\Uploads;
 use BookStack\Entities\Repos\PageRepo;
 use BookStack\Uploads\Image;
 use BookStack\Uploads\ImageService;
+use BookStack\Uploads\UserAvatars;
+use BookStack\Users\Models\Role;
 use Illuminate\Support\Str;
 use Tests\TestCase;
 
@@ -427,29 +429,6 @@ class ImageTest extends TestCase
         }
     }
 
-    public function test_secure_images_not_tracked_in_session_history()
-    {
-        config()->set('filesystems.images', 'local_secure');
-        $this->asEditor();
-        $page = $this->entities->page();
-        $result = $this->files->uploadGalleryImageToPage($this, $page);
-        $expectedPath = storage_path($result['path']);
-        $this->assertFileExists($expectedPath);
-
-        $this->get('/books');
-        $this->assertEquals(url('/books'), session()->previousUrl());
-
-        $resp = $this->get($result['path']);
-        $resp->assertOk();
-        $resp->assertHeader('Content-Type', 'image/png');
-
-        $this->assertEquals(url('/books'), session()->previousUrl());
-
-        if (file_exists($expectedPath)) {
-            unlink($expectedPath);
-        }
-    }
-
     public function test_system_images_remain_public_with_local_secure_restricted()
     {
         config()->set('filesystems.images', 'local_secure_restricted');
@@ -467,6 +446,26 @@ class ImageTest extends TestCase
         }
     }
 
+    public function test_avatar_images_visible_only_when_public_access_enabled_with_local_secure_restricted()
+    {
+        config()->set('filesystems.images', 'local_secure_restricted');
+        $user = $this->users->admin();
+        $avatars = $this->app->make(UserAvatars::class);
+        $avatars->assignToUserFromExistingData($user, $this->files->pngImageData(), 'png');
+
+        $avatarUrl = $user->getAvatar();
+
+        $resp = $this->get($avatarUrl);
+        $resp->assertRedirect('/login');
+
+        $this->permissions->makeAppPublic();
+
+        $resp = $this->get($avatarUrl);
+        $resp->assertOk();
+
+        $this->files->deleteAtRelativePath($user->avatar->path);
+    }
+
     public function test_secure_restricted_images_inaccessible_without_relation_permission()
     {
         config()->set('filesystems.images', 'local_secure_restricted');
@@ -491,6 +490,38 @@ class ImageTest extends TestCase
         }
     }
 
+    public function test_secure_restricted_images_accessible_with_public_guest_access()
+    {
+        config()->set('filesystems.images', 'local_secure_restricted');
+        $this->permissions->makeAppPublic();
+
+        $this->asEditor();
+        $page = $this->entities->page();
+        $this->files->uploadGalleryImageToPage($this, $page);
+        $image = Image::query()->where('type', '=', 'gallery')
+            ->where('uploaded_to', '=', $page->id)
+            ->first();
+
+        $expectedUrl = url($image->path);
+        $expectedPath = storage_path($image->path);
+        auth()->logout();
+
+        $this->get($expectedUrl)->assertOk();
+
+        $this->permissions->setEntityPermissions($page, [], []);
+
+        $resp = $this->get($expectedUrl);
+        $resp->assertNotFound();
+
+        $this->permissions->setEntityPermissions($page, ['view'], [Role::getSystemRole('public')]);
+
+        $this->get($expectedUrl)->assertOk();
+
+        if (file_exists($expectedPath)) {
+            unlink($expectedPath);
+        }
+    }
+
     public function test_thumbnail_path_handled_by_secure_restricted_images()
     {
         config()->set('filesystems.images', 'local_secure_restricted');

version

@@ -1 +1 @@
-v25.11.2
+v25.11.5