From 6c4cd6ae31d66df20e9a1043e96e6da7f68177aa Mon Sep 17 00:00:00 2001 From: Keerthana KT Date: Wed, 21 Oct 2026 13:43:18 +0530 Subject: [PATCH 1/2] fix(util): redact URL credentials in config values on the command line `remove_password_if_present` is applied to every command line that GitPython logs or stores in `GitCommandError` and the other `CommandError` subclasses. It redacted an argument only when the whole argument was a URL with userinfo, or when it was an `http.extraHeader=Authorization: ...` assignment. Two common forms were missed: - A URL that is the value of a configuration assignment, such as `-c http.proxy=http://user:pass@proxy:3128` or `-c remote.origin.url=https://user:token@host/repo`. The text before `://` contains `=`, so `urlsplit` finds no scheme and no userinfo, and the argument was kept verbatim. - The `--config==` form, which is what `transform_kwargs` produces for a `config=...` keyword argument. The leading `--config=` defeated both checks, so even `--config=http.extraHeader=Authorization: Bearer ` was printed in full. For example, after `repo.git.set_persistent_git_options(c="http.proxy=http://user:secret@proxy:3128")` every failing command raised a `GitCommandError` whose message contained `user:secret`, and the same text reached the log when command tracing is on. The URL redaction moves into a small `_redact_url_credentials` helper. `remove_password_if_present` strips a leading `--config=` before the extra header check, and when the whole argument is not a URL with userinfo it tries the assignment after `--config=` (which covers `url..insteadOf` keys that carry credentials) and then the value after the first `=`. Only the userinfo is replaced, so the key and the rest of the URL stay readable, and arguments without a URL are returned unchanged. Arguments that were already redacted produce the same output as before. Adds `test_remove_password_from_config_values` to `test/test_util.py`. Its five redaction cases fail before this change and pass after it, and the existing redaction tests still pass. `ruff check`, `ruff format --check` and `mypy` are clean. --- git/util.py | 58 +++++++++++++++++++++++++++++++---------------- test/test_util.py | 27 ++++++++++++++++++++++ 2 files changed, 65 insertions(+), 20 deletions(-) diff --git a/git/util.py b/git/util.py index 874b764d5..473ea37db 100644 --- a/git/util.py +++ b/git/util.py @@ -652,42 +652,60 @@ def expand_path(p: Union[None, PathLike], expand_vars: bool = True) -> Optional[ return None +def _redact_url_credentials(text: str) -> Optional[str]: + """Return `text` with the userinfo of a URL redacted, or ``None`` if it has none.""" + try: + url = urlsplit(text) + except ValueError: + # This is not a valid URL. + return None + if url.password is None and url.username is None: + return None + + # Match urllib.parse's userinfo boundary. Keeping the raw hostinfo + # preserves hostname case, IPv6 brackets, and port formatting. + _, _, hostinfo = url.netloc.rpartition("@") + redacted = "*****:*****" if url.password is not None else "*****" + return urlunsplit(url._replace(netloc=f"{redacted}@{hostinfo}")) + + def remove_password_if_present(cmdline: Sequence[str]) -> List[str]: """Redact credentials in URLs and HTTP Authorization extra headers in a command line. + URLs are also redacted when they are the value of a configuration assignment, such + as ``http.proxy=``, and assignments are recognized in both the ``-c =`` + and the ``--config==`` form. + If nothing is found, this just returns the command line as-is. 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) - config_key, separator, header = to_parse.partition("=") - header_name, colon, _ = header.partition(":") + for to_parse in cmdline: + option = "--config=" if to_parse.startswith("--config=") else "" + assignment = to_parse[len(option) :] + config_key, separator, value = assignment.partition("=") + header_name, colon, _ = value.partition(":") if ( separator and colon and config_key.lower().endswith(".extraheader") and header_name.strip().lower() == "authorization" ): - new_cmdline[index] = "%s%s%s%s *****" % (config_key, separator, header_name, colon) - continue - try: - url = urlsplit(to_parse) - # Remove password from the URL if present. - if url.password is None and url.username is None: - continue - - # Match urllib.parse's userinfo boundary. Keeping the raw hostinfo - # preserves hostname case, IPv6 brackets, and port formatting. - _, _, hostinfo = url.netloc.rpartition("@") - redacted = "*****:*****" if url.password is not None else "*****" - url = url._replace(netloc=f"{redacted}@{hostinfo}") - new_cmdline[index] = urlunsplit(url) - except ValueError: - # This is not a valid URL. + new_cmdline.append("%s%s%s%s%s *****" % (option, config_key, separator, header_name, colon)) continue + + redacted = _redact_url_credentials(to_parse) + if redacted is None and option: + redacted = _redact_url_credentials(assignment) + if redacted is not None: + redacted = option + redacted + if redacted is None and separator: + redacted = _redact_url_credentials(value) + if redacted is not None: + redacted = option + config_key + separator + redacted + new_cmdline.append(to_parse if redacted is None else redacted) return new_cmdline diff --git a/test/test_util.py b/test/test_util.py index ca592aaf7..da2f59c85 100644 --- a/test/test_util.py +++ b/test/test_util.py @@ -856,6 +856,33 @@ def test_remove_empty_password_keeps_host_intact(self): def test_remove_password_preserves_url_components(self, url, expected): assert remove_password_if_present([url]) == [expected] + @ddt.data( + ( + ["-c", "http.proxy=http://user:pass@proxy.example.com:3128"], + ["-c", "http.proxy=http://*****:*****@proxy.example.com:3128"], + ), + ( + ["-c", "remote.origin.url=https://user:pass@example.com/repo"], + ["-c", "remote.origin.url=https://*****:*****@example.com/repo"], + ), + ( + ["--config=http.proxy=http://user:pass@proxy.example.com:3128"], + ["--config=http.proxy=http://*****:*****@proxy.example.com:3128"], + ), + ( + ["--config=url.https://user:pass@example.com/.insteadOf=https://example.com/"], + ["--config=url.https://*****:*****@example.com/.insteadOf=https://example.com/"], + ), + ( + ["--config=http.extraHeader=Authorization: Bearer pass"], + ["--config=http.extraHeader=Authorization: *****"], + ), + (["-c", "user.name=pass"], ["-c", "user.name=pass"]), + ) + @ddt.unpack + def test_remove_password_from_config_values(self, cmdline, expected): + assert remove_password_if_present(cmdline) == expected + def test_mode_str_to_int_accepts_bytes(): assert mode_str_to_int("100644") == 0o100644 From f89c3431a7e825f3ffe3b4e262b67fb9318fba60 Mon Sep 17 00:00:00 2001 From: Byron Date: Wed, 21 Oct 2026 15:16:49 +0200 Subject: [PATCH 2/2] review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - [P1] Redact the value when the config key also contains a URL — /Users/byron/dev/github.com/gitpython-developers/GitPython.redact-config-url-c redentials/git/util.py:700-705 When a URL-specific config key and its value both contain credentials, such as `--config=http.https://alice@host.proxy=http://bob:token@proxy`, redacting the key sets `redacted`, so the value is never checked. `Git.execute` traces and `GitCommandError.command` still expose `bob:token`. Redact the key and value independently. - [P2] Recognize clone's compact -c configuration form — /Users/byron/dev/github.com/gitpython-developers/GitPython.redact-config-url-c redentials/git/util.py:686-688 When `Repo.clone_from` receives Git clone's valid `-c=` form through `multi_options`, a credentialed URL key such as `-curl.https://user:token@host/.insteadOf=https://old/` remains unchanged. The leading `-c` prevents URL parsing, while the fallback checks only the value, so traces and exceptions expose the token. Strip this prefix before checking the key. Assisted-by: GPT 6.0 Astra Assisted-by: GPT 6.1 Sol Co-authored-by: GPT 6.1 Sol --- git/util.py | 28 ++++++++++++++-------------- test/test_git.py | 35 +++++++++++++++++++++++++++++++++-- test/test_util.py | 30 ++++++++++++++++++++++++++++++ 3 files changed, 77 insertions(+), 16 deletions(-) diff --git a/git/util.py b/git/util.py index 473ea37db..137318eb8 100644 --- a/git/util.py +++ b/git/util.py @@ -672,9 +672,9 @@ def _redact_url_credentials(text: str) -> Optional[str]: def remove_password_if_present(cmdline: Sequence[str]) -> List[str]: """Redact credentials in URLs and HTTP Authorization extra headers in a command line. - URLs are also redacted when they are the value of a configuration assignment, such - as ``http.proxy=``, and assignments are recognized in both the ``-c =`` - and the ``--config==`` form. + URLs are also redacted in the key and value of a configuration assignment, such + as ``http.proxy=``. Assignments are recognized in the ``-c =``, + ``-c=``, and ``--config==`` forms. If nothing is found, this just returns the command line as-is. @@ -683,8 +683,15 @@ def remove_password_if_present(cmdline: Sequence[str]) -> List[str]: """ new_cmdline = [] for to_parse in cmdline: - option = "--config=" if to_parse.startswith("--config=") else "" + if to_parse.startswith("--config="): + option = "--config=" + elif to_parse.startswith("-c"): + option = "-c" + else: + option = "" assignment = to_parse[len(option) :] + # Redact the whole URL first, before splitting on a possible '=' in userinfo. + assignment = _redact_url_credentials(assignment) or assignment config_key, separator, value = assignment.partition("=") header_name, colon, _ = value.partition(":") if ( @@ -696,16 +703,9 @@ def remove_password_if_present(cmdline: Sequence[str]) -> List[str]: new_cmdline.append("%s%s%s%s%s *****" % (option, config_key, separator, header_name, colon)) continue - redacted = _redact_url_credentials(to_parse) - if redacted is None and option: - redacted = _redact_url_credentials(assignment) - if redacted is not None: - redacted = option + redacted - if redacted is None and separator: - redacted = _redact_url_credentials(value) - if redacted is not None: - redacted = option + config_key + separator + redacted - new_cmdline.append(to_parse if redacted is None else redacted) + if separator: + value = _redact_url_credentials(value) or value + new_cmdline.append(option + config_key + separator + value) return new_cmdline diff --git a/test/test_git.py b/test/test_git.py index 5224842ee..eb9d305b8 100644 --- a/test/test_git.py +++ b/test/test_git.py @@ -10,7 +10,6 @@ import logging import os import os.path as osp -from pathlib import Path import pickle import re import shutil @@ -20,6 +19,7 @@ import tempfile import threading import time +from pathlib import Path from unittest import mock, skipUnless import ddt @@ -27,7 +27,6 @@ from git import Git, GitCommandError, GitCommandNotFound, Repo, cmd, refresh from git.exc import UnsafeOptionError from git.util import cwd, finalize_process - from test.lib import TestBase, fixture_path, with_rw_directory @@ -547,6 +546,38 @@ def test_it_redacts_authorization_extra_header_from_error(self): self.assertNotIn(token, message) self.assertIn("http.extraHeader=Authorization: *****", message) + @ddt.data( + ( + "--config=http.https://alice@host.proxy=http://bob:token@proxy", + "--config=http.https://*****@host.proxy=http://*****:*****@proxy", + ), + ( + "-curl.https://user:token@host/.insteadOf=https://old/", + "-curl.https://*****:*****@host/.insteadOf=https://old/", + ), + ) + @ddt.unpack + def test_clone_redacts_config_urls_from_trace_and_error(self, option, expected): + for trace in (True, "full"): + with self.subTest(trace=trace), tempfile.TemporaryDirectory() as directory: + with mock.patch.object(Git, "GIT_PYTHON_TRACE", trace): + with self.assertLogs("git.cmd", level="INFO") as logs: + with self.assertRaises(GitCommandError) as context: + Repo.clone_from( + osp.join(directory, "missing-source"), + osp.join(directory, "destination"), + multi_options=[option], + allow_unsafe_options=True, + ) + self.assertIn(expected, context.exception.command) + message = str(context.exception) + assert logs is not None + log_output = " ".join(logs.output) + self.assertIn(expected, log_output) + for credential in ("alice", "bob", "user", "token"): + self.assertNotIn(credential, message) + self.assertNotIn(credential, log_output) + def test_it_accepts_environment_variables(self): filename = fixture_path("ls_tree_empty") with open(filename, "r") as fh: diff --git a/test/test_util.py b/test/test_util.py index da2f59c85..df8c2d452 100644 --- a/test/test_util.py +++ b/test/test_util.py @@ -849,6 +849,7 @@ def test_remove_empty_password_keeps_host_intact(self): ("//user:pass@[2001:db8::1]:0080/repo", "//*****:*****@[2001:db8::1]:0080/repo"), ("https://user:p@ss@example.com/repo", "https://*****:*****@example.com/repo"), ("https://user:@example.com/repo", "https://*****:*****@example.com/repo"), + ("https://user:p=ass@example.com/repo", "https://*****:*****@example.com/repo"), ("https://@example.com/repo", "https://*****@example.com/repo"), ("https://example.com/repo@name?q=a@b#c@d", "https://example.com/repo@name?q=a@b#c@d"), ) @@ -877,7 +878,36 @@ def test_remove_password_preserves_url_components(self, url, expected): ["--config=http.extraHeader=Authorization: Bearer pass"], ["--config=http.extraHeader=Authorization: *****"], ), + ( + ["--config=http.https://alice@host.proxy=http://bob:token@proxy"], + ["--config=http.https://*****@host.proxy=http://*****:*****@proxy"], + ), + ( + ["-c", "http.https://alice@host.proxy=http://bob:token@proxy"], + ["-c", "http.https://*****@host.proxy=http://*****:*****@proxy"], + ), + ( + ["-chttp.https://alice@host.proxy=http://bob:token@proxy"], + ["-chttp.https://*****@host.proxy=http://*****:*****@proxy"], + ), + ( + ["-curl.https://user:token@host/.insteadOf=https://old/"], + ["-curl.https://*****:*****@host/.insteadOf=https://old/"], + ), + ( + ["-chttp.proxy=http://bob:token@proxy"], + ["-chttp.proxy=http://*****:*****@proxy"], + ), + ( + ["--config=http.https://alice:token@host/.extraHeader=Authorization: Bearer pass"], + ["--config=http.https://*****:*****@host/.extraHeader=Authorization: *****"], + ), + ( + ["-chttp.extraHeader=Authorization: Bearer pass"], + ["-chttp.extraHeader=Authorization: *****"], + ), (["-c", "user.name=pass"], ["-c", "user.name=pass"]), + (["-cuser.name=pass"], ["-cuser.name=pass"]), ) @ddt.unpack def test_remove_password_from_config_values(self, cmdline, expected):