Add realpath error handling, temp cleanup on fatal errors, credentials sanitization #1922

imhayatunnabi · 2025-10-15T04:21:21Z

Missing Error Handling for realpath() Failures
Temporary Directory Not Cleaned Up on Certain Failures
Database Connection Error Messages Expose Credentials
Port Validation Accepts Invalid Ports

freekmurze · 2025-10-15T11:58:41Z

could you rebase this with main?

imhayatunnabi · 2025-10-15T12:48:13Z

Is this ok now ? @freekmurze

…ntial sanitization, and robust port validation

imhayatunnabi · 2025-10-15T14:57:10Z

rebase is done. was facing issue with my upstream. now resolved. rebased and updated in the latest commit. @freekmurze

freekmurze

Thank you for this. I think the changes are probably a bit too strict (and the code is a bit too complicated to my eyes).

There is one new exception that you introduced that we could keep, but could you refactor it so it follows the patterns of the other exceptions?

freekmurze · 2025-10-16T07:29:42Z

src/Helpers/CredentialSanitizer.php

@@ -0,0 +1,47 @@
+<?php
+
+namespace Spatie\Backup\Helpers;


Remove this class

freekmurze · 2025-10-16T07:30:11Z

src/Tasks/Backup/BackupJob.php

            ->force()
            ->create()
            ->empty();
+        $cleanupRegistered = false;


Remove the added lines

freekmurze · 2025-10-16T07:30:32Z

src/Tasks/Backup/BackupJob.php

            $this->copyToBackupDestinations($zipFile);
        } catch (Exception $exception) {
-            consoleOutput()->error("Backup failed because: {$exception->getMessage()}.".PHP_EOL.$exception->getTraceAsString());
+            $sanitizedError = CredentialSanitizer::sanitizeException($exception);


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

freekmurze · 2025-10-16T07:31:49Z

src/Tasks/Backup/BackupJob.php

                try {
                    if (! $backupDestination->isReachable()) {
-                        throw new Exception("Could not connect to disk {$backupDestination->diskName()} because: {$backupDestination->connectionError()}");
+                        $sanitizedError = CredentialSanitizer::sanitizeMessage($backupDestination->connectionError());


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

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

freekmurze · 2025-10-16T07:32:18Z

src/Tasks/Backup/BackupJob.php

                    $this->sendNotification(new BackupWasSuccessful($backupDestination));
                } catch (Exception $exception) {
-                    consoleOutput()->error("Copying zip failed because: {$exception->getMessage()}.");
+                    $sanitizedError = CredentialSanitizer::sanitizeMessage($exception->getMessage());


Remove this, I think the current exception is good enough

freekmurze · 2025-10-16T07:33:01Z

src/Tasks/Backup/DbDumperFactory.php


        if (isset($dbConfig['port'])) {
-            if (filter_var($dbConfig['port'], FILTER_VALIDATE_INT, [
+            $port = $dbConfig['port'];


Remove this. It'll fail elsewhere if the port is not correct.

freekmurze · 2025-10-16T07:34:00Z

src/Tasks/Backup/FileSelection.php

-            $path .= DIRECTORY_SEPARATOR;
+        $realPath = realpath($path);
+
+        if ($realPath === false) {


I'm fine with excluded paths being non-existent. Remove all changes to this function.

imhayatunnabi · 2025-10-18T17:06:12Z

@freekmurze any review or update ?

imhayatunnabi · 2025-11-03T20:35:14Z

@freekmurze about that .??

imhayatunnabi force-pushed the fix/teamp-cleanup-credential-sanitisation branch 2 times, most recently from a0c1462 to 5e29175 Compare October 15, 2025 12:41

imhayatunnabi force-pushed the fix/teamp-cleanup-credential-sanitisation branch from 890a51d to d262b49 Compare October 15, 2025 14:50

fix: add realpath error handling, temp cleanup on fatal errors, crede…

c8e2cf4

…ntial sanitization, and robust port validation

imhayatunnabi force-pushed the fix/teamp-cleanup-credential-sanitisation branch from d262b49 to c8e2cf4 Compare October 15, 2025 14:56

freekmurze requested changes Oct 16, 2025

View reviewed changes

feat: introduce CredentialExposure exception

e7f5adc

imhayatunnabi requested a review from freekmurze October 16, 2025 11:33

imhayatunnabi changed the title ~~fix: add realpath error handling, temp cleanup on fatal errors, credentials sanitisation~~ Add realpath error handling, temp cleanup on fatal errors, credentials sanitization Nov 4, 2025

Uh oh!

Add realpath error handling, temp cleanup on fatal errors, credentials sanitization #1922

Are you sure you want to change the base?

Add realpath error handling, temp cleanup on fatal errors, credentials sanitization #1922

Uh oh!

Conversation

imhayatunnabi commented Oct 15, 2025

Uh oh!

freekmurze commented Oct 15, 2025

Uh oh!

imhayatunnabi commented Oct 15, 2025

Uh oh!

imhayatunnabi commented Oct 15, 2025

Uh oh!

freekmurze left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

imhayatunnabi commented Oct 18, 2025

Uh oh!

imhayatunnabi commented Nov 3, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants