From 82e8b1577ec0c7b136da5eed9c89d4790714814c Mon Sep 17 00:00:00 2001
From: Dan Brown <ssddanbrown@googlemail.com>
Date: Sat, 2 Apr 2022 18:07:43 +0100
Subject: [PATCH 1/5] Updated attachment download responses to stream from
filesystem
This allows download of attachments that are larger than current memory
limits, since we're not loading the entire file into memory any more.
For inline file responses, we take a 1kb portion of the file to sniff
before to check mime before we proceed.
---
app/Http/Controllers/AttachmentController.php | 11 +++---
app/Http/Controllers/Controller.php | 37 +++++++++++++++++++
app/Uploads/AttachmentService.php | 14 ++++++-
3 files changed, 56 insertions(+), 6 deletions(-)
diff --git a/app/Http/Controllers/AttachmentController.php b/app/Http/Controllers/AttachmentController.php
index 084f6f96a..7f5ffc8cb 100644
--- a/app/Http/Controllers/AttachmentController.php
+++ b/app/Http/Controllers/AttachmentController.php
@@ -10,13 +10,14 @@ use BookStack\Uploads\AttachmentService;
use Exception;
use Illuminate\Contracts\Filesystem\FileNotFoundException;
use Illuminate\Http\Request;
+use Illuminate\Support\Facades\Storage;
use Illuminate\Support\MessageBag;
use Illuminate\Validation\ValidationException;
class AttachmentController extends Controller
{
- protected $attachmentService;
- protected $pageRepo;
+ protected AttachmentService $attachmentService;
+ protected PageRepo $pageRepo;
/**
* AttachmentController constructor.
@@ -230,13 +231,13 @@ class AttachmentController extends Controller
}
$fileName = $attachment->getFileName();
- $attachmentContents = $this->attachmentService->getAttachmentFromStorage($attachment);
+ $attachmentStream = $this->attachmentService->streamAttachmentFromStorage($attachment);
if ($request->get('open') === 'true') {
- return $this->inlineDownloadResponse($attachmentContents, $fileName);
+ return $this->streamedInlineDownloadResponse($attachmentStream, $fileName);
}
- return $this->downloadResponse($attachmentContents, $fileName);
+ return $this->streamedDownloadResponse($attachmentStream, $fileName);
}
/**
diff --git a/app/Http/Controllers/Controller.php b/app/Http/Controllers/Controller.php
index d616974c6..ae1f4e4ba 100644
--- a/app/Http/Controllers/Controller.php
+++ b/app/Http/Controllers/Controller.php
@@ -12,6 +12,7 @@ use Illuminate\Foundation\Validation\ValidatesRequests;
use Illuminate\Http\JsonResponse;
use Illuminate\Http\Response;
use Illuminate\Routing\Controller as BaseController;
+use Symfony\Component\HttpFoundation\StreamedResponse;
abstract class Controller extends BaseController
{
@@ -120,6 +121,21 @@ abstract class Controller extends BaseController
]);
}
+ /**
+ * Create a response that forces a download, from a given stream of content.
+ */
+ protected function streamedDownloadResponse($stream, string $fileName): StreamedResponse
+ {
+ return response()->stream(function() use ($stream) {
+ fpassthru($stream);
+ fclose($stream);
+ }, 200, [
+ 'Content-Type' => 'application/octet-stream',
+ 'Content-Disposition' => 'attachment; filename="' . $fileName . '"',
+ 'X-Content-Type-Options' => 'nosniff',
+ ]);
+ }
+
/**
* Create a file download response that provides the file with a content-type
* correct for the file, in a way so the browser can show the content in browser.
@@ -135,6 +151,27 @@ abstract class Controller extends BaseController
]);
}
+ /**
+ * Create a file download response that provides the file with a content-type
+ * correct for the file, in a way so the browser can show the content in browser,
+ * for a given content stream.
+ */
+ protected function streamedInlineDownloadResponse($stream, string $fileName): StreamedResponse
+ {
+ $sniffContent = fread($stream, 1000);
+ $mime = (new WebSafeMimeSniffer())->sniff($sniffContent);
+
+ return response()->stream(function() use ($sniffContent, $stream) {
+ echo $sniffContent;
+ fpassthru($stream);
+ fclose($stream);
+ }, 200, [
+ 'Content-Type' => $mime,
+ 'Content-Disposition' => 'inline; filename="' . $fileName . '"',
+ 'X-Content-Type-Options' => 'nosniff',
+ ]);
+ }
+
/**
* Show a positive, successful notification to the user on next view load.
*/
diff --git a/app/Uploads/AttachmentService.php b/app/Uploads/AttachmentService.php
index 7974d7ae9..05e70a502 100644
--- a/app/Uploads/AttachmentService.php
+++ b/app/Uploads/AttachmentService.php
@@ -14,7 +14,7 @@ use Symfony\Component\HttpFoundation\File\UploadedFile;
class AttachmentService
{
- protected $fileSystem;
+ protected FilesystemManager $fileSystem;
/**
* AttachmentService constructor.
@@ -73,6 +73,18 @@ class AttachmentService
return $this->getStorageDisk()->get($this->adjustPathForStorageDisk($attachment->path));
}
+ /**
+ * Stream an attachment from storage.
+ *
+ * @return resource|null
+ * @throws FileNotFoundException
+ */
+ public function streamAttachmentFromStorage(Attachment $attachment)
+ {
+
+ return $this->getStorageDisk()->readStream($this->adjustPathForStorageDisk($attachment->path));
+ }
+
/**
* Store a new attachment upon user upload.
*
From 6749faa89a9faa6f21ba173c1736f6bbba72a8f8 Mon Sep 17 00:00:00 2001
From: Dan Brown <ssddanbrown@googlemail.com>
Date: Sat, 2 Apr 2022 18:42:15 +0100
Subject: [PATCH 2/5] Fixed streamed outputs in more extreme scenarios
Fixes hitting memory limits where downloaded file sizes are much greater
than memory limit. Stopping and flushing output buffer seemed to stop
limits causing issues when fpassthru is used.
Tested with 24M memory limit and 734M file
---
app/Http/Controllers/Controller.php | 1 +
1 file changed, 1 insertion(+)
diff --git a/app/Http/Controllers/Controller.php b/app/Http/Controllers/Controller.php
index ae1f4e4ba..4c979c26a 100644
--- a/app/Http/Controllers/Controller.php
+++ b/app/Http/Controllers/Controller.php
@@ -127,6 +127,7 @@ abstract class Controller extends BaseController
protected function streamedDownloadResponse($stream, string $fileName): StreamedResponse
{
return response()->stream(function() use ($stream) {
+ ob_end_clean();
fpassthru($stream);
fclose($stream);
}, 200, [
From cb770c534d4d1ac776fb78126fee7b46f57a2f19 Mon Sep 17 00:00:00 2001
From: Dan Brown <ssddanbrown@googlemail.com>
Date: Sat, 2 Apr 2022 18:46:48 +0100
Subject: [PATCH 3/5] Added streamed uploads for attachments
---
app/Http/Controllers/AttachmentController.php | 1 -
app/Uploads/AttachmentService.php | 5 ++---
2 files changed, 2 insertions(+), 4 deletions(-)
diff --git a/app/Http/Controllers/AttachmentController.php b/app/Http/Controllers/AttachmentController.php
index 7f5ffc8cb..0a092b63a 100644
--- a/app/Http/Controllers/AttachmentController.php
+++ b/app/Http/Controllers/AttachmentController.php
@@ -10,7 +10,6 @@ use BookStack\Uploads\AttachmentService;
use Exception;
use Illuminate\Contracts\Filesystem\FileNotFoundException;
use Illuminate\Http\Request;
-use Illuminate\Support\Facades\Storage;
use Illuminate\Support\MessageBag;
use Illuminate\Validation\ValidationException;
diff --git a/app/Uploads/AttachmentService.php b/app/Uploads/AttachmentService.php
index 05e70a502..ec02182bb 100644
--- a/app/Uploads/AttachmentService.php
+++ b/app/Uploads/AttachmentService.php
@@ -223,8 +223,6 @@ class AttachmentService
*/
protected function putFileInStorage(UploadedFile $uploadedFile): string
{
- $attachmentData = file_get_contents($uploadedFile->getRealPath());
-
$storage = $this->getStorageDisk();
$basePath = 'uploads/files/' . date('Y-m-M') . '/';
@@ -233,10 +231,11 @@ class AttachmentService
$uploadFileName = Str::random(3) . $uploadFileName;
}
+ $attachmentStream = fopen($uploadedFile->getRealPath(), 'r');
$attachmentPath = $basePath . $uploadFileName;
try {
- $storage->put($this->adjustPathForStorageDisk($attachmentPath), $attachmentData);
+ $storage->writeStream($this->adjustPathForStorageDisk($attachmentPath), $attachmentStream);
} catch (Exception $e) {
Log::error('Error when attempting file upload:' . $e->getMessage());
From 08a8c0070e3f1574ceecd0ea120c26bbf52eb0f4 Mon Sep 17 00:00:00 2001
From: Dan Brown <ssddanbrown@googlemail.com>
Date: Sat, 2 Apr 2022 19:20:59 +0100
Subject: [PATCH 4/5] Added streaming support to API attachment read responses
Required some special handling due to the content being base64-encoded
within a JSON response.
---
.../Api/AttachmentApiController.php | 28 +++++++++++++++----
1 file changed, 23 insertions(+), 5 deletions(-)
diff --git a/app/Http/Controllers/Api/AttachmentApiController.php b/app/Http/Controllers/Api/AttachmentApiController.php
index fc5008e3c..2476cb951 100644
--- a/app/Http/Controllers/Api/AttachmentApiController.php
+++ b/app/Http/Controllers/Api/AttachmentApiController.php
@@ -87,14 +87,32 @@ class AttachmentApiController extends ApiController
'markdown' => $attachment->markdownLink(),
]);
- if (!$attachment->external) {
- $attachmentContents = $this->attachmentService->getAttachmentFromStorage($attachment);
- $attachment->setAttribute('content', base64_encode($attachmentContents));
- } else {
+ // Simply return a JSON response of the attachment for link-based attachments
+ if ($attachment->external) {
$attachment->setAttribute('content', $attachment->path);
+ return response()->json($attachment);
}
- return response()->json($attachment);
+ // Build and split our core JSON, at point of content.
+ $splitter = 'CONTENT_SPLIT_LOCATION_' . time() . '_' . rand(1, 40000);
+ $attachment->setAttribute('content', $splitter);
+ $json = $attachment->toJson();
+ $jsonParts = explode($splitter, $json);
+ // Get a stream for the file data from storage
+ $stream = $this->attachmentService->streamAttachmentFromStorage($attachment);
+
+ return response()->stream(function () use ($jsonParts, $stream) {
+ // Output the pre-content JSON data
+ echo $jsonParts[0];
+
+ // Stream out our attachment data as base64 content
+ stream_filter_append($stream, 'convert.base64-encode', STREAM_FILTER_READ);
+ fpassthru($stream);
+ fclose($stream);
+
+ // Output our post-content JSON data
+ echo $jsonParts[1];
+ }, 200, ['Content-Type' => 'application/json']);
}
/**
From 59d1fb2d1033616ac7edc143f4876485bb997c6a Mon Sep 17 00:00:00 2001
From: Dan Brown <ssddanbrown@googlemail.com>
Date: Sun, 3 Apr 2022 16:22:31 +0100
Subject: [PATCH 5/5] Fixed tests from streaming changes
- Added testing check to buffer stop/clear on streaming output due to
interference during tests.
- Made content-disposition header a little safer in download responses.
- Also aligned how we check for testing environment.
---
app/Http/Controllers/Controller.php | 15 ++++++++++-----
resources/views/common/export-styles.blade.php | 2 +-
tests/Api/AttachmentsApiTest.php | 7 +++++--
tests/Uploads/AttachmentTest.php | 3 ++-
4 files changed, 18 insertions(+), 9 deletions(-)
diff --git a/app/Http/Controllers/Controller.php b/app/Http/Controllers/Controller.php
index 4c979c26a..6ca2239cc 100644
--- a/app/Http/Controllers/Controller.php
+++ b/app/Http/Controllers/Controller.php
@@ -116,7 +116,7 @@ abstract class Controller extends BaseController
{
return response()->make($content, 200, [
'Content-Type' => 'application/octet-stream',
- 'Content-Disposition' => 'attachment; filename="' . $fileName . '"',
+ 'Content-Disposition' => 'attachment; filename="' . str_replace('"', '', $fileName) . '"',
'X-Content-Type-Options' => 'nosniff',
]);
}
@@ -127,12 +127,17 @@ abstract class Controller extends BaseController
protected function streamedDownloadResponse($stream, string $fileName): StreamedResponse
{
return response()->stream(function() use ($stream) {
- ob_end_clean();
+ // End & flush the output buffer otherwise we still seem to use memory.
+ // Ignore in testing since output buffers are used to gather a response.
+ if (!app()->runningUnitTests()) {
+ ob_end_clean();
+ }
+
fpassthru($stream);
fclose($stream);
}, 200, [
'Content-Type' => 'application/octet-stream',
- 'Content-Disposition' => 'attachment; filename="' . $fileName . '"',
+ 'Content-Disposition' => 'attachment; filename="' . str_replace('"', '', $fileName) . '"',
'X-Content-Type-Options' => 'nosniff',
]);
}
@@ -147,7 +152,7 @@ abstract class Controller extends BaseController
return response()->make($content, 200, [
'Content-Type' => $mime,
- 'Content-Disposition' => 'inline; filename="' . $fileName . '"',
+ 'Content-Disposition' => 'inline; filename="' . str_replace('"', '', $fileName) . '"',
'X-Content-Type-Options' => 'nosniff',
]);
}
@@ -168,7 +173,7 @@ abstract class Controller extends BaseController
fclose($stream);
}, 200, [
'Content-Type' => $mime,
- 'Content-Disposition' => 'inline; filename="' . $fileName . '"',
+ 'Content-Disposition' => 'inline; filename="' . str_replace('"', '', $fileName) . '"',
'X-Content-Type-Options' => 'nosniff',
]);
}
diff --git a/resources/views/common/export-styles.blade.php b/resources/views/common/export-styles.blade.php
index ee10637dd..1dfa6bb45 100644
--- a/resources/views/common/export-styles.blade.php
+++ b/resources/views/common/export-styles.blade.php
@@ -1,5 +1,5 @@
<style>
- @if (!app()->environment('testing'))
+ @if (!app()->runningUnitTests())
{!! file_get_contents(public_path('/dist/export-styles.css')) !!}
@endif
</style>
diff --git a/tests/Api/AttachmentsApiTest.php b/tests/Api/AttachmentsApiTest.php
index d7625c938..a34337016 100644
--- a/tests/Api/AttachmentsApiTest.php
+++ b/tests/Api/AttachmentsApiTest.php
@@ -5,6 +5,7 @@ namespace Tests\Api;
use BookStack\Entities\Models\Page;
use BookStack\Uploads\Attachment;
use Illuminate\Http\UploadedFile;
+use Illuminate\Testing\AssertableJsonString;
use Tests\TestCase;
class AttachmentsApiTest extends TestCase
@@ -228,9 +229,11 @@ class AttachmentsApiTest extends TestCase
$attachment = Attachment::query()->orderByDesc('id')->where('name', '=', $details['name'])->firstOrFail();
$resp = $this->getJson("{$this->baseEndpoint}/{$attachment->id}");
-
$resp->assertStatus(200);
- $resp->assertJson([
+ $resp->assertHeader('Content-Type', 'application/json');
+
+ $json = new AssertableJsonString($resp->streamedContent());
+ $json->assertSubset([
'id' => $attachment->id,
'content' => base64_encode(file_get_contents(storage_path($attachment->path))),
'external' => false,
diff --git a/tests/Uploads/AttachmentTest.php b/tests/Uploads/AttachmentTest.php
index 5545edf13..27a23bcae 100644
--- a/tests/Uploads/AttachmentTest.php
+++ b/tests/Uploads/AttachmentTest.php
@@ -128,7 +128,8 @@ class AttachmentTest extends TestCase
$pageGet->assertSee($attachment->getUrl());
$attachmentGet = $this->get($attachment->getUrl());
- $attachmentGet->assertSee('Hi, This is a test file for testing the upload process.');
+ $content = $attachmentGet->streamedContent();
+ $this->assertStringContainsString('Hi, This is a test file for testing the upload process.', $content);
$this->deleteUploads();
}