diff --git a/CHANGELOG.rst b/CHANGELOG.rst index d9ad591c02..66fcfac5cd 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -97,7 +97,10 @@ Fixed is acquired before write operations to avoid write conflicts. (bug fix) Stackstorm/orquesta#125 * Fix a bug with some API endpoints returning 500 internal server error when an exception contained unicode data. (bug fix) #4598 -* Fix the ``st2 workflow inspect`` command so it correctly passes authentication token. (bug fix) #4615 +* Fix the ``st2 workflow inspect`` command so it correctly passes authentication token. (bug fix) + #4615 +* Fix an issue with new line characters (``\n``) being converted to ``\r\n`` in remote shell + command and script actions which use sudo. (bug fix) #4623 2.10.4 - March 15, 2019 ----------------------- diff --git a/st2common/st2common/runners/paramiko_ssh.py b/st2common/st2common/runners/paramiko_ssh.py index fdca3dc19a..ee9c327133 100644 --- a/st2common/st2common/runners/paramiko_ssh.py +++ b/st2common/st2common/runners/paramiko_ssh.py @@ -31,6 +31,7 @@ from st2common.log import logging from st2common.util.misc import strip_shell_chars +from st2common.util.misc import sanitize_output from st2common.util.shell import quote_unix from st2common.constants.runners import DEFAULT_SSH_PORT, REMOTE_RUNNER_PRIVATE_KEY_HEADER import six @@ -363,7 +364,13 @@ def run(self, cmd, timeout=None, quote=False, call_line_handler_func=False): if cmd.startswith('sudo'): # Note that fabric does this as well. If you set pty, stdout and stderr # streams will be combined into one. + # NOTE: If pty is used, every new line character \n will be converted to \r\n which + # isn't desired. Because of that we sanitize the output and replace \r\n with \n at the + # bottom of this method + uses_pty = True chan.get_pty() + else: + uses_pty = False chan.exec_command(cmd) stdout = StringIO() @@ -404,8 +411,8 @@ def run(self, cmd, timeout=None, quote=False, call_line_handler_func=False): # TODO: Is this the right way to clean up? chan.close() - stdout = strip_shell_chars(stdout.getvalue()) - stderr = strip_shell_chars(stderr.getvalue()) + stdout = sanitize_output(stdout.getvalue(), uses_pty=uses_pty) + stderr = sanitize_output(stderr.getvalue(), uses_pty=uses_pty) raise SSHCommandTimeoutError(cmd=cmd, timeout=timeout, stdout=stdout, stderr=stderr) @@ -434,8 +441,8 @@ def run(self, cmd, timeout=None, quote=False, call_line_handler_func=False): # Receive the exit status code of the command we ran. status = chan.recv_exit_status() - stdout = strip_shell_chars(stdout.getvalue()) - stderr = strip_shell_chars(stderr.getvalue()) + stdout = sanitize_output(stdout.getvalue(), uses_pty=uses_pty) + stderr = sanitize_output(stderr.getvalue(), uses_pty=uses_pty) extra = {'_status': status, '_stdout': stdout, '_stderr': stderr} self.logger.debug('Command finished', extra=extra) diff --git a/st2common/st2common/util/misc.py b/st2common/st2common/util/misc.py index 02d8f31513..ab9197a207 100644 --- a/st2common/st2common/util/misc.py +++ b/st2common/st2common/util/misc.py @@ -28,9 +28,12 @@ __all__ = [ 'prefix_dict_keys', 'compare_path_file_name', - 'lowercase_value', 'get_field_name_from_mongoengine_error', + 'sanitize_output', + 'strip_shell_chars', + 'rstrip_last_char', + 'lowercase_value' ] @@ -67,6 +70,30 @@ def compare_path_file_name(file_path_a, file_path_b): return (file_name_a > file_name_b) - (file_name_a < file_name_b) +def sanitize_output(input_str, uses_pty=False): + """ + Function which sanitizes paramiko output (stdout / stderr). + + It strips trailing carriage return and new line characters and if pty is used, it also replaces + all occurrences of \r\n with \n. + + By default when pty is used, all \n characters are convered to \r\n and that's not desired + in our remote runner action output. + + :param input_str: Input string to be sanitized. + :type input_str: ``str`` + + :rtype: ``str`` + + """ + output = strip_shell_chars(input_str) + + if uses_pty: + output = output.replace('\r\n', '\n') + + return output + + def strip_shell_chars(input_str): """ Strips the last '\r' or '\n' or '\r\n' string at the end of diff --git a/st2common/tests/unit/test_misc_utils.py b/st2common/tests/unit/test_misc_utils.py index 32f1ac48d7..599f41d246 100644 --- a/st2common/tests/unit/test_misc_utils.py +++ b/st2common/tests/unit/test_misc_utils.py @@ -21,6 +21,7 @@ from st2common.util.misc import rstrip_last_char from st2common.util.misc import strip_shell_chars from st2common.util.misc import lowercase_value +from st2common.util.misc import sanitize_output from st2common.util.ujson import fast_deepcopy __all__ = [ @@ -94,3 +95,45 @@ def test_fast_deepcopy_success(self): result = fast_deepcopy(value) self.assertEqual(result, value) self.assertEqual(result, expected_value) + + def test_sanitize_output_use_pyt_false(self): + # pty is not used, \r\n shouldn't be replaced with \n + input_strs = [ + 'foo', + 'foo\n', + 'foo\r\n', + 'foo\nbar\nbaz\n', + 'foo\r\nbar\r\nbaz\r\n', + ] + expected = [ + 'foo', + 'foo', + 'foo', + 'foo\nbar\nbaz', + 'foo\r\nbar\r\nbaz', + ] + + for input_str, expected_output in zip(input_strs, expected): + output = sanitize_output(input_str, uses_pty=False) + self.assertEqual(expected_output, output) + + def test_sanitize_output_use_pyt_true(self): + # pty is used, \r\n should be replaced with \n + input_strs = [ + 'foo', + 'foo\n', + 'foo\r\n', + 'foo\nbar\nbaz\n', + 'foo\r\nbar\r\nbaz\r\n', + ] + expected = [ + 'foo', + 'foo', + 'foo', + 'foo\nbar\nbaz', + 'foo\nbar\nbaz', + ] + + for input_str, expected_output in zip(input_strs, expected): + output = sanitize_output(input_str, uses_pty=True) + self.assertEqual(expected_output, output)