Escape spaces in file paths when running commands on SSH remote (#4767)

* Escape file paths for calculation of md5 hashes on ssh remote

* Add tests for hardlink and improved md5 test with PropertyMock

* Add additional special characters to check for fix of issue #4628 and fix formatting

* Fix linting errors

* Move tests to correct file and use injected ssh_connection instead of setting it up from scratch

dvc/tree/ssh/connection.py

@@ -2,6 +2,7 @@ import errno
 import logging
 import os
 import posixpath
+import shlex
 import stat
 from contextlib import suppress
 
@@ -291,6 +292,7 @@ class SSHConnection:
          Example:
               MD5 (foo.txt) = f3d220a856b52aabbf294351e8a24300
         """
+        path = shlex.quote(path)
         if self.uname == "Linux":
             md5 = self.execute("md5sum " + path).split()[0]
         elif self.uname == "Darwin":
@@ -304,6 +306,8 @@ class SSHConnection:
         return md5
 
     def copy(self, src, dest):
+        dest = shlex.quote(dest)
+        src = shlex.quote(src)
         self.execute(f"cp {src} {dest}")
 
     def open_max_sftp_channels(self):
@@ -325,6 +329,8 @@ class SSHConnection:
         self.sftp.symlink(src, dest)
 
     def reflink(self, src, dest):
+        dest = shlex.quote(dest)
+        src = shlex.quote(src)
         if self.uname == "Linux":
             return self.execute(f"cp --reflink {src} {dest}")
 
@@ -334,4 +340,6 @@ class SSHConnection:
         raise DvcException(f"'{self.uname}' is not supported as a SSH remote")
 
     def hardlink(self, src, dest):
+        dest = shlex.quote(dest)
+        src = shlex.quote(src)
         self.execute(f"ln {src} {dest}")

tests/unit/remote/ssh/test_connection.py

@@ -8,9 +8,18 @@ import pytest
 
 from dvc.info import get_fs_type
 from dvc.system import System
+from dvc.tree.ssh.connection import SSHConnection
 
 here = os.path.abspath(os.path.dirname(__file__))
 
+SRC_PATH_WITH_SPECIAL_CHARACTERS = "Escape me [' , ']"
+ESCAPED_SRC_PATH_WITH_SPECIAL_CHARACTERS = "'Escape me ['\"'\"' , '\"'\"']'"
+
+DEST_PATH_WITH_SPECIAL_CHARACTERS = "Escape me too [' , ']"
+ESCAPED_DEST_PATH_WITH_SPECIAL_CHARACTERS = (
+    "'Escape me too ['\"'\"' , '\"'\"']'"
+)
+
 
 def test_isdir(ssh_connection):
     assert ssh_connection.isdir(here)
@@ -131,3 +140,60 @@ def test_move(tmp_dir, ssh_connection):
     ssh_connection.move("foo", "copy")
     assert os.path.exists("copy")
     assert not os.path.exists("foo")
+
+
+@pytest.mark.parametrize(
+    "uname,md5command", [("Linux", "md5sum"), ("Darwin", "md5")]
+)
+def test_escapes_filepaths_for_md5_calculation(
+    ssh_connection, uname, md5command, mocker
+):
+    fake_md5 = "x" * 32
+    uname_mock = mocker.PropertyMock(return_value=uname)
+    mocker.patch.object(SSHConnection, "uname", new_callable=uname_mock)
+    ssh_connection.execute = mocker.Mock(return_value=fake_md5)
+    ssh_connection.md5(SRC_PATH_WITH_SPECIAL_CHARACTERS)
+    ssh_connection.execute.assert_called_with(
+        f"{md5command} {ESCAPED_SRC_PATH_WITH_SPECIAL_CHARACTERS}"
+    )
+
+
+def test_escapes_filepaths_for_copy(ssh_connection, mocker):
+    ssh_connection.execute = mocker.Mock()
+    ssh_connection.copy(
+        SRC_PATH_WITH_SPECIAL_CHARACTERS, DEST_PATH_WITH_SPECIAL_CHARACTERS
+    )
+    ssh_connection.execute.assert_called_with(
+        f"cp {ESCAPED_SRC_PATH_WITH_SPECIAL_CHARACTERS} "
+        + f"{ESCAPED_DEST_PATH_WITH_SPECIAL_CHARACTERS}"
+    )
+
+
+@pytest.mark.parametrize(
+    "uname,cp_command", [("Linux", "cp --reflink"), ("Darwin", "cp -c")]
+)
+def test_escapes_filepaths_for_reflink(
+    ssh_connection, uname, cp_command, mocker
+):
+    uname_mock = mocker.PropertyMock(return_value=uname)
+    mocker.patch.object(SSHConnection, "uname", new_callable=uname_mock)
+    ssh_connection.execute = mocker.Mock()
+    ssh_connection.reflink(
+        SRC_PATH_WITH_SPECIAL_CHARACTERS, DEST_PATH_WITH_SPECIAL_CHARACTERS
+    )
+    ssh_connection.execute.assert_called_with(
+        f"{cp_command} "
+        + f"{ESCAPED_SRC_PATH_WITH_SPECIAL_CHARACTERS} "
+        + f"{ESCAPED_DEST_PATH_WITH_SPECIAL_CHARACTERS}"
+    )
+
+
+def test_escapes_filepaths_for_hardlink(ssh_connection, mocker):
+    ssh_connection.execute = mocker.Mock()
+    ssh_connection.hardlink(
+        SRC_PATH_WITH_SPECIAL_CHARACTERS, DEST_PATH_WITH_SPECIAL_CHARACTERS
+    )
+    ssh_connection.execute.assert_called_with(
+        f"ln {ESCAPED_SRC_PATH_WITH_SPECIAL_CHARACTERS} "
+        + f"{ESCAPED_DEST_PATH_WITH_SPECIAL_CHARACTERS}"
+    )