diff --git a/app/Exports/Controllers/BookExportController.php b/app/Exports/Controllers/BookExportController.php index f726175a0..184f7c235 100644 --- a/app/Exports/Controllers/BookExportController.php +++ b/app/Exports/Controllers/BookExportController.php @@ -75,6 +75,6 @@ class BookExportController extends Controller $book = $this->queries->findVisibleBySlugOrFail($bookSlug); $zip = $builder->buildForBook($book); - return $this->download()->streamedDirectly(fopen($zip, 'r'), $bookSlug . '.zip', filesize($zip)); + return $this->download()->streamedFileDirectly($zip, $bookSlug . '.zip', filesize($zip), true); } } diff --git a/app/Exports/Controllers/ChapterExportController.php b/app/Exports/Controllers/ChapterExportController.php index 0d7a5c0d1..4748ca6a8 100644 --- a/app/Exports/Controllers/ChapterExportController.php +++ b/app/Exports/Controllers/ChapterExportController.php @@ -81,6 +81,6 @@ class ChapterExportController extends Controller $chapter = $this->queries->findVisibleBySlugsOrFail($bookSlug, $chapterSlug); $zip = $builder->buildForChapter($chapter); - return $this->download()->streamedDirectly(fopen($zip, 'r'), $chapterSlug . '.zip', filesize($zip)); + return $this->download()->streamedFileDirectly($zip, $chapterSlug . '.zip', filesize($zip), true); } } diff --git a/app/Exports/Controllers/PageExportController.php b/app/Exports/Controllers/PageExportController.php index 34e67ffcf..203495fe9 100644 --- a/app/Exports/Controllers/PageExportController.php +++ b/app/Exports/Controllers/PageExportController.php @@ -85,6 +85,6 @@ class PageExportController extends Controller $page = $this->queries->findVisibleBySlugsOrFail($bookSlug, $pageSlug); $zip = $builder->buildForPage($page); - return $this->download()->streamedDirectly(fopen($zip, 'r'), $pageSlug . '.zip', filesize($zip)); + return $this->download()->streamedFileDirectly($zip, $pageSlug . '.zip', filesize($zip), true); } } diff --git a/app/Exports/ZipExports/ZipExportBuilder.php b/app/Exports/ZipExports/ZipExportBuilder.php index 4c5c638f5..9a52c9a3c 100644 --- a/app/Exports/ZipExports/ZipExportBuilder.php +++ b/app/Exports/ZipExports/ZipExportBuilder.php @@ -84,10 +84,27 @@ class ZipExportBuilder $zip->addEmptyDir('files'); $toRemove = []; - $this->files->extractEach(function ($filePath, $fileRef) use ($zip, &$toRemove) { - $zip->addFile($filePath, "files/$fileRef"); - $toRemove[] = $filePath; - }); + $addedNames = []; + + try { + $this->files->extractEach(function ($filePath, $fileRef) use ($zip, &$toRemove, &$addedNames) { + $entryName = "files/$fileRef"; + $zip->addFile($filePath, $entryName); + $toRemove[] = $filePath; + $addedNames[] = $entryName; + }); + } catch (\Exception $exception) { + // Cleanup the files we've processed so far and respond back with error + foreach ($toRemove as $file) { + unlink($file); + } + foreach ($addedNames as $name) { + $zip->deleteName($name); + } + $zip->close(); + unlink($zipFile); + throw new ZipExportException("Failed to add files for ZIP export, received error: " . $exception->getMessage()); + } $zip->close(); diff --git a/app/Http/DownloadResponseFactory.php b/app/Http/DownloadResponseFactory.php index f29aaa2e4..d06e2bac4 100644 --- a/app/Http/DownloadResponseFactory.php +++ b/app/Http/DownloadResponseFactory.php @@ -9,7 +9,7 @@ use Symfony\Component\HttpFoundation\StreamedResponse; class DownloadResponseFactory { public function __construct( - protected Request $request + protected Request $request, ) { } @@ -35,6 +35,32 @@ class DownloadResponseFactory ); } + /** + * Create a response that downloads the given file via a stream. + * Has the option to delete the provided file once the stream is closed. + */ + public function streamedFileDirectly(string $filePath, string $fileName, int $fileSize, bool $deleteAfter = false): StreamedResponse + { + $stream = fopen($filePath, 'r'); + + if ($deleteAfter) { + // Delete the given file if it still exists after the app terminates + $callback = function () use ($filePath) { + if (file_exists($filePath)) { + unlink($filePath); + } + }; + + // We watch both app terminate and php shutdown to cover both normal app termination + // as well as other potential scenarios (connection termination). + app()->terminating($callback); + register_shutdown_function($callback); + } + + return $this->streamedDirectly($stream, $fileName, $fileSize); + } + + /** * 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, diff --git a/tests/Exports/ZipExportTest.php b/tests/Exports/ZipExportTest.php index 163828c1b..0e17bc0e0 100644 --- a/tests/Exports/ZipExportTest.php +++ b/tests/Exports/ZipExportTest.php @@ -7,6 +7,7 @@ use BookStack\Entities\Repos\BookRepo; use BookStack\Entities\Tools\PageContent; use BookStack\Uploads\Attachment; use BookStack\Uploads\Image; +use FilesystemIterator; use Illuminate\Support\Carbon; use Illuminate\Testing\TestResponse; use Tests\TestCase; @@ -60,6 +61,24 @@ class ZipExportTest extends TestCase $this->assertEquals($instanceId, $zipInstanceId); } + public function test_export_leaves_no_temp_files() + { + $tempDir = sys_get_temp_dir(); + $startTempFileCount = iterator_count((new FileSystemIterator($tempDir, FilesystemIterator::SKIP_DOTS))); + + $page = $this->entities->pageWithinChapter(); + $this->asEditor(); + $pageResp = $this->get($page->getUrl("/export/zip")); + $pageResp->streamedContent(); + $pageResp->assertOk(); + $this->get($page->chapter->getUrl("/export/zip"))->assertOk(); + $this->get($page->book->getUrl("/export/zip"))->assertOk(); + + $afterTempFileCount = iterator_count((new FileSystemIterator($tempDir, FilesystemIterator::SKIP_DOTS))); + + $this->assertEquals($startTempFileCount, $afterTempFileCount); + } + public function test_page_export() { $page = $this->entities->page();