Add additional validation for import, Enhanced ZIP extraction in importer

app/Http/Requests/V1/Import/ImportRequest.php

@@ -24,6 +24,7 @@ class ImportRequest extends BaseFormRequest
             'data' => [
                 'required',
                 'string',
+                'max:'.config('import.max_data_size'),
             ],
         ];
     }

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);
 

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());
+            app(ZipImportHelper::class)->extract($temporaryDirectoryZip->path('import.zip'), $temporaryDirectory->path());
-            $zip->close();
 
             if (! file_exists($temporaryDirectory->path('meta.json'))) {
                 throw new ImportException('File "meta.json" missing in ZIP');

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());
+            app(ZipImportHelper::class)->extract($temporaryDirectoryZip->path('import.zip'), $temporaryDirectory->path());
-            $zip->close();
             if (! file_exists($temporaryDirectory->path('clients.json'))) {
                 throw new ImportException('File "clients.json" missing in ZIP');
             }

app/Service/Import/Importers/ZipImportHelper.php

@@ -0,0 +1,129 @@
+<?php
+
+declare(strict_types=1);
+
+namespace App\Service\Import\Importers;
+
+use ZipArchive;
+
+/**
+ * Extracts uploaded ZIP archives with limits on file count, total uncompressed
+ * size and entry paths, so a small malicious archive can not fill the disk
+ * (decompression bomb) or write outside the target directory (zip slip).
+ */
+class ZipImportHelper
+{
+    private const int CHUNK_SIZE = 1024 * 1024;
+
+    /**
+     * @throws ImportException
+     */
+    public function extract(string $zipPath, string $targetPath): void
+    {
+        $zip = new ZipArchive;
+        $res = $zip->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');
+        }
+    }
+}

config/import.php

@@ -0,0 +1,34 @@
+<?php
+
+declare(strict_types=1);
+
+return [
+
+    /*
+    |--------------------------------------------------------------------------
+    | Import payload limit
+    |--------------------------------------------------------------------------
+    |
+    | Maximum length of the base64 encoded "data" field of an import request in
+    | bytes. Requests with a larger payload are rejected with a validation error.
+    |
+    */
+
+    'max_data_size' => (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),
+
+];

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

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

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

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

tests/Unit/Service/Import/Importers/ZipImportHelperTest.php

@@ -0,0 +1,254 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Tests\Unit\Service\Import\Importers;
+
+use App\Service\Import\Importers\ImportException;
+use App\Service\Import\Importers\ZipImportHelper;
+use PHPUnit\Framework\Attributes\CoversClass;
+use Spatie\TemporaryDirectory\TemporaryDirectory;
+use Tests\TestCase;
+use ZipArchive;
+
+#[CoversClass(ZipImportHelper::class)]
+class ZipImportHelperTest extends TestCase
+{
+    private TemporaryDirectory $sourceDirectory;
+
+    private TemporaryDirectory $targetDirectory;
+
+    protected function setUp(): void
+    {
+        parent::setUp();
+        $this->sourceDirectory = TemporaryDirectory::make();
+        $this->targetDirectory = TemporaryDirectory::make();
+    }
+
+    protected function tearDown(): void
+    {
+        $this->sourceDirectory->delete();
+        $this->targetDirectory->delete();
+        parent::tearDown();
+    }
+
+    /**
+     * @param  array<string, string>  $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();
+    }
+}