From 4c17b0f05260c31d05f4d2d53987c41eef49ab13 Mon Sep 17 00:00:00 2001 From: Alex Date: Mon, 26 Oct 2026 00:31:05 +0100 Subject: [PATCH 1/7] fix(offline-sync): validate the screenshot archive before extracting it Offline screenshots are processed within the request, so an unbounded archive could exceed the 90 s execution limit after attaching part of it, or fill the disk when extracted. Before extraction the zip directory is checked: at most OFFLINE_SCREENSHOTS_MAX_FILES entries (default 500), at most OFFLINE_SCREENSHOTS_MAX_SIZE_BYTES uncompressed (default 1 GiB, per the archive headers), and only flat file names (no /, \, .., NUL or drive prefix). Violations return 422 with nothing extracted or attached. Closes #422 --- .env.example | 6 + .../Controllers/Api/IntervalController.php | 57 +++++++ config/app.php | 8 + resources/lang/en/validation.php | 5 +- resources/lang/ru/validation.php | 5 +- .../OfflineScreenshotsArchiveLimitsTest.php | 146 ++++++++++++++++++ .../TimeIntervals/ScreenshotTmpFilesTest.php | 55 +------ .../UploadsOfflineScreenshots.php | 81 ++++++++++ 8 files changed, 308 insertions(+), 55 deletions(-) create mode 100644 tests/Feature/TimeIntervals/OfflineScreenshotsArchiveLimitsTest.php create mode 100644 tests/Feature/TimeIntervals/UploadsOfflineScreenshots.php diff --git a/.env.example b/.env.example index 4cdec8823..d0d41c4a0 100644 --- a/.env.example +++ b/.env.example @@ -64,6 +64,12 @@ REVERB_APP_SECRET=secret ## Optional for every user, use optional SCREENSHOTS_STATE=any +# Offline screenshot archives are processed within the upload request (Octane max_execution_time: 90 s) and +# rejected before extraction above these limits: number of screenshots and total uncompressed size in bytes. +# ~75 ms per screenshot on the local disk; S3 is slower (up to ~300 ms), lower OFFLINE_SCREENSHOTS_MAX_FILES there. +# OFFLINE_SCREENSHOTS_MAX_FILES=500 +# OFFLINE_SCREENSHOTS_MAX_SIZE_BYTES=1073741824 + # Octane (Swoole) worker counts; unset = Swoole default (one per host CPU) # OCTANE_WORKERS= # OCTANE_TASK_WORKERS= diff --git a/app/Http/Controllers/Api/IntervalController.php b/app/Http/Controllers/Api/IntervalController.php index ef43205aa..ed02309c9 100644 --- a/app/Http/Controllers/Api/IntervalController.php +++ b/app/Http/Controllers/Api/IntervalController.php @@ -32,6 +32,7 @@ use Illuminate\Http\Request; use Illuminate\Http\UploadedFile; use Illuminate\Support\Arr; +use Illuminate\Validation\ValidationException; use MessagePack\MessagePack; use phpseclib4\Crypt\RSA; use Settings; @@ -807,6 +808,9 @@ public function uploadOfflineScreenshots(UploadOfflineScreenshotsRequest $reques __('Cannot open file.' . is_int($zipOpenResult) ? " ZipArchive error code: $zipOpenResult" : ""), ); + // Before the temporary directory is created: a rejected archive leaves nothing behind. + $this->validateOfflineScreenshotsArchive($zip); + // The archive is extracted to the local disk of this web container and processed within the request, // so the queue worker does not need access to these files. The directory is removed when the request // ends, also on failure; leftovers of interrupted requests are removed by cattr:tmp:clean. @@ -828,6 +832,59 @@ public function uploadOfflineScreenshots(UploadOfflineScreenshotsRequest $reques return responder()->success($creationResult)->respond(); } + /** + * Rejects the archive before anything is extracted or attached if it cannot be processed within the request + * (too many entries or too large when unpacked) or if an entry is not a plain file name. + * + * The desktop application exports a flat list of `_` entries without directories. + * + * @throws ValidationException + */ + private function validateOfflineScreenshotsArchive(ZipArchive $zip): void + { + $maxFiles = (int)config('app.offline_screenshots.max_files'); + $maxSize = (int)config('app.offline_screenshots.max_size'); + + $fileCount = $zip->count(); + if ($fileCount > $maxFiles) { + throw ValidationException::withMessages([ + 'file' => __('validation.offline-sync.too_many_files', ['count' => $fileCount, 'max' => $maxFiles]), + ]); + } + + $totalSize = 0; + for ($index = 0; $index < $fileCount; $index++) { + $name = $zip->getNameIndex($index); + $stat = $zip->statIndex($index); + + if (!is_string($name) || $stat === false || !self::isPlainArchiveEntryName($name)) { + throw ValidationException::withMessages([ + 'file' => __('validation.offline-sync.wrong_file_name'), + ]); + } + + $totalSize += $stat['size']; + if ($totalSize > $maxSize) { + throw ValidationException::withMessages([ + 'file' => __('validation.offline-sync.too_large', [ + 'max' => round($maxSize / 1024 / 1024), + ]), + ]); + } + } + } + + /** + * A plain file name: no directories, no parent references, no absolute or drive paths and no NUL bytes. + */ + private static function isPlainArchiveEntryName(string $name): bool + { + return $name !== '' + && strpbrk($name, "/\\\0") === false + && !str_contains($name, '..') + && preg_match('/^[A-Za-z]:/', $name) !== 1; + } + /** * Saves the extracted offline screenshots synchronously, so that failures are reported in the result. * diff --git a/config/app.php b/config/app.php index 717e6a8f4..b6c1a17ff 100644 --- a/config/app.php +++ b/config/app.php @@ -38,6 +38,14 @@ // Requests per minute and IP to the public signed screenshot routes (time-intervals/{id}/screenshot|thumb). // A timeline day of a user with 1-minute screenshots holds up to ~600 thumbnails (10 h of work). 'screenshots_rate_limit' => (int) env('SCREENSHOTS_RATE_LIMIT', 600), + // Offline screenshots (offline-sync/upload-screenshots) are processed within the upload request, which must + // finish within Octane's max_execution_time (90 s). Larger archives are rejected before anything is extracted. + // 500 entries ≈ 5 working days of 96 screenshots, ~40 s at ~75 ms per 1920×1080 screenshot on the local disk. + 'offline_screenshots' => [ + 'max_files' => (int) env('OFFLINE_SCREENSHOTS_MAX_FILES', 500), + // Total uncompressed size of all archive entries + 'max_size' => (int) env('OFFLINE_SCREENSHOTS_MAX_SIZE_BYTES', 1024 * 1024 * 1024), + ], 'user_activity' => [ 'online_status_time' => 60, 'heartbeat_period' => 30, diff --git a/resources/lang/en/validation.php b/resources/lang/en/validation.php index 127a96fb8..5ca910efc 100644 --- a/resources/lang/en/validation.php +++ b/resources/lang/en/validation.php @@ -114,7 +114,10 @@ 'screenshot_not_attached' => 'Screenshot not attached (see logs)', 'time_interval_already_exist' => 'The time for this interval already exists.', 'time_interval_added' => 'Added', - 'wrong_extension' => 'File extension should be .cattr' + 'wrong_extension' => 'File extension should be .cattr', + 'too_many_files' => 'The archive contains too many screenshots (:count), the maximum is :max', + 'too_large' => 'The unpacked screenshots are too large, the maximum is :max MB', + 'wrong_file_name' => 'The archive must contain only screenshot files without folders' ], 'tasks-relations' => [ diff --git a/resources/lang/ru/validation.php b/resources/lang/ru/validation.php index 503835413..bff7c6924 100644 --- a/resources/lang/ru/validation.php +++ b/resources/lang/ru/validation.php @@ -149,7 +149,10 @@ 'screenshot_not_attached' => 'Скриншот не прикреплён (см. логи)', 'time_interval_already_exist' => 'Время для этого интервала уже существует.', 'time_interval_added' => 'Добавлен', - 'wrong_extension' => 'Расширение файла должно быть .cattr' + 'wrong_extension' => 'Расширение файла должно быть .cattr', + 'too_many_files' => 'Архив содержит слишком много скриншотов (:count), максимум — :max', + 'too_large' => 'Распакованные скриншоты слишком большие, максимум — :max МБ', + 'wrong_file_name' => 'Архив должен содержать только файлы скриншотов без папок' ], 'tasks-relations' => [ diff --git a/tests/Feature/TimeIntervals/OfflineScreenshotsArchiveLimitsTest.php b/tests/Feature/TimeIntervals/OfflineScreenshotsArchiveLimitsTest.php new file mode 100644 index 000000000..bcc9529dc --- /dev/null +++ b/tests/Feature/TimeIntervals/OfflineScreenshotsArchiveLimitsTest.php @@ -0,0 +1,146 @@ +screenshotService = app(ScreenshotService::class); + + $this->user = $this->createOfflineAdmin(); + $this->interval = $this->createOfflineInterval($this->user, TaskFactory::forUser($this->user)->create()); + $this->screenshotId = $this->interval->screenshot_id; + } + + public function test_default_limits(): void + { + $this->assertSame(500, config('app.offline_screenshots.max_files')); + $this->assertSame(1024 * 1024 * 1024, config('app.offline_screenshots.max_size')); + } + + public function test_archive_within_the_limits_is_attached(): void + { + config(['app.offline_screenshots.max_files' => 2]); + + $response = $this->uploadOfflineArchive($this->user, $this->validEntry() + [ + "{$this->user->id}_" . Str::uuid() => $this->image(), + ]); + + $response->assertOk()->assertJsonCount(2, 'data'); + $result = collect($response->json('data'))->firstWhere('screenshot_id', $this->screenshotId); + $this->assertTrue($result['success']); + $this->interval->refresh(); + $this->assertNull($this->interval->screenshot_id); + Storage::assertExists($this->screenshotService->getScreenshotPath($this->interval)); + $this->assertTmpIsEmpty(); + } + + public function test_archive_with_too_many_entries_is_rejected(): void + { + config(['app.offline_screenshots.max_files' => 2]); + + $entries = $this->validEntry(); + for ($i = 0; $i < 2; $i++) { + $entries["{$this->user->id}_" . Str::uuid()] = $this->image(); + } + + $response = $this->uploadOfflineArchive($this->user, $entries); + + $this->assertRejected( + $response, + __('validation.offline-sync.too_many_files', ['count' => 3, 'max' => 2]), + ); + } + + public function test_archive_too_large_when_unpacked_is_rejected(): void + { + config(['app.offline_screenshots.max_size' => 1024 * 1024]); + + // ~1.5 MiB unpacked, a few KiB packed + $entries = $this->validEntry(); + for ($i = 0; $i < 3; $i++) { + $entries["{$this->user->id}_" . Str::uuid()] = str_repeat("\0", 512 * 1024); + } + + $response = $this->uploadOfflineArchive($this->user, $entries); + + $this->assertRejected($response, __('validation.offline-sync.too_large', ['max' => 1])); + } + + public static function unsafeEntryNames(): array + { + return [ + 'parent directory (Zip Slip)' => ['../evil.jpg'], + 'nested parent directory (Zip Slip)' => ['screenshots/../../evil.jpg'], + 'absolute path' => ['/tmp/evil.jpg'], + 'windows parent directory' => ['..\\evil.jpg'], + 'windows absolute path' => ['C:\\evil.jpg'], + 'windows drive-relative path' => ['C:evil.jpg'], + 'directory' => ['screenshots/1_00000000-0000-0000-0000-000000000000'], + ]; + } + + #[DataProvider('unsafeEntryNames')] + public function test_archive_with_an_unsafe_entry_name_is_rejected(string $name): void + { + $response = $this->uploadOfflineArchive($this->user, $this->validEntry() + [$name => $this->image()]); + + $this->assertRejected($response, __('validation.offline-sync.wrong_file_name')); + } + + /** + * @return array A screenshot that would be attached to the interval if the archive were accepted + */ + private function validEntry(): array + { + return ["{$this->user->id}_$this->screenshotId" => $this->image()]; + } + + private function image(): string + { + return UploadedFile::fake()->image('s.png', 64, 48)->getContent(); + } + + private function assertRejected(TestResponse $response, string $message): void + { + $response->assertStatus(self::HTTP_UNPROCESSABLE_ENTITY); + $this->assertStringContainsString($message, $response->getContent()); + + $this->interval->refresh(); + $this->assertSame($this->screenshotId, $this->interval->screenshot_id, 'The screenshot was attached'); + Storage::assertMissing($this->screenshotService->getScreenshotPath($this->interval)); + Queue::assertNotPushed(GenerateScreenshotThumbnail::class); + $this->assertTmpIsEmpty(); + } +} diff --git a/tests/Feature/TimeIntervals/ScreenshotTmpFilesTest.php b/tests/Feature/TimeIntervals/ScreenshotTmpFilesTest.php index 6c4978071..ed061dadf 100644 --- a/tests/Feature/TimeIntervals/ScreenshotTmpFilesTest.php +++ b/tests/Feature/TimeIntervals/ScreenshotTmpFilesTest.php @@ -21,7 +21,6 @@ use Tests\Facades\TaskFactory; use Tests\Facades\UserFactory; use Tests\TestCase; -use ZipArchive; /** * Uploaded screenshots must be processed within the request and leave nothing in storage/app/tmp. @@ -31,12 +30,11 @@ */ class ScreenshotTmpFilesTest extends TestCase { + use UploadsOfflineScreenshots; + private const CREATE_URI = 'time-intervals/create'; - private const OFFLINE_URI = 'offline-sync/upload-screenshots'; private ScreenshotService $screenshotService; - /** @var array Tokens of users created with a fixed ID */ - private array $tokens = []; protected function setUp(): void { @@ -285,53 +283,4 @@ private function postInterval(User $user, Task $task): \Illuminate\Testing\TestR ['Accept' => 'application/json'], ); } - - private function createOfflineInterval(User $user, Task $task): TimeInterval - { - $interval = IntervalFactory::forUser($user)->forTask($task)->create(); - $interval->update(['screenshot_id' => (string)Str::uuid()]); - - return $interval; - } - - /** - * Creates a regular user with the given ID, to cover user IDs of different lengths. - */ - private function createUserWithId(int $id): User - { - if (User::whereKey($id)->exists()) { - $this->markTestSkipped("User ID $id is taken."); - } - - $user = User::forceCreate(['id' => $id] + UserFactory::createRandomModelData()); - $this->tokens[$id] = $user->createToken('testing-token', ['*'], now()->addDay())->plainTextToken; - - return $user; - } - - /** - * @param array $entries File name => content - */ - private function uploadOfflineArchive(User $user, array $entries): \Illuminate\Testing\TestResponse - { - $zipPath = tempnam(sys_get_temp_dir(), 'offline'); - $zip = new ZipArchive(); - $zip->open($zipPath, ZipArchive::OVERWRITE); - foreach ($entries as $name => $content) { - $zip->addFromString($name, $content); - } - $zip->close(); - - return $this->actingAs($this->tokens[$user->id] ?? $user)->post( - self::OFFLINE_URI, - ['file' => new UploadedFile($zipPath, 'screenshots.cattr', 'application/zip', null, true)], - ['Accept' => 'application/json'], - ); - } - - private function assertTmpIsEmpty(): void - { - $this->assertSame([], Storage::disk('local')->allFiles('tmp'), 'Files left in storage/app/tmp'); - $this->assertSame([], Storage::disk('local')->directories('tmp'), 'Directories left in storage/app/tmp'); - } } diff --git a/tests/Feature/TimeIntervals/UploadsOfflineScreenshots.php b/tests/Feature/TimeIntervals/UploadsOfflineScreenshots.php new file mode 100644 index 000000000..351614e0e --- /dev/null +++ b/tests/Feature/TimeIntervals/UploadsOfflineScreenshots.php @@ -0,0 +1,81 @@ +_` entries. + */ +trait UploadsOfflineScreenshots +{ + /** @var array API tokens of the users created by these helpers, by user ID */ + private array $tokens = []; + + private function createOfflineInterval(User $user, Task $task): TimeInterval + { + $interval = IntervalFactory::forUser($user)->forTask($task)->create(); + $interval->update(['screenshot_id' => (string)Str::uuid()]); + + return $interval; + } + + /** + * Creates a regular user with the given ID, to cover user IDs of different lengths. + */ + private function createUserWithId(int $id): User + { + if (User::whereKey($id)->exists()) { + $this->markTestSkipped("User ID $id is taken."); + } + + $user = User::forceCreate(['id' => $id] + UserFactory::createRandomModelData()); + $this->tokens[$id] = $user->createToken('testing-token', ['*'], now()->addDay())->plainTextToken; + + return $user; + } + + private function createOfflineAdmin(): User + { + $user = User::forceCreate(['role_id' => UserFactoryInstance::ADMIN_ROLE] + UserFactory::createRandomModelData()); + $this->tokens[$user->id] = $user->createToken('testing-token', ['*'], now()->addDay())->plainTextToken; + + return $user; + } + + /** + * @param array $entries File name => content + */ + private function uploadOfflineArchive(User $user, array $entries): TestResponse + { + $zipPath = tempnam(sys_get_temp_dir(), 'offline'); + $zip = new ZipArchive(); + $zip->open($zipPath, ZipArchive::OVERWRITE); + foreach ($entries as $name => $content) { + $zip->addFromString($name, $content); + } + $zip->close(); + + return $this->actingAs($this->tokens[$user->id] ?? $user)->post( + 'offline-sync/upload-screenshots', + ['file' => new UploadedFile($zipPath, 'screenshots.cattr', 'application/zip', null, true)], + ['Accept' => 'application/json'], + ); + } + + private function assertTmpIsEmpty(): void + { + $this->assertSame([], Storage::disk('local')->allFiles('tmp'), 'Files left in storage/app/tmp'); + $this->assertSame([], Storage::disk('local')->directories('tmp'), 'Directories left in storage/app/tmp'); + } +} From c36eeed5b84564ddf4f96b806ce0fa39e9048c02 Mon Sep 17 00:00:00 2001 From: Alex Date: Mon, 26 Oct 2026 00:33:37 +0100 Subject: [PATCH 2/7] fix(offline-sync): default to 250 screenshots per offline archive 250 entries fit the 90 s request limit on both the local disk (~20 s) and S3 (~80 s at ~310 ms per screenshot). Refs #422 --- .env.example | 4 ++-- config/app.php | 5 +++-- .../TimeIntervals/OfflineScreenshotsArchiveLimitsTest.php | 2 +- 3 files changed, 6 insertions(+), 5 deletions(-) diff --git a/.env.example b/.env.example index d0d41c4a0..01f90b2ae 100644 --- a/.env.example +++ b/.env.example @@ -66,8 +66,8 @@ SCREENSHOTS_STATE=any # Offline screenshot archives are processed within the upload request (Octane max_execution_time: 90 s) and # rejected before extraction above these limits: number of screenshots and total uncompressed size in bytes. -# ~75 ms per screenshot on the local disk; S3 is slower (up to ~300 ms), lower OFFLINE_SCREENSHOTS_MAX_FILES there. -# OFFLINE_SCREENSHOTS_MAX_FILES=500 +# The default of 250 fits both the local disk (~75 ms per screenshot) and S3 (up to ~310 ms). +# OFFLINE_SCREENSHOTS_MAX_FILES=250 # OFFLINE_SCREENSHOTS_MAX_SIZE_BYTES=1073741824 # Octane (Swoole) worker counts; unset = Swoole default (one per host CPU) diff --git a/config/app.php b/config/app.php index b6c1a17ff..266566d50 100644 --- a/config/app.php +++ b/config/app.php @@ -40,9 +40,10 @@ 'screenshots_rate_limit' => (int) env('SCREENSHOTS_RATE_LIMIT', 600), // Offline screenshots (offline-sync/upload-screenshots) are processed within the upload request, which must // finish within Octane's max_execution_time (90 s). Larger archives are rejected before anything is extracted. - // 500 entries ≈ 5 working days of 96 screenshots, ~40 s at ~75 ms per 1920×1080 screenshot on the local disk. + // 250 entries ≈ 2.5 working days of 96 screenshots: ~20 s at ~75 ms per 1920×1080 screenshot on the local disk, + // ~80 s at ~310 ms on S3. 'offline_screenshots' => [ - 'max_files' => (int) env('OFFLINE_SCREENSHOTS_MAX_FILES', 500), + 'max_files' => (int) env('OFFLINE_SCREENSHOTS_MAX_FILES', 250), // Total uncompressed size of all archive entries 'max_size' => (int) env('OFFLINE_SCREENSHOTS_MAX_SIZE_BYTES', 1024 * 1024 * 1024), ], diff --git a/tests/Feature/TimeIntervals/OfflineScreenshotsArchiveLimitsTest.php b/tests/Feature/TimeIntervals/OfflineScreenshotsArchiveLimitsTest.php index bcc9529dc..59d7f4eae 100644 --- a/tests/Feature/TimeIntervals/OfflineScreenshotsArchiveLimitsTest.php +++ b/tests/Feature/TimeIntervals/OfflineScreenshotsArchiveLimitsTest.php @@ -45,7 +45,7 @@ protected function setUp(): void public function test_default_limits(): void { - $this->assertSame(500, config('app.offline_screenshots.max_files')); + $this->assertSame(250, config('app.offline_screenshots.max_files')); $this->assertSame(1024 * 1024 * 1024, config('app.offline_screenshots.max_size')); } From 0bc6e4e82a3c1bd27e99897395c3617777280070 Mon Sep 17 00:00:00 2001 From: Alex Date: Mon, 26 Oct 2026 00:45:31 +0100 Subject: [PATCH 3/7] fix(offline-sync): enforce the archive size limit while extracting The header check trusts the sizes declared in the zip, so a forged archive could pass it and still write its full decompressed content. Replace extractTo() with a loop that copies each entry in 64 KiB chunks against a running byte budget (OFFLINE_SCREENSHOTS_MAX_SIZE_BYTES) and rejects the upload with the same 422 too_large before the budget is exceeded. Entries that cannot be decoded are rejected with 400 instead of being written as empty files. Refs #422 --- .../Controllers/Api/IntervalController.php | 65 +++++++++- .../OfflineScreenshotsArchiveLimitsTest.php | 116 ++++++++++++++++++ .../UploadsOfflineScreenshots.php | 14 +++ 3 files changed, 189 insertions(+), 6 deletions(-) diff --git a/app/Http/Controllers/Api/IntervalController.php b/app/Http/Controllers/Api/IntervalController.php index ed02309c9..1edc96e5d 100644 --- a/app/Http/Controllers/Api/IntervalController.php +++ b/app/Http/Controllers/Api/IntervalController.php @@ -48,6 +48,8 @@ class IntervalController extends ItemController { protected const MODEL = TimeInterval::class; + private const OFFLINE_SCREENSHOTS_EXTRACTION_CHUNK_SIZE = 64 * 1024; + public function __construct(protected ScreenshotService $screenshotService) { } @@ -818,7 +820,7 @@ public function uploadOfflineScreenshots(UploadOfflineScreenshotsRequest $reques ->location(Storage::disk('local')->path('tmp'))->force()->create(); try { - $zip->extractTo($temporaryDirectory->path()); + $this->extractOfflineScreenshotsArchive($zip, $temporaryDirectory->path()); $zip->close(); $creationResult = $this->attachOfflineScreenshots( @@ -865,15 +867,66 @@ private function validateOfflineScreenshotsArchive(ZipArchive $zip): void $totalSize += $stat['size']; if ($totalSize > $maxSize) { - throw ValidationException::withMessages([ - 'file' => __('validation.offline-sync.too_large', [ - 'max' => round($maxSize / 1024 / 1024), - ]), - ]); + throw self::offlineScreenshotsArchiveTooLarge($maxSize); + } + } + } + + /** + * Extracts the validated archive into the directory, counting the bytes that are actually written. + * + * The sizes checked by validateOfflineScreenshotsArchive() come from the zip headers and can be forged, while + * ZipArchive::extractTo() writes whatever the entries decompress to. Here no more than the configured total + * size is ever written: each entry is copied in chunks of at most the remaining budget plus one byte, and the + * archive is rejected as soon as that extra byte is read. + * + * @throws ValidationException + */ + private function extractOfflineScreenshotsArchive(ZipArchive $zip, string $directory): void + { + $maxSize = (int)config('app.offline_screenshots.max_size'); + $remaining = $maxSize; + + $fileCount = $zip->count(); + for ($index = 0; $index < $fileCount; $index++) { + // The names were checked to be plain file names by validateOfflineScreenshotsArchive() + $name = $zip->getNameIndex($index); + $source = $name === false ? false : $zip->getStreamIndex($index); + abort_if($source === false, 400, __('Cannot open file.')); + + try { + $target = fopen($directory . DIRECTORY_SEPARATOR . $name, 'wb'); + try { + while (!feof($source)) { + // Undecodable compressed data is reported as a warning and a failed read + $chunk = @fread($source, min(self::OFFLINE_SCREENSHOTS_EXTRACTION_CHUNK_SIZE, $remaining + 1)); + abort_if($chunk === false, 400, __('Cannot open file.')); + + if (strlen($chunk) > $remaining) { + throw self::offlineScreenshotsArchiveTooLarge($maxSize); + } + + fwrite($target, $chunk); + $remaining -= strlen($chunk); + } + } finally { + fclose($target); + } + } finally { + fclose($source); } } } + private static function offlineScreenshotsArchiveTooLarge(int $maxSize): ValidationException + { + return ValidationException::withMessages([ + 'file' => __('validation.offline-sync.too_large', [ + 'max' => round($maxSize / 1024 / 1024), + ]), + ]); + } + /** * A plain file name: no directories, no parent references, no absolute or drive paths and no NUL bytes. */ diff --git a/tests/Feature/TimeIntervals/OfflineScreenshotsArchiveLimitsTest.php b/tests/Feature/TimeIntervals/OfflineScreenshotsArchiveLimitsTest.php index 59d7f4eae..542fe3db4 100644 --- a/tests/Feature/TimeIntervals/OfflineScreenshotsArchiveLimitsTest.php +++ b/tests/Feature/TimeIntervals/OfflineScreenshotsArchiveLimitsTest.php @@ -6,7 +6,9 @@ use App\Jobs\GenerateScreenshotThumbnail; use App\Models\TimeInterval; use App\Models\User; +use App\Services\ProductionScreenshotService; use Illuminate\Http\UploadedFile; +use Illuminate\Support\Collection; use Illuminate\Support\Facades\Queue; use Illuminate\Support\Facades\Storage; use Illuminate\Support\Str; @@ -14,6 +16,7 @@ use PHPUnit\Framework\Attributes\DataProvider; use Tests\Facades\TaskFactory; use Tests\TestCase; +use ZipArchive; /** * Offline screenshot archives are processed within the request, so an archive that cannot be processed in time @@ -98,6 +101,68 @@ public function test_archive_too_large_when_unpacked_is_rejected(): void $this->assertRejected($response, __('validation.offline-sync.too_large', ['max' => 1])); } + public function test_archive_exactly_at_the_size_limit_is_attached(): void + { + $entries = $this->validEntry() + ["{$this->user->id}_" . Str::uuid() => str_repeat("\0", 70 * 1024)]; + config(['app.offline_screenshots.max_size' => array_sum(array_map('strlen', $entries))]); + + $response = $this->uploadOfflineArchive($this->user, $entries); + + $response->assertOk()->assertJsonCount(2, 'data'); + $this->interval->refresh(); + $this->assertNull($this->interval->screenshot_id); + $this->assertTmpIsEmpty(); + } + + /** + * The zip headers claim a few bytes while the entry decompresses to several times the limit (a zip bomb with + * forged sizes): the header check passes, the limit is enforced on the bytes actually extracted. + */ + public function test_archive_with_forged_entry_sizes_is_rejected_while_extracting(): void + { + $maxSize = 1024 * 1024; + config(['app.offline_screenshots.max_size' => $maxSize]); + $extractedFileSizes = $this->recordExtractedFileSizesOnAttach(); + + // 3 MiB of zeros, ~3 KiB packed; the CRC stays the one of the real content + $name = "{$this->user->id}_$this->screenshotId"; + $zipPath = $this->createOfflineArchive([$name => str_repeat("\0", 3 * $maxSize)]); + $this->forgeArchiveEntrySize($zipPath, $name, 10); + + $zip = new ZipArchive(); + $zip->open($zipPath); + $this->assertSame(10, $zip->statIndex(0)['size'], 'The forged header is not read back'); + $zip->close(); + + $response = $this->uploadOfflineArchiveFile($this->user, $zipPath); + + // Without the limit on extraction the 3 MiB file is written and handed over to the screenshot service + $this->assertSame([], $extractedFileSizes->all(), 'Sizes of the extracted files when attaching'); + $this->assertRejected($response, __('validation.offline-sync.too_large', ['max' => 1])); + } + + /** + * ZipArchive::extractTo() wrote such an entry as an empty file without reporting an error. + */ + public function test_archive_with_a_corrupted_entry_is_rejected(): void + { + $name = "{$this->user->id}_$this->screenshotId"; + $zipPath = $this->createOfflineArchive([$name => $this->image()]); + + // An invalid deflate block type in place of the first compressed bytes of the only entry + $bytes = file_get_contents($zipPath); + $dataOffset = 30 + unpack('v', substr($bytes, 26, 2))[1] + unpack('v', substr($bytes, 28, 2))[1]; + file_put_contents($zipPath, substr_replace($bytes, str_repeat("\xFF", 8), $dataOffset, 8)); + + $response = $this->uploadOfflineArchiveFile($this->user, $zipPath); + + $response->assertStatus(self::HTTP_BAD_REQUEST); + $this->assertStringContainsString(__('Cannot open file.'), $response->getContent()); + $this->interval->refresh(); + $this->assertSame($this->screenshotId, $this->interval->screenshot_id, 'The screenshot was attached'); + $this->assertTmpIsEmpty(); + } + public static function unsafeEntryNames(): array { return [ @@ -119,6 +184,57 @@ public function test_archive_with_an_unsafe_entry_name_is_rejected(string $name) $this->assertRejected($response, __('validation.offline-sync.wrong_file_name')); } + /** + * Overwrites the uncompressed size of an entry in both its local file header and its central directory record + * (the archive has no data descriptors and no Zip64 records). The CRC-32 is left as is. + */ + private function forgeArchiveEntrySize(string $zipPath, string $name, int $size): void + { + + $bytes = file_get_contents($zipPath); + $patched = false; + for ($pos = strpos($bytes, "PK\x01\x02"); $pos !== false; $pos = strpos($bytes, "PK\x01\x02", $pos + 4)) { + $nameLength = unpack('v', substr($bytes, $pos + 28, 2))[1]; + if (substr($bytes, $pos + 46, $nameLength) !== $name) { + continue; + } + $localHeader = unpack('V', substr($bytes, $pos + 42, 4))[1]; + $bytes = substr_replace($bytes, pack('V', $size), $pos + 24, 4); + $bytes = substr_replace($bytes, pack('V', $size), $localHeader + 22, 4); + $patched = true; + } + $this->assertTrue($patched, "Entry $name not found in the archive"); + + file_put_contents($zipPath, $bytes); + } + + /** + * Replaces the screenshot service with one that records the size of every file in storage/app/tmp whenever a + * screenshot is attached, i.e. after the archive has been extracted. + * + * @return Collection + */ + private function recordExtractedFileSizesOnAttach(): Collection + { + $sizes = new Collection(); + $this->app->instance(ScreenshotService::class, new class ($sizes) extends ProductionScreenshotService { + public function __construct(private readonly Collection $sizes) + { + } + + public function saveScreenshot($file, $timeInterval): void + { + $disk = Storage::disk('local'); + foreach ($disk->allFiles('tmp') as $path) { + $this->sizes->push($disk->size($path)); + } + parent::saveScreenshot($file, $timeInterval); + } + }); + + return $sizes; + } + /** * @return array A screenshot that would be attached to the interval if the archive were accepted */ diff --git a/tests/Feature/TimeIntervals/UploadsOfflineScreenshots.php b/tests/Feature/TimeIntervals/UploadsOfflineScreenshots.php index 351614e0e..ae1a7fd8b 100644 --- a/tests/Feature/TimeIntervals/UploadsOfflineScreenshots.php +++ b/tests/Feature/TimeIntervals/UploadsOfflineScreenshots.php @@ -57,6 +57,15 @@ private function createOfflineAdmin(): User * @param array $entries File name => content */ private function uploadOfflineArchive(User $user, array $entries): TestResponse + { + return $this->uploadOfflineArchiveFile($user, $this->createOfflineArchive($entries)); + } + + /** + * @param array $entries File name => content + * @return string Path of the archive + */ + private function createOfflineArchive(array $entries): string { $zipPath = tempnam(sys_get_temp_dir(), 'offline'); $zip = new ZipArchive(); @@ -66,6 +75,11 @@ private function uploadOfflineArchive(User $user, array $entries): TestResponse } $zip->close(); + return $zipPath; + } + + private function uploadOfflineArchiveFile(User $user, string $zipPath): TestResponse + { return $this->actingAs($this->tokens[$user->id] ?? $user)->post( 'offline-sync/upload-screenshots', ['file' => new UploadedFile($zipPath, 'screenshots.cattr', 'application/zip', null, true)], From 5f61a5b700e8c201c73d4055d82d17da563651d4 Mon Sep 17 00:00:00 2001 From: Alex Date: Mon, 26 Oct 2026 13:35:00 +0100 Subject: [PATCH 4/7] fix(offline-sync): reject '.' entries and fail on short writes while extracting '.' passed the plain-name check and fopen() on the directory turned into a 500 instead of 422; it is now rejected as a wrong file name. A failed fopen() or a short fwrite() (e.g. a full disk) now aborts the extraction instead of leaving a truncated screenshot to be attached. Refs #422 --- app/Http/Controllers/Api/IntervalController.php | 12 ++++++++++-- .../OfflineScreenshotsArchiveLimitsTest.php | 1 + .../TimeIntervals/UploadsOfflineScreenshots.php | 4 +++- 3 files changed, 14 insertions(+), 3 deletions(-) diff --git a/app/Http/Controllers/Api/IntervalController.php b/app/Http/Controllers/Api/IntervalController.php index 1edc96e5d..98fa44b5f 100644 --- a/app/Http/Controllers/Api/IntervalController.php +++ b/app/Http/Controllers/Api/IntervalController.php @@ -40,6 +40,7 @@ use Storage; use Str; use Symfony\Component\HttpFoundation\StreamedResponse; +use RuntimeException; use Throwable; use Validator; use ZipArchive; @@ -896,6 +897,9 @@ private function extractOfflineScreenshotsArchive(ZipArchive $zip, string $direc try { $target = fopen($directory . DIRECTORY_SEPARATOR . $name, 'wb'); + if ($target === false) { + throw new RuntimeException("Unable to create the extracted file for archive entry $index"); + } try { while (!feof($source)) { // Undecodable compressed data is reported as a warning and a failed read @@ -906,7 +910,10 @@ private function extractOfflineScreenshotsArchive(ZipArchive $zip, string $direc throw self::offlineScreenshotsArchiveTooLarge($maxSize); } - fwrite($target, $chunk); + // A short write (e.g. a full disk) must not leave a truncated screenshot behind + if (fwrite($target, $chunk) !== strlen($chunk)) { + throw new RuntimeException("Unable to write the extracted file for archive entry $index"); + } $remaining -= strlen($chunk); } } finally { @@ -928,11 +935,12 @@ private static function offlineScreenshotsArchiveTooLarge(int $maxSize): Validat } /** - * A plain file name: no directories, no parent references, no absolute or drive paths and no NUL bytes. + * A plain file name: not `.`, no directories, no parent references, no absolute or drive paths and no NUL bytes. */ private static function isPlainArchiveEntryName(string $name): bool { return $name !== '' + && $name !== '.' && strpbrk($name, "/\\\0") === false && !str_contains($name, '..') && preg_match('/^[A-Za-z]:/', $name) !== 1; diff --git a/tests/Feature/TimeIntervals/OfflineScreenshotsArchiveLimitsTest.php b/tests/Feature/TimeIntervals/OfflineScreenshotsArchiveLimitsTest.php index 542fe3db4..73bb75176 100644 --- a/tests/Feature/TimeIntervals/OfflineScreenshotsArchiveLimitsTest.php +++ b/tests/Feature/TimeIntervals/OfflineScreenshotsArchiveLimitsTest.php @@ -173,6 +173,7 @@ public static function unsafeEntryNames(): array 'windows absolute path' => ['C:\\evil.jpg'], 'windows drive-relative path' => ['C:evil.jpg'], 'directory' => ['screenshots/1_00000000-0000-0000-0000-000000000000'], + 'current directory' => ['.'], ]; } diff --git a/tests/Feature/TimeIntervals/UploadsOfflineScreenshots.php b/tests/Feature/TimeIntervals/UploadsOfflineScreenshots.php index ae1a7fd8b..9fd7004be 100644 --- a/tests/Feature/TimeIntervals/UploadsOfflineScreenshots.php +++ b/tests/Feature/TimeIntervals/UploadsOfflineScreenshots.php @@ -47,7 +47,9 @@ private function createUserWithId(int $id): User private function createOfflineAdmin(): User { - $user = User::forceCreate(['role_id' => UserFactoryInstance::ADMIN_ROLE] + UserFactory::createRandomModelData()); + $user = User::forceCreate( + ['role_id' => UserFactoryInstance::ADMIN_ROLE] + UserFactory::createRandomModelData() + ); $this->tokens[$user->id] = $user->createToken('testing-token', ['*'], now()->addDay())->plainTextToken; return $user; From 0f983755598f8c727ba773f5158767f00961ca3a Mon Sep 17 00:00:00 2001 From: Alex Date: Mon, 26 Oct 2026 13:48:53 +0100 Subject: [PATCH 5/7] fix(offline-sync): 150 screenshots by default and readable size limit - Default OFFLINE_SCREENSHOTS_MAX_FILES to 150 (~47 s on S3 at ~310 ms per screenshot), leaving headroom below the 90 s request limit for extraction, queries and the response. - Show the size limit with Number::fileSize() ("256 KB", "1 GB") instead of rounding to whole megabytes. - Test the default and environment-overridden limits independently of the environment the tests run in. Refs #422 --- .env.example | 5 +- .../Controllers/Api/IntervalController.php | 3 +- config/app.php | 6 +- resources/lang/en/validation.php | 2 +- resources/lang/ru/validation.php | 2 +- .../OfflineScreenshotsArchiveLimitsTest.php | 75 ++++++++++++++++++- 6 files changed, 81 insertions(+), 12 deletions(-) diff --git a/.env.example b/.env.example index 01f90b2ae..3a1a8385e 100644 --- a/.env.example +++ b/.env.example @@ -66,8 +66,9 @@ SCREENSHOTS_STATE=any # Offline screenshot archives are processed within the upload request (Octane max_execution_time: 90 s) and # rejected before extraction above these limits: number of screenshots and total uncompressed size in bytes. -# The default of 250 fits both the local disk (~75 ms per screenshot) and S3 (up to ~310 ms). -# OFFLINE_SCREENSHOTS_MAX_FILES=250 +# The default of 150 leaves about half of the time limit as headroom on S3 (up to ~310 ms per screenshot); +# on the local disk (~75 ms) it can be raised. +# OFFLINE_SCREENSHOTS_MAX_FILES=150 # OFFLINE_SCREENSHOTS_MAX_SIZE_BYTES=1073741824 # Octane (Swoole) worker counts; unset = Swoole default (one per host CPU) diff --git a/app/Http/Controllers/Api/IntervalController.php b/app/Http/Controllers/Api/IntervalController.php index 98fa44b5f..269b2021b 100644 --- a/app/Http/Controllers/Api/IntervalController.php +++ b/app/Http/Controllers/Api/IntervalController.php @@ -32,6 +32,7 @@ use Illuminate\Http\Request; use Illuminate\Http\UploadedFile; use Illuminate\Support\Arr; +use Illuminate\Support\Number; use Illuminate\Validation\ValidationException; use MessagePack\MessagePack; use phpseclib4\Crypt\RSA; @@ -929,7 +930,7 @@ private static function offlineScreenshotsArchiveTooLarge(int $maxSize): Validat { return ValidationException::withMessages([ 'file' => __('validation.offline-sync.too_large', [ - 'max' => round($maxSize / 1024 / 1024), + 'max' => Number::fileSize($maxSize, maxPrecision: 2), ]), ]); } diff --git a/config/app.php b/config/app.php index 266566d50..95e1120ee 100644 --- a/config/app.php +++ b/config/app.php @@ -40,10 +40,10 @@ 'screenshots_rate_limit' => (int) env('SCREENSHOTS_RATE_LIMIT', 600), // Offline screenshots (offline-sync/upload-screenshots) are processed within the upload request, which must // finish within Octane's max_execution_time (90 s). Larger archives are rejected before anything is extracted. - // 250 entries ≈ 2.5 working days of 96 screenshots: ~20 s at ~75 ms per 1920×1080 screenshot on the local disk, - // ~80 s at ~310 ms on S3. + // 150 entries ≈ 1.5 working days of 96 screenshots: ~11 s at ~75 ms per 1920×1080 screenshot on the local disk, + // ~47 s at ~310 ms on S3, leaving headroom for extraction, queries and the response. 'offline_screenshots' => [ - 'max_files' => (int) env('OFFLINE_SCREENSHOTS_MAX_FILES', 250), + 'max_files' => (int) env('OFFLINE_SCREENSHOTS_MAX_FILES', 150), // Total uncompressed size of all archive entries 'max_size' => (int) env('OFFLINE_SCREENSHOTS_MAX_SIZE_BYTES', 1024 * 1024 * 1024), ], diff --git a/resources/lang/en/validation.php b/resources/lang/en/validation.php index 5ca910efc..2954318e8 100644 --- a/resources/lang/en/validation.php +++ b/resources/lang/en/validation.php @@ -116,7 +116,7 @@ 'time_interval_added' => 'Added', 'wrong_extension' => 'File extension should be .cattr', 'too_many_files' => 'The archive contains too many screenshots (:count), the maximum is :max', - 'too_large' => 'The unpacked screenshots are too large, the maximum is :max MB', + 'too_large' => 'The unpacked screenshots are too large, the maximum is :max', 'wrong_file_name' => 'The archive must contain only screenshot files without folders' ], diff --git a/resources/lang/ru/validation.php b/resources/lang/ru/validation.php index bff7c6924..39890a490 100644 --- a/resources/lang/ru/validation.php +++ b/resources/lang/ru/validation.php @@ -151,7 +151,7 @@ 'time_interval_added' => 'Добавлен', 'wrong_extension' => 'Расширение файла должно быть .cattr', 'too_many_files' => 'Архив содержит слишком много скриншотов (:count), максимум — :max', - 'too_large' => 'Распакованные скриншоты слишком большие, максимум — :max МБ', + 'too_large' => 'Распакованные скриншоты слишком большие, максимум — :max', 'wrong_file_name' => 'Архив должен содержать только файлы скриншотов без папок' ], diff --git a/tests/Feature/TimeIntervals/OfflineScreenshotsArchiveLimitsTest.php b/tests/Feature/TimeIntervals/OfflineScreenshotsArchiveLimitsTest.php index 73bb75176..71f0f4ac7 100644 --- a/tests/Feature/TimeIntervals/OfflineScreenshotsArchiveLimitsTest.php +++ b/tests/Feature/TimeIntervals/OfflineScreenshotsArchiveLimitsTest.php @@ -48,8 +48,75 @@ protected function setUp(): void public function test_default_limits(): void { - $this->assertSame(250, config('app.offline_screenshots.max_files')); - $this->assertSame(1024 * 1024 * 1024, config('app.offline_screenshots.max_size')); + $limits = $this->offlineScreenshotsConfig([])['offline_screenshots']; + + $this->assertSame(150, $limits['max_files']); + $this->assertSame(1024 * 1024 * 1024, $limits['max_size']); + } + + public function test_limits_can_be_overridden_by_the_environment(): void + { + $limits = $this->offlineScreenshotsConfig([ + 'OFFLINE_SCREENSHOTS_MAX_FILES' => '42', + 'OFFLINE_SCREENSHOTS_MAX_SIZE_BYTES' => '1048576', + ])['offline_screenshots']; + + $this->assertSame(42, $limits['max_files']); + $this->assertSame(1024 * 1024, $limits['max_size']); + } + + public function test_size_limit_message_keeps_fractional_units(): void + { + config(['app.offline_screenshots.max_size' => 256 * 1024]); + + $entries = $this->validEntry(); + $entries["{$this->user->id}_" . Str::uuid()] = str_repeat("\0", 512 * 1024); + + $response = $this->uploadOfflineArchive($this->user, $entries); + + $this->assertRejected($response, __('validation.offline-sync.too_large', ['max' => '256 KB'])); + } + + /** + * Evaluates config/app.php with exactly the given OFFLINE_SCREENSHOTS_* variables set, independent of the + * environment the tests run in. + * + * @param array $values + */ + private function offlineScreenshotsConfig(array $values): array + { + $names = ['OFFLINE_SCREENSHOTS_MAX_FILES', 'OFFLINE_SCREENSHOTS_MAX_SIZE_BYTES']; + $saved = []; + foreach ($names as $name) { + $saved[$name] = [$_ENV[$name] ?? null, $_SERVER[$name] ?? null, getenv($name)]; + } + + try { + foreach ($names as $name) { + unset($_ENV[$name], $_SERVER[$name]); + putenv($name); + if (isset($values[$name])) { + $_ENV[$name] = $_SERVER[$name] = $values[$name]; + putenv("$name=$values[$name]"); + } + } + + return require config_path('app.php'); + } finally { + foreach ($saved as $name => [$env, $server, $getenv]) { + unset($_ENV[$name], $_SERVER[$name]); + putenv($name); + if ($env !== null) { + $_ENV[$name] = $env; + } + if ($server !== null) { + $_SERVER[$name] = $server; + } + if ($getenv !== false) { + putenv("$name=$getenv"); + } + } + } } public function test_archive_within_the_limits_is_attached(): void @@ -98,7 +165,7 @@ public function test_archive_too_large_when_unpacked_is_rejected(): void $response = $this->uploadOfflineArchive($this->user, $entries); - $this->assertRejected($response, __('validation.offline-sync.too_large', ['max' => 1])); + $this->assertRejected($response, __('validation.offline-sync.too_large', ['max' => '1 MB'])); } public function test_archive_exactly_at_the_size_limit_is_attached(): void @@ -138,7 +205,7 @@ public function test_archive_with_forged_entry_sizes_is_rejected_while_extractin // Without the limit on extraction the 3 MiB file is written and handed over to the screenshot service $this->assertSame([], $extractedFileSizes->all(), 'Sizes of the extracted files when attaching'); - $this->assertRejected($response, __('validation.offline-sync.too_large', ['max' => 1])); + $this->assertRejected($response, __('validation.offline-sync.too_large', ['max' => '1 MB'])); } /** From 207ae8211b62e8fb4d1e1536b5f807e43aaafa43 Mon Sep 17 00:00:00 2001 From: Alex Date: Mon, 26 Oct 2026 15:39:30 +0100 Subject: [PATCH 6/7] build(deps): declare ext-intl The size-limit message uses Illuminate\Support\Number, which needs intl. The runtime image already ships it; declaring it makes installs without intl fail at composer install instead of at runtime. Refs #422 --- composer.json | 1 + composer.lock | 3 ++- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/composer.json b/composer.json index 90220fc26..41ff0d16c 100644 --- a/composer.json +++ b/composer.json @@ -15,6 +15,7 @@ "require": { "php": "^8.3", "ext-gd": "*", + "ext-intl": "*", "ext-json": "*", "ext-openssl": "*", "ext-pdo": "*", diff --git a/composer.lock b/composer.lock index 12d2c0dcf..66602eef1 100644 --- a/composer.lock +++ b/composer.lock @@ -4,7 +4,7 @@ "Read more about it at https://getcomposer.org/doc/01-basic-usage.md#installing-dependencies", "This file is @generated automatically" ], - "content-hash": "13ca53e9d9bd13a0313293294940c9fc", + "content-hash": "316b99775abe20c1f754573a3b691db0", "packages": [ { "name": "aws/aws-crt-php", @@ -13552,6 +13552,7 @@ "platform": { "php": "^8.3", "ext-gd": "*", + "ext-intl": "*", "ext-json": "*", "ext-openssl": "*", "ext-pdo": "*", From c1110de6cb006074161f59f735fd9c9b87526f58 Mon Sep 17 00:00:00 2001 From: Alex Date: Mon, 26 Oct 2026 15:39:31 +0100 Subject: [PATCH 7/7] fix(offline-sync): reject archive entries shorter than declared A damaged entry can end early without a read error, so a truncated screenshot was attached. Compare the bytes extracted per entry with the size in the zip directory and reject the archive with 400 on a mismatch. Refs #422 --- .../Controllers/Api/IntervalController.php | 8 ++++++- .../OfflineScreenshotsArchiveLimitsTest.php | 22 +++++++++++++++++++ 2 files changed, 29 insertions(+), 1 deletion(-) diff --git a/app/Http/Controllers/Api/IntervalController.php b/app/Http/Controllers/Api/IntervalController.php index 269b2021b..d584069c7 100644 --- a/app/Http/Controllers/Api/IntervalController.php +++ b/app/Http/Controllers/Api/IntervalController.php @@ -893,8 +893,10 @@ private function extractOfflineScreenshotsArchive(ZipArchive $zip, string $direc for ($index = 0; $index < $fileCount; $index++) { // The names were checked to be plain file names by validateOfflineScreenshotsArchive() $name = $zip->getNameIndex($index); - $source = $name === false ? false : $zip->getStreamIndex($index); + $stat = $zip->statIndex($index); + $source = $name === false || $stat === false ? false : $zip->getStreamIndex($index); abort_if($source === false, 400, __('Cannot open file.')); + $written = 0; try { $target = fopen($directory . DIRECTORY_SEPARATOR . $name, 'wb'); @@ -916,10 +918,14 @@ private function extractOfflineScreenshotsArchive(ZipArchive $zip, string $direc throw new RuntimeException("Unable to write the extracted file for archive entry $index"); } $remaining -= strlen($chunk); + $written += strlen($chunk); } } finally { fclose($target); } + + // A damaged entry can end early without a read error; never pass on a truncated screenshot + abort_if($written !== $stat['size'], 400, __('Cannot open file.')); } finally { fclose($source); } diff --git a/tests/Feature/TimeIntervals/OfflineScreenshotsArchiveLimitsTest.php b/tests/Feature/TimeIntervals/OfflineScreenshotsArchiveLimitsTest.php index 71f0f4ac7..197aee127 100644 --- a/tests/Feature/TimeIntervals/OfflineScreenshotsArchiveLimitsTest.php +++ b/tests/Feature/TimeIntervals/OfflineScreenshotsArchiveLimitsTest.php @@ -208,6 +208,28 @@ public function test_archive_with_forged_entry_sizes_is_rejected_while_extractin $this->assertRejected($response, __('validation.offline-sync.too_large', ['max' => '1 MB'])); } + /** + * An entry that ends before the size declared in the zip headers (e.g. after a CRC error) must not be attached + * as a truncated screenshot. + */ + public function test_archive_with_an_entry_shorter_than_declared_is_rejected(): void + { + $extractedFileSizes = $this->recordExtractedFileSizesOnAttach(); + $name = "{$this->user->id}_$this->screenshotId"; + $image = $this->image(); + $zipPath = $this->createOfflineArchive([$name => $image]); + $this->forgeArchiveEntrySize($zipPath, $name, strlen($image) + 1000); + + $response = $this->uploadOfflineArchiveFile($this->user, $zipPath); + + $response->assertStatus(self::HTTP_BAD_REQUEST); + $this->assertStringContainsString(__('Cannot open file.'), $response->getContent()); + $this->assertSame([], $extractedFileSizes->all(), 'Sizes of the extracted files when attaching'); + $this->interval->refresh(); + $this->assertSame($this->screenshotId, $this->interval->screenshot_id, 'The screenshot was attached'); + $this->assertTmpIsEmpty(); + } + /** * ZipArchive::extractTo() wrote such an entry as an empty file without reporting an error. */