From b3e5c4a1b199ee8dbff56bf62d7c8a26af81c90a Mon Sep 17 00:00:00 2001 From: "Tomi P. Hakala" <7030001+tphakala@users.noreply.github.com> Date: Fri, 25 Sep 2026 17:22:54 +0300 Subject: [PATCH 01/15] feat(bootstrap): add package skeleton and placeholder credentials Config, the sentinel errors, and the throwaway XXX-prefixed id and passkey sent to etterminal, generated from crypto/rand.Text. Verified with go test -race, vet and golangci-lint on linux and windows. --- AGENTS.md | 1 + internal/bootstrap/bootstrap.go | 27 ++++++++++++++++++++ internal/bootstrap/placeholder.go | 26 +++++++++++++++++++ internal/bootstrap/placeholder_test.go | 35 ++++++++++++++++++++++++++ 4 files changed, 89 insertions(+) create mode 100644 internal/bootstrap/bootstrap.go create mode 100644 internal/bootstrap/placeholder.go create mode 100644 internal/bootstrap/placeholder_test.go diff --git a/AGENTS.md b/AGENTS.md index a9e87a1..3f50eea 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -16,6 +16,7 @@ Module path: `github.com/tphakala/et-go`. Go version: see `go.mod`. | Path | Role | | ----------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | `cmd/et` | Entry point and flag parsing. | +| `internal/bootstrap` | Runs the system ssh to start etterminal; validates the config, parses and redacts the IDPASSKEY credentials it prints. | | `internal/etcp` | Reliable, ordered, encrypted packet connection over replaceable TCP links: replay ring, recover exchange, liveness probes, reconnect backoff, write backpressure. | | `internal/etservertest` | Test-only fake etserver (written independently from upstream semantics) and an in-memory `net.Pipe` network with cut and refuse controls, for synctest-driven etcp tests. | | `internal/protocol` | Wire messages generated from upstream's `.proto` files (opaque API), plus `Header`, `Version` and `Packet`. Regenerate with `go generate ./internal/protocol` (needs protoc 3.21.12). | diff --git a/internal/bootstrap/bootstrap.go b/internal/bootstrap/bootstrap.go new file mode 100644 index 0000000..06f988e --- /dev/null +++ b/internal/bootstrap/bootstrap.go @@ -0,0 +1,27 @@ +// Package bootstrap starts etterminal on the server over the system ssh and +// returns the session id and passkey it prints. +package bootstrap + +import ( + "errors" + "log/slog" +) + +var ( + // ErrNoCredentials means ssh finished without printing a valid IDPASSKEY line. + ErrNoCredentials = errors.New("bootstrap: no IDPASSKEY in ssh output") + // ErrInvalidConfig means a Config field failed validation before ssh ran. + ErrInvalidConfig = errors.New("bootstrap: invalid configuration") +) + +// Config describes how to start etterminal over ssh. The zero value is not +// usable: Destination is required. +type Config struct { + Destination string // ssh destination as typed; ssh_config aliases work + User string // optional; becomes user@destination + 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" argument + SSH string // ssh executable; default found with exec.LookPath("ssh") + Logger *slog.Logger // nil discards; receives the no-regeneration warning +} diff --git a/internal/bootstrap/placeholder.go b/internal/bootstrap/placeholder.go new file mode 100644 index 0000000..35bbba5 --- /dev/null +++ b/internal/bootstrap/placeholder.go @@ -0,0 +1,26 @@ +package bootstrap + +import "crypto/rand" + +const ( + // idLen and passkeyLen are the lengths etterminal expects and returns + // (upstream src/terminal/SshSetupHandler.cpp: genRandomAlphaNum(16) and (32)). + idLen = 16 + passkeyLen = 32 + + // regeneratePrefix makes etterminal discard the id and passkey it is sent + // and generate fresh ones (upstream src/terminal/TerminalMain.cpp:114-120). + regeneratePrefix = "XXX" +) + +// placeholder returns the throwaway id and passkey sent to etterminal. They +// are not secrets: with the regeneratePrefix the server replaces both and +// returns the real ones on stdout (spec 4.1 item 7). +// +// rand.Text returns at least 26 base32 characters ([A-Z2-7]), all +// alphanumeric, so slicing to 13 and to 32 (from two calls) is always in range. +func placeholder() (id, passkey string) { + id = regeneratePrefix + rand.Text()[:idLen-len(regeneratePrefix)] + passkey = (rand.Text() + rand.Text())[:passkeyLen] + return id, passkey +} diff --git a/internal/bootstrap/placeholder_test.go b/internal/bootstrap/placeholder_test.go new file mode 100644 index 0000000..aa50660 --- /dev/null +++ b/internal/bootstrap/placeholder_test.go @@ -0,0 +1,35 @@ +package bootstrap + +import ( + "regexp" + "strings" + "testing" +) + +// base32Text matches crypto/rand.Text output: the RFC 4648 base32 alphabet, +// which is alphanumeric, so it is safe inside the remote shell command. +var base32Text = regexp.MustCompile(`^[A-Z2-7]+$`) + +func TestPlaceholder(t *testing.T) { + id, passkey := placeholder() + + if len(id) != idLen { + t.Fatalf("len(id) = %d, want %d (id %q)", len(id), idLen, id) + } + if !strings.HasPrefix(id, regeneratePrefix) { + t.Fatalf("id %q does not start with %q", id, regeneratePrefix) + } + if len(passkey) != passkeyLen { + t.Fatalf("len(passkey) = %d, want %d", len(passkey), passkeyLen) + } + for _, s := range []string{id, passkey} { + if !base32Text.MatchString(s) { + t.Fatalf("%q is not base32 text", s) + } + } + + id2, passkey2 := placeholder() + if id2 == id || passkey2 == passkey { + t.Fatalf("two placeholders are equal: %q/%q and %q/%q", id, passkey, id2, passkey2) + } +} From 4e3c0db48dde8ad12dc34693bae4fa84c74e903b Mon Sep 17 00:00:00 2001 From: "Tomi P. Hakala" <7030001+tphakala@users.noreply.github.com> Date: Fri, 25 Sep 2026 17:24:00 +0300 Subject: [PATCH 02/15] feat(bootstrap): validate config and build the ssh command line TERM and the etterminal path are interpolated into a remote shell command, so both are checked against strict character sets; TERM also excludes '_', which etterminal uses as its stdin field separator (a TERM with '_' aborts etterminal on 7.0.0). A destination or user that ssh would read as an option, and a separate user combined with a destination that already names one, are rejected. Each ssh option is its own -o argument. Verified with the table tests, race detector and golangci-lint on linux and windows. --- internal/bootstrap/command.go | 95 +++++++++++++++++++++++++++ internal/bootstrap/command_test.go | 102 +++++++++++++++++++++++++++++ 2 files changed, 197 insertions(+) create mode 100644 internal/bootstrap/command.go create mode 100644 internal/bootstrap/command_test.go diff --git a/internal/bootstrap/command.go b/internal/bootstrap/command.go new file mode 100644 index 0000000..8744fbe --- /dev/null +++ b/internal/bootstrap/command.go @@ -0,0 +1,95 @@ +package bootstrap + +import ( + "fmt" + "regexp" + "strings" + "unicode" +) + +const ( + defaultTerm = "xterm-256color" + defaultTerminalPath = "etterminal" +) + +// termPattern and terminalPathPattern bound the two values interpolated into +// the remote shell command (spec 4.1 item 8, spec 5.5). Neither admits a quote, +// space or shell metacharacter. +// +// termPattern also excludes '_': etterminal splits its stdin line on '_' and +// aborts unless there are exactly two tokens, so a TERM containing '_' kills +// the session start (upstream src/terminal/TerminalMain.cpp:111-124; MEASURED +// against etserver 7.0.0: "Invalid number of tokens: 3", exit 134). +var ( + termPattern = regexp.MustCompile(`^[A-Za-z0-9.+-]+$`) + terminalPathPattern = regexp.MustCompile(`^[A-Za-z0-9._/~-]+$`) +) + +// applyDefaults fills in empty optional fields. Run calls it on its own copy +// of the caller's Config. +func (cfg *Config) applyDefaults() { + if cfg.Term == "" { + cfg.Term = defaultTerm + } + if cfg.TerminalPath == "" { + cfg.TerminalPath = defaultTerminalPath + } +} + +// validate reports the first problem with cfg, wrapped in ErrInvalidConfig. +// It expects applyDefaults to have been called. +func (cfg *Config) validate() error { + switch { + case cfg.Destination == "": + return fmt.Errorf("%w: destination is empty", ErrInvalidConfig) + case strings.HasPrefix(cfg.Destination, "-"): + // ssh would parse it as an option. + return fmt.Errorf("%w: destination %q starts with '-'", ErrInvalidConfig, cfg.Destination) + case hasSpaceOrControl(cfg.Destination): + return fmt.Errorf("%w: destination %q contains whitespace or control characters", ErrInvalidConfig, cfg.Destination) + case strings.ContainsRune(cfg.User, '@'), strings.HasPrefix(cfg.User, "-"), hasSpaceOrControl(cfg.User): + return fmt.Errorf("%w: user %q is not a valid user name", ErrInvalidConfig, cfg.User) + case cfg.User != "" && strings.ContainsRune(cfg.Destination, '@'): + // ssh would read "user@alice@host" as user "user@alice", not what either value meant. + return fmt.Errorf("%w: user %q given but destination %q already names a user", ErrInvalidConfig, cfg.User, cfg.Destination) + case !termPattern.MatchString(cfg.Term): + return fmt.Errorf("%w: TERM %q must match %s", ErrInvalidConfig, cfg.Term, termPattern) + case !terminalPathPattern.MatchString(cfg.TerminalPath): + return fmt.Errorf("%w: terminal path %q must match %s", ErrInvalidConfig, cfg.TerminalPath, terminalPathPattern) + } + for _, opt := range cfg.SSHOptions { + if opt == "" || hasControl(opt) { + return fmt.Errorf("%w: ssh option %q is empty or contains control characters", ErrInvalidConfig, opt) + } + } + return nil +} + +// remoteCommand is the command ssh runs on the server. It mirrors upstream's +// SshSetupHandler::genCommand: etterminal reads "/_" on stdin. +// Every interpolated value is validated or generated from [A-Za-z0-9]. +func remoteCommand(id, passkey, term, terminalPath string) string { + return "echo '" + id + "/" + passkey + "_" + term + "' | " + terminalPath + " --verbose=0" +} + +// sshArgs builds ssh's argument list: every option as its own "-o" +// argument, then [user@]destination, then the remote command as one argument. +func sshArgs(cfg *Config, remote string) []string { + dest := cfg.Destination + if cfg.User != "" { + dest = cfg.User + "@" + dest + } + args := make([]string, 0, len(cfg.SSHOptions)+2) + for _, opt := range cfg.SSHOptions { + args = append(args, "-o"+opt) + } + return append(args, dest, remote) +} + +func hasSpaceOrControl(s string) bool { + return strings.IndexFunc(s, func(r rune) bool { return unicode.IsSpace(r) || unicode.IsControl(r) }) >= 0 +} + +func hasControl(s string) bool { + return strings.IndexFunc(s, unicode.IsControl) >= 0 +} diff --git a/internal/bootstrap/command_test.go b/internal/bootstrap/command_test.go new file mode 100644 index 0000000..6f5c218 --- /dev/null +++ b/internal/bootstrap/command_test.go @@ -0,0 +1,102 @@ +package bootstrap + +import ( + "errors" + "slices" + "testing" +) + +func TestValidate(t *testing.T) { + valid := Config{Destination: "example.test"} + valid.applyDefaults() + + tests := []struct { + name string + mutate func(*Config) + wantErr bool + }{ + {"defaults", func(*Config) {}, false}, + {"alias with dots and dashes", func(c *Config) { c.Destination = "my-host.lan" }, false}, + {"user", func(c *Config) { c.User = "alice" }, false}, + {"term with plus", func(c *Config) { c.Term = "rxvt-unicode+256color" }, false}, + {"absolute terminal path", func(c *Config) { c.TerminalPath = "/usr/local/bin/etterminal" }, false}, + {"home terminal path", func(c *Config) { c.TerminalPath = "~/bin/etterminal" }, false}, + {"ssh options", func(c *Config) { c.SSHOptions = []string{"BatchMode=yes", "Port 2222"} }, false}, + {"empty destination", func(c *Config) { c.Destination = "" }, true}, + {"destination is an option", func(c *Config) { c.Destination = "-oProxyCommand=evil" }, true}, + {"destination with space", func(c *Config) { c.Destination = "a b" }, true}, + {"user with at", func(c *Config) { c.User = "a@b" }, true}, + {"user is an option", func(c *Config) { c.User = "-x" }, true}, + {"destination with user and no User", func(c *Config) { c.Destination = "alice@host" }, false}, + {"User and a user in destination", func(c *Config) { c.User = "bob"; c.Destination = "alice@host" }, true}, + {"term with underscore", func(c *Config) { c.Term = "xterm_256color" }, true}, + {"term with quote", func(c *Config) { c.Term = "xterm'; rm -rf ~; '" }, true}, + {"term with space", func(c *Config) { c.Term = "xterm 256" }, true}, + {"term with dollar", func(c *Config) { c.Term = "$(id)" }, true}, + {"terminal path with semicolon", func(c *Config) { c.TerminalPath = "etterminal;id" }, true}, + {"terminal path with space", func(c *Config) { c.TerminalPath = "/opt/my et/etterminal" }, true}, + {"empty ssh option", func(c *Config) { c.SSHOptions = []string{""} }, true}, + {"ssh option with newline", func(c *Config) { c.SSHOptions = []string{"A=b\nc"} }, true}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + cfg := valid + cfg.SSHOptions = slices.Clone(valid.SSHOptions) + tt.mutate(&cfg) + err := cfg.validate() + if (err != nil) != tt.wantErr { + t.Fatalf("validate() = %v, wantErr %v", err, tt.wantErr) + } + if err != nil && !errors.Is(err, ErrInvalidConfig) { + t.Fatalf("validate() = %v, want it to wrap ErrInvalidConfig", err) + } + }) + } +} + +func TestApplyDefaults(t *testing.T) { + got := Config{Destination: "h"} + got.applyDefaults() + if got.Term != "xterm-256color" || got.TerminalPath != "etterminal" { + t.Fatalf("applyDefaults() gave Term %q TerminalPath %q", got.Term, got.TerminalPath) + } + kept := Config{Destination: "h", Term: "screen", TerminalPath: "/x/etterminal"} + kept.applyDefaults() + if kept.Term != "screen" || kept.TerminalPath != "/x/etterminal" { + t.Fatalf("applyDefaults() overwrote set fields: Term %q TerminalPath %q", kept.Term, kept.TerminalPath) + } +} + +func TestRemoteCommand(t *testing.T) { + got := remoteCommand("XXXabcdefghijklm", "0123456789abcdef0123456789abcdef", "xterm-256color", "etterminal") + want := "echo 'XXXabcdefghijklm/0123456789abcdef0123456789abcdef_xterm-256color' | etterminal --verbose=0" + if got != want { + t.Fatalf("remoteCommand() =\n%s\nwant\n%s", got, want) + } +} + +func TestSSHArgs(t *testing.T) { + tests := []struct { + name string + cfg Config + want []string + }{ + { + name: "destination only", + cfg: Config{Destination: "host"}, + want: []string{"host", "REMOTE"}, + }, + { + name: "user and options", + cfg: Config{Destination: "host", User: "alice", SSHOptions: []string{"BatchMode=yes", "Port=2222"}}, + want: []string{"-oBatchMode=yes", "-oPort=2222", "alice@host", "REMOTE"}, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := sshArgs(&tt.cfg, "REMOTE"); !slices.Equal(got, tt.want) { + t.Fatalf("sshArgs() = %q, want %q", got, tt.want) + } + }) + } +} From 4a3e4f8cb9bea8ba9792c99b3d9e9f520e06df9b Mon Sep 17 00:00:00 2001 From: "Tomi P. Hakala" <7030001+tphakala@users.noreply.github.com> Date: Fri, 25 Sep 2026 17:24:40 +0300 Subject: [PATCH 03/15] feat(bootstrap): add Credentials with passkey redaction String, GoString and LogValue redact the passkey so it cannot reach fmt output, errors or slog by accident; the id stays visible for debugging. Verified through %v, %+v, %s, %#v, nested structs, Errorf and the slog JSON handler. --- internal/bootstrap/credentials.go | 32 ++++++++++++++++ internal/bootstrap/credentials_test.go | 51 ++++++++++++++++++++++++++ 2 files changed, 83 insertions(+) create mode 100644 internal/bootstrap/credentials.go create mode 100644 internal/bootstrap/credentials_test.go diff --git a/internal/bootstrap/credentials.go b/internal/bootstrap/credentials.go new file mode 100644 index 0000000..606b964 --- /dev/null +++ b/internal/bootstrap/credentials.go @@ -0,0 +1,32 @@ +package bootstrap + +import "log/slog" + +// redacted replaces the passkey wherever Credentials are formatted or logged. +const redacted = "[redacted]" + +// Credentials are the session id and passkey returned by etterminal. String, +// GoString and LogValue redact the passkey, so it cannot leak through fmt or +// slog by accident. The id is not secret and stays visible for debugging. +type Credentials struct { + ID string + Passkey string +} + +// String implements fmt.Stringer, used by %v, %+v and %s. +func (c Credentials) String() string { + return "{ID:" + c.ID + " Passkey:" + redacted + "}" +} + +// GoString implements fmt.GoStringer, used by %#v. +func (c Credentials) GoString() string { + return `bootstrap.Credentials{ID:"` + c.ID + `", Passkey:"` + redacted + `"}` +} + +// LogValue implements slog.LogValuer. +func (c Credentials) LogValue() slog.Value { + return slog.GroupValue( + slog.String("id", c.ID), + slog.String("passkey", redacted), + ) +} diff --git a/internal/bootstrap/credentials_test.go b/internal/bootstrap/credentials_test.go new file mode 100644 index 0000000..38e1a59 --- /dev/null +++ b/internal/bootstrap/credentials_test.go @@ -0,0 +1,51 @@ +package bootstrap + +import ( + "bytes" + "fmt" + "log/slog" + "strings" + "testing" +) + +// testID and testPasskey are well-formed credentials shared by this +// package's tests. +const ( + testID = "abcdEFGH12345678" // 16 alphanumeric + testPasskey = "0123456789abcdefABCDEF0123456789" // 32 alphanumeric +) + +func TestCredentialsRedaction(t *testing.T) { + c := Credentials{ID: testID, Passkey: testPasskey} + type holder struct{ Creds Credentials } + + // Each format carries surrounding text, as real log and error lines do. + formatted := map[string]string{ + "%v": fmt.Sprintf("creds %v", c), + "%+v": fmt.Sprintf("creds %+v", c), + "%s": fmt.Sprintf("creds %s", c), + "%#v": fmt.Sprintf("creds %#v", c), + "pointer %v": fmt.Sprintf("creds %v", &c), + "nested %+v": fmt.Sprintf("creds %+v", holder{c}), + "nested %#v": fmt.Sprintf("creds %#v", holder{c}), + "Sprint": fmt.Sprint("creds ", c), + "Errorf %v": fmt.Errorf("dial with %v", c).Error(), + } + for verb, s := range formatted { + if strings.Contains(s, testPasskey) { + t.Errorf("%s leaks the passkey: %s", verb, s) + } + if !strings.Contains(s, testID) || !strings.Contains(s, redacted) { + t.Errorf("%s = %s, want the id and %q", verb, s, redacted) + } + } + + var buf bytes.Buffer + slog.New(slog.NewJSONHandler(&buf, nil)).Info("got", "credentials", c) + if strings.Contains(buf.String(), testPasskey) { + t.Fatalf("slog JSON leaks the passkey: %s", buf.String()) + } + if !strings.Contains(buf.String(), `"credentials":{"id":"`+testID+`","passkey":"[redacted]"}`) { + t.Fatalf("slog JSON = %s, want a redacted credentials group", buf.String()) + } +} From 4c507835b5c3296b7f6fc2494b0ce381a86170ab Mon Sep 17 00:00:00 2001 From: "Tomi P. Hakala" <7030001+tphakala@users.noreply.github.com> Date: Fri, 25 Sep 2026 17:25:09 +0300 Subject: [PATCH 04/15] feat(bootstrap): parse IDPASSKEY output strictly The first IDPASSKEY marker must be followed by exactly 16 and 32 alphanumeric characters; shell noise before it is ignored and malformed values are rejected without quoting passkey material. Verified with a table test covering noise, CRLF, truncation, wrong lengths and non-alphanumeric input. --- internal/bootstrap/parse.go | 44 ++++++++++++++++++++++++++++ internal/bootstrap/parse_test.go | 49 ++++++++++++++++++++++++++++++++ 2 files changed, 93 insertions(+) create mode 100644 internal/bootstrap/parse.go create mode 100644 internal/bootstrap/parse_test.go diff --git a/internal/bootstrap/parse.go b/internal/bootstrap/parse.go new file mode 100644 index 0000000..10c8557 --- /dev/null +++ b/internal/bootstrap/parse.go @@ -0,0 +1,44 @@ +package bootstrap + +import ( + "bytes" + "fmt" +) + +// marker precedes the credentials in etterminal's output +// (upstream src/terminal/TerminalMain.cpp:185). +const marker = "IDPASSKEY:" + +// parseCredentials finds the first IDPASSKEY marker in out and returns the +// id and passkey that follow it. It requires exactly idLen alphanumeric +// characters, a '/', then exactly passkeyLen alphanumeric characters, followed +// by the end of output or a non-alphanumeric character. Anything before the +// marker (banners, motd, shell noise) is ignored. +func parseCredentials(out []byte) (Credentials, error) { + _, rest, found := bytes.Cut(out, []byte(marker)) + if !found { + return Credentials{}, ErrNoCredentials + } + id, n := alnumRun(rest) + if n != idLen || len(rest) == n || rest[n] != '/' { + return Credentials{}, fmt.Errorf("%w: malformed id after %q", ErrNoCredentials, marker) + } + passkey, m := alnumRun(rest[n+1:]) + if m != passkeyLen { + // Do not echo the value: a malformed passkey may still be a real one. + return Credentials{}, fmt.Errorf("%w: malformed passkey after %q", ErrNoCredentials, marker) + } + return Credentials{ID: string(id), Passkey: string(passkey)}, nil +} + +// alnumRun returns the leading run of ASCII letters and digits in b and its length. +func alnumRun(b []byte) (run []byte, n int) { + for n < len(b) && isAlnum(b[n]) { + n++ + } + return b[:n], n +} + +func isAlnum(c byte) bool { + return c >= '0' && c <= '9' || c >= 'a' && c <= 'z' || c >= 'A' && c <= 'Z' +} diff --git a/internal/bootstrap/parse_test.go b/internal/bootstrap/parse_test.go new file mode 100644 index 0000000..d9c2b5f --- /dev/null +++ b/internal/bootstrap/parse_test.go @@ -0,0 +1,49 @@ +package bootstrap + +import ( + "errors" + "strings" + "testing" +) + +func TestParseCredentials(t *testing.T) { + tests := []struct { + name string + out string + want Credentials + ok bool + }{ + {"plain", "IDPASSKEY:" + testID + "/" + testPasskey + "\n", Credentials{testID, testPasskey}, true}, + {"crlf", "IDPASSKEY:" + testID + "/" + testPasskey + "\r\n", Credentials{testID, testPasskey}, true}, + {"no trailing newline", "IDPASSKEY:" + testID + "/" + testPasskey, Credentials{testID, testPasskey}, true}, + {"noise before", "Last login: Thu\nWelcome!\nIDPASSKEY:" + testID + "/" + testPasskey + "\n", Credentials{testID, testPasskey}, true}, + {"first marker wins", "IDPASSKEY:" + testID + "/" + testPasskey + "\nIDPASSKEY:zzzzzzzzzzzzzzzz/" + testPasskey, Credentials{testID, testPasskey}, true}, + {"no marker", "Welcome!\n", Credentials{}, false}, + {"empty", "", Credentials{}, false}, + {"truncated id", "IDPASSKEY:abcd", Credentials{}, false}, + {"truncated passkey", "IDPASSKEY:" + testID + "/0123", Credentials{}, false}, + {"id too long", "IDPASSKEY:" + testID + "X/" + testPasskey, Credentials{}, false}, + {"id too short", "IDPASSKEY:" + testID[1:] + "/" + testPasskey, Credentials{}, false}, + {"passkey too long", "IDPASSKEY:" + testID + "/" + testPasskey + "X\n", Credentials{}, false}, + {"missing slash", "IDPASSKEY:" + testID + testPasskey, Credentials{}, false}, + {"non alphanumeric in id", "IDPASSKEY:abcdEFGH1234567-/" + testPasskey, Credentials{}, false}, + {"non alphanumeric in passkey", "IDPASSKEY:" + testID + "/0123456789abcdef_BCDEF0123456789", Credentials{}, false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := parseCredentials([]byte(tt.out)) + if tt.ok { + if err != nil || got != tt.want { + t.Fatalf("parseCredentials() = %v, %v; want %v, nil", got, err, tt.want) + } + return + } + if !errors.Is(err, ErrNoCredentials) { + t.Fatalf("parseCredentials() error = %v, want ErrNoCredentials", err) + } + if strings.Contains(err.Error(), testPasskey[:8]) { + t.Fatalf("error %q quotes passkey material", err) + } + }) + } +} From e9cd05271480f15166ca69e40ab87f1630daa593 Mon Sep 17 00:00:00 2001 From: "Tomi P. Hakala" <7030001+tphakala@users.noreply.github.com> Date: Fri, 25 Sep 2026 17:27:53 +0300 Subject: [PATCH 05/15] feat(bootstrap): run ssh to start etterminal and return credentials Run executes ssh with inherited stdin and stderr so prompts work, captures stdout (capped at 1 MiB), cancels with interrupt then kill plus a WaitDelay, and ignores ssh's exit status when credentials arrive because etterminal daemonises. Failures produce one error with the exit status, a hint for 127 (etterminal missing) and 255 (ssh failed) and an excerpt cut before any passkey material. A server that does not regenerate the id triggers a logged warning. Tests fake ssh by re-executing the test binary, so they need no shell scripts. Verified with go test -race, vet and golangci-lint on linux and windows, and by hand against a local etserver 7.0.0 over ssh (success, missing etterminal, failed authentication). --- internal/bootstrap/bootstrap.go | 133 ++++++++++++++ internal/bootstrap/bootstrap_test.go | 259 +++++++++++++++++++++++++++ 2 files changed, 392 insertions(+) create mode 100644 internal/bootstrap/bootstrap_test.go diff --git a/internal/bootstrap/bootstrap.go b/internal/bootstrap/bootstrap.go index 06f988e..46b2e47 100644 --- a/internal/bootstrap/bootstrap.go +++ b/internal/bootstrap/bootstrap.go @@ -3,8 +3,15 @@ package bootstrap import ( + "bytes" + "context" "errors" + "fmt" "log/slog" + "os" + "os/exec" + "strings" + "time" ) var ( @@ -14,6 +21,16 @@ var ( ErrInvalidConfig = errors.New("bootstrap: invalid configuration") ) +const ( + // maxOutput caps how much ssh stdout is kept. Shell startup noise before + // the marker is normally a few lines; the cap only bounds memory. + maxOutput = 1 << 20 + // excerptLen caps the output excerpt quoted in an error. + excerptLen = 512 + // waitDelay is how long Run waits after cancelling ssh before killing it. + waitDelay = 2 * time.Second +) + // Config describes how to start etterminal over ssh. The zero value is not // usable: Destination is required. type Config struct { @@ -25,3 +42,119 @@ type Config struct { SSH string // ssh executable; default found with exec.LookPath("ssh") Logger *slog.Logger // nil discards; receives the no-regeneration warning } + +// Run starts etterminal on the server through ssh and returns the session +// credentials it prints. ssh's stdin and stderr are the process's own, so +// password, passphrase and host key prompts work; stdout is captured. Run +// must finish before the local console switches to raw mode. +// +// ssh's exit status does not decide success: etterminal daemonises after +// printing its credentials, so ssh can exit 0 or not regardless +// (upstream src/terminal/TerminalMain.cpp:185-188, MEASURED against etserver 7.0.0). +// +//nolint:gocritic // hugeParam: Config is taken by value on purpose so Run fills defaults on its own copy and never mutates the caller's; bootstrap runs once per session, not on a hot path. +func Run(ctx context.Context, cfg Config) (Credentials, error) { + cfg.applyDefaults() + if err := cfg.validate(); err != nil { + return Credentials{}, err + } + logger := cfg.Logger + if logger == nil { + logger = slog.New(slog.DiscardHandler) + } + sshPath := cfg.SSH + if sshPath == "" { + p, err := exec.LookPath("ssh") + if err != nil { + return Credentials{}, fmt.Errorf("bootstrap: ssh not found on PATH (install OpenSSH): %w", err) + } + sshPath = p + } + + 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 + + runErr := cmd.Run() + creds, parseErr := parseCredentials(out.Bytes()) + if parseErr == nil { + if creds.ID == id { + logger.Warn("etterminal did not regenerate the session id; the passkey was visible in the local ssh command line", + "credentials", creds) + } + return creds, nil + } + if ctx.Err() != nil { + return Credentials{}, fmt.Errorf("bootstrap: ssh interrupted: %w", context.Cause(ctx)) + } + exitErr, isExit := errors.AsType[*exec.ExitError](runErr) + if runErr != nil && !isExit { + return Credentials{}, fmt.Errorf("bootstrap: run %s: %w", sshPath, runErr) + } + return Credentials{}, describeFailure(exitErr, parseErr, out.Bytes()) +} + +// 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 { + var b strings.Builder + b.WriteString("ssh ") + code := 0 + if exitErr != nil { + code = exitErr.ExitCode() + fmt.Fprintf(&b, "exited with status %d", code) + } else { + b.WriteString("exited with status 0") + } + 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 ex := excerpt(out); ex != "" { + fmt.Fprintf(&b, "; output: %q", ex) + } + return fmt.Errorf("%w: %s", parseErr, b.String()) +} + +// excerpt returns at most excerptLen bytes of the end of out, cut at the +// marker so no part of a (possibly malformed) passkey is ever quoted. +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:] + } + return s +} + +// 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. +type cappedBuffer struct { + buf bytes.Buffer + max int +} + +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)]) + } + return len(p), nil +} + +func (c *cappedBuffer) Bytes() []byte { return c.buf.Bytes() } diff --git a/internal/bootstrap/bootstrap_test.go b/internal/bootstrap/bootstrap_test.go new file mode 100644 index 0000000..75384b2 --- /dev/null +++ b/internal/bootstrap/bootstrap_test.go @@ -0,0 +1,259 @@ +package bootstrap + +import ( + "bytes" + "context" + "encoding/json" + "errors" + "fmt" + "log/slog" + "os" + "path/filepath" + "regexp" + "slices" + "strings" + "testing" + "time" +) + +// The test binary doubles as a fake ssh. When fakeSSHModeEnv is set, TestMain +// runs runFakeSSH instead of the tests, so Config.SSH = os.Args[0] makes Run +// execute this same binary as "ssh". It needs no shell scripts, so the same +// tests run in the Windows CI job. +const ( + fakeSSHModeEnv = "ET_FAKE_SSH_MODE" // which behaviour the fake shows + fakeSSHArgvEnv = "ET_FAKE_SSH_ARGV" // file the fake writes its argv to, as JSON +) + +func TestMain(m *testing.M) { + if mode := os.Getenv(fakeSSHModeEnv); mode != "" { + os.Exit(runFakeSSH(mode, os.Args[1:])) + } + os.Exit(m.Run()) +} + +// runFakeSSH records its arguments and then behaves like ssh running +// etterminal in the given mode. It returns the exit status. +func runFakeSSH(mode string, args []string) int { + if path := os.Getenv(fakeSSHArgvEnv); path != "" { + data, err := json.Marshal(args) + if err != nil { + return 90 + } + if err := os.WriteFile(path, data, 0o600); err != nil { + return 91 + } + } + idpasskey := "IDPASSKEY:" + testID + "/" + testPasskey + "\n" + switch mode { + case "ok": + fmt.Print("Last login: Thu Sep 24 21:00:00 2026\nWelcome to the server\n" + idpasskey) + return 0 + case "ok-exit1": + // etterminal printed its credentials but ssh itself exited non-zero. + fmt.Print(idpasskey) + return 1 + case "echo-sent": + // A server that does not regenerate: it echoes back what it was sent. + m := regexp.MustCompile(`echo '([A-Za-z0-9]{16})/([A-Za-z0-9]{32})_`).FindStringSubmatch(args[len(args)-1]) + if m == nil { + return 92 + } + fmt.Print("IDPASSKEY:" + m[1] + "/" + m[2] + "\n") + return 0 + case "notfound": + // What bash prints when etterminal is not installed (it goes to stderr). + fmt.Fprintln(os.Stderr, "bash: line 1: etterminal: command not found") + return 127 + case "authfail": + fmt.Fprintln(os.Stderr, "alice@example.test: Permission denied (publickey).") + return 255 + case "noise": + fmt.Print("Welcome to the server\n") + return 0 + case "malformed": + fmt.Print("IDPASSKEY:abcd\n") + return 0 + case "hang": + time.Sleep(time.Hour) + return 0 + } + return 93 +} + +// useFakeSSH points the fake at mode and returns a Config that runs it and the +// path its argv will be written to. +func useFakeSSH(t *testing.T, mode string) (cfg Config, argvPath string) { + t.Helper() + argvPath = filepath.Join(t.TempDir(), "argv.json") + t.Setenv(fakeSSHModeEnv, mode) + t.Setenv(fakeSSHArgvEnv, argvPath) + // Under -race the fake child otherwise pauses for the race runtime's + // default atexit_sleep_ms (1s) on every exit with status 0. + t.Setenv("GORACE", "atexit_sleep_ms=0") + cfg = Config{ + Destination: "example.test", + User: "alice", + SSHOptions: []string{"BatchMode=yes"}, + SSH: os.Args[0], + } + return cfg, argvPath +} + +func readArgv(t *testing.T, path string) []string { + t.Helper() + data, err := os.ReadFile(path) + if err != nil { + t.Fatalf("fake ssh did not record its argv: %v", err) + } + var args []string + if err := json.Unmarshal(data, &args); err != nil { + t.Fatalf("decode argv: %v", err) + } + return args +} + +func TestRunSuccess(t *testing.T) { + cfg, argvPath := useFakeSSH(t, "ok") + + got, err := Run(t.Context(), cfg) + if err != nil { + t.Fatalf("Run() error = %v", err) + } + if want := (Credentials{ID: testID, Passkey: testPasskey}); got != want { + t.Fatalf("Run() = %#v, want %#v", got, want) + } + + args := readArgv(t, argvPath) + if want := []string{"-oBatchMode=yes", "alice@example.test"}; len(args) != 3 || !slices.Equal(args[:2], want) { + t.Fatalf("ssh argv = %q, want %q followed by the remote command", args, want) + } + remote := regexp.MustCompile(`^echo 'XXX[A-Z2-7]{13}/[A-Z2-7]{32}_xterm-256color' \| etterminal --verbose=0$`) + if !remote.MatchString(args[2]) { + t.Fatalf("remote command = %q, want it to match %s", args[2], remote) + } +} + +func TestRunIgnoresSSHExitStatusWhenCredentialsArrive(t *testing.T) { + cfg, _ := useFakeSSH(t, "ok-exit1") + if _, err := Run(t.Context(), cfg); err != nil { + t.Fatalf("Run() error = %v, want success despite ssh exit status 1", err) + } +} + +func TestRunWarnsWhenServerDoesNotRegenerate(t *testing.T) { + cfg, _ := useFakeSSH(t, "echo-sent") + var logs bytes.Buffer + cfg.Logger = slog.New(slog.NewJSONHandler(&logs, nil)) + + got, err := Run(t.Context(), cfg) + if err != nil { + t.Fatalf("Run() error = %v", err) + } + if !strings.HasPrefix(got.ID, regeneratePrefix) { + t.Fatalf("Run() id = %q, want the echoed placeholder", got.ID) + } + if !strings.Contains(logs.String(), "did not regenerate") { + t.Fatalf("no regeneration warning logged; logs: %s", logs.String()) + } + if strings.Contains(logs.String(), got.Passkey) { + t.Fatalf("warning leaks the passkey: %s", logs.String()) + } +} + +func TestRunFailures(t *testing.T) { + tests := []struct { + mode string + contains []string + }{ + // Review Focus 1: etterminal missing on the server. + {"notfound", []string{"etterminal", "status 127", "--terminal-path"}}, + {"authfail", []string{"status 255", "ssh could not connect or authenticate"}}, + {"noise", []string{"status 0", `"Welcome to the server"`}}, + {"malformed", []string{"malformed id", "status 0"}}, + } + for _, tt := range tests { + t.Run(tt.mode, func(t *testing.T) { + cfg, _ := useFakeSSH(t, tt.mode) + _, err := Run(t.Context(), cfg) + if !errors.Is(err, ErrNoCredentials) { + t.Fatalf("Run() error = %v, want ErrNoCredentials", err) + } + for _, s := range tt.contains { + if !strings.Contains(err.Error(), s) { + t.Errorf("error %q does not mention %q", err, s) + } + } + // The marker ends in ':', so printing it bare before ": ssh exited" + // would render a confusing "IDPASSKEY::". + if strings.Contains(err.Error(), "::") { + t.Errorf("error %q contains a doubled colon", err) + } + }) + } +} + +func TestRunCancel(t *testing.T) { + cfg, _ := useFakeSSH(t, "hang") + ctx, cancel := context.WithTimeout(t.Context(), 200*time.Millisecond) + defer cancel() + + start := time.Now() + _, err := Run(ctx, cfg) + if !errors.Is(err, context.DeadlineExceeded) { + t.Fatalf("Run() error = %v, want context.DeadlineExceeded", err) + } + if elapsed := time.Since(start); elapsed > 10*time.Second { + t.Fatalf("Run() took %v after cancel, want it bounded by waitDelay", elapsed) + } +} + +func TestRunInvalidConfigDoesNotStartSSH(t *testing.T) { + cfg, argvPath := useFakeSSH(t, "ok") + cfg.Term = "xterm'; id; '" + if _, err := Run(t.Context(), cfg); !errors.Is(err, ErrInvalidConfig) { + t.Fatalf("Run() error = %v, want ErrInvalidConfig", err) + } + if _, err := os.Stat(argvPath); !errors.Is(err, os.ErrNotExist) { + t.Fatalf("ssh ran despite an invalid config (argv file stat: %v)", err) + } +} + +func TestRunMissingSSH(t *testing.T) { + cfg := Config{Destination: "example.test", SSH: filepath.Join(t.TempDir(), "no-such-ssh")} + _, err := Run(t.Context(), cfg) + if err == nil || errors.Is(err, ErrNoCredentials) || !strings.Contains(err.Error(), "no-such-ssh") { + t.Fatalf("Run() error = %v, want a start error naming the executable", err) + } +} + +func TestRunSSHNotOnPath(t *testing.T) { + t.Setenv("PATH", t.TempDir()) + _, err := Run(t.Context(), Config{Destination: "example.test"}) + if err == nil || errors.Is(err, ErrNoCredentials) || !strings.Contains(err.Error(), "ssh not found on PATH") { + t.Fatalf("Run() error = %v, want an ssh-not-found error", err) + } +} + +func TestExcerptCutsAtMarker(t *testing.T) { + out := []byte("banner\nIDPASSKEY:" + testID + "/" + testPasskey[:10]) + if got := excerpt(out); got != "banner" { + t.Fatalf("excerpt() = %q, want %q", got, "banner") + } + long := bytes.Repeat([]byte("x"), 2*excerptLen) + if got := excerpt(long); len(got) != excerptLen+len("...") || !strings.HasPrefix(got, "...") { + t.Fatalf("excerpt() of long output has length %d, want %d with a ... prefix", len(got), excerptLen+3) + } +} + +func TestCappedBuffer(t *testing.T) { + c := &cappedBuffer{max: 4} + for _, s := range []string{"ab", "cdef", "gh"} { + if n, err := c.Write([]byte(s)); n != len(s) || err != nil { + t.Fatalf("Write(%q) = %d, %v", s, n, err) + } + } + if got := string(c.Bytes()); got != "abcd" { + t.Fatalf("Bytes() = %q, want %q", got, "abcd") + } +} From b164ffaea8b0fbe2acd64524d68b4b185c124e3b Mon Sep 17 00:00:00 2001 From: "Tomi P. Hakala" <7030001+tphakala@users.noreply.github.com> Date: Fri, 25 Sep 2026 17:46:23 +0300 Subject: [PATCH 06/15] fix(bootstrap): redact the passkey for every fmt verb and encoder Credentials redacted only through String, GoString and LogValue, so %d, %t and %o, encoding/json, encoding/xml and slog JSON or text of a struct or slice holding Credentials all printed the passkey. Format (fmt.Formatter) now covers every verb, MarshalJSON and MarshalText cover the encoders, and the redaction text is REDACTED to match protocol and seal. The doc names the renderings no method can reach: an unexported struct field, %p on a value, and encoding/gob. Verified with a rendering matrix test (fmt verbs, pointer, nested, slice, map, Errorf, json, xml, slog JSON and text), and by mutating Format, MarshalJSON and MarshalText to print the passkey, each of which turned the test red. --- AGENTS.md | 2 +- internal/bootstrap/credentials.go | 54 +++++++++++++--- internal/bootstrap/credentials_test.go | 86 +++++++++++++++++++------- 3 files changed, 110 insertions(+), 32 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 3f50eea..41ed760 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -16,7 +16,7 @@ Module path: `github.com/tphakala/et-go`. Go version: see `go.mod`. | Path | Role | | ----------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | `cmd/et` | Entry point and flag parsing. | -| `internal/bootstrap` | Runs the system ssh to start etterminal; validates the config, parses and redacts the IDPASSKEY credentials it prints. | +| `internal/bootstrap` | Runs the system ssh to start etterminal; validates the config, parses the IDPASSKEY credentials it prints and redacts the passkey. | | `internal/etcp` | Reliable, ordered, encrypted packet connection over replaceable TCP links: replay ring, recover exchange, liveness probes, reconnect backoff, write backpressure. | | `internal/etservertest` | Test-only fake etserver (written independently from upstream semantics) and an in-memory `net.Pipe` network with cut and refuse controls, for synctest-driven etcp tests. | | `internal/protocol` | Wire messages generated from upstream's `.proto` files (opaque API), plus `Header`, `Version` and `Packet`. Regenerate with `go generate ./internal/protocol` (needs protoc 3.21.12). | diff --git a/internal/bootstrap/credentials.go b/internal/bootstrap/credentials.go index 606b964..85807e9 100644 --- a/internal/bootstrap/credentials.go +++ b/internal/bootstrap/credentials.go @@ -1,26 +1,60 @@ package bootstrap -import "log/slog" +import ( + "encoding/json" + "fmt" + "log/slog" +) -// redacted replaces the passkey wherever Credentials are formatted or logged. -const redacted = "[redacted]" +// redacted replaces the passkey wherever Credentials are formatted, encoded or +// logged. It matches the spelling used by protocol.TerminalUserInfo and +// seal.Stream. +const redacted = "REDACTED" -// Credentials are the session id and passkey returned by etterminal. String, -// GoString and LogValue redact the passkey, so it cannot leak through fmt or -// slog by accident. The id is not secret and stays visible for debugging. +// Credentials are the session id and passkey returned by etterminal. The id is +// not secret and stays visible for debugging; the passkey is replaced by +// REDACTED in every rendering the type controls: fmt with any verb (Format), +// encoding/json (MarshalJSON), text encoders such as encoding/xml +// (MarshalText) and slog (LogValue). Those methods also run when Credentials +// sits behind a pointer or inside an exported field, a slice or a map. +// +// Known renderings that bypass every method and print the passkey: fmt formatting a +// Credentials held in an unexported struct field (fmt cannot call methods on +// it), and %p applied to a Credentials value (fmt handles %p before it looks +// for any method, and go vet does not flag it), and encoding/gob, which encodes +// the exported fields directly. Keep Credentials in exported fields, never +// format it with %p, and never gob-encode it. type Credentials struct { ID string Passkey string } -// String implements fmt.Stringer, used by %v, %+v and %s. +// String returns the redacted form, for callers that want it as a string. func (c Credentials) String() string { return "{ID:" + c.ID + " Passkey:" + redacted + "}" } -// GoString implements fmt.GoStringer, used by %#v. -func (c Credentials) GoString() string { - return `bootstrap.Credentials{ID:"` + c.ID + `", Passkey:"` + redacted + `"}` +// Format implements fmt.Formatter so every verb prints the redacted form; %#v +// prints it as a Go literal. +func (c Credentials) Format(f fmt.State, verb rune) { + if verb == 'v' && f.Flag('#') { + _, _ = fmt.Fprint(f, `bootstrap.Credentials{ID:"`+c.ID+`", Passkey:"`+redacted+`"}`) + return + } + _, _ = fmt.Fprint(f, c.String()) +} + +// MarshalJSON implements json.Marshaler with the passkey redacted. +func (c Credentials) MarshalJSON() ([]byte, error) { + return json.Marshal(struct { + ID string `json:"id"` + Passkey string `json:"passkey"` + }{c.ID, redacted}) +} + +// MarshalText implements encoding.TextMarshaler with the passkey redacted. +func (c Credentials) MarshalText() ([]byte, error) { + return []byte(c.String()), nil } // LogValue implements slog.LogValuer. diff --git a/internal/bootstrap/credentials_test.go b/internal/bootstrap/credentials_test.go index 38e1a59..3dd19e0 100644 --- a/internal/bootstrap/credentials_test.go +++ b/internal/bootstrap/credentials_test.go @@ -2,6 +2,8 @@ package bootstrap import ( "bytes" + "encoding/json" + "encoding/xml" "fmt" "log/slog" "strings" @@ -19,33 +21,75 @@ func TestCredentialsRedaction(t *testing.T) { c := Credentials{ID: testID, Passkey: testPasskey} type holder struct{ Creds Credentials } - // Each format carries surrounding text, as real log and error lines do. - formatted := map[string]string{ - "%v": fmt.Sprintf("creds %v", c), - "%+v": fmt.Sprintf("creds %+v", c), - "%s": fmt.Sprintf("creds %s", c), - "%#v": fmt.Sprintf("creds %#v", c), - "pointer %v": fmt.Sprintf("creds %v", &c), - "nested %+v": fmt.Sprintf("creds %+v", holder{c}), - "nested %#v": fmt.Sprintf("creds %#v", holder{c}), - "Sprint": fmt.Sprint("creds ", c), - "Errorf %v": fmt.Errorf("dial with %v", c).Error(), - } - for verb, s := range formatted { + mustJSON := func(v any) string { + b, err := json.Marshal(v) + if err != nil { + t.Fatalf("json.Marshal: %v", err) + } + return string(b) + } + mustXML := func(v any) string { + b, err := xml.Marshal(v) + if err != nil { + t.Fatalf("xml.Marshal: %v", err) + } + return string(b) + } + slogOut := func(h func(*bytes.Buffer) slog.Handler, args ...any) string { + var buf bytes.Buffer + slog.New(h(&buf)).Info("got", args...) + return buf.String() + } + jsonHandler := func(b *bytes.Buffer) slog.Handler { return slog.NewJSONHandler(b, nil) } + textHandler := func(b *bytes.Buffer) slog.Handler { return slog.NewTextHandler(b, nil) } + + // Each rendering carries surrounding text, as real log and error lines do. + rendered := map[string]string{ + "%v": fmt.Sprintf("creds %v", c), + "%+v": fmt.Sprintf("creds %+v", c), + "%s": fmt.Sprintf("creds %s", c), + "%q": fmt.Sprintf("creds %q", c), + "%x": fmt.Sprintf("creds %x", c), + "%X": fmt.Sprintf("creds %X", c), + "%d": fmt.Sprintf("creds %d", c), + "%t": fmt.Sprintf("creds %t", c), + "%o": fmt.Sprintf("creds %o", c), + "%#v": fmt.Sprintf("creds %#v", c), + "pointer %v": fmt.Sprintf("creds %v", &c), + "pointer %d": fmt.Sprintf("creds %d", &c), + "nested %+v": fmt.Sprintf("creds %+v", holder{c}), + "nested %#v": fmt.Sprintf("creds %#v", holder{c}), + "slice %v": fmt.Sprintf("creds %v", []Credentials{c}), + "map %v": fmt.Sprintf("creds %v", map[string]Credentials{"k": c}), + "Sprint": fmt.Sprint("creds ", c), + "Errorf %v": fmt.Errorf("dial with %v", c).Error(), + "json": mustJSON(c), + "json nested": mustJSON(holder{c}), + "json slice": mustJSON([]Credentials{c}), + "xml nested": mustXML(holder{c}), + "slog json": slogOut(jsonHandler, "credentials", c), + "slog json nested": slogOut(jsonHandler, "h", holder{c}), + "slog json slice": slogOut(jsonHandler, "s", []Credentials{c}), + "slog text": slogOut(textHandler, "credentials", c), + "slog text nested": slogOut(textHandler, "h", holder{c}), + "slog json pointer": slogOut(jsonHandler, "credentials", &c), + } + for name, s := range rendered { if strings.Contains(s, testPasskey) { - t.Errorf("%s leaks the passkey: %s", verb, s) + t.Errorf("%s leaks the passkey: %s", name, s) } if !strings.Contains(s, testID) || !strings.Contains(s, redacted) { - t.Errorf("%s = %s, want the id and %q", verb, s, redacted) + t.Errorf("%s = %s, want the id and %q", name, s, redacted) } } - var buf bytes.Buffer - slog.New(slog.NewJSONHandler(&buf, nil)).Info("got", "credentials", c) - if strings.Contains(buf.String(), testPasskey) { - t.Fatalf("slog JSON leaks the passkey: %s", buf.String()) + if got := rendered["slog json"]; !strings.Contains(got, `"credentials":{"id":"`+testID+`","passkey":"REDACTED"}`) { + t.Fatalf("slog JSON = %s, want a redacted credentials group", got) + } + if got, want := rendered["%#v"], `creds bootstrap.Credentials{ID:"`+testID+`", Passkey:"REDACTED"}`; got != want { + t.Fatalf("%%#v = %s, want %s", got, want) } - if !strings.Contains(buf.String(), `"credentials":{"id":"`+testID+`","passkey":"[redacted]"}`) { - t.Fatalf("slog JSON = %s, want a redacted credentials group", buf.String()) + if got, want := rendered["json"], `{"id":"`+testID+`","passkey":"REDACTED"}`; got != want { + t.Fatalf("json = %s, want %s", got, want) } } From b699039074443d5d74923f11297dea57314d920c Mon Sep 17 00:00:00 2001 From: "Tomi P. Hakala" <7030001+tphakala@users.noreply.github.com> Date: Fri, 25 Sep 2026 17:54:41 +0300 Subject: [PATCH 07/15] test(bootstrap): pin the lines a regression could remove unnoticed Mutation runs showed production lines whose removal left every test green: the no-regeneration warning guard, the interrupt-first cmd.Cancel, cmd.WaitDelay, the ssh found on PATH, the parse bounds and slash guards, control-character validation, the excerpt keeping the tail, the per-status hints and the malformed-passkey message. Each now has an assertion that fails without it. The cancel test is Unix only: the fake ssh traps os.Interrupt, records it, and opens a FIFO once its handler is installed, so the test cancels without sleeping and proves both the interrupt and the WaitDelay kill. Verified by mutating each pinned line and watching its test fail on an assertion. --- internal/bootstrap/bootstrap_test.go | 100 ++++++++++++++++++---- internal/bootstrap/bootstrap_unix_test.go | 74 ++++++++++++++++ internal/bootstrap/command_test.go | 3 + internal/bootstrap/parse_test.go | 48 ++++++----- 4 files changed, 189 insertions(+), 36 deletions(-) create mode 100644 internal/bootstrap/bootstrap_unix_test.go diff --git a/internal/bootstrap/bootstrap_test.go b/internal/bootstrap/bootstrap_test.go index 75384b2..821e95d 100644 --- a/internal/bootstrap/bootstrap_test.go +++ b/internal/bootstrap/bootstrap_test.go @@ -8,8 +8,10 @@ import ( "fmt" "log/slog" "os" + "os/signal" "path/filepath" "regexp" + "runtime" "slices" "strings" "testing" @@ -21,8 +23,10 @@ import ( // execute this same binary as "ssh". It needs no shell scripts, so the same // tests run in the Windows CI job. const ( - fakeSSHModeEnv = "ET_FAKE_SSH_MODE" // which behaviour the fake shows - fakeSSHArgvEnv = "ET_FAKE_SSH_ARGV" // file the fake writes its argv to, as JSON + fakeSSHModeEnv = "ET_FAKE_SSH_MODE" // which behaviour the fake shows + fakeSSHArgvEnv = "ET_FAKE_SSH_ARGV" // file the fake writes its argv to, as JSON + fakeSSHReadyEnv = "ET_FAKE_SSH_READY" // "trap-int": FIFO opened once the handler is installed + fakeSSHSignaledEnv = "ET_FAKE_SSH_SIGNALED" // "trap-int": file created when os.Interrupt arrives ) func TestMain(m *testing.M) { @@ -74,9 +78,32 @@ func runFakeSSH(mode string, args []string) int { case "malformed": fmt.Print("IDPASSKEY:abcd\n") return 0 + case "exit1": + // A remote failure with some other status and no output. + return 1 case "hang": time.Sleep(time.Hour) return 0 + case "trap-int": + // Survives os.Interrupt and records that it arrived, so only the + // WaitDelay kill can end it. Opening the FIFO tells the test that the + // handler is installed. + sig := make(chan os.Signal, 1) + signal.Notify(sig, os.Interrupt) + ready, err := os.OpenFile(os.Getenv(fakeSSHReadyEnv), os.O_WRONLY, 0) + if err != nil { + return 94 + } + _ = ready.Close() + <-sig + if err := os.WriteFile(os.Getenv(fakeSSHSignaledEnv), nil, 0o600); err != nil { + return 95 + } + // Longer than the test's 15s bound, so only the WaitDelay kill ends it + // in time, but short enough that a regression which never kills it + // does not hold go test's output pipe open for long. + time.Sleep(30 * time.Second) + return 0 } return 93 } @@ -115,13 +142,19 @@ func readArgv(t *testing.T, path string) []string { func TestRunSuccess(t *testing.T) { cfg, argvPath := useFakeSSH(t, "ok") + var logs bytes.Buffer + cfg.Logger = slog.New(slog.NewJSONHandler(&logs, nil)) got, err := Run(t.Context(), cfg) if err != nil { t.Fatalf("Run() error = %v", err) } - if want := (Credentials{ID: testID, Passkey: testPasskey}); got != want { - t.Fatalf("Run() = %#v, want %#v", got, want) + if got.ID != testID || got.Passkey != testPasskey { + t.Fatalf("Run() = %v with passkey match %v, want id %s and the fake's passkey", got, got.Passkey == testPasskey, testID) + } + // The server regenerated the credentials, so there is nothing to warn about. + if logs.Len() != 0 { + t.Fatalf("Run() logged on a normal start: %s", logs.String()) } args := readArgv(t, argvPath) @@ -142,7 +175,7 @@ func TestRunIgnoresSSHExitStatusWhenCredentialsArrive(t *testing.T) { } func TestRunWarnsWhenServerDoesNotRegenerate(t *testing.T) { - cfg, _ := useFakeSSH(t, "echo-sent") + cfg, argvPath := useFakeSSH(t, "echo-sent") var logs bytes.Buffer cfg.Logger = slog.New(slog.NewJSONHandler(&logs, nil)) @@ -150,8 +183,9 @@ func TestRunWarnsWhenServerDoesNotRegenerate(t *testing.T) { if err != nil { t.Fatalf("Run() error = %v", err) } - if !strings.HasPrefix(got.ID, regeneratePrefix) { - t.Fatalf("Run() id = %q, want the echoed placeholder", got.ID) + sent := regexp.MustCompile(`^echo '([A-Z2-7]{16})/`).FindStringSubmatch(readArgv(t, argvPath)[2]) + if len(sent) != 2 || got.ID != sent[1] { + t.Fatalf("Run() id = %q, want the placeholder sent in the remote command (%q)", got.ID, sent) } if !strings.Contains(logs.String(), "did not regenerate") { t.Fatalf("no regeneration warning logged; logs: %s", logs.String()) @@ -162,15 +196,21 @@ func TestRunWarnsWhenServerDoesNotRegenerate(t *testing.T) { } func TestRunFailures(t *testing.T) { + const ( + missingHint = "--terminal-path" + sshHint = "ssh could not connect or authenticate" + ) tests := []struct { mode string contains []string + absent []string // hints that belong to other exit statuses }{ - // Review Focus 1: etterminal missing on the server. - {"notfound", []string{"etterminal", "status 127", "--terminal-path"}}, - {"authfail", []string{"status 255", "ssh could not connect or authenticate"}}, - {"noise", []string{"status 0", `"Welcome to the server"`}}, - {"malformed", []string{"malformed id", "status 0"}}, + // etterminal missing on the server. + {"notfound", []string{"etterminal", "status 127", missingHint}, []string{sshHint}}, + {"authfail", []string{"status 255", sshHint}, []string{missingHint}}, + {"noise", []string{"status 0", `"Welcome to the server"`}, []string{missingHint, sshHint}}, + {"malformed", []string{"malformed id", "status 0"}, []string{missingHint, sshHint}}, + {"exit1", []string{"status 1"}, []string{missingHint, sshHint, "; output:"}}, } for _, tt := range tests { t.Run(tt.mode, func(t *testing.T) { @@ -184,6 +224,11 @@ func TestRunFailures(t *testing.T) { t.Errorf("error %q does not mention %q", err, s) } } + for _, s := range tt.absent { + if strings.Contains(err.Error(), s) { + t.Errorf("error %q mentions %q, which does not apply", err, s) + } + } // The marker ends in ':', so printing it bare before ": ssh exited" // would render a confusing "IDPASSKEY::". if strings.Contains(err.Error(), "::") { @@ -240,9 +285,34 @@ func TestExcerptCutsAtMarker(t *testing.T) { if got := excerpt(out); got != "banner" { t.Fatalf("excerpt() = %q, want %q", got, "banner") } - long := bytes.Repeat([]byte("x"), 2*excerptLen) - if got := excerpt(long); len(got) != excerptLen+len("...") || !strings.HasPrefix(got, "...") { - t.Fatalf("excerpt() of long output has length %d, want %d with a ... prefix", len(got), excerptLen+3) + // The end of the output is what explains a failure, so the tail is kept. + long := []byte("HEAD" + strings.Repeat("x", 2*excerptLen) + "TAIL") + got := excerpt(long) + if len(got) != excerptLen+len("...") || !strings.HasPrefix(got, "...") || !strings.HasSuffix(got, "TAIL") || strings.Contains(got, "HEAD") { + t.Fatalf("excerpt() of long output = %q (length %d), want the last %d bytes after a ... prefix", got, len(got), excerptLen) + } +} + +func TestRunFindsSSHOnPath(t *testing.T) { + cfg, _ := useFakeSSH(t, "ok") + dir := t.TempDir() + name := "ssh" + if runtime.GOOS == "windows" { + name = "ssh.exe" + } + self, err := os.ReadFile(os.Args[0]) + if err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(dir, name), self, 0o700); err != nil { + t.Fatal(err) + } + t.Setenv("PATH", dir) + cfg.SSH = "" + + got, err := Run(t.Context(), cfg) + if err != nil || got.ID != testID { + t.Fatalf("Run() = %v, %v; want the fake's credentials through the ssh found on PATH", got, err) } } diff --git a/internal/bootstrap/bootstrap_unix_test.go b/internal/bootstrap/bootstrap_unix_test.go new file mode 100644 index 0000000..76f5f78 --- /dev/null +++ b/internal/bootstrap/bootstrap_unix_test.go @@ -0,0 +1,74 @@ +//go:build unix + +package bootstrap + +import ( + "context" + "errors" + "os" + "path/filepath" + "syscall" + "testing" + "time" +) + +// TestRunCancelInterruptsThenKills pins both halves of Run's cancellation: ssh +// is sent os.Interrupt first (so it can restore the terminal), and a child that +// survives the interrupt is still killed once waitDelay has passed. Windows +// cannot deliver os.Interrupt to another process, so this test is Unix only. +func TestRunCancelInterruptsThenKills(t *testing.T) { + cfg, _ := useFakeSSH(t, "trap-int") + dir := t.TempDir() + ready := filepath.Join(dir, "ready") + signaled := filepath.Join(dir, "signaled") + if err := syscall.Mkfifo(ready, 0o600); err != nil { + t.Fatalf("mkfifo: %v", err) + } + t.Setenv(fakeSSHReadyEnv, ready) + t.Setenv(fakeSSHSignaledEnv, signaled) + + ctx, cancel := context.WithCancel(t.Context()) + defer cancel() + type result struct{ err error } + done := make(chan result, 1) + go func() { + _, err := Run(ctx, cfg) + done <- result{err} + }() + + // Opening the FIFO for reading blocks until the fake opens it for + // writing, which it does only after installing its interrupt handler. + opened := make(chan error, 1) + go func() { + f, err := os.Open(ready) + if err == nil { + _ = f.Close() + } + opened <- err + }() + bound := time.NewTimer(15 * time.Second) + defer bound.Stop() + select { + case err := <-opened: + if err != nil { + t.Fatalf("open ready FIFO: %v", err) + } + case r := <-done: + t.Fatalf("Run() returned %v before the fake was ready", r.err) + case <-bound.C: + t.Fatal("the fake never opened the ready FIFO") + } + + cancel() + select { + case r := <-done: + if !errors.Is(r.err, context.Canceled) { + t.Fatalf("Run() error = %v, want context.Canceled", r.err) + } + case <-bound.C: + t.Fatal("Run() did not return after cancel; a child that ignores the interrupt must be killed after waitDelay") + } + if _, err := os.Stat(signaled); err != nil { + t.Fatalf("the fake never received os.Interrupt: %v", err) + } +} diff --git a/internal/bootstrap/command_test.go b/internal/bootstrap/command_test.go index 6f5c218..00bc3e4 100644 --- a/internal/bootstrap/command_test.go +++ b/internal/bootstrap/command_test.go @@ -25,7 +25,10 @@ func TestValidate(t *testing.T) { {"empty destination", func(c *Config) { c.Destination = "" }, true}, {"destination is an option", func(c *Config) { c.Destination = "-oProxyCommand=evil" }, true}, {"destination with space", func(c *Config) { c.Destination = "a b" }, true}, + {"destination with control", func(c *Config) { c.Destination = "host\x01" }, true}, {"user with at", func(c *Config) { c.User = "a@b" }, true}, + {"user with space", func(c *Config) { c.User = "a b" }, true}, + {"user with control", func(c *Config) { c.User = "a\x01" }, true}, {"user is an option", func(c *Config) { c.User = "-x" }, true}, {"destination with user and no User", func(c *Config) { c.Destination = "alice@host" }, false}, {"User and a user in destination", func(c *Config) { c.User = "bob"; c.Destination = "alice@host" }, true}, diff --git a/internal/bootstrap/parse_test.go b/internal/bootstrap/parse_test.go index d9c2b5f..38e89b1 100644 --- a/internal/bootstrap/parse_test.go +++ b/internal/bootstrap/parse_test.go @@ -8,31 +8,33 @@ import ( func TestParseCredentials(t *testing.T) { tests := []struct { - name string - out string - want Credentials - ok bool + name string + out string + want Credentials + wantErr string // "" means success; otherwise a substring the error must contain }{ - {"plain", "IDPASSKEY:" + testID + "/" + testPasskey + "\n", Credentials{testID, testPasskey}, true}, - {"crlf", "IDPASSKEY:" + testID + "/" + testPasskey + "\r\n", Credentials{testID, testPasskey}, true}, - {"no trailing newline", "IDPASSKEY:" + testID + "/" + testPasskey, Credentials{testID, testPasskey}, true}, - {"noise before", "Last login: Thu\nWelcome!\nIDPASSKEY:" + testID + "/" + testPasskey + "\n", Credentials{testID, testPasskey}, true}, - {"first marker wins", "IDPASSKEY:" + testID + "/" + testPasskey + "\nIDPASSKEY:zzzzzzzzzzzzzzzz/" + testPasskey, Credentials{testID, testPasskey}, true}, - {"no marker", "Welcome!\n", Credentials{}, false}, - {"empty", "", Credentials{}, false}, - {"truncated id", "IDPASSKEY:abcd", Credentials{}, false}, - {"truncated passkey", "IDPASSKEY:" + testID + "/0123", Credentials{}, false}, - {"id too long", "IDPASSKEY:" + testID + "X/" + testPasskey, Credentials{}, false}, - {"id too short", "IDPASSKEY:" + testID[1:] + "/" + testPasskey, Credentials{}, false}, - {"passkey too long", "IDPASSKEY:" + testID + "/" + testPasskey + "X\n", Credentials{}, false}, - {"missing slash", "IDPASSKEY:" + testID + testPasskey, Credentials{}, false}, - {"non alphanumeric in id", "IDPASSKEY:abcdEFGH1234567-/" + testPasskey, Credentials{}, false}, - {"non alphanumeric in passkey", "IDPASSKEY:" + testID + "/0123456789abcdef_BCDEF0123456789", Credentials{}, false}, + {"plain", "IDPASSKEY:" + testID + "/" + testPasskey + "\n", Credentials{testID, testPasskey}, ""}, + {"crlf", "IDPASSKEY:" + testID + "/" + testPasskey + "\r\n", Credentials{testID, testPasskey}, ""}, + {"no trailing newline", "IDPASSKEY:" + testID + "/" + testPasskey, Credentials{testID, testPasskey}, ""}, + {"noise before", "Last login: Thu\nWelcome!\nIDPASSKEY:" + testID + "/" + testPasskey + "\n", Credentials{testID, testPasskey}, ""}, + {"first marker wins", "IDPASSKEY:" + testID + "/" + testPasskey + "\nIDPASSKEY:zzzzzzzzzzzzzzzz/" + testPasskey, Credentials{testID, testPasskey}, ""}, + {"no marker", "Welcome!\n", Credentials{}, "no IDPASSKEY"}, + {"empty", "", Credentials{}, "no IDPASSKEY"}, + {"truncated id", "IDPASSKEY:abcd", Credentials{}, "malformed id"}, + {"id at end of output", "IDPASSKEY:" + testID, Credentials{}, "malformed id"}, + {"colon instead of slash", "IDPASSKEY:" + testID + ":" + testPasskey, Credentials{}, "malformed id"}, + {"truncated passkey", "IDPASSKEY:" + testID + "/0123", Credentials{}, "malformed passkey"}, + {"id too long", "IDPASSKEY:" + testID + "X/" + testPasskey, Credentials{}, "malformed id"}, + {"id too short", "IDPASSKEY:" + testID[1:] + "/" + testPasskey, Credentials{}, "malformed id"}, + {"passkey too long", "IDPASSKEY:" + testID + "/" + testPasskey + "X\n", Credentials{}, "malformed passkey"}, + {"missing slash", "IDPASSKEY:" + testID + testPasskey, Credentials{}, "malformed id"}, + {"non alphanumeric in id", "IDPASSKEY:abcdEFGH1234567-/" + testPasskey, Credentials{}, "malformed id"}, + {"non alphanumeric in passkey", "IDPASSKEY:" + testID + "/0123456789abcdef_BCDEF0123456789", Credentials{}, "malformed passkey"}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { got, err := parseCredentials([]byte(tt.out)) - if tt.ok { + if tt.wantErr == "" { if err != nil || got != tt.want { t.Fatalf("parseCredentials() = %v, %v; want %v, nil", got, err, tt.want) } @@ -41,7 +43,11 @@ func TestParseCredentials(t *testing.T) { if !errors.Is(err, ErrNoCredentials) { t.Fatalf("parseCredentials() error = %v, want ErrNoCredentials", err) } - if strings.Contains(err.Error(), testPasskey[:8]) { + if !strings.Contains(err.Error(), tt.wantErr) { + t.Fatalf("parseCredentials() error = %q, want it to mention %q", err, tt.wantErr) + } + // Every row's passkey material starts with "0123"; no error may quote it. + if strings.Contains(err.Error(), testPasskey[:4]) { t.Fatalf("error %q quotes passkey material", err) } }) From bf27250b91b0a09899060ed0c3945f0da567e2f4 Mon Sep 17 00:00:00 2001 From: "Tomi P. Hakala" <7030001+tphakala@users.noreply.github.com> Date: Fri, 25 Sep 2026 17:59:07 +0300 Subject: [PATCH 08/15] fix(bootstrap): report a lingering ssh stdout as missing credentials When ssh exited 0 without credentials while a descendant still held its stdout, exec returned ErrWaitDelay and Run reported a start failure with no exit status, hint or output excerpt. ErrWaitDelay only follows a successful exit, so it now goes to the same missing-credentials report as any other status-0 run. Verified with a fake ssh that exits 0 while a grandchild keeps writing to the inherited stdout; the test failed with the start-failure error before the fix and passes after it. --- internal/bootstrap/bootstrap.go | 4 +++- internal/bootstrap/bootstrap_test.go | 23 +++++++++++++++++++++++ 2 files changed, 26 insertions(+), 1 deletion(-) diff --git a/internal/bootstrap/bootstrap.go b/internal/bootstrap/bootstrap.go index 46b2e47..da34974 100644 --- a/internal/bootstrap/bootstrap.go +++ b/internal/bootstrap/bootstrap.go @@ -100,7 +100,9 @@ func Run(ctx context.Context, cfg Config) (Credentials, error) { return Credentials{}, fmt.Errorf("bootstrap: ssh interrupted: %w", context.Cause(ctx)) } exitErr, isExit := errors.AsType[*exec.ExitError](runErr) - if runErr != nil && !isExit { + // ErrWaitDelay means ssh exited 0 but a descendant kept its stdout open + // past waitDelay: a finished run without credentials, not a start failure. + if runErr != nil && !isExit && !errors.Is(runErr, exec.ErrWaitDelay) { return Credentials{}, fmt.Errorf("bootstrap: run %s: %w", sshPath, runErr) } return Credentials{}, describeFailure(exitErr, parseErr, out.Bytes()) diff --git a/internal/bootstrap/bootstrap_test.go b/internal/bootstrap/bootstrap_test.go index 821e95d..a584f93 100644 --- a/internal/bootstrap/bootstrap_test.go +++ b/internal/bootstrap/bootstrap_test.go @@ -8,6 +8,7 @@ import ( "fmt" "log/slog" "os" + "os/exec" "os/signal" "path/filepath" "regexp" @@ -81,6 +82,27 @@ func runFakeSSH(mode string, args []string) int { case "exit1": // A remote failure with some other status and no output. return 1 + case "linger": + // ssh exits 0 without credentials while a descendant still holds its + // stdout, so Run's WaitDelay has to close the pipe. + hold := exec.Command(os.Args[0]) + hold.Env = append(os.Environ(), fakeSSHModeEnv+"=hold", fakeSSHArgvEnv+"=") + hold.Stdout = os.Stdout + if err := hold.Start(); err != nil { + return 96 + } + return 0 + case "hold": + // Keeps writing to the inherited stdout and exits on the first failed + // write, which comes as soon as the reader closes the pipe; the loop + // bound stops it even if nobody does. + for range 50 { + if _, err := os.Stdout.WriteString("."); err != nil { + return 0 + } + time.Sleep(100 * time.Millisecond) + } + return 0 case "hang": time.Sleep(time.Hour) return 0 @@ -211,6 +233,7 @@ func TestRunFailures(t *testing.T) { {"noise", []string{"status 0", `"Welcome to the server"`}, []string{missingHint, sshHint}}, {"malformed", []string{"malformed id", "status 0"}, []string{missingHint, sshHint}}, {"exit1", []string{"status 1"}, []string{missingHint, sshHint, "; output:"}}, + {"linger", []string{"status 0"}, []string{missingHint, sshHint}}, } for _, tt := range tests { t.Run(tt.mode, func(t *testing.T) { From 9a835a26a2a97bdafbed18ded842ce72b85b2acb Mon Sep 17 00:00:00 2001 From: "Tomi P. Hakala" <7030001+tphakala@users.noreply.github.com> Date: Fri, 25 Sep 2026 18:00:34 +0300 Subject: [PATCH 09/15] docs(bootstrap): make comments match the code and stand on their own Drop pointers to private design documents, name upstream's genCommand correctly, say that a marker past the output cap is lost, describe the excerpt length exactly, state that the terminal path admits a tilde on purpose, note that the first marker wins as in the upstream client, and say the no-regeneration warning applies to the command line on both hosts. The two character checks use strings.ContainsFunc. Verified with vet, golangci-lint on linux and windows, and go test -race. --- internal/bootstrap/bootstrap.go | 10 ++++++---- internal/bootstrap/command.go | 14 ++++++++------ internal/bootstrap/parse.go | 4 +++- internal/bootstrap/placeholder.go | 2 +- 4 files changed, 18 insertions(+), 12 deletions(-) diff --git a/internal/bootstrap/bootstrap.go b/internal/bootstrap/bootstrap.go index da34974..30c4337 100644 --- a/internal/bootstrap/bootstrap.go +++ b/internal/bootstrap/bootstrap.go @@ -23,7 +23,8 @@ var ( const ( // maxOutput caps how much ssh stdout is kept. Shell startup noise before - // the marker is normally a few lines; the cap only bounds memory. + // the marker is normally a few lines, far below the cap; a marker that + // arrives after the cap is dropped and the start fails. maxOutput = 1 << 20 // excerptLen caps the output excerpt quoted in an error. excerptLen = 512 @@ -91,7 +92,7 @@ func Run(ctx context.Context, cfg Config) (Credentials, error) { creds, parseErr := parseCredentials(out.Bytes()) if parseErr == nil { if creds.ID == id { - logger.Warn("etterminal did not regenerate the session id; the passkey was visible in the local ssh command line", + logger.Warn("etterminal did not regenerate the session id; the passkey in use was visible in the ssh command line on both hosts", "credentials", creds) } return creds, nil @@ -133,8 +134,9 @@ func describeFailure(exitErr *exec.ExitError, parseErr error, out []byte) error return fmt.Errorf("%w: %s", parseErr, b.String()) } -// excerpt returns at most excerptLen bytes of the end of out, cut at the -// marker so no part of a (possibly malformed) passkey is ever quoted. +// 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. func excerpt(out []byte) string { before, _, _ := bytes.Cut(out, []byte(marker)) s := strings.TrimSpace(string(before)) diff --git a/internal/bootstrap/command.go b/internal/bootstrap/command.go index 8744fbe..4ec9d0c 100644 --- a/internal/bootstrap/command.go +++ b/internal/bootstrap/command.go @@ -12,9 +12,10 @@ const ( defaultTerminalPath = "etterminal" ) -// termPattern and terminalPathPattern bound the two values interpolated into -// the remote shell command (spec 4.1 item 8, spec 5.5). Neither admits a quote, -// space or shell metacharacter. +// termPattern and terminalPathPattern bound the two configurable values +// interpolated into the remote shell command. Neither admits a quote, space, +// glob, '$' or command separator; terminalPathPattern admits '~', which the +// remote shell expands on purpose so "~/bin/etterminal" works. // // termPattern also excludes '_': etterminal splits its stdin line on '_' and // aborts unless there are exactly two tokens, so a TERM containing '_' kills @@ -66,7 +67,8 @@ func (cfg *Config) validate() error { } // remoteCommand is the command ssh runs on the server. It mirrors upstream's -// SshSetupHandler::genCommand: etterminal reads "/_" on stdin. +// genCommand (src/terminal/SshSetupHandler.cpp): etterminal reads +// "/_" on stdin. // Every interpolated value is validated or generated from [A-Za-z0-9]. func remoteCommand(id, passkey, term, terminalPath string) string { return "echo '" + id + "/" + passkey + "_" + term + "' | " + terminalPath + " --verbose=0" @@ -87,9 +89,9 @@ func sshArgs(cfg *Config, remote string) []string { } func hasSpaceOrControl(s string) bool { - return strings.IndexFunc(s, func(r rune) bool { return unicode.IsSpace(r) || unicode.IsControl(r) }) >= 0 + return strings.ContainsFunc(s, func(r rune) bool { return unicode.IsSpace(r) || unicode.IsControl(r) }) } func hasControl(s string) bool { - return strings.IndexFunc(s, unicode.IsControl) >= 0 + return strings.ContainsFunc(s, unicode.IsControl) } diff --git a/internal/bootstrap/parse.go b/internal/bootstrap/parse.go index 10c8557..f655451 100644 --- a/internal/bootstrap/parse.go +++ b/internal/bootstrap/parse.go @@ -13,7 +13,9 @@ const marker = "IDPASSKEY:" // id and passkey that follow it. It requires exactly idLen alphanumeric // characters, a '/', then exactly passkeyLen alphanumeric characters, followed // by the end of output or a non-alphanumeric character. Anything before the -// marker (banners, motd, shell noise) is ignored. +// marker (banners, motd, shell noise) is ignored. Only the first marker counts, +// as in the upstream client (src/terminal/SshSetupHandler.cpp, sshBuffer.find), +// so a banner that itself prints "IDPASSKEY:" makes the start fail. func parseCredentials(out []byte) (Credentials, error) { _, rest, found := bytes.Cut(out, []byte(marker)) if !found { diff --git a/internal/bootstrap/placeholder.go b/internal/bootstrap/placeholder.go index 35bbba5..8d8a39b 100644 --- a/internal/bootstrap/placeholder.go +++ b/internal/bootstrap/placeholder.go @@ -15,7 +15,7 @@ const ( // placeholder returns the throwaway id and passkey sent to etterminal. They // are not secrets: with the regeneratePrefix the server replaces both and -// returns the real ones on stdout (spec 4.1 item 7). +// returns the real ones on stdout. // // rand.Text returns at least 26 base32 characters ([A-Z2-7]), all // alphanumeric, so slicing to 13 and to 32 (from two calls) is always in range. From 8423c52a3c3740e62a5503608bb6bbbcd429102b Mon Sep 17 00:00:00 2001 From: "Tomi P. Hakala" <7030001+tphakala@users.noreply.github.com> Date: Fri, 25 Sep 2026 18:20:27 +0300 Subject: [PATCH 10/15] fix(bootstrap)!: keep the passkey behind an unexported pointer Format, the marshalers and LogValue could not reach three renderings: fmt of a Credentials held in an unexported struct field, %p on a value, and encoding/gob. Each printed the passkey. The passkey now sits behind an unexported *string, read with Passkey() and set with NewCredentials, so reflection sees only an address and the encoders skip it. %#v quotes the id, and the doc says to hold Credentials in a named field because embedding promotes Format and the marshalers. Verified with the rendering matrix extended to the three bypasses, a LogValue kind check and a %#v row with a quote in the id; turning the field back into a plain string, renaming LogValue, dropping the quoting or breaking Passkey() each turns a test red. --- internal/bootstrap/bootstrap_test.go | 6 ++-- internal/bootstrap/credentials.go | 38 ++++++++++++++-------- internal/bootstrap/credentials_test.go | 44 +++++++++++++++++++++++++- internal/bootstrap/parse.go | 2 +- internal/bootstrap/parse_test.go | 14 ++++---- 5 files changed, 80 insertions(+), 24 deletions(-) diff --git a/internal/bootstrap/bootstrap_test.go b/internal/bootstrap/bootstrap_test.go index a584f93..bda7e48 100644 --- a/internal/bootstrap/bootstrap_test.go +++ b/internal/bootstrap/bootstrap_test.go @@ -171,8 +171,8 @@ func TestRunSuccess(t *testing.T) { if err != nil { t.Fatalf("Run() error = %v", err) } - if got.ID != testID || got.Passkey != testPasskey { - t.Fatalf("Run() = %v with passkey match %v, want id %s and the fake's passkey", got, got.Passkey == testPasskey, testID) + if got.ID != testID || got.Passkey() != testPasskey { + t.Fatalf("Run() = %v with passkey match %v, want id %s and the fake's passkey", got, got.Passkey() == testPasskey, testID) } // The server regenerated the credentials, so there is nothing to warn about. if logs.Len() != 0 { @@ -212,7 +212,7 @@ func TestRunWarnsWhenServerDoesNotRegenerate(t *testing.T) { if !strings.Contains(logs.String(), "did not regenerate") { t.Fatalf("no regeneration warning logged; logs: %s", logs.String()) } - if strings.Contains(logs.String(), got.Passkey) { + if strings.Contains(logs.String(), got.Passkey()) { t.Fatalf("warning leaks the passkey: %s", logs.String()) } } diff --git a/internal/bootstrap/credentials.go b/internal/bootstrap/credentials.go index 85807e9..783610d 100644 --- a/internal/bootstrap/credentials.go +++ b/internal/bootstrap/credentials.go @@ -12,21 +12,33 @@ import ( const redacted = "REDACTED" // Credentials are the session id and passkey returned by etterminal. The id is -// not secret and stays visible for debugging; the passkey is replaced by -// REDACTED in every rendering the type controls: fmt with any verb (Format), -// encoding/json (MarshalJSON), text encoders such as encoding/xml -// (MarshalText) and slog (LogValue). Those methods also run when Credentials -// sits behind a pointer or inside an exported field, a slice or a map. +// not secret and stays visible for debugging. The passkey sits behind an +// unexported pointer, read with Passkey: renderings that reach it by reflection +// (fmt of a Credentials held in an unexported field, %p, encoding/gob, +// encoding/json of the raw fields) see only an address or nothing. Format, +// MarshalJSON, MarshalText and LogValue add a readable form with the passkey +// replaced by REDACTED. // -// Known renderings that bypass every method and print the passkey: fmt formatting a -// Credentials held in an unexported struct field (fmt cannot call methods on -// it), and %p applied to a Credentials value (fmt handles %p before it looks -// for any method, and go vet does not flag it), and encoding/gob, which encodes -// the exported fields directly. Keep Credentials in exported fields, never -// format it with %p, and never gob-encode it. +// Hold Credentials in a named field, not embedded: embedding promotes Format +// and the marshalers, so the outer struct would print and encode as the +// credentials alone. Credentials hold a pointer, so == compares identity; +// compare ID and Passkey() instead. type Credentials struct { ID string - Passkey string + passkey *string +} + +// NewCredentials returns Credentials holding id and passkey. +func NewCredentials(id, passkey string) Credentials { + return Credentials{ID: id, passkey: &passkey} +} + +// Passkey returns the session passkey, or "" for the zero value. +func (c Credentials) Passkey() string { + if c.passkey == nil { + return "" + } + return *c.passkey } // String returns the redacted form, for callers that want it as a string. @@ -38,7 +50,7 @@ func (c Credentials) String() string { // prints it as a Go literal. func (c Credentials) Format(f fmt.State, verb rune) { if verb == 'v' && f.Flag('#') { - _, _ = fmt.Fprint(f, `bootstrap.Credentials{ID:"`+c.ID+`", Passkey:"`+redacted+`"}`) + _, _ = fmt.Fprintf(f, "bootstrap.Credentials{ID:%q, Passkey:%q}", c.ID, redacted) return } _, _ = fmt.Fprint(f, c.String()) diff --git a/internal/bootstrap/credentials_test.go b/internal/bootstrap/credentials_test.go index 3dd19e0..1be51c4 100644 --- a/internal/bootstrap/credentials_test.go +++ b/internal/bootstrap/credentials_test.go @@ -2,6 +2,7 @@ package bootstrap import ( "bytes" + "encoding/gob" "encoding/json" "encoding/xml" "fmt" @@ -18,8 +19,11 @@ const ( ) func TestCredentialsRedaction(t *testing.T) { - c := Credentials{ID: testID, Passkey: testPasskey} + c := NewCredentials(testID, testPasskey) type holder struct{ Creds Credentials } + // fmt cannot call methods on a value held in an unexported field, so only + // the unexported pointer keeps the passkey out of this rendering. + type hidden struct{ creds Credentials } mustJSON := func(v any) string { b, err := json.Marshal(v) @@ -35,6 +39,13 @@ func TestCredentialsRedaction(t *testing.T) { } return string(b) } + mustGob := func(v any) string { + var b bytes.Buffer + if err := gob.NewEncoder(&b).Encode(v); err != nil { + t.Fatalf("gob: %v", err) + } + return b.String() + } slogOut := func(h func(*bytes.Buffer) slog.Handler, args ...any) string { var buf bytes.Buffer slog.New(h(&buf)).Info("got", args...) @@ -74,6 +85,15 @@ func TestCredentialsRedaction(t *testing.T) { "slog text nested": slogOut(textHandler, "h", holder{c}), "slog json pointer": slogOut(jsonHandler, "credentials", &c), } + // Renderings that bypass every method: they must still not show the + // passkey, though they show neither REDACTED nor always the id. + bypassing := map[string]string{ + "unexported field %+v": fmt.Sprintf("creds %+v", hidden{c}), + "unexported field %#v": fmt.Sprintf("creds %#v", hidden{c}), + "%p on a value": fmt.Sprintf("creds %p", c), + "gob": mustGob(c), + "slog text hidden": slogOut(textHandler, "h", hidden{c}), + } for name, s := range rendered { if strings.Contains(s, testPasskey) { t.Errorf("%s leaks the passkey: %s", name, s) @@ -82,14 +102,36 @@ func TestCredentialsRedaction(t *testing.T) { t.Errorf("%s = %s, want the id and %q", name, s, redacted) } } + for name, s := range bypassing { + if strings.Contains(s, testPasskey) { + t.Errorf("%s leaks the passkey: %s", name, s) + } + } + // A LogValuer resolves to a group; without LogValue the JSON handler would + // fall back to MarshalJSON and the checks above could not tell. + if kind := slog.AnyValue(c).Resolve().Kind(); kind != slog.KindGroup { + t.Fatalf("slog value of Credentials resolves to %v, want a group from LogValue", kind) + } if got := rendered["slog json"]; !strings.Contains(got, `"credentials":{"id":"`+testID+`","passkey":"REDACTED"}`) { t.Fatalf("slog JSON = %s, want a redacted credentials group", got) } if got, want := rendered["%#v"], `creds bootstrap.Credentials{ID:"`+testID+`", Passkey:"REDACTED"}`; got != want { t.Fatalf("%%#v = %s, want %s", got, want) } + if got, want := fmt.Sprintf("%#v", NewCredentials(`a"b`, testPasskey)), `bootstrap.Credentials{ID:"a\"b", Passkey:"REDACTED"}`; got != want { + t.Fatalf("%%#v of an id with a quote = %s, want %s", got, want) + } if got, want := rendered["json"], `{"id":"`+testID+`","passkey":"REDACTED"}`; got != want { t.Fatalf("json = %s, want %s", got, want) } } + +func TestCredentialsPasskey(t *testing.T) { + if got := NewCredentials(testID, testPasskey).Passkey(); got != testPasskey { + t.Fatalf("Passkey() = %q, want the passkey given to NewCredentials", got) + } + if got := (Credentials{}).Passkey(); got != "" { + t.Fatalf("zero Credentials Passkey() = %q, want empty", got) + } +} diff --git a/internal/bootstrap/parse.go b/internal/bootstrap/parse.go index f655451..55d97f8 100644 --- a/internal/bootstrap/parse.go +++ b/internal/bootstrap/parse.go @@ -30,7 +30,7 @@ func parseCredentials(out []byte) (Credentials, error) { // Do not echo the value: a malformed passkey may still be a real one. return Credentials{}, fmt.Errorf("%w: malformed passkey after %q", ErrNoCredentials, marker) } - return Credentials{ID: string(id), Passkey: string(passkey)}, nil + return NewCredentials(string(id), string(passkey)), nil } // alnumRun returns the leading run of ASCII letters and digits in b and its length. diff --git a/internal/bootstrap/parse_test.go b/internal/bootstrap/parse_test.go index 38e89b1..e222cce 100644 --- a/internal/bootstrap/parse_test.go +++ b/internal/bootstrap/parse_test.go @@ -13,11 +13,11 @@ func TestParseCredentials(t *testing.T) { want Credentials wantErr string // "" means success; otherwise a substring the error must contain }{ - {"plain", "IDPASSKEY:" + testID + "/" + testPasskey + "\n", Credentials{testID, testPasskey}, ""}, - {"crlf", "IDPASSKEY:" + testID + "/" + testPasskey + "\r\n", Credentials{testID, testPasskey}, ""}, - {"no trailing newline", "IDPASSKEY:" + testID + "/" + testPasskey, Credentials{testID, testPasskey}, ""}, - {"noise before", "Last login: Thu\nWelcome!\nIDPASSKEY:" + testID + "/" + testPasskey + "\n", Credentials{testID, testPasskey}, ""}, - {"first marker wins", "IDPASSKEY:" + testID + "/" + testPasskey + "\nIDPASSKEY:zzzzzzzzzzzzzzzz/" + testPasskey, Credentials{testID, testPasskey}, ""}, + {"plain", "IDPASSKEY:" + testID + "/" + testPasskey + "\n", NewCredentials(testID, testPasskey), ""}, + {"crlf", "IDPASSKEY:" + testID + "/" + testPasskey + "\r\n", NewCredentials(testID, testPasskey), ""}, + {"no trailing newline", "IDPASSKEY:" + testID + "/" + testPasskey, NewCredentials(testID, testPasskey), ""}, + {"noise before", "Last login: Thu\nWelcome!\nIDPASSKEY:" + testID + "/" + testPasskey + "\n", NewCredentials(testID, testPasskey), ""}, + {"first marker wins", "IDPASSKEY:" + testID + "/" + testPasskey + "\nIDPASSKEY:zzzzzzzzzzzzzzzz/" + testPasskey, NewCredentials(testID, testPasskey), ""}, {"no marker", "Welcome!\n", Credentials{}, "no IDPASSKEY"}, {"empty", "", Credentials{}, "no IDPASSKEY"}, {"truncated id", "IDPASSKEY:abcd", Credentials{}, "malformed id"}, @@ -35,7 +35,9 @@ func TestParseCredentials(t *testing.T) { t.Run(tt.name, func(t *testing.T) { got, err := parseCredentials([]byte(tt.out)) if tt.wantErr == "" { - if err != nil || got != tt.want { + // want's passkey is read from the field, not through Passkey, + // so a broken accessor cannot make both sides agree. + if err != nil || got.ID != tt.want.ID || got.Passkey() != *tt.want.passkey { t.Fatalf("parseCredentials() = %v, %v; want %v, nil", got, err, tt.want) } return From e781a337fed351789fa976773eb2c75dee33615b Mon Sep 17 00:00:00 2001 From: "Tomi P. Hakala" <7030001+tphakala@users.noreply.github.com> Date: Fri, 25 Sep 2026 18:22:23 +0300 Subject: [PATCH 11/15] test(bootstrap): pin the nil Logger default on the warning path No test reached the no-regeneration warning with a nil Logger, so removing the discard default would have shipped a nil pointer panic for callers that leave Logger unset. The new test runs that path with a nil Logger; removing the default makes it fail with a message instead of passing. --- internal/bootstrap/bootstrap_test.go | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/internal/bootstrap/bootstrap_test.go b/internal/bootstrap/bootstrap_test.go index bda7e48..820bff1 100644 --- a/internal/bootstrap/bootstrap_test.go +++ b/internal/bootstrap/bootstrap_test.go @@ -217,6 +217,21 @@ func TestRunWarnsWhenServerDoesNotRegenerate(t *testing.T) { } } +// TestRunNilLoggerOnWarningPath pins the documented nil-Logger default on the +// only path that logs. +func TestRunNilLoggerOnWarningPath(t *testing.T) { + cfg, _ := useFakeSSH(t, "echo-sent") + cfg.Logger = nil + defer func() { + if r := recover(); r != nil { + t.Fatalf("Run() with a nil Logger panicked on the no-regeneration warning: %v", r) + } + }() + if _, err := Run(t.Context(), cfg); err != nil { + t.Fatalf("Run() error = %v", err) + } +} + func TestRunFailures(t *testing.T) { const ( missingHint = "--terminal-path" From 1901cf4aa7578f73afb803e4c13e5559652179d0 Mon Sep 17 00:00:00 2001 From: "Tomi P. Hakala" <7030001+tphakala@users.noreply.github.com> Date: Fri, 25 Sep 2026 18:22:59 +0300 Subject: [PATCH 12/15] docs(bootstrap): state waitDelay's second role and tag upstream citations waitDelay also bounds the wait for ssh's stdout to close after ssh exits, which the ErrWaitDelay branch relies on; the comment now says so. The two TerminalMain.cpp line citations name et-v7.0.0, since upstream master has moved. The exit1 failure row now requires the message to end with its status, because "status 1" also matched "status 127". Verified with vet, golangci-lint on linux and windows and go test -race. --- internal/bootstrap/bootstrap.go | 4 +++- internal/bootstrap/bootstrap_test.go | 5 +++++ internal/bootstrap/parse.go | 2 +- internal/bootstrap/placeholder.go | 2 +- 4 files changed, 10 insertions(+), 3 deletions(-) diff --git a/internal/bootstrap/bootstrap.go b/internal/bootstrap/bootstrap.go index 30c4337..3d00fda 100644 --- a/internal/bootstrap/bootstrap.go +++ b/internal/bootstrap/bootstrap.go @@ -28,7 +28,9 @@ const ( maxOutput = 1 << 20 // excerptLen caps the output excerpt quoted in an error. excerptLen = 512 - // waitDelay is how long Run waits after cancelling ssh before killing it. + // waitDelay is how long Run waits after cancelling ssh before killing it, + // and how long it waits for ssh's stdout to close after ssh exits; the + // exec.ErrWaitDelay branch in Run depends on the latter. waitDelay = 2 * time.Second ) diff --git a/internal/bootstrap/bootstrap_test.go b/internal/bootstrap/bootstrap_test.go index 820bff1..8826b52 100644 --- a/internal/bootstrap/bootstrap_test.go +++ b/internal/bootstrap/bootstrap_test.go @@ -267,6 +267,11 @@ func TestRunFailures(t *testing.T) { t.Errorf("error %q mentions %q, which does not apply", err, s) } } + // "status 1" alone would also match "status 127"; exit1 prints no + // output and gets no hint, so its status ends the message. + if tt.mode == "exit1" && !strings.HasSuffix(err.Error(), "status 1") { + t.Errorf("error %q does not end with the exit status", err) + } // The marker ends in ':', so printing it bare before ": ssh exited" // would render a confusing "IDPASSKEY::". if strings.Contains(err.Error(), "::") { diff --git a/internal/bootstrap/parse.go b/internal/bootstrap/parse.go index 55d97f8..6af37b1 100644 --- a/internal/bootstrap/parse.go +++ b/internal/bootstrap/parse.go @@ -6,7 +6,7 @@ import ( ) // marker precedes the credentials in etterminal's output -// (upstream src/terminal/TerminalMain.cpp:185). +// (upstream src/terminal/TerminalMain.cpp:185 at et-v7.0.0). const marker = "IDPASSKEY:" // parseCredentials finds the first IDPASSKEY marker in out and returns the diff --git a/internal/bootstrap/placeholder.go b/internal/bootstrap/placeholder.go index 8d8a39b..53a820b 100644 --- a/internal/bootstrap/placeholder.go +++ b/internal/bootstrap/placeholder.go @@ -9,7 +9,7 @@ const ( passkeyLen = 32 // regeneratePrefix makes etterminal discard the id and passkey it is sent - // and generate fresh ones (upstream src/terminal/TerminalMain.cpp:114-120). + // and generate fresh ones (upstream src/terminal/TerminalMain.cpp:114-120 at et-v7.0.0). regeneratePrefix = "XXX" ) From 8133b943faee286a10a9e578083d21269d05b405 Mon Sep 17 00:00:00 2001 From: "Tomi P. Hakala" <7030001+tphakala@users.noreply.github.com> Date: Fri, 25 Sep 2026 18:31:03 +0300 Subject: [PATCH 13/15] test(bootstrap): take expected values from outside the code under test The warning test compared the log against the passkey returned by the Credentials under test, and the parse table compared against Credentials built by the same constructor, so a broken accessor or constructor could make both sides agree or panic. Both now compare with values the code under test did not produce: the command line the fake ssh received, and the test constants. The fake's lingering child now outlasts waitDelay, so the linger row always goes through the ErrWaitDelay branch, and the Format doc says fmt handles %T and %p itself. Verified by making NewCredentials drop the passkey (both tests fail on assertions) and by raising waitDelay to 10s (the linger row still reaches ErrWaitDelay). --- internal/bootstrap/bootstrap_test.go | 20 +++++++----- internal/bootstrap/credentials.go | 4 +-- internal/bootstrap/parse_test.go | 46 ++++++++++++++-------------- 3 files changed, 38 insertions(+), 32 deletions(-) diff --git a/internal/bootstrap/bootstrap_test.go b/internal/bootstrap/bootstrap_test.go index 8826b52..20df07d 100644 --- a/internal/bootstrap/bootstrap_test.go +++ b/internal/bootstrap/bootstrap_test.go @@ -94,9 +94,10 @@ func runFakeSSH(mode string, args []string) int { return 0 case "hold": // Keeps writing to the inherited stdout and exits on the first failed - // write, which comes as soon as the reader closes the pipe; the loop - // bound stops it even if nobody does. - for range 50 { + // write, which comes as soon as the reader closes the pipe. The loop + // outlasts waitDelay, so only Run's WaitDelay can end the "linger" run + // early, and it is bounded so nothing outlives the test by much. + for range 3 * waitDelay / (100 * time.Millisecond) { if _, err := os.Stdout.WriteString("."); err != nil { return 0 } @@ -205,14 +206,19 @@ func TestRunWarnsWhenServerDoesNotRegenerate(t *testing.T) { if err != nil { t.Fatalf("Run() error = %v", err) } - sent := regexp.MustCompile(`^echo '([A-Z2-7]{16})/`).FindStringSubmatch(readArgv(t, argvPath)[2]) - if len(sent) != 2 || got.ID != sent[1] { - t.Fatalf("Run() id = %q, want the placeholder sent in the remote command (%q)", got.ID, sent) + // The expected values come from the command line the fake received, not + // from the Credentials under test, so a broken accessor cannot hide a leak. + sent := regexp.MustCompile(`^echo '([A-Z2-7]{16})/([A-Z2-7]{32})_`).FindStringSubmatch(readArgv(t, argvPath)[2]) + if len(sent) != 3 { + t.Fatalf("remote command %q does not carry a placeholder id and passkey", readArgv(t, argvPath)[2]) + } + if got.ID != sent[1] || got.Passkey() != sent[2] { + t.Fatalf("Run() = %v with passkey match %v, want the placeholder sent in the remote command", got, got.Passkey() == sent[2]) } if !strings.Contains(logs.String(), "did not regenerate") { t.Fatalf("no regeneration warning logged; logs: %s", logs.String()) } - if strings.Contains(logs.String(), got.Passkey()) { + if strings.Contains(logs.String(), sent[2]) { t.Fatalf("warning leaks the passkey: %s", logs.String()) } } diff --git a/internal/bootstrap/credentials.go b/internal/bootstrap/credentials.go index 783610d..086520b 100644 --- a/internal/bootstrap/credentials.go +++ b/internal/bootstrap/credentials.go @@ -46,8 +46,8 @@ func (c Credentials) String() string { return "{ID:" + c.ID + " Passkey:" + redacted + "}" } -// Format implements fmt.Formatter so every verb prints the redacted form; %#v -// prints it as a Go literal. +// Format implements fmt.Formatter so every verb fmt passes to it prints the +// redacted form (fmt handles %T and %p itself); %#v prints it as a Go literal. func (c Credentials) Format(f fmt.State, verb rune) { if verb == 'v' && f.Flag('#') { _, _ = fmt.Fprintf(f, "bootstrap.Credentials{ID:%q, Passkey:%q}", c.ID, redacted) diff --git a/internal/bootstrap/parse_test.go b/internal/bootstrap/parse_test.go index e222cce..0fa7ece 100644 --- a/internal/bootstrap/parse_test.go +++ b/internal/bootstrap/parse_test.go @@ -10,35 +10,35 @@ func TestParseCredentials(t *testing.T) { tests := []struct { name string out string - want Credentials - wantErr string // "" means success; otherwise a substring the error must contain + wantErr string // "" means success with testID and testPasskey; otherwise a substring the error must contain }{ - {"plain", "IDPASSKEY:" + testID + "/" + testPasskey + "\n", NewCredentials(testID, testPasskey), ""}, - {"crlf", "IDPASSKEY:" + testID + "/" + testPasskey + "\r\n", NewCredentials(testID, testPasskey), ""}, - {"no trailing newline", "IDPASSKEY:" + testID + "/" + testPasskey, NewCredentials(testID, testPasskey), ""}, - {"noise before", "Last login: Thu\nWelcome!\nIDPASSKEY:" + testID + "/" + testPasskey + "\n", NewCredentials(testID, testPasskey), ""}, - {"first marker wins", "IDPASSKEY:" + testID + "/" + testPasskey + "\nIDPASSKEY:zzzzzzzzzzzzzzzz/" + testPasskey, NewCredentials(testID, testPasskey), ""}, - {"no marker", "Welcome!\n", Credentials{}, "no IDPASSKEY"}, - {"empty", "", Credentials{}, "no IDPASSKEY"}, - {"truncated id", "IDPASSKEY:abcd", Credentials{}, "malformed id"}, - {"id at end of output", "IDPASSKEY:" + testID, Credentials{}, "malformed id"}, - {"colon instead of slash", "IDPASSKEY:" + testID + ":" + testPasskey, Credentials{}, "malformed id"}, - {"truncated passkey", "IDPASSKEY:" + testID + "/0123", Credentials{}, "malformed passkey"}, - {"id too long", "IDPASSKEY:" + testID + "X/" + testPasskey, Credentials{}, "malformed id"}, - {"id too short", "IDPASSKEY:" + testID[1:] + "/" + testPasskey, Credentials{}, "malformed id"}, - {"passkey too long", "IDPASSKEY:" + testID + "/" + testPasskey + "X\n", Credentials{}, "malformed passkey"}, - {"missing slash", "IDPASSKEY:" + testID + testPasskey, Credentials{}, "malformed id"}, - {"non alphanumeric in id", "IDPASSKEY:abcdEFGH1234567-/" + testPasskey, Credentials{}, "malformed id"}, - {"non alphanumeric in passkey", "IDPASSKEY:" + testID + "/0123456789abcdef_BCDEF0123456789", Credentials{}, "malformed passkey"}, + {"plain", "IDPASSKEY:" + testID + "/" + testPasskey + "\n", ""}, + {"crlf", "IDPASSKEY:" + testID + "/" + testPasskey + "\r\n", ""}, + {"no trailing newline", "IDPASSKEY:" + testID + "/" + testPasskey, ""}, + {"noise before", "Last login: Thu\nWelcome!\nIDPASSKEY:" + testID + "/" + testPasskey + "\n", ""}, + {"first marker wins", "IDPASSKEY:" + testID + "/" + testPasskey + "\nIDPASSKEY:zzzzzzzzzzzzzzzz/" + testPasskey, ""}, + {"no marker", "Welcome!\n", "no IDPASSKEY"}, + {"empty", "", "no IDPASSKEY"}, + {"truncated id", "IDPASSKEY:abcd", "malformed id"}, + {"id at end of output", "IDPASSKEY:" + testID, "malformed id"}, + {"colon instead of slash", "IDPASSKEY:" + testID + ":" + testPasskey, "malformed id"}, + {"truncated passkey", "IDPASSKEY:" + testID + "/0123", "malformed passkey"}, + {"id too long", "IDPASSKEY:" + testID + "X/" + testPasskey, "malformed id"}, + {"id too short", "IDPASSKEY:" + testID[1:] + "/" + testPasskey, "malformed id"}, + {"passkey too long", "IDPASSKEY:" + testID + "/" + testPasskey + "X\n", "malformed passkey"}, + {"missing slash", "IDPASSKEY:" + testID + testPasskey, "malformed id"}, + {"non alphanumeric in id", "IDPASSKEY:abcdEFGH1234567-/" + testPasskey, "malformed id"}, + {"non alphanumeric in passkey", "IDPASSKEY:" + testID + "/0123456789abcdef_BCDEF0123456789", "malformed passkey"}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { got, err := parseCredentials([]byte(tt.out)) if tt.wantErr == "" { - // want's passkey is read from the field, not through Passkey, - // so a broken accessor cannot make both sides agree. - if err != nil || got.ID != tt.want.ID || got.Passkey() != *tt.want.passkey { - t.Fatalf("parseCredentials() = %v, %v; want %v, nil", got, err, tt.want) + // Compared with the constants, so a broken constructor or + // accessor cannot make both sides agree. + if err != nil || got.ID != testID || got.Passkey() != testPasskey { + t.Fatalf("parseCredentials() = %v with passkey match %v, %v; want id %s, the test passkey, nil", + got, got.Passkey() == testPasskey, err, testID) } return } From acc806f5057171c585078dde123820cadcd9cd69 Mon Sep 17 00:00:00 2001 From: "Tomi P. Hakala" <7030001+tphakala@users.noreply.github.com> Date: Fri, 25 Sep 2026 18:56:10 +0300 Subject: [PATCH 14/15] fix(bootstrap): let cancellation win and keep the passkey out of failure output Run returned credentials after the caller cancelled when ssh had already printed them, so a Ctrl+C during bootstrap could still go on to connect; a cancelled ctx now wins. The failure excerpt was cut only at the first IDPASSKEY marker, so a remote side echoing the command line before failing put the generated passkey into the error, and against a server that does not regenerate that is the session passkey. The output is now passed through redactSecret, which removes the passkey and every 8-byte piece of it, so a copy split across lines or cut short is removed too. Verified with fake ssh modes that print credentials and hang, and that echo the command and fail, plus a table test of redactSecret with exact expected output; reverting each fix, stepping the window loop by 4, or wiping the whole output each turns a test red. --- internal/bootstrap/bootstrap.go | 32 +++++++++- internal/bootstrap/bootstrap_test.go | 89 ++++++++++++++++++++++++++++ 2 files changed, 119 insertions(+), 2 deletions(-) diff --git a/internal/bootstrap/bootstrap.go b/internal/bootstrap/bootstrap.go index 3d00fda..95af50b 100644 --- a/internal/bootstrap/bootstrap.go +++ b/internal/bootstrap/bootstrap.go @@ -54,6 +54,8 @@ type Config struct { // ssh's exit status does not decide success: etterminal daemonises after // printing its credentials, so ssh can exit 0 or not regardless // (upstream src/terminal/TerminalMain.cpp:185-188, MEASURED against etserver 7.0.0). +// Cancellation does: once ctx is done, Run returns its error even if the +// credentials had already been printed. // //nolint:gocritic // hugeParam: Config is taken by value on purpose so Run fills defaults on its own copy and never mutates the caller's; bootstrap runs once per session, not on a hot path. func Run(ctx context.Context, cfg Config) (Credentials, error) { @@ -92,7 +94,9 @@ func Run(ctx context.Context, cfg Config) (Credentials, error) { runErr := cmd.Run() creds, parseErr := parseCredentials(out.Bytes()) - if parseErr == nil { + // A cancelled ctx wins even over credentials that already arrived: the + // caller asked to stop, so it must not go on to connect. + if parseErr == nil && ctx.Err() == nil { if creds.ID == id { logger.Warn("etterminal did not regenerate the session id; the passkey in use was visible in the ssh command line on both hosts", "credentials", creds) @@ -108,7 +112,31 @@ func Run(ctx context.Context, cfg Config) (Credentials, error) { if runErr != nil && !isExit && !errors.Is(runErr, exec.ErrWaitDelay) { return Credentials{}, fmt.Errorf("bootstrap: run %s: %w", sshPath, runErr) } - return Credentials{}, describeFailure(exitErr, parseErr, out.Bytes()) + // The remote side may echo the command it ran (shell tracing, a + // diagnostic) before any marker, and against a server that does not + // regenerate, the generated passkey is the session passkey, so it is + // redacted before any output can be quoted. + return Credentials{}, describeFailure(exitErr, parseErr, redactSecret(out.Bytes(), passkey)) +} + +// minFragment is the shortest piece of a secret redactSecret removes. +const minFragment = 8 + +// redactSecret replaces secret in out, and every minFragment-byte piece of it, +// so a copy split across lines or cut short is removed too. Only pieces +// shorter than minFragment bytes can survive (the tail of a longer fragment, +// or a whole short one), far too little to recover a 32-character passkey. +func redactSecret(out []byte, secret string) []byte { + if secret == "" { + // bytes.ReplaceAll with an empty old value would insert the + // replacement between every byte. + return out + } + out = bytes.ReplaceAll(out, []byte(secret), []byte(redacted)) + for i := 0; i+minFragment <= len(secret); i++ { + out = bytes.ReplaceAll(out, []byte(secret[i:i+minFragment]), []byte(redacted)) + } + return out } // describeFailure builds the single error the user sees when ssh finished diff --git a/internal/bootstrap/bootstrap_test.go b/internal/bootstrap/bootstrap_test.go index 20df07d..0884755 100644 --- a/internal/bootstrap/bootstrap_test.go +++ b/internal/bootstrap/bootstrap_test.go @@ -82,6 +82,11 @@ func runFakeSSH(mode string, args []string) int { case "exit1": // A remote failure with some other status and no output. return 1 + case "echo-cmd": + // A remote side that echoes the command it ran (shell tracing, a + // diagnostic) and then fails, before any IDPASSKEY marker. + fmt.Println("+ " + args[len(args)-1]) + return 1 case "linger": // ssh exits 0 without credentials while a descendant still holds its // stdout, so Run's WaitDelay has to close the pipe. @@ -107,6 +112,15 @@ func runFakeSSH(mode string, args []string) int { case "hang": time.Sleep(time.Hour) return 0 + case "ok-hang": + // Credentials arrive but ssh keeps running until it is cancelled. The + // signaled file records that they were printed. + fmt.Print(idpasskey) + if err := os.WriteFile(os.Getenv(fakeSSHSignaledEnv), nil, 0o600); err != nil { + return 97 + } + time.Sleep(time.Hour) + return 0 case "trap-int": // Survives os.Interrupt and records that it arrived, so only the // WaitDelay kill can end it. Opening the FIFO tells the test that the @@ -287,6 +301,57 @@ func TestRunFailures(t *testing.T) { } } +// TestRunFailureRedactsPlaceholder: output that echoes the remote command +// carries the generated passkey before any marker, and the error excerpt must +// not quote it (against a server that does not regenerate, it is the session +// passkey). +func TestRunFailureRedactsPlaceholder(t *testing.T) { + cfg, argvPath := useFakeSSH(t, "echo-cmd") + _, err := Run(t.Context(), cfg) + if !errors.Is(err, ErrNoCredentials) { + t.Fatalf("Run() error = %v, want ErrNoCredentials", err) + } + sent := regexp.MustCompile(`^echo '([A-Z2-7]{16})/([A-Z2-7]{32})_`).FindStringSubmatch(readArgv(t, argvPath)[2]) + if len(sent) != 3 { + t.Fatalf("remote command %q does not carry a placeholder id and passkey", readArgv(t, argvPath)[2]) + } + if strings.Contains(err.Error(), sent[2]) { + t.Fatalf("error quotes the generated passkey: %v", err) + } + // The rest of the echoed line stays, so the excerpt still helps. + if !strings.Contains(err.Error(), redacted) || !strings.Contains(err.Error(), "etterminal --verbose=0") { + t.Fatalf("error %q lost the echoed command around the redaction", err) + } +} + +func TestRedactSecret(t *testing.T) { + // A secret with no repeated 8-byte window, so each expected value follows + // from the window rule alone; the values are written out so a redaction + // that also eats surrounding text fails. + const secret = "ABCDEFGHIJKLMNOPQRSTUVWXYZ234567" + tests := []struct { + name, out, want string + }{ + {"whole", "x " + secret + " y", "x REDACTED y"}, + {"twice", secret + secret, "REDACTEDREDACTED"}, + {"split across lines", "x " + secret[:20] + "\n" + secret[20:] + " y", "x REDACTEDREDACTEDQRST\nREDACTED4567 y"}, + {"cut short", "x " + secret[:10], "x REDACTEDIJ"}, + {"unaligned piece", "x " + secret[3:15] + " y", "x REDACTEDLMNO y"}, + {"short piece survives", "x " + secret[:5] + " y", "x ABCDE y"}, + {"unrelated", "Welcome to the server", "Welcome to the server"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := string(redactSecret([]byte(tt.out), secret)); got != tt.want { + t.Fatalf("redactSecret(%q) = %q, want %q", tt.out, got, tt.want) + } + }) + } + if got := string(redactSecret([]byte("abc"), "")); got != "abc" { + t.Fatalf("redactSecret with an empty secret = %q, want the output unchanged", got) + } +} + func TestRunCancel(t *testing.T) { cfg, _ := useFakeSSH(t, "hang") ctx, cancel := context.WithTimeout(t.Context(), 200*time.Millisecond) @@ -302,6 +367,30 @@ func TestRunCancel(t *testing.T) { } } +// TestRunCancelWinsOverCredentials: when the caller cancels while ssh is still +// running, Run reports the cancellation even if the credentials already +// arrived, so a Ctrl+C during bootstrap never goes on to connect. +func TestRunCancelWinsOverCredentials(t *testing.T) { + cfg, _ := useFakeSSH(t, "ok-hang") + printed := filepath.Join(t.TempDir(), "printed") + t.Setenv(fakeSSHSignaledEnv, printed) + ctx, cancel := context.WithTimeout(t.Context(), 500*time.Millisecond) + defer cancel() + + creds, err := Run(ctx, cfg) + if _, statErr := os.Stat(printed); statErr != nil { + // Without printed credentials the run never reaches the branch under + // test; say so instead of passing without exercising it. + t.Skipf("the fake did not print credentials before the deadline (%v); the host is too slow for this test", statErr) + } + if !errors.Is(err, context.DeadlineExceeded) { + t.Fatalf("Run() = %v, %v; want context.DeadlineExceeded", creds, err) + } + if creds.ID != "" || creds.Passkey() != "" { + t.Fatalf("Run() returned credentials %v alongside the cancellation", creds) + } +} + func TestRunInvalidConfigDoesNotStartSSH(t *testing.T) { cfg, argvPath := useFakeSSH(t, "ok") cfg.Term = "xterm'; id; '" From 456edcda9946cc33a8df0e11bf684b7ebc3fdf25 Mon Sep 17 00:00:00 2001 From: "Tomi P. Hakala" <7030001+tphakala@users.noreply.github.com> Date: Fri, 25 Sep 2026 19:06:48 +0300 Subject: [PATCH 15/15] fix(bootstrap): withhold failure output that holds any piece of the passkey Redacting 8-byte windows of the generated passkey still let a copy wrapped into shorter chunks reach the error excerpt, and no window size scrubs every split reliably. Output that contains any 4-byte piece of the passkey is now withheld as a whole, replaced by a note; the exit status and hints are still reported. Only output wrapped at 3 columns or fewer could slip past, and ordinary banners do not trigger it because the passkey is uppercase base32. Verified with TestContainsPiece (wrapped at 7 and at 4, unaligned and last pieces, shorter pieces, unrelated text) and the echoing fake ssh; making the check a no-op, or stepping its loop by 2 with a short bound, turns them red. --- internal/bootstrap/bootstrap.go | 41 ++++++++++++----------- internal/bootstrap/bootstrap_test.go | 50 ++++++++++++++++------------ 2 files changed, 50 insertions(+), 41 deletions(-) diff --git a/internal/bootstrap/bootstrap.go b/internal/bootstrap/bootstrap.go index 95af50b..25a9961 100644 --- a/internal/bootstrap/bootstrap.go +++ b/internal/bootstrap/bootstrap.go @@ -114,29 +114,32 @@ func Run(ctx context.Context, cfg Config) (Credentials, error) { } // The remote side may echo the command it ran (shell tracing, a // diagnostic) before any marker, and against a server that does not - // regenerate, the generated passkey is the session passkey, so it is - // redacted before any output can be quoted. - return Credentials{}, describeFailure(exitErr, parseErr, redactSecret(out.Bytes(), passkey)) + // regenerate, the generated passkey is the session passkey. Output that + // holds any piece of it cannot be scrubbed reliably (it may be split or + // wrapped), so it is not quoted at all. + shown := out.Bytes() + if containsPiece(shown, passkey) { + shown = []byte(withheldOutput) + } + return Credentials{}, describeFailure(exitErr, parseErr, shown) } -// minFragment is the shortest piece of a secret redactSecret removes. -const minFragment = 8 +const ( + // minPiece is the shortest piece of the passkey containsPiece looks for. + // Only output wrapped at fewer columns than this could slip past it. + minPiece = 4 + // withheldOutput replaces an excerpt that would quote passkey material. + withheldOutput = "[withheld: the output contains part of the generated session key]" +) -// redactSecret replaces secret in out, and every minFragment-byte piece of it, -// so a copy split across lines or cut short is removed too. Only pieces -// shorter than minFragment bytes can survive (the tail of a longer fragment, -// or a whole short one), far too little to recover a 32-character passkey. -func redactSecret(out []byte, secret string) []byte { - if secret == "" { - // bytes.ReplaceAll with an empty old value would insert the - // replacement between every byte. - return out - } - out = bytes.ReplaceAll(out, []byte(secret), []byte(redacted)) - for i := 0; i+minFragment <= len(secret); i++ { - out = bytes.ReplaceAll(out, []byte(secret[i:i+minFragment]), []byte(redacted)) +// containsPiece reports whether out holds any minPiece-byte piece of secret. +func containsPiece(out []byte, secret string) bool { + for i := 0; i+minPiece <= len(secret); i++ { + if bytes.Contains(out, []byte(secret[i:i+minPiece])) { + return true + } } - return out + return false } // describeFailure builds the single error the user sees when ssh finished diff --git a/internal/bootstrap/bootstrap_test.go b/internal/bootstrap/bootstrap_test.go index 0884755..f012c66 100644 --- a/internal/bootstrap/bootstrap_test.go +++ b/internal/bootstrap/bootstrap_test.go @@ -315,41 +315,47 @@ func TestRunFailureRedactsPlaceholder(t *testing.T) { if len(sent) != 3 { t.Fatalf("remote command %q does not carry a placeholder id and passkey", readArgv(t, argvPath)[2]) } - if strings.Contains(err.Error(), sent[2]) { - t.Fatalf("error quotes the generated passkey: %v", err) + if containsPiece([]byte(err.Error()), sent[2]) { + t.Fatalf("error quotes passkey material: %v", err) } - // The rest of the echoed line stays, so the excerpt still helps. - if !strings.Contains(err.Error(), redacted) || !strings.Contains(err.Error(), "etterminal --verbose=0") { - t.Fatalf("error %q lost the echoed command around the redaction", err) + // The output is withheld as a whole, but the status is still reported. + if !strings.Contains(err.Error(), withheldOutput) || !strings.Contains(err.Error(), "status 1") { + t.Fatalf("error %q does not say the output was withheld and give the status", err) } } -func TestRedactSecret(t *testing.T) { - // A secret with no repeated 8-byte window, so each expected value follows - // from the window rule alone; the values are written out so a redaction - // that also eats surrounding text fails. +func TestContainsPiece(t *testing.T) { const secret = "ABCDEFGHIJKLMNOPQRSTUVWXYZ234567" + // wrap splits s into lines of n bytes, as a narrow diagnostic might. + wrap := func(s string, n int) string { + var b strings.Builder + for len(s) > n { + b.WriteString(s[:n] + "\n") + s = s[n:] + } + return b.String() + s + } tests := []struct { - name, out, want string + name string + out string + want bool }{ - {"whole", "x " + secret + " y", "x REDACTED y"}, - {"twice", secret + secret, "REDACTEDREDACTED"}, - {"split across lines", "x " + secret[:20] + "\n" + secret[20:] + " y", "x REDACTEDREDACTEDQRST\nREDACTED4567 y"}, - {"cut short", "x " + secret[:10], "x REDACTEDIJ"}, - {"unaligned piece", "x " + secret[3:15] + " y", "x REDACTEDLMNO y"}, - {"short piece survives", "x " + secret[:5] + " y", "x ABCDE y"}, - {"unrelated", "Welcome to the server", "Welcome to the server"}, + {"whole", "x " + secret + " y", true}, + {"wrapped at 7", wrap(secret, 7), true}, + {"wrapped at minPiece", wrap(secret, minPiece), true}, + {"unaligned piece", "x " + secret[13:17] + " y", true}, + {"last piece", "x " + secret[len(secret)-minPiece:], true}, + {"shorter than minPiece", "x " + secret[:minPiece-1] + " y", false}, + {"unrelated", "Welcome to the server\nLast login: Thu", false}, + {"empty", "", false}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - if got := string(redactSecret([]byte(tt.out), secret)); got != tt.want { - t.Fatalf("redactSecret(%q) = %q, want %q", tt.out, got, tt.want) + if got := containsPiece([]byte(tt.out), secret); got != tt.want { + t.Fatalf("containsPiece(%q) = %v, want %v", tt.out, got, tt.want) } }) } - if got := string(redactSecret([]byte("abc"), "")); got != "abc" { - t.Fatalf("redactSecret with an empty secret = %q, want the output unchanged", got) - } } func TestRunCancel(t *testing.T) {