From 70646a0dd47ba5a15e3e56a9b1ede5b5ede1b333 Mon Sep 17 00:00:00 2001 From: Constantin Graf Date: Wed, 16 Sep 2026 14:45:26 +0200 Subject: [PATCH] Add additional validation for import, Enhanced ZIP extraction in importer --- app/Http/Requests/V1/Import/ImportRequest.php | 1 + app/Service/Import/ImportService.php | 5 - .../Import/Importers/SolidtimeImporter.php | 9 +- .../Import/Importers/TogglDataImporter.php | 9 +- .../Import/Importers/ZipImportHelper.php | 129 +++++++++ config/import.php | 34 +++ .../Endpoint/Api/V1/ImportEndpointTest.php | 23 ++ .../Unit/Service/Import/ImportServiceTest.php | 1 + .../Importers/SolidtimeImporterTest.php | 25 ++ .../Importers/TogglDataImporterTest.php | 25 ++ .../Import/Importers/ZipImportHelperTest.php | 254 ++++++++++++++++++ 11 files changed, 494 insertions(+), 21 deletions(-) create mode 100644 app/Service/Import/Importers/ZipImportHelper.php create mode 100644 config/import.php create mode 100644 tests/Unit/Service/Import/Importers/ZipImportHelperTest.php diff --git a/app/Http/Requests/V1/Import/ImportRequest.php b/app/Http/Requests/V1/Import/ImportRequest.php index 319e9880..41dfed7f 100644 --- a/app/Http/Requests/V1/Import/ImportRequest.php +++ b/app/Http/Requests/V1/Import/ImportRequest.php @@ -24,6 +24,7 @@ class ImportRequest extends BaseFormRequest 'data' => [ 'required', 'string', + 'max:'.config('import.max_data_size'), ], ]; } diff --git a/app/Service/Import/ImportService.php b/app/Service/Import/ImportService.php index 94103225..8e371444 100644 --- a/app/Service/Import/ImportService.php +++ b/app/Service/Import/ImportService.php @@ -9,11 +9,8 @@ use App\Service\Import\Importers\ImporterContract; use App\Service\Import\Importers\ImporterProvider; use App\Service\Import\Importers\ImportException; use App\Service\Import\Importers\ReportDto; -use Illuminate\Support\Carbon; use Illuminate\Support\Facades\Cache; use Illuminate\Support\Facades\DB; -use Illuminate\Support\Facades\Storage; -use Illuminate\Support\Str; class ImportService { @@ -25,8 +22,6 @@ class ImportService /** @var ImporterContract $importer */ $importer = app(ImporterProvider::class)->getImporter($importerType); $importer->init($organization); - Storage::disk(config('filesystems.default')) - ->put('import/'.Carbon::now()->toDateString().'-'.$organization->getKey().'-'.Str::uuid(), $data); $lock = Cache::lock('import:'.$organization->getKey(), config('octane.max_execution_time', 60) + 1); diff --git a/app/Service/Import/Importers/SolidtimeImporter.php b/app/Service/Import/Importers/SolidtimeImporter.php index 9904e04b..677b48bd 100644 --- a/app/Service/Import/Importers/SolidtimeImporter.php +++ b/app/Service/Import/Importers/SolidtimeImporter.php @@ -16,7 +16,6 @@ use Illuminate\Support\Str; use League\Csv\Reader; use Override; use Spatie\TemporaryDirectory\TemporaryDirectory; -use ZipArchive; class SolidtimeImporter extends DefaultImporter { @@ -34,16 +33,10 @@ class SolidtimeImporter extends DefaultImporter $temporaryDirectoryZip = null; $temporaryDirectory = null; try { - $zip = new ZipArchive; $temporaryDirectoryZip = TemporaryDirectory::make(); file_put_contents($temporaryDirectoryZip->path('import.zip'), $data); - $res = $zip->open($temporaryDirectoryZip->path('import.zip'), ZipArchive::RDONLY); - if ($res !== true) { - throw new ImportException('Invalid ZIP, error code: '.$res); - } $temporaryDirectory = TemporaryDirectory::make(); - $zip->extractTo($temporaryDirectory->path()); - $zip->close(); + app(ZipImportHelper::class)->extract($temporaryDirectoryZip->path('import.zip'), $temporaryDirectory->path()); if (! file_exists($temporaryDirectory->path('meta.json'))) { throw new ImportException('File "meta.json" missing in ZIP'); diff --git a/app/Service/Import/Importers/TogglDataImporter.php b/app/Service/Import/Importers/TogglDataImporter.php index 0785cf33..fbf6e9a4 100644 --- a/app/Service/Import/Importers/TogglDataImporter.php +++ b/app/Service/Import/Importers/TogglDataImporter.php @@ -13,7 +13,6 @@ use Illuminate\Support\Str; use Override; use Spatie\TemporaryDirectory\TemporaryDirectory; use ValueError; -use ZipArchive; class TogglDataImporter extends DefaultImporter { @@ -26,16 +25,10 @@ class TogglDataImporter extends DefaultImporter $temporaryDirectoryZip = null; $temporaryDirectory = null; try { - $zip = new ZipArchive; $temporaryDirectoryZip = TemporaryDirectory::make(); file_put_contents($temporaryDirectoryZip->path('import.zip'), $data); - $res = $zip->open($temporaryDirectoryZip->path('import.zip'), ZipArchive::RDONLY); - if ($res !== true) { - throw new ImportException('Invalid ZIP, error code: '.$res); - } $temporaryDirectory = TemporaryDirectory::make(); - $zip->extractTo($temporaryDirectory->path()); - $zip->close(); + app(ZipImportHelper::class)->extract($temporaryDirectoryZip->path('import.zip'), $temporaryDirectory->path()); if (! file_exists($temporaryDirectory->path('clients.json'))) { throw new ImportException('File "clients.json" missing in ZIP'); } diff --git a/app/Service/Import/Importers/ZipImportHelper.php b/app/Service/Import/Importers/ZipImportHelper.php new file mode 100644 index 00000000..0a351638 --- /dev/null +++ b/app/Service/Import/Importers/ZipImportHelper.php @@ -0,0 +1,129 @@ +open($zipPath, ZipArchive::RDONLY); + if ($res !== true) { + throw new ImportException('Invalid ZIP, error code: '.$res); + } + + try { + $maxFiles = (int) config('import.zip_max_files'); + $maxUncompressedSize = (int) config('import.zip_max_uncompressed_size'); + + if ($zip->numFiles > $maxFiles) { + throw new ImportException('ZIP contains too many files, maximum is '.$maxFiles); + } + + // Check the sizes declared in the archive before writing anything to disk + $declaredSize = 0; + for ($index = 0; $index < $zip->numFiles; $index++) { + $stat = $zip->statIndex($index); + if ($stat === false) { + throw new ImportException('Invalid ZIP entry'); + } + $this->validateEntryName($stat['name']); + $declaredSize += $stat['size']; + if ($declaredSize > $maxUncompressedSize) { + throw new ImportException('ZIP uncompressed size exceeds the maximum of '.$maxUncompressedSize.' bytes'); + } + } + + // The declared sizes can be forged, so the written bytes are counted as well + $writtenSize = 0; + for ($index = 0; $index < $zip->numFiles; $index++) { + $stat = $zip->statIndex($index); + if ($stat === false) { + throw new ImportException('Invalid ZIP entry'); + } + $name = $stat['name']; + $entryPath = $targetPath.DIRECTORY_SEPARATOR.$name; + + if (str_ends_with($name, '/')) { + $this->ensureDirectoryExists($entryPath); + + continue; + } + $this->ensureDirectoryExists(dirname($entryPath)); + + $stream = $zip->getStreamIndex($index); + if ($stream === false) { + throw new ImportException('ZIP entry "'.$name.'" can not be read'); + } + $target = fopen($entryPath, 'wb'); + if ($target === false) { + fclose($stream); + throw new ImportException('ZIP entry "'.$name.'" can not be extracted'); + } + try { + while (! feof($stream)) { + $chunk = fread($stream, self::CHUNK_SIZE); + if ($chunk === false) { + throw new ImportException('ZIP entry "'.$name.'" can not be read'); + } + $writtenSize += strlen($chunk); + if ($writtenSize > $maxUncompressedSize) { + throw new ImportException('ZIP uncompressed size exceeds the maximum of '.$maxUncompressedSize.' bytes'); + } + fwrite($target, $chunk); + } + } finally { + fclose($target); + fclose($stream); + } + } + } finally { + $zip->close(); + } + } + + /** + * @throws ImportException + */ + private function validateEntryName(string $name): void + { + if ($name === '' || str_contains($name, "\0") || str_contains($name, '\\') || str_starts_with($name, '/')) { + throw new ImportException('ZIP contains an invalid file path: "'.$name.'"'); + } + if (preg_match('/^[a-zA-Z]:/', $name) === 1) { + throw new ImportException('ZIP contains an invalid file path: "'.$name.'"'); + } + foreach (explode('/', rtrim($name, '/')) as $segment) { + if ($segment === '' || $segment === '..') { + throw new ImportException('ZIP contains an invalid file path: "'.$name.'"'); + } + } + } + + /** + * @throws ImportException + */ + private function ensureDirectoryExists(string $path): void + { + if (is_dir($path)) { + return; + } + if (! mkdir($path, 0700, true) && ! is_dir($path)) { + throw new ImportException('Directory "'.$path.'" can not be created'); + } + } +} diff --git a/config/import.php b/config/import.php new file mode 100644 index 00000000..05aec936 --- /dev/null +++ b/config/import.php @@ -0,0 +1,34 @@ + (int) (env('IMPORT_MAX_DATA_SIZE') ?: 50 * 1024 * 1024), + + /* + |-------------------------------------------------------------------------- + | ZIP extraction limits + |-------------------------------------------------------------------------- + | + | Limits applied to ZIP based importers before and during extraction to + | protect the instance against decompression bombs. The uncompressed size + | is the sum of all files in the archive in bytes. + | + */ + + 'zip_max_files' => (int) (env('IMPORT_ZIP_MAX_FILES') ?: 100), + + 'zip_max_uncompressed_size' => (int) (env('IMPORT_ZIP_MAX_UNCOMPRESSED_SIZE') ?: 500 * 1024 * 1024), + +]; diff --git a/tests/Unit/Endpoint/Api/V1/ImportEndpointTest.php b/tests/Unit/Endpoint/Api/V1/ImportEndpointTest.php index 175e3130..fd5c192b 100644 --- a/tests/Unit/Endpoint/Api/V1/ImportEndpointTest.php +++ b/tests/Unit/Endpoint/Api/V1/ImportEndpointTest.php @@ -97,6 +97,29 @@ class ImportEndpointTest extends ApiEndpointTestAbstract ]); } + public function test_import_fails_if_data_exceeds_maximum_size(): void + { + // Arrange + config(['import.max_data_size' => 16]); + $user = $this->createUserWithPermission([ + 'import', + ]); + $this->mock(ImportService::class, function (MockInterface $mock): void { + $mock->shouldNotReceive('import'); + }); + Passport::actingAs($user->user); + + // Act + $response = $this->postJson(route('api.v1.import.import', ['organization' => $user->organization->getKey()]), [ + 'type' => 'toggl_time_entries', + 'data' => base64_encode(str_repeat('a', 15)), + ]); + + // Assert + $response->assertStatus(422); + $response->assertJsonValidationErrors(['data']); + } + public function test_import_return_error_message_if_import_fails(): void { // Arrange diff --git a/tests/Unit/Service/Import/ImportServiceTest.php b/tests/Unit/Service/Import/ImportServiceTest.php index 6a2d6225..40ec8ce4 100644 --- a/tests/Unit/Service/Import/ImportServiceTest.php +++ b/tests/Unit/Service/Import/ImportServiceTest.php @@ -41,6 +41,7 @@ class ImportServiceTest extends TestCase $this->assertSame(1, $report->usersCreated); $this->assertSame(2, $report->projectsCreated); $this->assertSame(1, $report->clientsCreated); + Storage::disk(config('filesystems.default'))->assertDirectoryEmpty('import'); } public function test_import_releases_lock_if_an_exception_happens_during_the_import(): void diff --git a/tests/Unit/Service/Import/Importers/SolidtimeImporterTest.php b/tests/Unit/Service/Import/Importers/SolidtimeImporterTest.php index fd66681c..3e93d675 100644 --- a/tests/Unit/Service/Import/Importers/SolidtimeImporterTest.php +++ b/tests/Unit/Service/Import/Importers/SolidtimeImporterTest.php @@ -42,6 +42,31 @@ class SolidtimeImporterTest extends ImporterTestAbstract $this->fail(); } + public function test_import_throws_exception_if_zip_exceeds_uncompressed_size_limit(): void + { + // Arrange + config(['import.zip_max_uncompressed_size' => 10]); + $zipPath = $this->createTestZip('solidtime_import_test_1'); + $timezone = 'Europe/Vienna'; + $organization = Organization::factory()->create(); + $importer = new SolidtimeImporter; + $importer->init($organization); + $data = file_get_contents($zipPath); + + // Act + try { + $importer->importData($data, $timezone); + } catch (Exception $e) { + // Assert + $this->assertInstanceOf(ImportException::class, $e); + $this->assertSame('ZIP uncompressed size exceeds the maximum of 10 bytes', $e->getMessage()); + $this->assertSame(0, $importer->getReport()->timeEntriesCreated); + + return; + } + $this->fail(); + } + public function test_import_of_test_file_succeeds(): void { // Arrange diff --git a/tests/Unit/Service/Import/Importers/TogglDataImporterTest.php b/tests/Unit/Service/Import/Importers/TogglDataImporterTest.php index 35723774..453ce069 100644 --- a/tests/Unit/Service/Import/Importers/TogglDataImporterTest.php +++ b/tests/Unit/Service/Import/Importers/TogglDataImporterTest.php @@ -39,6 +39,31 @@ class TogglDataImporterTest extends ImporterTestAbstract $this->fail(); } + public function test_import_throws_exception_if_zip_contains_too_many_files(): void + { + // Arrange + config(['import.zip_max_files' => 1]); + $zipPath = $this->createTestZip('toggl_data_import_test_1'); + $timezone = 'Europe/Vienna'; + $organization = Organization::factory()->create(); + $importer = new TogglDataImporter; + $importer->init($organization); + $data = file_get_contents($zipPath); + + // Act + try { + $importer->importData($data, $timezone); + } catch (Exception $e) { + // Assert + $this->assertInstanceOf(ImportException::class, $e); + $this->assertSame('ZIP contains too many files, maximum is 1', $e->getMessage()); + $this->assertSame(0, $importer->getReport()->projectsCreated); + + return; + } + $this->fail(); + } + public function test_import_of_test_file_succeeds(): void { // Arrange diff --git a/tests/Unit/Service/Import/Importers/ZipImportHelperTest.php b/tests/Unit/Service/Import/Importers/ZipImportHelperTest.php new file mode 100644 index 00000000..0e818552 --- /dev/null +++ b/tests/Unit/Service/Import/Importers/ZipImportHelperTest.php @@ -0,0 +1,254 @@ +sourceDirectory = TemporaryDirectory::make(); + $this->targetDirectory = TemporaryDirectory::make(); + } + + protected function tearDown(): void + { + $this->sourceDirectory->delete(); + $this->targetDirectory->delete(); + parent::tearDown(); + } + + /** + * @param array $files + */ + private function createZip(array $files): string + { + $zipPath = $this->sourceDirectory->path('test.zip'); + $zip = new ZipArchive; + $zip->open($zipPath, ZipArchive::CREATE); + foreach ($files as $name => $content) { + $zip->addFromString($name, $content); + } + $zip->close(); + + return $zipPath; + } + + private function assertNothingExtracted(): void + { + $this->assertSame([], array_values(array_diff(scandir($this->targetDirectory->path()), ['.', '..']))); + } + + public function test_extract_extracts_files_and_nested_directories(): void + { + // Arrange + $zipPath = $this->createZip([ + 'meta.json' => '{"version":"1.0"}', + 'nested/dir/file.csv' => 'a,b', + ]); + + // Act + app(ZipImportHelper::class)->extract($zipPath, $this->targetDirectory->path()); + + // Assert + $this->assertSame('{"version":"1.0"}', file_get_contents($this->targetDirectory->path('meta.json'))); + $this->assertSame('a,b', file_get_contents($this->targetDirectory->path('nested/dir/file.csv'))); + } + + public function test_extract_throws_exception_if_file_is_not_a_zip(): void + { + // Arrange + $path = $this->sourceDirectory->path('not-a-zip.txt'); + file_put_contents($path, 'not a zip'); + + // Act + try { + app(ZipImportHelper::class)->extract($path, $this->targetDirectory->path()); + } catch (ImportException $e) { + // Assert + $this->assertSame('Invalid ZIP, error code: 19', $e->getMessage()); + $this->assertNothingExtracted(); + + return; + } + $this->fail(); + } + + public function test_extract_throws_exception_if_zip_contains_too_many_files(): void + { + // Arrange + config(['import.zip_max_files' => 2]); + $zipPath = $this->createZip([ + 'a.txt' => 'a', + 'b.txt' => 'b', + 'c.txt' => 'c', + ]); + + // Act + try { + app(ZipImportHelper::class)->extract($zipPath, $this->targetDirectory->path()); + } catch (ImportException $e) { + // Assert + $this->assertSame('ZIP contains too many files, maximum is 2', $e->getMessage()); + $this->assertNothingExtracted(); + + return; + } + $this->fail(); + } + + public function test_extract_throws_exception_before_writing_if_declared_uncompressed_size_exceeds_limit(): void + { + // Arrange + config(['import.zip_max_uncompressed_size' => 100]); + $zipPath = $this->createZip([ + 'a.txt' => str_repeat('a', 60), + 'b.txt' => str_repeat('b', 60), + ]); + + // Act + try { + app(ZipImportHelper::class)->extract($zipPath, $this->targetDirectory->path()); + } catch (ImportException $e) { + // Assert + $this->assertSame('ZIP uncompressed size exceeds the maximum of 100 bytes', $e->getMessage()); + $this->assertNothingExtracted(); + + return; + } + $this->fail(); + } + + public function test_extract_throws_exception_if_actual_uncompressed_size_exceeds_limit_despite_forged_headers(): void + { + // Arrange + config(['import.zip_max_uncompressed_size' => 1000]); + $zipPath = $this->createZip([ + 'bomb.bin' => str_repeat("\0", 100000), + ]); + // Forge the uncompressed size in the local file header (offset 22) and central directory header (offset 24) + $content = file_get_contents($zipPath); + $forgedSize = pack('V', 10); + $localHeaderOffset = strpos($content, "PK\x03\x04"); + $centralHeaderOffset = strpos($content, "PK\x01\x02"); + $this->assertNotFalse($localHeaderOffset); + $this->assertNotFalse($centralHeaderOffset); + $content = substr_replace($content, $forgedSize, $localHeaderOffset + 22, 4); + $content = substr_replace($content, $forgedSize, $centralHeaderOffset + 24, 4); + file_put_contents($zipPath, $content); + $zip = new ZipArchive; + $this->assertTrue($zip->open($zipPath, ZipArchive::RDONLY)); + $this->assertSame(10, $zip->statIndex(0)['size']); + $zip->close(); + + // Act + try { + app(ZipImportHelper::class)->extract($zipPath, $this->targetDirectory->path()); + } catch (ImportException $e) { + // Assert + $this->assertSame('ZIP uncompressed size exceeds the maximum of 1000 bytes', $e->getMessage()); + $extracted = $this->targetDirectory->path('bomb.bin'); + if (file_exists($extracted)) { + $this->assertLessThanOrEqual(1000, filesize($extracted)); + } + + return; + } + $this->fail(); + } + + public function test_extract_throws_exception_if_zip_contains_path_traversal(): void + { + // Arrange + $zipPath = $this->createZip([ + '../evil.txt' => 'evil', + ]); + + // Act + try { + app(ZipImportHelper::class)->extract($zipPath, $this->targetDirectory->path()); + } catch (ImportException $e) { + // Assert + $this->assertSame('ZIP contains an invalid file path: "../evil.txt"', $e->getMessage()); + $this->assertFileDoesNotExist(dirname($this->targetDirectory->path()).'/evil.txt'); + $this->assertNothingExtracted(); + + return; + } + $this->fail(); + } + + public function test_extract_throws_exception_if_zip_contains_nested_path_traversal(): void + { + // Arrange + $zipPath = $this->createZip([ + 'sub/../../evil.txt' => 'evil', + ]); + + // Act + try { + app(ZipImportHelper::class)->extract($zipPath, $this->targetDirectory->path()); + } catch (ImportException $e) { + // Assert + $this->assertSame('ZIP contains an invalid file path: "sub/../../evil.txt"', $e->getMessage()); + $this->assertNothingExtracted(); + + return; + } + $this->fail(); + } + + public function test_extract_throws_exception_if_zip_contains_absolute_path(): void + { + // Arrange + $zipPath = $this->createZip([ + '/tmp/evil.txt' => 'evil', + ]); + + // Act + try { + app(ZipImportHelper::class)->extract($zipPath, $this->targetDirectory->path()); + } catch (ImportException $e) { + // Assert + $this->assertSame('ZIP contains an invalid file path: "/tmp/evil.txt"', $e->getMessage()); + $this->assertNothingExtracted(); + + return; + } + $this->fail(); + } + + public function test_extract_throws_exception_if_zip_contains_backslash_path(): void + { + // Arrange + $zipPath = $this->createZip([ + '..\\evil.txt' => 'evil', + ]); + + // Act + try { + app(ZipImportHelper::class)->extract($zipPath, $this->targetDirectory->path()); + } catch (ImportException $e) { + // Assert + $this->assertSame('ZIP contains an invalid file path: "..\\evil.txt"', $e->getMessage()); + $this->assertNothingExtracted(); + + return; + } + $this->fail(); + } +}