gh-77065: Add optional keyword-only argument echochar for getpass.getpass

[donBarbos]

• Issue: Adding the ability for getpass to print asterisks when password is typed #77065

---

📚 Documentation preview 📚: https://cpython-previews--130496.org.readthedocs.build/

[donBarbos]

@picnixz sorry for choosing you, I just haven't found anyone else from the core team who is active yet (I looked at who is involved in issue and experts table). could you review?

[picnixz]

You can ping me for anything that is remotely related to security and cryptography

[picnixz]

A mask is something else for me. So I would either name it "echochar" or "maskchar" or "hidechar", at least with some "char" inside it.

In addition, for termios, you should use ECHOE to handle ERASE

[bedevere-app]

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

[donBarbos]

@picnixz I chose mask because it was used in the issue examples and also it seems that *char assumes one character while our argument can take arbitrary length. But it's not very important for me.
As for using keyword-only argument, I don't quite understand why that is.

[picnixz]

As for using keyword-only argument, I don't quite understand why that is.

Because if you were to use it, you would write getpass(mask="*"), likely not changing the prompt nor the stream value. And keyword-only arguments is good for future compatibility (e.g., if we want to add more options, we are not held back by the order of the arguments)

[picnixz]

I chose mask because it was used in the issue examples and also it seems that *char

Usually, you expect 1 character indeed. I don't think we should write something else than 1 char (or maybe we do but it's because of colors?) I would not expect words to be put as placeholders. Maybe colorized characters, which in this case are allowed to be strings of length > 1, but not real words.

Also, what we are echoing is really the corresponding password character. For each character, we use that placeholder. mask is really not a good idea because it could hint that we would (bitwise) mask the password (namely perform some & operation). But this is something we can think of at the end.

[donBarbos]

I have made the requested changes; please review again

[bedevere-app]

Thanks for making the requested changes!

@picnixz: please review the changes made to this pull request.

[donBarbos]

@picnixz I just thought maybe we also should to add , *, echochar=None to getpass.fallback_getpass to have a unified interface

changes were made

[donBarbos]

length of the emoji can be even longer if it is compound.

and it seems there is no objective reason to limit the length for echochar

I also opened a separate issue #130524 for testing this module as they don't cover much

[picnixz]

and it seems there is no objective reason to limit the length for echochar

There is. Because the purpose of this feature is visual feedback to help counting the number of characters. However, it doesn't seem useful to have a text being replaced. No CLI would do that honestly... For instance, Java uses a single character as well. And usually people would want to use echochar='*', not some fancy string. I really don't want to support non-ASCII string or string that could mess up the input. What if I use echochar='\b' for instance? or some control character instead? In sudo, it's hardcoded with *: https://github.com/sudo-project/sudo/blob/a4f31cff2b43cc0e5ad6b5c19ef5dd9a64bd883c/src/tgetpass.c#L416. Also, for non-ASCII characters (smileys for instance), we also need to have a fallback in case the encoding fails in the user's locale I think (not everything is encoded similarly depending on the locale).

So to ease our life, I would advise accepting for now simple ASCII characters.

[donBarbos]

yeah, it looks funny when we use '\b' as echochar but I don't think it's a problem, it looks like a feature :)

but if you suggest to support only ASCII characters at least for now I guess we can use .isascii method, right?

[donBarbos]

@picnixz maybe we should use UnicodeWarning if echochar and not echochar.isascii()? or just raise ValueError?

[picnixz]

just raise ValueError

Yes, we should just validate echochar before doing anything, and then raise ValueError if it's not ascii. Then use it normally.

[picnixz]

I still think we at least need tests for Unix. Or if we don't have the infrastructure yet, wait until we have it and then add them.

[donBarbos]

cc @picnixz

EDIT: I thought maybe I could do the echochar check the other way around, that is, allow everything except ord(echochar)<32

(I'll continue tomorrow)

[picnixz]

Until tests are added in this PR, I will not accept it. I have no idea whether the code for Windows will work properly or not. So I would really want tests. Until then, please do not request reviews from me.

If the infrastructure is not yet in place, please make it so (either by creating a new issue so that we can work on it or by adding it yourself). NVM, it already exists: #130529.

[bedevere-app]

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

[picnixz]

NVM, I forgot that there was an open PR for that. However it hadn't advanced since then. However, I would want to have tests for Unix.

[donBarbos]

I have made the requested changes; please review again

[bedevere-app]

Thanks for making the requested changes!

@picnixz: please review the changes made to this pull request.