review

- speedup additional .gitmodules check

Author: Byron

git/index/base.py

@@ -728,8 +728,10 @@ def _preprocess_add_items(
             else:
                 raise TypeError("Invalid Type: %r" % item)
         # END for each item
+        # Source paths must be safe to read, but their recorded names may be rewritten.
+        # Apply mode-dependent restrictions to the final entries in add().
         for entry in entries:
-            _validate_repo_path(entry.path, entry.mode)
+            _validate_repo_path(entry.path)
         return paths, entries
 
     def _store_path(self, filepath: PathLike, fprogress: Callable) -> BaseIndexEntry:

git/util.py

@@ -29,63 +29,62 @@
 if sys.platform == "win32":
     __all__.append("to_native_path_windows")
 
-from abc import abstractmethod
 import contextlib
-from functools import wraps
 import getpass
 import logging
 import ntpath
 import os
 import os.path as osp
-from pathlib import Path
 import platform
 import re
 import shutil
 import stat
 import subprocess
 import time
-from urllib.parse import urlsplit, urlunsplit
 import warnings
-
-# NOTE: Unused imports can be improved now that CI testing has fully resumed. Some of
-# these be used indirectly through other GitPython modules, which avoids having to write
-# gitdb all the time in their imports. They are not in __all__, at least currently,
-# because they could be removed or changed at any time, and so should not be considered
-# conceptually public to code outside GitPython. Linters of course do not like it.
-from gitdb.util import (
-    LazyMixin,  # noqa: F401
-    LockedFD,  # noqa: F401
-    bin_to_hex,  # noqa: F401
-    file_contents_ro,  # noqa: F401
-    file_contents_ro_filepath,  # noqa: F401
-    hex_to_bin,  # noqa: F401
-    make_sha,
-    to_bin_sha,  # noqa: F401
-    to_hex_sha,  # noqa: F401
-)
+from abc import abstractmethod
+from functools import wraps
+from pathlib import Path
 
 # typing ---------------------------------------------------------
-
 from typing import (
+    IO,
+    TYPE_CHECKING,
     Any,
     AnyStr,
     Callable,
     Dict,
     Generator,
-    IO,
     Iterator,
     List,
     Optional,
     Pattern,
     Sequence,
     Tuple,
-    TYPE_CHECKING,
     Type,
     TypeVar,
     Union,
     cast,
     overload,
 )
+from urllib.parse import urlsplit, urlunsplit
+
+# NOTE: Unused imports can be improved now that CI testing has fully resumed. Some of
+# these be used indirectly through other GitPython modules, which avoids having to write
+# gitdb all the time in their imports. They are not in __all__, at least currently,
+# because they could be removed or changed at any time, and so should not be considered
+# conceptually public to code outside GitPython. Linters of course do not like it.
+from gitdb.util import (
+    LazyMixin,  # noqa: F401
+    LockedFD,  # noqa: F401
+    bin_to_hex,  # noqa: F401
+    file_contents_ro,  # noqa: F401
+    file_contents_ro_filepath,  # noqa: F401
+    hex_to_bin,  # noqa: F401
+    make_sha,
+    to_bin_sha,  # noqa: F401
+    to_hex_sha,  # noqa: F401
+)
 
 if TYPE_CHECKING:
     from git.cmd import Git
@@ -94,9 +93,9 @@
     from git.repo.base import Repo
 
 from git.types import (
+    HSH_TD,
     Files_TD,
     Has_id_attribute,
-    HSH_TD,
     Literal,
     PathLike,
     Protocol,
@@ -385,11 +384,13 @@ def _to_relative_path(root: PathLike, path: PathLike) -> str:
     "", "", "\u200c\u200d\u200e\u200f\u202a\u202b\u202c\u202d\u202e\u206a\u206b\u206c\u206d\u206e\u206f\ufeff"
 )
 
-# Windows derives an 8.3 short name for ".gitmodules" from its first six
-# characters followed by "~1" through "~4", and from a hashed stem once those
-# are taken. Git recognizes both spellings (is_ntfs_dotgitmodules in path.c).
-_NTFS_DOTGITMODULES_SHORT_NAMES = frozenset(
-    ["gitmod~%d" % index for index in range(1, 5)] + ["gi7eba~%d" % index for index in range(1, 10)]
+# Match Git's is_ntfs_dotgitmodules in path.c on a lowercased, trimmed name.
+# Besides gitmod~1..4, fallback aliases have exactly eight ASCII characters:
+# a shrinking prefix of "gi7eba", "~", and digits with no leading zero.
+# Explicit digit counts avoid accepting shorter/longer names or Unicode digits.
+_NTFS_DOTGITMODULES_SHORT_NAME = re.compile(
+    r"(?:gitmod~[1-4]|gi7eba~[1-9]|gi7eb~[1-9][0-9]|gi7e~[1-9][0-9]{2}|"
+    r"gi7~[1-9][0-9]{3}|gi~[1-9][0-9]{4}|g~[1-9][0-9]{5}|~[1-9][0-9]{6})"
 )
 
 
@@ -419,11 +420,13 @@ def _validate_repo_path(path: PathLike, mode: Union[int, None] = None) -> None:
             hfs_name = part.translate(_HFS_IGNORABLES).lower()
             if ntfs_name in (".git", "git~1") or hfs_name == ".git":
                 raise ValueError("Repository path aliases Git metadata: %r" % name)
-            aliases_gitmodules = (
-                ntfs_name == ".gitmodules" or hfs_name == ".gitmodules" or ntfs_name in _NTFS_DOTGITMODULES_SHORT_NAMES
-            )
-            if aliases_gitmodules and mode is not None and stat.S_ISLNK(mode):
-                raise ValueError("Symbolic link aliases the submodule configuration: %r" % name)
+            if mode is not None and stat.S_ISLNK(mode):
+                if (
+                    ntfs_name == ".gitmodules"
+                    or hfs_name == ".gitmodules"
+                    or _NTFS_DOTGITMODULES_SHORT_NAME.fullmatch(ntfs_name) is not None
+                ):
+                    raise ValueError("Symbolic link aliases the submodule configuration: %r" % name)
 
 
 def assure_directory_exists(path: PathLike, is_file: bool = False) -> bool:

test/test_index.py

@@ -350,6 +350,21 @@ def test_index_reader_and_writer_reject_unsafe_paths(self, path):
         "gitmod~4",
         "gi7eba~1",
         "gi7eba~9",
+        "GI7EB~10",
+        "GI7EB~99",
+        "GI7E~100",
+        "GI7E~999",
+        "GI7~1000",
+        "GI7~9999",
+        "GI~10000",
+        "GI~99999",
+        "G~100000",
+        "G~999999",
+        "~1000000",
+        "~9999999",
+        "GI7EB~10. ",
+        "GI7E~100:$DATA",
+        "sub/~1000000",
         "sub/.gitmodules",
     )
     def test_index_reader_and_writer_reject_gitmodules_symlinks(self, path):
@@ -367,6 +382,26 @@ def test_index_reader_and_writer_reject_gitmodules_symlinks(self, path):
         stream.seek(0)
         assert next(iter(read_cache(stream)[1])) == (path, 0)
 
+    @ddt.data("GI7EB~10", "GI7E~100", "GI7~1000", "GI~10000", "G~100000", "~1000000", "~9999999")
+    @with_rw_directory
+    def test_index_add_and_write_tree_reject_gitmodules_fallback_symlinks(self, rw_dir, path):
+        with Repo.init(rw_dir) as repo:
+            binsha = repo.odb.store(IStream("blob", 6, BytesIO(b"target"))).binsha
+            index = repo.index
+            for item in (Blob(repo, binsha, 0o120000, path), BaseIndexEntry((0o120000, binsha, 0, path))):
+                with pytest.raises(ValueError, match="submodule configuration"):
+                    index.add([item], write=False)
+                assert not index.entries
+
+            index.entries[(path, 0)] = IndexEntry((0o120000, binsha, 0, path))
+            with pytest.raises(ValueError, match="submodule configuration"):
+                index.write_tree()
+            assert not Path(index.path).exists()
+
+            index.add([BaseIndexEntry((0o100644, binsha, 0, path))])
+            assert repo.index.entries[(path, 0)].mode == 0o100644
+            assert index.write_tree()[path].mode == 0o100644
+
     def test_valid_unusual_index_names_round_trip(self):
         names = ["a b", "a\nb", "a\tb", "name:value", "dir/.gitignore", "café"]
         if os.name != "nt":
@@ -408,20 +443,26 @@ def _cmp_tree_index(self, tree, index):
 
     @with_rw_repo("0.1.6")
     def test_index_lock_handling(self, rw_repo):
-        def add_bad_blob():
-            rw_repo.index.add([Blob(rw_repo, b"f" * 20, "bad-permissions", "foo")])
-
-        try:
-            ## First, fail on purpose adding into index.
-            add_bad_blob()
-        except Exception as ex:
-            assert "required argument is not an integer" in str(ex)
-
-        ## The second time should not fail due to stray lock file.
-        try:
-            add_bad_blob()
-        except Exception as ex:
-            assert "index.lock' could not be obtained" not in str(ex)
+        index = rw_repo.index
+        index_path = Path(index.path)
+        lock_path = Path(str(index_path) + ".lock")
+        before = index_path.read_bytes()
+
+        def fail_serialize(stream, ignore_extension_data):
+            assert lock_path.exists()
+            stream.write(b"partial index")
+            raise OSError("simulated index write failure")
+
+        with mock.patch.object(IndexFile, "_serialize", side_effect=fail_serialize):
+            for _ in range(2):
+                with pytest.raises(OSError, match="simulated index write failure"):
+                    index.write()
+                assert not lock_path.exists()
+                assert index_path.read_bytes() == before
+
+        index.add([Blob(rw_repo, b"f" * 20, 0o100644, "foo")])
+        assert not lock_path.exists()
+        assert rw_repo.index.entries[("foo", 0)].mode == 0o100644
 
     @with_rw_repo("0.1.6")
     def test_read_tree_methods_reject_index_output(self, rw_repo):
@@ -1248,6 +1289,60 @@ def test_staging_rejects_unsafe_object_paths_even_without_writing(self, rw_dir,
                 index.add([item], write=False, **kwargs)
             assert not index.entries
 
+    @ddt.data(*product(("path", "blob", "entry", "stored-blob", "stored-entry"), (False, True), (False, True)))
+    @ddt.unpack
+    @with_rw_directory
+    def test_staging_gitmodules_symlink_check_uses_rewritten_path(self, rw_dir, kind, unsafe_destination, write):
+        with Repo.init(rw_dir) as repo:
+            source, destination = ("safe-link", ".gitmodules") if unsafe_destination else (".gitmodules", "safe-link")
+            binsha = Blob.NULL_BIN_SHA
+            if kind.startswith("stored-"):
+                binsha = repo.odb.store(IStream("blob", 6, BytesIO(b"target"))).binsha
+            else:
+                try:
+                    (Path(rw_dir) / source).symlink_to("target")
+                except OSError:
+                    pytest.skip("Symlinks unavailable")
+            if kind == "path":
+                item = source
+            elif kind.endswith("blob"):
+                item = Blob(repo, binsha, 0o120000, source)
+            else:
+                item = BaseIndexEntry((0o120000, binsha, 0, source))
+            index = repo.index
+            rewriter = mock.Mock(return_value=destination)
+            if unsafe_destination:
+                with pytest.raises(ValueError, match="submodule configuration"):
+                    index.add([item], path_rewriter=rewriter, write=write)
+                assert not index.entries
+                assert not Path(index.path).exists()
+            else:
+                added = index.add([item], path_rewriter=rewriter, write=write)
+                assert [(entry.path, entry.mode) for entry in added] == [(destination, 0o120000)]
+                assert set(index.entries) == {(destination, 0)}
+                assert index.write_tree()[destination].mode == 0o120000
+                if write:
+                    assert repo.index.entries[(destination, 0)].mode == 0o120000
+            rewriter.assert_called_once()
+            assert rewriter.call_args[0][0].path == source
+
+    @ddt.data(*product(("blob", "entry"), ("../outside", ".git/config")))
+    @ddt.unpack
+    @with_rw_directory
+    def test_staging_rewriter_cannot_sanitize_unsafe_source_paths(self, rw_dir, kind, path):
+        with Repo.init(rw_dir) as repo:
+            item = (
+                Blob(repo, b"a" * 20, 0o120000, path)
+                if kind == "blob"
+                else BaseIndexEntry((0o120000, b"a" * 20, 0, path))
+            )
+            index = repo.index
+            rewriter = mock.Mock(return_value="safe-link")
+            with pytest.raises(ValueError):
+                index.add([item], path_rewriter=rewriter, write=False)
+            rewriter.assert_not_called()
+            assert not index.entries
+
     @with_rw_directory
     def test_staging_root_preserves_symlinks_and_skips_git_metadata(self, rw_dir):
         tmp_path = Path(rw_dir)

test/test_tree.py

@@ -138,7 +138,38 @@ def test_tree_names_are_checked_at_construction_and_serialization(self, name):
         with pytest.raises(ValueError):
             tree_to_stream([(b"a" * 20, 0o100644, name)], BytesIO().write)
 
-    @ddt.data(".gitmodules", ".GITMODULES", ".gitmodules ", ".gi\u200ctmodules", "gitmod~1", "gi7eba~1")
+    @ddt.data(
+        ".gitmodules",
+        ".GITMODULES",
+        ".gitmodules ",
+        ".gi\u200ctmodules",
+        "gitmod~1",
+        "gitmod~2",
+        "gitmod~3",
+        "GITMOD~4",
+        "gi7eba~1",
+        "GI7EBA~9",
+        "GI7EB~10",
+        "GI7EB~11",
+        "GI7EB~99",
+        "GI7E~100",
+        "GI7E~101",
+        "GI7E~999",
+        "GI7~1000",
+        "GI7~9999",
+        "GI~10000",
+        "GI~99999",
+        "G~100000",
+        "G~999999",
+        "~1000000",
+        "~9999999",
+        "Gi7Eb~42",
+        "gi7e~120",
+        "GITMOD~4 . ",
+        "GI7EB~10. ",
+        "GI7E~100:$DATA",
+        "~1000000 . :$DATA",
+    )
     def test_gitmodules_symlink_entries_are_rejected(self, name):
         """A symbolic link named like the submodule configuration would make Git read
         it from outside the repository, so such an entry is refused in both
@@ -160,6 +191,60 @@ def test_gitmodules_symlink_entries_are_rejected(self, name):
         tree_to_stream(cache, data.write)
         assert tree_entries_from_data(data.getvalue()) == cache
 
+    @ddt.data(
+        "gitmod~0",
+        "gitmod~5",
+        "gitmod~10",
+        "GI7EBA~",
+        "GI7EBA~0",
+        "GI7EBA~~1",
+        "GI7EBA~X",
+        "GI7EBA~10",
+        "Gx7EBA~1",
+        "GI7EBX~1",
+        "GI7EB~1",
+        "GI7EB~01",
+        "GI7EB~1X",
+        "GI7EB~100",
+        "GI7E~10",
+        "GI7E~010",
+        "GI7E~1000",
+        "GI7~100",
+        "GI7~0100",
+        "GI7~10000",
+        "GI~1000",
+        "GI~01000",
+        "GI~100000",
+        "G~10000",
+        "G~010000",
+        "G~1000000",
+        "~100000",
+        "~0100000",
+        "~10000000",
+        "GI7EBA~\u0661",
+        "GI7EB~1\uff10",
+        "GI7EB~10x",
+        "GI7EB~10.x",
+        " GI7EB~10",
+        "GI7EB~10\n",
+        "GI7EB~10\t",
+        "GI7EB~10x:$DATA",
+        ".gitmodules x",
+        ".gitmodules .x",
+        ".gitmodules,:$DATA",
+    )
+    def test_gitmodules_short_name_near_misses_round_trip(self, name):
+        """Only exact aliases are forbidden, and only for symbolic links."""
+        for mode in (0o100644, 0o120000):
+            cache = []
+            TreeModifier(cache).add(b"a" * 20, mode, name)
+            assert cache == [(b"a" * 20, mode, name)]
+            data = BytesIO()
+            tree_to_stream(cache, data.write)
+            raw = ("%o " % mode).encode() + name.encode() + b"\0" + b"a" * 20
+            assert data.getvalue() == raw
+            assert tree_entries_from_data(raw) == cache
+
     def test_traverse(self):
         root = self.rorepo.tree("0.1.6")
         num_recursive = 0