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

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

Merged
merged 4 commits into from
Aug 21, 2019

Conversation

cjerdonek
Copy link
Member

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
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. :)

src/pip/_internal/utils/misc.py Show resolved Hide resolved
src/pip/_internal/vcs/bazaar.py Outdated Show resolved Hide resolved
src/pip/_internal/vcs/git.py Outdated Show resolved Hide resolved
src/pip/_internal/vcs/versioncontrol.py Show resolved Hide resolved
tests/unit/test_utils.py Show resolved Hide resolved
@cjerdonek
Copy link
Member Author

PR updated. Thanks again for the thorough review, @chrahunt. 🙏

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.

src/pip/_internal/vcs/subversion.py Show resolved Hide resolved
@cjerdonek
Copy link
Member Author

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

Thanks again for the thoughtful reviews, @chrahunt and @pradyunsg!

# 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

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

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

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants