Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

gh-138577: Fix keyboard shortcuts in getpass with echo_char#141597

Open
CuriousLearner wants to merge 16 commits intopython:mainfrom
CuriousLearner:fix-gh-138577
Open

gh-138577: Fix keyboard shortcuts in getpass with echo_char#141597
CuriousLearner wants to merge 16 commits intopython:mainfrom
CuriousLearner:fix-gh-138577

Conversation

@CuriousLearner
Copy link
Member

@CuriousLearnerCuriousLearner commentedNov 15, 2025•
edited by github-actionsbot
Loading

Fixes#138577

When usinggetpass.getpass(echo_char='*'), keyboard shortcuts likeCtrl+U (kill line),Ctrl+W (erase word), andCtrl+V (literal next) now work correctly by reading the terminal's control character settings and processing them in non-canonical mode.

Tested with:

importgetpass# Test Ctrl+U (clear line)# Type "wrong", press Ctrl+U, type "correct"password=getpass.getpass(echo_char="*")print(f"Result:{password!r}")# Expected: 'correct'# Test Ctrl+W (erase word)# Type "hello world", press Ctrl+W, type "python"password=getpass.getpass(echo_char="*")print(f"Result:{password!r}")# Expected: 'hello python'# Test Ctrl+V (literal next)# Type "test", press Ctrl+V, press Ctrl+U, type "more"password=getpass.getpass(echo_char="*")print(f"Result:{password!r}")# Expected: 'test\x15more' (contains literal Ctrl+U)

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

When using getpass.getpass(echo_char='*'), keyboard shortcuts likeCtrl+U (kill line), Ctrl+W (erase word), and Ctrl+V (literal next)now work correctly by reading the terminal's control charactersettings and processing them in non-canonical mode.
picnixz
picnixz previously requested changesNov 15, 2025
Copy link
Member

@picnixzpicnixz left a comment

Choose a reason for hiding this comment

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

Thanks for doing this but this looks like a very complicate patch which is what I feared :( Also, I personally would use Ctrl+a/Ctrl+e for jumping at the start/end of what I write and ctrl+k (I think people are more familiar with ctrl+a/ctrl+k rather than ctrl+u for erasing an entire line in general...).

So I'm not really confident in actually changing this. I think deleting the last character that was typed (with DEL) is usually what people expect.

In addition, to make the feature more extendable in the future, I would prefer a tokenizer-based + dispatcher based approach instead (like we currently do for the REPL) because with many ifs like that it becomes hard to follow what's being done. But this would require an refactoring of this module which I don't really know if it's worth.

Comment on lines +211 to +215
# Control chars from termios are bytes, convert to str
erase_char = term_ctrl_chars['ERASE'].decode('latin-1') if isinstance(term_ctrl_chars['ERASE'], bytes) else term_ctrl_chars['ERASE']
kill_char = term_ctrl_chars['KILL'].decode('latin-1') if isinstance(term_ctrl_chars['KILL'], bytes) else term_ctrl_chars['KILL']
werase_char = term_ctrl_chars['WERASE'].decode('latin-1') if isinstance(term_ctrl_chars['WERASE'], bytes) else term_ctrl_chars['WERASE']
lnext_char = term_ctrl_chars['LNEXT'].decode('latin-1') if isinstance(term_ctrl_chars['LNEXT'], bytes) else term_ctrl_chars['LNEXT']
Copy link
Member

Choose a reason for hiding this comment

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

Those are too long loines and they are unreadable. All of that can be a single function that is given the action to perform and the current capabilities.

Comment on lines +218 to +221
erase_char = '\x7f' # DEL
kill_char = '\x15' # Ctrl+U
werase_char = '\x17' # Ctrl+W
lnext_char = '\x16' # Ctrl+V
Copy link
Member

Choose a reason for hiding this comment

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

Ideally, we should have them defined in a global private dict instead to ease maintenance.

CuriousLearner reacted with thumbs up emoji
Comment on lines +83 to +88
term_ctrl_chars = {
'ERASE': cc[termios.VERASE] if termios.VERASE < len(cc) else b'\x7f',
'KILL': cc[termios.VKILL] if termios.VKILL < len(cc) else b'\x15',
'WERASE': cc[termios.VWERASE] if termios.VWERASE < len(cc) else b'\x17',
'LNEXT': cc[termios.VLNEXT] if termios.VLNEXT < len(cc) else b'\x16',
}
Copy link
Member

Choose a reason for hiding this comment

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

This code must be refactored with a combination of a global dict with the defaults and a function.

@bedevere-app
Copy link

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 phraseI 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.

@CuriousLearner
Copy link
MemberAuthor

CuriousLearner commentedNov 15, 2025•
edited
Loading

Also, I personally would use Ctrl+a/Ctrl+e for jumping at the start/end of what I write and ctrl+k (I think people are more familiar with ctrl+a/ctrl+k rather than ctrl+u for erasing an entire line in general...).

Agree on this, since the original ticket asked for Ctrl+U, I would handle all these Ctrl characters.

In addition, to make the feature more extendable in the future, I would prefer a tokenizer-based + dispatcher based approach instead (like we currently do for the REPL) because with many ifs like that it becomes hard to follow what's being done

I'm refactoring the patch to be more extensible in the future, especially as more control characters are added.

@CuriousLearner
Copy link
MemberAuthor

It actually grew a lot bigger than I initially expected, but based on your review, I can iterate over this@picnixz

picnixz reacted with heart emoji

@picnixz
Copy link
Member

It looks great. The patch itself is small but the tests are long so I think it is fine. Nonetheless I think it would be better to have it in 3.15 rather than in 3.14.1. WDYT? (i need to review it but not today)

CuriousLearner reacted with thumbs up emoji

Copy link
Member

@picnixzpicnixz left a comment

Choose a reason for hiding this comment

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

Quick review.


def refresh_display(self):
"""Redraw the entire password line with asterisks."""
self.stream.write('\r' + ' ' * (len(self.passwd) + 20) + '\r')
Copy link
Member

Choose a reason for hiding this comment

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

Why are we adding 20 extra characters?

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

The +20 was an arbitrary buffer and has been removed. The current implementation now uses justlen(self.passwd) to clear only the necessary characters:

self.stream.write('\r'+' '*len(self.passwd)+'\r')

Comment on lines +251 to +252
self.ctrl = {name: _decode_ctrl_char(value)
for name, value in ctrl_chars.items()}
Copy link
Member

Choose a reason for hiding this comment

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

Can't we have the POSIX defaults already decoded?

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Refactored this inb8609bd

@CuriousLearner
Copy link
MemberAuthor

Hi@picnixz 👋🏼

Sorry, it took some time to address the review here. I've refactored the patch & I thinkPython 3.15 would be a better choice for this, though I don't see any issues if it gets intoPython 3.14.1 (unless I'm missing something).

Comment on lines +311 to +312
while self.cursor_pos > 0 and self.passwd[self.cursor_pos-1] == ' ':
self.cursor_pos -= 1
Copy link
Member

Choose a reason for hiding this comment

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

You can actually skip the trailing spaces as follows:

stripped = self.passwd.rstrip(' ')self.cursor_pos = self.cursor_pos - (len(self.passwd) - len(stripped))

Comment on lines +314 to +315
while self.cursor_pos > 0 and self.passwd[self.cursor_pos-1] != ' ':
self.cursor_pos -= 1
Copy link
Member

Choose a reason for hiding this comment

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

And here, usestr.rfind using the start of the new cursor position.

* upstream/main: (1475 commits)  Docs: replace all `datetime` imports with `import datetime as dt` (python#145640)pythongh-146153: Use `frozendict` in pure python fallback for `curses.has_key` (python#146154)pythongh-138234: clarify returncode behavior for subprocesses created with `shell=True` (python#138536)pythongh-140947: fix contextvars handling for server tasks in asyncio  (python#141158)pythonGH-100108: Add async generators best practices section (python#141885)pythonGH-145667: Merge `GET_ITER` and `GET_YIELD_FROM_ITER` (pythonGH-146120)pythongh-146228: Better fork support in cached FastPath (python#146231)pythongh-146227: Fix wrong type in _Py_atomic_load_uint16 in pyatomic_std.h (pythongh-146229)pythongh-145980: Fix copy/paste mistake in binascii.c (python#146230)pythongh-146092: Raise MemoryError on allocation failure in _zoneinfo (python#146165)pythongh-91279: Note `SOURCE_DATE_EPOCH` support in `ZipFile.writestr()` doc (python#139396)pythongh-146196: Fix Undefined Behavior in _PyUnicodeWriter_WriteASCIIString() (python#146201)pythongh-143930: Reject leading dashes in webbrowser URLspythongh-145916: Soft-deprecate ctypes.util.find_library (pythonGH-145919)pythongh-146205: Check the errno with != 0 in close impls in select module (python#146206)pythongh-146171: Fix nested AttributeError suggestions (python#146188)pythongh-146099: Optimize _GUARD_CODE_VERSION+IP via function version symbols (pythonGH-146101)pythongh-145980: Add support for alternative alphabets in the binascii module (pythonGH-145981)pythongh-145754: Update signature retrieval in unittest.mock to use forwardref annotation format (python#145756)pythongh-145177: Add emscripten run --test, uses test args from config.toml (python#146160)  ...
@CuriousLearner
Copy link
MemberAuthor

Hey@picnixz 👋🏼

Thanks for the review :) I've addressed all your reviews. Can you please take another pass at this?

Copy link
Member

@picnixzpicnixz left a comment

Choose a reason for hiding this comment

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

This looks great but I will try it locally to see whether there are other finalizing touches to do. Thanks for the awesome work though! I should review it by the end of next week.

CuriousLearner reacted with hooray emojiCuriousLearner reacted with rocket emoji
@vstinner
Copy link
Member

I tried thereproducer: type "hello world", press Ctrl+W. I get:

******rd: ***********

I expected the output:

Password: *****

_readline_with_echo_char() is not called withprompt argument and_PasswordLineEditor._refresh_display() writes\r without rewriting the prompt.

There are ways to trigger this output error. For example, type "abc", press CTRL+A and then press "x":

****word: ***

I expected the output:

Password: ****
CuriousLearner reacted with eyes emoji

@picnixz
Copy link
Member

Oh I think the problem is the prompt being counted inside the buffer. We should simply have a single buffer for the password and prepend the prompt everytime we refresh the line instead

CuriousLearner reacted with eyes emoji

…8577* 'main' of github.com:python/cpython:pythongh-146197: Run -m test.pythoninfo on the Emscripten CI (python#146332)pythongh-146325: Use `test.support.requires_fork` in test_fastpath_cache_cleared_in_forked_child (python#146330)pythongh-146197: Add Emscripten to CI (python#146198)pythongh-143387: Raise an exception instead of returning None when metadata file is missing. (python#146234)pythongh-108907: ctypes: Document _type_ codes (pythonGH-145837)pythongh-146175: Soft-deprecate outdated macros; convert internal usage (pythonGH-146178)pythongh-146056: Rework ref counting in treebuilder_handle_end() (python#146167)  Add a warning about untrusted input to `configparser` docs (python#146276)pythongh-145264: Do not ignore excess Base64 data after the first padded quad (pythonGH-145267)pythongh-146308: Fix error handling issues in _remote_debugging module (python#146309)pythongh-146192: Add base32 support to binascii (pythonGH-146193)pythongh-135953: Properly obtain main thread identifier in Gecko Collector (python#146045)pythongh-143414: Implement unique reference tracking for JIT, optimize unpacking of such tuples (pythonGH-144300)pythongh-146261: Fix bug in `_Py_uop_sym_set_func_version` (pythonGH-146291)pythongh-145144: Add more tests for UserList, UserDict, etc (pythonGH-145145)pythongh-143959: Fix test_datetime if _datetime is unavailable (pythonGH-145248)pythongh-146245: Fix reference and buffer leaks via audit hook in socket module (pythonGH-146248)pythongh-140049: Colorize exception notes in `traceback.py` (python#140051)  Update docs forpythongh-146056 (pythonGH-146213)
@CuriousLearner
Copy link
MemberAuthor

Thank you for your input@vstinner . I've addressed your reviews. Do you mind taking another pass?

cc@picnixz

'\b': self._handle_erase, # Backspace
}

def _refresh_display(self, prev_len=None):
Copy link
Member

Choose a reason for hiding this comment

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

The whole class is private, I don't think that it's useful to mark methods as private as well. Can you remove th underscore ("_") prefix from all methods?

CuriousLearner reacted with eyes emoji
Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Done, I’ve made the change. Just for my understanding and to follow the preferred style going forward: if a class is already internal/non-public, is it generally considered unnecessary to prefix all its methods with _ as well?

Copy link
Member

Choose a reason for hiding this comment

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

I don't know if it's a general rule, but yes, if a class is private, I consider that it's not needed to mark its methods as private as well.

…on into fix-pythongh-138577* 'fix-pythongh-138577' of github.com:CuriousLearner/cpython:pythongh-146202: Create tmp_dir in regrtest worker (python#146347)pythongh-144319: obtain SeLockMemoryPrivilege on Windows (python#144928)pythongh-146199: Fix error handling in `code_richcompare` when `PyObject_RichCompareBool` fails (python#146200)pythongh-146197: Include a bit more information in sys._emscripten_info.runtime (python#146346)pythongh-135871: Reload lock internal state while spinning in `PyMutex_LockTimed` (pythongh-146064)pythongh-145719: Add `.efi` file detection in `mimetypes` (python#145720)
…8577* 'main' of github.com:python/cpython:  Remove inactive CODEOWNERS (python#145930)pythongh-140196: Added constructor behavior changes in ast.rst for python 3.13 (pythonGH-140243)
Co-authored-by: Victor Stinner <vstinner@python.org>
Copy link
Member

@vstinnervstinner left a comment

Choose a reason for hiding this comment

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

LGTM.

@picnixz: Do you have to double check the change?

CuriousLearner reacted with hooray emoji
@picnixz
Copy link
Member

Yes, I'd like to double-check this either this evening or this w-e (I am a bit tight with my daily job's schedule)

vstinner and CuriousLearner reacted with thumbs up emoji

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@vstinnervstinnervstinner approved these changes

@picnixzpicnixzAwaiting requested review from picnixz

Assignees

No one assigned

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

getpass'secho_char should not affect keyboard shortcuts

4 participants

@CuriousLearner@picnixz@vstinner@nazeerali4325-commits
[8]ページ先頭
©2009-2026 Movatter.jp