FAIL: TestAccessiblePrompter/AuthToken_-_blank_input_returns_error #10916

Activity

1. pdostal

   added

   bugSomething isn't workingSomething isn't working

   on May 2, 2025
2. cliAutomation

   added

   needs-triageneeds to be reviewedneeds to be reviewed

   on May 2, 2025
3. BagToad

   added

   more-info-neededMore info needed from user/contributorMore info needed from user/contributor

   priority-3Affects a small number of users or is largely cosmeticAffects a small number of users or is largely cosmetic

   needs-investigationCLI team needs to investigateCLI team needs to investigate

   on May 2, 2025
4. BagToad commented on May 2, 2025

   

   BagToad

   on May 2, 2025

   Member

   More actions

   👋 Hey @pdostal, thanks for writing this up! ✨ Also, apologies for this friction.

   We did see this flakey test once in our CI too and it has been on our mind to investigate - we'll take a look!

   Could you let me know if this is blocking your build & distribution?
5. BagToad

   removed

   more-info-neededMore info needed from user/contributorMore info needed from user/contributor

   on May 2, 2025
6. andyfeller

   self-assigned this

   on May 2, 2025
7. andyfeller commented on May 2, 2025

   

   andyfeller

   on May 2, 2025

   Contributor

   More actions

   Documenting the cause of the problem and follow ups

   Testing routine race condition

   Credit to @BagToad, @babakks, and @williammartin for putting heads together here as the problem is a race condition where the test is sending the password and auth token back faster than huh accessible prompter can disable echo mode:

   cli/internal/prompter/accessible_prompter_test.go

   Lines 140 to 163 in 7b86830

   	t.Run("Password", func(t *testing.T) {
   	console := newTestVirtualTerminal(t)
   	p := newTestAccessiblePrompter(t, console)
   	dummyPassword := "12345abcdefg"
   	go func() {
   	// Wait for prompt to appear
   	_, err := console.ExpectString("Enter password")
   	require.NoError(t, err)
   	// Enter a number
   	_, err = console.SendLine(dummyPassword)
   	require.NoError(t, err)
   	}()
   	passwordValue, err := p.Password("Enter password")
   	require.NoError(t, err)
   	require.Equal(t, dummyPassword, passwordValue)
   	// Ensure the dummy password is not printed to the screen,
   	// asserting that echo mode is disabled.
   	_, err = console.ExpectString(" \r\n\r\n")
   	require.NoError(t, err)
   	})

   To elaborate, the asynchronous function is already waiting for Enter password: prompt when it is received and sends the dummy password before the prompter can disable echo mode. This results in password being in plaintext.

   Source: field_input.go in charmbracelet/huh

   func (i *Input) runAccessible(w io.Writer, r io.Reader) error {
   	// ...
   
   	switch i.textinput.EchoMode { //nolint:exhaustive
   	case textinput.EchoNormal:
   		// ...
   	default:
   		prompt := styles.Title.
   			PaddingRight(1).
   			Render(cmp.Or(i.title.val, "Password:"))
   		if fd, ok := r.(interface{ Fd() uintptr }); ok {
   			value, err := accessibility.PromptPassword(w, fd.Fd(), prompt, validator)
   			if err != nil {
   				return err //nolint:wrapcheck
   			}
   			i.accessor.Set(value)
   			return nil
   		}
   		return errors.New("password asking needs a tty")
   	}

   We are working on a PR to avoid the flaky tests by introducing a wait

   Ideas for improving virtual terminal testing setup

   Separately, we also realized that both our newTestVirtualTerminal testing harness and [netflix/go-expect] are creating PTYs for testing purposes, which we should follow up in another issue to address:

   Source: console.go in netflix/go-expect:

   // NewConsole returns a new Console with the given options.
   func NewConsole(opts ...ConsoleOpt) (*Console, error) {
   	options := ConsoleOpts{
   		Logger: log.New(ioutil.Discard, "", 0),
   	}
   
   	for _, opt := range opts {
   		if err := opt(&options); err != nil {
   			return nil, err
   		}
   	}
   
   	ptm, pts, err := pty.Open()
   	if err != nil {
   		return nil, err
   	}
   	closers := append(options.Closers, pts, ptm)
   
   	passthroughPipe, err := NewPassthroughPipe(ptm)
   	if err != nil {
   		return nil, err
   	}
   	closers = append(closers, passthroughPipe)
   
   	c := &Console{
   		opts:            options,
   		ptm:             ptm,
   		pts:             pts,
   		passthroughPipe: passthroughPipe,
   		runeReader:      bufio.NewReaderSize(passthroughPipe, utf8.UTFMax),
   		closers:         closers,
   	}
   
   	for _, stdin := range options.Stdins {
   		go func(stdin io.Reader) {
   			_, err := io.Copy(c, stdin)
   			if err != nil {
   				c.Logf("failed to copy stdin: %s", err)
   			}
   		}(stdin)
   	}
   
   	return c, nil
   }

   cli/internal/prompter/accessible_prompter_test.go

   Lines 433 to 457 in 7b86830

   	func newTestVirtualTerminal(t *testing.T) *expect.Console {
   	t.Helper()
   	// Create a PTY and hook up a virtual terminal emulator
   	ptm, pts, err := pty.Open()
   	require.NoError(t, err)
   	term := vt10x.New(vt10x.WithWriter(pts))
   	// Create a console via Expect that allows scripting against the terminal
   	consoleOpts := []expect.ConsoleOpt{
   	expect.WithStdin(ptm),
   	expect.WithStdout(term),
   	expect.WithCloser(ptm, pts),
   	failOnExpectError(t),
   	failOnSendError(t),
   	expect.WithDefaultTimeout(time.Second),
   	}
   	console, err := expect.NewConsole(consoleOpts...)
   	require.NoError(t, err)
   	t.Cleanup(func() { testCloser(t, console) })
   	return console
   	}
8. andyfeller

   assigned

   BagToad

   on May 2, 2025
9. BagToad

   added

   accessibility

   and removed

   needs-triageneeds to be reviewedneeds to be reviewed

   on May 2, 2025
10. BagToad

    closed this as completedin #10918on May 2, 2025