8000 Merge pull request #1198 from RyaxTech/replace-password-in-uri-by-stars · CoderboticsAI/GitPython@d1297f6 · GitHub
[go: up one dir, main page]
Skip to content

Commit d1297f6

Browse files
ByronByron
authored
Merge pull request gitpython-developers#1198 from RyaxTech/replace-password-in-uri-by-stars
Replace password in URI by stars if present to avoid leaking secrets in logs
2 parents d906f31 + d283c83 commit d1297f6

File tree

5 files changed

+81
-14
lines changed

5 files changed

+81
-14
lines changed

‎git/cmd.py

Lines changed: 13 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@
2929
is_win,
3030
)
3131
from git.exc import CommandError
32-
from git.util import is_cygwin_git, cygpath, expand_path
32+
from git.util import is_cygwin_git, cygpath, expand_path, remove_password_if_present
3333

3434
from .exc import (
3535
GitCommandError,
@@ -83,8 +83,8 @@ def pump_stream(cmdline, name, stream, is_decode, handler):
8383
line = line.decode(defenc)
8484
handler(line)
8585
except Exception as ex:
86-
log.error("Pumping %r of cmd(%s) failed due to: %r", name, cmdline, ex)
87-
raise CommandError(['<%s-pump>' % name] + cmdline, ex) from ex
86+
log.error("Pumping %r of cmd(%s) failed due to: %r", name, remove_password_if_present(cmdline), ex)
87+
raise CommandError(['<%s-pump>' % name] + remove_password_if_present(cmdline), ex) from ex
8888
finally:
8989
stream.close()
9090

@@ -406,7 +406,7 @@ def read_all_from_possibly_closed_stream(stream):
406406
if status != 0:
407407
errstr = read_all_from_possibly_closed_stream(self.proc.stderr)
408408
log.debug('AutoInterrupt wait stderr: %r' % (errstr,))
409-
raise GitCommandError(self.args, status, errstr)
409+
raise GitCommandError(remove_password_if_present(self.args), status, errstr)
410410
# END status handling
411411
return status
412412
# END auto interrupt
@@ -683,8 +683,10 @@ def execute(self, command,
683683
:note:
684684
If you add additional keyword arguments to the signature of this method,
685685
you must update the execute_kwargs tuple housed in this module."""
686+
# Remove password for the command if present
687+
redacted_command = remove_password_if_present(command)
686688
if self.GIT_PYTHON_TRACE and (self.GIT_PYTHON_TRACE != 'full' or as_process):
687-
log.info(' '.join(command))
689+
log.info(' '.join(redacted_command))
688690

689691
# Allow the user to have the command executed in their working dir.
690692
cwd = self._working_dir or os.getcwd()
@@ -705,7 +707,7 @@ def execute(self, command,
705707
if is_win:
706708
cmd_not_found_exception = OSError
707709
if kill_after_timeout:
708-
raise GitCommandError(command, '"kill_after_timeout" feature is not supported on Windows.')
710+
raise GitCommandError(redacted_command, '"kill_after_timeout" feature is not supported on Windows.')
709711
else:
710712
if sys.version_info[0] > 2:
711713
cmd_not_found_exception = FileNotFoundError # NOQA # exists, flake8 unknown @UndefinedVariable
@@ -720,7 +722,7 @@ def execute(self, command,
720722
if istream:
721723
istream_ok = "<valid stream>"
722724
log.debug("Popen(%s, cwd=%s, universal_newlines=%s, shell=%s, istream=%s)",
723-
command, cwd, universal_newlines, shell, istream_ok)
725+
redacted_command, cwd, universal_newlines, shell, istream_ok)
724726
try:
725727
proc = Popen(command,
726728
env=env,
@@ -736,7 +738,7 @@ def execute(self, command,
736738
**subprocess_kwargs
737739
)
738740
except cmd_not_found_exception as err:
739-
raise GitCommandNotFound(command, err) from err
741+
raise GitCommandNotFound(redacted_command, err) from err
740742

741743
if as_process:
742744
return self.AutoInterrupt(proc, command)
@@ -786,7 +788,7 @@ def _kill_process(pid):
786788
watchdog.cancel()
787789
if kill_check.isSet():
788790
stderr_value = ('Timeout: the command "%s" did not complete in %d '
789-
'secs.' % (" ".join(command), kill_after_timeout))
791+
'secs.' % (" ".join(redacted_command), kill_after_timeout))
790792
if not universal_newlines:
791793
stderr_value = stderr_value.encode(defenc)
792794
# strip trailing "\n"
@@ -810,7 +812,7 @@ def _kill_process(pid):
810812
proc.stderr.close()
811813

812814
if self.GIT_PYTHON_TRACE == 'full':
813-
cmdstr = " ".join(command)
815+
cmdstr = " ".join(redacted_command)
814816

815817
def as_text(stdout_value):
816818
return not output_stream and safe_decode(stdout_value) or '<OUTPUT_STREAM>'
@@ -826,7 +828,7 @@ def as_text(stdout_value):
826828
# END handle debug printing
827829

828830
if with_exceptions and status != 0:
829-
raise GitCommandError(command, status, stderr_value, stdout_value)
831+
raise GitCommandError(redacted_command, status, stderr_value, stdout_value)
830832

831833
if isinstance(stdout_value, bytes) and stdout_as_string: # could also be output_stream
832834
stdout_value = safe_decode(stdout_value)

‎git/repo/base.py

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,7 @@
2525
from git.objects import Submodule, RootModule, Commit
2626
from git.refs import HEAD, Head, Reference, TagReference
2727
from git.remote import Remote, add_progress, to_progress_instance
28-
from git.util import Actor, finalize_process, decygpath, hex_to_bin, expand_path
28+
from git.util import Actor, finalize_process, decygpath, hex_to_bin, expand_path, remove_password_if_present
2929
import os.path as osp
3030

3131
from .fun import rev_parse, is_git_dir, find_submodule_git_dir, touch, find_worktree_git_dir
@@ -1018,7 +1018,10 @@ def _clone(cls, git: 'Git', url: PathLike, path: PathLike, odb_default_type: Typ
10181018
finalize_process, decode_streams=False)
10191019
else:
10201020
(stdout, stderr) = proc.communicate()
1021-
log.debug("Cmd(%s)'s unused stdout: %s", getattr(proc, 'args', ''), stdout)
1021+
cmdline = getattr(proc, 'args', '')
1022+
cmdline = remove_password_if_present(cmdline)
1023+
1024+
log.debug("Cmd(%s)'s unused stdout: %s", cmdline, stdout)
10221025
finalize_process(proc, stderr=stderr)
10231026

10241027
# our git command could have a different working dir than our actual

‎git/util.py

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
from sys import maxsize
1818
import time
1919
from unittest import SkipTest
20+
from urllib.parse import urlsplit, urlunsplit
2021

2122
# typing ---------------------------------------------------------
2223

@@ -362,6 +363,34 @@ def expand_path(p: PathLike, expand_vars: bool = True) -> Optional[PathLike]:
362363
except Exception:
363364
return None
364365

366+
367+
def remove_password_if_present(cmdline):
368+
"""
369+
Parse any command line argument and if on of the element is an URL with a
370+
password, replace it by stars (in-place).
371+
372+
If nothing found just returns the command line as-is.
373+
374+
This should be used for every log line that print a command line.
375+
"""
376+
new_cmdline = []
377+
for index, to_parse in enumerate(cmdline):
378+
new_cmdline.append(to_parse)
379+
try:
380+
url = urlsplit(to_parse)
381+
# Remove password from the URL if present
382+
if url.password is None:
383+
continue
384+
385+
edited_url = url._replace(
386+
netloc=url.netloc.replace(url.password, "*****"))
387+
new_cmdline[index] = urlunsplit(edited_url)
388+
except ValueError:
389+
# This is not a valid URL
390+
continue
391+
return new_cmdline
392+
393+
365394
#} END utilities
366395

367396
#{ Classes

‎test/test_repo.py

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -238,6 +238,21 @@ def test_clone_from_with_path_contains_unicode(self):
238238
except UnicodeEncodeError:
239239
self.fail('Raised UnicodeEncodeError')
240240

241+
@with_rw_directory
242+
def test_leaking_password_in_clone_logs(self, rw_dir):
243+
password = "fakepassword1234"
244+
try:
245+
Repo.clone_from(
246+
url="https://fakeuser:{}@fakerepo.example.com/testrepo".format(
247+
password),
248+
to_path=rw_dir)
249+
except GitCommandError as err:
250+
assert password not in str(err), "The error message '%s' should not contain the password" % err
251+
# Working example from a blank private project
252+
Repo.clone_from(
253+
url="https://gitlab+deploy-token-392045:mLWhVus7bjLsy8xj8q2V@gitlab.com/mercierm/test_git_python",
254+
to_path=rw_dir)
255+
241256
@with_rw_repo('HEAD')
242257
def test_max_chunk_size(self, repo):
243258
class TestOutputStream(TestBase):

‎test/test_util.py

Lines changed: 19 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,8 @@
3030
Actor,
3131
IterableList,
3232
cygpath,
33-
decygpath
33+
decygpath,
34+
remove_password_if_present,
3435
)
3536

3637

@@ -322,3 +323,20 @@ def test_pickle_tzoffset(self):
322323
t2 = pickle.loads(pickle.dumps(t1))
323324
self.assertEqual(t1._offset, t2._offset)
324325
self.assertEqual(t1._name, t2._name)
326+
327+
def test_remove_password_from_command_line(self):
328+
password = "fakepassword1234"
329+
url_with_pass = "https://fakeuser:{}@fakerepo.example.com/testrepo".format(password)
330+
url_without_pass = "https://fakerepo.example.com/testrepo"
331+
332+
cmd_1 = ["git", "clone", "-v", url_with_pass]
333+
cmd_2 = ["git", "clone", "-v", url_without_pass]
334+
cmd_3 = ["no", "url", "in", "this", "one"]
335+
336+
redacted_cmd_1 = remove_password_if_present(cmd_1)
337+
assert password not in " ".join(redacted_cmd_1)
338+
# Check that we use a copy
339+
assert cmd_1 is not redacted_cmd_1
340+
assert password in " ".join(cmd_1)
341+
assert cmd_2 == remove_password_if_present(cmd_2)
342+
assert cmd_3 == remove_password_if_present(cmd_3)

0 commit comments

Comments
 (0)
0