Skip to content

fix: masked password prompt with asterisk echo for IMAP accounts#216

Merged
wesm merged 10 commits intomainfrom
issue-209
Mar 24, 2026
Merged

fix: masked password prompt with asterisk echo for IMAP accounts#216
wesm merged 10 commits intomainfrom
issue-209

Conversation

@wesm
Copy link
Copy Markdown
Owner

@wesm wesm commented Mar 23, 2026 •
edited
Loading

Summary

  • Replace term.ReadPassword (no visual feedback) with charmbracelet/huh masked input that shows * per keystroke during IMAP password entry
  • Add pipe detection: when stdin is not a terminal, read password from piped stdin for scripted usage (echo "pass" | msgvault add-imap ...)
  • Use go-isatty for TTY detection (including Cygwin/MSYS), matching existing codebase patterns
  • Extract choosePasswordStrategy() with table-driven tests covering all stdin/stdout/stderr TTY combinations
  • Remove golang.org/x/term direct dependency (no longer needed)
  • Update nix flake vendorHash for new dependency

The strategy selection is a pure function of four booleans, tested across 10 cases:

stdin stderr stdout method
any TTY TTY any huh → stderr
any TTY redirected TTY huh → stdout
any TTY redirected redirected clear error
not TTY any any pipe read

Test plan

  • Table-driven tests for choosePasswordStrategy: 10 cases covering native/Cygwin/piped stdin × stderr/stdout TTY combinations
  • Table-driven tests for readPasswordFromPipe: newline trimming, CRLF, empty input, whitespace-only, EOF, multi-line
  • Manual test: msgvault add-imap --host ... --username ... shows asterisks during password entry
  • Manual test: echo "pass" | msgvault add-imap --host ... --username ... reads from pipe

Closes #209

🤖 Generated with Claude Code

@wesm @claude
fix: use masked password prompt with asterisk echo for IMAP accounts
d008ae0 
Replace term.ReadPassword (which shows no visual feedback) with a
charmbracelet/huh masked input that displays asterisks during typing.
This fixes the Windows issue where the password prompt appeared to
accept no input, and improves UX on all platforms.

Also adds pipe detection: when stdin is not a terminal, the password
is read from the pipe, enabling scripted usage like:
  echo "password" | msgvault add-imap --host ... --username ...

Closes #209

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@roborev-ci
Copy link
Copy Markdown

roborev-ci bot commented Mar 23, 2026

roborev: Combined Review (d008ae0)

The PR introduces masked and piped password inputs, but contains a regression related to terminal output handling during interactive prompts
.

Medium

  • Location: cmd/msgvault/cmd/addimap.go:162
  • Problem: readPasswordInteractive now uses huh.NewInput().Run() whenever stdin is a terminal, but that TUI prompt also depends on having a
    usable terminal output. If stdout is redirected or captured while stdin is still interactive, this can fail or emit control sequences into the redirected stream, which is a regression from the previous term.ReadPassword behavior.
  • Fix: Only use the huh prompt when both input and output
    are attached to a terminal (or explicitly bind it to /dev/tty); otherwise fall back to a non-TUI masked prompt such as term.ReadPassword.

Synthesized from 3 reviews (agents: codex, gemini | types: default, security)

@wesm @claude
fix: handle Cygwin terminals and redirected stdout in password prompt
a61c385 
Address two review findings:

1. Use go-isatty (IsTerminal + IsCygwinTerminal) instead of x/term for
   TTY detection, matching the pattern used elsewhere in the codebase.
   This fixes Cygwin/MSYS pseudo-terminals falling through to the pipe
   path.

2. Only use the huh TUI prompt when both stdin and stdout are terminals.
   When stdout is redirected (but stdin is a terminal), fall back to
   term.ReadPassword to avoid leaking TUI escape sequences into the
   redirected output. The prompt is written to stderr in this case.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@roborev-ci
Copy link
Copy Markdown

roborev-ci bot commented Mar 23, 2026

roborev: Combined Review (a61c385)

Verdict: The PR successfully adds masked IMAP password entry and piped-stdin support, but requires a fix for Cygwin terminal compatibility when stdout is redirected.

Medium

  • Location: cmd/msgvault/cmd/addimap.go
  • Problem
    :     The case stdinTTY: fallback treats Cygwin terminals the same as real OS terminals and then calls term.ReadPassword(int(os.Stdin.Fd())). golang.org/x/term.ReadPassword does not reliably work on Cygwin/mintty PTYs, so
    add-imap can still fail when stdin is a Cygwin terminal and stdout is redirected.
  • Fix: Restrict term.ReadPassword to true native terminals only, and handle the Cygwin + redirected-stdout path with a TTY-backed prompt implementation that does not depend on x/ term.ReadPassword on the Cygwin PTY.

Synthesized from 3 reviews (agents: codex, gemini | types: default, security)

@wesm @claude
fix: restrict term.ReadPassword to native terminals, update nix vendo…
05bf91c 
…rHash

Address review finding: term.ReadPassword doesn't work on Cygwin/mintty
PTYs because it uses Windows console APIs. Split TTY detection into
native vs Cygwin:

- Native terminal + stdout redirected: term.ReadPassword (works fine)
- Cygwin terminal + stdout redirected: huh with output on stderr
  (avoids both the Cygwin PTY issue and TUI escape sequence leakage)

Also refactor readPasswordInteractive to accept an io.Writer so the
Form can direct TUI output to either stdout or stderr.

Update nix flake vendorHash for new huh dependency.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@roborev-ci
Copy link
Copy Markdown

roborev-ci bot commented Mar 23, 2026

roborev: Combined Review (05bf91c)

Summary Verdict: All
agents agree the code is clean, successfully improving IMAP password entry handling without introducing any security issues.

No issues found.

Synthesized from 3 reviews (agents: codex, gemini | types: default, security)

@wesm @claude
fix: always render password prompt to stderr, check stderr TTY status
11812cc 
Address two review findings:

1. Check stderr (not stdout) TTY status for the huh prompt, since
   the prompt is UI chrome and should never go to stdout. This means
   stdout redirection never triggers the fallback path at all.

2. Handle Cygwin PTY with stderr redirected: fail with a clear error
   message since neither term.ReadPassword (Cygwin-incompatible) nor
   huh (needs TTY for output) can work in that configuration.

The decision matrix is now:
- stdin TTY + stderr TTY → huh on stderr (asterisk masking)
- native stdin TTY + stderr redirected → term.ReadPassword
- Cygwin stdin TTY + stderr redirected → clear error
- stdin not TTY → pipe read

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@roborev-ci
Copy link
Copy Markdown

roborev-ci bot commented Mar 23, 2026

roborev: Combined Review (11812cc)

Verdict: The
changes introduce useful piped-stdin and masked password support, but require a fix for prompt visibility when stderr is redirected.

Medium

  • Location: cmd/msgvault/cmd/addimap.go:86
  • Problem: In the case isatty.IsTerminal(os .Stdin.Fd()) branch, the prompt is always written to stderr even though this branch is specifically taken when stderr is not a TTY. If a user runs the command with stderr redirected but stdout still attached to the terminal, term.ReadPassword will block for
    input with no visible prompt.
  • Fix: Write the prompt to whichever stream is actually attached to a terminal (stderr when it is a TTY, otherwise stdout if that is a TTY), or fail fast when no visible terminal output is available.

Synthesized from 3 reviews (agents: codex, gemini | types: default, security)

@wesm @claude
fix: fall back to stdout when stderr is redirected for password prompt
466c001 
Prefer stderr for the huh prompt (keeps stdout clean), but fall back
to stdout when stderr is redirected (e.g. 2>errors.log). This avoids
the prompt disappearing into a log file while the terminal appears to
hang.

The term.ReadPassword and Cygwin error paths only trigger when neither
output stream is a TTY.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@roborev-ci
Copy link
Copy Markdown

roborev-ci bot commented Mar 23, 2026

roborev: Combined Review (466c001)

Summary Verdict: The
PR successfully adds masked interactive IMAP password entry and piped-stdin support, but requires a fix for a potential hanging issue when output streams are redirected.

Medium

  • Location: cmd/msgvault/cmd/addimap.go:100 (case isatty.IsTerminal( os.Stdin.Fd()): branch)
  • Problem: When stdin is a native TTY but both stdout and stderr are redirected, this branch still calls term.ReadPassword after writing the prompt to redirected stderr. The command waits for hidden input with no visible prompt and appears to
    hang.
  • Fix: If no output TTY is available, return a clear error instead of prompting invisibly, or explicitly open the controlling terminal for prompt output.

Synthesized from 3 reviews (agents: codex, gemini | types: default, security)

@wesm @claude
refactor: extract password strategy into testable helper, remove x/term
b61afcf 
Address two review findings:

1. Extract choosePasswordStrategy() with table-driven tests covering
   all stdin/stdout/stderr TTY combinations (native, Cygwin, piped).
   This prevents the regressions that occurred across prior commits.

2. When both stdout and stderr are redirected, fail with a clear
   error instead of prompting invisibly. This replaces the
   term.ReadPassword path, which allowed the golang.org/x/term
   direct dependency to be removed.

The strategy is now a pure function of four booleans, fully tested:
- stdin TTY + stderr TTY → huh on stderr
- stdin TTY + stdout TTY (stderr redirected) → huh on stdout
- stdin TTY + no output TTY → clear error
- stdin not TTY → pipe read

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@roborev-ci
Copy link
Copy Markdown

roborev-ci bot commented Mar 23, 2026

roborev: Combined Review (b61afcf)

Summary Verdict: Updates to the add-imap command password handling look good; no medium or higher severity issues were found.

Synthesized from 3 reviews (agents: codex, gemini | types: default, security)

@wesm wesm changed the title fix: use masked password prompt for IMAP accounts fix: masked password prompt with asterisk echo for IMAP accounts Mar 23, 2026
@wesm @claude
fix: update nix vendorHash after removing x/term
4b080aa 
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@roborev-ci
Copy link
Copy Markdown

roborev-ci bot commented Mar 23, 2026

roborev: Combined Review (4b080aa)

Summary Verdict: All agents agree the code is clean.

No issues of Medium
severity or higher were found across the reviews.

Synthesized from 3 reviews (agents: codex, gemini | types: default, security)

@wesm @claude
add Windows dev build script
c965af7 
Usage: powershell -File scripts/build.ps1

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@roborev-ci
Copy link
Copy Markdown

roborev-ci bot commented Mar 23, 2026

roborev: Combined Review (c965af7)

Summary: The changes safely replace password reads
with a terminal-aware prompt and piped-stdin fallback, improving compatibility and scripting support.

All reviewers agree the code is clean; no medium, high, or critical issues were found.

Synthesized from 3 reviews (agents: codex, gemini | types: default, security)

@wesm @claude
fix: add msgvault.exe to .gitignore and Makefile clean target
b7f2808 
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@roborev-ci
Copy link
Copy Markdown

roborev-ci bot commented Mar 23, 2026

roborev: Combined Review (b7f2808)

Verdict: The PR is functionally clean but introduces a medium
-severity security issue in the command help text that needs to be addressed.

Medium

  • cmd/msgvault/cmd/addimap.go:69

    The new help text recommends echo "password" | msgvault add-imap ... for scripted use. That embeds the IMAP password directly in the command line, which commonly exposes it through shell history, terminal scrollback, CI/script logs, and in some environments process inspection. This contradicts the command’
    s own security rationale for avoiding a password flag.
    Fix: Remove the literal-secret example and replace it with guidance that reads the password without placing it on the command line, such as shell-specific secure prompt examples (read -s / Read-Host) or piping from a secret manager / protected file descriptor
    instead of a hardcoded literal.

Synthesized from 3 reviews (agents: codex, gemini | types: default, security)

@wesm @claude
fix: replace plaintext password example with secure piping guidance
de6f098 
The help text showed `echo "password" | msgvault add-imap ...` which
exposes the password in shell history and process listings. Replace
with `read -s` example that avoids this.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@roborev-ci
Copy link
Copy Markdown

roborev-ci bot commented Mar 23, 2026

roborev: Combined Review (de6f098)

Summary Verdict: All agents agree the code is clean; no Medium, High, or Critical severity issues
were identified.

Synthesized from 3 reviews (agents: codex, gemini | types: default, security)

@wesm wesm merged commit d2d538f into main Mar 24, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

CLI imap password prompt will not accept any console input in windows

1 participant

@wesm