diff --git a/app/Jobs/ScheduledTaskJob.php b/app/Jobs/ScheduledTaskJob.php index d24f36350b..f7bd5f933d 100644 --- a/app/Jobs/ScheduledTaskJob.php +++ b/app/Jobs/ScheduledTaskJob.php @@ -211,8 +211,9 @@ class ScheduledTaskJob implements ShouldBeEncrypted, ShouldQueue private function boundedTaskCommand(string $command): string { $maxOutputBytes = self::MAX_OUTPUT_SIZE_BYTES; + $readLimit = $maxOutputBytes + 1; - return "output_file=\$(mktemp); trap 'rm -f \"\$output_file\"' EXIT; set +e; set -o pipefail; {$command} 2>&1 | { head -c {$maxOutputBytes} > \"\$output_file\"; if IFS= read -r -n 1 extra_byte; then printf '\n\n[... Output truncated at 5MB limit ...]' >> \"\$output_file\"; fi; cat > /dev/null; }; exit_code=\${PIPESTATUS[0]}; if [ \"\$exit_code\" -eq 0 ]; then cat \"\$output_file\"; else cat \"\$output_file\" >&2; fi; exit \$exit_code"; + return "output_file=\$(mktemp); trap 'rm -f \"\$output_file\"' EXIT; set +e; set -o pipefail; {$command} 2>&1 | { head -c {$readLimit} > \"\$output_file\"; cat > /dev/null; }; exit_code=\${PIPESTATUS[0]}; if [ \"\$(wc -c < \"\$output_file\")\" -gt {$maxOutputBytes} ]; then truncate -s {$maxOutputBytes} \"\$output_file\"; printf '\n\n[... Output truncated at 5MB limit ...]' >> \"\$output_file\"; fi; if [ \"\$exit_code\" -eq 0 ]; then cat \"\$output_file\"; else cat \"\$output_file\" >&2; fi; exit \$exit_code"; } /** diff --git a/app/Models/LocalFileVolume.php b/app/Models/LocalFileVolume.php index 968e6c3d04..6b0124cb97 100644 --- a/app/Models/LocalFileVolume.php +++ b/app/Models/LocalFileVolume.php @@ -113,9 +113,9 @@ class LocalFileVolume extends BaseModel return; } - $content = instant_remote_process(["cat {$escapedPath}"], $server, false); + $content = $this->readRemoteFileContent($escapedPath, $server); // Check if content contains binary data by looking for null bytes or non-printable characters - if (str_contains($content, "\0") || preg_match('/[\x00-\x08\x0B\x0C\x0E-\x1F]/', $content)) { + if ($content !== self::TOO_LARGE_PLACEHOLDER && (str_contains($content, "\0") || preg_match('/[\x00-\x08\x0B\x0C\x0E-\x1F]/', $content))) { $content = self::BINARY_PLACEHOLDER; } $this->content = $content; @@ -136,6 +136,27 @@ class LocalFileVolume extends BaseModel return $size > self::MAX_CONTENT_SIZE; } + /** + * Cap the remote read itself so a file that grows after the size check + * cannot be fully slurped into PHP memory. + */ + protected function readRemoteFileContent(string $escapedPath, $server): string + { + $readLimit = self::MAX_CONTENT_SIZE + 1; + $content = instant_remote_process(["head -c {$readLimit} {$escapedPath}"], $server, false); + + return self::contentFromBoundedRead($content); + } + + public static function contentFromBoundedRead(?string $content): string + { + if (strlen((string) $content) > self::MAX_CONTENT_SIZE) { + return self::TOO_LARGE_PLACEHOLDER; + } + + return (string) $content; + } + public function deleteStorageOnServer() { if ($this->is_host_file) { @@ -228,7 +249,7 @@ class LocalFileVolume extends BaseModel if ($this->remoteFileExceedsLimit($escapedPath, $server)) { $this->content = self::TOO_LARGE_PLACEHOLDER; } else { - $this->content = instant_remote_process(["cat {$escapedPath}"], $server, false); + $this->content = $this->readRemoteFileContent($escapedPath, $server); } $this->is_directory = false; $this->save(); diff --git a/tests/Unit/LocalFileVolumeContentSizeTest.php b/tests/Unit/LocalFileVolumeContentSizeTest.php index 1954539785..1894bf4394 100644 --- a/tests/Unit/LocalFileVolumeContentSizeTest.php +++ b/tests/Unit/LocalFileVolumeContentSizeTest.php @@ -88,3 +88,38 @@ it('marks git-based file volumes as files before refreshing their content', func '/\$fileVolume->is_directory = false;\s+\$fileVolume->save\(\);\s+if \(\$fileVolume->is_based_on_git\) \{/' ); }); + +it('bounds the remote file read itself to prevent a size-check race', function () { + $source = file_get_contents(app_path('Models/LocalFileVolume.php')); + $loadStorage = str($source) + ->after('public function loadStorageOnServer()') + ->before('public function deleteStorageOnServer()'); + + expect($loadStorage->value()) + ->toContain('head -c') + ->not->toContain('instant_remote_process(["cat {$escapedPath}"]'); +}); + +it('bounds directory-to-file conflict reads the same way', function () { + $source = file_get_contents(app_path('Models/LocalFileVolume.php')); + $saveStorage = str($source) + ->after('public function saveStorageOnServer()') + ->before('protected function plainMountPath'); + + expect($saveStorage->value()) + ->not->toContain('instant_remote_process(["cat {$escapedPath}"]'); +}); + +it('treats a bounded remote read that exceeds the limit as too large', function () { + $oversized = str_repeat('a', LocalFileVolume::MAX_CONTENT_SIZE + 1); + + expect(LocalFileVolume::contentFromBoundedRead($oversized)) + ->toBe(LocalFileVolume::TOO_LARGE_PLACEHOLDER); +}); + +it('keeps a bounded remote read that fits the limit', function () { + expect(LocalFileVolume::contentFromBoundedRead('hello')) + ->toBe('hello') + ->and(LocalFileVolume::contentFromBoundedRead(null)) + ->toBe(''); +}); diff --git a/tests/Unit/RemoteOutputSizeLimitsTest.php b/tests/Unit/RemoteOutputSizeLimitsTest.php index b8341dd73f..3660e88284 100644 --- a/tests/Unit/RemoteOutputSizeLimitsTest.php +++ b/tests/Unit/RemoteOutputSizeLimitsTest.php @@ -58,7 +58,7 @@ it('bounds scheduled task output before storing or notifying', function () { expect($source) ->toContain('MAX_OUTPUT_SIZE_BYTES') - ->toContain('head -c {$maxOutputBytes}') + ->toContain('head -c {$readLimit}') ->toContain('[... Output truncated at'); $reflection = new ReflectionClass(ScheduledTaskJob::class); @@ -70,7 +70,7 @@ it('bounds scheduled task output before storing or notifying', function () { exec('bash -c '.escapeshellarg($failureCommand).' 2>&1', $failureOutput, $failureExitCode); expect(ScheduledTaskJob::MAX_OUTPUT_SIZE_BYTES)->toBe(5 * 1024 * 1024) - ->and($command)->toContain('head -c 5242880') + ->and($command)->toContain('head -c 5242881') ->and($exitCode)->toBe(0) ->and($commandExitCode)->toBe(0) ->and(implode("\n", $commandOutput))->toBe('hello') @@ -78,6 +78,20 @@ it('bounds scheduled task output before storing or notifying', function () { ->and(implode("\n", $failureOutput))->toBe('failure'); }); +it('marks scheduled task output that exceeds the limit as truncated', function () { + $reflection = new ReflectionClass(ScheduledTaskJob::class); + $method = $reflection->getMethod('boundedTaskCommand'); + $largeOutputCommand = 'head -c '.(ScheduledTaskJob::MAX_OUTPUT_SIZE_BYTES + 1).' /dev/zero | tr "\\0" "x"'; + $command = $method->invoke($reflection->newInstanceWithoutConstructor(), $largeOutputCommand); + + exec('bash -c '.escapeshellarg($command), $output, $exitCode); + $taskOutput = implode("\n", $output); + + expect($exitCode)->toBe(0) + ->and(strlen($taskOutput))->toBeGreaterThan(ScheduledTaskJob::MAX_OUTPUT_SIZE_BYTES) + ->and($taskOutput)->toEndWith('[... Output truncated at 5MB limit ...]'); +}); + it('does not pass the scheduled task output wrapper through the sudo rewriter', function () { $source = remoteOutputSource('app/Jobs/ScheduledTaskJob.php'); $reflection = new ReflectionClass(ScheduledTaskJob::class);