Mirrors/BookStack
1
0
Fork 0
mirror of https://github.com/BookStackApp/BookStack.git synced 2024-11-26 04:42:37 +01:00
Code Issues Releases Wiki Activity

Increased attachment link limit from 192 to 2k

Browse Source 
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
This commit is contained in:
Dan Brown 2023-02-20 13:05:23 +00:00
parent 8da3e64039
commit c80396136f
No known key found for this signature in database
GPG Key ID: 46D9F943C24A2EF9
6 changed files with 71 additions and 26 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

12
app/Http/Controllers/Api/AttachmentApiController.php
View File

@ -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'],
],
];
}

18
app/Http/Controllers/AttachmentController.php
View File

@ -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']), [

4
app/Providers/ValidationRuleServiceProvider.php
View File

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

8
app/Uploads/Attachment.php
View File

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

32
database/migrations/2023_02_20_093655_increase_attachments_path_length.php Normal file
View File

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

23
tests/Uploads/AttachmentTest.php
View File

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