mirror of
https://github.com/coollabsio/coolify.git
synced 2026-08-24 10:05:47 -05:00
fix: cap remote file and task reads after size-check races
Read local file volumes with a bounded head instead of cat so a file that grows after the size check cannot be fully slurped into PHP memory. Treat oversized bounded reads as too large and skip binary detection on that placeholder. For scheduled tasks, read one extra byte, then truncate and append the 5MB notice instead of relying on a one-byte peek.
This commit is contained in:
@@ -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";
|
||||
}
|
||||
|
||||
/**
|
||||
|
||||
@@ -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();
|
||||
|
||||
@@ -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('');
|
||||
});
|
||||
|
||||
@@ -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);
|
||||
|
||||
Reference in New Issue
Block a user