diff --git a/app/Uploads/AttachmentService.php b/app/Uploads/AttachmentService.php index 3de84bdd0..f30aad26b 100644 --- a/app/Uploads/AttachmentService.php +++ b/app/Uploads/AttachmentService.php @@ -3,7 +3,9 @@ namespace BookStack\Uploads; use BookStack\Exceptions\FileUploadException; +use BookStack\Exceptions\NotFoundException; use Exception; +use Illuminate\Database\Eloquent\ModelNotFoundException; use Symfony\Component\HttpFoundation\File\UploadedFile; class AttachmentService @@ -13,6 +15,20 @@ class AttachmentService ) { } + /** + * Get an attachment by ID, ensuring it is visible to the current user. + * + * @throws NotFoundException + */ + public function getVisibleAttachmentOrFail(string $id): Attachment + { + try { + return Attachment::visible()->findOrFail(intval($id)); + } catch (ModelNotFoundException) { + throw new NotFoundException(trans('errors.attachment_not_found')); + } + } + /** * Stream an attachment from storage. * diff --git a/app/Uploads/Controllers/AttachmentController.php b/app/Uploads/Controllers/AttachmentController.php index e1da0c72d..930763ad9 100644 --- a/app/Uploads/Controllers/AttachmentController.php +++ b/app/Uploads/Controllers/AttachmentController.php @@ -9,7 +9,6 @@ use BookStack\Exceptions\FileUploadException; use BookStack\Exceptions\NotFoundException; use BookStack\Http\Controller; use BookStack\Permissions\Permission; -use BookStack\Uploads\Attachment; use BookStack\Uploads\AttachmentService; use BookStack\Util\UrlFilter; use Exception; @@ -61,6 +60,7 @@ class AttachmentController extends Controller * Update an uploaded attachment. * * @throws ValidationException + * @throws NotFoundException */ public function uploadUpdate(Request $request, $attachmentId) { @@ -68,9 +68,7 @@ class AttachmentController extends Controller 'file' => array_merge(['required'], $this->attachmentService->getFileValidationRules()), ]); - /** @var Attachment $attachment */ - $attachment = Attachment::query()->findOrFail($attachmentId); - $this->checkOwnablePermission(Permission::PageView, $attachment->page); + $attachment = $this->attachmentService->getVisibleAttachmentOrFail($attachmentId); $this->checkOwnablePermission(Permission::PageUpdate, $attachment->page); $this->checkOwnablePermission(Permission::AttachmentUpdate, $attachment); @@ -87,13 +85,13 @@ class AttachmentController extends Controller /** * Get the update form for an attachment. + * + * @throws NotFoundException */ public function getUpdateForm(string $attachmentId) { - /** @var Attachment $attachment */ - $attachment = Attachment::query()->findOrFail($attachmentId); + $attachment = $this->attachmentService->getVisibleAttachmentOrFail($attachmentId); - $this->checkOwnablePermission(Permission::PageView, $attachment->page); $this->checkOwnablePermission(Permission::PageUpdate, $attachment->page); $this->checkOwnablePermission(Permission::AttachmentUpdate, $attachment); @@ -104,12 +102,12 @@ class AttachmentController extends Controller /** * Update the details of an existing file. + * + * @throws NotFoundException */ public function update(Request $request, string $attachmentId) { - /** @var Attachment $attachment */ - $attachment = Attachment::query()->findOrFail($attachmentId); - $this->checkOwnablePermission(Permission::PageView, $attachment->page); + $attachment = $this->attachmentService->getVisibleAttachmentOrFail($attachmentId); $this->checkOwnablePermission(Permission::PageUpdate, $attachment->page); $this->checkOwnablePermission(Permission::AttachmentUpdate, $attachment); @@ -213,14 +211,7 @@ class AttachmentController extends Controller */ public function get(Request $request, string $attachmentId) { - /** @var Attachment $attachment */ - $attachment = Attachment::query()->findOrFail($attachmentId); - - try { - $page = $this->pageQueries->findVisibleByIdOrFail($attachment->uploaded_to); - } catch (NotFoundException $exception) { - throw new NotFoundException(trans('errors.attachment_not_found')); - } + $attachment = $this->attachmentService->getVisibleAttachmentOrFail($attachmentId); if ($attachment->external) { $url = (new UrlFilter($attachment->path))->clean(); @@ -245,15 +236,7 @@ class AttachmentController extends Controller */ public function delete(string $attachmentId) { - /** @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')); - } - + $attachment = $this->attachmentService->getVisibleAttachmentOrFail($attachmentId); $this->checkOwnablePermission(Permission::AttachmentDelete, $attachment); $this->attachmentService->deleteFile($attachment); diff --git a/tests/Uploads/AttachmentTest.php b/tests/Uploads/AttachmentTest.php index 30e45fff6..300829883 100644 --- a/tests/Uploads/AttachmentTest.php +++ b/tests/Uploads/AttachmentTest.php @@ -7,6 +7,7 @@ use BookStack\Entities\Repos\PageRepo; use BookStack\Entities\Tools\TrashCan; use BookStack\Permissions\Permission; use BookStack\Uploads\Attachment; +use Illuminate\Testing\TestResponse; use Tests\TestCase; class AttachmentTest extends TestCase @@ -171,7 +172,7 @@ class AttachmentTest extends TestCase 'attachment_edit_url' => 'https://test.example.com', ]); - $this->assertPermissionError($resp); + $resp->assertNotFound(); } public function test_attachment_update_without_permission_with_validation_errors() @@ -187,7 +188,7 @@ class AttachmentTest extends TestCase 'attachment_edit_url' => 'https://test.example.com', ]); - $this->assertPermissionError($resp); + $resp->assertNotFound(); $resp->assertDontSee($attachment->path); } @@ -286,7 +287,7 @@ class AttachmentTest extends TestCase $this->permissions->setEntityPermissionsForRole($page, ['update'], $editor->roles()->first()); $resp = $this->actingAs($editor)->get("/attachments/edit/{$attachment->id}"); - $this->assertPermissionError($resp); + $resp->assertNotFound(); $this->permissions->setEntityPermissionsForRole($page, ['view', 'update'], $editor->roles()->first()); $resp = $this->actingAs($editor)->get("/attachments/edit/{$attachment->id}"); @@ -407,6 +408,33 @@ class AttachmentTest extends TestCase $html->assertElementExists(".card[data-id=\"{$attachment->id}\"] button[title=\"Edit\"]"); } + public function test_attachment_actions_fail_for_other_users_page_drafts() + { + $this->asAdmin(); + $page = $this->entities->newDraftPage(); + $this->files->uploadAttachmentFile($this, 'upload_test.txt', $page->id); + $attachment = $page->attachments()->first(); + + $user = $this->users->editor(); + $this->permissions->grantUserRolePermissions($user, [Permission::AttachmentUpdateAll, Permission::AttachmentDeleteAll]); + $this->actingAs($user); + + /** @var array $actions */ + $actions = [ + 'uploadUpdate' => $this->call('POST', "/attachments/upload/{$attachment->id}", [], [], ['file' => $this->files->uploadedTextFile('text.txt')]), + 'getUpdateForm' => $this->get("/attachments/edit/{$attachment->id}"), + 'update' => $this->put("/attachments/{$attachment->id}", ['attachment_edit_name' => 'new_name']), + 'listForPage' => $this->get("/attachments/get/page/{$page->id}"), + 'sortForPage' => $this->put("/attachments/sort/page/{$page->id}", ['order' => [$attachment->id]]), + 'get' => $this->get("/attachments/{$attachment->id}"), + 'delete' => $this->delete("/attachments/{$attachment->id}"), + ]; + + foreach ($actions as $action => $response) { + $this->assertEquals(404, $response->getStatusCode(), "Failed to assert 404 status for {$action}"); + } + } + public function test_file_access_with_open_query_param_provides_inline_response_with_correct_content_type() { $page = $this->entities->page();