◐ Shell
reader mode source ↗
Skip to content

Fix leaking environment variables#662

Merged
Byron merged 2 commits into
gitpython-developers:masterfrom
Plazmaz:master
Sep 28, 2017
Merged

Fix leaking environment variables#662
Byron merged 2 commits into
gitpython-developers:masterfrom
Plazmaz:master

Conversation

@Plazmaz

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

ankostis
ankostis reviewed Aug 21, 2017
View reviewed changes

@ankostis ankostis left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hide comment

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?

@codecov-io

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%.

Impacted file tree graph

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

@Plazmaz

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.

@Byron Byron merged commit 67291f0 into gitpython-developers:master Sep 28, 2017
@Byron

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.

@rgkimball rgkimball mentioned this pull request Nov 22, 2019
Merged
@EliahKagan EliahKagan mentioned this pull request Mar 30, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@ankostis ankostis ankostis left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

acknowledged

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@Plazmaz @codecov-io @Byron @ankostis