Password leak fix

svn/common_base.py

@@ -12,7 +12,16 @@ class CommonBase(object):
     def external_command(
             self, cmd, success_code=0, do_combine=False, return_binary=False,
             environment={}, wd=None, do_discard_stderr=True):
-        _LOGGER.debug("RUN: %s" % (cmd,))
+
+        # the cmd may contain a user password that should not be logged, therefore we make a clean version of the
+        # comamnd for logging purposes
+        if '--password' in cmd:
+            safe_logging_comand = cmd.copy()
+            safe_logging_comand[cmd.index('--password') + 1] = '**************'
+        else:
+            safe_logging_comand = cmd
+
+        _LOGGER.debug("RUN: %s" % (safe_logging_comand,))
 
         env = os.environ.copy()
 
@@ -49,7 +58,7 @@ def external_command(
 
             raise svn.exception.SvnException(
                 "Command failed with ({}): {}\nSTDOUT:\n\n{}\nSTDERR:\n\n{}".format(
-                return_code, cmd, stdout, stderr))
+                return_code, safe_logging_comand, stdout, stderr))
 
         if return_binary is True or do_combine is True:
             return stdout

tests/test_remote.py

@@ -41,6 +41,28 @@ def test_checkout(self):
         """
         svn.remote.RemoteClient(self.test_svn_url).checkout('trial')
         self.assertTrue(os.path.exists('trial'))
+
+        def test_password_logging(self):
+        """
+        Testing password are being masked out of the log and exception messages
+        :return:
+        """
+        with self.assertLogs(logger=None, level=logging.DEBUG) as cap_logging:
+            with self.assertRaises(svn.exception.SvnException) as cap_exec:
+                # this call will fail because it is trying to check out the file from the fake url
+                svn.remote.RemoteClient(self.test_fake_url,
+                                        username='fake_username',
+                                        password='fake_password').checkout('trial')
+
+        # make sure the password was present in the log but has been replaced
+        self.assertIn('--password', str(cap_exec.exception))
+        self.assertNotIn('fake_password', str(cap_exec.exception))
+
+        # there may be multiple log entries generated by test, need to check that any which include the '--password'
+        # command line argument do not feature the password in the clear
+        for log_line in cap_logging.output:
+            if '--password' in log_line:
+                self.assertNotIn('fake_password', log_line)
 
     def test_remove(self):
         with svn.test_support.temp_repo() as (_, rc):