Merge pull request #2215 from gitpython-developers/various-fixes

Harden diff path and actor identity parsing

Author: Byron

doc/source/changes.rst

@@ -2,6 +2,19 @@
 Changelog
 =========
 
+3.1.60
+======
+
+Security fixes for
+
+* https://github.com/gitpython-developers/GitPython/security/advisories/GHSA-g5vv-9gxw-82hx
+
+If you can, also try and provide feedback on the upcoming v4 branch
+https://github.com/gitpython-developers/GitPython/pull/2177 - patches welcome.
+
+See the following for all changes.
+https://github.com/gitpython-developers/GitPython/releases/tag/3.1.60
+
 3.1.59
 ======
 

git/diff.py

@@ -95,24 +95,43 @@ class DiffConstants(enum.Enum):
 :const:`git.INDEX` and :const:`Diffable.INDEX`, as well as :const:`Diffable.Index`.
 """
 
-_octal_byte_re = re.compile(rb"\\([0-9]{3})")
 
-
-def _octal_repl(matchobj: Match) -> bytes:
-    value = matchobj.group(1)
-    value = int(value, 8)
-    value = bytes(bytearray((value,)))
-    return value
+def _unquote_path(path: bytes) -> bytes:
+    result = bytearray()
+    escapes = {
+        ord("a"): 7,
+        ord("b"): 8,
+        ord("f"): 12,
+        ord("n"): 10,
+        ord("r"): 13,
+        ord("t"): 9,
+        ord("v"): 11,
+    }
+    i = 0
+    while i < len(path):
+        if path[i] != ord("\\") or i + 1 == len(path):
+            result.append(path[i])
+            i += 1
+            continue
+        if path[i + 1] in b"0123" and i + 3 < len(path) and all(c in b"01234567" for c in path[i + 2 : i + 4]):
+            result.append(int(path[i + 1 : i + 4], 8))
+            i += 4
+            continue
+        escaped = path[i + 1]
+        if escaped in escapes or escaped in b'\\"':
+            result.append(escapes.get(escaped, escaped))
+        else:
+            result.extend(path[i : i + 2])
+        i += 2
+    return bytes(result)
 
 
 def decode_path(path: bytes, has_ab_prefix: bool = True) -> Optional[bytes]:
     if path == b"/dev/null":
         return None
 
     if path.startswith(b'"') and path.endswith(b'"'):
-        path = path[1:-1].replace(b"\\n", b"\n").replace(b"\\t", b"\t").replace(b'\\"', b'"').replace(b"\\\\", b"\\")
-
-    path = _octal_byte_re.sub(_octal_repl, path)
+        path = _unquote_path(path[1:-1])
 
     if has_ab_prefix:
         assert path.startswith(b"a/") or path.startswith(b"b/")

git/util.py

@@ -858,10 +858,6 @@ class Actor:
     committers and authors or anything with a name and an email as mentioned in the git
     log entries."""
 
-    # PRECOMPILED REGEX
-    name_only_regex = re.compile(r"<(.*)>")
-    name_email_regex = re.compile(r"(.*) <(.*?)>")
-
     # ENVIRONMENT VARIABLES
     # These are read when creating new commits.
     env_author_name = "GIT_AUTHOR_NAME"
@@ -906,18 +902,14 @@ def _from_string(cls, string: str) -> "Actor":
         :return:
             :class:`Actor`
         """
-        m = cls.name_email_regex.search(string)
-        if m:
-            name, email = m.groups()
-            return Actor(name, email)
-        else:
-            m = cls.name_only_regex.search(string)
-            if m:
-                return Actor(m.group(1), None)
-            # Assume the best and use the whole string as name.
-            return Actor(string, None)
-            # END special case name
-        # END handle name/email matching
+        line = string.partition("\n")[0]
+        left_bracket = line.find("<")
+        right_bracket = line.find(">", left_bracket + 1)
+        if left_bracket >= 0 and right_bracket >= 0:
+            return Actor(line[:left_bracket].rstrip(), line[left_bracket + 1 : right_bracket])
+
+        # Assume the best and use the whole string as name.
+        return Actor(string, None)
 
     @classmethod
     def _main_actor(

test/test_actor.py

@@ -27,6 +27,26 @@ def test_from_string_should_handle_just_name(self):
         self.assertEqual("Michael Trier", a.name)
         self.assertEqual(None, a.email)
 
+    def test_from_string_handles_unterminated_email_without_regex_backtracking(self):
+        value = "A" * 20_000 + " <unterminated"
+        actor = Actor._from_string(value)
+        self.assertNotIn("name_email_regex", vars(Actor))
+        self.assertEqual(actor, Actor(value, None))
+
+    def test_from_string_does_not_parse_across_lines(self):
+        self.assertEqual(Actor._from_string("x <a>\n y <b>"), Actor("x", "a"))
+
+    def test_from_string_uses_git_delimiters(self):
+        for value, expected in (
+            ("Name <e<mail>", Actor("Name", "e<mail")),
+            ("Name <email>>", Actor("Name", "email")),
+            ("Name<email>", Actor("Name", "email")),
+            (" <>", Actor("", "")),
+            ("Name <email", Actor("Name <email", None)),
+            ("Name email>", Actor("Name email>", None)),
+        ):
+            self.assertEqual(Actor._from_string(value), expected)
+
     def test_should_display_representation(self):
         a = Actor._from_string("Michael Trier <mtrier@example.com>")
         self.assertEqual('<git.Actor "Michael Trier <mtrier@example.com>">', repr(a))

test/test_diff.py

@@ -14,6 +14,7 @@
 
 from git import NULL_TREE, Diff, DiffIndex, Diffable, GitCommandError, Repo, Submodule
 from git.cmd import Git
+from git.diff import decode_path
 from git.exc import UnsafeOptionError
 
 from test.lib import StringProcessAdapter, TestBase, fixture, with_rw_directory
@@ -324,6 +325,11 @@ def test_diff_patch_format(self):
             Diff._index_from_patch_format(self.rorepo, diff_proc)
         # END for each fixture
 
+    def test_decode_path_distinguishes_escaped_backslashes_from_octal_bytes(self):
+        self.assertEqual(decode_path(b'"foo\\\\899bar"', False), b"foo\\899bar")
+        self.assertEqual(decode_path(b'"foo\\\\123bar"', False), b"foo\\123bar")
+        self.assertEqual(decode_path(b'"foo\\123bar"', False), b"fooSbar")
+
     def test_diff_with_spaces(self):
         data = StringProcessAdapter(fixture("diff_file_with_spaces"))
         diff_index = Diff._index_from_patch_format(self.rorepo, data)