Fix #1284: strip usernames from URLs as well as passwords

glennmatthews · glennmatthews · commit 85fe2735b7c9 · 2022-05-04T12:48:09.000-04:00
diff --git a/git/exc.py b/git/exc.py
@@ -8,6 +8,7 @@
 from gitdb.exc import BadName  # NOQA @UnusedWildImport skipcq: PYL-W0401, PYL-W0614
 from gitdb.exc import *     # NOQA @UnusedWildImport skipcq: PYL-W0401, PYL-W0614
 from git.compat import safe_decode
+from git.util import remove_password_if_present
 
 # typing ----------------------------------------------------
 
@@ -54,7 +55,7 @@ def __init__(self, command: Union[List[str], Tuple[str, ...], str],
                  stdout: Union[bytes, str, None] = None) -> None:
         if not isinstance(command, (tuple, list)):
             command = command.split()
-        self.command = command
+        self.command = remove_password_if_present(command)
         self.status = status
         if status:
             if isinstance(status, Exception):
@@ -66,8 +67,8 @@ def __init__(self, command: Union[List[str], Tuple[str, ...], str],
                     s = safe_decode(str(status))
                     status = "'%s'" % s if isinstance(status, str) else s
 
-        self._cmd = safe_decode(command[0])
-        self._cmdline = ' '.join(safe_decode(i) for i in command)
+        self._cmd = safe_decode(self.command[0])
+        self._cmdline = ' '.join(safe_decode(i) for i in self.command)
         self._cause = status and " due to: %s" % status or "!"
         stdout_decode = safe_decode(stdout)
         stderr_decode = safe_decode(stderr)
diff --git a/git/util.py b/git/util.py
@@ -5,7 +5,6 @@
 # the BSD License: http://www.opensource.org/licenses/bsd-license.php
 
 from abc import abstractmethod
-from .exc import InvalidGitRepositoryError
 import os.path as osp
 from .compat import is_win
 import contextlib
@@ -94,6 +93,8 @@ def unbare_repo(func: Callable[..., T]) -> Callable[..., T]:
     """Methods with this decorator raise InvalidGitRepositoryError if they
     encounter a bare repository"""
 
+    from .exc import InvalidGitRepositoryError
+
     @wraps(func)
     def wrapper(self: 'Remote', *args: Any, **kwargs: Any) -> T:
         if self.repo.bare:
@@ -412,24 +413,29 @@ def expand_path(p: Union[None, PathLike], expand_vars: bool = True) -> Optional[
 def remove_password_if_present(cmdline: Sequence[str]) -> List[str]:
     """
     Parse any command line argument and if on of the element is an URL with a
-    password, replace it by stars (in-place).
+    username and/or password, replace them by stars (in-place).
 
     If nothing found just returns the command line as-is.
 
-    This should be used for every log line that print a command line.
+    This should be used for every log line that print a command line, as well as
+    exception messages.
     """
     new_cmdline = []
     for index, to_parse in enumerate(cmdline):
         new_cmdline.append(to_parse)
         try:
             url = urlsplit(to_parse)
             # Remove password from the URL if present
-            if url.password is None:
+            if url.password is None and url.username is None:
                 continue
 
-            edited_url = url._replace(
-                netloc=url.netloc.replace(url.password, "*****"))
-            new_cmdline[index] = urlunsplit(edited_url)
+            if url.password is not None:
+                url = url._replace(
+                    netloc=url.netloc.replace(url.password, "*****"))
+            if url.username is not None:
+                url = url._replace(
+                    netloc=url.netloc.replace(url.username, "*****"))
+            new_cmdline[index] = urlunsplit(url)
         except ValueError:
             # This is not a valid URL
             continue
diff --git a/test/test_exc.py b/test/test_exc.py
@@ -22,6 +22,7 @@
     HookExecutionError,
     RepositoryDirtyError,
 )
+from git.util import remove_password_if_present
 from test.lib import TestBase
 
 import itertools as itt
@@ -34,6 +35,7 @@
     ('cmd', '&epsilon;&lambda;&lambda;&eta;&nu;&iota;&kappa;&alpha;', 'args'),
     ('&theta;&nu;&iota;&psi;&omicron;&delta;&epsilon;', '&kappa;&iota;', '&alpha;&lambda;&lambda;&alpha;', 'strange', 'args'),
     ('&theta;&nu;&iota;&psi;&omicron;&delta;&epsilon;', '&kappa;&iota;', '&alpha;&lambda;&lambda;&alpha;', 'non-unicode', 'args'),
+    ('git', 'clone', '-v', 'https://fakeuser:fakepassword1234@fakerepo.example.com/testrepo'),
 )
 _causes_n_substrings = (
     (None,                      None),                          # noqa: E241 @IgnorePep8
@@ -81,7 +83,7 @@ def test_CommandError_unicode(self, case):
         self.assertIsNotNone(c._msg)
         self.assertIn('  cmdline: ', s)
 
-        for a in argv:
+        for a in remove_password_if_present(argv):
             self.assertIn(a, s)
 
         if not cause:
@@ -137,14 +139,15 @@ def test_GitCommandNotFound(self, init_args):
     @ddt.data(
         (['cmd1'], None),
         (['cmd1'], "some cause"),
-        (['cmd1'], Exception()),
+        (['cmd1', 'https://fakeuser@fakerepo.example.com/testrepo'], Exception()),
     )
     def test_GitCommandError(self, init_args):
         argv, cause = init_args
         c = GitCommandError(argv, cause)
         s = str(c)
 
-        self.assertIn(argv[0], s)
+        for arg in remove_password_if_present(argv):
+            self.assertIn(arg, s)
         if cause:
             self.assertIn(' failed due to: ', s)
             self.assertIn(str(cause), s)
diff --git a/test/test_util.py b/test/test_util.py
@@ -343,18 +343,34 @@ def test_pickle_tzoffset(self):
         self.assertEqual(t1._name, t2._name)
 
     def test_remove_password_from_command_line(self):
+        username = "fakeuser"
         password = "fakepassword1234"
-        url_with_pass = "https://fakeuser:{}@fakerepo.example.com/testrepo".format(password)
-        url_without_pass = "https://fakerepo.example.com/testrepo"
+        url_with_user_and_pass = "https://{}:{}@fakerepo.example.com/testrepo".format(username, password)
+        url_with_user = "https://{}@fakerepo.example.com/testrepo".format(username)
+        url_with_pass = "https://:{}@fakerepo.example.com/testrepo".format(password)
+        url_without_user_or_pass = "https://fakerepo.example.com/testrepo"
 
-        cmd_1 = ["git", "clone", "-v", url_with_pass]
-        cmd_2 = ["git", "clone", "-v", url_without_pass]
-        cmd_3 = ["no", "url", "in", "this", "one"]
+        cmd_1 = ["git", "clone", "-v", url_with_user_and_pass]
+        cmd_2 = ["git", "clone", "-v", url_with_user]
+        cmd_3 = ["git", "clone", "-v", url_with_pass]
+        cmd_4 = ["git", "clone", "-v", url_without_user_or_pass]
+        cmd_5 = ["no", "url", "in", "this", "one"]
 
         redacted_cmd_1 = remove_password_if_present(cmd_1)
+        assert username not in " ".join(redacted_cmd_1)
         assert password not in " ".join(redacted_cmd_1)
         # Check that we use a copy
         assert cmd_1 is not redacted_cmd_1
+        assert username in " ".join(cmd_1)
         assert password in " ".join(cmd_1)
-        assert cmd_2 == remove_password_if_present(cmd_2)
-        assert cmd_3 == remove_password_if_present(cmd_3)
+
+        redacted_cmd_2 = remove_password_if_present(cmd_2)
+        assert username not in " ".join(redacted_cmd_2)
+        assert password not in " ".join(redacted_cmd_2)
+
+        redacted_cmd_3 = remove_password_if_present(cmd_3)
+        assert username not in " ".join(redacted_cmd_3)
+        assert password not in " ".join(redacted_cmd_3)
+
+        assert cmd_4 == remove_password_if_present(cmd_4)
+        assert cmd_5 == remove_password_if_present(cmd_5)
