mirror of
https://github.com/coollabsio/coolify.git
synced 2025-12-28 05:34:50 +00:00
feat: improve S3 restore path handling and validation state
- Add path attribute mutator to S3Storage model ensuring paths start with / - Add updatedS3Path hook to normalize path and reset validation state on blur - Add updatedS3StorageId hook to reset validation state when storage changes - Add Enter key support to trigger file check from path input - Use wire:model.live for S3 storage select, wire:model.blur for path input - Improve shell escaping in restore job cleanup commands - Fix isSafeTmpPath helper logic for directory validation 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
This commit is contained in:
parent
6d8144c18c
commit
875351188f
@ -22,11 +22,11 @@ class RestoreJobFinished
|
||||
$commands = [];
|
||||
|
||||
if (isSafeTmpPath($scriptPath)) {
|
||||
$commands[] = "docker exec {$container} sh -c 'rm {$scriptPath} 2>/dev/null || true'";
|
||||
$commands[] = "docker exec {$container} sh -c 'rm ".escapeshellarg($scriptPath)." 2>/dev/null || true'";
|
||||
}
|
||||
|
||||
if (isSafeTmpPath($tmpPath)) {
|
||||
$commands[] = "docker exec {$container} sh -c 'rm {$tmpPath} 2>/dev/null || true'";
|
||||
$commands[] = "docker exec {$container} sh -c 'rm ".escapeshellarg($tmpPath)." 2>/dev/null || true'";
|
||||
}
|
||||
|
||||
if (! empty($commands)) {
|
||||
|
||||
@ -27,21 +27,21 @@ class S3RestoreJobFinished
|
||||
|
||||
// Ensure helper container is removed (may already be gone from inline cleanup)
|
||||
if (filled($containerName)) {
|
||||
$commands[] = "docker rm -f {$containerName} 2>/dev/null || true";
|
||||
$commands[] = 'docker rm -f '.escapeshellarg($containerName).' 2>/dev/null || true';
|
||||
}
|
||||
|
||||
// Clean up server temp file if still exists (should already be cleaned)
|
||||
if (isSafeTmpPath($serverTmpPath)) {
|
||||
$commands[] = "rm -f {$serverTmpPath} 2>/dev/null || true";
|
||||
$commands[] = 'rm -f '.escapeshellarg($serverTmpPath).' 2>/dev/null || true';
|
||||
}
|
||||
|
||||
// Clean up any remaining files in database container (may already be cleaned)
|
||||
if (filled($container)) {
|
||||
if (isSafeTmpPath($containerTmpPath)) {
|
||||
$commands[] = "docker exec {$container} rm -f {$containerTmpPath} 2>/dev/null || true";
|
||||
$commands[] = 'docker exec '.escapeshellarg($container).' rm -f '.escapeshellarg($containerTmpPath).' 2>/dev/null || true';
|
||||
}
|
||||
if (isSafeTmpPath($scriptPath)) {
|
||||
$commands[] = "docker exec {$container} rm -f {$scriptPath} 2>/dev/null || true";
|
||||
$commands[] = 'docker exec '.escapeshellarg($container).' rm -f '.escapeshellarg($scriptPath).' 2>/dev/null || true';
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@ -280,6 +280,23 @@ EOD;
|
||||
}
|
||||
}
|
||||
|
||||
public function updatedS3Path($value)
|
||||
{
|
||||
// Reset validation state when path changes
|
||||
$this->s3FileSize = null;
|
||||
|
||||
// Ensure path starts with a slash
|
||||
if ($value !== null && $value !== '') {
|
||||
$this->s3Path = str($value)->trim()->start('/')->value();
|
||||
}
|
||||
}
|
||||
|
||||
public function updatedS3StorageId()
|
||||
{
|
||||
// Reset validation state when storage changes
|
||||
$this->s3FileSize = null;
|
||||
}
|
||||
|
||||
public function checkS3File()
|
||||
{
|
||||
if (! $this->s3StorageId) {
|
||||
|
||||
@ -3,6 +3,7 @@
|
||||
namespace App\Models;
|
||||
|
||||
use App\Traits\HasSafeStringAttribute;
|
||||
use Illuminate\Database\Eloquent\Casts\Attribute;
|
||||
use Illuminate\Database\Eloquent\Factories\HasFactory;
|
||||
use Illuminate\Notifications\Messages\MailMessage;
|
||||
use Illuminate\Support\Facades\Storage;
|
||||
@ -41,6 +42,19 @@ class S3Storage extends BaseModel
|
||||
return "{$this->endpoint}/{$this->bucket}";
|
||||
}
|
||||
|
||||
protected function path(): Attribute
|
||||
{
|
||||
return Attribute::make(
|
||||
set: function (?string $value) {
|
||||
if ($value === null || $value === '') {
|
||||
return null;
|
||||
}
|
||||
|
||||
return str($value)->trim()->start('/')->value();
|
||||
}
|
||||
);
|
||||
}
|
||||
|
||||
public function testConnection(bool $shouldSave = false)
|
||||
{
|
||||
try {
|
||||
|
||||
@ -3250,18 +3250,16 @@ function isSafeTmpPath(?string $path): bool
|
||||
$dirPath = dirname($resolvedPath);
|
||||
|
||||
// If the directory exists, resolve it via realpath to catch symlink attacks
|
||||
if (file_exists($resolvedPath) || is_dir($dirPath)) {
|
||||
if (is_dir($dirPath)) {
|
||||
// For existing paths, resolve to absolute path to catch symlinks
|
||||
if (is_dir($dirPath)) {
|
||||
$realDir = realpath($dirPath);
|
||||
if ($realDir === false) {
|
||||
return false;
|
||||
}
|
||||
$realDir = realpath($dirPath);
|
||||
if ($realDir === false) {
|
||||
return false;
|
||||
}
|
||||
|
||||
// Check if the real directory is within /tmp (or its canonical path)
|
||||
if (! str($realDir)->startsWith('/tmp') && ! str($realDir)->startsWith($canonicalTmpPath)) {
|
||||
return false;
|
||||
}
|
||||
// Check if the real directory is within /tmp (or its canonical path)
|
||||
if (! str($realDir)->startsWith('/tmp') && ! str($realDir)->startsWith($canonicalTmpPath)) {
|
||||
return false;
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@ -173,7 +173,7 @@
|
||||
<div x-show="restoreType === 's3'" class="pt-6">
|
||||
<h3>Restore from S3</h3>
|
||||
<div class="flex flex-col gap-2 pt-2">
|
||||
<x-forms.select label="S3 Storage" wire:model="s3StorageId">
|
||||
<x-forms.select label="S3 Storage" wire:model.live="s3StorageId">
|
||||
<option value="">Select S3 Storage</option>
|
||||
@foreach ($availableS3Storages as $storage)
|
||||
<option value="{{ $storage->id }}">{{ $storage->name }}
|
||||
@ -186,7 +186,8 @@
|
||||
|
||||
<x-forms.input label="S3 File Path (within bucket)"
|
||||
helper="Path to the backup file in your S3 bucket, e.g., /backups/database-2025-01-15.gz"
|
||||
placeholder="/backups/database-backup.gz" wire:model='s3Path'></x-forms.input>
|
||||
placeholder="/backups/database-backup.gz" wire:model.blur='s3Path'
|
||||
wire:keydown.enter='checkS3File'></x-forms.input>
|
||||
|
||||
<div class="flex gap-2">
|
||||
<x-forms.button class="w-full" wire:click='checkS3File' x-bind:disabled="!s3StorageId || !s3Path">
|
||||
|
||||
118
tests/Unit/RestoreJobFinishedShellEscapingTest.php
Normal file
118
tests/Unit/RestoreJobFinishedShellEscapingTest.php
Normal file
@ -0,0 +1,118 @@
|
||||
<?php
|
||||
|
||||
/**
|
||||
* Security tests for shell metacharacter escaping in restore events.
|
||||
*
|
||||
* These tests verify that escapeshellarg() properly neutralizes shell injection
|
||||
* attempts in paths that pass isSafeTmpPath() validation.
|
||||
*/
|
||||
describe('Shell metacharacter escaping in restore events', function () {
|
||||
it('demonstrates that malicious paths can pass isSafeTmpPath but are neutralized by escapeshellarg', function () {
|
||||
// This path passes isSafeTmpPath() validation (it's within /tmp/, no .., no null bytes)
|
||||
$maliciousPath = "/tmp/file'; whoami; '";
|
||||
|
||||
// Path validation passes - it's a valid /tmp/ path
|
||||
expect(isSafeTmpPath($maliciousPath))->toBeTrue();
|
||||
|
||||
// But when escaped, the shell metacharacters become literal strings
|
||||
$escaped = escapeshellarg($maliciousPath);
|
||||
|
||||
// The escaped version wraps in single quotes and escapes internal single quotes
|
||||
expect($escaped)->toBe("'/tmp/file'\\''; whoami; '\\'''");
|
||||
|
||||
// Building a command with escaped path is safe
|
||||
$command = "rm -f {$escaped}";
|
||||
|
||||
// The command contains the quoted path, not an unquoted injection
|
||||
expect($command)->toStartWith("rm -f '");
|
||||
expect($command)->toEndWith("'");
|
||||
});
|
||||
|
||||
it('escapes paths with semicolon injection attempts', function () {
|
||||
$path = '/tmp/backup; rm -rf /; echo';
|
||||
expect(isSafeTmpPath($path))->toBeTrue();
|
||||
|
||||
$escaped = escapeshellarg($path);
|
||||
expect($escaped)->toBe("'/tmp/backup; rm -rf /; echo'");
|
||||
|
||||
// The semicolons are inside quotes, so they're treated as literals
|
||||
$command = "rm -f {$escaped}";
|
||||
expect($command)->toBe("rm -f '/tmp/backup; rm -rf /; echo'");
|
||||
});
|
||||
|
||||
it('escapes paths with backtick command substitution attempts', function () {
|
||||
$path = '/tmp/backup`whoami`.sql';
|
||||
expect(isSafeTmpPath($path))->toBeTrue();
|
||||
|
||||
$escaped = escapeshellarg($path);
|
||||
expect($escaped)->toBe("'/tmp/backup`whoami`.sql'");
|
||||
|
||||
// Backticks inside single quotes are not executed
|
||||
$command = "rm -f {$escaped}";
|
||||
expect($command)->toBe("rm -f '/tmp/backup`whoami`.sql'");
|
||||
});
|
||||
|
||||
it('escapes paths with $() command substitution attempts', function () {
|
||||
$path = '/tmp/backup$(id).sql';
|
||||
expect(isSafeTmpPath($path))->toBeTrue();
|
||||
|
||||
$escaped = escapeshellarg($path);
|
||||
expect($escaped)->toBe("'/tmp/backup\$(id).sql'");
|
||||
|
||||
// $() inside single quotes is not executed
|
||||
$command = "rm -f {$escaped}";
|
||||
expect($command)->toBe("rm -f '/tmp/backup\$(id).sql'");
|
||||
});
|
||||
|
||||
it('escapes paths with pipe injection attempts', function () {
|
||||
$path = '/tmp/backup | cat /etc/passwd';
|
||||
expect(isSafeTmpPath($path))->toBeTrue();
|
||||
|
||||
$escaped = escapeshellarg($path);
|
||||
expect($escaped)->toBe("'/tmp/backup | cat /etc/passwd'");
|
||||
|
||||
// Pipe inside single quotes is treated as literal
|
||||
$command = "rm -f {$escaped}";
|
||||
expect($command)->toBe("rm -f '/tmp/backup | cat /etc/passwd'");
|
||||
});
|
||||
|
||||
it('escapes paths with newline injection attempts', function () {
|
||||
$path = "/tmp/backup\nwhoami";
|
||||
expect(isSafeTmpPath($path))->toBeTrue();
|
||||
|
||||
$escaped = escapeshellarg($path);
|
||||
// Newline is preserved inside single quotes
|
||||
expect($escaped)->toContain("\n");
|
||||
expect($escaped)->toStartWith("'");
|
||||
expect($escaped)->toEndWith("'");
|
||||
});
|
||||
|
||||
it('handles normal paths without issues', function () {
|
||||
$normalPaths = [
|
||||
'/tmp/restore-backup.sql',
|
||||
'/tmp/restore-script.sh',
|
||||
'/tmp/database-dump-abc123.sql',
|
||||
'/tmp/deeply/nested/path/to/file.sql',
|
||||
];
|
||||
|
||||
foreach ($normalPaths as $path) {
|
||||
expect(isSafeTmpPath($path))->toBeTrue();
|
||||
|
||||
$escaped = escapeshellarg($path);
|
||||
// Normal paths are just wrapped in single quotes
|
||||
expect($escaped)->toBe("'{$path}'");
|
||||
}
|
||||
});
|
||||
|
||||
it('escapes container names with injection attempts', function () {
|
||||
// Container names are not validated by isSafeTmpPath, so escaping is critical
|
||||
$maliciousContainer = 'container"; rm -rf /; echo "pwned';
|
||||
$escaped = escapeshellarg($maliciousContainer);
|
||||
|
||||
expect($escaped)->toBe("'container\"; rm -rf /; echo \"pwned'");
|
||||
|
||||
// Building a docker command with escaped container is safe
|
||||
$command = "docker rm -f {$escaped}";
|
||||
expect($command)->toBe("docker rm -f 'container\"; rm -rf /; echo \"pwned'");
|
||||
});
|
||||
});
|
||||
Loading…
Reference in New Issue
Block a user