Fix slipery zip extraction

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',

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
@@ -52,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.
@@ -85,9 +88,11 @@ 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();
 
 		$path_extracted = Storage::disk('extract-jobs')->path(date('Ymd') . ' ' . $this->getExtractFolderName());
+
 		$this->extract_zip($path_extracted);
 
 		$config_manager = app(ConfigManager::class);
@@ -166,17 +171,25 @@ 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(
 			max_total_size: $config_manager->getValueAsByteSize('zip_bomb_max_total_size'),
 			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,
 		);
 	}
 
@@ -248,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;
@@ -291,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());
 

app/Rules/FilenameRule.php

@@ -0,0 +1,60 @@
+<?php
+
+/**
+ * SPDX-License-Identifier: MIT
+ * Copyright (c) 2017-2018 Tobias Reich
+ * Copyright (c) 2018-2026 LycheeOrg.
+ */
+
+namespace App\Rules;
+
+use Illuminate\Contracts\Validation\ValidationRule;
+use Illuminate\Support\Facades\App;
+use Illuminate\Support\Str;
+
+/**
+ * This rule is designed specifically to avoid path injection.
+ */
+final class FilenameRule implements ValidationRule
+{
+	/**
+	 * {@inheritDoc}
+	 */
+	public function validate(string $attribute, mixed $value, \Closure $fail): void
+	{
+		if (is_string($value) === false) {
+			$fail(':attribute is not a string.');
+
+			return;
+		}
+
+		if ($value === '.') {
+			$fail(':attribute is not a valid file name.');
+
+			return;
+		}
+
+		$dir_name = pathinfo($value, PATHINFO_DIRNAME);
+
+		// 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;
+		}
+
+		if ($dir_name !== '' && $dir_name !== '.') {
+			$fail(':attribute contains a directory, give proper filename.');
+
+			return;
+		}
+
+		if (Str::of($value)->contains(['..', '/', '\\'])) {
+			$fail(':attribute contains invalid characters:.');
+
+			return;
+		}
+	}
+}

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,21 @@ public function extract(string $zip_path, string $dest_dir): void
 			throw new ZipBombDetectedException('Archive rejected by pre-flight inspection.');
 		}
 
+		$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.');
+		}
+
 		if (!is_dir($dest_dir)) {
 			mkdir($dest_dir, 0755, true);
 		}
+
 		$base_real = realpath($dest_dir);
 
 		$zip = new \ZipArchive();
@@ -200,6 +214,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 +260,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);
 	}
 }

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

tests/Unit/Rules/FilenameRuleTest.php

@@ -0,0 +1,100 @@
+<?php
+
+/**
+ * SPDX-License-Identifier: MIT
+ * Copyright (c) 2017-2018 Tobias Reich
+ * Copyright (c) 2018-2026 LycheeOrg.
+ */
+
+/**
+ * We don't care for unhandled exceptions in tests.
+ * It is the nature of a test to throw an exception.
+ * Without this suppression we had 100+ Linter warning in this file which
+ * don't help anything.
+ *
+ * @noinspection PhpDocMissingThrowsInspection
+ * @noinspection PhpUnhandledExceptionInspection
+ */
+
+namespace Tests\Unit\Rules;
+
+use App\Rules\FilenameRule;
+use Tests\AbstractTestCase;
+
+class FilenameRuleTest extends AbstractTestCase
+{
+	private function assertPasses(mixed $value): void
+	{
+		$rule = new FilenameRule();
+		$failed = false;
+		$rule->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('.');
+		$this->assertFails('../secret.jpg');
+		$this->assertFails('test/Samples/../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('');
+	}
+}