Skip to content
Merged
Show file tree
Hide file tree
Changes from all 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
127 changes: 126 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.
$zip->close();

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

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.
]),
]);
}

/**
* 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.
}

/**
* Saves the extracted offline screenshots synchronously, so that failures are reported in the result.
*
Expand Down Expand Up @@ -963,7 +1088,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 1091 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
1 change: 1 addition & 0 deletions composer.json
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
"require": {
"php": "^8.3",
"ext-gd": "*",
"ext-intl": "*",
"ext-json": "*",
"ext-openssl": "*",
"ext-pdo": "*",
Expand Down
3 changes: 2 additions & 1 deletion composer.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

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