From a889c18c02312f5055f62c7caeefab1ada5446cd Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Wed, 19 Aug 2026 12:17:58 +0100 Subject: [PATCH] Attachment API: Fixed missing check on attachment page change On the change of an attachment uploaded_to (page) this ensures that a PageUpdate permission check is made against the original attachment page, in addition to the new page. Thanks to Wade Sparks / ByteMe.Red for reporting. --- .../Controllers/AttachmentApiController.php | 17 +++++++----- tests/Api/AttachmentsApiTest.php | 26 +++++++++++++++++++ 2 files changed, 36 insertions(+), 7 deletions(-) diff --git a/app/Uploads/Controllers/AttachmentApiController.php b/app/Uploads/Controllers/AttachmentApiController.php index 2448b79b5..f540833f0 100644 --- a/app/Uploads/Controllers/AttachmentApiController.php +++ b/app/Uploads/Controllers/AttachmentApiController.php @@ -24,7 +24,7 @@ class AttachmentApiController extends ApiController /** * Get a listing of attachments visible to the user. - * The external property indicates whether the attachment is simple a link. + * The external property indicates whether the attachment is simply a link. * A false value for the external property would indicate a file upload. */ public function list() @@ -39,7 +39,7 @@ class AttachmentApiController extends ApiController * An uploaded_to value must be provided containing an ID of the page * that this upload will be related to. * - * If you're uploading a file the POST data should be provided via + * If you're uploading a file, the POST data should be provided via * a multipart/form-data type request instead of JSON. * * @throws ValidationException @@ -71,7 +71,7 @@ class AttachmentApiController extends ApiController } /** - * Get the details & content of a single attachment of the given ID. + * Get the details and content of a single attachment of the given ID. * The attachment link or file content is provided via a 'content' property. * For files the content will be base64 encoded. * @@ -120,7 +120,7 @@ class AttachmentApiController extends ApiController /** * Update the details of a single attachment. - * As per the create endpoint, if a file is being provided as the attachment content + * As per the create endpoint, if a file is being provided as the attachment content, * the request should be formatted as a multipart/form-data request instead of JSON. * * @throws ValidationException @@ -134,9 +134,12 @@ class AttachmentApiController extends ApiController $page = $attachment->page; if ($requestData['uploaded_to'] ?? false) { - $pageId = $request->input('uploaded_to'); - $page = $this->pageQueries->findVisibleByIdOrFail($pageId); - $attachment->uploaded_to = $requestData['uploaded_to']; + $pageId = intval($requestData['uploaded_to']); + if ($pageId !== $page->id) { + $this->checkOwnablePermission(Permission::PageUpdate, $page); + $page = $this->pageQueries->findVisibleByIdOrFail($pageId); + $attachment->uploaded_to = $pageId; + } } $this->checkOwnablePermission(Permission::PageView, $page); diff --git a/tests/Api/AttachmentsApiTest.php b/tests/Api/AttachmentsApiTest.php index b23465879..3c927c22b 100644 --- a/tests/Api/AttachmentsApiTest.php +++ b/tests/Api/AttachmentsApiTest.php @@ -3,6 +3,7 @@ namespace Tests\Api; use BookStack\Entities\Models\Page; +use BookStack\Permissions\Permission; use BookStack\Uploads\Attachment; use Illuminate\Http\UploadedFile; use Illuminate\Testing\AssertableJsonString; @@ -345,6 +346,31 @@ class AttachmentsApiTest extends TestCase $this->assertEquals('', $attachment->extension); } + public function test_update_uploaded_to_change_requires_edit_permission_to_old_page() + { + $editor = $this->users->editor(); + $this->actingAsForApi($editor); + + $originalPage = $this->entities->page(); + $attachment = $this->createAttachmentForPage($originalPage); + $newPage = $this->entities->page(); + + $details = [ + 'name' => 'My updated API attachment', + 'uploaded_to' => $newPage->id, + ]; + + $this->permissions->setEntityPermissions($originalPage, ['view'], [$editor->roles->first()]); + $this->permissions->grantUserRolePermissions($editor, [Permission::AttachmentUpdateAll]); + + $resp = $this->putJson("{$this->baseEndpoint}/{$attachment->id}", $details); + $attachment->refresh(); + + $this->assertPermissionError($resp); + $this->assertNotEquals($newPage->id, $attachment->uploaded_to); + $this->assertEquals($originalPage->id, $attachment->uploaded_to); + } + public function test_delete_endpoint() { $this->actingAsApiAdmin();