From 57540164fa007c5c6bdb5bfda50911ad94afd9f3 Mon Sep 17 00:00:00 2001 From: ignacionelson Date: Mon, 24 Aug 2026 16:24:39 -0300 Subject: [PATCH] Read a file from the disk it is actually on, everywhere MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two routes still assumed every file sits on local disk, which stopped being true the moment external storage was switched on. A share link answered with X-Accel-Redirect whatever the file's disk said, pointing nginx at a path it has nothing behind; a public listing built a thumbnail from Storage::disk('files')->path(), which for an externally stored file is a path nobody ever wrote. Both fail only for installs using S3, and only on those two routes, so the same file downloading correctly from the file manager made the share link look like the broken thing rather than where the file lives. Neither is a new rule. FileDownloadController and FileThumbnailController already did it right, which is the actual finding: the knowledge was sitting in a private method on one class and inline in another, so the next caller could not inherit it and did not. Both are now objects with one job. StoredFileResponse replaces InlineFileResponse and grows an attachment() alongside inline(), since the two differ only by disposition. LocalSourceFile takes a closure rather than returning a path: the version that returned one also left the caller to unlink it, and both of those are exactly the mistakes made here. The regression tests fail against the previous controllers — checked in both directions rather than assumed. --- CHANGELOG.md | 8 ++ .../Files/Delivery/InlineFileResponse.php | 55 ------------- .../Files/Delivery/StoredFileResponse.php | 70 ++++++++++++++++ .../Controllers/FileDownloadController.php | 33 ++------ .../Controllers/FileThumbnailController.php | 57 +++---------- .../Controllers/PublicShareController.php | 10 +-- .../Files/Thumbnails/LocalSourceFile.php | 80 +++++++++++++++++++ .../Controllers/PublicGroupsController.php | 21 ++++- tests/Feature/Files/ShareLinksTest.php | 35 ++++++++ tests/Feature/Groups/PublicGroupsTest.php | 27 +++++++ 10 files changed, 260 insertions(+), 136 deletions(-) delete mode 100644 app/Modules/Files/Delivery/InlineFileResponse.php create mode 100644 app/Modules/Files/Delivery/StoredFileResponse.php create mode 100644 app/Modules/Files/Thumbnails/LocalSourceFile.php diff --git a/CHANGELOG.md b/CHANGELOG.md index bd7a59dc..18594149 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,14 @@ a version is cut. ### Fixed +- **Downloads and thumbnails for installations using external storage.** Two places assumed every + file sat on the server's own disk, which stopped being true the moment S3-compatible storage was + switched on. A share link to a file held in a bucket produced a broken download, and a public + listing could not draw a thumbnail for one at all — while the same file downloaded and previewed + correctly everywhere else, which made it look like the share link or the listing was at fault + rather than where the file lived. Both now read the file from wherever it actually is. Nothing + changes for installations keeping files on local disk, which is most of them. + - **One confirmation message instead of two.** Saving a new client, system user or role showed the same green "Client created." twice, stacked. So did deleting one. It was only ever cosmetic — nothing happened twice — but it read as though something had, which is the last thing a diff --git a/app/Modules/Files/Delivery/InlineFileResponse.php b/app/Modules/Files/Delivery/InlineFileResponse.php deleted file mode 100644 index acf62a47..00000000 --- a/app/Modules/Files/Delivery/InlineFileResponse.php +++ /dev/null @@ -1,55 +0,0 @@ - seeking through an hour of footage issues a long tail - * of Range requests; nginx's static handler answers those with 206s on - * its own, and drops the Content-Length below in favour of the range it - * actually served. Anything else — S3 and friends — gets a short-lived - * presigned URL carrying an inline disposition, which the object store - * ranges just as well. - * - * Callers must have established that the mime type is inline-safe first; - * PreviewKind is the allowlist, and the reason there is one. - */ -class InlineFileResponse -{ - public function make(File $file): Response|RedirectResponse - { - if ($file->disk !== 'files') { - $url = Storage::disk($file->disk)->temporaryUrl( - $file->path, - now()->addHour(), - ['ResponseContentDisposition' => ContentDisposition::inline($file->original_name)], - ); - - return redirect()->away($url); - } - - return response('', 200, [ - 'X-Accel-Redirect' => '/protected-files/'.$file->path, - 'Content-Type' => $file->mime_type, - 'Content-Disposition' => ContentDisposition::inline($file->original_name), - 'Content-Length' => (string) $file->size, - ]); - } -} diff --git a/app/Modules/Files/Delivery/StoredFileResponse.php b/app/Modules/Files/Delivery/StoredFileResponse.php new file mode 100644 index 00000000..3db34524 --- /dev/null +++ b/app/Modules/Files/Delivery/StoredFileResponse.php @@ -0,0 +1,70 @@ +disk` decides how the bytes travel. + * + * Local disk: X-Accel-Redirect, so nginx streams the file and PHP never + * touches the bytes. Anything else — S3, GCS and friends — gets a + * short-lived presigned URL carrying the disposition, which an object + * store ranges just as well. + * + * That distinction matters most for inline(): a