Skip to content
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions .env.example
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,13 @@ 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.
# 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)
# OCTANE_WORKERS=
# OCTANE_TASK_WORKERS=
Expand Down
121 changes: 120 additions & 1 deletion app/Http/Controllers/Api/IntervalController.php
Original file line number Diff line number Diff line change
Expand Up @@ -32,13 +32,16 @@
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;
use Settings;
use Spatie\TemporaryDirectory\TemporaryDirectory;
use Storage;
use Str;
use Symfony\Component\HttpFoundation\StreamedResponse;
use RuntimeException;
use Throwable;
use Validator;
use ZipArchive;
Expand All @@ -47,6 +50,8 @@
{
protected const MODEL = TimeInterval::class;

private const OFFLINE_SCREENSHOTS_EXTRACTION_CHUNK_SIZE = 64 * 1024;

public function __construct(protected ScreenshotService $screenshotService)
{
}
Expand Down Expand Up @@ -807,14 +812,17 @@
__('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.
$temporaryDirectory = (new TemporaryDirectory())
->location(Storage::disk('local')->path('tmp'))->force()->create();

try {
$zip->extractTo($temporaryDirectory->path());
$this->extractOfflineScreenshotsArchive($zip, $temporaryDirectory->path());
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Resolved
$zip->close();

$creationResult = $this->attachOfflineScreenshots(
Expand All @@ -828,6 +836,117 @@
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 `<user_id>_<screenshot_id>` 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 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');
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
$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);
}

// 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 {
fclose($target);
}
} finally {
fclose($source);
}
}
}

private static function offlineScreenshotsArchiveTooLarge(int $maxSize): ValidationException
{
return ValidationException::withMessages([
'file' => __('validation.offline-sync.too_large', [
'max' => Number::fileSize($maxSize, maxPrecision: 2),
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Resolved
]),
]);
}

/**
* 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;
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Resolved
}

/**
* Saves the extracted offline screenshots synchronously, so that failures are reported in the result.
*
Expand Down Expand Up @@ -963,7 +1082,7 @@

CatEvent::dispatch(Filter::getAfterActionEventName(), [$timeIntervals, $filters]);

$totalTime = $timeIntervals->sum(static fn($el) => (int) Carbon::parse($el->end_at)->diffInSeconds($el->start_at, true));

Check warning on line 1085 in app/Http/Controllers/Api/IntervalController.php

View workflow job for this annotation

GitHub Actions / PHP_CodeSniffer

Line exceeds 120 characters; contains 129 characters

return responder()->success([
'time' => $totalTime,
Expand Down
9 changes: 9 additions & 0 deletions config/app.php
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,15 @@
// 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.
// 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', 150),
// 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,
Expand Down
5 changes: 4 additions & 1 deletion resources/lang/en/validation.php
Original file line number Diff line number Diff line change
Expand Up @@ -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',
'wrong_file_name' => 'The archive must contain only screenshot files without folders'
],

'tasks-relations' => [
Expand Down
5 changes: 4 additions & 1 deletion resources/lang/ru/validation.php
Original file line number Diff line number Diff line change
Expand Up @@ -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' => [
Expand Down
Loading
Loading