Complete files quote migration (#611)

* Update `make_formatted_string_command` to require explicit quoting.

This function now requires arguments to be explicitly quoted. The change
makes it possible to quote some but not all arguments. It also removes
the magic and makes its usage explicit.

* Update files operations to explicitly quote formatting values.

* Add new `files.*` operation tests for filenames that require quotes.

* Update `files.*` facts to properly quote filenames.

* Add `__repr__` for `QuoteString` class.

* Remove pointless non-idempotent touch flag on `files.file` tests.

* Correct docstring on `make_formatted_string_command`.

* Properly escape paths for regex in file hash facts.

* Add tests for files facts with paths that need quoting.

pyinfra/api/command.py

@@ -1,33 +1,41 @@
 import shlex
 
+from string import Formatter
+
 from six.moves import shlex_quote
 
 from .operation_kwargs import get_executor_kwarg_keys
 
 
 def make_formatted_string_command(string, *args, **kwargs):
     '''
-    Helper function that takes a shell command or script as a string, splits
-    it, applies formatting to each bit - quoting any formatted values - before
-    returning them as a `StringCommand` object.
+    Helper function that takes a shell command or script as a string, splits it
+    using ``shlex.split`` and then formats each bit, returning a ``StringCommand``
+    instance with each bit.
 
     Useful to enable string formatted commands/scripts, for example:
 
     .. code:: python
 
-            curl_command = make_formatted_string_command('curl -sSLf {0} -o {1}', src, dest)
+        curl_command = make_formatted_string_command(
+            'curl -sSLf {0} -o {1}',
+            QuoteString(src),
+            QuoteString(dest),
+        )
     '''
 
-    formatted_bits = []
+    formatter = Formatter()
+    string_bits = []
 
     for bit in shlex.split(string):
-        formatted = bit.format(*args, **kwargs)
-        if bit.startswith('{') and bit.endswith('}'):
-            formatted_bits.append(QuoteString(formatted))
-        else:
-            formatted_bits.append(formatted)
+        for item in formatter.parse(bit):
+            if item[0]:
+                string_bits.append(item[0])
+            if item[1]:
+                value, _ = formatter.get_field(item[1], args, kwargs)
+                string_bits.append(value)
 
-    return StringCommand(*formatted_bits)
+    return StringCommand(*string_bits)
 
 
 class MaskString(str):
@@ -38,6 +46,9 @@ class QuoteString(object):
     def __init__(self, obj):
         self.object = obj
 
+    def __repr__(self):
+        return 'QuoteString({0})'.format(self.object)
+
 
 class PyinfraCommand(object):
     def __init__(self, *args, **kwargs):

pyinfra/facts/files.py

@@ -4,9 +4,7 @@
 
 from datetime import datetime
 
-from six.moves import shlex_quote
-
-from pyinfra.api.connectors.util import escape_unix_path
+from pyinfra.api.command import make_formatted_string_command, QuoteString
 from pyinfra.api.facts import FactBase
 from pyinfra.api.util import try_int
 
@@ -66,12 +64,11 @@ class File(FactBase):
     test_flag = '-e'
 
     def command(self, path):
-        path = escape_unix_path(path)
-        return (
-            '! test {test_flag} {path} || '  # only stat if the file exists
-            '({linux_stat_command} {path} 2> /dev/null || {bsd_stat_command} {path})'
-        ).format(
-            path=path,
+        return make_formatted_string_command((
+            '! test {test_flag} {0} || '  # only stat if the file exists
+            '( {linux_stat_command} {0} 2> /dev/null || {bsd_stat_command} {0} )'
+        ),
+            QuoteString(path),
             test_flag=self.test_flag,
             linux_stat_command=LINUX_STAT_COMMAND,
             bsd_stat_command=BSD_STAT_COMMAND,
@@ -143,17 +140,16 @@ class Sha1File(FactBase):
     ]
 
     def command(self, name):
-        name = escape_unix_path(name)
         self.name = name
-        return (
-            'test -e {0} && ('
-            'sha1sum {0} 2> /dev/null || shasum {0} 2> /dev/null || sha1 {0}'
+        return make_formatted_string_command((
+            'test -e {0} && ( '
+            'sha1sum {0} 2> /dev/null || shasum {0} 2> /dev/null || sha1 {0} '
             ') || true'
-        ).format(name)
+        ), QuoteString(name))
 
     def process(self, output):
         for regex in self._regexes:
-            regex = regex % self.name
+            regex = regex % re.escape(self.name)
             matches = re.match(regex, output[0])
             if matches:
                 return matches.group(1)
@@ -170,19 +166,18 @@ class Sha256File(FactBase):
     ]
 
     def command(self, name):
-        name = escape_unix_path(name)
         self.name = name
-        return (
-            'test -e {0} && ('
+        return make_formatted_string_command((
+            'test -e {0} && ( '
             'sha256sum {0} 2> /dev/null '
             '|| shasum -a 256 {0} 2> /dev/null '
-            '|| sha256 {0}'
+            '|| sha256 {0} '
             ') || true'
-        ).format(name)
+        ), QuoteString(name))
 
     def process(self, output):
         for regex in self._regexes:
-            regex = regex % self.name
+            regex = regex % re.escape(self.name)
             matches = re.match(regex, output[0])
             if matches:
                 return matches.group(1)
@@ -199,13 +194,15 @@ class Md5File(FactBase):
     ]
 
     def command(self, name):
-        name = escape_unix_path(name)
         self.name = name
-        return 'test -e {0} && (md5sum {0} 2> /dev/null || md5 {0}) || true'.format(name)
+        return make_formatted_string_command(
+            'test -e {0} && ( md5sum {0} 2> /dev/null || md5 {0} ) || true',
+            QuoteString(name),
+        )
 
     def process(self, output):
         for regex in self._regexes:
-            regex = regex % self.name
+            regex = regex % re.escape(self.name)
             matches = re.match(regex, output[0])
             if matches:
                 return matches.group(1)
@@ -218,21 +215,20 @@ class FindInFile(FactBase):
     '''
 
     def command(self, name, pattern):
-        name = escape_unix_path(name)
-        pattern = shlex_quote(pattern)
+        self.exists_name = '__pyinfra_exists_{0}'.format(name)
 
-        self.name = name
+        # TODO: remove special charts from __pyinfra_exists thing
 
-        return (
+        return make_formatted_string_command((
             'grep -e {0} {1} 2> /dev/null || '
-            '(find {1} -type f > /dev/null && echo "__pyinfra_exists_{1}" || true)'
-        ).format(pattern, name).strip()
+            '( find {1} -type f > /dev/null && echo {2} || true )'
+        ), QuoteString(pattern), QuoteString(name), QuoteString(self.exists_name))
 
     def process(self, output):
         # If output is the special string: no matches, so return an empty list;
         # this allows us to differentiate between no matches in an existing file
         # or a file not existing.
-        if output and output[0] == '__pyinfra_exists_{0}'.format(self.name):
+        if output and output[0] == self.exists_name:
             return []
 
         return output
@@ -245,7 +241,10 @@ class FindFiles(FactBase):
 
     @staticmethod
     def command(name):
-        return 'find {0} -type f || true'.format(escape_unix_path(name))
+        return make_formatted_string_command(
+            'find {0} -type f || true',
+            QuoteString(name),
+        )
 
     @staticmethod
     def process(output):
@@ -259,7 +258,10 @@ class FindLinks(FindFiles):
 
     @staticmethod
     def command(name):
-        return 'find {0} -type l || true'.format(escape_unix_path(name))
+        return make_formatted_string_command(
+            'find {0} -type l || true',
+            QuoteString(name),
+        )
 
 
 class FindDirectories(FindFiles):
@@ -269,4 +271,7 @@ class FindDirectories(FindFiles):
 
     @staticmethod
     def command(name):
-        return 'find {0} -type d || true'.format(escape_unix_path(name))
+        return make_formatted_string_command(
+            'find {0} -type d || true',
+            QuoteString(name),
+        )

pyinfra/operations/files.py

@@ -118,17 +118,23 @@ def download(
 
     # If we download, always do user/group/mode as SSH user may be different
     if download:
-        curl_command = make_formatted_string_command('curl -sSLf {0} -o {1}', src, dest)
+        curl_command = make_formatted_string_command(
+            'curl -sSLf {0} -o {1}',
+            QuoteString(src),
+            QuoteString(dest),
+        )
         wget_command = make_formatted_string_command(
-            'wget -q {0} -O {1} || (rm -f {1}; exit 1)', src, dest,
+            'wget -q {0} -O {1} || ( rm -f {1} ; exit 1 )',
+            QuoteString(src),
+            QuoteString(dest),
         )
 
         if host.fact.which('curl'):
             yield curl_command
         elif host.fact.which('wget'):
             yield wget_command
         else:
-            yield '({0}) || ({1})'.format(curl_command, wget_command)
+            yield '( {0} ) || ( {1} )'.format(curl_command, wget_command)
 
         if user or group:
             yield chown(dest, user, group)
@@ -138,21 +144,21 @@ def download(
 
         if sha1sum:
             yield make_formatted_string_command((
-                '((sha1sum {0} 2> /dev/null || shasum {0} || sha1 {0}) | grep {1}) '
-                '|| (echo {2} && exit 1)'
-            ), dest, sha1sum, 'SHA1 did not match!')
+                '(( sha1sum {0} 2> /dev/null || shasum {0} || sha1 {0} ) | grep {1} ) '
+                '|| ( echo {2} && exit 1 )'
+            ), QuoteString(dest), sha1sum, QuoteString('SHA1 did not match!'))
 
         if sha256sum:
             yield make_formatted_string_command((
-                '((sha256sum {0} 2> /dev/null || shasum -a 256 {0} || sha256 {0}) | grep {1}) '
-                '|| (echo {2} && exit 1)'
-            ), dest, sha256sum, 'SHA256 did not match!')
+                '(( sha256sum {0} 2> /dev/null || shasum -a 256 {0} || sha256 {0} ) | grep {1}) '
+                '|| ( echo {2} && exit 1 )'
+            ), QuoteString(dest), sha256sum, QuoteString('SHA256 did not match!'))
 
         if md5sum:
             yield make_formatted_string_command((
-                '((md5sum {0} 2> /dev/null || md5 {0}) | grep {1}) '
-                '|| (echo {2} && exit 1)'
-            ), dest, md5sum, 'MD5 did not match!')
+                '(( md5sum {0} 2> /dev/null || md5 {0} ) | grep {1}) '
+                '|| ( echo {2} && exit 1 )'
+            ), QuoteString(dest), md5sum, QuoteString('MD5 did not match!'))
 
     else:
         host.noop('file {0} has already been downloaded'.format(dest))
@@ -259,14 +265,16 @@ def line(
         replace = ''
 
     # Save commands for re-use in dynamic script when file not present at fact stage
-    echo_command_formatter = 'echo "{0}" >> {1}' if interpolate_variables else "echo '{0}' >> {1}"
     echo_command = make_formatted_string_command(
-        echo_command_formatter, line, path,
+        'echo {0} >> {1}',
+        '"{0}"'.format(line) if interpolate_variables else QuoteString(line),
+        QuoteString(path),
     )
 
     if backup:
+        backup_filename = '{0}.{1}'.format(path, get_timestamp())
         echo_command = StringCommand(make_formatted_string_command(
-            'cp {0} {0}.{1} && ', path, get_timestamp(),
+            'cp {0} {1} && ', QuoteString(path), QuoteString(backup_filename),
         ), echo_command)
 
     sed_replace_command = sed_replace(
@@ -284,15 +292,15 @@ def line(
             yield make_formatted_string_command(
                 '''
                     if [ -f '{target}' ]; then
-                        (grep {match_line} '{target}' && \
+                        ( grep {match_line} '{target}' && \
                         {sed_replace_command}) 2> /dev/null || \
-                        {echo_command};
+                        {echo_command} ;
                     else
-                        {echo_command};
+                        {echo_command} ;
                     fi
                 ''',
-                target=path,
-                match_line=match_line,
+                target=QuoteString(path),
+                match_line=QuoteString(match_line),
                 echo_command=echo_command,
                 sed_replace_command=sed_replace_command,
             )

tests/facts/directory/file.json

@@ -1,6 +1,6 @@
 {
     "arg": "/path/to/a/file",
-    "command": "! test -e /path/to/a/file || (stat -c 'user=%U group=%G mode=%A atime=%X mtime=%Y ctime=%Z size=%s %N' /path/to/a/file 2> /dev/null || stat -f 'user=%Su group=%Sg mode=%Sp atime=%a mtime=%m ctime=%c size=%z %N%SY' /path/to/a/file)",
+    "command": "! test -e /path/to/a/file || ( stat -c 'user=%U group=%G mode=%A atime=%X mtime=%Y ctime=%Z size=%s %N' /path/to/a/file 2> /dev/null || stat -f 'user=%Su group=%Sg mode=%Sp atime=%a mtime=%m ctime=%c size=%z %N%SY' /path/to/a/file )",
     "output": [
         "user=pyinfra group=pyinfra mode=-rwxrwxrwx atime=1594804767 mtime=1594804767 ctime=0 size=8 '/path/to/a/file'"
     ],

tests/facts/directory/link.json

@@ -1,6 +1,6 @@
 {
     "arg": "/home/pyinfra/mylink",
-    "command": "! test -e /home/pyinfra/mylink || (stat -c 'user=%U group=%G mode=%A atime=%X mtime=%Y ctime=%Z size=%s %N' /home/pyinfra/mylink 2> /dev/null || stat -f 'user=%Su group=%Sg mode=%Sp atime=%a mtime=%m ctime=%c size=%z %N%SY' /home/pyinfra/mylink)",
+    "command": "! test -e /home/pyinfra/mylink || ( stat -c 'user=%U group=%G mode=%A atime=%X mtime=%Y ctime=%Z size=%s %N' /home/pyinfra/mylink 2> /dev/null || stat -f 'user=%Su group=%Sg mode=%Sp atime=%a mtime=%m ctime=%c size=%z %N%SY' /home/pyinfra/mylink )",
     "output": [
         "user=root group=root mode=lrwxrwxrwx atime=1594804774 mtime=1594804770 ctime=0 size=6 '/home/pyinfra/mylink' -> 'file.txt'"
     ],

tests/facts/directory/valid.json

@@ -1,6 +1,6 @@
 {
     "arg": "/home/pyinfra/myd@-_ir",
-    "command": "! test -e /home/pyinfra/myd@-_ir || (stat -c 'user=%U group=%G mode=%A atime=%X mtime=%Y ctime=%Z size=%s %N' /home/pyinfra/myd@-_ir 2> /dev/null || stat -f 'user=%Su group=%Sg mode=%Sp atime=%a mtime=%m ctime=%c size=%z %N%SY' /home/pyinfra/myd@-_ir)",
+    "command": "! test -e /home/pyinfra/myd@-_ir || ( stat -c 'user=%U group=%G mode=%A atime=%X mtime=%Y ctime=%Z size=%s %N' /home/pyinfra/myd@-_ir 2> /dev/null || stat -f 'user=%Su group=%Sg mode=%Sp atime=%a mtime=%m ctime=%c size=%z %N%SY' /home/pyinfra/myd@-_ir )",
     "output": [
         "user=pyinfra group=pyinfra mode=drw-r--r-- atime=1594804583 mtime=1594804583 ctime=0 size=0 '/home/pyinfra/myd@-_ir'"
     ],

tests/facts/file/directory.json

@@ -1,6 +1,6 @@
 {
     "arg": "/home/pyinfra",
-    "command": "! test -e /home/pyinfra || (stat -c 'user=%U group=%G mode=%A atime=%X mtime=%Y ctime=%Z size=%s %N' /home/pyinfra 2> /dev/null || stat -f 'user=%Su group=%Sg mode=%Sp atime=%a mtime=%m ctime=%c size=%z %N%SY' /home/pyinfra)",
+    "command": "! test -e /home/pyinfra || ( stat -c 'user=%U group=%G mode=%A atime=%X mtime=%Y ctime=%Z size=%s %N' /home/pyinfra 2> /dev/null || stat -f 'user=%Su group=%Sg mode=%Sp atime=%a mtime=%m ctime=%c size=%z %N%SY' /home/pyinfra )",
     "output": [
         "user=root group=root mode=drw-r--r-- atime=1594804583 mtime=1594804583 ctime=0 size=0 '/home/pyinfra'"
     ],

tests/facts/file/invalid_output.json

@@ -1,6 +1,6 @@
 {
     "arg": "/home/pyinfra/fil-@_e.txt",
-    "command": "! test -e /home/pyinfra/fil-@_e.txt || (stat -c 'user=%U group=%G mode=%A atime=%X mtime=%Y ctime=%Z size=%s %N' /home/pyinfra/fil-@_e.txt 2> /dev/null || stat -f 'user=%Su group=%Sg mode=%Sp atime=%a mtime=%m ctime=%c size=%z %N%SY' /home/pyinfra/fil-@_e.txt)",
+    "command": "! test -e /home/pyinfra/fil-@_e.txt || ( stat -c 'user=%U group=%G mode=%A atime=%X mtime=%Y ctime=%Z size=%s %N' /home/pyinfra/fil-@_e.txt 2> /dev/null || stat -f 'user=%Su group=%Sg mode=%Sp atime=%a mtime=%m ctime=%c size=%z %N%SY' /home/pyinfra/fil-@_e.txt )",
     "output": [
         "not-gonna-match"
     ],

tests/facts/file/link.json

@@ -1,6 +1,6 @@
 {
     "arg": "/home/pyinfra/mylink",
-    "command": "! test -e /home/pyinfra/mylink || (stat -c 'user=%U group=%G mode=%A atime=%X mtime=%Y ctime=%Z size=%s %N' /home/pyinfra/mylink 2> /dev/null || stat -f 'user=%Su group=%Sg mode=%Sp atime=%a mtime=%m ctime=%c size=%z %N%SY' /home/pyinfra/mylink)",
+    "command": "! test -e /home/pyinfra/mylink || ( stat -c 'user=%U group=%G mode=%A atime=%X mtime=%Y ctime=%Z size=%s %N' /home/pyinfra/mylink 2> /dev/null || stat -f 'user=%Su group=%Sg mode=%Sp atime=%a mtime=%m ctime=%c size=%z %N%SY' /home/pyinfra/mylink )",
     "output": [
         "user=root group=root mode=lrwxrwxrwx atime=1594804774 mtime=1594804770 ctime=0 size=6 '/home/pyinfra/mylink' -> 'file.txt'"
     ],

tests/facts/file/valid.json

@@ -1,6 +1,6 @@
 {
     "arg": "/home/pyinfra/fil-@_e.txt",
-    "command": "! test -e /home/pyinfra/fil-@_e.txt || (stat -c 'user=%U group=%G mode=%A atime=%X mtime=%Y ctime=%Z size=%s %N' /home/pyinfra/fil-@_e.txt 2> /dev/null || stat -f 'user=%Su group=%Sg mode=%Sp atime=%a mtime=%m ctime=%c size=%z %N%SY' /home/pyinfra/fil-@_e.txt)",
+    "command": "! test -e /home/pyinfra/fil-@_e.txt || ( stat -c 'user=%U group=%G mode=%A atime=%X mtime=%Y ctime=%Z size=%s %N' /home/pyinfra/fil-@_e.txt 2> /dev/null || stat -f 'user=%Su group=%Sg mode=%Sp atime=%a mtime=%m ctime=%c size=%z %N%SY' /home/pyinfra/fil-@_e.txt )",
     "output": [
         "user=pyinfra group=domain users mode=-rwxrwx--- atime=1594804767 mtime=1594804767 ctime=0 size=8 '/home/pyinfra/fil-@_e.txt'"
     ],

tests/facts/file/valid_needs_quotes.json

@@ -0,0 +1,16 @@
+{
+    "arg": "fil () &&-@_e.txt",
+    "command": "! test -e 'fil () &&-@_e.txt' || ( stat -c 'user=%U group=%G mode=%A atime=%X mtime=%Y ctime=%Z size=%s %N' 'fil () &&-@_e.txt' 2> /dev/null || stat -f 'user=%Su group=%Sg mode=%Sp atime=%a mtime=%m ctime=%c size=%z %N%SY' 'fil () &&-@_e.txt' )",
+    "output": [
+        "user=pyinfra group=domain users mode=-rwxrwx--- atime=1594804767 mtime=1594804767 ctime=0 size=8 'fil () &&-@_e.txt'"
+    ],
+    "fact": {
+        "group": "domain users",
+        "user": "pyinfra",
+        "mode": 770,
+        "atime": "2020-07-15T09:19:27",
+        "mtime": "2020-07-15T09:19:27",
+        "ctime": "1970-01-01T00:00:00",
+        "size": 8
+    }
+}

tests/facts/file/valid_with_space.json

@@ -1,6 +1,6 @@
 {
     "arg": "/home/pyinfra dir/fil-@_e.txt",
-    "command": "! test -e /home/pyinfra\\ dir/fil-@_e.txt || (stat -c 'user=%U group=%G mode=%A atime=%X mtime=%Y ctime=%Z size=%s %N' /home/pyinfra\\ dir/fil-@_e.txt 2> /dev/null || stat -f 'user=%Su group=%Sg mode=%Sp atime=%a mtime=%m ctime=%c size=%z %N%SY' /home/pyinfra\\ dir/fil-@_e.txt)",
+    "command": "! test -e '/home/pyinfra dir/fil-@_e.txt' || ( stat -c 'user=%U group=%G mode=%A atime=%X mtime=%Y ctime=%Z size=%s %N' '/home/pyinfra dir/fil-@_e.txt' 2> /dev/null || stat -f 'user=%Su group=%Sg mode=%Sp atime=%a mtime=%m ctime=%c size=%z %N%SY' '/home/pyinfra dir/fil-@_e.txt' )",
     "output": [
         "user=pyinfra group=pyinfra mode=-rw-r--r-- atime=1594804348 mtime=1594804348 ctime=0 size=0 '/home/pyinfra dir/fil-@_e.txt'"
     ],