Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upFix leaking environment variables #662
Conversation
Isn't this change kind of intrusive?
|
@@ -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): |
ankostis
Aug 21, 2017
Contributor
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))
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))
@@ -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): |
ankostis
Aug 21, 2017
Contributor
Wouldn't this default break existing clients expecting variable-expansion?
I guess hooks would suffer.
Wouldn't this default break existing clients expecting variable-expansion?
I guess hooks would suffer.
Plazmaz
Aug 21, 2017
Author
Contributor
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.
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.
Codecov Report
@@ Coverage Diff @@
## master #662 +/- ##
=========================================
+ Coverage 92.57% 94.37% +1.8%
=========================================
Files 61 61
Lines 9968 9976 +8
=========================================
+ Hits 9228 9415 +187
+ Misses 740 561 -179
Continue to review full report at Codecov.
|
Regarding compatibility, this is OFF by default for now. It will work with existing projects just fine. |
Thanks a lot for your contribution! |
When cloning a repo, GitPython will leak environment variables in error messages. For instance, this code:
will output something like:
This behavior has unwanted security implications. To counter this, I've added an
unsafe
variable, which will allow for environment variables to be expanded, otherwise, this behavior is disabled. By default, this variable is set to True. However, when used with environment variables, a warning is displayed. Hopefully, this will eventually be set to False by default. When running the same code, but with unsafe set to False, here's the output: