fix: block unsafe long-option prefixes (GHSA-2f96-g7mh-g2hx)

git/cmd.py

@@ -960,18 +960,89 @@ def _canonicalize_option_name(cls, option: str) -> str:
  return dashify(option_tokens[0])
 
  @classmethod
- def check_unsafe_options(cls, options: List[str], unsafe_options: List[str]) -> None:
+ def _option_token(cls, option: str) -> str:
+ option_name = option.split("=", 1)[0]
+ option_tokens = option_name.split(None, 1)
+ if not option_tokens:
+ return ""
+ return option_tokens[0]
+
+ @classmethod
+ def _is_long_option(cls, option: str, bare_options_are_long: bool = True) -> bool:
+ """Return whether an option is a long option before canonicalization."""
+ option_token = cls._option_token(option)
+ if not option_token:
+ return False
+
+ if option_token.startswith("--"):
+ return True
+ if option_token.startswith("-"):
+ return False
+ return bare_options_are_long and len(option_token) > 1
+
+ @classmethod
+ def _matches_unsafe_short_option(
+ cls, option: str, canonical_unsafe_option: str, unsafe_option_is_long: bool
+ ) -> bool:
+ """Return whether a single-dash option matches an unsafe short option."""
+ if unsafe_option_is_long or len(canonical_unsafe_option) != 1:
+ return False
+
+ option_token = cls._option_token(option)
+ if not option_token:
+ return False
+
+ if not option_token.startswith("-") or option_token.startswith("--"):
+ return False
+
+ clusterable_flags = frozenset("46flnqsv")
+ for option_char in option_token[1:]:
+ if option_char == canonical_unsafe_option:
+ return True
+ if option_char not in clusterable_flags:
+ return False
+ return False
+
+ @classmethod
+ def check_unsafe_options(
+ cls, options: List[str], unsafe_options: List[str], bare_options_are_long: bool = True
+ ) -> None:
  """Check for unsafe options.
 
  Some options that are passed to ``git <command>`` can be used to execute
  arbitrary commands. These are blocked by default.
  """
  # Options can be of the form `foo`, `--foo`, `--foo bar`, or `--foo=bar`.
- canonical_unsafe_options = {cls._canonicalize_option_name(option): option for option in unsafe_options}
+ # We also treat long-option prefix forms as unsafe, matching Git's option
+ # parser behavior for long options.
+ canonical_unsafe_options = [
+ (cls._canonicalize_option_name(option), option, cls._is_long_option(option)) for option in unsafe_options
+ ]
  for option in options:
- unsafe_option = canonical_unsafe_options.get(cls._canonicalize_option_name(option))
- if unsafe_option is not None:
- raise UnsafeOptionError(f"{unsafe_option} is not allowed, use `allow_unsafe_options=True` to allow it.")
+ option_token = cls._option_token(option)
+ if not bare_options_are_long and not option_token.startswith("-"):
+ continue
+ canonical_option = cls._canonicalize_option_name(option)
+ if not canonical_option:
+ continue
+ option_is_long = cls._is_long_option(option, bare_options_are_long=bare_options_are_long)
+ for (
+ canonical_unsafe_option,
+ unsafe_option,
+ unsafe_option_is_long,
+ ) in canonical_unsafe_options:
+ if (
+ canonical_option == canonical_unsafe_option
+ or (
+ option_is_long
+ and unsafe_option_is_long
+ and canonical_unsafe_option.startswith(canonical_option)
+ )
+ or cls._matches_unsafe_short_option(option, canonical_unsafe_option, unsafe_option_is_long)
+ ):
+ raise UnsafeOptionError(
+ f"{unsafe_option} is not allowed, use `allow_unsafe_options=True` to allow it."
+ )
 
  AutoInterrupt: TypeAlias = _AutoInterrupt
 

git/repo/base.py

@@ -1410,7 +1410,9 @@ def _clone(
  if not allow_unsafe_options:
  Git.check_unsafe_options(options=list(kwargs.keys()), unsafe_options=cls.unsafe_git_clone_options)
  if not allow_unsafe_options and multi:
- Git.check_unsafe_options(options=multi, unsafe_options=cls.unsafe_git_clone_options)
+ Git.check_unsafe_options(
+ options=multi, unsafe_options=cls.unsafe_git_clone_options, bare_options_are_long=False
+ )
 
  proc = git.clone(
  multi,

test/test_clone.py

@@ -118,8 +118,13 @@ def test_clone_unsafe_options(self, rw_repo):
  unsafe_options = [
  f"--upload-pack='touch {tmp_file}'",
  f"-u 'touch {tmp_file}'",
+ f"-utouch {tmp_file}; false",
+ f"-futouch${{IFS}}{tmp_file}; false",
+ f"-qutouch${{IFS}}{tmp_file}; false",
  "--config=protocol.ext.allow=always",
  "-c protocol.ext.allow=always",
+ "-cprotocol.ext.allow=always",
+ "-vcprotocol.ext.allow=always",
  ]
  for unsafe_option in unsafe_options:
  with self.assertRaises(UnsafeOptionError):
@@ -129,6 +134,7 @@ def test_clone_unsafe_options(self, rw_repo):
  unsafe_options = [
  {"upload-pack": f"touch {tmp_file}"},
  {"upload_pack": f"touch {tmp_file}"},
+ {"upload_p": f"touch {tmp_file}"},
  {"u": f"touch {tmp_file}"},
  {"config": "protocol.ext.allow=always"},
  {"c": "protocol.ext.allow=always"},
@@ -191,7 +197,9 @@ def test_clone_safe_options(self, rw_repo):
  options = [
  "--depth=1",
  "--single-branch",
+ "--origin upload",
  "-q",
+ "-oupstream",
  ]
  for option in options:
  destination = tmp_dir / option
@@ -207,8 +215,13 @@ def test_clone_from_unsafe_options(self, rw_repo):
  unsafe_options = [
  f"--upload-pack='touch {tmp_file}'",
  f"-u 'touch {tmp_file}'",
+ f"-utouch {tmp_file}; false",
+ f"-futouch${{IFS}}{tmp_file}; false",
+ f"-qutouch${{IFS}}{tmp_file}; false",
  "--config=protocol.ext.allow=always",
  "-c protocol.ext.allow=always",
+ "-cprotocol.ext.allow=always",
+ "-vcprotocol.ext.allow=always",
  ]
  for unsafe_option in unsafe_options:
  with self.assertRaises(UnsafeOptionError):
@@ -218,6 +231,7 @@ def test_clone_from_unsafe_options(self, rw_repo):
  unsafe_options = [
  {"upload-pack": f"touch {tmp_file}"},
  {"upload_pack": f"touch {tmp_file}"},
+ {"upload_p": f"touch {tmp_file}"},
  {"u": f"touch {tmp_file}"},
  {"config": "protocol.ext.allow=always"},
  {"c": "protocol.ext.allow=always"},

test/test_git.py

@@ -158,10 +158,21 @@ def test_it_transforms_kwargs_into_git_command_arguments(self):
  def test_check_unsafe_options_normalizes_kwargs(self):
  cases = [
  (["upload_pack"], ["--upload-pack"]),
+ (["upload_p"], ["--upload-pack"]),
+ (["upload_pac"], ["--upload-pack"]),
  (["receive_pack"], ["--receive-pack"]),
+ (["receive_p"], ["--receive-pack"]),
  (["exec"], ["--exec"]),
+ (["exe"], ["--exec"]),
  (["u"], ["-u"]),
+ (["-utouch /tmp/pwn"], ["-u"]),
+ (["-futouch /tmp/pwn"], ["-u"]),
+ (["-qutouch${IFS}/tmp/pwn"], ["-u"]),
  (["c"], ["-c"]),
+ (["-cprotocol.ext.allow=always"], ["-c"]),
+ (["-vcprotocol.ext.allow=always"], ["-c"]),
+ (["conf"], ["--config"]),
+ (["confi"], ["--config"]),
  (["--upload-pack=/tmp/helper"], ["--upload-pack"]),
  (["--config core.filemode=false"], ["--config"]),
  ]
@@ -170,6 +181,35 @@ def test_check_unsafe_options_normalizes_kwargs(self):
  with self.assertRaises(UnsafeOptionError):
  Git.check_unsafe_options(options=options, unsafe_options=unsafe_options)
 
+ def test_check_unsafe_options_keeps_short_options_exact(self):
+ cases = [
+ (["u"], ["--upload-pack"]),
+ (["-u helper"], ["--upload-pack"]),
+ (["c"], ["--config"]),
+ (["-c protocol.ext.allow=always"], ["--config"]),
+ (["e"], ["--exec"]),
+ (["r"], ["--receive-pack"]),
+ ]
+
+ for options, unsafe_options in cases:
+ Git.check_unsafe_options(options=options, unsafe_options=unsafe_options)
+
+ def test_check_unsafe_options_ignores_multi_option_values(self):
+ Git.check_unsafe_options(
+ options=["--origin", "upload"],
+ unsafe_options=["--upload-pack", "--config"],
+ bare_options_are_long=False,
+ )
+
+ def test_check_unsafe_options_allows_attached_safe_short_option_values(self):
+ cases = [
+ ["-oupstream"],
+ ["-bcurrent"],
+ ]
+
+ for options in cases:
+ Git.check_unsafe_options(options=options, unsafe_options=["--upload-pack", "-u", "--config", "-c"])
+
  _shell_cases = (
  # value_in_call, value_from_class, expected_popen_arg
  (None, False, False),

test/test_remote.py

@@ -832,7 +832,11 @@ def test_fetch_unsafe_options(self, rw_repo):
  remote = rw_repo.remote("origin")
  tmp_dir = Path(tdir)
  tmp_file = tmp_dir / "pwn"
- unsafe_options = [{"upload-pack": f"touch {tmp_file}"}, {"upload_pack": f"touch {tmp_file}"}]
+ unsafe_options = [
+ {"upload-pack": f"touch {tmp_file}"},
+ {"upload_pack": f"touch {tmp_file}"},
+ {"upload_p": f"touch {tmp_file}"},
+ ]
  for unsafe_option in unsafe_options:
  with self.assertRaises(UnsafeOptionError):
  remote.fetch(**unsafe_option)
@@ -900,7 +904,11 @@ def test_pull_unsafe_options(self, rw_repo):
  remote = rw_repo.remote("origin")
  tmp_dir = Path(tdir)
  tmp_file = tmp_dir / "pwn"
- unsafe_options = [{"upload-pack": f"touch {tmp_file}"}, {"upload_pack": f"touch {tmp_file}"}]
+ unsafe_options = [
+ {"upload-pack": f"touch {tmp_file}"},
+ {"upload_pack": f"touch {tmp_file}"},
+ {"upload_p": f"touch {tmp_file}"},
+ ]
  for unsafe_option in unsafe_options:
  with self.assertRaises(UnsafeOptionError):
  remote.pull(**unsafe_option)
@@ -971,7 +979,9 @@ def test_push_unsafe_options(self, rw_repo):
  unsafe_options = [
  {"receive-pack": f"touch {tmp_file}"},
  {"receive_pack": f"touch {tmp_file}"},
+ {"receive_p": f"touch {tmp_file}"},
  {"exec": f"touch {tmp_file}"},
+ {"exe": f"touch {tmp_file}"},
  ]
  for unsafe_option in unsafe_options:
  assert not tmp_file.exists()