Fix erroneous carriage return character (\r) in the output of remote shell script and command actions

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
 -----------------------

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)

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

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)