gitpython-developers/GitPythonPublic

NotificationsYou must be signed in to change notification settings
Fork937
Star4.9k

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.

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

Already on GitHub?Sign in to your account

Jump to bottom

Merged

Byron merged 2 commits intogitpython-developers:masterfromPlazmaz:master

Sep 28, 2017

Merged

Fix leaking environment variables#662

Byron merged 2 commits intogitpython-developers:masterfromPlazmaz:master

Sep 28, 2017

Conversation

Copy link

Contributor

Plazmaz commentedAug 21, 2017

When cloning a repo, GitPython will leak environment variables in error messages. For instance, this code:

import gitrepo = git.Repo('https://www.github.com/Plazmaz/${PATH}')repo.clone('testrepo/$PATH')

will output something like:

Traceback (most recent call last):  File "test.py", line 2, in <module>    repo = repo.Repo('https://www.github.com/Plazmaz/${PATH}', unsafe=True)  File "GitPython/git/repo/base.py", line 133, in __init__    raise NoSuchPathError(epath)git.exc.NoSuchPathError: <THE CONTENTS OF $PATH>

This behavior has unwanted security implications. To counter this, I've added anunsafe 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:

Traceback (most recent call last):  File "test.py", line 2, in <module>    repo = repo.Repo('https://www.github.com/Plazmaz/${PATH}', unsafe=False)  File "GitPython/git/repo/base.py", line 133, in __init__    raise NoSuchPathError(epath)git.exc.NoSuchPathError: Documents/https:/www.github.com/Plazmaz/${PATH}

ankostis reviewed

Aug 21, 2017

View reviewed changes

Copy link

Contributor

ankostis left a comment

Choose a reason for hiding this comment

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

Isn't this change kind of intrusive?

Hooks expect certain variables to function, is this working with new default NOT to expand env-variables at all?
Or break BW-compatibility with existing clients?

git/repo/base.py Outdated

		@@ -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

ankostisAug 21, 2017

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)ifexpand_vars:p=osp.expandvars(p)returnosp.normpath(osp.abspath(p))

git/repo/base.py Outdated

		@@ -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

ankostisAug 21, 2017

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 guesshooks would suffer.

Copy link

ContributorAuthor

PlazmazAug 21, 2017

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 isbad, 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.

Plazmaz added2 commits

August 21, 2017 12:19

Fix leaking environment variables

54709d9

Fixed missing parameter and changed name

67291f0

Copy link

codecov-io commentedAug 24, 2017

Codecov Report

Merging#662 intomaster willincrease coverage by1.8%.
The diff coverage is92.85%.

@@            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

Impacted Files	Coverage Δ
git/repo/base.py	`95.66% <92.85%> (+0.53%)`	⬆️
git/test/test_remote.py	`97.82% <0%> (ø)`	⬆️
git/refs/remote.py	`91.11% <0%> (ø)`	⬆️
git/compat.py	`63.19% <0%> (+0.22%)`	⬆️
git/refs/symbolic.py	`96.5% <0%> (+0.31%)`	⬆️
git/config.py	`92.76% <0%> (+0.32%)`	⬆️
git/test/test_submodule.py	`99.22% <0%> (+0.38%)`	⬆️
git/test/test_docs.py	`100% <0%> (+0.39%)`	⬆️
git/cmd.py	`85.54% <0%> (+0.47%)`	⬆️
... and13 more

Continue to review full report at Codecov.

Legend -Click here to learn more
Δ = absolute <relative> (impact),ø = not affected,? = missing data
Powered byCodecov. Last updatecf8dc25...67291f0. Read thecomment docs.

Copy link

ContributorAuthor

Plazmaz commentedAug 24, 2017

Regarding compatibility, this is OFF by default for now. It will work with existing projects just fine.

Byron added the acknowledged label

Sep 28, 2017

Byron merged commit67291f0 intogitpython-developers:master

Sep 28, 2017

Copy link

Member

Byron commentedSep 28, 2017

Thanks a lot for your contribution!
Without a new major release, this flag can't be OFF by default, and in maintenance mode, GitPython probably is not going to do that.