Read a file from the disk it is actually on, everywhere

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

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

app/Modules/Files/Delivery/InlineFileResponse.php

@@ -1,55 +0,0 @@
-<?php
-
-declare(strict_types=1);
-
-namespace App\Modules\Files\Delivery;
-
-use App\Modules\Files\Models\File;
-use App\Support\ContentDisposition;
-use Illuminate\Http\RedirectResponse;
-use Illuminate\Http\Response;
-use Illuminate\Support\Facades\Storage;
-
-/**
- * A stored file's own bytes, served to be looked at rather than saved.
- *
- * The two preview endpoints — FileThumbnailController::preview for
- * someone signed in, PublicGroupsController::preview for a visitor —
- * reach this after they have each authorized in their own way. It
- * authorizes nothing itself; it only knows how to put bytes on the wire
- * for whichever disk the file lives on.
- *
- * Local disk: X-Accel-Redirect, so nginx streams the file and PHP never
- * touches the bytes. That matters more here than it does for a download,
- * because a <video> 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,
-        ]);
-    }
-}

app/Modules/Files/Delivery/StoredFileResponse.php

@@ -0,0 +1,70 @@
+<?php
+
+declare(strict_types=1);
+
+namespace App\Modules\Files\Delivery;
+
+use App\Modules\Files\Models\File;
+use App\Support\ContentDisposition;
+use Illuminate\Http\RedirectResponse;
+use Illuminate\Http\Response;
+use Illuminate\Support\Facades\Storage;
+
+/**
+ * A stored file's own bytes, put on the wire for whichever disk it lives
+ * on.
+ *
+ * Every route that hands over a file reaches this after authorizing in
+ * its own way — a policy, a share token, a public-listing check. It
+ * authorizes nothing itself, and deliberately knows nothing about who is
+ * asking. The one thing it knows is the thing each caller kept getting
+ * wrong on its own: that `$file->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 <video> seeking through
+ * an hour of footage issues a long tail of Range requests, and nginx's
+ * static handler answers those with 206s on its own, dropping the
+ * Content-Length below in favour of the range it actually served.
+ *
+ * Callers of inline() must have established that the mime type is
+ * inline-safe first; PreviewKind is the allowlist, and the reason there
+ * is one.
+ */
+class StoredFileResponse
+{
+    /** Shown in place — a preview. */
+    public function inline(File $file): Response|RedirectResponse
+    {
+        return $this->make($file, ContentDisposition::inline($file->original_name));
+    }
+
+    /** Handed over — a download. */
+    public function attachment(File $file): Response|RedirectResponse
+    {
+        return $this->make($file, ContentDisposition::attachment($file->original_name));
+    }
+
+    private function make(File $file, string $disposition): Response|RedirectResponse
+    {
+        if ($file->disk !== 'files') {
+            $url = Storage::disk($file->disk)->temporaryUrl(
+                $file->path,
+                now()->addHour(),
+                ['ResponseContentDisposition' => $disposition],
+            );
+
+            return redirect()->away($url);
+        }
+
+        return response('', 200, [
+            'X-Accel-Redirect' => '/protected-files/'.$file->path,
+            'Content-Type' => $file->mime_type,
+            'Content-Disposition' => $disposition,
+            'Content-Length' => (string) $file->size,
+        ]);
+    }
+}

app/Modules/Files/Http/Controllers/FileDownloadController.php

@@ -8,28 +8,26 @@ use App\Http\Controllers\Controller;
 use App\Modules\Audit\Action;
 use App\Modules\Audit\ActivityLogger;
 use App\Modules\Files\Access\DownloadAllowance;
+use App\Modules\Files\Delivery\StoredFileResponse;
 use App\Modules\Files\Models\File;
-use App\Support\ContentDisposition;
 use Illuminate\Http\RedirectResponse;
 use Illuminate\Http\Request;
 use Illuminate\Http\Response;
 use Illuminate\Support\Facades\Gate;
-use Illuminate\Support\Facades\Storage;
 
 /**
- * Authorized downloads without the bytes ever traversing PHP: for a file
- * on the local disk, the app checks the policy and answers with
- * X-Accel-Redirect; nginx streams the file from the protected location
- * (brief §3). The cloud edition swaps this for presigned URLs behind the
- * same route. A file on the community-only external storage disk already
- * gets exactly that — a presigned URL redirect — since nginx has no way
- * to serve bytes it doesn't have on disk.
+ * Authorized downloads without the bytes ever traversing PHP: the app
+ * checks the policy, and StoredFileResponse answers with either an
+ * X-Accel-Redirect for nginx to stream from the protected location
+ * (brief §3) or a presigned URL when the file lives on external storage,
+ * since nginx has no way to serve bytes it doesn't have on disk.
  */
 class FileDownloadController extends Controller
 {
     public function __construct(
         private readonly ActivityLogger $activity,
         private readonly DownloadAllowance $allowance,
+        private readonly StoredFileResponse $bytes,
     ) {}
 
     public function __invoke(Request $request, File $file): Response|RedirectResponse
@@ -44,21 +42,6 @@ class FileDownloadController extends Controller
 
         $this->activity->log(Action::FileDownloaded, subject: $file);
 
-        if ($file->disk !== 'files') {
-            $url = Storage::disk($file->disk)->temporaryUrl(
-                $file->path,
-                now()->addHour(),
-                ['ResponseContentDisposition' => ContentDisposition::attachment($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::attachment($file->original_name),
-            'Content-Length' => (string) $file->size,
-        ]);
+        return $this->bytes->attachment($file);
     }
 }

app/Modules/Files/Http/Controllers/FileThumbnailController.php

@@ -8,12 +8,13 @@ use App\Http\Controllers\Controller;
 use App\Modules\Audit\Action;
 use App\Modules\Audit\ActivityLogger;
 use App\Modules\Files\Access\DownloadAllowance;
-use App\Modules\Files\Delivery\InlineFileResponse;
+use App\Modules\Files\Delivery\StoredFileResponse;
 use App\Modules\Files\Models\File;
 use App\Modules\Files\Preview\PreviewKind;
 use App\Modules\Files\Thumbnails\Events\ResolvingImageRendering;
 use App\Modules\Files\Thumbnails\ImageAudience;
 use App\Modules\Files\Thumbnails\ImageRendition;
+use App\Modules\Files\Thumbnails\LocalSourceFile;
 use App\Modules\Files\Thumbnails\ThumbnailGenerator;
 use App\Modules\Platform\Settings\Setting;
 use App\Modules\Platform\Settings\Settings;
@@ -73,7 +74,8 @@ class FileThumbnailController extends Controller
         private readonly ThumbnailGenerator $thumbnails,
         private readonly ActivityLogger $activity,
         private readonly DownloadAllowance $allowance,
-        private readonly InlineFileResponse $inline,
+        private readonly StoredFileResponse $bytes,
+        private readonly LocalSourceFile $source,
         private readonly Settings $settings,
     ) {}
 
@@ -159,7 +161,7 @@ class FileThumbnailController extends Controller
             }
         }
 
-        return $this->inline->make($file);
+        return $this->bytes->inline($file);
     }
 
     /**
@@ -205,15 +207,14 @@ class FileThumbnailController extends Controller
         }
 
         $disk->makeDirectory(dirname($path));
-        $sourcePath = $this->localSourcePathFor($file);
 
-        try {
-            $this->thumbnails->generate($sourcePath, $disk->path($path), $file->mime_type, $audience, $rendition);
-        } finally {
-            if ($file->disk !== 'files') {
-                @unlink($sourcePath);
-            }
-        }
+        $this->source->use($file, fn (string $sourcePath) => $this->thumbnails->generate(
+            $sourcePath,
+            $disk->path($path),
+            $file->mime_type,
+            $audience,
+            $rendition,
+        ));
 
         return $path;
     }
@@ -226,38 +227,4 @@ class FileThumbnailController extends Controller
             'Content-Disposition' => ContentDisposition::inline($file->original_name),
         ]);
     }
-
-    /**
-     * A local-disk file's real path (fast path). Anything else is
-     * stream-copied to a temp file first — the caller unlinks it once
-     * rendering is done.
-     */
-    private function localSourcePathFor(File $file): string
-    {
-        if ($file->disk === 'files') {
-            return Storage::disk('files')->path($file->path);
-        }
-
-        $tempPath = tempnam(sys_get_temp_dir(), 'thumb-src-');
-
-        if ($tempPath === false) {
-            throw new \RuntimeException('Could not create a temp file for '.$file->original_name);
-        }
-
-        $stream = Storage::disk($file->disk)->readStream($file->path);
-        $out = fopen($tempPath, 'wb');
-
-        if ($stream === null || $out === false) {
-            throw new \RuntimeException('Could not read '.$file->original_name.' from its storage disk.');
-        }
-
-        stream_copy_to_stream($stream, $out);
-        fclose($out);
-
-        if (is_resource($stream)) {
-            fclose($stream);
-        }
-
-        return $tempPath;
-    }
 }

app/Modules/Files/Http/Controllers/PublicShareController.php

@@ -8,10 +8,10 @@ use App\Http\Controllers\Controller;
 use App\Modules\Audit\Action;
 use App\Modules\Audit\ActivityLogger;
 use App\Modules\Files\Access\DownloadAllowance;
+use App\Modules\Files\Delivery\StoredFileResponse;
 use App\Modules\Files\Models\Category;
 use App\Modules\Files\Models\File;
 use App\Modules\Files\Models\ShareLink;
-use App\Support\ContentDisposition;
 use Illuminate\Http\RedirectResponse;
 use Illuminate\Http\Response;
 use Inertia\Inertia;
@@ -28,6 +28,7 @@ class PublicShareController extends Controller
     public function __construct(
         private readonly ActivityLogger $activity,
         private readonly DownloadAllowance $allowance,
+        private readonly StoredFileResponse $bytes,
     ) {}
 
     public function show(string $token): InertiaResponse
@@ -101,11 +102,6 @@ class PublicShareController extends Controller
 
         $this->activity->log(Action::ShareLinkDownloaded, subject: $file);
 
-        return response('', 200, [
-            'X-Accel-Redirect' => '/protected-files/'.$file->path,
-            'Content-Type' => $file->mime_type,
-            'Content-Disposition' => ContentDisposition::attachment($file->original_name),
-            'Content-Length' => (string) $file->size,
-        ]);
+        return $this->bytes->attachment($file);
     }
 }

app/Modules/Files/Thumbnails/LocalSourceFile.php

@@ -0,0 +1,80 @@
+<?php
+
+declare(strict_types=1);
+
+namespace App\Modules\Files\Thumbnails;
+
+use App\Modules\Files\Models\File;
+use Illuminate\Support\Facades\Storage;
+use RuntimeException;
+
+/**
+ * A real path on this machine for a stored file, so that something which
+ * can only work on local bytes — image and video rendering, all of which
+ * shells out or hands a path to a C library — can work on any file
+ * whatever disk it lives on.
+ *
+ * A local file is used where it lies. Anything else is stream-copied to a
+ * temp file and removed afterwards.
+ *
+ * The callback shape is the point. This started as a private method on
+ * one controller that returned a path and left the caller to unlink it,
+ * and the second place that needed it did not call it at all — it passed
+ * the *local* disk's path() for a file on external storage, which is a
+ * path that does not exist, so every public-listing thumbnail of an
+ * externally stored file failed. Handing back a path is an invitation to
+ * both of those mistakes; a closure that owns the lifetime is not.
+ */
+class LocalSourceFile
+{
+    /**
+     * @template TReturn
+     *
+     * @param  callable(string): TReturn  $work
+     * @return TReturn
+     */
+    public function use(File $file, callable $work): mixed
+    {
+        if ($file->disk === 'files') {
+            return $work(Storage::disk('files')->path($file->path));
+        }
+
+        $tempPath = tempnam(sys_get_temp_dir(), 'thumb-src-');
+
+        if ($tempPath === false) {
+            throw new RuntimeException('Could not create a temp file for '.$file->original_name);
+        }
+
+        try {
+            $this->copyDown($file, $tempPath);
+
+            return $work($tempPath);
+        } finally {
+            @unlink($tempPath);
+        }
+    }
+
+    private function copyDown(File $file, string $tempPath): void
+    {
+        $stream = Storage::disk($file->disk)->readStream($file->path);
+        $out = fopen($tempPath, 'wb');
+
+        if ($stream === null || $out === false) {
+            if (is_resource($out)) {
+                fclose($out);
+            }
+
+            throw new RuntimeException('Could not read '.$file->original_name.' from its storage disk.');
+        }
+
+        try {
+            stream_copy_to_stream($stream, $out);
+        } finally {
+            fclose($out);
+
+            if (is_resource($stream)) {
+                fclose($stream);
+            }
+        }
+    }
+}

app/Modules/Groups/Http/Controllers/PublicGroupsController.php

@@ -9,13 +9,14 @@ use App\Modules\Audit\Action;
 use App\Modules\Audit\ActivityLogger;
 use App\Modules\Comments\CommentingRules;
 use App\Modules\Files\Access\DownloadAllowance;
-use App\Modules\Files\Delivery\InlineFileResponse;
+use App\Modules\Files\Delivery\StoredFileResponse;
 use App\Modules\Files\Models\Category;
 use App\Modules\Files\Models\File;
 use App\Modules\Files\Models\Folder;
 use App\Modules\Files\Preview\PreviewKind;
 use App\Modules\Files\Thumbnails\ImageAudience;
 use App\Modules\Files\Thumbnails\ImageRendition;
+use App\Modules\Files\Thumbnails\LocalSourceFile;
 use App\Modules\Files\Thumbnails\ThumbnailGenerator;
 use App\Modules\Files\Versions\FileVersionLinks;
 use App\Modules\Groups\Http\Controllers\Concerns\InteractsWithPublicListing;
@@ -81,7 +82,8 @@ class PublicGroupsController extends Controller
         private readonly PublicThemeRegistry $themes,
         private readonly CapabilityRegistry $capabilities,
         private readonly CommentingRules $commenting,
-        private readonly InlineFileResponse $inline,
+        private readonly StoredFileResponse $bytes,
+        private readonly LocalSourceFile $source,
     ) {}
 
     public function index(Request $request, string $publicSlug): InertiaResponse|RedirectResponse
@@ -251,7 +253,18 @@ class PublicGroupsController extends Controller
 
         if (! $disk->exists($thumbnailPath)) {
             $disk->makeDirectory(dirname($thumbnailPath));
-            $this->thumbnails->generate($disk->path($file->path), $disk->path($thumbnailPath), $file->mime_type, ImageAudience::External, ImageRendition::Thumbnail);
+
+            // Never $disk->path($file->path): the rendition is cached on
+            // the local disk, but the *source* lives on whichever disk the
+            // file was uploaded to, and a local path for an externally
+            // stored file is a path that does not exist.
+            $this->source->use($file, fn (string $sourcePath) => $this->thumbnails->generate(
+                $sourcePath,
+                $disk->path($thumbnailPath),
+                $file->mime_type,
+                ImageAudience::External,
+                ImageRendition::Thumbnail,
+            ));
         }
 
         return response('', 200, [
@@ -287,7 +300,7 @@ class PublicGroupsController extends Controller
 
         $this->activity->log(Action::PublicFilePreviewed, subject: $file);
 
-        return $this->inline->make($file);
+        return $this->bytes->inline($file);
     }
 
     /**

tests/Feature/Files/ShareLinksTest.php

@@ -10,6 +10,7 @@ use App\Modules\Files\Models\File;
 use App\Modules\Files\Models\ShareLink;
 use App\Modules\Identity\Models\Role;
 use App\Modules\Identity\Models\RolePermission;
+use Illuminate\Support\Facades\Storage;
 use Illuminate\Support\Str;
 use Inertia\Testing\AssertableInertia;
 
@@ -164,6 +165,40 @@ test('the public show and download routes work with no authenticated user at all
         ->and($entry->actor_name)->toBeNull();
 });
 
+test('a share link to an externally stored file hands out a presigned url, not an nginx path', function () {
+    // The bug this covers: this route used to answer every download with
+    // X-Accel-Redirect regardless of the file's disk, so a share link to a
+    // file on external storage pointed nginx at a path that does not exist
+    // on its filesystem. Every other download path already got this right.
+    Storage::fake('files_external');
+    Storage::disk('files_external')->buildTemporaryUrlsUsing(
+        fn (string $path, $expiration, array $options) => 'https://storage.example.test/'.$path.'?disposition='.urlencode($options['ResponseContentDisposition'] ?? '')
+    );
+
+    $file = shareTestFile($this->admin);
+    $file->update(['disk' => 'files_external']);
+
+    $link = ShareLink::query()->create([
+        'shareable_type' => $file->getMorphClass(),
+        'shareable_id' => $file->id,
+        'token' => Str::random(32),
+    ]);
+
+    $response = $this->get("/s/{$link->token}/download");
+
+    $response->assertRedirect();
+    $response->assertHeaderMissing('X-Accel-Redirect');
+
+    $target = $response->headers->get('Location');
+    expect($target)->toStartWith('https://storage.example.test/'.$file->path)
+        // The filename has to survive into the signed URL, or the download
+        // arrives named after the storage key.
+        ->and(urldecode((string) $target))->toContain('attachment; filename="report.pdf"');
+
+    // The link's counter still moves for an external file.
+    expect($link->refresh()->downloads_count)->toBe(1);
+});
+
 test('an unknown token shows a not-found state instead of a 404', function () {
     $this->get('/s/does-not-exist')->assertOk()->assertInertia(fn (AssertableInertia $page) => $page->where('status', 'not_found'));
     $this->get('/s/does-not-exist/download')->assertRedirect(route('share.show', 'does-not-exist'));

tests/Feature/Groups/PublicGroupsTest.php

@@ -310,6 +310,33 @@ test('the public thumbnail route generates and serves a thumbnail for a public i
     $this->get(route('public.thumbnail', ['public', $privateImage->slug]))->assertNotFound();
 });
 
+test('a public thumbnail renders from external storage rather than a local path that does not exist', function () {
+    // The bug this covers: this route read its *source* through
+    // Storage::disk('files')->path(), which for an externally stored file
+    // is a path nothing ever wrote. The rendition is still cached locally
+    // — only the source moves. FileThumbnailController already handled
+    // this; the public twin did not.
+    Storage::fake('files_external');
+
+    $staff = User::factory()->create();
+    $image = publicListingImageFile($staff);
+
+    // Restage the bytes where an install with external storage configured
+    // would have put them, and remove the local copy so a local path
+    // cannot accidentally satisfy the request.
+    Storage::disk('files_external')->put($image->path, Storage::disk('files')->get($image->path));
+    Storage::disk('files')->delete($image->path);
+    $image->update(['disk' => 'files_external']);
+
+    auth()->logout();
+
+    $this->get(route('public.thumbnail', ['public', $image->slug]))
+        ->assertOk()
+        ->assertHeader('Content-Type', 'image/jpeg');
+
+    expect(Storage::disk('files')->exists("thumbnails/external/{$image->id}.jpg"))->toBeTrue();
+});
+
 test('existing literal routes are unaffected by the new catch-all public routes', function () {
     $this->actingAs(User::factory()->create())->get('/dashboard')->assertOk();
     $this->actingAs(User::factory()->create())->get('/files')->assertOk();