diff --git a/app/Uploads/Controllers/AttachmentController.php b/app/Uploads/Controllers/AttachmentController.php index 9c60fa415..aeee1f528 100644 --- a/app/Uploads/Controllers/AttachmentController.php +++ b/app/Uploads/Controllers/AttachmentController.php @@ -195,6 +195,7 @@ class AttachmentController extends Controller $this->validate($request, [ 'order' => ['required', 'array'], ]); + $page = $this->pageQueries->findVisibleByIdOrFail($pageId); $this->checkOwnablePermission(Permission::PageUpdate, $page); @@ -221,8 +222,6 @@ class AttachmentController extends Controller throw new NotFoundException(trans('errors.attachment_not_found')); } - $this->checkOwnablePermission(Permission::PageView, $page); - if ($attachment->external) { return redirect($attachment->path); } @@ -247,6 +246,13 @@ class AttachmentController extends Controller { /** @var Attachment $attachment */ $attachment = Attachment::query()->findOrFail($attachmentId); + + try { + $this->pageQueries->findVisibleByIdOrFail($attachment->uploaded_to); + } catch (NotFoundException $exception) { + throw new NotFoundException(trans('errors.attachment_not_found')); + } + $this->checkOwnablePermission(Permission::AttachmentDelete, $attachment); $this->attachmentService->deleteFile($attachment); diff --git a/tests/Uploads/AttachmentTest.php b/tests/Uploads/AttachmentTest.php index 945fc258d..2d402c340 100644 --- a/tests/Uploads/AttachmentTest.php +++ b/tests/Uploads/AttachmentTest.php @@ -5,6 +5,7 @@ namespace Tests\Uploads; use BookStack\Entities\Models\Page; use BookStack\Entities\Repos\PageRepo; use BookStack\Entities\Tools\TrashCan; +use BookStack\Permissions\Permission; use BookStack\Uploads\Attachment; use Tests\TestCase; @@ -206,6 +207,21 @@ class AttachmentTest extends TestCase $this->files->deleteAllAttachmentFiles(); } + public function test_attachment_deletion_requires_page_access() + { + $page = $this->entities->page(); + $attachment = Attachment::factory()->create(['uploaded_to' => $page->id]); + $editor = $this->users->editor(); + + $this->permissions->disableEntityInheritedPermissions($page); + $this->permissions->grantUserRolePermissions($editor, [Permission::AttachmentDeleteAll]); + + $resp = $this->actingAs($editor)->delete($attachment->getUrl()); + $resp->assertNotFound(); + + $this->assertDatabaseHas('attachments', ['id' => $attachment->id]); + } + public function test_attachment_access_without_permission_shows_404() { $admin = $this->users->admin();