Skip to content

Hide security-sensitive strings in VCS command log messages #6890

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
Merged
merged 4 commits into from
Aug 21, 2019
Merged

Hide security-sensitive strings in VCS command log messages #6890

merged 4 commits into from
Aug 21, 2019

Conversation

@cjerdonek
Copy link
Member

@cjerdonek cjerdonek commented Aug 17, 2019

This PR adds a new HiddenText class to wrap sensitive strings and starts using them in the VCS modules to protect (1) URL's containing potential auth info, and (2) passwords that can be included in VCS command-line arguments. This finishes the work started in PR #6862.
@cjerdonek cjerdonek added C: vcs pip's interaction with version control systems like git, svn and bzr type: bugfix type: security Has potential security implications labels Aug 17, 2019
@cjerdonek cjerdonek force-pushed the vcs-hidden-text branch 3 times, most recently from a93a8f7 to b9d70a6 Compare August 17, 2019 17:55
@cjerdonek cjerdonek mentioned this pull request Aug 17, 2019
Closed
chrahunt
chrahunt reviewed Aug 17, 2019
View reviewed changes
Copy link
Member

@chrahunt chrahunt left a comment

Choose a reason for hiding this comment

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

What a nice idea, good job. :)
@chrahunt chrahunt mentioned this pull request Aug 18, 2019
Closed
@cjerdonek
Copy link
Member Author

cjerdonek commented Aug 18, 2019

PR updated. Thanks again for the thorough review, @chrahunt. 🙏
chrahunt
chrahunt reviewed Aug 18, 2019
View reviewed changes
Copy link
Member

@chrahunt chrahunt left a comment

Choose a reason for hiding this comment

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

One minor comment, otherwise LGTM.
pradyunsg
pradyunsg reviewed Aug 18, 2019
View reviewed changes
@cjerdonek
Copy link
Member Author

cjerdonek commented Aug 19, 2019

PR updated. I also implemented HiddenText.__ne__() (which is needed for Python 2) and added some unit tests for HiddenText's equality operators.
@cjerdonek cjerdonek merged commit 7783c47 into pypa:master Aug 21, 2019
@cjerdonek cjerdonek deleted the vcs-hidden-text branch August 21, 2019 10:23
@cjerdonek
Copy link
Member Author

cjerdonek commented Aug 21, 2019

Thanks again for the thoughtful reviews, @chrahunt and @pradyunsg!
pradyunsg
pradyunsg reviewed Aug 21, 2019
View reviewed changes
src/pip/_internal/utils/misc.py
# Also, we don't apply str() to arguments that aren't HiddenText since
# this can trigger a UnicodeDecodeError in Python 2 if the argument
# has type unicode and includes a non-ascii character. (The type
# checker doesn't ensure the annotations are correct in all cases.)
Copy link
Member

@pradyunsg pradyunsg Aug 21, 2019

Choose a reason for hiding this comment

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

Reg the type-checker: I think we're supposed to use mypy.Text for things that would be unicode on Python 2.
Copy link
Member Author

@cjerdonek cjerdonek Aug 21, 2019

Choose a reason for hiding this comment

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

This is supposed to always be str (if everything is working correctly), but because our type-checking isn't 100%, it's possible something of unicode type can slip through the cracks (e.g. because of # type: ignore comments at some of our annotations). One example is the _get_win_folder_with_ctypes() function in appdirs.py that is annotated with str but returns unicode in Python 2. In cases that slip through the cracks, the point was not to force a crash if we don't need to..
@pradyunsg
Copy link
Member

pradyunsg commented Aug 21, 2019

This PR looks great @cjerdonek! ^>^
@lock lock bot added the auto-locked Outdated issues that have been locked by automation label Sep 20, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Sep 20, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

auto-locked Outdated issues that have been locked by automation C: vcs pip's interaction with version control systems like git, svn and bzr type: security Has potential security implications

3 participants

@cjerdonek @pradyunsg @chrahunt