Quality sweep: tighten ssh argument handling and clear batched review findings

cmd/et/flags.go

@@ -131,11 +131,19 @@ func parseArgs(args []string, stderr io.Writer) (*options, error) {
 
 // parseDestination parses [user@]host[:port]. IPv6 literals need brackets to
 // carry a port ("[::1]:2022"); a bare IPv6 literal ("::1") has no port. A
-// user or host beginning with "-" is rejected, since it would otherwise be
-// read as an option by ssh -G in resolveHost. A ":" with nothing after it is
-// rejected rather than treated as "no port".
+// user or host beginning with "-" is rejected as a usage error: bootstrap
+// refuses it anyway, since ssh substitutes the host and user into a
+// ProxyCommand or Match exec (MEASURED against OpenSSH_10.0p2 on 2026-09-26,
+// see bootstrap's shellMeta). A ":" with nothing after it is rejected rather
+// than treated as "no port", and so is an ssh:// URI.
 func parseDestination(s string) (destination, error) {
 	var d destination
+	// An ssh:// URI would be split at its last '@' into a user such as
+	// "ssh://bob" or taken whole as a host, and its port names sshd's port
+	// where et's host:port names etserver's; refuse it rather than guess.
+	if strings.Contains(s, "://") {
+		return d, fmt.Errorf("ssh:// URIs are not supported, use [user@]host[:port]: %q", s)
+	}
 	if before, after, found := strings.CutLast(s, "@"); found {
 		if before == "" {
 			return d, fmt.Errorf("empty user in destination %q", s)
@@ -178,9 +186,8 @@ func parseDestination(s string) (destination, error) {
 	if d.Host == "" {
 		return d, fmt.Errorf("empty host in destination")
 	}
-	// A host beginning with "-" would otherwise reach "ssh -G <target>" (see
-	// resolveHost in resolve.go) as an option rather than as an argument;
-	// reject it here so the caller gets a usage error instead.
+	// A host beginning with "-" is refused by bootstrap's validation; reject
+	// it here so the caller gets a usage error instead.
 	if strings.HasPrefix(d.Host, "-") {
 		return d, fmt.Errorf("host %q looks like an option in destination %q", d.Host, s)
 	}

cmd/et/flags_test.go

@@ -44,6 +44,10 @@ func TestParseArgsDestination(t *testing.T) {
 		{name: "host looks like an option", args: []string{"--", "-evil"}, wantErr: true},
 		{name: "user looks like an option", args: []string{"--", "-evil@box"}, wantErr: true},
 		{name: "-u looks like an option", args: []string{"-u", "-oProxyCommand=x", "box"}, wantErr: true},
+		{name: "user holding @", args: []string{"me@corp.example@box"}, want: destination{User: "me@corp.example", Host: "box", Port: 2022}},
+		{name: "ssh URI with user", args: []string{"ssh://bob@h1"}, wantErr: true},
+		{name: "user and ssh URI host", args: []string{"bob@ssh://h1:2222"}, wantErr: true},
+		{name: "ssh URI", args: []string{"ssh://h1"}, wantErr: true},
 		{name: "trailing colon", args: []string{"box:"}, wantErr: true},
 		{name: "bracket trailing colon", args: []string{"[::1]:"}, wantErr: true},
 	}

cmd/et/resolve.go

@@ -20,20 +20,20 @@ const (
 // resolveHost returns the host name OpenSSH would connect to for this
 // destination, so an ssh_config alias ("Host box / HostName 10.0.0.5") works
 // for the etserver TCP connection too. "ssh -G" prints the effective client
-// configuration without connecting. Each of opts is passed as "-o<opt>", the
-// same way bootstrap passes it to the ssh that starts etterminal, so an
-// option such as HostName resolves here too. On any failure it returns host
-// unchanged.
+// configuration without connecting. The arguments have the shape bootstrap
+// gives the ssh that starts etterminal: each of opts as "-o<opt>", the user
+// as "-l <user>", and "--" before the host, so an option such as HostName
+// and a user name holding '@' resolve as they do there.
+// On any failure it returns host unchanged.
 func resolveHost(ctx context.Context, sshPath, user, host string, opts []string) string {
-	target := host
-	if user != "" {
-		target = user + "@" + host
-	}
-	args := make([]string, 0, len(opts)+2)
+	args := make([]string, 0, len(opts)+5)
 	for _, opt := range opts {
 		args = append(args, "-o"+opt)
 	}
-	args = append(args, "-G", target)
+	if user != "" {
+		args = append(args, "-l", user)
+	}
+	args = append(args, "-G", "--", host)
 	ctx, cancel := context.WithTimeout(ctx, resolveTimeout)
 	defer cancel()
 	cmd := exec.CommandContext(ctx, sshPath, args...)

cmd/et/resolve_test.go

@@ -31,10 +31,11 @@ func fakeSSH(t *testing.T, body string) string {
 	return path
 }
 
-// TestResolveHostPassesOptions pins the exact ssh -G argument list: each
-// option is its own "-o<opt>" word before -G, as bootstrap passes it to the
-// ssh that starts etterminal. The fake prints its arguments joined by "|" as
-// the hostname, so an option containing a space stays one word.
+// TestResolveHostPassesOptions pins the exact ssh -G argument list, which has
+// the shape bootstrap gives the ssh that starts etterminal: each option as
+// its own "-o<opt>" word, the user as "-l <user>", then "--" before the host.
+// The fake prints its arguments joined by "|" as the hostname, so an option
+// containing a space stays one word.
 func TestResolveHostPassesOptions(t *testing.T) {
 	ssh := fakeSSH(t, `printf 'user x\nhostname '; printf '%s|' "$@"; printf '\n'`)
 	tests := []struct {
@@ -43,13 +44,14 @@ func TestResolveHostPassesOptions(t *testing.T) {
 		opts []string
 		want string
 	}{
-		{name: "no options", want: "-G|box|"},
-		{name: "user", user: "me", want: "-G|me@box|"},
+		{name: "no options", want: "-G|--|box|"},
+		{name: "user", user: "me", want: "-l|me|-G|--|box|"},
+		{name: "user with at", user: "me@corp.example", want: "-l|me@corp.example|-G|--|box|"},
 		{
 			name: "options",
 			user: "me",
 			opts: []string{"HostName=10.0.0.5", "ProxyCommand=nc a b"},
-			want: "-oHostName=10.0.0.5|-oProxyCommand=nc a b|-G|me@box|",
+			want: "-oHostName=10.0.0.5|-oProxyCommand=nc a b|-l|me|-G|--|box|",
 		},
 	}
 	for _, tt := range tests {

internal/bootstrap/bootstrap.go

@@ -12,6 +12,7 @@ import (
 	"os/exec"
 	"strings"
 	"time"
+	"unicode/utf8"
 )
 
 var (
@@ -24,7 +25,9 @@ var (
 const (
 	// maxOutput caps how much ssh stdout is kept. Shell startup noise before
 	// the marker is normally a few lines, far below the cap; a marker that
-	// arrives after the cap is dropped and the start fails.
+	// arrives after the cap is dropped and the start fails, with an error
+	// that says output was dropped. Keep it a whole number of MiB: that
+	// error states it in MiB.
 	maxOutput = 1 << 20
 	// excerptLen caps the output excerpt quoted in an error.
 	excerptLen = 512
@@ -38,7 +41,7 @@ const (
 // usable: Destination is required.
 type Config struct {
 	Destination  string       // ssh destination as typed; ssh_config aliases work
-	User         string       // optional; becomes user@destination
+	User         string       // optional; passed to ssh as -l, so it may contain '@'
 	TerminalPath string       // default "etterminal"; must match [A-Za-z0-9._/~-]+
 	Term         string       // default "xterm-256color"; must match [A-Za-z0-9.+-]+ (no underscore)
 	SSHOptions   []string     // each passed as its own "-o<opt>" argument
@@ -77,21 +80,7 @@ func Run(ctx context.Context, cfg Config) (Credentials, error) {
 	}
 
 	id, passkey := placeholder()
-	cmd := exec.CommandContext(ctx, sshPath, sshArgs(&cfg, remoteCommand(id, passkey, cfg.Term, cfg.TerminalPath))...)
-	cmd.Stdin = os.Stdin
-	cmd.Stderr = os.Stderr
-	out := &cappedBuffer{max: maxOutput}
-	cmd.Stdout = out
-	cmd.Cancel = func() error {
-		// Interrupt first so ssh can restore the terminal; Windows cannot
-		// deliver os.Interrupt to another process, so kill there.
-		if err := cmd.Process.Signal(os.Interrupt); err != nil {
-			return cmd.Process.Kill()
-		}
-		return nil
-	}
-	cmd.WaitDelay = waitDelay
-
+	cmd, out := newCommand(ctx, sshPath, sshArgs(&cfg, remoteCommand(id, passkey, cfg.Term, cfg.TerminalPath)))
 	runErr := cmd.Run()
 	creds, parseErr := parseCredentials(out.Bytes())
 	// A cancelled ctx wins even over credentials that already arrived: the
@@ -103,8 +92,15 @@ func Run(ctx context.Context, cfg Config) (Credentials, error) {
 		}
 		return creds, nil
 	}
-	if ctx.Err() != nil {
-		return Credentials{}, fmt.Errorf("bootstrap: ssh interrupted: %w", context.Cause(ctx))
+	if ctxErr := ctx.Err(); ctxErr != nil {
+		// A caller can match context.Canceled or DeadlineExceeded as well
+		// as a cause it set. A cause that already wraps the context error
+		// (or is it) is reported alone, so the text names it once.
+		cause := context.Cause(ctx)
+		if errors.Is(cause, ctxErr) {
+			return Credentials{}, fmt.Errorf("bootstrap: ssh interrupted: %w", cause)
+		}
+		return Credentials{}, fmt.Errorf("bootstrap: ssh interrupted: %w: %w", ctxErr, cause)
 	}
 	exitErr, isExit := errors.AsType[*exec.ExitError](runErr)
 	// ErrWaitDelay means ssh exited 0 but a descendant kept its stdout open
@@ -121,7 +117,28 @@ func Run(ctx context.Context, cfg Config) (Credentials, error) {
 	if containsPiece(shown, passkey) {
 		shown = []byte(withheldOutput)
 	}
-	return Credentials{}, describeFailure(exitErr, parseErr, shown)
+	return Credentials{}, describeFailure(exitErr, parseErr, shown, out.dropped > 0)
+}
+
+// newCommand prepares the ssh command. Its stdin and stderr are the process's
+// own, so what ssh asks or reports on them reaches the user; its stdout is
+// captured in the returned buffer.
+func newCommand(ctx context.Context, sshPath string, args []string) (*exec.Cmd, *cappedBuffer) {
+	cmd := exec.CommandContext(ctx, sshPath, args...)
+	cmd.Stdin = os.Stdin
+	cmd.Stderr = os.Stderr
+	out := &cappedBuffer{max: maxOutput}
+	cmd.Stdout = out
+	cmd.Cancel = func() error {
+		// Interrupt first so ssh can restore the terminal; Windows cannot
+		// deliver os.Interrupt to another process, so kill there.
+		if err := cmd.Process.Signal(os.Interrupt); err != nil {
+			return cmd.Process.Kill()
+		}
+		return nil
+	}
+	cmd.WaitDelay = waitDelay
+	return cmd, out
 }
 
 const (
@@ -143,54 +160,67 @@ func containsPiece(out []byte, secret string) bool {
 }
 
 // describeFailure builds the single error the user sees when ssh finished
-// without valid credentials: the parse problem, ssh's exit status, a hint for
-// the common exit statuses, and a trimmed excerpt of stdout.
-func describeFailure(exitErr *exec.ExitError, parseErr error, out []byte) error {
+// without valid credentials: the parse problem, ssh's exit status or the
+// signal that killed it, a hint for the common exit statuses, a note when
+// output past maxOutput was dropped, and a trimmed excerpt of stdout.
+func describeFailure(exitErr *exec.ExitError, parseErr error, out []byte, overflowed bool) error {
 	var b strings.Builder
 	b.WriteString("ssh ")
-	code := 0
+	code, sig, killed := 0, "", false
 	if exitErr != nil {
-		code = exitErr.ExitCode()
-		fmt.Fprintf(&b, "exited with status %d", code)
+		code = exitErr.ExitCode() // -1 when a signal ended ssh
+		sig, killed = killedBy(exitErr)
+	}
+	if killed {
+		fmt.Fprintf(&b, "was killed by signal %s", sig)
 	} else {
-		b.WriteString("exited with status 0")
+		fmt.Fprintf(&b, "exited with status %d", code)
 	}
 	switch code {
 	case 127:
 		b.WriteString(": etterminal was not found on the server; install Eternal Terminal there or pass --terminal-path")
 	case 255:
 		b.WriteString(": ssh could not connect or authenticate; check that plain ssh to this host works")
 	}
+	if overflowed {
+		fmt.Fprintf(&b, "; output exceeded %d MiB; the credentials may have been cut off", maxOutput>>20)
+	}
 	if ex := excerpt(out); ex != "" {
 		fmt.Fprintf(&b, "; output: %q", ex)
 	}
 	return fmt.Errorf("%w: %s", parseErr, b.String())
 }
 
-// excerpt returns the last excerptLen bytes of the trimmed output before the
-// first marker, prefixed with "..." when it was cut, so no part of a (possibly
-// malformed) passkey is ever quoted.
+// excerpt returns at most the last excerptLen bytes of the trimmed output
+// before the first marker, prefixed with "..." when it was cut, so no part of
+// a (possibly malformed) passkey is ever quoted. A cut never splits a UTF-8
+// sequence: the excerpt then starts at the next rune.
 func excerpt(out []byte) string {
 	before, _, _ := bytes.Cut(out, []byte(marker))
-	s := strings.TrimSpace(string(before))
-	if len(s) > excerptLen {
-		s = "..." + s[len(s)-excerptLen:]
+	b := bytes.TrimSpace(before)
+	if len(b) <= excerptLen {
+		return string(b)
 	}
-	return s
+	b = b[len(b)-excerptLen:]
+	for len(b) > 0 && !utf8.RuneStart(b[0]) {
+		b = b[1:]
+	}
+	return "..." + string(b)
 }
 
-// cappedBuffer keeps the first max bytes written to it and silently drops the
-// rest, so a chatty login shell cannot grow memory without bound. It never
-// returns an error, so ssh never sees a broken pipe.
+// cappedBuffer keeps the first max bytes written to it and drops the rest,
+// counting them in dropped, so a chatty login shell cannot grow memory
+// without bound. It never returns an error, so ssh never sees a broken pipe.
 type cappedBuffer struct {
-	buf bytes.Buffer
-	max int
+	buf     bytes.Buffer
+	max     int
+	dropped int // bytes written past max and discarded
 }
 
 func (c *cappedBuffer) Write(p []byte) (int, error) {
-	if room := c.max - c.buf.Len(); room > 0 {
-		c.buf.Write(p[:min(len(p), room)])
-	}
+	kept := min(len(p), max(c.max-c.buf.Len(), 0))
+	c.buf.Write(p[:kept])
+	c.dropped += len(p) - kept
 	return len(p), nil
 }