Skip to content

feat(pass): prompt for the secret when STDIN is a terminal - #673

Open
joe0BAB wants to merge 2 commits into
mainfrom
feat/set-hidden
Open

joe0BAB wants to merge 2 commits into
mainfrom
feat/set-hidden

Conversation

@joe0BAB

@joe0BAB joe0BAB commented Oct 6, 2026 •

Copy link
Copy Markdown
Collaborator

docker pass set <id> used to block on STDIN whenever no value was given, echoing whatever the user typed. It now detects an interactive terminal and shows a masked prompt instead, so the value neither lands in shell history nor on screen, and nobody has to pipe it from a file. Pipes and redirects still read STDIN to the end as before.

The prompt is a bubbletea v2 program around bubbles' textinput in password echo mode. Enter submits, an empty Enter is an error, and Ctrl-C, which raw mode delivers as a key press rather than a signal, is reported as context.Canceled so the root exits 130 silently. Pasted text keeps a trailing newline trimmed; a value spanning several lines is rejected with a pointer to STDIN, because textinput would otherwise replace the newlines with spaces and store a mangled secret. The renderer erases its frame on exit, so the masked line is printed again afterwards, as a plain prompt would have left it.

Cobra hands the subcommands docker's stream wrappers, which hide the terminal file behind File(); unwrapFile reaches it so bubbletea can switch the terminal to raw mode and size its output.

bubbletea v2 requires Go 1.26, which moves go.work and the module to 1.26.0.

Sample rendering:

$ docker pass set foo
Enter secret for foo: ******

@joe0BAB
joe0BAB force-pushed the feat/set-hidden branch 2 times, most recently from fa714e7 to 0f195db Compare October 6, 2026 10:23
`docker pass set <id>` used to block on STDIN whenever no value was
given, echoing whatever the user typed. It now detects an interactive
terminal and shows a masked prompt instead, so the value neither lands
in shell history nor on screen, and nobody has to pipe it from a file.
Pipes and redirects still read STDIN to the end as before.

The prompt is a bubbletea v2 program around bubbles' textinput in
password echo mode. Enter submits, an empty Enter is an error, and
Ctrl-C, which raw mode delivers as a key press rather than a signal,
is reported as context.Canceled so the root exits 130 silently. Pasted
text keeps a trailing newline trimmed; a value spanning several lines
is rejected with a pointer to STDIN, because textinput would otherwise
replace the newlines with spaces and store a mangled secret. The
renderer erases its frame on exit, so the masked line is printed again
afterwards, as a plain prompt would have left it.

Cobra hands the subcommands docker's stream wrappers, which hide the
terminal file behind File(); unwrapFile reaches it so bubbletea can
switch the terminal to raw mode and size its output.

bubbletea v2 requires Go 1.26, which moves go.work and the module to
1.26.0.

Signed-off-by: Johannes Großmann <grossmann.johannes@t-online.de>
@joe0BAB
joe0BAB marked this pull request as ready for review October 6, 2026 10:47

@docker-agent docker-agent left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Review incomplete

chunk 1: The diff chunk contains only newly vendored third-party libraries (go-colorful, cancelreader). Application-layer files (prompt.go, set.go, command_test.go) were read from disk. Two out-of-scope findings identified (in_diff: false). No high-severity bugs found.
chunk 2: This chunk adds vendored libraries (rivo/uniseg gen files, xo/terminfo, and part of other libraries). Two medium-severity bugs found in newly vendored xo/terminfo parametizer.
chunk 3: Drafter did not complete (review_complete: false — truncated at chunk boundary for golang.org/x/sync/errgroup).

Findings so far: 🟡 NEEDS ATTENTION

Comment on lines +235 to +253
for {
p.pos++
ch, err = p.peek()
if err != nil {
return nil
}
f = append(f, ch)
switch ch {
case 'o', 'd', 'x', 'X':
fmt.Fprintf(p.buf, string(f), p.s.popInt())
break
case 's':
fmt.Fprintf(p.buf, string(f), p.s.popString())
break
case 'c':
fmt.Fprintf(p.buf, string(f), p.s.popByte())
break
}
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[medium] scanFormatFn: break inside switch does not exit the for loop — infinite loop on format-width modifiers

In Go, a break inside a switch only terminates the innermost switch, not any enclosing for loop. The loop in scanFormatFn never exits after matching a format-type character (o, d, x, X, s, c): it calls fmt.Fprintf, then break exits only the switch, and the for loop immediately continues reading the next character. The two lines after the loop (p.pos++; return p.scanTextFn) are unreachable dead code.

Trigger: any terminfo capability string containing a format specifier with an explicit width or precision modifier, such as %:-9.9d. On a matching character the function loops until p.peek() returns an error (EOF), returning nil from stateFn and aborting terminal capability evaluation.

Impact: for the bubbletea password-prompt use case the common sequences (cursor movement, clearing, raw-mode setup) use simple %p1%d-style parameters and do not exercise this path, so the defect is unlikely to surface in typical usage. However, on terminals whose terminfo databases use width-formatted capabilities, the renderer silently fails and may leave the terminal in a broken state.

Fix: replace break with a return p.scanTextFn (or use a labelled break on the outer loop):

Suggested change
for {
p.pos++
ch, err = p.peek()
if err != nil {
return nil
}
f = append(f, ch)
switch ch {
case 'o', 'd', 'x', 'X':
fmt.Fprintf(p.buf, string(f), p.s.popInt())
break
case 's':
fmt.Fprintf(p.buf, string(f), p.s.popString())
break
case 'c':
fmt.Fprintf(p.buf, string(f), p.s.popByte())
break
}
}
for {
p.pos++
ch, err = p.peek()
if err != nil {
return nil
}
f = append(f, ch)
switch ch {
case 'o', 'd', 'x', 'X':
fmt.Fprintf(p.buf, string(f), p.s.popInt())
return p.scanTextFn
case 's':
fmt.Fprintf(p.buf, string(f), p.s.popString())
return p.scanTextFn
case 'c':
fmt.Fprintf(p.buf, string(f), p.s.popByte())
return p.scanTextFn
}
}
Confidence Score
🟢 strong 100/100

Comment on lines +289 to +305
func (p *parametizer) getDsVarFn() stateFn {
ch, err := p.peek()
if err != nil {
return nil
}
var a byte
if ch >= 'A' && ch <= 'Z' {
a = 'A'
} else if ch >= 'a' && ch <= 'z' {
a = 'a'
}
staticVars.Lock()
p.s.push(staticVars.vars[int(ch-a)])
staticVars.Unlock()
p.pos++
return p.scanTextFn
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[medium] getDsVarFn: lowercase dynamic variables are read from staticVars instead of p.vars

setDsVarFn correctly writes lowercase (a–z) per-call dynamic variables to p.vars[int(ch-'a')], but getDsVarFn always reads back from staticVars.vars[int(ch-a)], regardless of case. The staticVars mutex is also acquired unnecessarily for a lowercase lookup that should never touch the global map.

Trigger: any terminfo capability that uses the %P / %g terminfo opcodes with a lowercase variable (e.g. %Pa %ga). A store via %P writes to p.vars, but the corresponding %g read returns staticVars.vars[0] (which is nil) instead of the stored value.

Impact: affected capabilities produce incorrect output, typically producing 0 or empty strings where a computed intermediate value was expected. In practice these opcodes appear rarely in standard terminal capability databases, so the password-prompt use case is not likely to trigger this; however any terminal whose database relies on dynamic variables (some custom or extended terminfo entries) will render incorrectly.

Fix: read lowercase variables from p.vars and skip the mutex for that branch:

Suggested change
func (p *parametizer) getDsVarFn() stateFn {
ch, err := p.peek()
if err != nil {
return nil
}
var a byte
if ch >= 'A' && ch <= 'Z' {
a = 'A'
} else if ch >= 'a' && ch <= 'z' {
a = 'a'
}
staticVars.Lock()
p.s.push(staticVars.vars[int(ch-a)])
staticVars.Unlock()
p.pos++
return p.scanTextFn
}
func (p *parametizer) getDsVarFn() stateFn {
ch, err := p.peek()
if err != nil {
return nil
}
if ch >= 'A' && ch <= 'Z' {
staticVars.Lock()
p.s.push(staticVars.vars[int(ch-'A')])
staticVars.Unlock()
} else if ch >= 'a' && ch <= 'z' {
p.s.push(p.vars[int(ch-'a')])
}
p.pos++
return p.scanTextFn
}
Confidence Score
🟢 strong 100/100

Benehiko
Benehiko previously approved these changes Oct 6, 2026
When the terminal does not support bracketed paste, a multi-line paste
reaches the prompt as keystrokes. CR ended the prompt, but keys already
decoded from the same read raced tea.Quit and could land in the value; a
bare LF decoded as ctrl+j and was dropped, so lines concatenated; and bytes
still queued in the tty when the prompt exited were typed into the shell.
textinput added two more deviations: its ctrl+v binding shelled out to
pbpaste or xclip and inserted the clipboard with newlines turned into
spaces, and its sanitizer rewrote tabs to spaces and dropped other control
characters, so the stored secret differed from what was pasted while the
STDIN path stores bytes verbatim.

Replace textinput with a small line editor that follows the canonical-mode
tty: input is stored verbatim; Enter, LF and ctrl+d end the line; Backspace,
ctrl+w and ctrl+u erase; ctrl+v inserts the next key literally; input after
Enter is ignored so the value is exactly the first line; and pending
terminal input is flushed once the program returns (TIOCFLUSH on Darwin,
TCFLSH on Linux, FlushConsoleInputBuffer on Windows). Keys are matched on
code and modifiers with Shift ignored, because the key disambiguation
Bubble Tea requests makes capable terminals report Shift+Enter and ctrl+m
as distinct keys where a tty sees CR for both. A bracketed paste spanning
several lines is still rejected. Pastes larger than the pty input queue
still reach the shell, as they do with sudo and ssh prompts.

Dropping textinput removes github.com/atotto/clipboard from the module.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants