diff --git a/man/simple_backup.1 b/man/simple_backup.1 index 0483960..a394363 100644 --- a/man/simple_backup.1 +++ b/man/simple_backup.1 @@ -209,7 +209,7 @@ Backup failed because output directory for storing the backup does not exist. Permission denied to access the output directory. .TP .B 4 -rsync error (rsync returned a value other than 0 or 24). +rsync error (rsync returned a value other than 0 or 24, or rsync is not installed). .TP .B 5 SSH connection failed. diff --git a/simple_backup/simple_backup.py b/simple_backup/simple_backup.py index bc7f2ca..c6a0fac 100755 --- a/simple_backup/simple_backup.py +++ b/simple_backup/simple_backup.py @@ -229,7 +229,8 @@ class Backup: self._exclude_path = '' self._remote = False self._ssh: paramiko.client.SSHClient | None = None - self._password_auth = False + # Passed to sshpass through rsync's environment only + self._password: str | None = None self._removed_count = 0 def check_params(self, homedir: str = '') -> int: @@ -558,8 +559,7 @@ class Backup: ssh.connect(**host, username=self.ssh_user, timeout=SSH_TIMEOUT, password=password) - self._password_auth = True - os.environ['SSHPASS'] = password + self._password = password return ssh except GetPassWarning as e: @@ -690,6 +690,16 @@ class Backup: _notify('Starting backup...') + # Check before anything is removed (--remove-before-backup) + rsync = which('rsync') + + if rsync is None: + logger.critical('rsync not found. Install rsync to perform the backup') + + _notify('Backup failed (check log for details)') + + return 4 + self.define_backup_dir() fd, self._inputs_path = mkstemp(prefix='tmp_inputs', text=True) @@ -729,7 +739,7 @@ class Backup: logger.info('Copying files. This may take a long time...') # Build the argument list directly, so paths with spaces or quotes are passed unchanged - args = ['/usr/bin/rsync', *self.options.split()] + args = [rsync, *self.options.split()] if self._last_backup != '': args.append(f'--link-dest={self._last_backup}') @@ -737,11 +747,15 @@ class Backup: args.extend([f'--exclude-from={self._exclude_path}', f'--files-from={self._inputs_path}', '/', f'{self._server}{self._output_dir}']) + # None inherits our environment + env: dict[str, str] | None = None + # rsync splits the -e command itself, so quote the parts that may contain spaces if self.ssh_keyfile is not None: ssh = f'ssh -i {shlex.quote(self.ssh_keyfile)} -o StrictHostKeyChecking=accept-new' - elif self._password_auth and self.ssh_user is not None and which('sshpass'): + elif self._password is not None and self.ssh_user is not None and which('sshpass'): ssh = f'sshpass -e ssh -l {shlex.quote(self.ssh_user)} -o StrictHostKeyChecking=accept-new' + env = {**os.environ, 'SSHPASS': self._password} else: ssh = 'ssh -o StrictHostKeyChecking=accept-new' @@ -756,7 +770,7 @@ class Backup: args.append('--info=progress2') bar = _ProgressBar() - with Popen(args, stdin=PIPE, stdout=PIPE, stderr=STDOUT, shell=False) as p: + with Popen(args, stdin=PIPE, stdout=PIPE, stderr=STDOUT, shell=False, env=env) as p: assert p.stdin is not None and isinstance(p.stdout, io.BufferedIOBase) p.stdin.close() @@ -770,11 +784,6 @@ class Backup: if bar is not None: bar.clear() - try: - del os.environ['SSHPASS'] - except KeyError: - pass - returncode = p.wait() if returncode == 0: diff --git a/tests/test_local.py b/tests/test_local.py index 7af0a53..63ce460 100644 --- a/tests/test_local.py +++ b/tests/test_local.py @@ -6,6 +6,7 @@ import os from collections.abc import Callable, Iterator from datetime import datetime, timedelta from pathlib import Path +from shutil import which from typing import Any import pytest @@ -338,10 +339,21 @@ class TestReturnCodes: assert run_backup(out, [str(src)]) == 0 (args,) = fake_popen.calls - assert args[0] == '/usr/bin/rsync' + assert args[0] == which('rsync') assert f'--link-dest={out}/simple_backup/{old_backups[-1]}' in args assert args[args.index('/') + 1] == f'{out}/simple_backup/2026-10-14_10-00-00' + def test_rsync_missing(self, src: Path, out: Path, tmp_path: Path, fake_popen: type[FakePopen], + old_backups: list[str], monkeypatch: pytest.MonkeyPatch, + caplog: pytest.LogCaptureFixture) -> None: + monkeypatch.setenv('PATH', str(tmp_path / 'empty')) + + assert run_backup(out, [str(src)], keep=0, remove_before=True) == 4 + assert fake_popen.calls == [] + # Nothing was removed + assert backups(out) == old_backups + assert 'rsync not found' in caplog.text + @pytest.mark.parametrize('returncode, result, removed', [ (0, 0, True), (24, 0, True), diff --git a/tests/test_remote.py b/tests/test_remote.py index 9f52097..8506cb3 100644 --- a/tests/test_remote.py +++ b/tests/test_remote.py @@ -254,9 +254,11 @@ class RecordPopen: returncode = 0 calls: list[list[str]] = [] + envs: list[dict[str, str] | None] = [] def __init__(self, args: list[str], **kwargs: Any) -> None: RecordPopen.calls.append(args) + RecordPopen.envs.append(kwargs.get('env')) self.stdin = io.BytesIO() self.stdout = io.BytesIO(b'sent 1 bytes received 2 bytes\ntotal size is 3\n') @@ -276,6 +278,7 @@ class TestRsyncArgs: def record_popen(self, monkeypatch: pytest.MonkeyPatch, tmp_path: Path) -> None: RecordPopen.returncode = 0 RecordPopen.calls = [] + RecordPopen.envs = [] monkeypatch.setattr(sb, 'Popen', RecordPopen) # The input must exist locally (tmp_path / 'input').mkdir() @@ -322,19 +325,26 @@ class TestRsyncArgs: def test_password_auth(self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: monkeypatch.setattr(sb, 'which', lambda cmd: f'/usr/bin/{cmd}') - monkeypatch.setenv('SSHPASS', 'secret') + monkeypatch.delenv('SSHPASS', raising=False) backup = connected_backup() backup.inputs = [str(tmp_path / 'input')] - backup._password_auth = True + backup._password = 'secret' assert backup.run() == 0 (args,) = RecordPopen.calls assert shlex.split(self.ssh_command(args)) == ['sshpass', '-e', 'ssh', '-l', 'user', '-o', 'StrictHostKeyChecking=accept-new'] - # The password must not stay in the environment + # Only rsync gets the password, never our own environment + (env,) = RecordPopen.envs + assert env is not None and env['SSHPASS'] == 'secret' assert 'SSHPASS' not in os.environ + def test_no_password_env(self, tmp_path: Path) -> None: + self.run(tmp_path) + + assert RecordPopen.envs == [None] + def test_failure(self, tmp_path: Path, remote_home: Path) -> None: base = make_backups(remote_home, '2026-10-01_10-00-00') RecordPopen.returncode = 23 @@ -471,13 +481,13 @@ class TestLogin: def test_password(self, client: FakeClient, monkeypatch: pytest.MonkeyPatch) -> None: client.outcomes = [ssh_error(), ssh_error(), None] monkeypatch.setattr(sb, 'getpass', lambda prompt: 'secret') - monkeypatch.setenv('SSHPASS', '') + monkeypatch.delenv('SSHPASS', raising=False) backup = remote_backup() assert backup._ssh_connect() is client assert client.connects[-1]['password'] == 'secret' - assert backup._password_auth - assert os.environ['SSHPASS'] == 'secret' + assert backup._password == 'secret' + assert 'SSHPASS' not in os.environ def test_wrong_password(self, client: FakeClient, monkeypatch: pytest.MonkeyPatch) -> None: client.outcomes = [ssh_error(), ssh_error(), ssh_error()]