From 85fe2735b7c9119804813bcbbdd8d14018291ed3 Mon Sep 17 00:00:00 2001 From: Glenn Matthews Date: Wed, 4 May 2022 12:48:09 -0400 Subject: [PATCH] Fix #1284: strip usernames from URLs as well as passwords --- git/exc.py | 7 ++++--- git/util.py | 20 +++++++++++++------- test/test_exc.py | 9 ++++++--- test/test_util.py | 30 +++++++++++++++++++++++------- 4 files changed, 46 insertions(+), 20 deletions(-) diff --git a/git/exc.py b/git/exc.py index e8ff784c7..045ea9d27 100644 --- 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 index 6e6f09557..0711265a6 100644 --- 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,11 +413,12 @@ 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): @@ -424,12 +426,16 @@ def remove_password_if_present(cmdline: Sequence[str]) -> List[str]: 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 index f16498ab5..c77be7824 100644 --- 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', 'ελληνικα', 'args'), ('θνιψοδε', 'κι', 'αλλα', 'strange', 'args'), ('θνιψοδε', 'κι', 'αλλα', '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 index 3961ff356..a213b46c9 100644 --- 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)