diff --git a/git/cmd.py b/git/cmd.py index 050efaedf..796066072 100644 --- a/git/cmd.py +++ b/git/cmd.py @@ -28,7 +28,7 @@ is_win, ) from git.exc import CommandError -from git.util import is_cygwin_git, cygpath, expand_path +from git.util import is_cygwin_git, cygpath, expand_path, remove_password_if_present from .exc import ( GitCommandError, @@ -82,8 +82,8 @@ def pump_stream(cmdline, name, stream, is_decode, handler): line = line.decode(defenc) handler(line) except Exception as ex: - log.error("Pumping %r of cmd(%s) failed due to: %r", name, cmdline, ex) - raise CommandError(['<%s-pump>' % name] + cmdline, ex) from ex + log.error("Pumping %r of cmd(%s) failed due to: %r", name, remove_password_if_present(cmdline), ex) + raise CommandError(['<%s-pump>' % name] + remove_password_if_present(cmdline), ex) from ex finally: stream.close() @@ -405,7 +405,7 @@ def read_all_from_possibly_closed_stream(stream): if status != 0: errstr = read_all_from_possibly_closed_stream(self.proc.stderr) log.debug('AutoInterrupt wait stderr: %r' % (errstr,)) - raise GitCommandError(self.args, status, errstr) + raise GitCommandError(remove_password_if_present(self.args), status, errstr) # END status handling return status # END auto interrupt @@ -638,7 +638,7 @@ def execute(self, command, :param env: A dictionary of environment variables to be passed to `subprocess.Popen`. - + :param max_chunk_size: Maximum number of bytes in one chunk of data passed to the output_stream in one invocation of write() method. If the given number is not positive then @@ -682,8 +682,10 @@ def execute(self, command, :note: If you add additional keyword arguments to the signature of this method, you must update the execute_kwargs tuple housed in this module.""" + # Remove password for the command if present + redacted_command = remove_password_if_present(command) if self.GIT_PYTHON_TRACE and (self.GIT_PYTHON_TRACE != 'full' or as_process): - log.info(' '.join(command)) + log.info(' '.join(redacted_command)) # Allow the user to have the command executed in their working dir. cwd = self._working_dir or os.getcwd() @@ -704,7 +706,7 @@ def execute(self, command, if is_win: cmd_not_found_exception = OSError if kill_after_timeout: - raise GitCommandError(command, '"kill_after_timeout" feature is not supported on Windows.') + raise GitCommandError(redacted_command, '"kill_after_timeout" feature is not supported on Windows.') else: if sys.version_info[0] > 2: cmd_not_found_exception = FileNotFoundError # NOQA # exists, flake8 unknown @UndefinedVariable @@ -719,7 +721,7 @@ def execute(self, command, if istream: istream_ok = "" log.debug("Popen(%s, cwd=%s, universal_newlines=%s, shell=%s, istream=%s)", - command, cwd, universal_newlines, shell, istream_ok) + redacted_command, cwd, universal_newlines, shell, istream_ok) try: proc = Popen(command, env=env, @@ -735,7 +737,7 @@ def execute(self, command, **subprocess_kwargs ) except cmd_not_found_exception as err: - raise GitCommandNotFound(command, err) from err + raise GitCommandNotFound(redacted_command, err) from err if as_process: return self.AutoInterrupt(proc, command) @@ -785,7 +787,7 @@ def _kill_process(pid): watchdog.cancel() if kill_check.isSet(): stderr_value = ('Timeout: the command "%s" did not complete in %d ' - 'secs.' % (" ".join(command), kill_after_timeout)) + 'secs.' % (" ".join(redacted_command), kill_after_timeout)) if not universal_newlines: stderr_value = stderr_value.encode(defenc) # strip trailing "\n" @@ -809,7 +811,7 @@ def _kill_process(pid): proc.stderr.close() if self.GIT_PYTHON_TRACE == 'full': - cmdstr = " ".join(command) + cmdstr = " ".join(redacted_command) def as_text(stdout_value): return not output_stream and safe_decode(stdout_value) or '' @@ -825,7 +827,7 @@ def as_text(stdout_value): # END handle debug printing if with_exceptions and status != 0: - raise GitCommandError(command, status, stderr_value, stdout_value) + raise GitCommandError(redacted_command, status, stderr_value, stdout_value) if isinstance(stdout_value, bytes) and stdout_as_string: # could also be output_stream stdout_value = safe_decode(stdout_value) diff --git a/git/repo/base.py b/git/repo/base.py index 8f1ef0a6e..e68bc0779 100644 --- a/git/repo/base.py +++ b/git/repo/base.py @@ -26,7 +26,7 @@ from git.objects import Submodule, RootModule, Commit from git.refs import HEAD, Head, Reference, TagReference from git.remote import Remote, add_progress, to_progress_instance -from git.util import Actor, finalize_process, decygpath, hex_to_bin, expand_path +from git.util import Actor, finalize_process, decygpath, hex_to_bin, expand_path, remove_password_if_present import os.path as osp from .fun import rev_parse, is_git_dir, find_submodule_git_dir, touch, find_worktree_git_dir @@ -969,7 +969,10 @@ def _clone(cls, git, url, path, odb_default_type, progress, multi_options=None, handle_process_output(proc, None, progress.new_message_handler(), finalize_process, decode_streams=False) else: (stdout, stderr) = proc.communicate() - log.debug("Cmd(%s)'s unused stdout: %s", getattr(proc, 'args', ''), stdout) + cmdline = getattr(proc, 'args', '') + cmdline = remove_password_if_present(cmdline) + + log.debug("Cmd(%s)'s unused stdout: %s", cmdline, stdout) finalize_process(proc, stderr=stderr) # our git command could have a different working dir than our actual diff --git a/git/util.py b/git/util.py index 04c967891..d0aec5ae9 100644 --- a/git/util.py +++ b/git/util.py @@ -16,6 +16,7 @@ from sys import maxsize import time from unittest import SkipTest +from urllib.parse import urlsplit, urlunsplit from gitdb.util import (# NOQA @IgnorePep8 make_sha, @@ -338,6 +339,34 @@ def expand_path(p, expand_vars=True): except Exception: return None + +def remove_password_if_present(cmdline): + """ + Parse any command line argument and if on of the element is an URL with a + password, replace it 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. + """ + 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: + continue + + edited_url = url._replace( + netloc=url.netloc.replace(url.password, "*****")) + new_cmdline[index] = urlunsplit(edited_url) + except ValueError: + # This is not a valid URL + continue + return new_cmdline + + #} END utilities #{ Classes diff --git a/test/test_repo.py b/test/test_repo.py index d5ea8664a..8dc178337 100644 --- a/test/test_repo.py +++ b/test/test_repo.py @@ -238,6 +238,21 @@ def test_clone_from_with_path_contains_unicode(self): except UnicodeEncodeError: self.fail('Raised UnicodeEncodeError') + @with_rw_directory + def test_leaking_password_in_clone_logs(self, rw_dir): + password = "fakepassword1234" + try: + Repo.clone_from( + url="https://fakeuser:{}@fakerepo.example.com/testrepo".format( + password), + to_path=rw_dir) + except GitCommandError as err: + assert password not in str(err), "The error message '%s' should not contain the password" % err + # Working example from a blank private project + Repo.clone_from( + url="https://gitlab+deploy-token-392045:mLWhVus7bjLsy8xj8q2V@gitlab.com/mercierm/test_git_python", + to_path=rw_dir) + @with_rw_repo('HEAD') def test_max_chunk_size(self, repo): class TestOutputStream(TestBase): diff --git a/test/test_util.py b/test/test_util.py index 5eba6c500..ddc5f628f 100644 --- a/test/test_util.py +++ b/test/test_util.py @@ -30,7 +30,8 @@ Actor, IterableList, cygpath, - decygpath + decygpath, + remove_password_if_present, ) @@ -322,3 +323,20 @@ def test_pickle_tzoffset(self): t2 = pickle.loads(pickle.dumps(t1)) self.assertEqual(t1._offset, t2._offset) self.assertEqual(t1._name, t2._name) + + def test_remove_password_from_command_line(self): + password = "fakepassword1234" + url_with_pass = "https://fakeuser:{}@fakerepo.example.com/testrepo".format(password) + url_without_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"] + + redacted_cmd_1 = remove_password_if_present(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 password in " ".join(cmd_1) + assert cmd_2 == remove_password_if_present(cmd_2) + assert cmd_3 == remove_password_if_present(cmd_3)