cli: a command reading stdin on a terminal says so !249

merged merged by cmc on 2026-09-04 19:43 UTC · krz/gitbay:stdin-prompt into main

3 files changed, +205 −10

Layout: unified · split

cmd/gitbay/main.go +42 −10

 const serverPath = "gitbay.server_path"
 const stdinMode = "gitbay.stdin_mode"
+// stdinWhat carries the payload's name onto the command tree so the
+// coverage test can assert every stdin-payload command has one; without
+// it the terminal prompt says the unhelpful word "input" (#150).
+const stdinWhat = "gitbay.stdin_what"
 // passOpts describes how one CLI command maps onto the server command.
 type passOpts struct {
 	server      []string // server-side command path
 	needsRepo   bool     // prepend inferred owner/name unless given
 	stdinOK     bool     // wire local stdin through when --file - asks for it
 	alwaysStdin bool     // stdin is the payload, named by no flag: a bare redirect
-	editor      string   // open $EDITOR for a body when none given
+	// stdinWhat names the payload for the prompt shown when stdin is a
-	inferSource bool     // --source defaults to the checked-out branch inside a clone
+	// terminal; stdinSecret hides the input and takes one line, for a
+	// value that should not reach the scrollback.
+	stdinWhat   string
+	stdinSecret bool
+	editor      string // open $EDITOR for a body when none given
+	inferSource bool   // --source defaults to the checked-out branch inside a clone
 }
 // pass builds a passthrough command. Flags are parsed by the server, which
 		Annotations: map[string]string{
 			serverPath: strings.Join(o.server, " "),
 			stdinMode:  o.stdinModeName(),
+			stdinWhat:  o.stdinWhat,
 		},
 		DisableFlagParsing: true,
 		RunE: func(cmd *cobra.Command, args []string) error {
 	}
 	if stdin == nil || isEmptyReader(stdin) {
 		if o.alwaysStdin || (o.stdinOK && usesStdin(args)) {
-			stdin = os.Stdin
+			what := o.stdinWhat
+			if what == "" {
+				what = "input"
+			}
+			r, err := stdinPayload(os.Stdin, what, o.stdinSecret)
+			if err != nil {
+				fmt.Fprintln(os.Stderr, "gitbay:", err)
+				return protocol.ExitFailure
+			}
+			stdin = r
 		}
 	}
 	return runSSH(t, append(o.server, args...), stdin)
 func authCmd() *cobra.Command {
 	keysAdd := pass("add", "register an SSH public key (reads the key from stdin or --file -)",
-		passOpts{server: []string{"keys", "add"}, alwaysStdin: true})
+		passOpts{server: []string{"keys", "add"}, alwaysStdin: true, stdinWhat: "an SSH public key"})
 	// keys add always reads stdin on the server; wire it through directly.
 	keysAdd.RunE = func(cmd *cobra.Command, args []string) error {
 		t, err := resolveTarget()
 		if err != nil {
 			return err
 		}
-		os.Exit(runSSH(t, append([]string{"keys", "add"}, args...), os.Stdin))
+		in, err := stdinPayload(os.Stdin, "an SSH public key", false)
+		if err != nil {
+			return err
+		}
+		os.Exit(runSSH(t, append([]string{"keys", "add"}, args...), in))
 		return nil
 	}
 	pgpAdd := &cobra.Command{
 		Use: "add", Short: "register an OpenPGP public key (armored, on stdin)",
-		Annotations:        map[string]string{serverPath: "pgp add"},
+		Annotations: map[string]string{
+			serverPath: "pgp add",
+			stdinMode:  "always",
+			stdinWhat:  "an armored OpenPGP public key",
+		},
 		DisableFlagParsing: true,
 		RunE: func(cmd *cobra.Command, args []string) error {
 			t, err := resolveTarget()
 			if err != nil {
 				return err
 			}
-			os.Exit(runSSH(t, append([]string{"pgp", "add"}, args...), os.Stdin))
+			in, err := stdinPayload(os.Stdin, "an armored OpenPGP public key", false)
+			if err != nil {
+				return err
+			}
+			os.Exit(runSSH(t, append([]string{"pgp", "add"}, args...), in))
 			return nil
 		},
 	}
 		pass("import-issues", "import GitHub issue/PR history: --from <ghowner/ghrepo> [--token-stdin]",
 			passOpts{server: []string{"repo", "import-issues"}, needsRepo: true, stdinOK: true}),
 		group("deploy-key", "repository-bound CI keys",
-			pass("add", "bind a key: [--rw] < key.pub", passOpts{server: []string{"repo", "deploy-key", "add"}, needsRepo: true, alwaysStdin: true}),
+			pass("add", "bind a key: [--rw] < key.pub", passOpts{server: []string{"repo", "deploy-key", "add"}, needsRepo: true, alwaysStdin: true, stdinWhat: "an SSH public key"}),
 			pass("list", "list deploy keys", passOpts{server: []string{"repo", "deploy-key", "list"}, needsRepo: true}),
 			pass("remove", "remove a deploy key: <fingerprint>", passOpts{server: []string{"repo", "deploy-key", "remove"}, needsRepo: true}),
 		),
 			pass("status", "show check state and what is behind", passOpts{server: []string{"repo", "deps", "status"}, needsRepo: true}),
 		),
 		group("secret", "build secrets (values on stdin, injected into build env)",
-			pass("set", "set a secret: <NAME> (value on stdin)", passOpts{server: []string{"repo", "secret", "set"}, needsRepo: true, alwaysStdin: true}),
+			pass("set", "set a secret: <NAME> (value on stdin)", passOpts{server: []string{"repo", "secret", "set"}, needsRepo: true, alwaysStdin: true, stdinWhat: "the secret value", stdinSecret: true}),
 			pass("list", "list secret names", passOpts{server: []string{"repo", "secret", "list"}, needsRepo: true}),
 			pass("remove", "remove a secret: <NAME>", passOpts{server: []string{"repo", "secret", "remove"}, needsRepo: true}),
 		),
 		pass("show", "show a release with assets: <tag>", passOpts{server: []string{"release", "show"}, needsRepo: true}),
 		pass("delete", "delete a release and its assets: <tag> --yes", passOpts{server: []string{"release", "delete"}, needsRepo: true}),
 		group("asset", "binary assets on a release",
-			pass("add", "upload from stdin: <tag> <filename> < file", passOpts{server: []string{"release", "asset", "add"}, needsRepo: true, alwaysStdin: true}),
+			pass("add", "upload from stdin: <tag> <filename> < file", passOpts{server: []string{"release", "asset", "add"}, needsRepo: true, alwaysStdin: true, stdinWhat: "the asset's bytes"}),
 			pass("get", "download to stdout: <tag> <filename> > file", passOpts{server: []string{"release", "asset", "get"}, needsRepo: true}),
 			pass("remove", "remove an asset: <tag> <filename>", passOpts{server: []string{"release", "asset", "remove"}, needsRepo: true}),
 		),

cmd/gitbay/stdinpayload.go added +47

@@ -0,0 +1,47 @@
	1	package main
	2
	3	import (
	4	"bytes"
	5	"fmt"
	6	"io"
	7	"os"
	8
	9	"golang.org/x/term"
	10	)
	11
	12	// A command whose payload is stdin blocks on a terminal with nothing
	13	// printed, which is indistinguishable from a hung connection — and
	14	// pressing Enter does not end it, because the server reads to EOF. So it
	15	// looks the same before and after you have done the right thing (#150).
	16	//
	17	// The fix belongs here rather than in the daemon: whether stdin is a
	18	// terminal is a fact about the client, and the server cannot see it.
	19
	20	// isTerminal is a variable so tests can drive both paths; a test process
	21	// has no terminal, which is the case that must stay byte-identical.
	22	var isTerminal = func(f *os.File) bool { return term.IsTerminal(int(f.Fd())) }
	23
	24	// stdinPayload returns the reader for a command that takes its payload on
	25	// stdin. Piped or redirected input is passed through untouched — no
	26	// prompt, no added or removed bytes — because a script's `printf %s
	27	// "$TOKEN" \| gitbay ...` must send exactly the token.
	28	//
	29	// what names the thing being read, for the prompt. secret hides the input
	30	// and takes a single line, so a credential never lands in the terminal's
	31	// scrollback and Enter is enough to finish.
	32	func stdinPayload(in *os.File, what string, secret bool) (io.Reader, error) {
	33	if !isTerminal(in) {
	34	return in, nil
	35	}
	36	if secret {
	37	fmt.Fprintf(os.Stderr, "%s (input hidden, Enter when done): ", what)
	38	raw, err := term.ReadPassword(int(in.Fd()))
	39	fmt.Fprintln(os.Stderr)
	40	if err != nil {
	41	return nil, fmt.Errorf("reading %s: %w", what, err)
	42	}
	43	return bytes.NewReader(raw), nil
	44	}
	45	fmt.Fprintf(os.Stderr, "reading %s from stdin — paste it, then press Ctrl-D. (Or pipe it in.)\n", what)
	46	return in, nil
	47	}

cmd/gitbay/stdinpayload_test.go added +116

@@ -0,0 +1,116 @@
	1	package main
	2
	3	import (
	4	"io"
	5	"os"
	6	"sort"
	7	"strings"
	8	"testing"
	9
	10	"github.com/spf13/cobra"
	11	)
	12
	13	// The piped path must stay byte-identical: a script doing
	14	// `printf %s "$TOKEN" \| gitbay repo secret set ...` has to send exactly
	15	// the token, with no prompt written and no byte added or removed. This is
	16	// the half that can silently corrupt a credential, so it is the half with
	17	// a test.
	18	func TestStdinPayloadPipedIsUntouched(t *testing.T) {
	19	defer swapTerminal(func(*os.File) bool { return false })()
	20
	21	r, w, err := os.Pipe()
	22	if err != nil {
	23	t.Fatal(err)
	24	}
	25	const token = "sqp_0123456789abcdef"
	26	go func() { io.WriteString(w, token); w.Close() }()
	27
	28	got, err := stdinPayload(r, "the secret value", true)
	29	if err != nil {
	30	t.Fatal(err)
	31	}
	32	if got != io.Reader(r) {
	33	t.Fatal("piped stdin was replaced rather than passed through")
	34	}
	35	b, _ := io.ReadAll(got)
	36	if string(b) != token {
	37	t.Fatalf("piped payload = %q, want %q", b, token)
	38	}
	39	}
	40
	41	// A non-terminal stdin must not be prompted at, in either mode. The
	42	// prompt goes to stderr, so a test watching stdout would miss this.
	43	func TestStdinPayloadPipedIsSilent(t *testing.T) {
	44	defer swapTerminal(func(*os.File) bool { return false })()
	45
	46	prev := os.Stderr
	47	r, w, err := os.Pipe()
	48	if err != nil {
	49	t.Fatal(err)
	50	}
	51	os.Stderr = w
	52	for _, secret := range []bool{false, true} {
	53	if _, err := stdinPayload(os.Stdin, "the secret value", secret); err != nil {
	54	os.Stderr = prev
	55	t.Fatalf("secret=%v: %v", secret, err)
	56	}
	57	}
	58	w.Close()
	59	os.Stderr = prev
	60	out, _ := io.ReadAll(r)
	61	if len(out) != 0 {
	62	t.Fatalf("piped stdin was prompted at: %q", out)
	63	}
	64	}
	65
	66	// On a terminal the non-secret path says what it wants and how to end it.
	67	// Someone who does not know Ctrl-D is the person this exists for.
	68	func TestStdinPayloadTerminalExplainsItself(t *testing.T) {
	69	defer swapTerminal(func(*os.File) bool { return true })()
	70
	71	prev := os.Stderr
	72	r, w, err := os.Pipe()
	73	if err != nil {
	74	t.Fatal(err)
	75	}
	76	os.Stderr = w
	77	_, err = stdinPayload(os.Stdin, "an SSH public key", false)
	78	w.Close()
	79	os.Stderr = prev
	80	if err != nil {
	81	t.Fatal(err)
	82	}
	83	out, _ := io.ReadAll(r)
	84	got := string(out)
	85	for _, want := range []string{"an SSH public key", "Ctrl-D"} {
	86	if !strings.Contains(got, want) {
	87	t.Errorf("terminal prompt %q lacks %q", got, want)
	88	}
	89	}
	90	}
	91
	92	// Every command whose payload is stdin names it, or the prompt above says
	93	// the unhelpful word "input".
	94	func TestStdinCommandsNameTheirPayload(t *testing.T) {
	95	var missing []string
	96	var walk func(*cobra.Command, string)
	97	walk = func(c *cobra.Command, path string) {
	98	if c.Annotations[stdinMode] == "always" && c.Annotations[stdinWhat] == "" {
	99	missing = append(missing, strings.TrimSpace(path))
	100	}
	101	for _, sub := range c.Commands() {
	102	walk(sub, path+" "+sub.Name())
	103	}
	104	}
	105	walk(newRoot(), "")
	106	sort.Strings(missing)
	107	if len(missing) > 0 {
	108	t.Errorf("stdin-payload commands with no stdinWhat: %s", strings.Join(missing, ", "))
	109	}
	110	}
	111
	112	func swapTerminal(f func(*os.File) bool) func() {
	113	prev := isTerminal
	114	isTerminal = f
	115	return func() { isTerminal = prev }
	116	}