Skip to content

Commit 6effc6d

Browse files
ssddanbrownssddanbrown
authored
Merge pull request #5379 from BookStackApp/better_cleanup
Export limits and cleanup
2 parents ff6c5aa + 1ff2826 commit 6effc6d

9 files changed

+130
-10
lines changed

‎app/App/Providers/RouteServiceProvider.php

+7
Original file line numberDiff line numberDiff line change
@@ -85,5 +85,12 @@ protected function configureRateLimiting(): void
8585
RateLimiter::for('public', function (Request $request) {
8686
return Limit::perMinute(10)->by($request->ip());
8787
});
88+
89+
RateLimiter::for('exports', function (Request $request) {
90+
$user = user();
91+
$attempts = $user->isGuest() ? 4 : 10;
92+
$key = $user->isGuest() ? $request->ip() : $user->id;
93+
return Limit::perMinute($attempts)->by($key);
94+
});
8895
}
8996
}

‎app/Exports/Controllers/BookExportController.php

+2-1
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ public function __construct(
1616
protected ExportFormatter $exportFormatter,
1717
) {
1818
$this->middleware('can:content-export');
19+
$this->middleware('throttle:exports');
1920
}
2021

2122
/**
@@ -75,6 +76,6 @@ public function zip(string $bookSlug, ZipExportBuilder $builder)
7576
$book = $this->queries->findVisibleBySlugOrFail($bookSlug);
7677
$zip = $builder->buildForBook($book);
7778

78-
return $this->download()->streamedDirectly(fopen($zip, 'r'), $bookSlug . '.zip', filesize($zip));
79+
return $this->download()->streamedFileDirectly($zip, $bookSlug . '.zip', filesize($zip), true);
7980
}
8081
}

‎app/Exports/Controllers/ChapterExportController.php

+2-1
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ public function __construct(
1616
protected ExportFormatter $exportFormatter,
1717
) {
1818
$this->middleware('can:content-export');
19+
$this->middleware('throttle:exports');
1920
}
2021

2122
/**
@@ -81,6 +82,6 @@ public function zip(string $bookSlug, string $chapterSlug, ZipExportBuilder $bui
8182
$chapter = $this->queries->findVisibleBySlugsOrFail($bookSlug, $chapterSlug);
8283
$zip = $builder->buildForChapter($chapter);
8384

84-
return $this->download()->streamedDirectly(fopen($zip, 'r'), $chapterSlug . '.zip', filesize($zip));
85+
return $this->download()->streamedFileDirectly($zip, $chapterSlug . '.zip', filesize($zip), true);
8586
}
8687
}

‎app/Exports/Controllers/PageExportController.php

+2-1
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ public function __construct(
1717
protected ExportFormatter $exportFormatter,
1818
) {
1919
$this->middleware('can:content-export');
20+
$this->middleware('throttle:exports');
2021
}
2122

2223
/**
@@ -85,6 +86,6 @@ public function zip(string $bookSlug, string $pageSlug, ZipExportBuilder $builde
8586
$page = $this->queries->findVisibleBySlugsOrFail($bookSlug, $pageSlug);
8687
$zip = $builder->buildForPage($page);
8788

88-
return $this->download()->streamedDirectly(fopen($zip, 'r'), $pageSlug . '.zip', filesize($zip));
89+
return $this->download()->streamedFileDirectly($zip, $pageSlug . '.zip', filesize($zip), true);
8990
}
9091
}

‎app/Exports/PdfGenerator.php

+11-1
Original file line numberDiff line numberDiff line change
@@ -90,18 +90,28 @@ protected function renderUsingCommand(string $html): string
9090
$process = Process::fromShellCommandline($command);
9191
$process->setTimeout($timeout);
9292

93+
$cleanup = function () use ($inputHtml, $outputPdf) {
94+
foreach ([$inputHtml, $outputPdf] as $file) {
95+
if (file_exists($file)) {
96+
unlink($file);
97+
}
98+
}
99+
};
100+
93101
try {
94102
$process->run();
95103
} catch (ProcessTimedOutException $e) {
104+
$cleanup();
96105
throw new PdfExportException("PDF Export via command failed due to timeout at {$timeout} second(s)");
97106
}
98107

99108
if (!$process->isSuccessful()) {
109+
$cleanup();
100110
throw new PdfExportException("PDF Export via command failed with exit code {$process->getExitCode()}, stdout: {$process->getOutput()}, stderr: {$process->getErrorOutput()}");
101111
}
102112

103113
$pdfContents = file_get_contents($outputPdf);
104-
unlink($outputPdf);
114+
$cleanup();
105115

106116
if ($pdfContents === false) {
107117
throw new PdfExportException("PDF Export via command failed, unable to read PDF output file");

‎app/Exports/ZipExports/ZipExportBuilder.php

+21-4
Original file line numberDiff line numberDiff line change
@@ -84,10 +84,27 @@ protected function build(): string
8484
$zip->addEmptyDir('files');
8585

8686
$toRemove = [];
87-
$this->files->extractEach(function ($filePath, $fileRef) use ($zip, &$toRemove) {
88-
$zip->addFile($filePath, "files/$fileRef");
89-
$toRemove[] = $filePath;
90-
});
87+
$addedNames = [];
88+
89+
try {
90+
$this->files->extractEach(function ($filePath, $fileRef) use ($zip, &$toRemove, &$addedNames) {
91+
$entryName = "files/$fileRef";
92+
$zip->addFile($filePath, $entryName);
93+
$toRemove[] = $filePath;
94+
$addedNames[] = $entryName;
95+
});
96+
} catch (\Exception $exception) {
97+
// Cleanup the files we've processed so far and respond back with error
98+
foreach ($toRemove as $file) {
99+
unlink($file);
100+
}
101+
foreach ($addedNames as $name) {
102+
$zip->deleteName($name);
103+
}
104+
$zip->close();
105+
unlink($zipFile);
106+
throw new ZipExportException("Failed to add files for ZIP export, received error: " . $exception->getMessage());
107+
}
91108

92109
$zip->close();
93110

‎app/Http/DownloadResponseFactory.php

+27-1
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@
99
class DownloadResponseFactory
1010
{
1111
public function __construct(
12-
protected Request $request
12+
protected Request $request,
1313
) {
1414
}
1515

@@ -35,6 +35,32 @@ public function streamedDirectly($stream, string $fileName, int $fileSize): Stre
3535
);
3636
}
3737

38+
/**
39+
* Create a response that downloads the given file via a stream.
40+
* Has the option to delete the provided file once the stream is closed.
41+
*/
42+
public function streamedFileDirectly(string $filePath, string $fileName, int $fileSize, bool $deleteAfter = false): StreamedResponse
43+
{
44+
$stream = fopen($filePath, 'r');
45+
46+
if ($deleteAfter) {
47+
// Delete the given file if it still exists after the app terminates
48+
$callback = function () use ($filePath) {
49+
if (file_exists($filePath)) {
50+
unlink($filePath);
51+
}
52+
};
53+
54+
// We watch both app terminate and php shutdown to cover both normal app termination
55+
// as well as other potential scenarios (connection termination).
56+
app()->terminating($callback);
57+
register_shutdown_function($callback);
58+
}
59+
60+
return $this->streamedDirectly($stream, $fileName, $fileSize);
61+
}
62+
63+
3864
/**
3965
* Create a file download response that provides the file with a content-type
4066
* correct for the file, in a way so the browser can show the content in browser,

‎tests/Exports/PdfExportTest.php

+17-1
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
use BookStack\Entities\Models\Page;
66
use BookStack\Exceptions\PdfExportException;
77
use BookStack\Exports\PdfGenerator;
8+
use FilesystemIterator;
89
use Tests\TestCase;
910

1011
class PdfExportTest extends TestCase
@@ -128,7 +129,7 @@ public function test_pdf_command_option_errors_if_command_returns_error_status()
128129
}, PdfExportException::class);
129130
}
130131

131-
public function test_pdf_command_timout_option_limits_export_time()
132+
public function test_pdf_command_timeout_option_limits_export_time()
132133
{
133134
$page = $this->entities->page();
134135
$command = 'php -r \'sleep(4);\'';
@@ -143,4 +144,19 @@ public function test_pdf_command_timout_option_limits_export_time()
143144
}, PdfExportException::class,
144145
"PDF Export via command failed due to timeout at 1 second(s)");
145146
}
147+
148+
public function test_pdf_command_option_does_not_leave_temp_files()
149+
{
150+
$tempDir = sys_get_temp_dir();
151+
$startTempFileCount = iterator_count((new FileSystemIterator($tempDir, FilesystemIterator::SKIP_DOTS)));
152+
153+
$page = $this->entities->page();
154+
$command = 'cp {input_html_path} {output_pdf_path}';
155+
config()->set('exports.pdf_command', $command);
156+
157+
$this->asEditor()->get($page->getUrl('/export/pdf'));
158+
159+
$afterTempFileCount = iterator_count((new FileSystemIterator($tempDir, FilesystemIterator::SKIP_DOTS)));
160+
$this->assertEquals($startTempFileCount, $afterTempFileCount);
161+
}
146162
}

‎tests/Exports/ZipExportTest.php

+41
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
use BookStack\Entities\Tools\PageContent;
88
use BookStack\Uploads\Attachment;
99
use BookStack\Uploads\Image;
10+
use FilesystemIterator;
1011
use Illuminate\Support\Carbon;
1112
use Illuminate\Testing\TestResponse;
1213
use Tests\TestCase;
@@ -60,6 +61,24 @@ public function test_export_metadata()
6061
$this->assertEquals($instanceId, $zipInstanceId);
6162
}
6263

64+
public function test_export_leaves_no_temp_files()
65+
{
66+
$tempDir = sys_get_temp_dir();
67+
$startTempFileCount = iterator_count((new FileSystemIterator($tempDir, FilesystemIterator::SKIP_DOTS)));
68+
69+
$page = $this->entities->pageWithinChapter();
70+
$this->asEditor();
71+
$pageResp = $this->get($page->getUrl("/export/zip"));
72+
$pageResp->streamedContent();
73+
$pageResp->assertOk();
74+
$this->get($page->chapter->getUrl("/export/zip"))->assertOk();
75+
$this->get($page->book->getUrl("/export/zip"))->assertOk();
76+
77+
$afterTempFileCount = iterator_count((new FileSystemIterator($tempDir, FilesystemIterator::SKIP_DOTS)));
78+
79+
$this->assertEquals($startTempFileCount, $afterTempFileCount);
80+
}
81+
6382
public function test_page_export()
6483
{
6584
$page = $this->entities->page();
@@ -404,6 +423,28 @@ public function test_links_in_markdown_are_parsed()
404423
$this->assertStringContainsString("[Link to chapter]([[bsexport:chapter:{$chapter->id}]])", $pageData['markdown']);
405424
}
406425

426+
public function test_exports_rate_limited_low_for_guest_viewers()
427+
{
428+
$this->setSettings(['app-public' => 'true']);
429+
430+
$page = $this->entities->page();
431+
for ($i = 0; $i < 4; $i++) {
432+
$this->get($page->getUrl("/export/zip"))->assertOk();
433+
}
434+
$this->get($page->getUrl("/export/zip"))->assertStatus(429);
435+
}
436+
437+
public function test_exports_rate_limited_higher_for_logged_in_viewers()
438+
{
439+
$this->asAdmin();
440+
441+
$page = $this->entities->page();
442+
for ($i = 0; $i < 10; $i++) {
443+
$this->get($page->getUrl("/export/zip"))->assertOk();
444+
}
445+
$this->get($page->getUrl("/export/zip"))->assertStatus(429);
446+
}
447+
407448
protected function extractZipResponse(TestResponse $response): ZipResultData
408449
{
409450
$zipData = $response->streamedContent();

0 commit comments

Comments
 (0)