Merge pull request #2142 from gitpython-developers/fix-validate-config-key-newlines

Validate config key names before writing

Author: Byron

git/config.py

@@ -72,6 +72,9 @@
 See: https://git-scm.com/docs/git-config#_conditional_includes
 """
 
+UNSAFE_CONFIG_CHARS_RE = re.compile(r"[\r\n\x00]")
+"""Characters that cannot be safely written in config names or values."""
+
 
 class MetaParserBuilder(abc.ABCMeta):  # noqa: B024
     """Utility class wrapping base-class methods into decorators that assure read-only
@@ -778,6 +781,7 @@ def _assure_writable(self, method_name: str) -> None:
 
     def add_section(self, section: "cp._SectionName") -> None:
         """Assures added options will stay in order."""
+        self._assure_config_name_safe(section, "section")
         return super().add_section(section)
 
     @property
@@ -884,10 +888,14 @@ def _value_to_string(self, value: Union[str, bytes, int, float, bool]) -> str:
 
     def _value_to_string_safe(self, value: Union[str, bytes, int, float, bool]) -> str:
         value_str = self._value_to_string(value)
-        if re.search(r"[\r\n\x00]", value_str):
+        if UNSAFE_CONFIG_CHARS_RE.search(value_str):
             raise ValueError("Git config values must not contain CR, LF, or NUL")
         return value_str
 
+    def _assure_config_name_safe(self, name: "cp._SectionName", label: str) -> None:
+        if isinstance(name, str) and UNSAFE_CONFIG_CHARS_RE.search(name):
+            raise ValueError("Git config %s names must not contain CR, LF, or NUL" % label)
+
     @needs_values
     @set_dirty_and_flush_changes
     def set(
@@ -896,6 +904,8 @@ def set(
         option: str,
         value: Union[str, bytes, int, float, bool, None] = None,
     ) -> None:
+        self._assure_config_name_safe(section, "section")
+        self._assure_config_name_safe(option, "option")
         if value is not None:
             value = self._value_to_string_safe(value)
         return super().set(section, option, value)
@@ -920,6 +930,8 @@ def set_value(self, section: str, option: str, value: Union[str, bytes, int, flo
         :return:
             This instance
         """
+        self._assure_config_name_safe(section, "section")
+        self._assure_config_name_safe(option, "option")
         value_str = self._value_to_string_safe(value)
         if not self.has_section(section):
             self.add_section(section)
@@ -948,6 +960,8 @@ def add_value(self, section: str, option: str, value: Union[str, bytes, int, flo
         :return:
             This instance
         """
+        self._assure_config_name_safe(section, "section")
+        self._assure_config_name_safe(option, "option")
         value_str = self._value_to_string_safe(value)
         if not self.has_section(section):
             self.add_section(section)
@@ -968,6 +982,7 @@ def rename_section(self, section: str, new_name: str) -> "GitConfigParser":
         """
         if not self.has_section(section):
             raise ValueError("Source section '%s' doesn't exist" % section)
+        self._assure_config_name_safe(new_name, "section")
         if self.has_section(new_name):
             raise ValueError("Destination section '%s' already exists" % new_name)
 

test/test_config.py

@@ -163,6 +163,37 @@ def test_set_value_rejects_config_injection(self, rw_dir):
             self.assertFalse(git_config.has_section("user"))
             self.assertFalse(git_config.has_section("core"))
 
+    @with_rw_directory
+    def test_set_value_rejects_unsafe_section_and_option_names(self, rw_dir):
+        config_path = osp.join(rw_dir, "config")
+        bad_keys = ("user]\n[core", "user]\r[core", "user]\x00[core")
+
+        with GitConfigParser(config_path, read_only=False) as git_config:
+            git_config.add_section("user")
+            for bad_key in bad_keys:
+                with pytest.raises(ValueError, match="CR, LF, or NUL"):
+                    git_config.add_section(bad_key)
+                with pytest.raises(ValueError, match="CR, LF, or NUL"):
+                    git_config.set(bad_key, "hooksPath", "/tmp/hooks")
+                with pytest.raises(ValueError, match="CR, LF, or NUL"):
+                    git_config.set("user", bad_key, "/tmp/hooks")
+                with pytest.raises(ValueError, match="CR, LF, or NUL"):
+                    git_config.set_value(bad_key, "hooksPath", "/tmp/hooks")
+                with pytest.raises(ValueError, match="CR, LF, or NUL"):
+                    git_config.set_value("user", bad_key, "/tmp/hooks")
+                with pytest.raises(ValueError, match="CR, LF, or NUL"):
+                    git_config.add_value(bad_key, "hooksPath", "/tmp/hooks")
+                with pytest.raises(ValueError, match="CR, LF, or NUL"):
+                    git_config.add_value("user", bad_key, "/tmp/hooks")
+                with pytest.raises(ValueError, match="CR, LF, or NUL"):
+                    git_config.rename_section("user", bad_key)
+
+            git_config.set_value("user", "name", "safe")
+
+        with GitConfigParser(config_path, read_only=True) as git_config:
+            self.assertEqual(git_config.get_value("user", "name"), "safe")
+            self.assertFalse(git_config.has_section("core"))
+
     @with_rw_directory
     def test_set_and_add_value_reject_unsafe_value_characters(self, rw_dir):
         config_path = osp.join(rw_dir, "config")