GHSA-RWJ8-PGH3-R573: Root-Cause Fix for GitPython Clone URL Expansion

Vulnerability

GHSA-RWJ8-PGH3-R573 describes a flaw in GitPython's clone path handling where untrusted URL input was passed through Git.polish_url, which expanded environment variables and ~ before use. In the vulnerable implementation, Repo.clone_from also performed Git.check_unsafe_protocols(url) on the original input rather than on the normalized value, creating a mismatch between what was validated and what was executed. The combined effect was credential disclosure through local environment expansion and a protocol validation bypass if normalization changed the effective transport string.

# Vulnerable flow (from patch snippets)
Git.check_unsafe_protocols(url)
Git.polish_url(url)  # expands vars / ~

The issue is source-grounded in the patch diff for PR #2172 and the fixing commit 8ac5a30519b6f4af85398b9b9d7064ff4d452da2. The advisory summary also aligns with this behavior: clone URLs containing tokens like $VAR, ${VAR}, or Windows-style %VAR% could be expanded automatically, and the post-expansion string could differ materially from the pre-check string.

Patch

The patch introduces an expand_vars parameter to Git.polish_url and threads the same control into Cygwin-specific helpers in git/util.py. For clone-related paths, the code now explicitly computes clone_url = Git.polish_url(url, expand_vars=False), validates the polished value with Git.check_unsafe_protocols(clone_url), and uses that same non-expanded value for the clone operation and for storing the remote URL.

# Patched flow (from patch snippets)
clone_url = Git.polish_url(url, expand_vars=False)
Git.check_unsafe_protocols(clone_url)
clone_url

This is the key design change: untrusted remote URLs are no longer subject to local shell-like expansion semantics. The patch also updates cygpath and _cygexpath so Cygwin path normalization does not silently reintroduce expansion when expand_vars=False is requested. The new tests in the PR verify four important properties: clone input is passed through without environment expansion, stored remote URLs preserve literal variable markers, unsafe protocol checks are applied to the polished URL, and Cygwin mode respects the no-expansion setting.

Review

Pros

• The patch addresses the root trust-boundary error by distinguishing trusted local path normalization from untrusted remote URL handling. That is a stronger fix than trying to blacklist specific variable syntaxes.
• Protocol validation is now applied to the same string that will be used for cloning, eliminating the pre-check/post-transform inconsistency documented by the advisory.
• The expand_vars flag is propagated into cygpath and _cygexpath, which closes an easy platform-specific bypass where Cygwin normalization could otherwise keep expanding variables.
• Regression coverage is good and directly tied to the vulnerability mechanics: Unix-style, brace-style, and Windows-style variable forms are tested, along with persistence of the remote URL and protocol-check ordering.
• The API documentation added to polish_url explicitly warns that expansion should be disabled for untrusted values such as remote URLs, which improves maintainability.

Cons

• The default for expand_vars remains True. That preserves backward compatibility, but it also means future call sites can still misuse polish_url on attacker-controlled input if developers forget to opt out.
• The fix is narrowly applied to clone-related flows shown in the diff. Based on the provided snippets alone, it is not possible to confirm whether every other URL-accepting path in the library has been audited for the same trust-boundary issue.
• The tests demonstrate that unsafe protocol checking now uses the polished URL, but they do not enumerate a broader matrix of edge-case URL transformations beyond the examples in the patch snippets.

Verdict

Root-cause.

The patch corrects the underlying flaw rather than masking symptoms: it stops expansion of untrusted clone URLs, ensures protocol validation is performed on the effective value, and extends the same behavior into Cygwin-specific normalization paths. While the default-expanding API still leaves some residual misuse risk for future callers, the clone vulnerability described in the advisory is directly and comprehensively addressed by the changes in PR #2172 and the fixing commit.