Skip to content

Commit b0a8cb0

Browse files
ssddanbrownssddanbrown
authored
Merge pull request #5968 from BookStackApp/limits
Add some additional resource-based limits
2 parents 38d3697 + b08d1b3 commit b0a8cb0

File tree

10 files changed

+171
-3
lines changed

10 files changed

+171
-3
lines changed

‎app/Exports/ZipExports/ZipExportReader.php‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,16 @@ public function readData(): array
5858
{
5959
$this->open();
6060

61+
$info = $this->zip->statName('data.json');
62+
if ($info === false) {
63+
throw new ZipExportException(trans('errors.import_zip_cant_decode_data'));
64+
}
65+
66+
$maxSize = max(intval(config()->get('app.upload_limit')), 1) * 1000000;
67+
if ($info['size'] > $maxSize) {
68+
throw new ZipExportException(trans('errors.import_zip_data_too_large'));
69+
}
70+
6171
// Validate json data exists, including metadata
6272
$jsonData = $this->zip->getFromName('data.json') ?: '';
6373
$importData = json_decode($jsonData, true);
@@ -73,6 +83,17 @@ public function fileExists(string $fileName): bool
7383
return $this->zip->statName("files/{$fileName}") !== false;
7484
}
7585

86+
public function fileWithinSizeLimit(string $fileName): bool
87+
{
88+
$fileInfo = $this->zip->statName("files/{$fileName}");
89+
if ($fileInfo === false) {
90+
return false;
91+
}
92+
93+
$maxSize = max(intval(config()->get('app.upload_limit')), 1) * 1000000;
94+
return $fileInfo['size'] <= $maxSize;
95+
}
96+
7697
/**
7798
* @return false|resource
7899
*/

‎app/Exports/ZipExports/ZipFileReferenceRule.php‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,6 @@ public function __construct(
1313
) {
1414
}
1515

16-
1716
/**
1817
* @inheritDoc
1918
*/
@@ -23,6 +22,13 @@ public function validate(string $attribute, mixed $value, Closure $fail): void
2322
$fail('validation.zip_file')->translate();
2423
}
2524

25+
if (!$this->context->zipReader->fileWithinSizeLimit($value)) {
26+
$fail('validation.zip_file_size')->translate([
27+
'attribute' => $value,
28+
'size' => config('app.upload_limit'),
29+
]);
30+
}
31+
2632
if (!empty($this->acceptedMimes)) {
2733
$fileMime = $this->context->zipReader->sniffFileMime($value);
2834
if (!in_array($fileMime, $this->acceptedMimes)) {

‎app/Exports/ZipExports/ZipImportRunner.php‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -265,6 +265,12 @@ protected function exportTagsToInputArray(array $exportTags): array
265265

266266
protected function zipFileToUploadedFile(string $fileName, ZipExportReader $reader): UploadedFile
267267
{
268+
if (!$reader->fileWithinSizeLimit($fileName)) {
269+
throw new ZipImportException([
270+
"File $fileName exceeds app upload limit."
271+
]);
272+
}
273+
268274
$tempPath = tempnam(sys_get_temp_dir(), 'bszipextract');
269275
$fileStream = $reader->streamFile($fileName);
270276
$tempStream = fopen($tempPath, 'wb');

‎app/Search/SearchController.php‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -78,8 +78,9 @@ public function searchForSelector(Request $request, QueryPopular $queryPopular)
7878

7979
// Search for entities otherwise show most popular
8080
if ($searchTerm !== false) {
81-
$searchTerm .= ' {type:' . implode('|', $entityTypes) . '}';
82-
$entities = $this->searchRunner->searchEntities(SearchOptions::fromString($searchTerm), 'all', 1, 20)['results'];
81+
$options = SearchOptions::fromString($searchTerm);
82+
$options->setFilter('type', implode('|', $entityTypes));
83+
$entities = $this->searchRunner->searchEntities($options, 'all', 1, 20)['results'];
8384
} else {
8485
$entities = $queryPopular->run(20, 0, $entityTypes);
8586
}

‎app/Search/SearchOptionSet.php‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -82,4 +82,12 @@ public function nonNegated(): self
8282
$values = array_values(array_filter($this->options, fn (SearchOption $option) => !$option->negated));
8383
return new self($values);
8484
}
85+
86+
/**
87+
* @return self<T>
88+
*/
89+
public function limit(int $limit): self
90+
{
91+
return new self(array_slice(array_values($this->options), 0, $limit));
92+
}
8593
}

‎app/Search/SearchOptions.php‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@ public static function fromString(string $search): self
3535
{
3636
$instance = new self();
3737
$instance->addOptionsFromString($search);
38+
$instance->limitOptions();
3839
return $instance;
3940
}
4041

@@ -87,6 +88,8 @@ public static function fromRequest(Request $request): self
8788
$instance->filters = $instance->filters->merge($extras->filters);
8889
}
8990

91+
$instance->limitOptions();
92+
9093
return $instance;
9194
}
9295

@@ -147,6 +150,25 @@ protected function addOptionsFromString(string $searchString): void
147150
$this->filters = $this->filters->merge(new SearchOptionSet($terms['filters']));
148151
}
149152

153+
/**
154+
* Limit the amount of search options to reasonable levels.
155+
* Provides higher limits to logged-in users since that signals a slightly
156+
* higher level of trust.
157+
*/
158+
protected function limitOptions(): void
159+
{
160+
$userLoggedIn = !user()->isGuest();
161+
$searchLimit = $userLoggedIn ? 10 : 5;
162+
$exactLimit = $userLoggedIn ? 4 : 2;
163+
$tagLimit = $userLoggedIn ? 8 : 4;
164+
$filterLimit = $userLoggedIn ? 10 : 5;
165+
166+
$this->searches = $this->searches->limit($searchLimit);
167+
$this->exacts = $this->exacts->limit($exactLimit);
168+
$this->tags = $this->tags->limit($tagLimit);
169+
$this->filters = $this->filters->limit($filterLimit);
170+
}
171+
150172
/**
151173
* Decode backslash escaping within the input string.
152174
*/

‎lang/en/errors.php‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -109,6 +109,7 @@
109109
'import_zip_cant_read' => 'Could not read ZIP file.',
110110
'import_zip_cant_decode_data' => 'Could not find and decode ZIP data.json content.',
111111
'import_zip_no_data' => 'ZIP file data has no expected book, chapter or page content.',
112+
'import_zip_data_too_large' => 'ZIP data.json content exceeds the configured application maximum upload size.',
112113
'import_validation_failed' => 'Import ZIP failed to validate with errors:',
113114
'import_zip_failed_notification' => 'Failed to import ZIP file.',
114115
'import_perms_books' => 'You are lacking the required permissions to create books.',

‎lang/en/validation.php‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -106,6 +106,7 @@
106106
'uploaded' => 'The file could not be uploaded. The server may not accept files of this size.',
107107

108108
'zip_file' => 'The :attribute needs to reference a file within the ZIP.',
109+
'zip_file_size' => 'The file :attribute must not exceed :size MB.',
109110
'zip_file_mime' => 'The :attribute needs to reference a file of type :validTypes, found :foundType.',
110111
'zip_model_expected' => 'Data object expected but ":type" found.',
111112
'zip_unique' => 'The :attribute must be unique for the object type within the ZIP.',

‎tests/Exports/ZipImportRunnerTest.php‎

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
use BookStack\Entities\Models\Book;
66
use BookStack\Entities\Models\Chapter;
77
use BookStack\Entities\Models\Page;
8+
use BookStack\Exceptions\ZipImportException;
89
use BookStack\Exports\ZipExports\ZipImportRunner;
910
use BookStack\Uploads\Image;
1011
use Tests\TestCase;
@@ -431,4 +432,56 @@ public function test_drawing_references_are_updated_within_content()
431432

432433
ZipTestHelper::deleteZipForImport($import);
433434
}
435+
436+
public function test_error_thrown_if_zip_item_exceeds_app_file_upload_limit()
437+
{
438+
$tempFile = tempnam(sys_get_temp_dir(), 'bs-zip-test');
439+
file_put_contents($tempFile, str_repeat('a', 2500000));
440+
$parent = $this->entities->chapter();
441+
config()->set('app.upload_limit', 1);
442+
443+
$import = ZipTestHelper::importFromData([], [
444+
'page' => [
445+
'name' => 'Page A',
446+
'html' => '<p>Hello</p>',
447+
'attachments' => [
448+
[
449+
'name' => 'Text attachment',
450+
'file' => 'file_attachment'
451+
]
452+
],
453+
],
454+
], [
455+
'file_attachment' => $tempFile,
456+
]);
457+
458+
$this->asAdmin();
459+
460+
$this->expectException(ZipImportException::class);
461+
$this->expectExceptionMessage('The file file_attachment must not exceed 1 MB.');
462+
463+
$this->runner->run($import, $parent);
464+
ZipTestHelper::deleteZipForImport($import);
465+
}
466+
467+
public function test_error_thrown_if_zip_data_exceeds_app_file_upload_limit()
468+
{
469+
$parent = $this->entities->chapter();
470+
config()->set('app.upload_limit', 1);
471+
472+
$import = ZipTestHelper::importFromData([], [
473+
'page' => [
474+
'name' => 'Page A',
475+
'html' => '<p>' . str_repeat('a', 2500000) . '</p>',
476+
],
477+
]);
478+
479+
$this->asAdmin();
480+
481+
$this->expectException(ZipImportException::class);
482+
$this->expectExceptionMessage('ZIP data.json content exceeds the configured application maximum upload size.');
483+
484+
$this->runner->run($import, $parent);
485+
ZipTestHelper::deleteZipForImport($import);
486+
}
434487
}

‎tests/Search/SearchOptionsTest.php‎

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -142,4 +142,53 @@ public function test_from_request_properly_parses_out_extras_as_string()
142142
$this->assertEquals('dino', $options->exacts->all()[0]->value);
143143
$this->assertTrue($options->exacts->all()[0]->negated);
144144
}
145+
146+
public function test_from_string_results_are_count_limited_and_larger_for_logged_in_users()
147+
{
148+
$terms = [
149+
...array_fill(0, 40, 'cat'),
150+
...array_fill(0, 50, '"bees"'),
151+
...array_fill(0, 50, '{is_template}'),
152+
...array_fill(0, 50, '[a=b]'),
153+
];
154+
155+
$options = SearchOptions::fromString(implode(' ', $terms));
156+
157+
$this->assertCount(5, $options->searches->all());
158+
$this->assertCount(2, $options->exacts->all());
159+
$this->assertCount(4, $options->tags->all());
160+
$this->assertCount(5, $options->filters->all());
161+
162+
$this->asEditor();
163+
$options = SearchOptions::fromString(implode(' ', $terms));
164+
165+
$this->assertCount(10, $options->searches->all());
166+
$this->assertCount(4, $options->exacts->all());
167+
$this->assertCount(8, $options->tags->all());
168+
$this->assertCount(10, $options->filters->all());
169+
}
170+
171+
public function test_from_request_results_are_count_limited_and_larger_for_logged_in_users()
172+
{
173+
$request = new Request([
174+
'search' => str_repeat('hello ', 20),
175+
'tags' => array_fill(0, 20, 'a=b'),
176+
'extras' => str_repeat('-[b=c] -{viewed_by_me} -"dino"', 20),
177+
]);
178+
179+
$options = SearchOptions::fromRequest($request);
180+
181+
$this->assertCount(5, $options->searches->all());
182+
$this->assertCount(2, $options->exacts->all());
183+
$this->assertCount(4, $options->tags->all());
184+
$this->assertCount(5, $options->filters->all());
185+
186+
$this->asEditor();
187+
$options = SearchOptions::fromRequest($request);
188+
189+
$this->assertCount(10, $options->searches->all());
190+
$this->assertCount(4, $options->exacts->all());
191+
$this->assertCount(8, $options->tags->all());
192+
$this->assertCount(10, $options->filters->all());
193+
}
145194
}

0 commit comments

Comments
 (0)