guard unsafe git command options by Byron · Pull Request #2163 · gitpython-developers/GitPython · GitHub
Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 36 additions & 1 deletion git/cmd.py
12 changes: 12 additions & 0 deletions git/objects/commit.py
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,12 @@ class Commit(base.Object, TraversableIterableObj, Diffable, Serializable):
# INVARIANTS
default_encoding = "UTF-8"

# Options to :manpage:`git-rev-list(1)` that can overwrite files.
unsafe_git_rev_options = [
"--output",
"-o",
]

type: Literal["commit"] = "commit"

__slots__ = (
Expand Down Expand Up @@ -302,6 +308,7 @@ def iter_items(
repo: "Repo",
rev: Union[str, "Commit", "SymbolicReference"],
paths: Union[PathLike, Sequence[PathLike]] = "",
allow_unsafe_options: bool = False,
**kwargs: Any,
) -> Iterator["Commit"]:
R"""Find all commits matching the given criteria.
Expand Down Expand Up @@ -330,6 +337,11 @@ def iter_items(
raise ValueError("--pretty cannot be used as parsing expects single sha's only")
# END handle pretty

if not allow_unsafe_options:
Git.check_unsafe_options(
options=Git._option_candidates([rev], kwargs), unsafe_options=cls.unsafe_git_rev_options
)

# Use -- in all cases, to prevent possibility of ambiguous arguments.
# See https://github.com/gitpython-developers/GitPython/issues/264.

Expand Down
15 changes: 12 additions & 3 deletions git/remote.py
Original file line number Diff line number Diff line change
Expand Up @@ -1071,7 +1071,10 @@ def fetch(
Git.check_unsafe_protocols(ref)

if not allow_unsafe_options:
Git.check_unsafe_options(options=list(kwargs.keys()), unsafe_options=self.unsafe_git_fetch_options)
Git.check_unsafe_options(
options=Git._option_candidates([], kwargs),
unsafe_options=self.unsafe_git_fetch_options,
)

proc = self.repo.git.fetch(
"--", self, *args, as_process=True, with_stdout=False, universal_newlines=True, v=verbose, **kwargs
Expand Down Expand Up @@ -1125,7 +1128,10 @@ def pull(
Git.check_unsafe_protocols(ref)

if not allow_unsafe_options:
Git.check_unsafe_options(options=list(kwargs.keys()), unsafe_options=self.unsafe_git_pull_options)
Git.check_unsafe_options(
options=Git._option_candidates([], kwargs),
unsafe_options=self.unsafe_git_pull_options,
)

proc = self.repo.git.pull(
"--", self, refspec, with_stdout=False, as_process=True, universal_newlines=True, v=True, **kwargs
Expand Down Expand Up @@ -1198,7 +1204,10 @@ def push(
Git.check_unsafe_protocols(ref)

if not allow_unsafe_options:
Git.check_unsafe_options(options=list(kwargs.keys()), unsafe_options=self.unsafe_git_push_options)
Git.check_unsafe_options(
options=Git._option_candidates([], kwargs),
unsafe_options=self.unsafe_git_push_options,
)

proc = self.repo.git.push(
"--",
Expand Down
80 changes: 73 additions & 7 deletions git/repo/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,20 @@ class Repo:
https://git-scm.com/docs/git-clone#Documentation/git-clone.txt---configltkeygtltvaluegt
"""

unsafe_git_archive_options = [
# Allows arbitrary command execution through the remote git-upload-archive command.
"--exec",
# Writes output to a caller-controlled filesystem path.
"--output",
"-o",
]

unsafe_git_revision_options = [
# This option allows output to be written to arbitrary files before revision parsing.
"--output",
"-o",
]

# Invariants
config_level: ConfigLevels_Tup = ("system", "user", "global", "repository")
"""Represents the configuration level of a configuration file."""
Expand Down Expand Up @@ -775,6 +789,7 @@ def iter_commits(
self,
rev: Union[str, Commit, "SymbolicReference", None] = None,
paths: Union[PathLike, Sequence[PathLike]] = "",
allow_unsafe_options: bool = False,
**kwargs: Any,
) -> Iterator[Commit]:
Comment thread
Byron marked this conversation as resolved.
"""An iterator of :class:`~git.objects.commit.Commit` objects representing the
Expand All @@ -792,6 +807,9 @@ def iter_commits(
Arguments to be passed to :manpage:`git-rev-list(1)`.
Common ones are ``max_count`` and ``skip``.

:param allow_unsafe_options:
Allow unsafe options in the revision argument, like ``--output``.

:note:
To receive only commits between two named revisions, use the
``"revA...revB"`` revision specifier.
Expand All @@ -802,7 +820,18 @@ def iter_commits(
if rev is None:
rev = self.head.commit

return Commit.iter_items(self, rev, paths, **kwargs)
if not allow_unsafe_options:
Git.check_unsafe_options(
options=Git._option_candidates([rev], kwargs), unsafe_options=self.unsafe_git_revision_options
)
Comment thread
Byron marked this conversation as resolved.

return Commit.iter_items(
self,
rev,
paths,
allow_unsafe_options=allow_unsafe_options,
**kwargs,
)

def merge_base(self, *rev: TBD, **kwargs: Any) -> List[Commit]:
R"""Find the closest common ancestor for the given revision
Expand Down Expand Up @@ -1079,7 +1108,9 @@ def active_branch(self) -> Head:
)
return active_branch

def blame_incremental(self, rev: str | HEAD | None, file: str, **kwargs: Any) -> Iterator["BlameEntry"]:
def blame_incremental(
self, rev: str | HEAD | None, file: str, allow_unsafe_options: bool = False, **kwargs: Any
) -> Iterator["BlameEntry"]:
Comment thread
Byron marked this conversation as resolved.
"""Iterator for blame information for the given file at the given revision.

Unlike :meth:`blame`, this does not return the actual file's contents, only a
Expand All @@ -1090,6 +1121,9 @@ def blame_incremental(self, rev: str | HEAD | None, file: str, **kwargs: Any) ->
uncommitted changes. Otherwise, anything successfully parsed by
:manpage:`git-rev-parse(1)` is a valid option.

:param allow_unsafe_options:
Allow unsafe options in revision argument, like ``--output``.

:return:
Lazy iterator of :class:`BlameEntry` tuples, where the commit indicates the
commit to blame for the line, and range indicates a span of line numbers in
Expand All @@ -1098,6 +1132,10 @@ def blame_incremental(self, rev: str | HEAD | None, file: str, **kwargs: Any) ->
If you combine all line number ranges outputted by this command, you should get
a continuous range spanning all line numbers in the file.
"""
if not allow_unsafe_options:
Git.check_unsafe_options(
options=Git._option_candidates([rev], kwargs), unsafe_options=self.unsafe_git_revision_options
)

data: bytes = self.git.blame(rev, "--", file, p=True, incremental=True, stdout_as_string=False, **kwargs)
commits: Dict[bytes, Commit] = {}
Expand Down Expand Up @@ -1176,7 +1214,8 @@ def blame(
rev: Union[str, HEAD, None],
file: str,
incremental: bool = False,
rev_opts: Optional[List[str]] = None,
rev_opts: Optional[Sequence[str]] = None,
allow_unsafe_options: bool = False,
**kwargs: Any,
) -> List[List[Commit | List[str | bytes] | None]] | Iterator[BlameEntry] | None:
Comment thread
Byron marked this conversation as resolved.
"""The blame information for the given file at the given revision.
Expand All @@ -1186,6 +1225,9 @@ def blame(
uncommitted changes. Otherwise, anything successfully parsed by
:manpage:`git-rev-parse(1)` is a valid option.

:param allow_unsafe_options:
Allow unsafe options in revision argument, like ``--output``.

:return:
list: [git.Commit, list: [<line>]]

Expand All @@ -1195,9 +1237,14 @@ def blame(
appearance.
"""
if incremental:
return self.blame_incremental(rev, file, **kwargs)
rev_opts = rev_opts or []
data: bytes = self.git.blame(rev, *rev_opts, "--", file, p=True, stdout_as_string=False, **kwargs)
return self.blame_incremental(rev, file, allow_unsafe_options=allow_unsafe_options, **kwargs)
rev_opts_list = list(rev_opts or [])
if not allow_unsafe_options:
Git.check_unsafe_options(
options=Git._option_candidates([rev, rev_opts_list], kwargs),
unsafe_options=self.unsafe_git_revision_options,
)
Comment thread
Byron marked this conversation as resolved.
data: bytes = self.git.blame(rev, *rev_opts_list, "--", file, p=True, stdout_as_string=False, **kwargs)
commits: Dict[str, Commit] = {}
blames: List[List[Commit | List[str | bytes] | None]] = []

Expand Down Expand Up @@ -1408,7 +1455,10 @@ def _clone(
if not allow_unsafe_protocols:
Git.check_unsafe_protocols(url)
if not allow_unsafe_options:
Git.check_unsafe_options(options=list(kwargs.keys()), unsafe_options=cls.unsafe_git_clone_options)
Git.check_unsafe_options(
options=Git._option_candidates([], kwargs),
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)

Expand Down Expand Up @@ -1583,6 +1633,8 @@ def archive(
ostream: Union[TextIO, BinaryIO],
treeish: Optional[str] = None,
prefix: Optional[str] = None,
allow_unsafe_options: bool = False,
allow_unsafe_protocols: bool = False,
**kwargs: Any,
) -> Repo:
"""Archive the tree at the given revision.
Expand All @@ -1605,6 +1657,12 @@ def archive(
repository-relative path to a directory or file to place into the archive,
or a list or tuple of multiple paths.

:param allow_unsafe_options:
Allow unsafe options, like ``--exec`` or ``--output``.

:param allow_unsafe_protocols:
Allow unsafe protocols to be used in ``remote``, like ``ext``.

:raise git.exc.GitCommandError:
If something went wrong.

Expand All @@ -1615,6 +1673,14 @@ def archive(
treeish = self.head.commit
if prefix and "prefix" not in kwargs:
kwargs["prefix"] = prefix
remote = kwargs.get("remote")
if not allow_unsafe_protocols and remote is not None:
Git.check_unsafe_protocols(str(remote))
if not allow_unsafe_options:
Git.check_unsafe_options(
options=Git._option_candidates([], kwargs),
unsafe_options=self.unsafe_git_archive_options,
)
kwargs["output_stream"] = ostream
path = kwargs.pop("path", [])
path = cast(Union[PathLike, List[PathLike], Tuple[PathLike, ...]], path)
Expand Down
4 changes: 4 additions & 0 deletions test/test_clone.py
Original file line number Diff line number Diff line change
Expand Up @@ -117,11 +117,13 @@ def test_clone_unsafe_options(self, rw_repo):
tmp_file = tmp_dir / "pwn"
unsafe_options = [
f"--upload-pack='touch {tmp_file}'",
f"--upl='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",
"--conf=protocol.ext.allow=always",
"-c protocol.ext.allow=always",
"-cprotocol.ext.allow=always",
"-vcprotocol.ext.allow=always",
Expand All @@ -134,8 +136,10 @@ def test_clone_unsafe_options(self, rw_repo):
unsafe_options = [
{"upload-pack": f"touch {tmp_file}"},
{"upload_pack": f"touch {tmp_file}"},
{"upl": f"touch {tmp_file}"},
{"u": f"touch {tmp_file}"},
{"config": "protocol.ext.allow=always"},
{"conf": "protocol.ext.allow=always"},
{"c": "protocol.ext.allow=always"},
]
for unsafe_option in unsafe_options:
Expand Down
13 changes: 13 additions & 0 deletions test/test_commit.py
Loading
Loading