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

diff --git a/app/Http/Controllers/Api/AttachmentApiController.php b/app/Http/Controllers/Api/AttachmentApiController.php
index 7059ca28248456f8e1fc027fc7a39b4a37e13a56..9fc7f3bdef2c1fdc8539971e1f5438747be213bf 100644 (file)
--- 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 03e362f4a78f273573195d120ae806a9d65931ae..b6ce261d481aa6d8aad0bbdd5bb48d81676c3a37 100644 (file)
--- 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 928918dc7ffc11db36de233d62a9f507da999bc0..b3c2a4aa796a92a38fba27b2d079d1005936d04b 100644 (file)
--- 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 01c927382e6c90217d2ddc39a61ce82c85144265..e33b13db449a8f51eca600ff7eb4cb2a10242ae0 100644 (file)
--- 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 (file)
index 0000000..f7cb64c
--- /dev/null
+++ b/database/migrations/2023_02_20_093655_increase_attachments_path_length.php
@@ -0,0 +1,32 @@
+<?php
+
+use Illuminate\Database\Migrations\Migration;
+use Illuminate\Database\Schema\Blueprint;
+use Illuminate\Support\Facades\Schema;
+
+return new class extends Migration
+{
+    /**
+     * Run the migrations.
+     *
+     * @return void
+     */
+    public function up()
+    {
+        Schema::table('attachments', function (Blueprint $table) {
+            $table->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 1da12cd1c34176fca2d0775b132f41b238c4681d..bd03c339c4385bd8f69b654f7d69c3df65de4068 100644 (file)
--- 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();