Skip to content

Fix leaking environment variables #662

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Sep 28, 2017
Merged

Fix leaking environment variables #662

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
54709d9
Fix leaking environment variables
Plazmaz Aug 21, 2017
67291f0
Fixed missing parameter and changed name
Plazmaz Aug 21, 2017
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
Next Next commit
Fix leaking environment variables
  • Loading branch information
@Plazmaz
Plazmaz committed Aug 21, 2017
commit 54709d9efd4624745ed0f67029ca30ee2ca87bc9
24 changes: 18 additions & 6 deletions git/repo/base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
import os
import re
import sys
import warnings

from git.cmd import (
Git,
Expand Down Expand Up @@ -50,8 +51,11 @@
__all__ = ('Repo',)


def _expand_path(p):
return osp.normpath(osp.abspath(osp.expandvars(osp.expanduser(p))))
def _expand_path(p, unsafe=True):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better call it what it is, expand_vars, and explain the reason for using this flag in the invoking site.

And I would split it, not to have to compare lines to discover the difference:

p = osp.expanduser(p)
if expand_vars:
    p = osp.expandvars(p)
return osp.normpath(osp.abspath(p))

if unsafe:
return osp.normpath(osp.abspath(osp.expandvars(osp.expanduser(p))))
else:
return osp.normpath(osp.abspath(osp.expanduser(p)))


class Repo(object):
Expand Down Expand Up @@ -90,7 +94,7 @@ class Repo(object):
# Subclasses may easily bring in their own custom types by placing a constructor or type here
GitCommandWrapperType = Git

def __init__(self, path=None, odbt=DefaultDBType, search_parent_directories=False):
def __init__(self, path=None, odbt=DefaultDBType, search_parent_directories=False, unsafe=True):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't this default break existing clients expecting variable-expansion?
I guess hooks would suffer.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default is True, meaning variables will be expanded by default. However, this behavior is bad, especially seeing as a standard practice is storing secrets and keys in environment variables. The reason for the flag is to give a bit of a grace period, and allow users to transition.

"""Create a new Repo instance

:param path:
Expand Down Expand Up @@ -121,7 +125,10 @@ def __init__(self, path=None, odbt=DefaultDBType, search_parent_directories=Fals
epath = os.getcwd()
if Git.is_cygwin():
epath = decygpath(epath)
epath = _expand_path(epath or path or os.getcwd())
if unsafe and ("%" in epath or "$" in epath):
warnings.warn("The use of environment variables in paths is deprecated"
+ "\nfor security reasons and may be removed in the future!!")
epath = _expand_path(epath or path or os.getcwd(), unsafe)
if not os.path.exists(epath):
raise NoSuchPathError(epath)

Expand All @@ -148,7 +155,7 @@ def __init__(self, path=None, odbt=DefaultDBType, search_parent_directories=Fals
sm_gitpath = find_worktree_git_dir(dotgit)

if sm_gitpath is not None:
self.git_dir = _expand_path(sm_gitpath)
self.git_dir = _expand_path(sm_gitpath, unsafe)
self._working_tree_dir = curpath
break

Expand Down Expand Up @@ -862,12 +869,17 @@ def init(cls, path=None, mkdir=True, odbt=DefaultDBType, **kwargs):
the directory containing the database objects, i.e. .git/objects.
It will be used to access all object data

:param unsafe:
if specified, environment variables will not be escaped. This
can lead to information disclosure, allowing attackers to
access the contents of environment variables

:parm kwargs:
keyword arguments serving as additional options to the git-init command

:return: ``git.Repo`` (the newly created repo)"""
if path:
path = _expand_path(path)
path = _expand_path(path, unsafe)
if mkdir and path and not osp.exists(path):
os.makedirs(path, 0o755)

Expand Down