Merge pull request #2791 from BookStackApp/attachments_open_in_browser

Attachment serving without forced download

app/Http/Controllers/AttachmentController.php

@@ -14,16 +14,14 @@ use Illuminate\Validation\ValidationException;
 class AttachmentController extends Controller
 {
     protected $attachmentService;
-    protected $attachment;
     protected $pageRepo;
 
     /**
      * AttachmentController constructor.
      */
-    public function __construct(AttachmentService $attachmentService, Attachment $attachment, PageRepo $pageRepo)
+    public function __construct(AttachmentService $attachmentService, PageRepo $pageRepo)
     {
         $this->attachmentService = $attachmentService;
-        $this->attachment = $attachment;
         $this->pageRepo = $pageRepo;
     }
 
@@ -67,7 +65,7 @@ class AttachmentController extends Controller
             'file' => 'required|file'
         ]);
 
-        $attachment = $this->attachment->newQuery()->findOrFail($attachmentId);
+        $attachment = Attachment::query()->findOrFail($attachmentId);
         $this->checkOwnablePermission('view', $attachment->page);
         $this->checkOwnablePermission('page-update', $attachment->page);
         $this->checkOwnablePermission('attachment-create', $attachment);
@@ -89,7 +87,7 @@ class AttachmentController extends Controller
      */
     public function getUpdateForm(string $attachmentId)
     {
-        $attachment = $this->attachment->findOrFail($attachmentId);
+        $attachment = Attachment::query()->findOrFail($attachmentId);
 
         $this->checkOwnablePermission('page-update', $attachment->page);
         $this->checkOwnablePermission('attachment-create', $attachment);
@@ -202,9 +200,10 @@ class AttachmentController extends Controller
      * @throws FileNotFoundException
      * @throws NotFoundException
      */
-    public function get(string $attachmentId)
+    public function get(Request $request, string $attachmentId)
     {
-        $attachment = $this->attachment->findOrFail($attachmentId);
+        /** @var Attachment $attachment */
+        $attachment = Attachment::query()->findOrFail($attachmentId);
         try {
             $page = $this->pageRepo->getById($attachment->uploaded_to);
         } catch (NotFoundException $exception) {
@@ -217,8 +216,13 @@ class AttachmentController extends Controller
             return redirect($attachment->path);
         }
 
+        $fileName = $attachment->getFileName();
         $attachmentContents = $this->attachmentService->getAttachmentFromStorage($attachment);
-        return $this->downloadResponse($attachmentContents, $attachment->getFileName());
+
+        if ($request->get('open') === 'true') {
+            return $this->inlineDownloadResponse($attachmentContents, $fileName);
+        }
+        return $this->downloadResponse($attachmentContents, $fileName);
     }
 
     /**
@@ -227,7 +231,7 @@ class AttachmentController extends Controller
      */
     public function delete(string $attachmentId)
     {
-        $attachment = $this->attachment->findOrFail($attachmentId);
+        $attachment = Attachment::query()->findOrFail($attachmentId);
         $this->checkOwnablePermission('attachment-delete', $attachment);
         $this->attachmentService->deleteFile($attachment);
         return response()->json(['message' => trans('entities.attachments_deleted')]);

app/Http/Controllers/Controller.php

@@ -6,6 +6,7 @@ use BookStack\Facades\Activity;
 use BookStack\Interfaces\Loggable;
 use BookStack\HasCreatorAndUpdater;
 use BookStack\Model;
+use finfo;
 use Illuminate\Foundation\Bus\DispatchesJobs;
 use Illuminate\Foundation\Validation\ValidatesRequests;
 use Illuminate\Http\Exceptions\HttpResponseException;
@@ -121,6 +122,20 @@ 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.
+     */
+    protected function inlineDownloadResponse(string $content, string $fileName): Response
+    {
+        $finfo = new finfo(FILEINFO_MIME_TYPE);
+        $mime = $finfo->buffer($content) ?: 'application/octet-stream';
+        return response()->make($content, 200, [
+            'Content-Type'        => $mime,
+            'Content-Disposition' => 'inline; filename="' . $fileName . '"'
+        ]);
+    }
+
     /**
      * Show a positive, successful notification to the user on next view load.
      */

app/Uploads/Attachment.php

@@ -41,12 +41,12 @@ class Attachment extends Model
     /**
      * Get the url of this file.
      */
-    public function getUrl(): string
+    public function getUrl($openInline = false): string
     {
         if ($this->external && strpos($this->path, 'http') !== 0) {
             return $this->path;
         }
-        return url('/attachments/' . $this->id);
+        return url('/attachments/' . $this->id . ($openInline ? '?open=true' : ''));
     }
 
     /**

app/Uploads/AttachmentService.php

@@ -3,8 +3,10 @@
 use BookStack\Exceptions\FileUploadException;
 use Exception;
 use Illuminate\Contracts\Filesystem\Factory as FileSystem;
+use Illuminate\Contracts\Filesystem\FileNotFoundException;
 use Illuminate\Contracts\Filesystem\Filesystem as FileSystemInstance;
 use Illuminate\Support\Str;
+use Log;
 use Symfony\Component\HttpFoundation\File\UploadedFile;
 
 class AttachmentService
@@ -38,11 +40,9 @@ class AttachmentService
 
     /**
      * Get an attachment from storage.
-     * @param Attachment $attachment
+     * @throws FileNotFoundException
-     * @return string
-     * @throws \Illuminate\Contracts\Filesystem\FileNotFoundException
      */
-    public function getAttachmentFromStorage(Attachment $attachment)
+    public function getAttachmentFromStorage(Attachment $attachment): string
     {
         return $this->getStorage()->get($attachment->path);
     }
@@ -202,7 +202,7 @@ class AttachmentService
         try {
             $storage->put($attachmentPath, $attachmentData);
         } catch (Exception $e) {
-            \Log::error('Error when attempting file upload:' . $e->getMessage());
+            Log::error('Error when attempting file upload:' . $e->getMessage());
             throw new FileUploadException(trans('errors.path_not_writable', ['filePath' => $attachmentPath]));
         }
 

composer.json

@@ -8,6 +8,7 @@
         "php": "^7.3|^8.0",
         "ext-curl": "*",
         "ext-dom": "*",
+        "ext-fileinfo": "*",
         "ext-gd": "*",
         "ext-json": "*",
         "ext-mbstring": "*",

resources/js/components/attachments-list.js

@@ -0,0 +1,47 @@
+/**
+ * Attachments List
+ * Adds '?open=true' query to file attachment links
+ * when ctrl/cmd is pressed down.
+ * @extends {Component}
+ */
+class AttachmentsList {
+
+    setup() {
+        this.container = this.$el;
+        this.setupListeners();
+    }
+
+    setupListeners() {
+        const isExpectedKey = (event) => event.key === 'Control' || event.key === 'Meta';
+        window.addEventListener('keydown', event => {
+             if (isExpectedKey(event)) {
+                this.addOpenQueryToLinks();
+             }
+        }, {passive: true});
+        window.addEventListener('keyup', event => {
+            if (isExpectedKey(event)) {
+                this.removeOpenQueryFromLinks();
+            }
+        }, {passive: true});
+    }
+
+    addOpenQueryToLinks() {
+        const links = this.container.querySelectorAll('a.attachment-file');
+        for (const link of links) {
+            if (link.href.split('?')[1] !== 'open=true') {
+                link.href = link.href + '?open=true';
+                link.setAttribute('target', '_blank');
+            }
+        }
+    }
+
+    removeOpenQueryFromLinks() {
+        const links = this.container.querySelectorAll('a.attachment-file');
+        for (const link of links) {
+            link.href = link.href.split('?')[0];
+            link.removeAttribute('target');
+        }
+    }
+}
+
+export default AttachmentsList;

resources/js/components/index.js

@@ -2,6 +2,7 @@ import addRemoveRows from "./add-remove-rows.js"
 import ajaxDeleteRow from "./ajax-delete-row.js"
 import ajaxForm from "./ajax-form.js"
 import attachments from "./attachments.js"
+import attachmentsList from "./attachments-list.js"
 import autoSuggest from "./auto-suggest.js"
 import backToTop from "./back-to-top.js"
 import bookSort from "./book-sort.js"
@@ -56,6 +57,7 @@ const componentMapping = {
     "ajax-delete-row": ajaxDeleteRow,
     "ajax-form": ajaxForm,
     "attachments": attachments,
+    "attachments-list": attachmentsList,
     "auto-suggest": autoSuggest,
     "back-to-top": backToTop,
     "book-sort": bookSort,

resources/views/attachments/list.blade.php

@@ -1,8 +1,10 @@
-@foreach($attachments as $attachment)
+<div component="attachments-list">
-    <div class="attachment icon-list">
+    @foreach($attachments as $attachment)
-        <a class="icon-list-item py-xs" href="{{ $attachment->getUrl() }}" @if($attachment->external) target="_blank" @endif>
+        <div class="attachment icon-list">
-            <span class="icon">@icon($attachment->external ? 'export' : 'file')</span>
+            <a class="icon-list-item py-xs attachment-{{ $attachment->external ? 'link' : 'file' }}" href="{{ $attachment->getUrl() }}" @if($attachment->external) target="_blank" @endif>
-            <span>{{ $attachment->name }}</span>
+                <span class="icon">@icon($attachment->external ? 'export' : 'file')</span>
-        </a>
+                <span>{{ $attachment->name }}</span>
-    </div>
+            </a>
-@endforeach
+        </div>
+    @endforeach
+</div>

tests/Uploads/AttachmentTest.php

@@ -4,11 +4,9 @@ use BookStack\Entities\Tools\TrashCan;
 use BookStack\Entities\Repos\PageRepo;
 use BookStack\Uploads\Attachment;
 use BookStack\Entities\Models\Page;
-use BookStack\Auth\Permissions\PermissionService;
 use BookStack\Uploads\AttachmentService;
 use Illuminate\Http\UploadedFile;
 use Tests\TestCase;
-use Tests\TestResponse;
 
 class AttachmentTest extends TestCase
 {
@@ -57,7 +55,7 @@ class AttachmentTest extends TestCase
 
     public function test_file_upload()
     {
-        $page = Page::first();
+        $page = Page::query()->first();
         $this->asAdmin();
         $admin = $this->getAdmin();
         $fileName = 'upload_test_file.txt';
@@ -85,7 +83,7 @@ class AttachmentTest extends TestCase
 
     public function test_file_upload_does_not_use_filename()
     {
-        $page = Page::first();
+        $page = Page::query()->first();
         $fileName = 'upload_test_file.txt';
 
 
@@ -99,7 +97,7 @@ class AttachmentTest extends TestCase
 
     public function test_file_display_and_access()
     {
-        $page = Page::first();
+        $page = Page::query()->first();
         $this->asAdmin();
         $fileName = 'upload_test_file.txt';
 
@@ -119,7 +117,7 @@ class AttachmentTest extends TestCase
 
     public function test_attaching_link_to_page()
     {
-        $page = Page::first();
+        $page = Page::query()->first();
         $admin = $this->getAdmin();
         $this->asAdmin();
 
@@ -156,7 +154,7 @@ class AttachmentTest extends TestCase
 
     public function test_attachment_updating()
     {
-        $page = Page::first();
+        $page = Page::query()->first();
         $this->asAdmin();
 
         $attachment = $this->createAttachment($page);
@@ -180,7 +178,7 @@ class AttachmentTest extends TestCase
 
     public function test_file_deletion()
     {
-        $page = Page::first();
+        $page = Page::query()->first();
         $this->asAdmin();
         $fileName = 'deletion_test.txt';
         $this->uploadFile($fileName, $page->id);
@@ -202,7 +200,7 @@ class AttachmentTest extends TestCase
 
     public function test_attachment_deletion_on_page_deletion()
     {
-        $page = Page::first();
+        $page = Page::query()->first();
         $this->asAdmin();
         $fileName = 'deletion_test.txt';
         $this->uploadFile($fileName, $page->id);
@@ -230,7 +228,7 @@ class AttachmentTest extends TestCase
     {
         $admin = $this->getAdmin();
         $viewer = $this->getViewer();
-        $page = Page::first(); /** @var Page $page */
+        $page = Page::query()->first(); /** @var Page $page */
 
         $this->actingAs($admin);
         $fileName = 'permission_test.txt';
@@ -253,7 +251,7 @@ class AttachmentTest extends TestCase
 
     public function test_data_and_js_links_cannot_be_attached_to_a_page()
     {
-        $page = Page::first();
+        $page = Page::query()->first();
         $this->asAdmin();
 
         $badLinks = [
@@ -291,4 +289,22 @@ class AttachmentTest extends TestCase
             ]);
         }
     }
+
+    public function test_file_access_with_open_query_param_provides_inline_response_with_correct_content_type()
+    {
+        $page = Page::query()->first();
+        $this->asAdmin();
+        $fileName = 'upload_test_file.txt';
+
+        $upload = $this->uploadFile($fileName, $page->id);
+        $upload->assertStatus(200);
+        $attachment = Attachment::query()->orderBy('id', 'desc')->take(1)->first();
+
+        $attachmentGet = $this->get($attachment->getUrl(true));
+        // http-foundation/Response does some 'fixing' of responses to add charsets to text responses.
+        $attachmentGet->assertHeader('Content-Type', 'text/plain; charset=UTF-8');
+        $attachmentGet->assertHeader('Content-Disposition', "inline; filename=\"upload_test_file.txt\"");
+
+        $this->deleteUploads();
+    }
 }