Merge pull request #5379 from BookStackApp/better_cleanup
Export limits and cleanup
This commit is contained in:
commit
6effc6d262
|
@ -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);
|
||||
});
|
||||
}
|
||||
}
|
||||
|
|
|
@ -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);
|
||||
}
|
||||
}
|
||||
|
|
|
@ -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);
|
||||
}
|
||||
}
|
||||
|
|
|
@ -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);
|
||||
}
|
||||
}
|
||||
|
|
|
@ -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");
|
||||
|
|
|
@ -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();
|
||||
|
||||
|
|
|
@ -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,
|
||||
|
|
|
@ -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);
|
||||
}
|
||||
}
|
||||
|
|
|
@ -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();
|
||||
|
|
Loading…
Reference in New Issue