fix single quote escape for append, contains

patchwork/files.py

@@ -2,8 +2,6 @@
 Tools for file and directory management.
 """
 
-import re
-
 from invoke.vendor import six
 
 from .util import set_runner
@@ -25,12 +23,16 @@ def directory(c, runner, path, user=None, group=None, mode=None):
     :param str mode:
         ``chmod`` compatible mode string to apply to the directory.
     """
-    runner("mkdir -p {}".format(path))
+    path = path.replace(r'"', r'\"')
+    runner('mkdir -p "{}"'.format(path))
     if user is not None:
         group = group or user
-        runner("chown {}:{} {}".format(user, group, path))
+        user = user.replace(r'"', r'\"')
+        group = group.replace(r'"', r'\"')
+        runner('chown "{}:{}" "{}"'.format(user, group, path))
     if mode is not None:
-        runner("chmod {} {}".format(mode, path))
+        mode = mode.replace(r'"', r'\"')
+        runner('chmod "{}" "{}"'.format(mode, path))
 
 
 @set_runner
@@ -43,7 +45,7 @@ def exists(c, runner, path):
     :param str path:
         Path to check for existence.
     """
-    cmd = 'test -e "$(echo {})"'.format(path)
+    cmd = 'test -e "$(echo {})"'.format(path.replace(r'"', r'\"'))
     return runner(cmd, hide=True, warn=True).ok
 
 
@@ -126,17 +128,34 @@ def append(c, runner, filename, text, partial=False, escape=True):
             and contains(c, filename, regex, escape=False, runner=runner)
         ):
             continue
-        line = line.replace("'", r"'\\''") if escape else line
-        runner("echo '{}' >> {}".format(line, filename))
+        line = line.replace("'", r"'\''") if escape else line
+        filename = filename.replace(r'"', r'\"')
+        runner("echo '{}' >> \"{}\"".format(line, filename))
 
 
 def _escape_for_regex(text):
     """Escape ``text`` to allow literal matching using egrep"""
-    regex = re.escape(text)
-    # Seems like double escaping is needed for \
-    regex = regex.replace("\\\\", "\\\\\\")
-    # Triple-escaping seems to be required for $ signs
-    regex = regex.replace(r"\$", r"\\\$")
-    # Whereas single quotes should not be escaped
-    regex = regex.replace(r"\'", "'")
-    return regex
+    re_specials = '\\^$|(){}[]*+?.'
+    sh_specials = '\\$`"'
+    re_chars = []
+    sh_chars = []
+
+    # Removes newline characters.
+    # egrep does not support multi-line patterns, so they should not be used
+    # with these functions. But, this might as well remain consistent with
+    # its original behavior regarding newlines, which was to escape them,
+    # causing the shell to ignore/omit them.
+
+    for c in text:
+        if c == '\n':
+            continue
+        if c in re_specials:
+            re_chars.append('\\')
+        re_chars.append(c)
+
+    for c in re_chars:
+        if c in sh_specials:
+            sh_chars.append('\\')
+        sh_chars.append(c)
+
+    return ''.join(sh_chars)

tests/files.py

@@ -1,6 +1,6 @@
-from mock import call
+from mock import call, patch
 
-from patchwork.files import directory
+from patchwork.files import directory, append, contains
 
 
 class files:
@@ -9,26 +9,94 @@ class directory_:
 
         def base_case_creates_dir_with_dash_p(self, cxn):
             directory(cxn, "/some/dir")
-            cxn.run.assert_called_once_with("mkdir -p /some/dir")
+            cxn.run.assert_called_once_with('mkdir -p "/some/dir"')
+
+        def creates_dir_with_double_quotes(self, cxn):
+            directory(cxn, '/some/double"quote')
+            cxn.run.assert_called_once_with('mkdir -p "/some/double\\"quote"')
 
         def user_sets_owner_and_group(self, cxn):
             directory(cxn, "/some/dir", user="user")
-            cxn.run.assert_any_call("chown user:user /some/dir")
+            cxn.run.assert_any_call('chown "user:user" "/some/dir"')
 
         def group_may_be_given_to_change_group(self, cxn):
             directory(cxn, "/some/dir", user="user", group="admins")
-            cxn.run.assert_any_call("chown user:admins /some/dir")
+            cxn.run.assert_any_call('chown "user:admins" "/some/dir"')
 
         def mode_adds_a_chmod(self, cxn):
             directory(cxn, "/some/dir", mode="0700")
-            cxn.run.assert_any_call("chmod 0700 /some/dir")
+            cxn.run.assert_any_call('chmod "0700" "/some/dir"')
 
         def all_args_in_play(self, cxn):
             directory(
                 cxn, "/some/dir", user="user", group="admins", mode="0700"
             )
             assert cxn.run.call_args_list == [
-                call("mkdir -p /some/dir"),
-                call("chown user:admins /some/dir"),
-                call("chmod 0700 /some/dir"),
+                call('mkdir -p "/some/dir"'),
+                call('chown "user:admins" "/some/dir"'),
+                call('chmod "0700" "/some/dir"'),
+            ]
+
+        def contains_issue34(self, cxn):
+            contains(cxn, "/some/file", "'*'")
+            cxn.run.assert_any_call(
+                r'''egrep "'\\*'" "/some/file"''',
+                hide=True, warn=True
+            )
+
+        def contains_trailing_dollar(self, cxn):
+            contains(cxn, "/some/file", "trailing $")
+            cxn.run.assert_called_once_with(
+                r'egrep "trailing \\\$" "/some/file"',
+                hide=True, warn=True
+            )
+
+        def contains_trailing_backslash(self, cxn):
+            contains(cxn, "/some/file", "trailing \\")
+            cxn.run.assert_called_once_with(
+                r'egrep "trailing \\\\" "/some/file"',
+                hide=True, warn=True
+            )
+
+        @patch('patchwork.files.contains')
+        def append_ending_single_quote(self, m, cxn):
+            m.return_value = False
+            append(cxn, "/some/file", "alias l='ls -rtl'")
+            cxn.run.assert_any_call(
+                'echo \'alias l=\'\\\'\'ls -rtl\'\\\'\'\' >> "/some/file"'
+            )
+
+        @patch('patchwork.files.contains')
+        def append_trailing_dollar(self, m, cxn):
+            m.return_value = False
+            append(cxn, "/some/file", "trailing $")
+            cxn.run.assert_any_call(
+                r'''echo 'trailing $' >> "/some/file"''',
+            )
+
+        @patch('patchwork.files.contains')
+        def append_trailing_backslash(self, m, cxn):
+            m.return_value = False
+            append(cxn, "/some/file", "trailing \\")
+
+            assert cxn.run.call_args_list == [
+                call('test -e "$(echo /some/file)"', hide=True, warn=True),
+                call(r'''echo 'trailing \' >> "/some/file"''')
             ]
+
+
+        @patch('patchwork.files.contains')
+        def append_trailing_backtick(self, m, cxn):
+            m.return_value = False
+            append(cxn, "/some/file", "trailing `")
+            cxn.run.assert_any_call(
+                r'''echo 'trailing `' >> "/some/file"'''
+            )
+
+        @patch('patchwork.files.contains')
+        def append_issue_34(self, m, cxn):
+            m.return_value = False
+            append(cxn, "/some/file", "listen_addresses='*'", escape=True)
+            cxn.run.assert_any_call(
+                'echo \'listen_addresses=\'\\\'\'*\'\\\'\'\' >> "/some/file"'
+            )