Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Effect of refreshing with a relative path is not obvious #1831

Closed
@EliahKagan

Description

@EliahKagan

Whengit.refresh orgit.cmd.Git.refresh (whichgit.refresh calls) is passed a relative path as an explicitpath argument, it is taken relative to the current working directory of the process GitPython is running in at the time the refresh occurs. However, if instead one of thoserefresh functions is called with no argument and the value of theGIT_PYTHON_GIT_EXECUTABLE environment variable is a relative path, that value isnot resolved, but is instead looked up every time it is run. (The default ofgit is likewise not resolved.)

GitPython/git/cmd.py

Lines 365 to 370 inafa5754

# Discern which path to refresh with.
ifpathisnotNone:
new_git=os.path.expanduser(path)
new_git=os.path.abspath(new_git)
else:
new_git=os.environ.get(cls._git_exec_env_var,cls.git_exec_name)

This appears intentional, and in8dc8eb9 (#1815) I added tests that assert this behavior.But this should also be clarified for users, by documenting it explicitly in the docstring of at least one of therefresh functions. I am unsure how best to do this, because ideally the difference should beexplained, and I don't know if there is any good reason for the two cases to work differently, other than avoiding a breaking change within the same major version of the library.

If this is only for compatibility, then it might make sense to havegit.refresh andgit.cmd.Git.refresh accept a second optionalresolve argument to indicate if the first argument is supposed to be eagerly resolved, and issue aDeprecationWarning when theresolve argument is not passed (i.e., one-argumentgit.refresh calls would be deprecated). This would not substitute for adding an explanation to the docstring.

Security implications

A user who is confused about this behavior may write code likegit.refresh("git"), perhaps with the intention of undoing the effect of a previous refresh. If this is done when the current working directory is the working tree of an untrusted repository that contains a maliciousgit executable (or a malicious executable otherwise named the same as the command passed torefresh), then GitPython will use that command asgit, which would be a situation likeCVE-2023-40590 orCVE-2024-22190.

However, I am inclined to consider improving how this is documented to be a security enhancement, but not a fix for an existing security vulnerability in GitPython. I think this is not really a vulnerability in GitPython for three reasons. In decreasing order of significance:

  • Such code would typically be identified readily, because agit or other such executable inside a repository would not ordinarily occur in testing or normal usage, and an unexpectedGitCommandNotFound would be raised and observed. In particular, for the typical case of callinggit.refresh early on, such a mistake would be identified immediately. This differs from those vulnerabilities, where the current directory was searched but then the expected places were searched, and also differs in that this is about a small likelihood of software that uses GitPython introducing its own vulnerability, rather than GitPython itself having inherently vulnerable behavior.
  • The behavior of GitPython need not change to fix this, since it is mainly a matter of documentation.
  • Searching for uses ofgit.refresh suggests this is not often used at all, and didn't turn up any incorrect uses of relative paths (though this does not guarantee there are no such incorrect uses).

Integration considerations

With#1791, the case for documenting this inconsistency becomes stronger, because that will add another refresh-related function,refresh_bash, which never resolves the path. Unlikegit, GitPython often does not needbash or does not need it until a hook is needed to run on Windows, so it is more likely that a wrong call torefresh_bash would go undetected. Therefore, I very much agree with the decision there not to resolve the path, on security grounds:

GitPython/git/cmd.py

Lines 439 to 446 in8200ad1

# Discern which path to refresh with.
ifpathisnotNone:
new_bash=os.path.expanduser(path)
# new_bash = os.path.abspath(new_bash)
else:
new_bash=os.environ.get(cls._bash_exec_env_var)
ifnotnew_bash:
new_bash=cls._get_default_bash_path()

Because after#1791 this will be a behavioral difference between therefresh functions andrefresh_bash, this will be a further reason to document the subtlety.

This could possibly be included in the docstring modifications there, which would avoid a conflict, but I am somewhat inclined not to request unnecessary enhancements there.

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions


      [8]ページ先頭

      ©2009-2025 Movatter.jp