From cb46ced01a8a5a683b8e8b908028cc5b0c7b5cda Mon Sep 17 00:00:00 2001 From: Andrew Herron Date: Thu, 30 Jul 2026 17:21:51 +1000 Subject: [PATCH 1/2] Set content-disposition 'attachment' for both local and S3 downloads. Added a bunch of tests. Fixes #1148. --- .../Controllers/Api/V1/ExportController.php | 4 +- .../Api/V1/TimeEntryController.php | 8 +- app/Providers/AppServiceProvider.php | 27 ++++ .../Api/V1/ApiEndpointTestAbstract.php | 25 ++++ .../Endpoint/Api/V1/ExportEndpointTest.php | 31 ++++ .../Endpoint/Api/V1/TimeEntryEndpointTest.php | 61 ++++++++ .../Endpoint/Web/FileDownloadEndpointTest.php | 134 ++++++++++++++++++ 7 files changed, 287 insertions(+), 3 deletions(-) create mode 100644 tests/Unit/Endpoint/Web/FileDownloadEndpointTest.php diff --git a/app/Http/Controllers/Api/V1/ExportController.php b/app/Http/Controllers/Api/V1/ExportController.php index adca0462e..91d6164c4 100644 --- a/app/Http/Controllers/Api/V1/ExportController.php +++ b/app/Http/Controllers/Api/V1/ExportController.php @@ -28,7 +28,9 @@ public function export(Organization $organization, ExportService $exportService) $filepath = $exportService->export($organization); $downloadUrl = Storage::disk(config('filesystems.private')) - ->temporaryUrl($filepath, Carbon::now()->addMinutes(10)); + ->temporaryUrl($filepath, Carbon::now()->addMinutes(10), [ + 'ResponseContentDisposition' => 'attachment; filename="'.basename($filepath).'"', + ]); return new JsonResponse([ 'success' => true, diff --git a/app/Http/Controllers/Api/V1/TimeEntryController.php b/app/Http/Controllers/Api/V1/TimeEntryController.php index b0a3bf314..585bd242b 100644 --- a/app/Http/Controllers/Api/V1/TimeEntryController.php +++ b/app/Http/Controllers/Api/V1/TimeEntryController.php @@ -334,7 +334,9 @@ public function indexExport(Organization $organization, TimeEntryIndexExportRequ return response()->json([ 'download_url' => Storage::disk(config('filesystems.private')) - ->temporaryUrl($path, now()->addMinutes(5)), + ->temporaryUrl($path, now()->addMinutes(5), [ + 'ResponseContentDisposition' => 'attachment; filename="'.$filename.'"', + ]), ]); } @@ -545,7 +547,9 @@ public function aggregateExport(Organization $organization, TimeEntryAggregateEx return response()->json([ 'download_url' => Storage::disk(config('filesystems.private')) - ->temporaryUrl($path, now()->addMinutes(5)), + ->temporaryUrl($path, now()->addMinutes(5), [ + 'ResponseContentDisposition' => 'attachment; filename="'.$filename.'"', + ]), ]); } diff --git a/app/Providers/AppServiceProvider.php b/app/Providers/AppServiceProvider.php index 9856f21f7..699b63ecd 100644 --- a/app/Providers/AppServiceProvider.php +++ b/app/Providers/AppServiceProvider.php @@ -20,6 +20,7 @@ use App\Service\IpLookup\IpLookupServiceContract; use App\Service\IpLookup\NoIpLookupService; use App\Service\PermissionStore; +use DateTimeInterface; use Dedoc\Scramble\Scramble; use Dedoc\Scramble\Support\Generator\OpenApi; use Dedoc\Scramble\Support\Generator\SecurityScheme; @@ -29,8 +30,13 @@ use Illuminate\Database\Eloquent\Model; use Illuminate\Database\Eloquent\Relations\Relation; use Illuminate\Foundation\Application; +use Illuminate\Http\Request; use Illuminate\Support\Facades\Route; +use Illuminate\Support\Facades\Storage; +use Illuminate\Support\Facades\URL; use Illuminate\Support\ServiceProvider; +use Illuminate\Support\Str; +use Symfony\Component\HttpFoundation\StreamedResponse; class AppServiceProvider extends ServiceProvider { @@ -98,6 +104,27 @@ public function boot(): void $this->app->bind(IpLookupServiceContract::class, NoIpLookupService::class); $this->app->bind(BillingContract::class); + // Storage + // The local driver ignores the ResponseContentDisposition option of temporaryUrl, + // so mirror it through the signed query parameters of the storage route. + $privateDisk = config('filesystems.private'); + if (config('filesystems.disks.'.$privateDisk.'.driver') === 'local') { + $disk = Storage::disk($privateDisk); + $disk->serveUsing(function (Request $request, string $path, array $headers) use ($disk): StreamedResponse { + return $disk->response($path, null, $headers, $request->query('disposition', 'inline')); + }); + $disk->buildTemporaryUrlsUsing(function (string $path, DateTimeInterface $expiration, array $options) use ($privateDisk): string { + $parameters = array_filter([ + 'path' => $path, + 'disposition' => isset($options['ResponseContentDisposition']) + ? Str::before($options['ResponseContentDisposition'], ';') + : null, + ]); + + return url(URL::temporarySignedRoute('storage.'.$privateDisk, $expiration, $parameters, absolute: false)); + }); + } + // Routing Route::model('member', Member::class); Route::model('invitation', OrganizationInvitation::class); diff --git a/tests/Unit/Endpoint/Api/V1/ApiEndpointTestAbstract.php b/tests/Unit/Endpoint/Api/V1/ApiEndpointTestAbstract.php index b3c7adebd..a59bd2eec 100644 --- a/tests/Unit/Endpoint/Api/V1/ApiEndpointTestAbstract.php +++ b/tests/Unit/Endpoint/Api/V1/ApiEndpointTestAbstract.php @@ -4,6 +4,9 @@ namespace Tests\Unit\Endpoint\Api\V1; +use Closure; +use DateTimeInterface; +use Illuminate\Support\Facades\Storage; use Illuminate\Testing\TestResponse; use Tests\TestCaseWithDatabase; @@ -16,4 +19,26 @@ protected function assertResponseCode(TestResponse $response, int $statusCode): } $response->assertStatus($statusCode); } + + /** + * Replaces the temporary URL builder of the private disk to capture the options + * passed to temporaryUrl. Returns a closure that yields the captured options. + * + * @return Closure(): (array|null) + */ + protected function captureTemporaryUrlOptions(): Closure + { + $captured = null; + Storage::disk(config('filesystems.private'))->buildTemporaryUrlsUsing( + function (string $path, DateTimeInterface $expiration, array $options) use (&$captured): string { + $captured = $options; + + return 'https://storage.fake/'.$path; + } + ); + + return function () use (&$captured): ?array { + return $captured; + }; + } } diff --git a/tests/Unit/Endpoint/Api/V1/ExportEndpointTest.php b/tests/Unit/Endpoint/Api/V1/ExportEndpointTest.php index ca658051f..01b3f0c6c 100644 --- a/tests/Unit/Endpoint/Api/V1/ExportEndpointTest.php +++ b/tests/Unit/Endpoint/Api/V1/ExportEndpointTest.php @@ -90,4 +90,35 @@ public function test_export_calls_export_service_if_user_has_permission(): void $response->assertJsonPath('success', true); $this->assertStringContainsString($filepath, $response->json('download_url')); } + + public function test_export_requests_the_download_url_as_an_attachment(): void + { + // Arrange + $user = $this->createUserWithPermission([ + 'export', + ]); + $filepath = 'exports/export_test.zip'; + $this->mock(ExportService::class, function (MockInterface $mock) use (&$user, $filepath): void { + $mock->shouldReceive('export') + ->withArgs(function (Organization $organization) use (&$user): bool { + return $organization->is($user->organization); + }) + ->andReturn($filepath) + ->once(); + }); + Passport::actingAs($user->user); + $capturedOptions = $this->captureTemporaryUrlOptions(); + + // Act + $response = $this->postJson(route('api.v1.export.export', [ + 'organization' => $user->organization->getKey(), + ])); + + // Assert + $response->assertStatus(200); + $options = $capturedOptions(); + $this->assertIsArray($options); + $this->assertSame('attachment; filename="export_test.zip"', $options['ResponseContentDisposition'] ?? null); + $this->assertSame('https://storage.fake/'.$filepath, $response->json('download_url')); + } } diff --git a/tests/Unit/Endpoint/Api/V1/TimeEntryEndpointTest.php b/tests/Unit/Endpoint/Api/V1/TimeEntryEndpointTest.php index cbd2b49fc..5e623adb0 100644 --- a/tests/Unit/Endpoint/Api/V1/TimeEntryEndpointTest.php +++ b/tests/Unit/Endpoint/Api/V1/TimeEntryEndpointTest.php @@ -945,6 +945,35 @@ public function test_index_export_endpoint_can_create_a_detailed_time_entry_repo $this->assertResponseCode($response, 200); } + public function test_index_export_endpoint_requests_the_download_url_as_an_attachment(): void + { + // Arrange + $data = $this->createUserWithPermission([ + 'time-entries:view:all', + ]); + TimeEntry::factory()->forOrganization($data->organization)->forMember($data->member)->startWithDuration(Carbon::now(), 100)->create(); + Passport::actingAs($data->user); + $capturedOptions = $this->captureTemporaryUrlOptions(); + + // Act + $response = $this->getJson(route('api.v1.time-entries.index-export', [ + $data->organization->getKey(), + 'format' => ExportFormat::CSV, + 'start' => Carbon::now()->startOfYear()->toIso8601ZuluString(), + 'end' => Carbon::now()->endOfYear()->toIso8601ZuluString(), + ])); + + // Assert + $this->assertResponseCode($response, 200); + $options = $capturedOptions(); + $this->assertIsArray($options); + $this->assertMatchesRegularExpression( + '/^attachment; filename="time-entries-export-.+\.csv"$/', + $options['ResponseContentDisposition'] ?? '' + ); + $this->assertStringStartsWith('https://storage.fake/exports/', $response->json('download_url')); + } + public function test_index_export_endpoint_can_create_a_detailed_time_entry_report_in_format_ods(): void { // Arrange @@ -1319,6 +1348,38 @@ public function test_aggregate_export_endpoints_can_create_a_csv_report(): void $this->assertResponseCode($response, 200); } + public function test_aggregate_export_endpoint_requests_the_download_url_as_an_attachment(): void + { + // Arrange + $data = $this->createUserWithPermission([ + 'time-entries:view:all', + ]); + TimeEntry::factory()->forOrganization($data->organization)->forMember($data->member)->startWithDuration(Carbon::now(), 100)->create(); + Passport::actingAs($data->user); + $capturedOptions = $this->captureTemporaryUrlOptions(); + + // Act + $response = $this->getJson(route('api.v1.time-entries.aggregate-export', [ + $data->organization->getKey(), + 'format' => ExportFormat::CSV, + 'group' => TimeEntryAggregationType::Client, + 'sub_group' => TimeEntryAggregationType::Project, + 'history_group' => TimeEntryAggregationTypeInterval::Month, + 'start' => Carbon::now()->startOfYear()->toIso8601ZuluString(), + 'end' => Carbon::now()->endOfYear()->toIso8601ZuluString(), + ])); + + // Assert + $this->assertResponseCode($response, 200); + $options = $capturedOptions(); + $this->assertIsArray($options); + $this->assertMatchesRegularExpression( + '/^attachment; filename="time-entries-report-.+\.csv"$/', + $options['ResponseContentDisposition'] ?? '' + ); + $this->assertStringStartsWith('https://storage.fake/exports/', $response->json('download_url')); + } + public function test_aggregate_export_endpoints_can_create_a_csv_report_as_employee_role_with_show_billable_rate(): void { // Arrange diff --git a/tests/Unit/Endpoint/Web/FileDownloadEndpointTest.php b/tests/Unit/Endpoint/Web/FileDownloadEndpointTest.php new file mode 100644 index 000000000..3d5ac914d --- /dev/null +++ b/tests/Unit/Endpoint/Web/FileDownloadEndpointTest.php @@ -0,0 +1,134 @@ +privateDisk(); + $disk->put('exports/test-attachment.csv', 'Description,Duration'); + $url = $disk->temporaryUrl('exports/test-attachment.csv', now()->addMinutes(5), [ + 'ResponseContentDisposition' => 'attachment; filename="test-attachment.csv"', + ]); + + // Act + $response = $this->get($url); + + // Assert + $response->assertOk(); + $response->assertDownload('test-attachment.csv'); + $disk->delete('exports/test-attachment.csv'); + } + + public function test_temporary_url_without_options_serves_file_inline(): void + { + // Arrange + $disk = $this->privateDisk(); + $disk->put('exports/test-inline.csv', 'Description,Duration'); + $url = $disk->temporaryUrl('exports/test-inline.csv', now()->addMinutes(5)); + + // Act + $response = $this->get($url); + + // Assert + $response->assertOk(); + $this->assertStringStartsWith('inline', (string) $response->headers->get('Content-Disposition')); + $disk->delete('exports/test-inline.csv'); + } + + public function test_download_fails_with_tampered_disposition(): void + { + // Arrange + $url = $this->privateDisk()->temporaryUrl('exports/test-tampered.csv', now()->addMinutes(5), [ + 'ResponseContentDisposition' => 'attachment; filename="test-tampered.csv"', + ]); + + // Act + $response = $this->get(str_replace('attachment', 'inline', $url)); + + // Assert + $response->assertForbidden(); + } + + public function test_download_fails_with_expired_signature(): void + { + // Arrange + $url = $this->privateDisk()->temporaryUrl('exports/test-expired.csv', now()->addMinutes(5), [ + 'ResponseContentDisposition' => 'attachment; filename="test-expired.csv"', + ]); + $this->travel(6)->minutes(); + + // Act + $response = $this->get($url); + + // Assert + $response->assertForbidden(); + } + + public function test_download_fails_if_file_does_not_exist(): void + { + // Arrange + $url = $this->privateDisk()->temporaryUrl('exports/test-missing.csv', now()->addMinutes(5), [ + 'ResponseContentDisposition' => 'attachment; filename="test-missing.csv"', + ]); + + // Act + $response = $this->get($url); + + // Assert + $response->assertNotFound(); + } + + public function test_temporary_url_of_private_local_disk_points_to_storage_route_with_signed_disposition(): void + { + // Act + $url = $this->privateDisk()->temporaryUrl('exports/test-export.pdf', now()->addMinutes(5), [ + 'ResponseContentDisposition' => 'attachment; filename="test-export.pdf"', + ]); + + // Assert + $this->assertStringContainsString('/storage/exports/test-export.pdf', $url); + $this->assertStringContainsString('disposition=attachment', $url); + $this->assertStringContainsString('signature=', $url); + } + + public function test_temporary_url_of_s3_disk_includes_content_disposition_from_options(): void + { + // Arrange + config([ + 'filesystems.disks.s3.key' => 'test-key', + 'filesystems.disks.s3.secret' => 'test-secret', + 'filesystems.disks.s3.region' => 'us-east-1', + 'filesystems.disks.s3.bucket' => 'test-bucket', + ]); + + // Act + $url = Storage::disk('s3')->temporaryUrl('exports/test-export.pdf', now()->addMinutes(5), [ + 'ResponseContentDisposition' => 'attachment; filename="test-export.pdf"', + ]); + + // Assert + $query = []; + parse_str((string) parse_url($url, PHP_URL_QUERY), $query); + $this->assertSame('attachment; filename="test-export.pdf"', $query['response-content-disposition'] ?? null); + $this->assertArrayHasKey('X-Amz-Signature', $query); + } +} From 30cbf9edc9c4a9e824e329c2a4072877155acfb8 Mon Sep 17 00:00:00 2001 From: Andrew Herron Date: Thu, 30 Jul 2026 17:24:17 +1000 Subject: [PATCH 2/2] Stopped downloads opening a new tab now that it's guaranteed to be an attachment. --- .../js/Components/Common/Reporting/ReportingExportModal.vue | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/resources/js/Components/Common/Reporting/ReportingExportModal.vue b/resources/js/Components/Common/Reporting/ReportingExportModal.vue index 589fa32eb..a081bf285 100644 --- a/resources/js/Components/Common/Reporting/ReportingExportModal.vue +++ b/resources/js/Components/Common/Reporting/ReportingExportModal.vue @@ -9,7 +9,7 @@ const showExportModal = defineModel('show', { default: false }); function downloadCurrentExport() { if (props.exportUrl) { - window.open(props.exportUrl, '_blank')?.focus(); + window.open(props.exportUrl, '_self'); } }