Skip to content

Commit 73c6bf4

Browse files
committed
Images: Updated access to consider public secure_restricted
Had prevented public access for images when secure_restricted images was enabled (and for just secure images) when app settings allowed public access. This considers the app public setting, and adds tests to cover extra scenarios to prevent regression.
1 parent 47f12cc commit 73c6bf4

File tree

3 files changed

+68
-3
lines changed

3 files changed

+68
-3
lines changed

app/Uploads/ImageService.php

Lines changed: 13 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -264,7 +264,7 @@ public function pathAccessible(string $imagePath): bool
264264
return false;
265265
}
266266

267-
if ($this->storage->usingSecureImages() && user()->isGuest()) {
267+
if ($this->blockedBySecureImages()) {
268268
return false;
269269
}
270270

@@ -280,13 +280,24 @@ public function imageAccessible(Image $image): bool
280280
return false;
281281
}
282282

283-
if ($this->storage->usingSecureImages() && user()->isGuest()) {
283+
if ($this->blockedBySecureImages()) {
284284
return false;
285285
}
286286

287287
return $this->imageFileExists($image->path, $image->type);
288288
}
289289

290+
/**
291+
* Check if the current user should be blocked from accessing images based on if secure images are enabled
292+
* and if public access is enabled for the application.
293+
*/
294+
protected function blockedBySecureImages(): bool
295+
{
296+
$enforced = $this->storage->usingSecureImages() && !setting('app-public');
297+
298+
return $enforced && user()->isGuest();
299+
}
300+
290301
/**
291302
* Check if the given image path exists for the given image type and that it is likely an image file.
292303
*/

app/Uploads/ImageStorage.php

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -74,7 +74,7 @@ protected function getDiskName(string $imageType): string
7474
return 'local';
7575
}
7676

77-
// Rename local_secure options to get our image specific storage driver which
77+
// Rename local_secure options to get our image-specific storage driver, which
7878
// is scoped to the relevant image directories.
7979
if ($localSecureInUse) {
8080
return 'local_secure_images';

tests/Uploads/ImageTest.php

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,8 @@
55
use BookStack\Entities\Repos\PageRepo;
66
use BookStack\Uploads\Image;
77
use BookStack\Uploads\ImageService;
8+
use BookStack\Uploads\UserAvatars;
9+
use BookStack\Users\Models\Role;
810
use Illuminate\Support\Str;
911
use Tests\TestCase;
1012

@@ -467,6 +469,26 @@ public function test_system_images_remain_public_with_local_secure_restricted()
467469
}
468470
}
469471

472+
public function test_avatar_images_visible_only_when_public_access_enabled_with_local_secure_restricted()
473+
{
474+
config()->set('filesystems.images', 'local_secure_restricted');
475+
$user = $this->users->admin();
476+
$avatars = $this->app->make(UserAvatars::class);
477+
$avatars->assignToUserFromExistingData($user, $this->files->pngImageData(), 'png');
478+
479+
$avatarUrl = $user->getAvatar();
480+
481+
$resp = $this->get($avatarUrl);
482+
$resp->assertRedirect('/login');
483+
484+
$this->permissions->makeAppPublic();
485+
486+
$resp = $this->get($avatarUrl);
487+
$resp->assertOk();
488+
489+
$this->files->deleteAtRelativePath($user->avatar->path);
490+
}
491+
470492
public function test_secure_restricted_images_inaccessible_without_relation_permission()
471493
{
472494
config()->set('filesystems.images', 'local_secure_restricted');
@@ -491,6 +513,38 @@ public function test_secure_restricted_images_inaccessible_without_relation_perm
491513
}
492514
}
493515

516+
public function test_secure_restricted_images_accessible_with_public_guest_access()
517+
{
518+
config()->set('filesystems.images', 'local_secure_restricted');
519+
$this->permissions->makeAppPublic();
520+
521+
$this->asEditor();
522+
$page = $this->entities->page();
523+
$this->files->uploadGalleryImageToPage($this, $page);
524+
$image = Image::query()->where('type', '=', 'gallery')
525+
->where('uploaded_to', '=', $page->id)
526+
->first();
527+
528+
$expectedUrl = url($image->path);
529+
$expectedPath = storage_path($image->path);
530+
auth()->logout();
531+
532+
$this->get($expectedUrl)->assertOk();
533+
534+
$this->permissions->setEntityPermissions($page, [], []);
535+
536+
$resp = $this->get($expectedUrl);
537+
$resp->assertNotFound();
538+
539+
$this->permissions->setEntityPermissions($page, ['view'], [Role::getSystemRole('public')]);
540+
541+
$this->get($expectedUrl)->assertOk();
542+
543+
if (file_exists($expectedPath)) {
544+
unlink($expectedPath);
545+
}
546+
}
547+
494548
public function test_thumbnail_path_handled_by_secure_restricted_images()
495549
{
496550
config()->set('filesystems.images', 'local_secure_restricted');

0 commit comments

Comments
 (0)