From 2d8a995448bf958ef35cb0f04c7c34f8363d26db Mon Sep 17 00:00:00 2001 From: ildyria Date: Thu, 6 Aug 2026 20:31:23 +0200 Subject: [PATCH 1/7] Fix slipery zip extraction --- .../Requests/Photo/UploadPhotoRequest.php | 3 +- app/Jobs/ExtractZip.php | 14 ++- app/Rules/FilenameRule.php | 43 ++++++++ tests/Unit/Rules/FilenameRuleTest.php | 98 +++++++++++++++++++ 4 files changed, 156 insertions(+), 2 deletions(-) create mode 100644 app/Rules/FilenameRule.php create mode 100644 tests/Unit/Rules/FilenameRuleTest.php diff --git a/app/Http/Requests/Photo/UploadPhotoRequest.php b/app/Http/Requests/Photo/UploadPhotoRequest.php index 50627bb519f..2a27dfbe49a 100644 --- a/app/Http/Requests/Photo/UploadPhotoRequest.php +++ b/app/Http/Requests/Photo/UploadPhotoRequest.php @@ -20,6 +20,7 @@ use App\Rules\AlbumIDRule; use App\Rules\DescriptionRule; use App\Rules\ExtensionRule; +use App\Rules\FilenameRule; use App\Rules\FileUuidRule; use App\Rules\TitleRule; use Illuminate\Http\UploadedFile; @@ -56,7 +57,7 @@ public function rules(): array RequestAttribute::ALBUM_ID_ATTRIBUTE => ['present', new AlbumIDRule(true)], RequestAttribute::FILE_LAST_MODIFIED_TIME => 'sometimes|nullable|numeric', RequestAttribute::FILE_ATTRIBUTE => ['required', 'file'], - 'file_name' => 'required|string', + 'file_name' => ['required', new FilenameRule()], 'uuid_name' => ['present', new FileUuidRule()], 'extension' => ['present', new ExtensionRule()], 'chunk_number' => 'required|integer|min:1', diff --git a/app/Jobs/ExtractZip.php b/app/Jobs/ExtractZip.php index 44288eddbcd..f3a255e43f3 100644 --- a/app/Jobs/ExtractZip.php +++ b/app/Jobs/ExtractZip.php @@ -12,6 +12,7 @@ use App\Contracts\Models\AbstractAlbum; use App\DTO\ImportMode; use App\Enum\JobStatus; +use App\Exceptions\Internal\LycheeLogicException; use App\Exceptions\Internal\ZipBombDetectedException; use App\Exceptions\Internal\ZipExtractionException; use App\Exceptions\ZipInvalidException; @@ -33,6 +34,7 @@ use Illuminate\Support\Facades\Storage; use Illuminate\Support\Str; use function Safe\date; +use function Safe\realpath; use function Safe\unlink; class ExtractZip implements ShouldQueue @@ -87,7 +89,17 @@ public function handle(): void $this->validate_zip(); - $path_extracted = Storage::disk('extract-jobs')->path(date('Ymd') . ' ' . $this->getExtractFolderName()); + $valid_path = realpath(Storage::disk('extract-jobs')->path('')); + $path_extracted = realpath(Storage::disk('extract-jobs')->path(date('Ymd') . ' ' . $this->getExtractFolderName())); + + if (str_starts_with($path_extracted, $valid_path) === false) { + Log::channel('jobs')->critical("Extraction path {$path_extracted} is not within the valid extraction directory {$valid_path}."); + $this->history->status = JobStatus::FAILURE; + $this->history->save(); + + throw new LycheeLogicException("Extraction path {$path_extracted} is not within the valid extraction directory {$valid_path}."); + } + $this->extract_zip($path_extracted); $config_manager = app(ConfigManager::class); diff --git a/app/Rules/FilenameRule.php b/app/Rules/FilenameRule.php new file mode 100644 index 00000000000..0d17e54d014 --- /dev/null +++ b/app/Rules/FilenameRule.php @@ -0,0 +1,43 @@ +contains(['..', '/', '\\'])) { + $fail(':attribute contains invalid characters:.'); + + return; + } + } +} diff --git a/tests/Unit/Rules/FilenameRuleTest.php b/tests/Unit/Rules/FilenameRuleTest.php new file mode 100644 index 00000000000..45204cdf368 --- /dev/null +++ b/tests/Unit/Rules/FilenameRuleTest.php @@ -0,0 +1,98 @@ +validate('file_name', $value, function () use (&$failed): void { $failed = true; }); + self::assertFalse($failed, 'Expected "' . var_export($value, true) . '" to pass but it failed.'); + } + + private function assertFails(mixed $value, ?string $expected_message = null): void + { + $rule = new FilenameRule(); + $msg = null; + $rule->validate('file_name', $value, function ($message) use (&$msg): void { $msg = $message; }); + self::assertNotNull($msg, 'Expected "' . var_export($value, true) . '" to fail but it passed.'); + if ($expected_message !== null) { + self::assertSame($expected_message, $msg); + } + } + + public function testValidFilenames(): void + { + $this->assertPasses('test.jpg'); + $this->assertPasses('IMG_1234.png'); + $this->assertPasses('.hidden'); + $this->assertPasses('file with spaces.jpg'); + $this->assertPasses('café.jpg'); + $this->assertPasses('a'); + $this->assertPasses('file.tar.gz'); + } + + public function testNonStringValues(): void + { + $this->assertFails(123, ':attribute is not a string.'); + $this->assertFails(1.5, ':attribute is not a string.'); + $this->assertFails(true, ':attribute is not a string.'); + $this->assertFails(null, ':attribute is not a string.'); + $this->assertFails([], ':attribute is not a string.'); + $this->assertFails(['file.jpg'], ':attribute is not a string.'); + } + + public function testContainsDirectory(): void + { + $this->assertFails('foo/bar.jpg', ':attribute contains a directory, give proper filename.'); + $this->assertFails('/etc/passwd', ':attribute contains a directory, give proper filename.'); + $this->assertFails('/file.jpg', ':attribute contains a directory, give proper filename.'); + $this->assertFails('a/b/c.jpg', ':attribute contains a directory, give proper filename.'); + } + + public function testPathTraversalAttempts(): void + { + $this->assertFails('../secret.jpg'); + $this->assertFails('../../etc/passwd'); + $this->assertFails('..\\..\\windows\\win.ini'); + $this->assertFails('foo\\bar.jpg'); + $this->assertFails('foo/../bar.jpg'); + } + + public function testDoubleDotWithoutSeparatorStillFails(): void + { + // ".." alone is not treated as a directory by pathinfo(), but it must + // still be rejected because it is caught by the invalid-characters check. + $this->assertFails('..'); + $this->assertFails('a..b.jpg'); + } + + public function testEmptyStringPasses(): void + { + // pathinfo('', PATHINFO_DIRNAME) === '' and it contains none of the + // forbidden substrings, so the rule itself does not flag it. Emptiness + // should be rejected by a separate `required` rule upstream. + $this->assertPasses(''); + } +} From 3d937e626d5dd6d896d4c0ebfe442e54fcb4471c Mon Sep 17 00:00:00 2001 From: ildyria Date: Thu, 6 Aug 2026 21:49:46 +0200 Subject: [PATCH 2/7] Improved validation --- app/Jobs/ExtractZip.php | 27 ++++++++++--------- app/Rules/FilenameRule.php | 6 +++++ app/Services/Zip/SafeZipExtractor.php | 39 +++++++++++++++++++++------ tests/Unit/Rules/FilenameRuleTest.php | 1 + 4 files changed, 52 insertions(+), 21 deletions(-) diff --git a/app/Jobs/ExtractZip.php b/app/Jobs/ExtractZip.php index f3a255e43f3..0e894b3aaf2 100644 --- a/app/Jobs/ExtractZip.php +++ b/app/Jobs/ExtractZip.php @@ -54,6 +54,7 @@ class ExtractZip implements ShouldQueue public ?int $file_last_modified_time; private FileExtensionService $file_extension_service; + private ?string $base_extract_dir = null; /** * Create a new job instance. @@ -87,18 +88,10 @@ public function handle(): void $this->history->status = JobStatus::STARTED; $this->history->save(); + $this->base_extract_dir = realpath(Storage::disk('extract-jobs')->path('')); $this->validate_zip(); - $valid_path = realpath(Storage::disk('extract-jobs')->path('')); - $path_extracted = realpath(Storage::disk('extract-jobs')->path(date('Ymd') . ' ' . $this->getExtractFolderName())); - - if (str_starts_with($path_extracted, $valid_path) === false) { - Log::channel('jobs')->critical("Extraction path {$path_extracted} is not within the valid extraction directory {$valid_path}."); - $this->history->status = JobStatus::FAILURE; - $this->history->save(); - - throw new LycheeLogicException("Extraction path {$path_extracted} is not within the valid extraction directory {$valid_path}."); - } + $path_extracted = Storage::disk('extract-jobs')->path(date('Ymd') . ' ' . $this->getExtractFolderName()); $this->extract_zip($path_extracted); @@ -178,10 +171,17 @@ private function should_import_from_extracted(string $path_extracted): bool * Builds a SafeZipExtractor configured from the "Image Processing" zip-bomb * protection settings (expert settings, in the admin config). * + * @param string $base_extract_dir The base directory where the ZIP file will be extracted. + * This is used to prevent path traversal attacks (zip slip). + * * @return SafeZipExtractor */ - private function makeSafeZipExtractor(): SafeZipExtractor + private function makeSafeZipExtractor(?string $base_extract_dir): SafeZipExtractor { + if ($base_extract_dir === null) { + throw new LycheeLogicException('Base extract directory must be set before creating SafeZipExtractor.'); + } + $config_manager = app(ConfigManager::class); return new SafeZipExtractor( @@ -189,6 +189,7 @@ private function makeSafeZipExtractor(): SafeZipExtractor max_file_size: $config_manager->getValueAsByteSize('zip_bomb_max_file_size'), max_entries: $config_manager->getValueAsInt('zip_bomb_max_entries'), max_ratio: $config_manager->getValueAsInt('zip_bomb_max_ratio'), + base_extract_dir: $base_extract_dir, ); } @@ -260,7 +261,7 @@ private function validate_zip(): void } try { - $is_safe = $this->makeSafeZipExtractor()->inspect($this->file_path); + $is_safe = $this->makeSafeZipExtractor($this->base_extract_dir)->inspect($this->file_path); } catch (\Throwable $e) { Log::channel('jobs')->critical('Zip file ' . $this->file_path . ' could not be inspected: ' . $e->getMessage()); $is_safe = false; @@ -303,7 +304,7 @@ private function validate_zip(): void private function extract_zip(string $path_extracted): void { try { - $this->makeSafeZipExtractor()->extract($this->file_path, $path_extracted); + $this->makeSafeZipExtractor($this->base_extract_dir)->extract($this->file_path, $path_extracted); } catch (ZipBombDetectedException $e) { Log::channel('jobs')->critical('Zip file ' . $this->file_path . ' exceeds the configured zip-bomb protection limits: ' . $e->getMessage()); diff --git a/app/Rules/FilenameRule.php b/app/Rules/FilenameRule.php index 0d17e54d014..a08112c810d 100644 --- a/app/Rules/FilenameRule.php +++ b/app/Rules/FilenameRule.php @@ -27,6 +27,12 @@ public function validate(string $attribute, mixed $value, \Closure $fail): void return; } + if ($value === '.') { + $fail(':attribute is not a valid file name.'); + + return; + } + $dir_name = pathinfo($value, PATHINFO_DIRNAME); if ($dir_name !== '' && $dir_name !== '.') { $fail(':attribute contains a directory, give proper filename.'); diff --git a/app/Services/Zip/SafeZipExtractor.php b/app/Services/Zip/SafeZipExtractor.php index f602dbc6d45..acde634a56f 100644 --- a/app/Services/Zip/SafeZipExtractor.php +++ b/app/Services/Zip/SafeZipExtractor.php @@ -46,6 +46,8 @@ public function __construct( private int $max_ratio, // Read chunk size while streaming. private int $chunk_size = 8192, + // Base extract directory (for path traversal checks). + private string $base_extract_dir = '', ) { } @@ -110,9 +112,19 @@ public function extract(string $zip_path, string $dest_dir): void throw new ZipBombDetectedException('Archive rejected by pre-flight inspection.'); } + $dest_dir = DIRECTORY_SEPARATOR . $this->resolveTarget($dest_dir); + if ($dest_dir === null) { + throw new \RuntimeException('Invalid destination directory.'); + } + + if ($this->base_extract_dir !== '' && !str_starts_with($dest_dir, $this->base_extract_dir)) { + throw new \RuntimeException('Destination directory is outside the allowed base directory.'); + } + if (!is_dir($dest_dir)) { mkdir($dest_dir, 0755, true); } + $base_real = realpath($dest_dir); $zip = new \ZipArchive(); @@ -200,6 +212,24 @@ public function extract(string $zip_path, string $dest_dir): void * $base_real, or null if the entry tries to escape (zip slip). */ private function resolveSafeTarget(string $base_real, string $entry_name): ?string + { + $target = $this->resolveTarget($entry_name); + if ($target === null) { + // Invalid entry name (empty, null byte, or path traversal). + return null; + } + + return $base_real . DIRECTORY_SEPARATOR . $target; + } + + /** + * Resolve the target path while getting rid of any path traversal attempts. + * + * @param string $entry_name + * + * @return string|null the resolved path relative to the base directory, or null if the entry is invalid or tries to escape + */ + private function resolveTarget(string $entry_name): string|null { if ($entry_name === '' || str_contains($entry_name, "\0")) { return null; @@ -228,13 +258,6 @@ private function resolveSafeTarget(string $base_real, string $entry_name): ?stri return null; } - $target = $base_real . DIRECTORY_SEPARATOR . implode(DIRECTORY_SEPARATOR, $parts); - - // Final belt-and-braces prefix check. - if (!str_starts_with($target, $base_real . DIRECTORY_SEPARATOR)) { - return null; - } - - return $target; + return implode(DIRECTORY_SEPARATOR, $parts); } } diff --git a/tests/Unit/Rules/FilenameRuleTest.php b/tests/Unit/Rules/FilenameRuleTest.php index 45204cdf368..6968995a570 100644 --- a/tests/Unit/Rules/FilenameRuleTest.php +++ b/tests/Unit/Rules/FilenameRuleTest.php @@ -73,6 +73,7 @@ public function testContainsDirectory(): void public function testPathTraversalAttempts(): void { + $this->assertFails('.'); $this->assertFails('../secret.jpg'); $this->assertFails('../../etc/passwd'); $this->assertFails('..\\..\\windows\\win.ini'); From 0fbfe5cb69ae0048331d785abe8a5a592671fa26 Mon Sep 17 00:00:00 2001 From: ildyria Date: Thu, 6 Aug 2026 21:56:08 +0200 Subject: [PATCH 3/7] fix --- app/Services/Zip/SafeZipExtractor.php | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/app/Services/Zip/SafeZipExtractor.php b/app/Services/Zip/SafeZipExtractor.php index acde634a56f..64d1d1f789b 100644 --- a/app/Services/Zip/SafeZipExtractor.php +++ b/app/Services/Zip/SafeZipExtractor.php @@ -112,10 +112,12 @@ public function extract(string $zip_path, string $dest_dir): void throw new ZipBombDetectedException('Archive rejected by pre-flight inspection.'); } - $dest_dir = DIRECTORY_SEPARATOR . $this->resolveTarget($dest_dir); + $dest_dir = $this->resolveTarget($dest_dir); if ($dest_dir === null) { throw new \RuntimeException('Invalid destination directory.'); } + // Ensure the destination directory is absolute and starts with a slash. + $dest_dir = DIRECTORY_SEPARATOR . ltrim($dest_dir, DIRECTORY_SEPARATOR); if ($this->base_extract_dir !== '' && !str_starts_with($dest_dir, $this->base_extract_dir)) { throw new \RuntimeException('Destination directory is outside the allowed base directory.'); From ad5abad377cd8b05e33abdb10d30a4142ac133f3 Mon Sep 17 00:00:00 2001 From: ildyria Date: Thu, 6 Aug 2026 22:42:42 +0200 Subject: [PATCH 4/7] fix test --- tests/Unit/Jobs/ExtractZipTest.php | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/tests/Unit/Jobs/ExtractZipTest.php b/tests/Unit/Jobs/ExtractZipTest.php index e73f4ea2fb7..3754457677d 100644 --- a/tests/Unit/Jobs/ExtractZipTest.php +++ b/tests/Unit/Jobs/ExtractZipTest.php @@ -68,6 +68,14 @@ private function makeFile(): ProcessableJobFile private function callValidateZip(ExtractZip $job): void { + // `base_extract_dir` is normally set by handle() before validate_zip() + // runs. Since these tests invoke validate_zip() directly (handle() + // performs a real import that isn't unit-testable), it must be seeded + // here, or makeSafeZipExtractor() throws and validate_zip() silently + // treats the archive as unsafe. + $base_extract_dir = new \ReflectionProperty(ExtractZip::class, 'base_extract_dir'); + $base_extract_dir->setValue($job, sys_get_temp_dir()); + $method = new \ReflectionMethod(ExtractZip::class, 'validate_zip'); $method->invoke($job); } From d2667eb5dcf70cf2dc1676ac2ab6a0badcd58450 Mon Sep 17 00:00:00 2001 From: ildyria Date: Fri, 7 Aug 2026 15:09:53 +0200 Subject: [PATCH 5/7] make the rule more flexible for tests --- app/Rules/FilenameRule.php | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/app/Rules/FilenameRule.php b/app/Rules/FilenameRule.php index a08112c810d..131ca361bdd 100644 --- a/app/Rules/FilenameRule.php +++ b/app/Rules/FilenameRule.php @@ -9,6 +9,7 @@ namespace App\Rules; use Illuminate\Contracts\Validation\ValidationRule; +use Illuminate\Support\Facades\App; use Illuminate\Support\Str; /** @@ -34,6 +35,12 @@ public function validate(string $attribute, mixed $value, \Closure $fail): void } $dir_name = pathinfo($value, PATHINFO_DIRNAME); + + // Allow test files to be in subdirectories of tests/Feature_v2/ + if (App::runningUnitTests() && str_starts_with($dir_name, 'tests/Samples')) { + return; + } + if ($dir_name !== '' && $dir_name !== '.') { $fail(':attribute contains a directory, give proper filename.'); From 67a551e5fdf05eb738fa3e577262545956ce37a9 Mon Sep 17 00:00:00 2001 From: ildyria Date: Fri, 7 Aug 2026 15:16:33 +0200 Subject: [PATCH 6/7] more armors --- app/Rules/FilenameRule.php | 8 ++++++-- tests/Unit/Rules/FilenameRuleTest.php | 1 + 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/app/Rules/FilenameRule.php b/app/Rules/FilenameRule.php index 131ca361bdd..b139e4de07e 100644 --- a/app/Rules/FilenameRule.php +++ b/app/Rules/FilenameRule.php @@ -36,8 +36,12 @@ public function validate(string $attribute, mixed $value, \Closure $fail): void $dir_name = pathinfo($value, PATHINFO_DIRNAME); - // Allow test files to be in subdirectories of tests/Feature_v2/ - if (App::runningUnitTests() && str_starts_with($dir_name, 'tests/Samples')) { + // Allow test files to be in subdirectories of tests/Samples, + // but not in any other subdirectory. + if (App::runningUnitTests() + && str_starts_with($dir_name, 'tests/Samples') + && !str_contains($dir_name, '..') + ) { return; } diff --git a/tests/Unit/Rules/FilenameRuleTest.php b/tests/Unit/Rules/FilenameRuleTest.php index 6968995a570..6bb70ff5ea1 100644 --- a/tests/Unit/Rules/FilenameRuleTest.php +++ b/tests/Unit/Rules/FilenameRuleTest.php @@ -75,6 +75,7 @@ public function testPathTraversalAttempts(): void { $this->assertFails('.'); $this->assertFails('../secret.jpg'); + $this->assertFails('test/Samples/../secret.jpg'); $this->assertFails('../../etc/passwd'); $this->assertFails('..\\..\\windows\\win.ini'); $this->assertFails('foo\\bar.jpg'); From f916f6b908bac869475e3ac6d7abb1d152570317 Mon Sep 17 00:00:00 2001 From: ildyria Date: Fri, 7 Aug 2026 15:25:24 +0200 Subject: [PATCH 7/7] formatting --- app/Rules/FilenameRule.php | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/app/Rules/FilenameRule.php b/app/Rules/FilenameRule.php index b139e4de07e..dfc3996f1ef 100644 --- a/app/Rules/FilenameRule.php +++ b/app/Rules/FilenameRule.php @@ -38,9 +38,9 @@ public function validate(string $attribute, mixed $value, \Closure $fail): void // Allow test files to be in subdirectories of tests/Samples, // but not in any other subdirectory. - if (App::runningUnitTests() - && str_starts_with($dir_name, 'tests/Samples') - && !str_contains($dir_name, '..') + if (App::runningUnitTests() && + str_starts_with($dir_name, 'tests/Samples') && + !str_contains($dir_name, '..') ) { return; }