Fix broken escaping logic for "pg_dump_command" (#822) + bonus shell injection fixes.

NEWS

@@ -1,3 +1,12 @@
+1.8.8
+ * #370: For the PostgreSQL hook, pass the "PGSSLMODE" environment variable through to Borg when the
+   database's configuration omits the "ssl_mode" option.
+ * #818: Allow the "--repository" flag to match across multiple configuration files.
+ * #820: Fix broken repository detection in the "rcreate" action with Borg 1.4. The issue did not
+   occur with other versions of Borg.
+ * #822: Fix broken escaping logic in the PostgreSQL hook's "pg_dump_command" option.
+ * SECURITY: Prevent additional shell injection attacks within the PostgreSQL hook.
+
 1.8.7
  * #736: Store included configuration files within each backup archive in support of the "config
    bootstrap" action. Previously, only top-level configuration files were stored.

borgmatic/borg/rcreate.py

@@ -8,7 +8,7 @@ from borgmatic.execute import DO_NOT_CAPTURE, execute_command
 logger = logging.getLogger(__name__)
 
 
-RINFO_REPOSITORY_NOT_FOUND_EXIT_CODE = 2
+RINFO_REPOSITORY_NOT_FOUND_EXIT_CODES = {2, 13}
 
 
 def create_repository(
@@ -45,7 +45,7 @@ def create_repository(
         logger.info(f'{repository_path}: Repository already exists. Skipping creation.')
         return
     except subprocess.CalledProcessError as error:
-        if error.returncode != RINFO_REPOSITORY_NOT_FOUND_EXIT_CODE:
+        if error.returncode not in RINFO_REPOSITORY_NOT_FOUND_EXIT_CODES:
             raise
 
     lock_wait = config.get('lock_wait')

borgmatic/config/validate.py

@@ -167,11 +167,10 @@ def repositories_match(first, second):
 def guard_configuration_contains_repository(repository, configurations):
     '''
     Given a repository path and a dict mapping from config filename to corresponding parsed config
-    dict, ensure that the repository is declared exactly once in all of the configurations. If no
+    dict, ensure that the repository is declared at least once in all of the configurations. If no
     repository is given, skip this check.
 
-    Raise ValueError if the repository is not found in a configuration, or is declared multiple
-    times.
+    Raise ValueError if the repository is not found in any configurations.
     '''
     if not repository:
         return
@@ -186,9 +185,7 @@ def guard_configuration_contains_repository(repository, configurations):
     )
 
     if count == 0:
-        raise ValueError(f'Repository {repository} not found in configuration files')
-    if count > 1:
-        raise ValueError(f'Repository {repository} found in multiple configuration files')
+        raise ValueError(f'Repository "{repository}" not found in configuration files')
 
 
 def guard_single_repository_selected(repository, configurations):

borgmatic/hooks/postgresql.py

@@ -25,8 +25,8 @@ def make_dump_path(config):  # pragma: no cover
 
 def make_extra_environment(database, restore_connection_params=None):
     '''
-    Make the extra_environment dict from the given database configuration.
-    If restore connection params are given, this is for a restore operation.
+    Make the extra_environment dict from the given database configuration. If restore connection
+    params are given, this is for a restore operation.
     '''
     extra = dict()
 
@@ -40,7 +40,8 @@ def make_extra_environment(database, restore_connection_params=None):
     except (AttributeError, KeyError):
         pass
 
-    extra['PGSSLMODE'] = database.get('ssl_mode', 'disable')
+    if 'ssl_mode' in database:
+        extra['PGSSLMODE'] = database['ssl_mode']
     if 'ssl_cert' in database:
         extra['PGSSLCERT'] = database['ssl_cert']
     if 'ssl_key' in database:
@@ -49,6 +50,7 @@ def make_extra_environment(database, restore_connection_params=None):
         extra['PGSSLROOTCERT'] = database['ssl_root_cert']
     if 'ssl_crl' in database:
         extra['PGSSLCRL'] = database['ssl_crl']
+
     return extra
 
 
@@ -71,9 +73,11 @@ def database_names_to_dump(database, extra_environment, log_prefix, dry_run):
     if dry_run:
         return ()
 
-    psql_command = shlex.split(database.get('psql_command') or 'psql')
+    psql_command = tuple(
+        shlex.quote(part) for part in shlex.split(database.get('psql_command') or 'psql')
+    )
     list_command = (
-        tuple(psql_command)
+        psql_command
         + ('--list', '--no-password', '--no-psqlrc', '--csv', '--tuples-only')
         + (('--host', database['hostname']) if 'hostname' in database else ())
         + (('--port', str(database['port'])) if 'port' in database else ())
@@ -125,7 +129,10 @@ def dump_data_sources(databases, config, log_prefix, dry_run):
         for database_name in dump_database_names:
             dump_format = database.get('format', None if database_name == 'all' else 'custom')
             default_dump_command = 'pg_dumpall' if database_name == 'all' else 'pg_dump'
-            dump_command = database.get('pg_dump_command') or default_dump_command
+            dump_command = tuple(
+                shlex.quote(part)
+                for part in shlex.split(database.get('pg_dump_command') or default_dump_command)
+            )
             dump_filename = dump.make_data_source_dump_filename(
                 dump_path, database_name, database.get('hostname')
             )
@@ -136,8 +143,8 @@ def dump_data_sources(databases, config, log_prefix, dry_run):
                 continue
 
             command = (
-                (
-                    shlex.quote(dump_command),
+                dump_command
+                + (
                     '--no-password',
                     '--clean',
                     '--if-exists',
@@ -240,9 +247,11 @@ def restore_data_source_dump(
     dump_filename = dump.make_data_source_dump_filename(
         make_dump_path(config), data_source['name'], data_source.get('hostname')
     )
-    psql_command = shlex.split(data_source.get('psql_command') or 'psql')
+    psql_command = tuple(
+        shlex.quote(part) for part in shlex.split(data_source.get('psql_command') or 'psql')
+    )
     analyze_command = (
-        tuple(psql_command)
+        psql_command
         + ('--no-password', '--no-psqlrc', '--quiet')
         + (('--host', hostname) if hostname else ())
         + (('--port', port) if port else ())
@@ -256,9 +265,12 @@ def restore_data_source_dump(
         + ('--command', 'ANALYZE')
     )
     use_psql_command = all_databases or data_source.get('format') == 'plain'
-    pg_restore_command = shlex.split(data_source.get('pg_restore_command') or 'pg_restore')
+    pg_restore_command = tuple(
+        shlex.quote(part)
+        for part in shlex.split(data_source.get('pg_restore_command') or 'pg_restore')
+    )
     restore_command = (
-        tuple(psql_command if use_psql_command else pg_restore_command)
+        (psql_command if use_psql_command else pg_restore_command)
         + ('--no-password',)
         + (('--no-psqlrc',) if use_psql_command else ('--if-exists', '--exit-on-error', '--clean'))
         + (('--dbname', data_source['name']) if not all_databases else ())

setup.py

@@ -1,6 +1,6 @@
 from setuptools import find_packages, setup
 
-VERSION = '1.8.7'
+VERSION = '1.8.8'
 
 
 setup(

test_requirements.txt

@@ -1,10 +1,10 @@
-appdirs==1.4.4; python_version >= '3.8'
+appdirs==1.4.4
 apprise==1.3.0
-attrs==22.2.0; python_version >= '3.8'
-black==23.3.0; python_version >= '3.8'
+attrs==22.2.0
+black==23.3.0
 certifi==2023.7.22
 chardet==5.1.0
-click==8.1.3; python_version >= '3.8'
+click==8.1.3
 codespell==2.2.4
 colorama==0.4.6
 coverage==7.2.3

tests/unit/borg/test_rcreate.py

@@ -18,7 +18,7 @@ def insert_rinfo_command_found_mock():
 
 def insert_rinfo_command_not_found_mock():
     flexmock(module.rinfo).should_receive('display_repository_info').and_raise(
-        subprocess.CalledProcessError(module.RINFO_REPOSITORY_NOT_FOUND_EXIT_CODE, [])
+        subprocess.CalledProcessError(sorted(module.RINFO_REPOSITORY_NOT_FOUND_EXIT_CODES)[0], [])
     )
 
 

tests/unit/config/test_validate.py

@@ -184,21 +184,6 @@ def test_guard_configuration_contains_repository_errors_when_repository_missing_
         )
 
 
-def test_guard_configuration_contains_repository_errors_when_repository_matches_config_twice():
-    flexmock(module).should_receive('repositories_match').replace_with(
-        lambda first, second: first == second
-    )
-
-    with pytest.raises(ValueError):
-        module.guard_configuration_contains_repository(
-            repository='repo',
-            configurations={
-                'config.yaml': {'repositories': ['repo', 'repo2']},
-                'other.yaml': {'repositories': ['repo']},
-            },
-        )
-
-
 def test_guard_single_repository_selected_raises_when_multiple_repositories_configured_and_none_selected():
     with pytest.raises(ValueError):
         module.guard_single_repository_selected(

tests/unit/hooks/test_postgresql.py

@@ -50,6 +50,14 @@ def test_make_extra_environment_without_cli_password_or_configured_password_does
     assert 'PGPASSWORD' not in extra
 
 
+def test_make_extra_environment_without_ssl_mode_does_not_set_ssl_mode():
+    database = {'name': 'foo'}
+
+    extra = module.make_extra_environment(database)
+
+    assert 'PGSSLMODE' not in extra
+
+
 def test_database_names_to_dump_passes_through_individual_database_name():
     database = {'name': 'foo'}
 
@@ -164,11 +172,17 @@ def test_database_names_to_dump_with_all_and_format_excludes_particular_database
 
 
 def test_database_names_to_dump_with_all_and_psql_command_uses_custom_command():
-    database = {'name': 'all', 'format': 'custom', 'psql_command': 'docker exec mycontainer psql'}
+    database = {
+        'name': 'all',
+        'format': 'custom',
+        'psql_command': 'docker exec --workdir * mycontainer psql',
+    }
     flexmock(module).should_receive('execute_command_and_capture_output').with_args(
         (
             'docker',
             'exec',
+            '--workdir',
+            "'*'",  # Should get shell escaped to prevent injection attacks.
             'mycontainer',
             'psql',
             '--list',
@@ -468,7 +482,7 @@ def test_dump_data_sources_runs_pg_dumpall_for_all_databases():
 
 
 def test_dump_data_sources_runs_non_default_pg_dump():
-    databases = [{'name': 'foo', 'pg_dump_command': 'special_pg_dump'}]
+    databases = [{'name': 'foo', 'pg_dump_command': 'special_pg_dump --compress *'}]
     process = flexmock()
     flexmock(module).should_receive('make_extra_environment').and_return({'PGSSLMODE': 'disable'})
     flexmock(module).should_receive('make_dump_path').and_return('')
@@ -482,6 +496,8 @@ def test_dump_data_sources_runs_non_default_pg_dump():
     flexmock(module).should_receive('execute_command').with_args(
         (
             'special_pg_dump',
+            '--compress',
+            "'*'",  # Should get shell escaped to prevent injection attacks.
             '--no-password',
             '--clean',
             '--if-exists',
@@ -979,8 +995,8 @@ def test_restore_data_source_dump_runs_non_default_pg_restore_and_psql():
     hook_config = [
         {
             'name': 'foo',
-            'pg_restore_command': 'docker exec mycontainer pg_restore',
-            'psql_command': 'docker exec mycontainer psql',
+            'pg_restore_command': 'docker exec --workdir * mycontainer pg_restore',
+            'psql_command': 'docker exec --workdir * mycontainer psql',
             'schemas': None,
         }
     ]
@@ -993,6 +1009,8 @@ def test_restore_data_source_dump_runs_non_default_pg_restore_and_psql():
         (
             'docker',
             'exec',
+            '--workdir',
+            "'*'",  # Should get shell escaped to prevent injection attacks.
             'mycontainer',
             'pg_restore',
             '--no-password',
@@ -1011,6 +1029,8 @@ def test_restore_data_source_dump_runs_non_default_pg_restore_and_psql():
         (
             'docker',
             'exec',
+            '--workdir',
+            "'*'",  # Should get shell escaped to prevent injection attacks.
             'mycontainer',
             'psql',
             '--no-password',