Repository navigation
refactor(cli): replace hand-rolled terminal code with crossterm - #127
Conversation
tee8z
left a comment
There was a problem hiding this comment.
Only one finding. Everything else holds up. Raw mode is on before the prompt is printed, the guard restores the terminal on cancel, the buffer is preallocated and zeroized, and the key handling has tests for every branch. Removing the unsafe exception is a good trade.
Finding: the rest of an over-long paste reaches the shell. When type_hidden returns TooLong, read_hidden_keys returns at once. crossterm reads at most 1,024 bytes per read, so with a paste of, say, 3,000 bytes, about 2,000 bytes are still in the terminal's input queue. Raw mode is then turned off and the process exits, and the shell's line editor reads those bytes as typed input. They appear on the command line, and if the paste ends in a newline they run as a command and land in shell history. rpassword read to the end of the line, so this is new. A real API key never gets this long, but a mistaken paste of a file that holds secrets would.
I found this by reading the code and crossterm's 1,024-byte read. I have not reproduced it on a PTY. The existing test sends exactly 1,001 bytes, which all fit in one read, so it cannot see this case.
Fix: drain pending input while raw mode is still on (suggestion inline). A test could extend the_hidden_key_prompt_refuses_a_key_over_its_limit_and_saves_nothing to send a few thousand bytes, then check that the PTY has nothing left to read after the CLI exits.
Not reviewed as a finding (judgment calls):
- The prompt now goes to stderr, not
/dev/tty, soauth import-key ... 2>logon a terminal shows no prompt and looks hung. The CHANGELOG states the move. If that case matters,can_promptcould also require that stderr is a terminal and point to--stdinotherwise. - On Windows, AltGr characters arrive with
CONTROL | ALT, sotype_hiddendrops them. That is harmless for an alphanumeric key.
CRAP: not run on this head yet; I will add the result when it finishes. CI is green.
|
CRAP on 9dfa9d0: 869 of 1084 functions measured, 0 violations. Highest: 18.10 |
|
On the two judgment calls:
CRAP on 7b77dd7: 870 of 1086 functions measured, 0 violations. |
The hidden API-key prompt reads keys in crossterm's raw mode instead of rpassword, and a guard turns raw mode off through crossterm, which restores the saved terminal modes when Ctrl-C cancels a read that cannot return. This removes the unsafe tcgetattr/tcsetattr exception, the rpassword dependency, and the Unix-only restore: pickers and the hidden prompt now restore the terminal on every platform. A typed Ctrl-C at the hidden prompt exits 130 with the same interruption message as before. The picker, live-watch, and status-line escape sequences come from crossterm commands instead of literal bytes; the bytes written are the same, except that home is now ESC[1;1H.
78298d3 to
5f64b27
Compare
tee8z
left a comment
There was a problem hiding this comment.
Reviewed 5f64b27 against master. No blocking findings in this layer.
The hidden reader enables raw mode before showing the prompt, bounds the buffer, rejects overlong input, and flushes the remaining paste before restoring modes. Linux PTY tests cover editing, typed Ctrl-C, SIGINT, over-limit input, and terminal restoration.
Local validation passed: cargo fmt --check, cargo clippy --locked --all-targets -- -D warnings, cargo machete --with-metadata, cargo test --locked (209 unit + 153 integration), and python3 scripts/check-coverage.py. Required scripts/crap-report.sh also passed: 871/1088 functions measured, 217 unmeasured, zero scores >=20; highest 18.10 (wait_for_payment).
Limits: no live staging credential import or manual Windows/macOS terminal run; current cross-platform GitHub checks pass.
Fixes DEV-2904
Problem
terminal.rskept an approvedunsafeexception:TerminalModescalledlibc::tcgetattrandlibc::tcsetattrto put echo back when Ctrl-C cancelled the hidden API-key prompt, because rpassword restores echo only when its read returns. The restore was Unix-only. The picker, live-watch, and status-line guards also wrote escape sequences as literal bytes, although crossterm, which the CLI already uses for the width and the pickers, has a command for each.Change
/dev/ttywith echo and signals offTerminalModessaves and restores termios withunsafelibc calls (Unix only)RawModeguard callscrossterm::terminal::disable_raw_mode, which restores the modes crossterm saved when raw mode went on. It covers the hidden prompt and inquire's pickers on every platformInterrupted before any resource change was submitted(exit 130), the same as the pickersMAX_HIDDEN_BYTES), far above an API key. The buffer is allocated at that size up front, so typing never reallocates and leaves no unzeroized copy. One more character ends the read withThe secret is longer than 1000 bytes(exit 2) and saves nothing\x1b[?2004l\x1b[?25h,\x1b[?1049h…,\x1b[H\x1b[2J,\r\x1b[2Kas literalsDisableBracketedPaste,cursor::Show/Hide,Enter/LeaveAlternateScreen,MoveTo,Clear, rendered through aterminal::ansihelperansi()usesCommand::write_ansi, notexecute!. On Windows,execute!can call the console API directly, which would reorder the sequence with the buffered bytes around it. The bytes written are unchanged, except that home isESC[1;1Hinstead ofESC[H.docs/quality.md.rpasswordis removed (−22 lines inCargo.lock). Thecrosstermentry now listseventsandbracketed-paste; inquire already turned both on, so the dependency graph does not change.unsafeandlibcinsrc/are now limited tocheck_ownerandO_NOFOLLOWinconfig.rs. The integration tests still use libc to open and inspect pseudo-terminals.User-visible, so there is a CHANGELOG line: the hidden prompt is written to stderr instead of
/dev/tty, Ctrl-U clears it, and the restore works on every platform.Try it on staging
Setup: Running against staging. This saves an API key under a throwaway account name and then logs it out.
Use the organization and environment from
context:At
Environment API key:, type a few characters and press Ctrl-C. Expect: nothing you typed is shown,Interrupted before any resource change was submittedappears,echo $?prints 130, and typing in the shell echoes again (nostty saneneeded).Run the command again and type
wrong, press Ctrl-U, type a real or dummy key, use Backspace once, and press Enter. Expect: the key is not shown, and the command reportsimported. Then remove it:voltage logout --account try-keyRun it once more and paste more than 1,000 characters at the prompt, for example after
printf 'k%.0s' $(seq 1001) | pbcopy(macOS). Expect:The secret is longer than 1000 bytes,echo $?prints 2, the shell echoes again, andvoltage logout --account try-keyreports no such account.To check the picker guard, run
voltage wallets list --account mewithout--org. If your login sees more than one organization, a picker opens. Press Ctrl-C there, and check that the shell still echoes and shows its cursor.Validation
master(conflicts: CHANGELOG, kept both lines;Cargo.toml, keptmaster'swebbrowser = "=1.2.5"and droppedrpassword). On the head:cargo fmt --check,cargo clippy --locked --all-targets -- -D warnings,cargo machete --with-metadata,cargo test --locked(203 unit, 151 integration), andpython3 scripts/check-coverage.pyall pass.ctutils0.4.2 (non-constant-time masks), pulled in throughdigestbysha2andhkdf/secret-service/keyring.masteralready moved it to the fixed 0.4.5 in fix(deps): update ctutils to 0.4.5 for RUSTSEC-2026-0385 #113, so themastermerge fixes it;cargo auditon the head reports no vulnerabilities (only the existing allowed unmaintained/yanked warnings). No ignore was added.a_hidden_prompt_stops_at_its_limit_without_reallocatingand integration testthe_hidden_key_prompt_refuses_a_key_over_its_limit_and_saves_nothing(exit 2, modes restored, no credential).type_hiddenkey handling (typing, Shift, Backspace, Ctrl-U, ignored Alt/Ctrl chords, arrows, and key releases; Enter, Ctrl-J, and Ctrl-C), the read-result mapping (secret, interruption, and the transport error for a terminal that cannot enter raw mode or be read), and theansibyte output.the_hidden_key_prompt_saves_the_edited_key_without_showing_it: a pseudo-terminal types a key with Ctrl-U and Backspace edits. It checks the saved key, that the terminal output never contains it, and that the terminal's local modes match their values before the run.disable_raw_modecall removed, all three hidden-prompt tests fail, so the guard is what restores the terminal.scripts/crap-report.sh, cargo-llvm-cov 0.8.5, knots 1.16.0, on the head): 854 of 1067 functions measured, 0 violations. The highest score is 18.10,wait_for_paymentinsrc/api.rs, which this PR does not change. The new functions scoreread_hidden_keys5.0,type_hidden5.0,read_hidden3.01,hidden2.0, and the rest 1–2.CHANGELOG.mdandtests/cli_integration.rs; a trial merge conflicts only on adjacent CHANGELOG lines and the suite passes. Landing this one first is preferred, so feat(cli): pick the wallet, organization, or environment to show on a terminal #126's new picker tests run against the new guard.Limits
On Linux, crossterm reads a paste longer than 1,024 bytes in 1,024-byte parts, each after more input arrives. Any paste past the 1,000-byte limit is refused before that matters.
style.rsstill writes the OSC 8 hyperlink sequence by hand, because neither crossterm 0.29 nor anstyle has a hyperlink command. Thewordmark.rsescapes are test assertions only.Checks not run: the Windows and Linux restore paths were not tested by hand. CI builds and tests them, but there is no pseudo-terminal test on Windows.
Stack
Stack #135, layer 5 of 9 (bottom targets
master):--allonorganizations,environments, andprofiles listVOLTAGE_*variablevoltage context --format envvoltage feedbackand panicsvoltage environments createflake.nixwith a KaifileThe layers are chained with merge commits, because history is not rewritten. Use squash and merge, bottom first.