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.

Sign up for GitHub

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

Jump to bottom

Mask passwds in downloads.py output #6834

Closed
wants to merge 2 commits into from
Closed

Mask passwds in downloads.py output #6834

wants to merge 2 commits into from

Conversation

liam-stevenson
Copy link

I found that the URL variable in download.py was just just being printed to stout so I took the function redact_password_from_url used else where (such as collector.py), imported it and used it to mask the passwords:
python pip install https://test:[email protected]/test Collecting https://test:****@github.com/test Downloading https://test:****@github.com/test

@liam-stevenson liam-stevenson changed the title Fix for 6783 Mask passwds in downloads.py output Aug 4, 2019
uranusjr
uranusjr approved these changes Aug 5, 2019
View reviewed changes
Copy link
Member

@uranusjr uranusjr left a comment

Choose a reason for hiding this comment

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

Looks trivial enough to me :)

@chrahunt chrahunt mentioned this pull request Aug 12, 2019
Closed
cjerdonek
cjerdonek reviewed Aug 19, 2019
View reviewed changes
src/pip/_internal/download.py
@@ -839,13 +840,13 @@ def written_chunks(chunks):
progress_indicator = DownloadProgressProvider(progress_bar,
max=total_length)
if total_length:
logger.info("Downloading %s (%s)", url, format_size(total_length))
logger.info("Downloading %s (%s)", redact_password_from_url(url), format_size(total_length))
Copy link
Member

Choose a reason for hiding this comment

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

It would be good to call this once above if show_progress IMO:

redacted_url = redact_password_from_url(url)

It will make the code a bit less cluttered.

@cjerdonek cjerdonek added type: bugfix type: security Has potential security implications labels Aug 19, 2019
cjerdonek
cjerdonek suggested changes Aug 19, 2019
View reviewed changes
Copy link
Member

@cjerdonek cjerdonek left a comment •
edited
Loading

Choose a reason for hiding this comment

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

You should also add a bugfix news entry. The bug number can be the number of this PR. @xavfernandez's message below has an appropriate issue number.

@pradyunsg
Copy link
Member

That'd be a file news/6834.bugfix.

@liam-stevenson
Copy link
Author

I'll make the amendments, thanks for the feedback!

@xavfernandez
Copy link
Member

This solves #6783 (so this could be 6783.bugfix ;) )

@chrahunt
Copy link
Member

Hi @YoloSecurity! Were you planning to get back to this?

@chrahunt chrahunt added the S: awaiting response Waiting for a response/more information label Sep 18, 2019
@liam-stevenson
Copy link
Author

liam-stevenson commented Sep 18, 2019 via email

@chrahunt
Copy link
Member

No problem! And no stress, if you find yourself still busy just say the word and one of us can pick this up. 👍 Thanks for your work on this so far.

@chrahunt chrahunt removed the S: awaiting response Waiting for a response/more information label Sep 18, 2019
@BrownTruck
Copy link
Contributor

Hello!

I am an automated bot and I have noticed that this pull request is not currently able to be merged. If you are able to either merge the master branch into this pull request or rebase this pull request against master then it will be eligible for code review and hopefully merging!

@BrownTruck BrownTruck added the needs rebase or merge PR has conflicts with current master label Sep 21, 2019
@liam-stevenson
Copy link
Author

liam-stevenson commented Sep 23, 2019 via email

@VarunWachaspati
Copy link

@YoloSecurity any update ?
If not then someone else can pick it up

@Cactusmachete
Copy link
Contributor

If no one's looking into this right now, I'll gladly pick it up and see what I can do.

@Cactusmachete Cactusmachete mentioned this pull request Nov 17, 2019
Merged
@chrahunt
Copy link
Member

This was taken care of in #7373. Thanks @YoloSecurity for the initial implementation and everyone else for the helpful comments!

@chrahunt chrahunt closed this Nov 17, 2019
@lock lock bot added the auto-locked Outdated issues that have been locked by automation label Dec 17, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Dec 17, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@cjerdonek cjerdonek cjerdonek requested changes

@uranusjr uranusjr uranusjr approved these changes

Assignees
No one assigned
Labels
auto-locked Outdated issues that have been locked by automation needs rebase or merge PR has conflicts with current master type: security Has potential security implications
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@liam-stevenson @pradyunsg @xavfernandez @chrahunt @BrownTruck @VarunWachaspati @Cactusmachete @cjerdonek @uranusjr