ENG-3613: Remove GitPython dependency, reimplement dirty check with subprocess#8237
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
Dependency Review✅ No vulnerabilities found.Snapshot WarningsEnsure that dependencies are being submitted on PR branches and consider enabling retry-on-snapshot-warnings. See the documentation for more information and troubleshooting advice. Scanned Files
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8237 +/- ##
==========================================
+ Coverage 85.12% 85.14% +0.01%
==========================================
Files 670 670
Lines 43415 43413 -2
Branches 5081 5081
==========================================
+ Hits 36957 36962 +5
+ Misses 5351 5345 -6
+ Partials 1107 1106 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
da715fa to
18ecf8a
Compare
GitPython shipped as a runtime dep despite only being used by git_is_dirty(), a CLI helper in fides pull. Three High-severity CVEs (CVE-2026-42215, CVE-2026-42284, CVE-2026-44243) applied to the pinned version (3.1.41), triggering C1 SLA requirements against code never executed in the server. Replace the GitPython-based implementation with subprocess + os.path: - os.path.isdir(".git/") replaces is_git_dir() - subprocess.run(["git", "status", ...]) replaces Repo/git_session.status() - FileNotFoundError on missing git binary replaces ImportError fallback Behavior is identical. The git CLI is always present on machines running fides pull (git is required to have the repo). GitPython and its transitive dep gitdb are fully removed from the dependency tree.
18ecf8a to
74b90b9
Compare
|
/code-review |
ca8a774 to
ff3530d
Compare
There was a problem hiding this comment.
Code Review: Remove GitPython dependency, reimplement dirty check with subprocess
The core approach here is correct and well-motivated — replacing a third-party library with stdlib subprocess to eliminate three CVEs from the production image is the right call. The changelog entry is accurate and the uv.lock update is clean.
The main concerns are in the new subprocess-based implementation of git_is_dirty.
Critical (should fix before merge)
1. NameError if subprocess.run raises a non-FileNotFoundError exception
The return bool(result.stdout.strip()) is outside the try block, so if subprocess.run raises anything other than FileNotFoundError (e.g. PermissionError, OSError, or TimeoutExpired once a timeout is added), result will be unbound and the function will raise NameError. Moving the return inside the try block fixes this. See inline comment on line 163.
2. No timeout on subprocess.run — can hang indefinitely
git status on a network-mounted filesystem or a repo with a slow/timing-out remote can stall. Adding timeout=10 and catching subprocess.TimeoutExpired prevents the CLI from blocking indefinitely. See inline comment on lines 153–158.
Minor
3. os.path.exists(".git") vs os.path.isdir(".git")
os.path.exists returns True for a .git file (present in worktrees and submodules), making the guard pass and the error message misleading. os.path.isdir is more semantically correct. Also, the old is_git_dir validated internal git structure (HEAD, objects, refs); the new check will proceed to git status even on a corrupt .git dir — the exit-code check (suggestion 4) would catch that case.
Suggestions
4. Silent failure on non-zero git status exit code
With check=False, a code-128 exit (corrupt repo, etc.) produces empty stdout and silently returns False — indistinguishable from a clean tree. Checking result.returncode and logging on failure makes errors visible. See inline comment on line 157.
5. Unpinned GitPython>=3.1.50 in dev dependencies
Every other dev dependency is pinned to an exact version for reproducibility. Since the CVE concern is about the runtime dependency (now removed), pinning the dev entry to ==3.1.50 (matching the resolved lock) avoids silent float on future uv lock --upgrade. See inline comment on line 147 of pyproject.toml.
6. Docstring says "unstaged changes" but --porcelain also catches untracked files — minor wording nit on line 145.
Overall: two small but real bugs (NameError, missing timeout) should be addressed before merge. The other items are low-risk suggestions. The security goal of removing GitPython from the production image is achieved cleanly.
🔬 Codegraph: unavailable
💡 Write /code-review in a comment to re-run this review.
ed12b5d to
76b8e77
Compare
| "Faker==14.1.0", | ||
| "freezegun==1.5.5", | ||
| "GitPython==3.1.41", | ||
| "GitPython==3.1.50", |
There was a problem hiding this comment.
This adds as a dev dependency only and updates it to the latest version (for tests)
Ticket ENG-3613
Description Of Changes
GitPython was declared as a runtime dependency in
pyproject.tomldespite only being used bygit_is_dirty(), a CLI helper infides pullthat warns operators if they are running from a dirty git working tree. This caused three High-severity CVEs (CVE-2026-42215, CVE-2026-42284, CVE-2026-44243) to apply to the production fidesplus slim image against code that never executes in the deployed server.Rather than bumping the pinned version (which would only defer the problem — GitPython shipped 3 patch releases in 8 days), we remove the dependency entirely by reimplementing
git_is_dirty()using stdlibsubprocessandos.path. The git CLI is always available on machines runningfides pull(git is required to clone the repo in the first place).Behavior is identical to before:
TrueFalse.git/directory → prints "No git repo detected", returnsFalsegitbinary not found → prints "Git executable not detected", returnsFalseGitPython and its transitive dependency
gitdbare fully removed from the dependency tree.Code Changes
src/fides/core/utils.py— replace GitPython-basedgit_is_dirty()withsubprocess.run(["git", "status", ...])+os.path.isdir(".git/")checkpyproject.toml— removeGitPython==3.1.41from[project.dependencies]and from[dependency-groups] devuv.lock— updated to reflect removed packagesSteps to Confirm
fides pull— confirm the dirty-tree warning appears as beforefides pull— confirm no dirty warningdocker run --rm ethyca/fidesplus:local pip show GitPython # Expected: WARNING: Package(s) not found: GitPythonPre-Merge Checklist
CHANGELOG.mdupdated