Skip to content

Conversation

@matteius
Copy link
Member

Fixes #2635

@matteius matteius requested a review from oz123 August 26, 2022 15:23
@matteius
Copy link
Member Author

I'll add a news fragment before merging once this gets reviewed.

Copy link
Contributor

@oz123 oz123 left a comment

Choose a reason for hiding this comment

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

The string method of package.link should redact the authentication according to the code here:
https://github.com/pypa/pip/blob/65680b4bb1a2412e97bc90e49df6999b5f362e40/src/pip/_internal/models/link.py#L93

I suspect the f-string (which calls the repr ) should be also using the string method explicitly.

In other words: package.link should not show auth information.

@matteius
Copy link
Member Author

@oz123 That is the problem -- people are using environment variables for the auth and they get redacted.

@oz123
Copy link
Contributor

oz123 commented Aug 27, 2022

I still don't understand what issue is being fixed here.

We should not leak authentication information to Pipefile or Pipefile.lock.
If we merge this, we can expect people to open a CVE for this.

@oz123
Copy link
Contributor

oz123 commented Aug 27, 2022

@oz123 That is the problem -- people are using environment variables for the auth and they get redacted.

Redacted from where? From the Pipefile? That's great, pipenv does its job.
If they get redacted from the files pipenv is writing while trying to install a package, then the fix should be some where else.

@matteius
Copy link
Member Author

@oz123 For the case where the user is using environment variables, they don't get expanded, but they do get redacted, which means this doesn't work:

# requirements.txt
git+https://${AUTH_USER}:${AUTH_PASSWORD}@github.com/user/myproject.git@1.2.3#egg=myproject

Then run

> pip install -r requirements.txt

Becasue str(package.link) will redact it to be git+https://${AUTH_USER}:****@github.com/user/myproject.git@1.2.3#egg=myproject

If you have just a user variable, like this: git+https://${AUTH_USER}@github.com/user/myproject.git@1.2.3#egg=myproject
Then that variable will get redacted as well:
git+https://****@github.com/user/myproject.git@1.2.3#egg=myproject

Which creates a not lockable or installable Pipfile. The ask of #2635 is to fix this -- but you are probably correct as well, we need a way to detect if they are environment variables, to not redact them.

@oz123
Copy link
Contributor

oz123 commented Aug 27, 2022

OK, I see where is the problem.

These are not environment variables, rather references to them. So if someone was smart and didn't specify the password, pip's own code still think it's a password.

Using _url can be dangerous, we could use split_auth_from_netloc from pip to check for a password or a variable and do something about it.

Something could be:

  1. Warn the user.
  2. Redact if it is a password
  3. Pass it on to Pipfile if the password is the known form for a variable reference, e.g. ${PASSWORD} or ${password} or in regex: ${\w+}.

@matteius matteius changed the title Allow the unredacted package link URL to prevent environment variable from being redacted. **** Allow environment variable references to be passed through to the Pipfile unredacted Aug 31, 2022
@matteius matteius requested a review from oz123 August 31, 2022 01:13
@matteius
Copy link
Member Author

Changes made to address the concerns and tests added for the various cases.

@matteius
Copy link
Member Author

@oz123 this PR is insightful, because mac os failed on every test pips_to_dep tests regarding requests, possibly because of my new Pipfile tests? I noticed a weird behavior on my mac this past week that took me a while to track down -- even if there is no virtualenv, if there is a Pipfile in a higher level directory, then pipenv prefers to use that over the Pipfile in your current working directory, at least on a mac.

@oz123 oz123 merged commit f0d29d4 into main Aug 31, 2022
@oz123 oz123 deleted the issue-2635 branch August 31, 2022 23:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Import Private Github Repo via Environment Variables

3 participants