Attachments: Added page access check to attachment delete

Thanks to github.com/404-pkj for reporting.

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

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