Skip to content

fix(cli): drop escape-sequence tail in masked_secret_prompt on POSIX - #64135

Open
thejesh23 wants to merge 2 commits into
NousResearch:mainfrom
thejesh23:fix/secret-prompt-drain-escape-sequence
Open

thejesh23 wants to merge 2 commits into
NousResearch:mainfrom
thejesh23:fix/secret-prompt-drain-escape-sequence

Conversation

@thejesh23

Copy link
Copy Markdown

What does this PR do?

Fixes silent secret corruption in hermes_cli.secret_prompt._masked_secret_prompt_posix: pressing an arrow key (or any CSI/SS3 key — Home/End, PageUp/PageDown, function keys, Insert, Delete) mid-secret used to leak the sequence's tail bytes (e.g. [A for arrow-up) into the captured value, because _collect_masked_input only skipped the leading \x1b. Every keystroke is masked with *, so the corruption was invisible — the user would see a downstream "wrong API key / bad password" error with no clue why.

Root cause: the POSIX reader used sys.stdin.read(1) in a raw-mode loop with no notion of escape sequences.

Fix: read raw bytes via os.read(fd, 1) (bypasses any TextIOWrapper buffering that might hide bytes from the drain) and, after an ESC byte, drain any bytes pending on the fd within a 5 ms select window. 5 ms is comfortably longer than any local terminal takes to burst these bytes and shorter than a plausible human ESC-then-typed-key interval. Multi-byte UTF-8 code points are reassembled with an incremental decode so non-ASCII secrets keep working.

Windows path (_masked_secret_prompt_windows) unchanged — msvcrt.getwch() already collapses two-byte special-key sequences into a single \x1b token.

Related Issue

Fixes #64134

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)
  • ✨ New feature (non-breaking change that adds functionality)
  • 🔒 Security fix
  • 📝 Documentation update
  • ✅ Tests (adding or improving test coverage)
  • ♻️ Refactor (no behavior change)
  • 🎯 New skill (bundled or hub)

Changes Made

  • hermes_cli/secret_prompt.py — extracted _read_posix_raw_char(fd) for a byte-safe raw-mode read; _masked_secret_prompt_posix composes it with the unchanged mask/erase loop.
  • tests/hermes_cli/test_secret_prompt.py — four new POSIX-only regression tests: single ASCII byte, multi-byte UTF-8 reassembly, arrow-key drain, and an end-to-end "typed secret with arrow key mid-input" via _collect_masked_input. Uses os.pipe with realistic timing (small sleeps between the escape-burst and the next typed byte, mirroring how a real terminal delivers them). No pty needed, so tests are stable across CI runners.

How to Test

  1. Deterministic reproducer (this is the pre-fix behavior — on main, this prints 'pass[Aword'):
    from hermes_cli.secret_prompt import _collect_masked_input
    stream = iter(list('pass') + ['\x1b', '[', 'A'] + list('word') + ['\r'])
    out = []
    captured = _collect_masked_input(lambda: next(stream), out.append, 'pw: ', mask='*')
    print(repr(captured))  # want 'password', bug prints 'pass[Aword'
  2. Full test file: pytest tests/hermes_cli/test_secret_prompt.py -v — 8 tests pass locally (4 pre-existing + 4 new) in <0.3 s.
  3. Manual: hermes auth login (or any other flow that calls masked_secret_prompt), paste part of a token, press an arrow key, finish typing, submit. Post-fix the token should be accepted; pre-fix it is silently corrupted.

Checklist

Code

Documentation & Housekeeping

  • I've updated relevant documentation (docstring on the new helper explains the drain and its timing budget) — no README/docs/ changes needed
  • I've updated cli-config.yaml.example if I added/changed config keys — N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — N/A
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide — Windows path is untouched, tests are guarded with pytest.mark.skipif(sys.platform == "win32", ...)
  • I've updated tool descriptions/schemas if I changed tool behavior — N/A

When a user pressed an arrow key (or any CSI/SS3 key: Home/End,
PageUp/PageDown, function keys, etc.) mid-secret, the terminal sent
ESC + one or more follow-up bytes. `_collect_masked_input` only skipped
the leading ESC; the trailing bytes (e.g. "[A" for arrow-up) were
appended to the captured secret. Every keystroke is masked, so the
corruption was silent — the user would see wrong-password errors and
no clue why.

Reproducer (pre-fix): typing "pass<Up>word<Enter>" captured "pass[Aword".

Fix: read raw bytes via `os.read(fd, 1)` (bypasses TextIOWrapper
buffering) and, after an ESC byte, drain any bytes pending on the fd
within a 5 ms `select` window before returning. Multi-byte UTF-8 is
reassembled via incremental decode so non-ASCII secrets keep working.

Adds four tests covering ASCII, multi-byte UTF-8, escape-sequence
drain, and the end-to-end "typed secret with arrow key" case. Windows
path is unchanged (msvcrt already returns "\x1b" as a single token
for special keys).
@alt-glitch alt-glitch added type/bug Something isn't working comp/cli CLI entry point, hermes_cli/, setup wizard area/auth Authentication, OAuth, credential pools P2 Medium — degraded but workaround exists labels Jul 14, 2026

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for tracing this to the shared POSIX reader. The premise is verified on current main: hermes_cli/secret_prompt.py:115-116 reads one character at a time, while _collect_masked_input ignores only ESC at lines 46-49 and appends subsequent bytes at line 51.

Problems

  • Proposed hermes_cli/secret_prompt.py:135 drains every currently readable byte after ESC without confirming a CSI/SS3 introducer. This can discard non-sequence input that follows ESC, including the character in terminals that encode Meta/Alt as ESC plus a character.

Suggested changes

  • Parse and consume only confirmed CSI/SS3 tails, preserving an unrecognized byte for normal collection. Add coverage for ESC followed by an ordinary character as well as the arrow-key regression.

Automated hermes-sweeper review.

Comment thread hermes_cli/secret_prompt.py Outdated
if not b:
return ""
if b == b"\x1b":
while select.select([fd], [], [], 0.005)[0]:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This drains any byte arriving within the window, not only a CSI/SS3 tail. Please confirm ESC [ or ESC O before consuming a sequence and preserve an unrecognized following byte; otherwise Meta/Alt-style ESC + character input is silently lost.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Good catch — pushed 1916401 addressing this.

_read_posix_raw_char no longer drains everything after ESC. It now peeks one follow-up byte and:

[ → consume CSI tail up to a final in 0x40..0x7E, return \x1b
O → consume exactly one SS3 final byte, return \x1b
anything else → return the follow-up byte as an ordinary character (Meta/Alt- or paste-starts-with-ESC), so it flows into _collect_masked_input normally
no follow-up in the 5 ms window → return \x1b (lone Escape, collector skips it)
Sequence-tail reads still go through short select polls so a malformed sequence can't hang the prompt.

Added four POSIX-only tests: SS3 drain, lone-Escape return, Meta/Alt follow-up preservation, and an end-to-end "typed secret with Alt-key mid-input" mirroring the existing arrow-key end-to-end test. All 12 tests pass locally.

…ending byte

The prior fix drained every byte readable within 5 ms of an ESC. That
over-reaches: terminals encode Meta/Alt-<char> as ESC + <char>, and any
paste that happens to start with ESC also has a real follow-up byte.
Draining those would trade the arrow-key corruption bug for a Meta-key
corruption bug in the same silent-secret class.

After the initial ESC, peek for a follow-up byte; if it's `[` consume a
CSI tail up to a final byte in 0x40..0x7E, if `O` consume exactly one
SS3 final byte, otherwise return the follow-up byte as an ordinary
character so it's preserved for normal collection. Lone ESC (no
follow-up in the short window) still surfaces as "\x1b" for the
collector to skip. Sequence-tail reads still go through a small
`select` poll so a malformed sequence can't hang the prompt.

Adds four POSIX-only tests: SS3 drain, lone-Escape return, Meta/Alt
follow-up preservation, and an end-to-end "typed secret with Alt-key
mid-input" that mirrors the existing arrow-key end-to-end test.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@teknium1 teknium1 added sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 16, 2026

This branch has not been deployed

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

Labels

area/auth Authentication, OAuth, credential pools comp/cli CLI entry point, hermes_cli/, setup wizard P2 Medium — degraded but workaround exists sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

hermes_cli.secret_prompt: arrow keys and other CSI/SS3 sequences silently corrupt captured secrets on POSIX

3 participants