webapp/bookstack
1
0
Fork 0
mirror of https://github.com/BookStackApp/BookStack.git synced 2026-10-23 06:49:01 +03:00
Code Activity

Attachments: Made view check for UI functions consistent

Browse Source 
Ensures proper checking of page-based visibility, ensuring that the
draft status of the related page is also considered when checking view
ability.

Thanks to Huzaifa Dal for responsibly reporting.
This commit is contained in:
Dan Brown
2026-10-15 01:49:13 +01:00
parent f239524447
commit fcadecccda
3 changed files with 57 additions and 30 deletions
Download Patch File Download Diff File Expand all files Collapse all files

16
app/Uploads/AttachmentService.php
View File

@@ -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.
*

37
app/Uploads/Controllers/AttachmentController.php
View File

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

34
tests/Uploads/AttachmentTest.php
View File

@@ -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<string, TestResponse> $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();