Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit 38f91f0

Browse filesBrowse files
codexByron
codex
authored and
Byron
committed
address review feedback and CI failures
Consolidate follow-up fixes from review and CI: - fix lint and mypy issues in reference log path handling - validate remote reference paths before invoking git branch deletion - add symlink escape coverage where realpath resolves symlinks - ensure temporary test repositories release git resources during cleanup
1 parent 25ba54d commit 38f91f0
Copy full SHA for 38f91f0

3 files changed

+67-34Lines changed: 67 additions & 34 deletions

File tree

Expand file treeCollapse file tree
Open diff view settings
Filter options
Expand file treeCollapse file tree
Open diff view settings
Collapse file

‎git/refs/log.py‎

Copy file name to clipboardExpand all lines: git/refs/log.py
+2-3Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,6 @@
44
__all__ = ["RefLog", "RefLogEntry"]
55

66
from mmap import mmap
7-
import os.path as osp
87
import re
98
import time as _time
109

@@ -26,7 +25,7 @@
2625

2726
# typing ------------------------------------------------------------------
2827

29-
from typing import Iterator, List, Tuple, TYPE_CHECKING, Union
28+
from typing import Iterator, List, Tuple, TYPE_CHECKING, Union, cast
3029

3130
from git.types import PathLike
3231

@@ -213,7 +212,7 @@ def path(cls, ref: "SymbolicReference") -> str:
213212
:param ref:
214213
:class:`~git.refs.symbolic.SymbolicReference` instance
215214
"""
216-
return to_native_path(ref._get_validated_reflog_path(ref.repo, ref.path))
215+
return cast(str, to_native_path(ref._get_validated_reflog_path(ref.repo, ref.path)))
217216

218217
@classmethod
219218
def iter_entries(cls, stream: Union[str, "BytesIO", mmap]) -> Iterator[RefLogEntry]:
Collapse file

‎git/refs/remote.py‎

Copy file name to clipboardExpand all lines: git/refs/remote.py
+3-1Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -58,12 +58,14 @@ def delete(cls, repo: "Repo", *refs: "RemoteReference", **kwargs: Any) -> None:
5858
`kwargs` are given for comparability with the base class method as we
5959
should not narrow the signature.
6060
"""
61+
for ref in refs:
62+
cls._check_ref_name_valid(ref.path)
63+
6164
repo.git.branch("-d", "-r", *refs)
6265
# The official deletion method will ignore remote symbolic refs - these are
6366
# generally ignored in the refs/ folder. We don't though and delete remainders
6467
# manually.
6568
for ref in refs:
66-
cls._check_ref_name_valid(ref.path)
6769
try:
6870
os.remove(cls._get_validated_path(repo.common_dir, ref.path))
6971
except OSError:
Collapse file

‎test/test_refs.py‎

Copy file name to clipboardExpand all lines: test/test_refs.py
+62-30Lines changed: 62 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
# This module is part of GitPython and is released under the
44
# 3-Clause BSD License: https://opensource.org/license/bsd-3-clause/
55

6+
import contextlib
67
from itertools import chain
78
import os.path as osp
89
from pathlib import Path
@@ -30,13 +31,17 @@
3031

3132

3233
class TestRefs(TestBase):
34+
@contextlib.contextmanager
3335
def _repo_with_initial_commit(self, base_dir):
3436
repo_dir = base_dir / "repo"
3537
repo = Repo.init(repo_dir)
3638
(repo_dir / "file.txt").write_text("initial\n", encoding="utf-8")
3739
repo.index.add(["file.txt"])
3840
repo.index.commit("initial")
39-
return repo
41+
try:
42+
yield repo
43+
finally:
44+
repo.git.clear_cache()
4045

4146
def test_from_path(self):
4247
# Should be able to create any reference directly.
@@ -660,60 +665,84 @@ def test_refs_outside_repo(self):
660665
def test_reference_create_rejects_path_traversal(self):
661666
with tempfile.TemporaryDirectory() as tmp_dir:
662667
base_dir = Path(tmp_dir)
663-
repo = self._repo_with_initial_commit(base_dir)
664-
outside_path = base_dir / "outside_write.txt"
668+
with self._repo_with_initial_commit(base_dir) as repo:
669+
outside_path = base_dir / "outside_write.txt"
665670

666-
self.assertRaises(ValueError, Reference.create, repo, "../../../outside_write.txt", "HEAD")
667-
assert not outside_path.exists()
671+
self.assertRaises(ValueError, Reference.create, repo, "../../../outside_write.txt", "HEAD")
672+
assert not outside_path.exists()
668673

669674
def test_symbolic_reference_create_rejects_path_traversal(self):
670675
with tempfile.TemporaryDirectory() as tmp_dir:
671676
base_dir = Path(tmp_dir)
672-
repo = self._repo_with_initial_commit(base_dir)
673-
outside_path = base_dir / "outside_write.txt"
677+
with self._repo_with_initial_commit(base_dir) as repo:
678+
outside_path = base_dir / "outside_write.txt"
674679

675-
self.assertRaises(ValueError, SymbolicReference.create, repo, "../../outside_write.txt", "HEAD")
676-
assert not outside_path.exists()
680+
self.assertRaises(ValueError, SymbolicReference.create, repo, "../../outside_write.txt", "HEAD")
681+
assert not outside_path.exists()
677682

678683
def test_symbolic_reference_set_reference_rejects_path_traversal(self):
679684
with tempfile.TemporaryDirectory() as tmp_dir:
680685
base_dir = Path(tmp_dir)
681-
repo = self._repo_with_initial_commit(base_dir)
682-
outside_path = base_dir / "outside_write.txt"
686+
with self._repo_with_initial_commit(base_dir) as repo:
687+
outside_path = base_dir / "outside_write.txt"
683688

684-
self.assertRaises(ValueError, SymbolicReference(repo, "../../outside_write.txt").set_reference, "HEAD")
685-
assert not outside_path.exists()
689+
self.assertRaises(ValueError, SymbolicReference(repo, "../../outside_write.txt").set_reference, "HEAD")
690+
assert not outside_path.exists()
686691

687692
def test_symbolic_reference_rename_rejects_path_traversal(self):
688693
with tempfile.TemporaryDirectory() as tmp_dir:
689694
base_dir = Path(tmp_dir)
690-
repo = self._repo_with_initial_commit(base_dir)
691-
outside_path = base_dir / "outside_move.txt"
692-
ref = SymbolicReference.create(repo, "SAFE_RENAME_SOURCE", "HEAD")
695+
with self._repo_with_initial_commit(base_dir) as repo:
696+
outside_path = base_dir / "outside_move.txt"
697+
ref = SymbolicReference.create(repo, "SAFE_RENAME_SOURCE", "HEAD")
693698

694-
self.assertRaises(ValueError, ref.rename, "../../outside_move.txt")
695-
assert not outside_path.exists()
696-
assert Path(ref.abspath).is_file()
699+
self.assertRaises(ValueError, ref.rename, "../../outside_move.txt")
700+
assert not outside_path.exists()
701+
assert Path(ref.abspath).is_file()
697702

698703
def test_symbolic_reference_delete_rejects_path_traversal(self):
699704
with tempfile.TemporaryDirectory() as tmp_dir:
700705
base_dir = Path(tmp_dir)
701-
repo = self._repo_with_initial_commit(base_dir)
702-
outside_path = base_dir / "outside_delete.txt"
703-
outside_path.write_text("do not delete\n", encoding="utf-8")
706+
with self._repo_with_initial_commit(base_dir) as repo:
707+
outside_path = base_dir / "outside_delete.txt"
708+
outside_path.write_text("do not delete\n", encoding="utf-8")
704709

705-
self.assertRaises(ValueError, SymbolicReference.delete, repo, "../../outside_delete.txt")
706-
assert outside_path.read_text(encoding="utf-8") == "do not delete\n"
710+
self.assertRaises(ValueError, SymbolicReference.delete, repo, "../../outside_delete.txt")
711+
assert outside_path.read_text(encoding="utf-8") == "do not delete\n"
707712

708713
def test_symbolic_reference_log_append_rejects_path_traversal(self):
709714
with tempfile.TemporaryDirectory() as tmp_dir:
710715
base_dir = Path(tmp_dir)
711-
repo = self._repo_with_initial_commit(base_dir)
712-
outside_path = base_dir / "outside_reflog.txt"
716+
with self._repo_with_initial_commit(base_dir) as repo:
717+
outside_path = base_dir / "outside_reflog.txt"
718+
719+
ref = SymbolicReference(repo, "../../../outside_reflog.txt")
720+
self.assertRaises(
721+
ValueError, ref.log_append, Commit.NULL_BIN_SHA, "do not write", repo.head.commit.binsha
722+
)
723+
assert not outside_path.exists()
713724

714-
ref = SymbolicReference(repo, "../../../outside_reflog.txt")
715-
self.assertRaises(ValueError, ref.log_append, Commit.NULL_BIN_SHA, "do not write", repo.head.commit.binsha)
716-
assert not outside_path.exists()
725+
def test_symbolic_reference_set_reference_rejects_symlink_escape(self):
726+
with tempfile.TemporaryDirectory() as tmp_dir:
727+
base_dir = Path(tmp_dir)
728+
with self._repo_with_initial_commit(base_dir) as repo:
729+
outside_dir = base_dir / "outside_refs"
730+
outside_dir.mkdir()
731+
outside_path = outside_dir / "escaped"
732+
733+
refs_heads_dir = Path(repo.common_dir) / "refs" / "heads"
734+
refs_heads_dir.mkdir(parents=True, exist_ok=True)
735+
symlink_path = refs_heads_dir / "link_out"
736+
try:
737+
symlink_path.symlink_to(outside_dir, target_is_directory=True)
738+
except (OSError, NotImplementedError) as ex:
739+
self.skipTest("symlinks unavailable on this platform: %s" % ex)
740+
if osp.realpath(symlink_path / "escaped") == osp.abspath(symlink_path / "escaped"):
741+
self.skipTest("realpath does not resolve directory symlinks on this platform")
742+
743+
ref = SymbolicReference(repo, "refs/heads/link_out/escaped")
744+
self.assertRaises(ValueError, ref.set_reference, "HEAD")
745+
assert not outside_path.exists()
717746

718747
def test_remote_reference_delete_cleanup_rejects_path_traversal(self):
719748
with tempfile.TemporaryDirectory() as tmp_dir:
@@ -724,8 +753,10 @@ def test_remote_reference_delete_cleanup_rejects_path_traversal(self):
724753
outside_path.write_text("do not delete\n", encoding="utf-8")
725754

726755
class GitStub:
756+
branch_called = False
757+
727758
def branch(self, *args):
728-
pass
759+
self.branch_called = True
729760

730761
class RepoStub:
731762
pass
@@ -737,6 +768,7 @@ class RepoStub:
737768
ref = RemoteReference(repo, "../../outside_remote_delete.txt", check_path=False)
738769

739770
self.assertRaises(ValueError, RemoteReference.delete, repo, ref)
771+
assert not repo.git.branch_called
740772
assert outside_path.read_text(encoding="utf-8") == "do not delete\n"
741773

742774
def test_validity_ref_names(self):

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.