Merge pull request #142 from mattsb42-aws/dev-110

fix for Windows config file quote character handling

Author: mattsb42-aws

CHANGELOG.rst

@@ -2,6 +2,15 @@
 Changelog
 *********
 
+1.1.4
+=====
+
+Bugfixes
+--------
+* Fixed config file handling of quotes in Windows
+  `#110 <https://github.com/awslabs/aws-encryption-sdk-cli/issues/110>`_
+
+
 1.1.3
 =====
 

README.rst

@@ -355,12 +355,6 @@ Configuration Files
 As with any CLI where the configuration can get rather complex, you might want to use a configuration
 file to define some or all of your desired behavior.
 
-.. warning::
-
-   There is a `known issue with config file parsing in Windows`_. Including single or double quote
-   characters in a config file on Windows will fail until we fix this issue. Please let us know
-   if this impacts you in the linked GitHub issue.
-
 Configuration files are supported using Python's native `argparse file support`_, which allows
 you to write configuration files exactly as you would enter arguments in the shell. Configuration
 file references passed to ``aws-encryption-cli`` are identified by the ``@`` prefix and the
@@ -409,8 +403,9 @@ Configuration files can be referenced anywhere in ``aws-encryption-cli`` paramet
 
    aws-encryption-cli -e -i $INPUT_DIR -o $OUTPUT_DIR @master-key.conf @caching.conf --recursive
 
-Configuration files can have many lines, include comments using ``#``, and include
-references to other configuration files.
+Configuration files can have many lines, include comments using ``#``. Escape characters are
+platform-specific: ``\`` on Linux and MacOS and ````` on Windows. Configuration files may
+also include references to other configuration files.
 
 **my-encrypt.config**
 
@@ -568,4 +563,3 @@ Execution
 .. _named profile: http://docs.aws.amazon.com/cli/latest/userguide/cli-multiple-profiles.html
 .. _setuptools entry point: http://setuptools.readthedocs.io/en/latest/setuptools.html#dynamic-discovery-of-services-and-plugins
 .. _you must not specify a key: https://docs.aws.amazon.com/encryption-sdk/latest/developer-guide/crypto-cli-how-to.html#crypto-cli-master-key
-.. _known issue with config file parsing in Windows: https://github.com/awslabs/aws-encryption-sdk-cli/issues/110

src/aws_encryption_sdk_cli/internal/arg_parsing.py

@@ -20,8 +20,9 @@
 import shlex
 
 import aws_encryption_sdk
+import six
 
-from aws_encryption_sdk_cli.exceptions import BadUserArgumentError, ParameterParseError
+from aws_encryption_sdk_cli.exceptions import ParameterParseError
 from aws_encryption_sdk_cli.internal.identifiers import __version__, ALGORITHM_NAMES, DEFAULT_MASTER_KEY_PROVIDER
 from aws_encryption_sdk_cli.internal.logging_utils import LOGGER_NAME
 from aws_encryption_sdk_cli.internal.metadata import MetadataWriter
@@ -49,7 +50,7 @@ def __init__(self, *args, **kwargs):
         # I would rather not duplicate the typeshed's effort keeping it up to date.
         # https://github.com/python/typeshed/blob/master/stdlib/2and3/argparse.pyi#L27-L39
         self.__dummy_arguments = []
-        self.__is_posix = not any(platform.win32_ver())
+        self.__is_windows = any(platform.win32_ver())
         super(CommentIgnoringArgumentParser, self).__init__(*args, **kwargs)
 
     def add_dummy_redirect_argument(self, expected_name):
@@ -85,22 +86,34 @@ def add_argument(self, *args, **kwargs):
 
         return super(CommentIgnoringArgumentParser, self).add_argument(*args, **kwargs)
 
+    def __parse_line(self, arg_line):
+        # type: (ARGPARSE_TEXT) -> List[str]
+        """Parses a line of arguments into individual arguments intelligently for different platforms.
+        This differs from standard shlex behavior in that is supports escaping both single and double
+        quotes and on Windows platforms uses the Windows-native escape character "`".
+
+        :param str arg_line: Raw argument line
+        :returns: Parsed line members
+        :rtype: list of str
+        """
+        shlexer = shlex.shlex(six.StringIO(arg_line), posix=True)  # type: ignore #  shlex confuses mypy
+        shlexer.whitespace_split = True
+        shlexer.escapedquotes = '\'"'
+        if self.__is_windows:
+            shlexer.escape = '`'
+        return list(shlexer)  # type: ignore #  shlex confuses mypy
+
     def convert_arg_line_to_args(self, arg_line):
         # type: (ARGPARSE_TEXT) -> List[str]
-        """Applies whitespace stripping to individual arguments in each line and
-        drops both full-line and in-line comments.
+        """Converts a line of arguments into individual arguments, expanding user and environment variables.
+
+        :param str arg_line: Raw argument line
+        :returns: Converted line members
+        :rtype: list of str
         """
         converted_line = []
-        problematic_characters = ['\'', '"']
-        if not self.__is_posix and any([val in arg_line for val in problematic_characters]):
-            raise BadUserArgumentError(
-                'Config files containing characters {} are not currently supported:'
-                ' https://github.com/awslabs/aws-encryption-sdk-cli/issues/110'.format(repr(problematic_characters))
-            )
-        for arg in shlex.split(str(arg_line), posix=self.__is_posix):
+        for arg in self.__parse_line(arg_line):
             arg = arg.strip()
-            if arg.startswith('#'):
-                break
             user_arg = os.path.expanduser(arg)
             environ_arg = os.path.expandvars(user_arg)
             converted_line.append(environ_arg)

src/aws_encryption_sdk_cli/internal/identifiers.py

@@ -30,7 +30,7 @@
     'DEFAULT_MASTER_KEY_PROVIDER',
     'OperationResult'
 )
-__version__ = '1.1.3'  # type: str
+__version__ = '1.1.4'  # type: str
 
 #: Suffix added to output files if specific output filename is not specified.
 OUTPUT_SUFFIX = {

test/unit/test_arg_parsing.py

@@ -21,9 +21,8 @@
 from pytest_mock import mocker  # noqa pylint: disable=unused-import
 
 import aws_encryption_sdk_cli
-from aws_encryption_sdk_cli.exceptions import BadUserArgumentError, ParameterParseError
+from aws_encryption_sdk_cli.exceptions import ParameterParseError
 from aws_encryption_sdk_cli.internal import arg_parsing, identifiers, metadata
-from .unit_test_utils import is_windows
 
 pytestmark = [pytest.mark.unit, pytest.mark.local]
 
@@ -121,24 +120,64 @@ def test_comment_ignoring_argument_parser_convert_filename(patch_platform_win32_
     parser = arg_parsing.CommentIgnoringArgumentParser()
 
     if any(win32_version):
-        assert not parser._CommentIgnoringArgumentParser__is_posix
+        assert parser._CommentIgnoringArgumentParser__is_windows
     else:
-        assert parser._CommentIgnoringArgumentParser__is_posix
+        assert not parser._CommentIgnoringArgumentParser__is_windows
 
     parsed_line = [arg for arg in parser.convert_arg_line_to_args(expected_transform[0])]
     assert expected_transform[1] == parsed_line
 
 
+def build_convert_special_cases():
+    test_cases = []
+    escape_chars = {
+        False: '\\',
+        True: '`'
+    }
+    for plat_is_windows in (True, False):
+        test_cases.append((
+            '-o "example file with spaces surrounded by double quotes"',
+            ['-o', 'example file with spaces surrounded by double quotes'],
+            plat_is_windows
+        ))
+        test_cases.append((
+            "-o 'example file with spaces surrounded by single quotes'",
+            ['-o', 'example file with spaces surrounded by single quotes'],
+            plat_is_windows
+        ))
+        test_cases.append((
+            '-o "example with an inner {}" double quote"'.format(escape_chars[plat_is_windows]),
+            ['-o', 'example with an inner " double quote'],
+            plat_is_windows
+        ))
+        test_cases.append((
+            "-o 'example with an inner {}' single quote'".format(escape_chars[plat_is_windows]),
+            ['-o', "example with an inner ' single quote"],
+            plat_is_windows
+        ))
+    return test_cases
+
+
+@pytest.mark.parametrize('arg_line, expected_args, plat_is_windows', build_convert_special_cases())
+def test_comment_ignoring_argument_parser_convert_special_cases(arg_line, expected_args, plat_is_windows):
+    parser = arg_parsing.CommentIgnoringArgumentParser()
+    parser._CommentIgnoringArgumentParser__is_windows = plat_is_windows
+
+    actual_args = parser.convert_arg_line_to_args(arg_line)
+
+    assert actual_args == expected_args
+
+
 @pytest.mark.functional
 def test_f_comment_ignoring_argument_parser_convert_filename():
     # Actually checks against the current local system
     parser = arg_parsing.CommentIgnoringArgumentParser()
 
     if any(platform.win32_ver()):
-        assert not parser._CommentIgnoringArgumentParser__is_posix
+        assert parser._CommentIgnoringArgumentParser__is_windows
         expected_transform = NON_POSIX_FILEPATH
     else:
-        assert parser._CommentIgnoringArgumentParser__is_posix
+        assert not parser._CommentIgnoringArgumentParser__is_windows
         expected_transform = POSIX_FILEPATH
 
     parsed_line = [arg for arg in parser.convert_arg_line_to_args(expected_transform[0])]
@@ -218,15 +257,10 @@ def build_expected_good_args(from_file=False):  # pylint: disable=too-many-local
         {'some': 'data', 'not': 'secret'}
     ))
     if from_file:
-        good_args.append(pytest.param(
+        good_args.append((
             default_encrypt + ' -c "key with a space=value with a space"',
             'encryption_context',
-            {'key with a space': 'value with a space'},
-            marks=pytest.mark.xfail(
-                is_windows(),
-                reason='https://github.com/awslabs/aws-encryption-sdk-cli/issues/110',
-                strict=True
-            )
+            {'key with a space': 'value with a space'}
         ))
     else:
         good_args.append((
@@ -660,17 +694,3 @@ def test_process_encryption_context_encrypt_required_key_fail():
             raw_encryption_context=['encryption=context', 'with=values', 'key_3'],
             raw_required_encryption_context_keys=['key_1', 'key_2']
         )
-
-
-@pytest.mark.parametrize('arg_line', (
-    'single-quote \' line',
-    'double-quote " line'
-))
-def test_line_contains_problematic_characters(arg_line):
-    parser = arg_parsing.CommentIgnoringArgumentParser()
-    parser._CommentIgnoringArgumentParser__is_posix = False
-
-    with pytest.raises(BadUserArgumentError) as excinfo:
-        parser.convert_arg_line_to_args(arg_line)
-
-    excinfo.match(r'Config files containing characters *')

tox.ini

@@ -28,8 +28,7 @@ passenv =
 sitepackages = False
 deps =
     mock
-    pytest!=3.3.0
-    pytest-catchlog
+    pytest>3.3.0
     pytest-cov
     pytest-mock
     pytest-sugar