From a8b1987e2ce1e6b9e16ed19bdab49a3a72965c00 Mon Sep 17 00:00:00 2001 From: denkfabrik-li <274324701+denkfabrik-li@users.noreply.github.com> Date: Wed, 26 Aug 2026 03:15:49 +0200 Subject: [PATCH] Hand over a public download from the disk the file is on MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 5754016 moved this controller's thumbnail() and preview() onto StoredFileResponse and left download(), the last method in the same class, building its own response: 'X-Accel-Redirect' => '/protected-files/'.$file->path, That prefix is nginx's internal location for the local files disk, and $file->disk is never consulted. On an install with external storage switched on it names a path nothing ever wrote, so the public download fails — while the same file downloads correctly from the file manager and from a share link, and previews correctly from this very page, because all three go through the object that knows the rule. That commit's own message names the shape: 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". It is an object now, and this is the call site that was not moved onto it. StoredFileResponse is already injected here as $this->bytes — preview(), two methods above, uses it — and attachment() is the method FileDownloadController and PublicShareController already call. Nothing changes for a local install: attachment() emits the same four headers this method wrote by hand, through the same ContentDisposition call. The return type widens to Response|RedirectResponse because a non-local disk answers with a redirect to a presigned URL, which is the signature preview() already declares. The regression test fails against the unfixed controller — checked in both directions rather than assumed. The existing local-disk case grew assertions for the other three headers, so "unchanged for local" is pinned rather than argued: it passes before and after. --- .../Controllers/PublicGroupsController.php | 9 +---- tests/Feature/Groups/PublicGroupsTest.php | 38 ++++++++++++++++++- 2 files changed, 39 insertions(+), 8 deletions(-) diff --git a/app/Modules/Groups/Http/Controllers/PublicGroupsController.php b/app/Modules/Groups/Http/Controllers/PublicGroupsController.php index d67019b9..f4e292b2 100644 --- a/app/Modules/Groups/Http/Controllers/PublicGroupsController.php +++ b/app/Modules/Groups/Http/Controllers/PublicGroupsController.php @@ -326,7 +326,7 @@ class PublicGroupsController extends Controller return route('public.preview', [$publicSlug, $file->slug]); } - public function download(string $publicSlug, File $file): Response + public function download(string $publicSlug, File $file): Response|RedirectResponse { $this->guardSlug($publicSlug); @@ -340,11 +340,6 @@ class PublicGroupsController extends Controller $this->activity->log(Action::PublicFileDownloaded, 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); } } diff --git a/tests/Feature/Groups/PublicGroupsTest.php b/tests/Feature/Groups/PublicGroupsTest.php index 0a4a64bf..cea1b4dc 100644 --- a/tests/Feature/Groups/PublicGroupsTest.php +++ b/tests/Feature/Groups/PublicGroupsTest.php @@ -191,15 +191,51 @@ test('download serves a public file and 404s a non-public one regardless of grou $notPublic = publicListingFile(['name' => 'Not Downloadable', 'public' => false]); $this->actingAs($staff)->post("/files/{$notPublic->id}/assignments", ['type' => 'group', 'id' => $group->id]); + // The whole header set, not only the path: local delivery is what this + // route used to build by hand, and moving it behind StoredFileResponse + // has to leave a local install's response exactly as it was. $this->get("/public/files/{$public->slug}/download") ->assertOk() - ->assertHeader('X-Accel-Redirect', '/protected-files/'.$public->path); + ->assertHeader('X-Accel-Redirect', '/protected-files/'.$public->path) + ->assertHeader('Content-Type', 'application/pdf') + ->assertHeader('Content-Disposition', 'attachment; filename="report.pdf"') + ->assertHeader('Content-Length', (string) $public->size); expect(ActivityLog::query()->where('action', Action::PublicFileDownloaded)->where('subject_name', 'Downloadable')->exists())->toBeTrue(); $this->get("/public/files/{$notPublic->slug}/download")->assertNotFound(); }); +test('a public download of an externally stored file hands out a presigned url, not an nginx path', function () { + // The bug this covers: this route answered every download with + // X-Accel-Redirect regardless of the file's disk, so a public download + // of an externally stored file pointed nginx at a path nothing ever + // wrote. Its two neighbours on this same controller, thumbnail() and + // preview(), were moved onto the shared delivery object; download() + // was left building the response itself. + 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 = publicListingFile(['name' => 'Externally Stored', 'disk' => 'files_external']); + + $response = $this->get(route('public.download', ['public', $file->slug])); + + $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"'); + + // Delivery moved; the checks in front of it did not. The download is + // still logged, which is also what the download limit counts. + expect(ActivityLog::query()->where('action', Action::PublicFileDownloaded)->where('subject_name', 'Externally Stored')->exists())->toBeTrue(); +}); + test('an expired public file 404s on its detail, thumbnail, and download routes, and drops out of the standalone listing', function () { $expired = publicListingFile(['name' => 'Expired', 'expires_at' => now()->subDay()]);