From 8a84ef80e26850b1b174932ed7d0a50cec8b1452 Mon Sep 17 00:00:00 2001 From: Toshio Kuratomi Date: Wed, 11 May 2016 17:54:01 -0700 Subject: [PATCH] Strip junk after JSON return. (#15822) Fixes #15601 --- lib/ansible/plugins/action/__init__.py | 52 ++++++++++++++++++------ test/units/plugins/action/test_action.py | 41 +++++++++++++++++++ 2 files changed, 81 insertions(+), 12 deletions(-) diff --git a/lib/ansible/plugins/action/__init__.py b/lib/ansible/plugins/action/__init__.py index 9a0e99fb8d6..7a8ee2b4bc8 100644 --- a/lib/ansible/plugins/action/__init__.py +++ b/lib/ansible/plugins/action/__init__.py @@ -324,7 +324,7 @@ class ActionBase(with_metaclass(ABCMeta, object)): # contain a path to a tmp dir but doesn't know if it needs to # exist or not. If there's no path, then there's no need for us # to do work - self._display.debug('_fixup_perms called with remote_path==None. Sure this is correct?') + display.debug('_fixup_perms called with remote_path==None. Sure this is correct?') return remote_path if self._play_context.become and self._play_context.become_user not in ('root', remote_user): @@ -360,7 +360,7 @@ class ActionBase(with_metaclass(ABCMeta, object)): if C.ALLOW_WORLD_READABLE_TMPFILES: # fs acls failed -- do things this insecure way only # if the user opted in in the config file - self._display.warning('Using world-readable permissions for temporary files Ansible needs to create when becoming an unprivileged user which may be insecure. For information on securing this, see https://docs.ansible.com/ansible/become.html#becoming-an-unprivileged-user') + display.warning('Using world-readable permissions for temporary files Ansible needs to create when becoming an unprivileged user which may be insecure. For information on securing this, see https://docs.ansible.com/ansible/become.html#becoming-an-unprivileged-user') res = self._remote_chmod('a+%s' % mode, remote_path, recursive=recursive) if res['rc'] != 0: raise AnsibleError('Failed to set file mode on remote files (rc: {0}, err: {1})'.format(res['rc'], res['stderr'])) @@ -480,21 +480,49 @@ class ActionBase(with_metaclass(ABCMeta, object)): else: return initial_fragment - def _filter_leading_non_json_lines(self, data): + @staticmethod + def _filter_non_json_lines(data): ''' Used to avoid random output from SSH at the top of JSON output, like messages from tcagetattr, or where dropbear spews MOTD on every single command (which is nuts). - need to filter anything which starts not with '{', '[', ', '=' or is an empty line. - filter only leading lines since multiline JSON is valid. + need to filter anything which does not start with '{', '[', or is an empty line. + Have to be careful how we filter trailing junk as multiline JSON is valid. ''' - idx = 0 - for line in data.splitlines(True): - if line.startswith((u'{', u'[')): + # Filter initial junk + lines = data.splitlines() + for start, line in enumerate(lines): + line = line.strip() + if line.startswith(u'{'): + endchar = u'}' break - idx = idx + len(line) + elif line.startswith(u'['): + endchar = u']' + break + else: + display.debug('No start of json char found') + raise ValueError('No start of json char found') + + # Filter trailing junk + lines = lines[start:] + lines.reverse() + for end, line in enumerate(lines): + if line.strip().endswith(endchar): + break + else: + display.debug('No end of json char found') + raise ValueError('No end of json char found') + + if end < len(lines) - 1: + # Trailing junk is uncommon and can point to things the user might + # want to change. So print a warning if we find any + trailing_junk = lines[:end] + trailing_junk.reverse() + display.warning('Module invocation had junk after the JSON data: %s' % '\n'.join(trailing_junk)) - return data[idx:] + lines = lines[end:] + lines.reverse() + return '\n'.join(lines) def _strip_success_message(self, data): ''' @@ -539,7 +567,7 @@ class ActionBase(with_metaclass(ABCMeta, object)): module_args['_ansible_diff'] = self._play_context.diff # let module know our verbosity - module_args['_ansible_verbosity'] = self._display.verbosity + module_args['_ansible_verbosity'] = display.verbosity (module_style, shebang, module_data) = self._configure_module(module_name=module_name, module_args=module_args, task_vars=task_vars) if not shebang: @@ -627,7 +655,7 @@ class ActionBase(with_metaclass(ABCMeta, object)): def _parse_returned_data(self, res): try: - data = json.loads(self._filter_leading_non_json_lines(res.get('stdout', u''))) + data = json.loads(self._filter_non_json_lines(res.get('stdout', u''))) except ValueError: # not valid json, lets try to capture error data = dict(failed=True, parsed=False) diff --git a/test/units/plugins/action/test_action.py b/test/units/plugins/action/test_action.py index e9b608790f5..2a9bb55693d 100644 --- a/test/units/plugins/action/test_action.py +++ b/test/units/plugins/action/test_action.py @@ -33,6 +33,8 @@ try: except ImportError: import __builtin__ as builtins +from nose.tools import eq_, raises + from ansible.release import __version__ as ansible_version from ansible import constants as C from ansible.compat.six import text_type @@ -630,3 +632,42 @@ class TestActionBase(unittest.TestCase): play_context.make_become_cmd.assert_called_once_with("ECHO SAME", executable=None) finally: C.BECOME_ALLOW_SAME_USER = become_allow_same_user + +# Note: Using nose's generator test cases here so we can't inherit from +# unittest.TestCase +class TestFilterNonJsonLines(object): + parsable_cases = ( + (u'{"hello": "world"}', u'{"hello": "world"}'), + (u'{"hello": "world"}\n', u'{"hello": "world"}'), + (u'{"hello": "world"} ', u'{"hello": "world"} '), + (u'{"hello": "world"} \n', u'{"hello": "world"} '), + (u'Message of the Day\n{"hello": "world"}', u'{"hello": "world"}'), + (u'{"hello": "world"}\nEpilogue', u'{"hello": "world"}'), + (u'Several\nStrings\nbefore\n{"hello": "world"}\nAnd\nAfter\n', u'{"hello": "world"}'), + (u'{"hello": "world",\n"olá": "mundo"}', u'{"hello": "world",\n"olá": "mundo"}'), + (u'\nPrecedent\n{"hello": "world",\n"olá": "mundo"}\nAntecedent', u'{"hello": "world",\n"olá": "mundo"}'), + ) + + unparsable_cases = ( + u'No json here', + u'"olá": "mundo"', + u'{"No json": "ending"', + u'{"wrong": "ending"]', + u'["wrong": "ending"}', + ) + + def check_filter_non_json_lines(self, stdout_line, parsed): + eq_(parsed, ActionBase._filter_non_json_lines(stdout_line)) + + def test_filter_non_json_lines(self): + for stdout_line, parsed in self.parsable_cases: + yield self.check_filter_non_json_lines, stdout_line, parsed + + @raises(ValueError) + def check_unparsable_filter_non_json_lines(self, stdout_line): + ActionBase._filter_non_json_lines(stdout_line) + + def test_unparsable_filter_non_json_lines(self): + for stdout_line in self.unparsable_cases: + yield self.check_unparsable_filter_non_json_lines, stdout_line +