Effect of refreshing with a relative path is not obvious · Issue #1831 · gitpython-developers/GitPython (original) (raw)

When git.refresh or git.cmd.Git.refresh (which git.refresh calls) is passed a relative path as an explicit path 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 those refresh functions is called with no argument and the value of the GIT_PYTHON_GIT_EXECUTABLE environment variable is a relative path, that value is not resolved, but is instead looked up every time it is run. (The default of git is likewise not resolved.)

# Discern which path to refresh with.
if path is not None:
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 in 8dc8eb9 (#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 the refresh functions. I am unsure how best to do this, because ideally the difference should be explained, 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 have git.refresh and git.cmd.Git.refresh accept a second optional resolve argument to indicate if the first argument is supposed to be eagerly resolved, and issue a DeprecationWarning when the resolve argument is not passed (i.e., one-argument git.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 like git.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 malicious git executable (or a malicious executable otherwise named the same as the command passed to refresh), then GitPython will use that command as git, which would be a situation like CVE-2023-40590 or CVE-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:

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. Unlike git, GitPython often does not need bash or does not need it until a hook is needed to run on Windows, so it is more likely that a wrong call to refresh_bash would go undetected. Therefore, I very much agree with the decision there not to resolve the path, on security grounds:

# Discern which path to refresh with.
if path is not None:
new_bash = os.path.expanduser(path)
# new_bash = os.path.abspath(new_bash)
else:
new_bash = os.environ.get(cls._bash_exec_env_var)
if not new_bash:
new_bash = cls._get_default_bash_path()

Because after #1791 this will be a behavioral difference between the refresh functions and refresh_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.