Improve the handling of TUIs when secrets are involved

Signed-off-by: David Gageot <david.gageot@docker.com>

Author: dgageot

AGENTS.md

@@ -211,6 +211,11 @@ The Go toolchain version comes from `go.mod` (`go 1.26.3`). Tests run with
   check to before env composition. The new top-level `secrets:` block in
   `taskfile/secrets.go` runs as the *last* layer of `buildEnv`, so a
   `secrets: [X]` reference still feeds the existing `op://` detection.
+  When the task's stdout *and* stderr are both terminals, gogo passes
+  `--no-masking` to `op run` (`opRunArgs` in `taskfile/shell.go`) so
+  interactive TUIs keep their TTY — op's default masking pipes the
+  streams through itself and breaks anything more elaborate than line
+  output. Non-interactive runs (CI, redirected output) keep masking on.
   New backends plug in by adding a `case strings.HasPrefix(uri, ...)`
   branch in `resolveSecretURI` and a scheme constant in
   `supportedSecretSchemes`.

docs/features/secrets/index.md

@@ -72,3 +72,13 @@ secret "X" has unknown backend in "vault://somewhere/else" (supported: op://)
 ## Requirements
 
 The `op` CLI must be installed and available on PATH. Install it from <https://developer.1password.com/docs/cli/get-started/>.
+
+## Interactive TUIs
+
+By default `op run` masks secret values found in the wrapped command's
+output. To do that it pipes stdout/stderr through itself, which strips the
+TTY and breaks any interactive program (e.g. `docker agent eval`, `fzf`,
+`less`, `vim`). To keep interactive use working, gogo passes `--no-masking`
+to `op run` when both stdout and stderr are attached to a terminal. In
+non-interactive runs (CI, output redirected to a file or pipe) the default
+masking is left on, so secret leaks in logs are still concealed.

taskfile/shell.go

@@ -4,6 +4,7 @@ import (
 	"errors"
 	"fmt"
 	"io"
+	"os"
 	"os/exec"
 	"strings"
 	"sync"
@@ -100,11 +101,41 @@ func (s *defaultShellRunner) shellExecCommand(req ShellCommand) (*exec.Cmd, erro
 		if _, err := s.opPath(); err != nil {
 			return nil, fmt.Errorf("uses op:// secrets but the 1Password CLI (op) is not installed: %w\n\nInstall it from https://developer.1password.com/docs/cli/get-started/", err)
 		}
-		return configuredShellCommand(exec.Command("op", "run", "--", "/bin/sh", "-c", req.Command), req), nil
+		return configuredShellCommand(exec.Command("op", opRunArgs(req)...), req), nil
 	}
 	return configuredShellCommand(exec.Command("/bin/sh", "-c", req.Command), req), nil
 }
 
+// opRunArgs builds the argv passed to `op run`. When the task's stdout and
+// stderr are both attached to a terminal we add `--no-masking` so op leaves
+// the streams alone — by default op pipes them through itself to mask
+// secret values, which strips the TTY and breaks any TUI the command might
+// launch (docker agent eval, fzf, less, vim, …). In non-interactive runs
+// (CI, redirected output) we keep masking so accidental secret leaks into
+// logs are still concealed.
+func opRunArgs(req ShellCommand) []string {
+	args := []string{"run"}
+	if isTerminal(req.Stdout) && isTerminal(req.Stderr) {
+		args = append(args, "--no-masking")
+	}
+	return append(args, "--", "/bin/sh", "-c", req.Command)
+}
+
+// isTerminal reports whether w is a *os.File backed by a character device,
+// i.e. a terminal. Anything else (a pipe, buffer, regular file, or nil)
+// returns false.
+func isTerminal(w io.Writer) bool {
+	f, ok := w.(*os.File)
+	if !ok || f == nil {
+		return false
+	}
+	fi, err := f.Stat()
+	if err != nil {
+		return false
+	}
+	return fi.Mode()&os.ModeCharDevice != 0
+}
+
 func configuredShellCommand(cmd *exec.Cmd, req ShellCommand) *exec.Cmd {
 	cmd.Dir = req.Dir
 	cmd.Env = req.Env

taskfile/shell_test.go

@@ -1,7 +1,10 @@
 package taskfile
 
 import (
+	"bytes"
 	"errors"
+	"os"
+	"slices"
 	"sync"
 	"testing"
 
@@ -51,3 +54,53 @@ func TestDefaultShellRunnerCachesOpLookup(t *testing.T) {
 	}
 	assert.Equal(t, 1, calls, "op lookup must only happen once per runner")
 }
+
+func TestOpRunUsesNoMaskingWhenStdioIsATerminal(t *testing.T) {
+	// `op run` masks secrets in stdout/stderr by piping them through itself,
+	// which strips the TTY and breaks any interactive TUI the command may
+	// launch (docker agent eval, fzf, less, vim, …). When both streams are
+	// terminals we pass --no-masking so op leaves them untouched.
+	tty, err := os.OpenFile("/dev/tty", os.O_RDWR, 0)
+	if err != nil {
+		t.Skipf("no /dev/tty available: %v", err)
+	}
+	t.Cleanup(func() { _ = tty.Close() })
+
+	args := opRunArgs(ShellCommand{
+		Command: "echo hi",
+		Stdout:  tty,
+		Stderr:  tty,
+	})
+	assert.Contains(t, args, "--no-masking")
+	assert.Equal(t, []string{"--", "/bin/sh", "-c", "echo hi"}, args[len(args)-4:])
+}
+
+func TestOpRunKeepsMaskingWhenStdioIsNotATerminal(t *testing.T) {
+	// In CI / scripts (output redirected to a buffer or a file) we keep the
+	// default masking so accidental secret leaks into logs are still
+	// concealed.
+	args := opRunArgs(ShellCommand{
+		Command: "echo hi",
+		Stdout:  &bytes.Buffer{},
+		Stderr:  &bytes.Buffer{},
+	})
+	assert.False(t, slices.Contains(args, "--no-masking"))
+	assert.Equal(t, []string{"run", "--", "/bin/sh", "-c", "echo hi"}, args)
+}
+
+func TestOpRunKeepsMaskingWhenOnlyStderrIsATerminal(t *testing.T) {
+	// Half-redirected: piping just stdout (e.g. `gogo evals | tee log`) is a
+	// non-interactive use, so masking stays on.
+	tty, err := os.OpenFile("/dev/tty", os.O_RDWR, 0)
+	if err != nil {
+		t.Skipf("no /dev/tty available: %v", err)
+	}
+	t.Cleanup(func() { _ = tty.Close() })
+
+	args := opRunArgs(ShellCommand{
+		Command: "echo hi",
+		Stdout:  &bytes.Buffer{},
+		Stderr:  tty,
+	})
+	assert.False(t, slices.Contains(args, "--no-masking"))
+}