-
-
Notifications
You must be signed in to change notification settings - Fork 786
Add realpath error handling, temp cleanup on fatal errors, credentials sanitization #1922
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,47 @@ | ||
| <?php | ||
|
|
||
| namespace Spatie\Backup\Helpers; | ||
|
|
||
| class CredentialSanitizer | ||
| { | ||
| /** | ||
| * Sanitize exception messages to remove sensitive credentials. | ||
| */ | ||
| public static function sanitizeMessage(string $message): string | ||
| { | ||
| // Pattern to match common credential formats in connection strings and URLs | ||
| $patterns = [ | ||
| // MySQL/PostgreSQL connection strings: password=secret or password='secret' or password="secret" | ||
| '/password\s*=\s*["\']?[^"\'\s;]+["\']?/i' => 'password=***', | ||
| // URLs with credentials: user:password@host | ||
| '/:([^:@\s]+)@/i' => ':***@', | ||
| // Environment variable patterns | ||
| '/DB_PASSWORD\s*=\s*.+/i' => 'DB_PASSWORD=***', | ||
| ]; | ||
|
|
||
| foreach ($patterns as $pattern => $replacement) { | ||
| $message = preg_replace($pattern, $replacement, $message); | ||
| } | ||
|
|
||
| return $message; | ||
| } | ||
|
|
||
| /** | ||
| * Sanitize exception object by replacing its message with a sanitized version. | ||
| */ | ||
| public static function sanitizeException(\Throwable $exception): string | ||
| { | ||
| $sanitizedMessage = self::sanitizeMessage($exception->getMessage()); | ||
| $trace = self::sanitizeStackTrace($exception->getTraceAsString()); | ||
|
|
||
| return $sanitizedMessage . PHP_EOL . $trace; | ||
| } | ||
|
|
||
| /** | ||
| * Sanitize stack trace to remove credentials. | ||
| */ | ||
| protected static function sanitizeStackTrace(string $trace): string | ||
| { | ||
| return self::sanitizeMessage($trace); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,6 +16,8 @@ | |
| use Spatie\Backup\Events\DumpingDatabase; | ||
| use Spatie\Backup\Exceptions\BackupFailed; | ||
| use Spatie\Backup\Exceptions\InvalidBackupJob; | ||
| use Spatie\Backup\Helpers\CredentialSanitizer; | ||
| use Spatie\DbDumper\Compressors\GzipCompressor; | ||
| use Spatie\DbDumper\Databases\MongoDb; | ||
| use Spatie\DbDumper\Databases\Sqlite; | ||
| use Spatie\DbDumper\DbDumper; | ||
|
|
@@ -155,6 +157,14 @@ public function run(): void | |
| ->force() | ||
| ->create() | ||
| ->empty(); | ||
| $cleanupRegistered = false; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Remove the added lines |
||
| $shutdownHandler = function () use (&$cleanupRegistered) { | ||
| if ($cleanupRegistered && $this->temporaryDirectory->exists()) { | ||
| $this->temporaryDirectory->delete(); | ||
| } | ||
| }; | ||
| register_shutdown_function($shutdownHandler); | ||
| $cleanupRegistered = true; | ||
|
|
||
| if ($this->signals) { | ||
| Signal::handle(SIGINT, function (Command $command) { | ||
|
|
@@ -179,14 +189,16 @@ public function run(): void | |
|
|
||
| $this->copyToBackupDestinations($zipFile); | ||
| } catch (Exception $exception) { | ||
| consoleOutput()->error("Backup failed because: {$exception->getMessage()}.".PHP_EOL.$exception->getTraceAsString()); | ||
| $sanitizedError = CredentialSanitizer::sanitizeException($exception); | ||
|
||
| consoleOutput()->error("Backup failed because: {$sanitizedError}"); | ||
|
|
||
| $this->temporaryDirectory->delete(); | ||
|
|
||
| throw BackupFailed::from($exception); | ||
| } | ||
|
|
||
| $this->temporaryDirectory->delete(); | ||
| $cleanupRegistered = false; // Prevent double cleanup | ||
|
|
||
| if ($this->signals) { | ||
| Signal::clearHandlers(SIGINT); | ||
|
|
@@ -302,7 +314,8 @@ protected function copyToBackupDestinations(string $path): void | |
| ->each(function (BackupDestination $backupDestination) use ($path) { | ||
| try { | ||
| if (! $backupDestination->isReachable()) { | ||
| throw new Exception("Could not connect to disk {$backupDestination->diskName()} because: {$backupDestination->connectionError()}"); | ||
| $sanitizedError = CredentialSanitizer::sanitizeMessage($backupDestination->connectionError()); | ||
|
||
| throw new Exception("Could not connect to disk {$backupDestination->diskName()} because: {$sanitizedError}"); | ||
| } | ||
|
|
||
| consoleOutput()->info("Copying zip to disk named {$backupDestination->diskName()}..."); | ||
|
|
@@ -313,7 +326,8 @@ protected function copyToBackupDestinations(string $path): void | |
|
|
||
| $this->sendNotification(new BackupWasSuccessful($backupDestination)); | ||
| } catch (Exception $exception) { | ||
| consoleOutput()->error("Copying zip failed because: {$exception->getMessage()}."); | ||
| $sanitizedError = CredentialSanitizer::sanitizeMessage($exception->getMessage()); | ||
|
||
| consoleOutput()->error("Copying zip failed because: {$sanitizedError}"); | ||
|
|
||
| throw BackupFailed::from($exception)->destination($backupDestination); | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -58,13 +58,17 @@ public static function createFromConnection(string $dbConnectionName): DbDumper | |
| } | ||
|
|
||
| if (isset($dbConfig['port'])) { | ||
| if (filter_var($dbConfig['port'], FILTER_VALIDATE_INT, [ | ||
| $port = $dbConfig['port']; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Remove this. It'll fail elsewhere if the port is not correct. |
||
| if ($port === '' || $port === null) { | ||
| } elseif (filter_var($port, FILTER_VALIDATE_INT, [ | ||
| 'options' => [ | ||
| 'min_range' => 1, | ||
| 'max_range' => 65535, | ||
| ], | ||
| ]) !== false) { | ||
| $dbDumper = $dbDumper->setPort((int) $dbConfig['port']); | ||
| $dbDumper = $dbDumper->setPort((int) $port); | ||
| } else { | ||
| consoleOutput()->warn("Invalid port value '{$port}' for database connection '{$dbConnectionName}'. Using default port."); | ||
| } | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -111,14 +111,20 @@ protected function includedDirectories(): array | |
|
|
||
| protected function shouldExclude(string $path): bool | ||
| { | ||
| $path = realpath($path); | ||
| if (is_dir($path)) { | ||
| $path .= DIRECTORY_SEPARATOR; | ||
| $realPath = realpath($path); | ||
|
|
||
| if ($realPath === false) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm fine with excluded paths being non-existent. Remove all changes to this function. |
||
| consoleOutput()->warn("Cannot resolve path: {$path}. Skipping..."); | ||
| return false; | ||
| } | ||
|
|
||
| if (is_dir($realPath)) { | ||
| $realPath .= DIRECTORY_SEPARATOR; | ||
| } | ||
|
|
||
| foreach ($this->excludeFilesAndDirectories as $excludedPath) { | ||
| if (Str::startsWith($path, $excludedPath.(is_dir($excludedPath) ? DIRECTORY_SEPARATOR : ''))) { | ||
| if ($path != $excludedPath && is_file($excludedPath)) { | ||
| if (Str::startsWith($realPath, $excludedPath.(is_dir($excludedPath) ? DIRECTORY_SEPARATOR : ''))) { | ||
| if ($realPath != $excludedPath && is_file($excludedPath)) { | ||
| continue; | ||
| } | ||
|
|
||
|
|
@@ -137,7 +143,13 @@ protected function sanitize(string|array $paths): Collection | |
| return collect($paths) | ||
| ->reject(fn (string $path) => $path === '') | ||
| ->flatMap(fn (string $path) => $this->getMatchingPaths($path)) | ||
| ->map(fn (string $path) => realpath($path)) | ||
| ->map(function (string $path) { | ||
| $realPath = realpath($path); | ||
| if ($realPath === false) { | ||
| consoleOutput()->warn("Cannot resolve path: {$path}. This path will be excluded from backup."); | ||
| } | ||
| return $realPath; | ||
| }) | ||
| ->reject(fn ($path) => $path === false); | ||
| } | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this class