GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — Fix leaking environment variables by Plazmaz · Pull Request #662 · gitpython-developers/GitPython

Conversation

Plazmaz commented Aug 21, 2017

Copy link

Copy Markdown

Contributor

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

import git
repo = 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 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:

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}

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

ankostis reviewed

Aug 21, 2017

View reviewed changes

ankostis left a comment

Copy link

Copy Markdown

Contributor

There was a problem hiding this 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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

git/repo/base.py Outdated


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

ankostis Aug 21, 2017

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

git/repo/base.py Outdated

		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

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Plazmaz Aug 21, 2017

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Plazmaz added 2 commits

August 21, 2017 12:19


          Fix leaking environment variables

54709d9


          Fixed missing parameter and changed name

67291f0

codecov-io commented Aug 24, 2017

Copy link

Copy Markdown

Codecov Report

Merging #662 into master will increase coverage by 1.8%.
The diff coverage is 92.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%)`	⬆️
... and 13 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cf8dc25...67291f0. Read the comment docs.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Plazmaz commented Aug 24, 2017

Copy link

Copy Markdown

Contributor Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Byron added the acknowledged label

Sep 28, 2017

Byron merged commit 67291f0 into gitpython-developers:master

Sep 28, 2017

Byron commented Sep 28, 2017

Copy link

Copy Markdown

Member

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

rgkimball mentioned this pull request

Nov 22, 2019

Avoids env var warning when path contains $/%; fix #832 #961

Merged

EliahKagan mentioned this pull request

Mar 30, 2024

Issue and test deprecation warnings #1886

Merged

Reviewers

1 more reviewer

ankostis

ankostis left review comments

Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

acknowledged

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.