Show more helpful messages for invalid passwords

[bhrutledge]

Update 10/5: Following the comments below, the implementation has changed to print warnings when it looks like the password isn't valid.

As noted in #671 (comment), when Windows users use Ctrl+V to paste their password, the resulting value is a control character, as documented in https://bugs.python.org/issue37426. The current behavior via #685 is to show <hidden> in that case, but that's not useful information. The intention of that PR was to show <empty>, which would have indicated that pasting didn't work as expected.

This is a lightweight attempt to give the user better feedback, and aid troubleshooting. In #685 (comment), @sigmavirus24 said:

I am strongly against revealing the length of a password. That's more metadata than is necessary.

However, given that this would be opt-in via --verbose, and the prevalence of this issue, I think the benefit outweighs the risk.

A further step could be to detect this case, and display a helpful warning message (possibly as a link to a new section in Twine's docs).

[bhrutledge]

@sigmavirus24 Any thoughts about my rationale for adding this?

[sigmavirus24]

I have thoughts but haven't been near a computer to respond. Presently mostly using my phone for triage of notifications these days. To jot them down

• we're leaking information that's sensitive, especially of this is run somewhere like GitHub actions with the appropriate flag (probably less of a concern with tokens but still not good security posture)

• we could detect "this is windows and that code point is what happens when someone fails to paste their password" at the first character and be helpful (tell the user they pasted incorrectly on a Windows prompt, link them to docs, and reprompt for the password)

• we could print something more helpful for "empty" or detect this and log something more helpful there too

There are better solutions than printing the number of characters in the password

[sigmavirus24]

Additionally I think verbose is repeatable which means we could have a trade off where after -vvv (or more) we show the asterisks.

[bhrutledge]

@sigmavirus24 Thanks for your thoughts. I realize we're in bike-shedding territory here, but it's helpful for me to think through this sort of thing.

we're leaking information that's sensitive, especially of this is run somewhere like GitHub actions with the appropriate flag

I was thinking that --verbose in CI seems unlikely, but a GitHub search shows that's an incorrect assumption. 😄 Still, it feels like the risk of a compromised password is low (although I'm far from a security expert).

I think verbose is repeatable which means we could have a trade off where after -vvv (or more) we show the asterisks.

It's currently a boolean, and I'm not inclined to add the complexity of additional levels for this.

twine/twine/settings.py

Lines 149 to 155 in 4c9d8c1

	@verbose.setter
	def verbose(self, verbose: bool) -> None:
	"""Initialize a logger based on the --verbose option."""
	self._verbose = verbose
	root_logger = logging.getLogger("twine")
	root_logger.addHandler(logging.StreamHandler(sys.stdout))
	root_logger.setLevel(logging.INFO if verbose else logging.WARNING)

we could detect "this is windows and that code point is what happens when someone fails to paste their password" (or)
we could print something more helpful for "empty" or detect this and log something more helpful (tell the user they pasted incorrectly on a Windows prompt, link them to docs, and reprompt for the password)

I was thinking about this, but I prefer a lightweight, general indication of "unusable password", rather than spending time writing and maintaining a complex edge case. I'm also wary of re-prompting for password, because Twine currently doesn't do that.

I might try to word-smith a general-purpose logger.warning message if the password is empty/short/unprintable. Beyond that, a quick survey of other CLI tools that prompt for a password (including Flit and Poetry) didn't turn up any that try to be more helpful. That doesn't mean we shouldn't try, but does make this a lower-priority issue for me.

[sigmavirus24]

I was thinking that --verbose in CI seems unlikely, but a GitHub search shows that's an incorrect assumption. smile Still, it feels like the risk of a compromised password is low (although I'm far from a security expert).

The issue isn't "we'll accidentally leak the plain-text of the password". The issue is that we're not providing a potential malicious actor enough information about the password to cut the problem space of brute forcing/generating the possible passwords significantly.

Without that information, the problem space is much larger.

It's currently a boolean, and I'm not inclined to add the complexity of additional levels for this.

Having different levels of available information is quite useful. Flake8 leverages this and it helps users find the right level of information when debugging a problem and helps developers of the tool and plugins as well.

I was thinking about this, but I prefer a lightweight, general indication of "unusable password", rather than spending time writing and maintaining a complex edge case.

Sure, does PyPI have a "minimum length" requirement on passwords? If so len(password) < minimum_length exits early

I'm also wary of re-prompting for password, because Twine currently doesn't do that.

I'd like to understand what makes you wary of this.

Beyond that, a quick survey of other CLI tools that prompt for a password (including Flit and Poetry) didn't turn up any that try to be more helpful.

I suspect those tools don't have wide adoption to the extent that Twine does. We have more users who tend to struggle with exactly this scenario. If you're working at a company and you have an increasing support burden on the product you work on, do you say "Yeah, we'll ignore this because it might add a tad bit of complexity to reduce the time we spend telling customers how to get things to behave as they expect it to based on our existing documentation?" We've fielded many issues about this exact problem. It's not a productive use of your or my severely limited time.

What you're saying here is that you don't value your time or mine enough to fix this. I, on the other hand, do and feel like:

1. Re-prompting a user is less frustrating to the user than exiting with an error message when we know what to tell the user to do
2. This current approach isn't going to actually lower the number of new comments or issues we receive about this issue.

If you'd like, I'm happy to put together my own PR for this

[bhrutledge]

Having different levels of available information is quite useful. Flake8 leverages this and it helps users find the right level of information when debugging a problem and helps developers of the tool and plugins as well.

I agree, but I think it would take a fair amount of work to implement them, and I'd rather have a solution that offers help to the users in a more immediate/obvious way.

I prefer a lightweight, general indication of "unusable password", rather than spending time writing and maintaining a complex edge case.

Sure, does PyPI have a "minimum length" requirement on passwords? If so len(password) < minimum_length exits early

I'm also wary of re-prompting for password, because Twine currently doesn't do that.

I'd like to understand what makes you wary of this.

PyPI seems to allow a 1-character password, but with a "password strength" meter. Other repositories could have different requirements (including maybe no password at all?). I'd rather let the repository be the source of truth for the validity of the password, and limit Twine to printing potentially helpful information (but ultimately accepting whatever the user enters, instead of exiting or re-prompting). Additionally, the prompting code is already complex, and seems to assume a single input, so I hesitate to bolt on re-prompting.

We've fielded many issues about this exact problem. It's not a productive use of your or my severely limited time.

Agreed.

What you're saying here is that you don't value your time or mine enough to fix this.

That's not what I'm saying, though I see how it could come across that way. I do value our time, and I'm invested in folks being successful with Twine. That's why I opened this PR. However, because of my limited time, I'm trying to be mindful of what we prioritize, and look for solutions that balance improving the user experience with the time it takes to implement them.

With that in mind, and because I'm curious to explore this bit of Python:

I might will try to word-smith a general-purpose logger.warning message if the password is empty/short/unprintable.

[sigmavirus24]

I agree, but I think it would take a fair amount of work to implement them, and I'd rather have a solution that offers help to the users in a more immediate/obvious way.

So we have different understandings of how long it would take to implement a secure change versus this change. That helps explain the disconnect here.

Additionally, the prompting code is already complex, and seems to assume a single input, so I hesitate to bolt on re-prompting.

I think it's complex primarily because it was refactored prematurely. Again, I don't feel like we're thinking of solving this at the same place and our estimates for difficulty are very different.

[bhrutledge]

@sigmavirus24 What do you think of something like this? I haven't tried it on Windows (yet), but here's what it looks like with an empty password:

Uploading distributions to https://test.pypi.org/legacy/
Enter your username: brian
Enter your password: 
  Your password is empty. Did you enter it correctly?
  See https://pypi.org/help/#invalid-auth for more information.
Uploading twine-3.4.2.dev12+g6694f57.d20210619-py3-none-any.whl
100%|██████████| 41.7k/41.7k [00:00<00:00, 153kB/s] 
For more details, retry the upload with the --verbose option.
HTTPError: 403 Forbidden from https://test.pypi.org/legacy/
Invalid or non-existent authentication information. See https://test.pypi.org/help/#invalid-auth for more information.

If this seems reasonable, I'm happy to add a new section to Twine's docs, to keep this independent of a specific repository.

[sigmavirus24]

LGTM

[bhrutledge]

@sigmavirus24 I added this note, and updated the code to it. What do you think?

https://twine--815.org.readthedocs.build/en/815/#using-twine

[sigmavirus24]

I might move this up and indent it underneath item 2, but that's more of a nit

[bhrutledge]

I did that at first, but it felt distracting. I also think it works better as a direct link outside of the steps.