Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix a bug with enviroment variable injection to the git repo url #2710

Closed
wants to merge 4 commits into from
Closed

Fix a bug with enviroment variable injection to the git repo url #2710

wants to merge 4 commits into from

Conversation

ehengao
Copy link

@ehengao ehengao commented Aug 7, 2018

Thank you for contributing to Pipenv!

The issue

Fix a enviroment injection issue when dealing with git repo url dependencies. The problem issued when pipenv trying to execute command similar to "git clone -q git+https://${USER}:${PASSWORD}/github.com" using python subprocess.Popen, the enviroment variable inside this command is not propely resolved.Which will cause a permission deny error or a repo not found error.

The fix

using os.path.expandvars(cmd-parts) before passing to subprocess.

The checklist
  • Associated issue

#2635

  • A news fragment:

news/2365.bugfix

@ehengao
Copy link
Author

ehengao commented Aug 9, 2018

By any chance we can merge this?

@uranusjr
Copy link
Member

uranusjr commented Aug 9, 2018

I want to sit on this for a while. Generally a patch to vendor or patched is a last resort we want to avoid, since it may result in unintended subsequences.

@techalchemy
Copy link
Member

Interesting issue -- I'm pretty sure we're going to un-vendor pip soon though so this might not be the best approach to solving it

@pcraciunoiu
Copy link

Is there a fix for installing private git repos with pipenv? I found this issue and I'm trying to avoid rolling a private pypi just for a repo or two...

@uranusjr
Copy link
Member

One way is to use SSH, such as GitHub deploy keys.

@techalchemy
Copy link
Member

Really anything that involves putting a password in a URI directly is a bad practice, and we've had similar versions of this proposed in the past and closed for various security related reasons that I can't completely recall. If someone can dig through the past issues on the topic it would be helpful

@techalchemy techalchemy added Category: Security Relates to security Category: VCS Relates to version control system dependencies. Priority: Low This item is low priority and may not be looked at in the next few release cycles. Status: Deferred / On Hold 🛑 This item is on hold until further notice. Status: Needs More Information This issue does not provide enough information to take further action. Type: Discussion This issue is open for discussion. labels May 27, 2019
@DiddiLeija DiddiLeija deleted the branch pypa:master October 26, 2021 17:43
@DiddiLeija DiddiLeija closed this Oct 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category: Security Relates to security Category: VCS Relates to version control system dependencies. Priority: Low This item is low priority and may not be looked at in the next few release cycles. Status: Deferred / On Hold 🛑 This item is on hold until further notice. Status: Needs More Information This issue does not provide enough information to take further action. Type: Discussion This issue is open for discussion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants