evolution-cms · elcreator · Sep 9, 2026
diff --git a/core/src/Services/DatabaseBackupService.php b/core/src/Services/DatabaseBackupService.php
@@ -6,6 +6,14 @@
 
 class DatabaseBackupService
 {
+    /**
+     * Apache deny rule for the directories dumps land in. `Order deny,allow` on its own is 2.2
+     * syntax: on 2.4 without mod_access_compat it is a 500, which leaves the directory served
+     * rather than denied, so both forms are written and each is guarded by its module.
+     */
+    public const DENY_HTACCESS = "<IfModule mod_authz_core.c>\n    Require all denied\n</IfModule>\n"
+        . "<IfModule !mod_authz_core.c>\n    Order deny,allow\n    Deny from all\n</IfModule>\n";
+
     protected string $basePath;
 
     public function __construct(?string $basePath = null)
@@ -87,7 +95,7 @@ protected function prepareSnapshotPath($snapshotPath)
 
         $htaccess = $path . '/.htaccess';
         if (!is_file($htaccess)) {
-            file_put_contents($htaccess, "order deny,allow\ndeny from all\n");
+            file_put_contents($htaccess, self::DENY_HTACCESS);
         }
 
         if (!is_writable($path)) {
@@ -164,46 +172,116 @@ protected function createDriverSnapshot($driver, $database, $filePath)
 
     protected function createPostgresSnapshot($database, $filePath)
     {
-        $config = evo()->getDatabase()->getConfig();
-        $password = isset($config['password']) ? (string) $config['password'] : '';
+        $config = $this->databaseConfig();
         $host = isset($config['host']) ? (string) $config['host'] : '';
-        $username = isset($config['username']) ? (string) $config['username'] : '';
         $tempFilePath = $this->buildTempSnapshotFilePath((string) $filePath);
 
         file_put_contents($tempFilePath, $this->buildSqlHeader('--', (string) $database, $host));
 
-        $handle = fopen($tempFilePath, 'ab');
+        if (!$this->runPostgresDump(['--clean', '--inserts', '--no-owner', '--no-privileges'], $tempFilePath)) {
+            if (is_file($tempFilePath)) {
+                unlink($tempFilePath);
+            }
 
-        if ($handle === false) {
+            return false;
+        }
+
+        if (is_file((string) $filePath)) {
+            unlink((string) $filePath);
+        }
+
+        return rename($tempFilePath, (string) $filePath);
+    }
+
+    /**
+     * Dumps the given tables to a file under the snapshot directory and returns its path, or null
+     * when the dump failed. The caller owns the file from there on.
+     *
+     * @since 3.5.8
+     * @param array $tables
+     * @param bool $dropTables
+     * @return string|null
+     */
+    public function dumpPostgresTables(array $tables, $dropTables = true)
+    {
+        $config = $this->databaseConfig();
+        $database = isset($config['database']) ? (string) $config['database'] : '';
+        $host = isset($config['host']) ? (string) $config['host'] : '';
+        $snapshotPath = $this->resolveSnapshotPath();
+
+        $this->prepareSnapshotPath($snapshotPath);
+
+        // Under the snapshot directory rather than the old assets/backup/temp.php: that path sat
+        // in the web root with nothing denying it, so a full dump was readable by anyone who
+        // guessed the name. Here the directory carries a deny rule and the name is not guessable.
+        $tempFilePath = $this->buildTempSnapshotFilePath($snapshotPath . 'download.sql');
+
+        file_put_contents($tempFilePath, $this->buildSqlHeader('--', $database, $host));
+
+        $arguments = ['--inserts', '--no-owner', '--no-privileges'];
+
+        if ($dropTables) {
+            $arguments[] = '--clean';
+        }
+
+        foreach ($tables as $table) {
+            $arguments[] = '--table';
+            $arguments[] = (string) $table;
+        }
+
+        if (!$this->runPostgresDump($arguments, $tempFilePath)) {
             if (is_file($tempFilePath)) {
                 unlink($tempFilePath);
             }
 
+            return null;
+        }
+
+        return $tempFilePath;
+    }
+
+    /**
+     * Replays a SQL file into the database.
+     *
+     * @since 3.5.8
+     * @param string $path
+     * @return bool
+     */
+    public function restorePostgresFile($path)
+    {
+        $path = (string) $path;
+
+        if (!is_file($path)) {
             return false;
         }
 
-        // No shell is involved here, and that is the point. The previous form
-        // was `PGPASSWORD=… pg_dump … >> file`, and a leading VAR=value
-        // assignment is POSIX shell syntax that cmd.exe rejects outright with
-        // "'PGPASSWORD' is not recognized", so this backup could never succeed
-        // on Windows. Passing the password as an environment entry and the
-        // arguments as a list works the same way on every platform, and has
-        // the side benefit that nothing has to be quoted for a shell.
-        $process = new Process(
-            [
-                'pg_dump',
-                '--host', $host,
-                '--username', $username,
-                '--dbname', (string) $database,
-                '--clean',
-                '--inserts',
-                '--no-owner',
-                '--no-privileges',
-            ],
-            null,
-            ['PGPASSWORD' => $password]
-        );
-        $process->setTimeout(null);
+        $process = $this->buildPostgresProcess('psql', ['--file', $path]);
+
+        try {
+            $process->run();
+        } catch (\Throwable $exception) {
+            return false;
+        }
+
+        return $process->isSuccessful();
+    }
+
+    /**
+     * Streams a pg_dump run onto the end of the given file.
+     *
+     * @param array $arguments
+     * @param string $tempFilePath
+     * @return bool
+     */
+    protected function runPostgresDump(array $arguments, $tempFilePath)
+    {
+        $handle = fopen($tempFilePath, 'ab');
+
+        if ($handle === false) {
+            return false;
+        }
+
+        $process = $this->buildPostgresProcess('pg_dump', $arguments);
 
         try {
             // Streamed rather than buffered: a dump is as large as the
@@ -218,29 +296,65 @@ protected function createPostgresSnapshot($database, $filePath)
         } catch (\Throwable $exception) {
             fclose($handle);
 
-            if (is_file($tempFilePath)) {
-                unlink($tempFilePath);
-            }
-
             return false;
         }
 
         fclose($handle);
         clearstatcache(true, $tempFilePath);
 
-        if (!$process->isSuccessful() || !is_file($tempFilePath) || filesize($tempFilePath) <= 0) {
-            if (is_file($tempFilePath)) {
-                unlink($tempFilePath);
-            }
+        return $process->isSuccessful() && is_file($tempFilePath) && filesize($tempFilePath) > 0;
+    }
 
-            return false;
-        }
+    /**
+     * Builds a PostgreSQL client invocation.
+     *
+     * No shell is involved here, and that is the point. The previous form was
+     * `PGPASSWORD=... pg_dump ... >> file`, and a leading VAR=value assignment
+     * is POSIX shell syntax that cmd.exe rejects outright with "'PGPASSWORD'
+     * is not recognized", so this backup could never succeed on Windows.
+     * Passing the password as an environment entry and the arguments as a list
+     * works the same way on every platform, keeps the password out of the
+     * process list, and has the side benefit that nothing has to be quoted for
+     * a shell - an argument holding a semicolon stays one argument.
+     *
+     * @param string $binary
+     * @param array $arguments
+     * @return Process
+     */
+    /**
+     * The connection settings the pg client is invoked with.
+     *
+     * @return array
+     */
+    protected function databaseConfig()
+    {
+        return (array) evo()->getDatabase()->getConfig();
+    }
 
-        if (is_file((string) $filePath)) {
-            unlink((string) $filePath);
-        }
+    protected function buildPostgresProcess($binary, array $arguments)
+    {
+        $config = $this->databaseConfig();
+        $password = isset($config['password']) ? (string) $config['password'] : '';
+        $host = isset($config['host']) ? (string) $config['host'] : '';
+        $username = isset($config['username']) ? (string) $config['username'] : '';
+        $database = isset($config['database']) ? (string) $config['database'] : '';
 
-        return rename($tempFilePath, (string) $filePath);
+        $process = new Process(
+            array_merge(
+                [
+                    (string) $binary,
+                    '--host', $host,
+                    '--username', $username,
+                    '--dbname', $database,
+                ],
+                array_values($arguments)
+            ),
+            null,
+            ['PGPASSWORD' => $password]
+        );
+        $process->setTimeout(null);
+
+        return $process;
     }
 
     protected function buildTempSnapshotFilePath($filePath)

diff --git a/core/tests/Unit/Security/BackupManagerShellTest.php b/core/tests/Unit/Security/BackupManagerShellTest.php
@@ -0,0 +1,122 @@
+<?php
+
+/*
+|--------------------------------------------------------------------------
+| Backup manager: no shell, no dump in the web root
+|--------------------------------------------------------------------------
+|
+| The PostgreSQL branches of the backup manager built psql and pg_dump command lines by
+| concatenation and ran them through exec(). The snapshot name from the restore form and the
+| table names from the checkbox list went in unquoted, so anyone holding bk_manager could append
+| a command of their own - a step up from that permission's intended reach, which is arbitrary
+| SQL, not arbitrary shell. The password rode along in argv, where the process list shows it.
+|
+| Everything now goes through DatabaseBackupService, which invokes the client with an argument
+| list and the password in the environment: no shell parses either, so nothing needs quoting.
+|
+| @since 3.5.8
+*/
+
+use EvolutionCMS\Services\DatabaseBackupService;
+
+/**
+ * Exposes the process builder and stands in for the connection settings, so the invocation can
+ * be inspected without a bootstrapped application.
+ */
+class InspectableBackupService extends DatabaseBackupService
+{
+    public $config = [
+        'host' => 'db.example.test',
+        'username' => 'evo',
+        'database' => 'evo_site',
+        'password' => 'sup3r-s3cret',
+    ];
+
+    protected function databaseConfig()
+    {
+        return $this->config;
+    }
+
+    public function inspectProcess($binary, array $arguments)
+    {
+        return $this->buildPostgresProcess($binary, $arguments);
+    }
+}
+
+describe('pg client invocation', function () {
+
+    test('the password travels in the environment, not on the command line', function () {
+        $process = (new InspectableBackupService())->inspectProcess('pg_dump', ['--clean']);
+
+        expect($process->getEnv())->toBe(['PGPASSWORD' => 'sup3r-s3cret'])
+            // argv is world readable through ps and /proc/<pid>/cmdline.
+            ->and($process->getCommandLine())->not->toContain('sup3r-s3cret');
+    });
+
+    test('an argument carrying shell metacharacters stays a single argument', function () {
+        $table = 'evo_users; rm -rf /';
+        $line = (new InspectableBackupService())->inspectProcess('pg_dump', ['--table', $table])
+            ->getCommandLine();
+
+        // Both escaping styles Symfony uses - sh and cmd.exe - wrap the whole value in quotes,
+        // so the semicolon never reaches a command parser.
+        expect(str_contains($line, '"' . $table . '"') || str_contains($line, "'" . $table . "'"))
+            ->toBeTrue();
+    });
+
+    test('the connection settings are passed as separate arguments', function () {
+        $line = (new InspectableBackupService())->inspectProcess('psql', ['--file', '/tmp/x.sql'])
+            ->getCommandLine();
+
+        expect($line)
+            ->toContain('db.example.test')
+            ->toContain('evo_site')
+            // The old form put credentials into a postgresql://user:password@host URI.
+            ->not->toContain('postgresql://');
+    });
+});
+
+describe('deny rule for dump directories', function () {
+
+    test('covers both Apache generations', function () {
+        expect(DatabaseBackupService::DENY_HTACCESS)
+            ->toContain('mod_authz_core')
+            ->toContain('Require all denied')
+            // Order/Deny alone is 2.2 syntax: on 2.4 without mod_access_compat it is a 500,
+            // which serves the directory instead of denying it.
+            ->toContain('Deny from all');
+    });
+});
+
+describe('backup manager call sites', function () {
+
+    test('no shell is spawned any more', function () {
+        $source = file_get_contents(__DIR__ . '/../../../../manager/actions/bkmanager.static.php');
+
+        expect($source)
+            ->not->toContain('exec($dump_request')
+            ->not->toContain('PGPASSWORD=')
+            ->toContain('->restorePostgresFile(')
+            ->toContain('->dumpPostgresTables(');
+    });
+
+    test('no dump is written under the web root', function () {
+        $source = file_get_contents(__DIR__ . '/../../../../manager/actions/bkmanager.static.php');
+
+        // assets/backup/temp.php is served by Apache: the default ht.access excludes assets/
+        // from every rule it has, so a dump left there is one guessed URL away.
+        expect($source)->not->toContain("EVO_BASE_PATH . 'assets/backup/temp.php'");
+    });
+
+    test('the snapshot to restore has to resolve inside the snapshot directory', function () {
+        $source = file_get_contents(__DIR__ . '/../../../../manager/actions/bkmanager.static.php');
+
+        // basename() alone is not enough - the name is also read back as SQL, so the resolved
+        // path is compared against the directory it must sit in.
+        expect($source)
+            ->toContain('$filename = basename(')
+            ->toContain("preg_match('/^[A-Za-z0-9_.-]+\\.sql$/', \$filename)")
+            ->toContain('strncmp($path, $snapshotDir')
+            ->not->toContain("EvolutionCMS()->getConfig('snapshot_path') . \$_POST['filename']");
+    });
+});