GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | ||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | ||||||||||||
GeezerVison is showing github.com — Fix repository discovery precedence by Byron · Pull Request #2218 · gitpython-developers/GitPython Conversation
|
| File | Review summary |
|---|---|
test/test_repo.py |
Adds repository discovery and common-directory tests. |
git/repo/fun.py |
Three unresolved moderate findings: malformed HEAD targets are accepted (3 votes); malformed commondir handling can escape or raise incorrectly (2 votes); FIFO HEAD files can block discovery (2 votes). |
git/repo/base.py |
One unresolved moderate finding (2 votes): relative GIT_COMMON_DIR values can cause subsequent Git operations to target the wrong repository. |
doc/source/changes.rst |
Adds the security advisory to the changelog. |
git/repo/base.py:310
osp.exists follows symlinks, so a dangling .git symlink makes this condition false. Discovery then continues into is_git_dir(curpath) or a parent, allowing a bare-looking worktree/ancestor repository to be selected despite an existing malformed .git entry. Use an existence check that does not follow symlinks (for example, lexists) so broken .git entries stop discovery like other invalid gitfiles. if osp.exists(dotgit) and not osp.isdir(dotgit):
break
git/repo/fun.py:97
or treats an explicitly set empty GIT_OBJECT_DIRECTORY as if the variable were unset. Git's discovery code checks whether this variable is present and then validates that exact path, so with GIT_OBJECT_DIRECTORY="" this implementation can accept a repository that Git rejects and leave later Git commands inconsistent with discovery. Test for presence in os.environ and use the empty value as invalid rather than falling back to <common_dir>/objects. object_dir = os.getenv("GIT_OBJECT_DIRECTORY") or osp.join(common_dir, "objects")
git/repo/fun.py:103
gitdir, commondir, and HEAD; the .git file is in the working tree, not at d/gitfile. Replacing the HEAD check with gitfile makes this unsupported-worktree detection miss its documented layout when common storage is invalid. Keep checking HEAD here. and osp.isfile(osp.join(d, "gitfile"))
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
Codex on behalf of Byron: I also checked the three suppressed review notes. Commit 32baeab now rejects empty GIT_OBJECT_DIRECTORY and restores HEAD-based linked-worktree detection. I left dangling .git symlink behavior unchanged: Git setup.c read_gitfile_gently uses stat, so a dangling symlink is treated as missing and discovery falls through to the bare candidate or parent; local git rev-parse checks confirmed that behavior. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
Previously missed (2) — in code that hasn't changed since the last review.
git/repo/base.py:309
osp.lexists rather than osp.exists here. exists follows symlinks, so a dangling .git symlink makes this condition false; discovery then proceeds to is_git_dir(curpath) and can classify the directory as a bare repository (or continue to a parent) despite the invalid .git entry. That bypasses the precedence/rejection this guard is meant to enforce. if osp.exists(dotgit) and not osp.isdir(dotgit):
git/repo/base.py:300
.git pointers through find_submodule_git_dir before the bare-repository check, but that helper has no cycle detection. A malformed file containing gitdir: .git (or a two-file cycle) recurses until RecursionError instead of being rejected as invalid metadata; bound the resolution or track visited paths and return an invalid-candidate result. sm_gitpath = find_submodule_git_dir(dotgit)
git/repo/base.py:385
GIT_DIR is set to a relative path, discovery expands it to an absolute self.git_dir, but this conditional leaves the wrapper's inherited relative GIT_DIR unchanged unless GIT_COMMON_DIR is also set. Git commands run with self.working_dir, so they resolve that value from a different directory and can fail or address the wrong repository. Normalize GIT_DIR whenever it is supplied, independently of GIT_COMMON_DIR. if common_dir_env is not None:
self.git.update_environment(GIT_DIR=os.fspath(self.git_dir), GIT_COMMON_DIR=os.fspath(self.common_dir))
git/repo/base.py:300
find_submodule_git_dir reads the .git entry with the default text decoder and does not catch UnicodeError. Consequently, a .git file containing invalid UTF-8 raises UnicodeDecodeError from this new discovery path instead of being rejected as an invalid repository; use the same byte-oriented decoding approach as commondir or catch decode errors in the resolver. sm_gitpath = find_submodule_git_dir(dotgit)
git/repo/base.py:364
is_git_dir reads commondir with os.fsdecode, so a valid common-directory path containing non-UTF-8 filesystem bytes can pass discovery. The constructor then re-reads the same file with Path.read_text(), which raises UnicodeDecodeError instead of constructing the repository; use the same filesystem decoding here. common_dir = (Path(self.git_dir) / "commondir").read_text().splitlines()[0].strip()
git/repo/fun.py:93
commondir symlink as malformed, not as an absent file. read_bytes() raises FileNotFoundError for the dangling link, so this branch assigns d and can accept local HEAD/objects/refs; Repo then opens the candidate as a bare repository instead of rejecting the malformed metadata. Check lexists before taking the missing-file fallback. except FileNotFoundError:
common_dir = os.fspath(d)
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
Codex on behalf of Byron: I reviewed all six suppressed notes from the latest Copilot pass. Commit b8c000e fixes one-level, regular, size-bounded Gitfile resolution, filesystem decoding for .git and commondir metadata, and relative GIT_DIR persistence. Commit 83c3e64 fixes dangling commondir symlinks. I left dangling .git symlinks unchanged because Git read_gitfile_gently() uses stat and treats them as missing; the visible FIFO commondir thread likewise documents Git's file_exists()/strbuf_read_file() behavior. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
Previously missed (1) — in code that hasn't changed since the last review.
git/repo/base.py:310
osp.exists() returns False for a dangling symlink. Consequently, a worktree containing a broken .git symlink can still fall through to is_git_dir(curpath) and be accepted as a bare repository if it has HEAD, objects, and refs, defeating the malformed-.git guard above. Use osp.lexists(dotgit) so every non-directory .git entry blocks fallback. if osp.exists(dotgit) and not osp.isdir(dotgit):
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
git/repo/base.py:310
.git is a dangling symlink, find_submodule_git_dir() returns None and osp.exists(dotgit) is false, so this guard does not stop discovery. With search_parent_directories=True, the loop can then open an unrelated parent repository, contrary to the malformed .git rejection described by this change. Use osp.lexists(dotgit) so a present but unresolved non-directory .git entry blocks fallback. if osp.exists(dotgit) and not osp.isdir(dotgit):
git/repo/fun.py:130
st_size check is not sufficient to enforce the 1 MiB Gitfile limit: read_bytes() then reads the entire file without a bound. If a regular file grows or is replaced between stat and the read, repository discovery can allocate unbounded input and defeat the size safeguard. Open the file and read at most (1 << 20) + 1 bytes, rejecting an over-limit read. try:
content = os.fsdecode(Path(dotgit).read_bytes()).rstrip("\r\n")
except OSError:
return None
return content[8:] if len(content) >= 9 and content.startswith("gitdir: ") else None
git/repo/fun.py:110
GIT_OBJECT_DIRECTORY as the repository's object store, but Repo does not carry that value into construction: its ODB is still rooted at common_dir/objects and the Git wrapper only pins GIT_DIR/GIT_COMMON_DIR. A repository whose objects exist only in GIT_OBJECT_DIRECTORY can therefore use a different or nonexistent object database (and a relative value will break when the process cwd changes). Resolve and preserve GIT_OBJECT_DIRECTORY as well, or reject this environment mode consistently. object_dir = os.getenv("GIT_OBJECT_DIRECTORY")
if object_dir is None:
object_dir = osp.join(common_dir, "objects")
if valid_head and osp.isdir(object_dir) and osp.isdir(osp.join(common_dir, "refs")):
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Previously missed (2) — in code that hasn't changed since the last review.
git/repo/base.py:388
GIT_OBJECT_DIRECTORY is validated during discovery, but it is not pinned in the Git wrapper's environment. When this value is relative and Repo(path) gives the wrapper a different cwd, discovery checks the path relative to the caller's cwd while later Git commands resolve it relative to the repository's working directory, so object lookups can target a different store. Resolve and propagate this variable along with GIT_DIR and GIT_COMMON_DIR. if common_dir_env is not None:
self.git.update_environment(GIT_DIR=os.fspath(self.git_dir), GIT_COMMON_DIR=os.fspath(self.common_dir))
elif git_dir_env is not None:
self.git.update_environment(GIT_DIR=os.fspath(self.git_dir))
git/repo/base.py:310
osp.exists follows symlinks, so a dangling .git symlink makes this guard false. If the current directory also has bare-repository markers (or parent search is enabled), discovery can then fall through and accept that directory/parent even though an invalid .git entry is present, defeating the precedence check above. Use lexists so dangling metadata is treated as an existing non-directory entry, as is already done for commondir in fun.py. if osp.exists(dotgit) and not osp.isdir(dotgit):
git/repo/fun.py:110
GIT_OBJECT_DIRECTORY as the repository's object store, but the constructor still creates the object database from self.common_dir/objects (base.py:391). A valid repository whose objects exist only in this environment-provided directory will therefore be opened with an unusable GitDB (and an inaccurate GitCmdObjectDB.root_path()); either use the resolved object-directory environment value when constructing the ODB or reject this configuration consistently. object_dir = os.getenv("GIT_OBJECT_DIRECTORY")
if object_dir is None:
object_dir = osp.join(common_dir, "objects")
if valid_head and osp.isdir(object_dir) and osp.isdir(osp.join(common_dir, "refs")):
git/repo/fun.py:129
os.stat; Path.read_bytes() reopens the path and reads to EOF. A concurrent replacement or growth can therefore bypass the 1 MiB limit and make repository discovery read an arbitrarily large Gitfile. Read from one descriptor with a bound of statbuf.st_size (and reject a short read), as Git does. try:
content = os.fsdecode(Path(dotgit).read_bytes()).rstrip("\r\n")
except (OSError, UnicodeError):
return None
test/test_repo.py:139
Repo.init(path) leaves a valid path/.git directory in place, so even the bare layout is resolved by find_submodule_git_dir(path/.git) before the new implicit-bare branch at base.py:313 runs. Add a bare-only directory without .git and assert Repo(path) discovers it, otherwise this new discovery path has no positive regression coverage. Repo.init(path).close()
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
git/repo/base.py:391
cwd while receiving a relative GIT_DIR. For example, GIT_DIR=git with a commondir file makes working_dir the common directory, so Git resolves GIT_DIR=git below that directory instead of the original repository; the same applies when only GIT_COMMON_DIR is set and git_dir came from a relative path. Normalize GIT_DIR before storing it in both branches. if common_dir_env is not None:
self.git.update_environment(GIT_DIR=os.fspath(self.git_dir), GIT_COMMON_DIR=os.fspath(self.common_dir))
elif git_dir_env is not None:
self.git.update_environment(GIT_DIR=os.fspath(self.git_dir))
git/repo/base.py:314
.git exists as a malformed directory, find_submodule_git_dir(dotgit) returns None, but this condition does not stop discovery because the entry is a directory. A worktree containing repository-looking HEAD, objects, and refs can then be accepted as a bare repository, defeating the new malformed-.git precedence rule; osp.exists also misses dangling .git symlinks. Treat any present but unresolved .git entry as terminal (using lexists) before checking curpath as a bare repository. if osp.exists(dotgit) and not osp.isdir(dotgit):
break
git/repo/base.py:307
find_submodule_git_dir() has already resolved a relative gitdir: target against dirname(d) (lines 147-149). When Repo is opened with a relative path, sm_gitpath is therefore relative to the process cwd, but joining curpath again prefixes the worktree a second time and selects a nonexistent/wrong admin directory. Use the helper's resolved path directly here so relative Gitfiles work with relative Repo paths. git_dir = osp.normpath(osp.join(curpath, os.fspath(sm_gitpath)))
git/repo/base.py:369
common_dir here even though Git's get_common_dir_noenv() removes them. Once discovery accepts such a file, this leaves self.common_dir pointing at the wrong path and causes config/object access to diverge from Git; use rstrip() here as well. try:
common_dir = os.fsdecode((Path(self.git_dir) / "commondir").read_bytes()).rstrip("\r\n")
self._common_dir = osp.join(self.git_dir, common_dir)
git/repo/fun.py:130
read_gitfile_gently() trims trailing whitespace from the gitdir record, not just CR/LF. With this normalization, a valid .git file such as gitdir: /path/to/repo \n is rejected because the target retains the trailing space, so discovery diverges from Git. Strip all trailing whitespace here before extracting the target. content = os.fsdecode(Path(dotgit).read_bytes()).rstrip("\r\n")
except (OSError, UnicodeError):
return None
return content[8:] if len(content) >= 9 and content.startswith("gitdir: ") else None
git/repo/fun.py:93
get_common_dir_noenv() applies Git's trailing-whitespace trimming to the commondir file. Leaving spaces or tabs in common_dir makes this check look in the wrong directory and rejects repositories that Git opens successfully; use the same trimming rule as the later Repo construction path. common_dir_file = Path(d) / "commondir"
try:
common_dir = os.fsdecode(common_dir_file.read_bytes()).rstrip("\r\n")
except FileNotFoundError:
test/test_repo.py:252
cwd(tdir) and the patched environment are still active, so relative GIT_DIR/GIT_OBJECT_DIRECTORY values would still resolve correctly even if the wrapper failed to retain absolute paths. Move the repository and GitDB assertions outside both context managers to actually cover the preservation behavior this regression test is intended to protect. assert repo.git.cat_file("blob", blob_hexsha) == payload.decode()
test/test_repo.py:223
cwd(tdir)/environment context, so it does not verify that the relative GIT_DIR remains usable after construction changes the execution context. Place the assertion after the context managers (as in the preceding environment case) to cover the wrapper path normalization. assert osp.samefile(repo.git.rev_parse("--absolute-git-dir"), git_dir)
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
git/repo/base.py:396
GIT_OBJECT_DIRECTORY is accepted as the active object store and used for rootpath, Repo.alternates still reads and writes common_dir/objects/info/alternates. With objects relocated to the environment-provided directory, that API silently ignores the active alternates file or modifies the wrong repository; derive this path from the resolved object root as well. rootpath = object_dir_env if object_dir_env is not None else osp.join(self.common_dir, "objects")
git/repo/base.py:313
osp.exists here misses a dangling .git symlink: find_submodule_git_dir returns None, then this guard is skipped and the loop can still accept curpath as a bare repository if it has HEAD/objects/refs. That bypasses the new malformed-.git no-fallback rule; use osp.lexists (or an equivalent lstat check) for this guard. if osp.exists(dotgit) and not osp.isdir(dotgit):
git/repo/base.py:391
git_dir_env and common_dir_env are both unset, this code does not pin the resolved self.git_dir into the command wrapper. A valid repository opened from a separate git directory via core.worktree can therefore have self.working_dir set to its worktree while that worktree has no .git entry; subsequent repo.git commands run without GIT_DIR and fail discovery. Preserve the resolved GIT_DIR for path-based discovery too. if common_dir_env is not None:
self.git.update_environment(GIT_DIR=os.fspath(self.git_dir), GIT_COMMON_DIR=os.fspath(self.common_dir))
elif git_dir_env is not None:
self.git.update_environment(GIT_DIR=os.fspath(self.git_dir))
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
git/repo/base.py:313
.git symlink is a non-directory entry, but osp.exists() follows symlinks and returns False for it. Since find_submodule_git_dir() has already rejected the dangling target, discovery then reaches is_git_dir(curpath) and can still misclassify a worktree with objects/refs/HEAD as a bare repository. Use a lexists-style check for this guard so any present, invalid .git entry stops fallback. self._working_tree_dir = curpath
git/repo/base.py:391
commondir file and neither GIT_COMMON_DIR nor GIT_DIR was set, neither branch initializes the wrapper environment. For a split bare repository, working_dir becomes self.common_dir, which may have no HEAD or .git, so later repo.git commands fail repository discovery even though construction succeeded. Pin the resolved GIT_DIR/GIT_COMMON_DIR whenever the resolved common directory differs from the git directory, not only when those variables were inherited. self._working_tree_dir = None
# END working dir handling
self.working_dir: PathLike = self._working_tree_dir or self.common_dir
git/repo/fun.py:127
read_gitfile_gently() trims trailing whitespace from the single gitdir: record, so a valid .git file such as gitdir: /path/to/git \n is accepted by Git. This parser only strips CR/LF and returns a target with the trailing space, causing is_git_dir() to reject an otherwise valid worktree; trim the same whitespace set before extracting the target. with open(dotgit, "rb") as fp:
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
<!-- agent --> GitPython considered worktree administration and bare-repository signatures before a worktree's real .git entry. Align discovery with Git so .git files and directories win, malformed .git files stop discovery, and candidate git directories validate HEAD plus commondir-backed object and ref storage. This addresses GHSA-239g-whfq-7xj9. Regression coverage compares ambiguous layouts with git rev-parse and rejects invalid HEAD/.git metadata. Git baseline: 15c6308cf7ad276b306aa5b3ababfbdebfb1a917; setup.c setup_git_directory_gently_1(), is_git_directory(), and validate_headref(). Repository validation can use GIT_COMMON_DIR for refs and objects. Preserve the same value on Repo so later config, ref, and object access uses the directory that made discovery succeed. Capture GIT_COMMON_DIR before the first repository config read so bare-state detection uses the same metadata location as discovery. Resolve relative environment values immediately so later working-directory changes cannot retarget the Repo. Review feedback: relative GIT_COMMON_DIR left Git subprocesses resolving GIT_DIR and GIT_COMMON_DIR from a different working directory; malformed commondir data, empty GIT_OBJECT_DIRECTORY, and the linked-worktree signature were also handled inconsistently. Pin the repository environment to resolved paths, reject invalid metadata without consulting the process working directory, and restore HEAD-based linked-worktree detection. Keep the loose HEAD and dangling .git behavior because both match Git setup.c at baseline 15c6308cf7ad276b306aa5b3ababfbdebfb1a917. Review feedback identified that chained or self-referential .git pointers recurse, filesystem-encoded metadata can fail text decoding, and a relative GIT_DIR is not retained for later Git commands. Parse one regular, size-bounded Gitfile exactly once, decode Gitfile and commondir paths with the filesystem codec, and retain the resolved GIT_DIR for subprocesses. This rejects cycles like Git instead of recursing and keeps commands stable after working-directory changes. Review feedback noted that a dangling commondir symlink was treated as absent, allowing local objects and refs to validate the repository. Distinguish a truly missing commondir from a dangling symlink. This follows Git's get_common_dir_noenv(), whose file_exists check uses lstat before attempting to read the entry. Python 3.9 on Windows raised UnicodeDecodeError while the repository-discovery regression test parsed invalid commondir bytes, causing the Python package test (windows, 3.9) check to fail. Treat UnicodeError like an unreadable metadata file in both commondir and gitfile parsing. Invalid bytes now make discovery reject the candidate repository, matching Git's behavior. Review feedback noted that GIT_OBJECT_DIRECTORY made discovery succeed without becoming the Repo ODB root, while a relative value could later be resolved from the Git wrapper's different working directory. Resolve the environment value against the construction directory, use it as the ODB root, and preserve the absolute value for later Git commands. The regression moves the only object store outside the git directory and verifies access through both GitDB and git cat-file after the original environment and current directory are restored. Review feedback identified four setup mismatches: non-missing .git stat failures could fall through to another repository, explicit GIT_DIR could be redirected through a nested .git entry, Gitfile reads were not bounded to the stat-reported size, and alternates ignored GIT_OBJECT_DIRECTORY. Match Git setup.c by bypassing discovery for an environment-selected GIT_DIR, stopping discovery when Gitfile stat fails for reasons other than a missing path, and reading exactly the previously observed Gitfile size. Resolve alternates below the active ODB root so custom object stores remain internally consistent. The commit review noted that treating every present GIT_DIR as explicit broke two documented cases: an empty GIT_DIR must fall back to current-directory discovery, while an empty Repo path must still use a nonempty GIT_DIR. Assisted-by: GPT 5.6 Co-authored-by: GPT 5.6 <[email protected]>
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.