diff --git a/app/App/Providers/RouteServiceProvider.php b/app/App/Providers/RouteServiceProvider.php index d7c1cb737..97c3e7c77 100644 --- a/app/App/Providers/RouteServiceProvider.php +++ b/app/App/Providers/RouteServiceProvider.php @@ -85,5 +85,12 @@ class RouteServiceProvider extends ServiceProvider RateLimiter::for('public', function (Request $request) { return Limit::perMinute(10)->by($request->ip()); }); + + RateLimiter::for('exports', function (Request $request) { + $user = user(); + $attempts = $user->isGuest() ? 4 : 10; + $key = $user->isGuest() ? $request->ip() : $user->id; + return Limit::perMinute($attempts)->by($key); + }); } } diff --git a/app/Exports/Controllers/BookExportController.php b/app/Exports/Controllers/BookExportController.php index f726175a0..b6b1006bd 100644 --- a/app/Exports/Controllers/BookExportController.php +++ b/app/Exports/Controllers/BookExportController.php @@ -16,6 +16,7 @@ class BookExportController extends Controller protected ExportFormatter $exportFormatter, ) { $this->middleware('can:content-export'); + $this->middleware('throttle:exports'); } /** @@ -75,6 +76,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..de2385bb1 100644 --- a/app/Exports/Controllers/ChapterExportController.php +++ b/app/Exports/Controllers/ChapterExportController.php @@ -16,6 +16,7 @@ class ChapterExportController extends Controller protected ExportFormatter $exportFormatter, ) { $this->middleware('can:content-export'); + $this->middleware('throttle:exports'); } /** @@ -81,6 +82,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..d7145411e 100644 --- a/app/Exports/Controllers/PageExportController.php +++ b/app/Exports/Controllers/PageExportController.php @@ -17,6 +17,7 @@ class PageExportController extends Controller protected ExportFormatter $exportFormatter, ) { $this->middleware('can:content-export'); + $this->middleware('throttle:exports'); } /** @@ -85,6 +86,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/PdfGenerator.php b/app/Exports/PdfGenerator.php index 0524e063f..f31d8aad0 100644 --- a/app/Exports/PdfGenerator.php +++ b/app/Exports/PdfGenerator.php @@ -90,18 +90,28 @@ class PdfGenerator $process = Process::fromShellCommandline($command); $process->setTimeout($timeout); + $cleanup = function () use ($inputHtml, $outputPdf) { + foreach ([$inputHtml, $outputPdf] as $file) { + if (file_exists($file)) { + unlink($file); + } + } + }; + try { $process->run(); } catch (ProcessTimedOutException $e) { + $cleanup(); throw new PdfExportException("PDF Export via command failed due to timeout at {$timeout} second(s)"); } if (!$process->isSuccessful()) { + $cleanup(); throw new PdfExportException("PDF Export via command failed with exit code {$process->getExitCode()}, stdout: {$process->getOutput()}, stderr: {$process->getErrorOutput()}"); } $pdfContents = file_get_contents($outputPdf); - unlink($outputPdf); + $cleanup(); if ($pdfContents === false) { throw new PdfExportException("PDF Export via command failed, unable to read PDF output file"); 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/PdfExportTest.php b/tests/Exports/PdfExportTest.php index 9d85c69e2..e4de87d0d 100644 --- a/tests/Exports/PdfExportTest.php +++ b/tests/Exports/PdfExportTest.php @@ -5,6 +5,7 @@ namespace Tests\Exports; use BookStack\Entities\Models\Page; use BookStack\Exceptions\PdfExportException; use BookStack\Exports\PdfGenerator; +use FilesystemIterator; use Tests\TestCase; class PdfExportTest extends TestCase @@ -128,7 +129,7 @@ class PdfExportTest extends TestCase }, PdfExportException::class); } - public function test_pdf_command_timout_option_limits_export_time() + public function test_pdf_command_timeout_option_limits_export_time() { $page = $this->entities->page(); $command = 'php -r \'sleep(4);\''; @@ -143,4 +144,19 @@ class PdfExportTest extends TestCase }, PdfExportException::class, "PDF Export via command failed due to timeout at 1 second(s)"); } + + public function test_pdf_command_option_does_not_leave_temp_files() + { + $tempDir = sys_get_temp_dir(); + $startTempFileCount = iterator_count((new FileSystemIterator($tempDir, FilesystemIterator::SKIP_DOTS))); + + $page = $this->entities->page(); + $command = 'cp {input_html_path} {output_pdf_path}'; + config()->set('exports.pdf_command', $command); + + $this->asEditor()->get($page->getUrl('/export/pdf')); + + $afterTempFileCount = iterator_count((new FileSystemIterator($tempDir, FilesystemIterator::SKIP_DOTS))); + $this->assertEquals($startTempFileCount, $afterTempFileCount); + } } diff --git a/tests/Exports/ZipExportTest.php b/tests/Exports/ZipExportTest.php index 163828c1b..1434c013f 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(); @@ -404,6 +423,28 @@ class ZipExportTest extends TestCase $this->assertStringContainsString("[Link to chapter]([[bsexport:chapter:{$chapter->id}]])", $pageData['markdown']); } + public function test_exports_rate_limited_low_for_guest_viewers() + { + $this->setSettings(['app-public' => 'true']); + + $page = $this->entities->page(); + for ($i = 0; $i < 4; $i++) { + $this->get($page->getUrl("/export/zip"))->assertOk(); + } + $this->get($page->getUrl("/export/zip"))->assertStatus(429); + } + + public function test_exports_rate_limited_higher_for_logged_in_viewers() + { + $this->asAdmin(); + + $page = $this->entities->page(); + for ($i = 0; $i < 10; $i++) { + $this->get($page->getUrl("/export/zip"))->assertOk(); + } + $this->get($page->getUrl("/export/zip"))->assertStatus(429); + } + protected function extractZipResponse(TestResponse $response): ZipResultData { $zipData = $response->streamedContent();