| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
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.
|
Thanks a lot, good catch! |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
Summary
remove_password_if_present() redacts credentials by substituting the username/password into url.netloc with str.replace(). str.replace() rewrites every occurrence anywhere in the string, and the netloc contains the host and port as well as the userinfo — so the host gets corrupted too.
The username git is the most common Git username and is a substring of github.com:
An empty username or password makes it considerably worse: urlsplit returns '' (not None) for https://:token@host, and str.replace('', '*****') matches at every position:
That second case is the reason this is more than cosmetic: it inflates the message ~10x and makes the remote unidentifiable, at exactly the moment a user is reading a GitCommandError to work out which remote failed.
This 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 the fix is one guard in the shared function rather than a patch per caller.
The fix
Rebuild the netloc from its userinfo rather than substituting into it:
urlsplit() already returns username/password as non-None only when the
netloc carries an @, so by this point in the loop a userinfo boundary is
guaranteed and the rpartition separator never needs a separate guard.
Output is byte-identical to the current behaviour for every non-overlapping input, including all three inputs in the existing test and port/case preservation.
Why the existing test did not catch it
test_remove_password_from_command_line already contains the empty-password case as cmd_3, and it passes today — because it only asserts password not in " ".join(...), which any mangling satisfies. The two new tests assert the full redacted command line, which is what pins the host in place.
Testing
Both new tests fail on main and pass with this change. The pre-existing test_remove_password_from_command_line passes both before and after, so there is no behaviour change for the cases it already covers. ruff check and ruff format --check are clean.
The suite was not run locally (this was prepared on a memory-constrained machine where pip install of the test dependencies is not possible), so the new tests have not been executed against the real package — CI is the verification. What was run locally is remove_password_if_present's body copied verbatim out of this branch with the assertion table below, executed as a standalone script under the stdlib only:
All pass, and the pre-fix code reproduces both reported outputs (https://*****@*****hub.com/... and the shredded host) exactly as described above.