From c80396136f1f34dce643ff5649e74e67449e6f78 Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Mon, 20 Feb 2023 13:05:23 +0000 Subject: [PATCH] Increased attachment link limit from 192 to 2k Added test to cover. Did attempt a 64k limit, but values over 2k significantly increase chance of other issues since this URL may be used in redirect headers. Would rather catch issues in-app. For #4044 --- .../Api/AttachmentApiController.php | 12 +++---- app/Http/Controllers/AttachmentController.php | 18 ++++------- .../ValidationRuleServiceProvider.php | 4 +-- app/Uploads/Attachment.php | 8 ++--- ...93655_increase_attachments_path_length.php | 32 +++++++++++++++++++ tests/Uploads/AttachmentTest.php | 23 +++++++++++++ 6 files changed, 71 insertions(+), 26 deletions(-) create mode 100644 database/migrations/2023_02_20_093655_increase_attachments_path_length.php diff --git a/app/Http/Controllers/Api/AttachmentApiController.php b/app/Http/Controllers/Api/AttachmentApiController.php index 7059ca282..9fc7f3bde 100644 --- a/app/Http/Controllers/Api/AttachmentApiController.php +++ b/app/Http/Controllers/Api/AttachmentApiController.php @@ -13,11 +13,9 @@ use Illuminate\Validation\ValidationException; class AttachmentApiController extends ApiController { - protected $attachmentService; - - public function __construct(AttachmentService $attachmentService) - { - $this->attachmentService = $attachmentService; + public function __construct( + protected AttachmentService $attachmentService + ) { } /** @@ -174,13 +172,13 @@ class AttachmentApiController extends ApiController 'name' => ['required', 'min:1', 'max:255', 'string'], 'uploaded_to' => ['required', 'integer', 'exists:pages,id'], 'file' => array_merge(['required_without:link'], $this->attachmentService->getFileValidationRules()), - 'link' => ['required_without:file', 'min:1', 'max:255', 'safe_url'], + 'link' => ['required_without:file', 'min:1', 'max:2000', 'safe_url'], ], 'update' => [ 'name' => ['min:1', 'max:255', 'string'], 'uploaded_to' => ['integer', 'exists:pages,id'], 'file' => $this->attachmentService->getFileValidationRules(), - 'link' => ['min:1', 'max:255', 'safe_url'], + 'link' => ['min:1', 'max:2000', 'safe_url'], ], ]; } diff --git a/app/Http/Controllers/AttachmentController.php b/app/Http/Controllers/AttachmentController.php index 03e362f4a..b6ce261d4 100644 --- a/app/Http/Controllers/AttachmentController.php +++ b/app/Http/Controllers/AttachmentController.php @@ -15,16 +15,10 @@ use Illuminate\Validation\ValidationException; class AttachmentController extends Controller { - protected AttachmentService $attachmentService; - protected PageRepo $pageRepo; - - /** - * AttachmentController constructor. - */ - public function __construct(AttachmentService $attachmentService, PageRepo $pageRepo) - { - $this->attachmentService = $attachmentService; - $this->pageRepo = $pageRepo; + public function __construct( + protected AttachmentService $attachmentService, + protected PageRepo $pageRepo + ) { } /** @@ -112,7 +106,7 @@ class AttachmentController extends Controller try { $this->validate($request, [ 'attachment_edit_name' => ['required', 'string', 'min:1', 'max:255'], - 'attachment_edit_url' => ['string', 'min:1', 'max:255', 'safe_url'], + 'attachment_edit_url' => ['string', 'min:1', 'max:2000', 'safe_url'], ]); } catch (ValidationException $exception) { return response()->view('attachments.manager-edit-form', array_merge($request->only(['attachment_edit_name', 'attachment_edit_url']), [ @@ -148,7 +142,7 @@ class AttachmentController extends Controller $this->validate($request, [ 'attachment_link_uploaded_to' => ['required', 'integer', 'exists:pages,id'], 'attachment_link_name' => ['required', 'string', 'min:1', 'max:255'], - 'attachment_link_url' => ['required', 'string', 'min:1', 'max:255', 'safe_url'], + 'attachment_link_url' => ['required', 'string', 'min:1', 'max:2000', 'safe_url'], ]); } catch (ValidationException $exception) { return response()->view('attachments.manager-link-form', array_merge($request->only(['attachment_link_name', 'attachment_link_url']), [ diff --git a/app/Providers/ValidationRuleServiceProvider.php b/app/Providers/ValidationRuleServiceProvider.php index 928918dc7..b3c2a4aa7 100644 --- a/app/Providers/ValidationRuleServiceProvider.php +++ b/app/Providers/ValidationRuleServiceProvider.php @@ -21,8 +21,8 @@ class ValidationRuleServiceProvider extends ServiceProvider Validator::extend('safe_url', function ($attribute, $value, $parameters, $validator) { $cleanLinkName = strtolower(trim($value)); - $isJs = strpos($cleanLinkName, 'javascript:') === 0; - $isData = strpos($cleanLinkName, 'data:') === 0; + $isJs = str_starts_with($cleanLinkName, 'javascript:'); + $isData = str_starts_with($cleanLinkName, 'data:'); return !$isJs && !$isData; }); diff --git a/app/Uploads/Attachment.php b/app/Uploads/Attachment.php index 01c927382..e33b13db4 100644 --- a/app/Uploads/Attachment.php +++ b/app/Uploads/Attachment.php @@ -40,12 +40,10 @@ class Attachment extends Model /** * Get the downloadable file name for this upload. - * - * @return mixed|string */ - public function getFileName() + public function getFileName(): string { - if (strpos($this->name, '.') !== false) { + if (str_contains($this->name, '.')) { return $this->name; } @@ -71,7 +69,7 @@ class Attachment extends Model */ public function getUrl($openInline = false): string { - if ($this->external && strpos($this->path, 'http') !== 0) { + if ($this->external && !str_starts_with($this->path, 'http')) { return $this->path; } diff --git a/database/migrations/2023_02_20_093655_increase_attachments_path_length.php b/database/migrations/2023_02_20_093655_increase_attachments_path_length.php new file mode 100644 index 000000000..f7cb64ce6 --- /dev/null +++ b/database/migrations/2023_02_20_093655_increase_attachments_path_length.php @@ -0,0 +1,32 @@ +text('path')->change(); + }); + } + + /** + * Reverse the migrations. + * + * @return void + */ + public function down() + { + Schema::table('attachments', function (Blueprint $table) { + $table->string('path')->change(); + }); + } +}; diff --git a/tests/Uploads/AttachmentTest.php b/tests/Uploads/AttachmentTest.php index 1da12cd1c..bd03c339c 100644 --- a/tests/Uploads/AttachmentTest.php +++ b/tests/Uploads/AttachmentTest.php @@ -111,6 +111,29 @@ class AttachmentTest extends TestCase $this->files->deleteAllAttachmentFiles(); } + public function test_attaching_long_links_to_a_page() + { + $page = $this->entities->page(); + + $link = 'https://example.com?query=' . str_repeat('catsIScool', 195); + $linkReq = $this->asAdmin()->post('attachments/link', [ + 'attachment_link_url' => $link, + 'attachment_link_name' => 'Example Attachment Link', + 'attachment_link_uploaded_to' => $page->id, + ]); + + $linkReq->assertStatus(200); + $this->assertDatabaseHas('attachments', [ + 'uploaded_to' => $page->id, + 'path' => $link, + 'external' => true, + ]); + + $attachment = $page->attachments()->where('external', '=', true)->first(); + $resp = $this->get($attachment->getUrl()); + $resp->assertRedirect($link); + } + public function test_attachment_updating() { $page = $this->entities->page();