bug: Don't try to handle SIGINT when prompting for passwords (#1498)

cli/cliui/prompt.go

@@ -34,8 +34,6 @@ func Prompt(cmd *cobra.Command, opts PromptOptions) (string, error) {
 		_, _ = fmt.Fprint(cmd.OutOrStdout(), Styles.Placeholder.Render("("+opts.Default+") "))
 	}
 	interrupt := make(chan os.Signal, 1)
-	signal.Notify(interrupt, os.Interrupt)
-	defer signal.Stop(interrupt)
 
 	errCh := make(chan error, 1)
 	lineCh := make(chan string)
@@ -45,8 +43,12 @@ func Prompt(cmd *cobra.Command, opts PromptOptions) (string, error) {
 
 		inFile, isInputFile := cmd.InOrStdin().(*os.File)
 		if opts.Secret && isInputFile && isatty.IsTerminal(inFile.Fd()) {
+			// we don't install a signal handler here because speakeasy has its own
 			line, err = speakeasy.Ask("")
 		} else {
+			signal.Notify(interrupt, os.Interrupt)
+			defer signal.Stop(interrupt)
+
 			reader := bufio.NewReader(cmd.InOrStdin())
 			line, err = reader.ReadString('\n')
 

cli/cliui/prompt_test.go

@@ -2,12 +2,16 @@ package cliui_test
 
 import (
 	"context"
+	"os"
+	"os/exec"
 	"testing"
+	"time"
 
 	"github.com/spf13/cobra"
 	"github.com/stretchr/testify/require"
 
 	"github.com/coder/coder/cli/cliui"
+	"github.com/coder/coder/pty"
 	"github.com/coder/coder/pty/ptytest"
 )
 
@@ -110,3 +114,56 @@ func newPrompt(ptty *ptytest.PTY, opts cliui.PromptOptions) (string, error) {
 	cmd.SetIn(ptty.Input())
 	return value, cmd.ExecuteContext(context.Background())
 }
+
+func TestPasswordTerminalState(t *testing.T) {
+	if os.Getenv("TEST_SUBPROCESS") == "1" {
+		passwordHelper()
+		return
+	}
+	t.Parallel()
+
+	ptty := ptytest.New(t)
+	ptyWithFlags, ok := ptty.PTY.(pty.WithFlags)
+	if !ok {
+		t.Skip("unable to check PTY local echo on this platform")
+	}
+
+	cmd := exec.Command(os.Args[0], "-test.run=TestPasswordTerminalState") //nolint:gosec
+	cmd.Env = append(os.Environ(), "TEST_SUBPROCESS=1")
+	// connect the child process's stdio to the PTY directly, not via a pipe
+	cmd.Stdin = ptty.Input().Reader
+	cmd.Stdout = ptty.Output().Writer
+	cmd.Stderr = os.Stderr
+	err := cmd.Start()
+	require.NoError(t, err)
+	process := cmd.Process
+	defer process.Kill()
+
+	ptty.ExpectMatch("Password: ")
+	time.Sleep(100 * time.Millisecond) // wait for child process to turn off echo and start reading input
+
+	echo, err := ptyWithFlags.EchoEnabled()
+	require.NoError(t, err)
+	require.False(t, echo, "echo is on while reading password")
+
+	err = process.Signal(os.Interrupt)
+	require.NoError(t, err)
+	_, err = process.Wait()
+	require.NoError(t, err)
+
+	echo, err = ptyWithFlags.EchoEnabled()
+	require.NoError(t, err)
+	require.True(t, echo, "echo is off after reading password")
+}
+
+func passwordHelper() {
+	cmd := &cobra.Command{
+		Run: func(cmd *cobra.Command, args []string) {
+			cliui.Prompt(cmd, cliui.PromptOptions{
+				Text:   "Password:",
+				Secret: true,
+			})
+		},
+	}
+	cmd.ExecuteContext(context.Background())
+}

pty/pty.go

@@ -2,6 +2,7 @@ package pty
 
 import (
 	"io"
+	"os"
 )
 
 // PTY is a minimal interface for interacting with a TTY.
@@ -14,7 +15,7 @@ type PTY interface {
 	// uses the output stream for writing.
 	//
 	// The same stream could be read to validate output.
-	Output() io.ReadWriter
+	Output() ReadWriter
 
 	// Input handles TTY input.
 	//
@@ -22,18 +23,38 @@ type PTY interface {
 	// uses the PTY input for reading.
 	//
 	// The same stream would be used to provide user input: pty.Input().Write(...)
-	Input() io.ReadWriter
+	Input() ReadWriter
 
 	// Resize sets the size of the PTY.
 	Resize(height uint16, width uint16) error
 }
 
+// WithFlags represents a PTY whose flags can be inspected, in particular
+// to determine whether local echo is enabled.
+type WithFlags interface {
+	PTY
+
+	// EchoEnabled determines whether local echo is currently enabled for this terminal.
+	EchoEnabled() (bool, error)
+}
+
 // New constructs a new Pty.
 func New() (PTY, error) {
 	return newPty()
 }
 
-type readWriter struct {
+// ReadWriter is an implementation of io.ReadWriter that wraps two separate
-	io.Reader
+// underlying file descriptors, one for reading and one for writing, and allows
-	io.Writer
+// them to be accessed separately.
+type ReadWriter struct {
+	Reader *os.File
+	Writer *os.File
+}
+
+func (rw ReadWriter) Read(p []byte) (int, error) {
+	return rw.Reader.Read(p)
+}
+
+func (rw ReadWriter) Write(p []byte) (int, error) {
+	return rw.Writer.Write(p)
 }

pty/pty_linux.go

@@ -0,0 +1,13 @@
+// go:build linux
+
+package pty
+
+import "golang.org/x/sys/unix"
+
+func (p *otherPty) EchoEnabled() (bool, error) {
+	termios, err := unix.IoctlGetTermios(int(p.pty.Fd()), unix.TCGETS)
+	if err != nil {
+		return false, err
+	}
+	return (termios.Lflag & unix.ECHO) != 0, nil
+}

pty/pty_other.go

@@ -4,7 +4,6 @@
 package pty
 
 import (
-	"io"
 	"os"
 	"sync"
 
@@ -28,15 +27,15 @@ type otherPty struct {
 	pty, tty *os.File
 }
 
-func (p *otherPty) Input() io.ReadWriter {
+func (p *otherPty) Input() ReadWriter {
-	return readWriter{
+	return ReadWriter{
 		Reader: p.tty,
 		Writer: p.pty,
 	}
 }
 
-func (p *otherPty) Output() io.ReadWriter {
+func (p *otherPty) Output() ReadWriter {
-	return readWriter{
+	return ReadWriter{
 		Reader: p.pty,
 		Writer: p.tty,
 	}

pty/pty_windows.go

@@ -4,7 +4,6 @@
 package pty
 
 import (
-	"io"
 	"os"
 	"sync"
 	"unsafe"
@@ -67,15 +66,15 @@ type ptyWindows struct {
 	closed     bool
 }
 
-func (p *ptyWindows) Output() io.ReadWriter {
+func (p *ptyWindows) Output() ReadWriter {
-	return readWriter{
+	return ReadWriter{
 		Reader: p.outputRead,
 		Writer: p.outputWrite,
 	}
 }
 
-func (p *ptyWindows) Input() io.ReadWriter {
+func (p *ptyWindows) Input() ReadWriter {
-	return readWriter{
+	return ReadWriter{
 		Reader: p.inputRead,
 		Writer: p.inputWrite,
 	}