ZIP Exports: Improved temp file tracking & clean-up

diff --git a/app/Exports/Controllers/BookExportController.php b/app/Exports/Controllers/BookExportController.php
index f726175a086acab697ce6e05575f69f90b58be8f..184f7c2350e62d75df3dd384dde642c5c26b2b61 100644 (file)
--- 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 0d7a5c0d195ec94181cbacca251a632d2008e7c0..4748ca6a80a4ce3d93aeea5068d5b055fc3538bc 100644 (file)
--- 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 34e67ffcf7075ede6b30a2842789ef8fa204c7f0..203495fe900126b1a962a5f991a57980b875a786 100644 (file)
--- 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 4c5c638f59193da085734b1294f67acf20228c7d..9a52c9a3cb8d021b8e6f9962edbba75484f76907 100644 (file)
--- 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 f29aaa2e45a131e466a240aa8931d72e9287af6a..d06e2bac44d99ef37640149bb30cf63fc1faa42a 100644 (file)
--- 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 163828c1b6e9785cc74ca8364ffcabeb0300bdac..0e17bc0e01d5124092353beb75ceb0b983f5b90f 100644 (file)
--- 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();