Repository navigation
Commit 6fdfa31
committed
fix(util): redact URL credentials without corrupting the host
remove_password_if_present() redacted credentials by substituting them
into url.netloc with str.replace(), which rewrites every occurrence
anywhere in the netloc -- including the host.
https://git@github.com/user/repo.git
-> https://*****@*****hub.com/user/repo.git
"git" is the most common Git username and a substring of "github.com",
so this hits the common case. An empty username or password makes it
worse: str.replace("", "*****") matches at every position, so
https://:token@fakerepo.example.com/testrepo
comes back shredded into a URL roughly ten times longer, which is what
lands in GitCommandError messages when a fetch fails.
Rebuild the netloc from its userinfo instead, so only the credentials
are replaced. The existing test already covered the empty-password case
but only asserted the password is absent, which any mangling satisfies.
The function is the single redaction path used for logged command lines
and for exception messages (git/cmd.py, git/exc.py, git/repo/base.py),
so this fixes all of them at once.1 parent 574aec2 commit 6fdfa31
2 files changed
Lines changed: 21 additions & 4 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
@@ -658,10 +658,14 @@ def remove_password_if_present(cmdline: Sequence[str]) -> List[str]: | |||
| 658 | 658 | if url.password is None and url.username is None: | |
| 659 | 659 | continue | |
| 660 | 660 | ||
| 661 | - if url.password is not None: | ||
| 662 | - url = url._replace(netloc=url.netloc.replace(url.password, "*****")) | ||
| 663 | - if url.username is not None: | ||
| 664 | - url = url._replace(netloc=url.netloc.replace(url.username, "*****")) | ||
| 661 | + # Redact the userinfo as a whole rather than substituting the | ||
| 662 | + # username/password into the netloc: a substring replace also hits | ||
| 663 | + # the host ("git" in "github.com") and an empty username or password | ||
| 664 | + # matches at every position. | ||
| 665 | + _, at, hostinfo = url.netloc.rpartition("@") | ||
| 666 | + if at: | ||
| 667 | + redacted = "*****:*****" if url.password is not None else "*****" | ||
| 668 | + url = url._replace(netloc=f"{redacted}@{hostinfo}") | ||
| 665 | 669 | new_cmdline[index] = urlunsplit(url) | |
| 666 | 670 | except ValueError: | |
| 667 | 671 | # This is not a valid URL. | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
@@ -828,6 +828,19 @@ def test_remove_password_from_command_line(self): | |||
| 828 | 828 | assert authorization not in " ".join(redacted_cmd_6) | |
| 829 | 829 | assert "http.extraHeader=Authorization: *****" in redacted_cmd_6 | |
| 830 | 830 | ||
| 831 | + def test_remove_password_keeps_host_intact(self): | ||
| 832 | + """Redaction must not touch the host, even when it contains the username.""" | ||
| 833 | + redacted = remove_password_if_present(["git", "clone", "https://git@github.com/user/repo.git"]) | ||
| 834 | + assert redacted == ["git", "clone", "https://*****@github.com/user/repo.git"] | ||
| 835 | + | ||
| 836 | + redacted = remove_password_if_present(["git", "clone", "ssh://git@github.com/u/r.git"]) | ||
| 837 | + assert redacted == ["git", "clone", "ssh://*****@github.com/u/r.git"] | ||
| 838 | + | ||
| 839 | + def test_remove_empty_password_keeps_host_intact(self): | ||
| 840 | + """An empty password must not expand into every position of the netloc.""" | ||
| 841 | + redacted = remove_password_if_present(["git", "clone", "https://:@fakerepo.example.com/testrepo"]) | ||
| 842 | + assert redacted == ["git", "clone", "https://*****:*****@fakerepo.example.com/testrepo"] | ||
| 843 | + | ||
| 831 | 844 | ||
| 832 | 845 | def test_mode_str_to_int_accepts_bytes(): | |
| 833 | 846 | assert mode_str_to_int("100644") == 0o100644 | |
0 commit comments