Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
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
47 changes: 47 additions & 0 deletions src/Helpers/CredentialSanitizer.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
<?php

namespace Spatie\Backup\Helpers;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this class


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);
}
}
20 changes: 17 additions & 3 deletions src/Tasks/Backup/BackupJob.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -155,6 +157,14 @@ public function run(): void
->force()
->create()
->empty();
$cleanupRegistered = false;
Copy link
Member

Choose a reason for hiding this comment

The 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) {
Expand All @@ -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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this, it think just throwing the exception is fine.

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);
Expand Down Expand Up @@ -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());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm ok with throwing an exception here. You don't need to sanitize the message.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do refactor the exception to how it's done for the other exceptions. See BackupFailed

throw new Exception("Could not connect to disk {$backupDestination->diskName()} because: {$sanitizedError}");
}

consoleOutput()->info("Copying zip to disk named {$backupDestination->diskName()}...");
Expand All @@ -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());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this, I think the current exception is good enough

consoleOutput()->error("Copying zip failed because: {$sanitizedError}");

throw BackupFailed::from($exception)->destination($backupDestination);
}
Expand Down
8 changes: 6 additions & 2 deletions src/Tasks/Backup/DbDumperFactory.php
Original file line number Diff line number Diff line change
Expand Up @@ -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'];
Copy link
Member

Choose a reason for hiding this comment

The 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.");
}
}

Expand Down
24 changes: 18 additions & 6 deletions src/Tasks/Backup/FileSelection.php
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Member

Choose a reason for hiding this comment

The 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;
}

Expand All @@ -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);
}

Expand Down