betterbird: never lose an unsent message, never answer an address, and keep the test apart (review of #155)

A restart ended Betterbird with whatever a composer held; now it refuses while a Msgcompose window is
open unless discard_unsent says the operator agreed, and answers within the runtime's 30 s. A
preference's value could carry an address, a user%40domain@host URI, a host or the home; those are
masked, recent-folder, account and cloud-file preferences and every add-on preference but the theme's
id are withheld, and a request for everything is refused. A log kept the token after Bearer or Basic.
The reminder test's Betterbird counted as the operator's and left its profile behind when it ended on
its own; it is now left out everywhere and its unit removes the profile when it stops.

modules/betterbird/cmd/betterbird-tools/betterbird.go

@@ -56,7 +56,8 @@ var readLink = os.Readlink
 func (m *Machine) mainProcs() []Proc {
 	var out []Proc
 	for _, p := range m.procs(mainComm) {
-		if !strings.Contains(p.Command, "-contentproc") {
+		// The reminder test's Betterbird is never the operator's: its command line names its own profile.
+		if !strings.Contains(p.Command, "-contentproc") && !strings.Contains(p.Command, testUnit) {
 			out = append(out, p)
 		}
 	}
@@ -255,11 +256,17 @@ var (
 	passwordSecret = regexp.MustCompile(`(?i)(password|passwd|secret|token|oauth|auth|cookie|credential|signon|apikey|api_key|privatekey|private_key|\.key$)`)
 	accountPrefix  = []string{"mail.server.", "mail.identity.", "mail.smtpserver.", "mail.smtp.", "mail.account.",
 		"mail.accountmanager.", "ldap_2.", "extensions.owl.", "calendar.registry.", "mail.root.", "mailnews.",
-		"services.sync.", "mail.displayname", "mail.default_"}
+		"services.sync.", "mail.displayname", "mail.default_", "mail.last_msg", "messenger.account.", "mail.cloud_files."}
+	// allowedExtensions are the only extensions.* preferences answered: the add-ons' own preferences can
+	// hold anything, and a theme's id is what a window's look depends on.
+	allowedExtensions = map[string]bool{"extensions.activeThemeID": true}
 )
 
 // withheld says why a preference's value is never answered, or "" when it may be.
 func withheld(name string) string {
+	if strings.HasPrefix(name, "extensions.") && !allowedExtensions[name] {
+		return "an add-on's own preference: only the theme's id (extensions.activeThemeID) is answered"
+	}
 	if passwordSecret.MatchString(name) {
 		return "its name says it can hold a credential"
 	}
@@ -288,6 +295,11 @@ func (m *Machine) Prefs(names []string) (map[string]any, error) {
 	if len(names) == 0 {
 		return nil, errors.New("names is required: preferences by name, or a prefix ending in . or *, e.g. calendar.alarms.")
 	}
+	for _, n := range names {
+		if strings.Trim(strings.TrimSpace(n), ".*") == "" {
+			return nil, fmt.Errorf("%q asks for every preference: name one, or a prefix such as calendar.alarms.", n)
+		}
+	}
 	p, err := m.profile()
 	if err != nil {
 		return nil, err
@@ -339,8 +351,10 @@ func (m *Machine) Prefs(names []string) (map[string]any, error) {
 			pr.From = from
 			if why := withheld(n); why != "" {
 				pr.Withheld = why
-			} else {
+			} else if allowedExtensions[n] {
 				pr.Value = v
+			} else {
+				pr.Value = m.maskValue(v)
 			}
 			out = append(out, pr)
 		}
@@ -581,11 +595,31 @@ var (
 	credentialWord = regexp.MustCompile(`(?i)((?:password|passwd|token|secret|authorization|cookie)\S*\s*[=:]\s*)\S+`)
 )
 
+// userAtHost and hostName are what a preference's value may name a server by.
+var (
+	userAtHost = regexp.MustCompile(`[A-Za-z0-9._%+\-]+%40[A-Za-z0-9.\-]+@[A-Za-z0-9.\-]+`)
+	// A host is a name of three labels or more, or of two whose last is a common top-level domain: a
+	// file's name (a.pdf) is not one.
+	hostName      = regexp.MustCompile(`(?i)\b(?:(?:[a-z0-9](?:[a-z0-9-]*[a-z0-9])?\.){2,}[a-z]{2,}|[a-z0-9](?:[a-z0-9-]*[a-z0-9])?\.(?:com|org|net|be|nl|de|fr|eu|io|uk|us|info|biz|dev|app|cloud|local|lan|internal|home|corp))\b`)
+	bearerOrBasic = regexp.MustCompile(`(?i)\b(bearer|basic)\s+[A-Za-z0-9._~+/=\-]+`)
+)
+
 func mask(s string) string {
+	s = userAtHost.ReplaceAllString(s, "<hidden>")
 	s = emailAddress.ReplaceAllString(s, "<hidden>")
+	s = bearerOrBasic.ReplaceAllString(s, "$1 <hidden>")
 	return credentialWord.ReplaceAllString(s, "${1}<hidden>")
 }
 
+// maskValue is a preference's value as answered: the home as ~, and no address, user or host.
+func (m *Machine) maskValue(v string) string {
+	if m.Home != "" && m.Home != "/" {
+		v = strings.ReplaceAll(v, strings.TrimSuffix(m.Home, "/")+"/", "~/")
+	}
+	v = mask(v)
+	return hostName.ReplaceAllString(v, "<host>")
+}
+
 // mostAnswer bounds a log answer.
 const mostAnswer = 128 << 10
 
@@ -631,24 +665,49 @@ func (m *Machine) Log(n int, problems bool, unit string) (map[string]any, error)
 
 // RestartAnswer is what betterbird_restart answers.
 type RestartAnswer struct {
-	Ended   []int   `json:"ended"`
-	Killed  []int   `json:"killed,omitempty"`
-	UserJS  string  `json:"user_js"`
-	Running []Proc  `json:"running"`
-	Session Session `json:"session"`
-	Unit    string  `json:"unit"`
+	// Composers are the message windows that were open: what was unsent and is gone.
+	Composers int     `json:"composers_discarded,omitempty"`
+	Note      string  `json:"note,omitempty"`
+	Ended     []int   `json:"ended"`
+	Killed    []int   `json:"killed,omitempty"`
+	UserJS    string  `json:"user_js"`
+	Running   []Proc  `json:"running"`
+	Session   Session `json:"session"`
+	Unit      string  `json:"unit"`
 }
 
-// Restart ends Betterbird (asked to quit, forced after 20 s), places the module's user.js when it can,
+// composerClass is the X instance of a message being written.
+var composerClass = regexp.MustCompile(`\("Msgcompose" "eu\.betterbird\.Betterbird"\)`)
+
+// composers counts the open message windows, from the X window tree.
+func (m *Machine) composers(s Session) (int, error) {
+	o := m.cmd(5*time.Second, s.Env(), "xwininfo", "-root", "-tree")
+	if err := failed(o, "xwininfo", "-root", "-tree"); err != nil {
+		return 0, err
+	}
+	return len(composerClass.FindAllString(o.Stdout, -1)), nil
+}
+
+// Restart ends Betterbird (asked to quit, forced after 10 s), places the module's user.js when it can,
 // and starts Betterbird again in the operator's session under the account's service manager, its output
-// in the journal.
-func (m *Machine) Restart() (RestartAnswer, error) {
+// in the journal. While a message is being written it refuses, unless discard says the unsent may go.
+// It answers within the runtime's 30 s: a start that has not shown after 4 s is said, and
+// betterbird_status sees it come.
+func (m *Machine) Restart(discard bool) (RestartAnswer, error) {
 	s, err := m.session()
 	if err != nil {
 		return RestartAnswer{}, err
 	}
 	a := RestartAnswer{Session: s, Unit: restartAs + ".service"}
-	a.Ended, a.Killed = m.stopProcs(20*time.Second, m.mainProcs())
+	n, err := m.composers(s)
+	switch {
+	case err != nil && !discard:
+		return a, fmt.Errorf("whether a message is being written cannot be told (%v): restart with discard_unsent only if none is", err)
+	case n > 0 && !discard:
+		return a, fmt.Errorf("%d composer window(s) are open: an unsent message would be lost. Send or save it, or ask the operator, then call again with discard_unsent", n)
+	}
+	a.Composers = n
+	a.Ended, a.Killed = m.stopProcs(10*time.Second, m.mainProcs())
 	if u, err := m.userJS(true); err != nil {
 		a.UserJS = "not placed: " + err.Error()
 	} else {
@@ -657,13 +716,15 @@ func (m *Machine) Restart() (RestartAnswer, error) {
 	if err := m.detach(s, restartAs, startBin); err != nil {
 		return a, err
 	}
-	m.waitFor(mainComm, 8*time.Second)
+	for waited := time.Duration(0); waited < 4*time.Second && len(m.mainProcs()) == 0; waited += 250 * time.Millisecond {
+		m.Sleep(250 * time.Millisecond)
+	}
 	a.Running = m.mainProcs()
 	if a.Running == nil {
 		a.Running = []Proc{}
 	}
 	if len(a.Running) == 0 {
-		return a, fmt.Errorf("Betterbird was started as %s but no main process appeared within 8 s: betterbird_log says why", a.Unit)
+		a.Note = "started as " + a.Unit + "; no main process yet after 4 s: betterbird_status shows it once it runs, betterbird_log says why if it does not"
 	}
 	return a, nil
 }

modules/betterbird/cmd/betterbird-tools/betterbird_test.go

@@ -205,7 +205,7 @@ func TestRestartEndsItPlacesTheUserJSAndStartsItUnderTheServiceManager(t *testin
 	f.onStart = func(argv []string) {
 		f.proc(5000, 1000, "betterbird-bin", []string{"betterbird"}, "mesh-betterbird.service")
 	}
-	a, err := f.Restart()
+	a, err := f.Restart(false)
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -243,7 +243,7 @@ func TestTheReminderTestTouchesNothingOfTheOperatorsProfile(t *testing.T) {
 		a.Due != "2027-01-15T08:00:40Z" {
 		t.Fatalf("%s %s", cal, a.Due)
 	}
-	if !f.called("systemd-run --user --collect --quiet --unit=" + testUnit + " -p RuntimeMaxSec=600 --setenv=MOZ_NO_REMOTE=1") {
+	if !f.called("systemd-run --user --collect --quiet --unit=" + testUnit + " -p RuntimeMaxSec=600 -p ExecStopPost=/usr/bin/rm -rf /run/user/1000/" + testUnit + " --setenv=MOZ_NO_REMOTE=1") {
 		t.Fatalf("%v", f.calls)
 	}
 	after, _ := os.ReadFile(filepath.Join(f.Root, profileDir, "prefs.js"))

modules/betterbird/cmd/betterbird-tools/main.go

@@ -100,11 +100,21 @@ func tools() []stdio.Tool {
 		},
 		{
 			Name: "betterbird_restart",
-			Description: "End Betterbird (asked to quit, forced after 20 s), place the module's user.js when the profile " +
+			Description: "End Betterbird (asked to quit, forced after 10 s), place the module's user.js when the profile " +
 				"allows it, and start Betterbird again in the operator's desktop session under the account's service " +
-				"manager (unit mesh-betterbird), its output in the journal. An unsent message in a composer is lost: ask " +
-				"the operator first. Answers the pids ended and the new main process. (a)",
-			Run: func(map[string]any) (any, error) { return machine.Restart() },
+				"manager (unit mesh-betterbird), its output in the journal. Refused while a message is being written " +
+				"(a composer window is open), because the unsent message would be lost, unless discard_unsent is true — " +
+				"ask the operator first. Answers the pids ended and the new main process. (a)",
+			Input: map[string]any{
+				"discard_unsent": map[string]any{"type": "boolean", "description": "restart even with a composer open, losing what it holds (default false)"},
+			},
+			Run: func(args map[string]any) (any, error) {
+				discard, err := flag(args, "discard_unsent", false)
+				if err != nil {
+					return nil, err
+				}
+				return machine.Restart(discard)
+			},
 		},
 		{
 			Name: "betterbird_check",

modules/betterbird/cmd/betterbird-tools/reminder.go

@@ -133,8 +133,10 @@ func (m *Machine) ReminderTest(after time.Duration, carry []string, stop bool) (
 	if err := os.WriteFile(filepath.Join(dir, "profile", "user.js"), []byte(user), 0o600); err != nil {
 		return a, err
 	}
+	// The unit removes the profile when it ends, however it ends: at its 10 minutes, on stop, or when the
+	// test Betterbird is closed.
 	call := []string{"--user", "--collect", "--quiet", "--unit=" + testUnit, "-p", "RuntimeMaxSec=600",
-		"--setenv=MOZ_NO_REMOTE=1"}
+		"-p", "ExecStopPost=/usr/bin/rm -rf " + strings.TrimPrefix(dir, m.Root), "--setenv=MOZ_NO_REMOTE=1"}
 	for _, kv := range [][2]string{{"DISPLAY", s.Display}, {"XAUTHORITY", s.XAuthority}} {
 		if kv[1] != "" {
 			call = append(call, "--setenv="+kv[0]+"="+kv[1])

modules/betterbird/cmd/betterbird-tools/review_test.go

@@ -0,0 +1,141 @@
+package main
+
+import (
+	"strings"
+	"testing"
+	"time"
+)
+
+// The review of pull request 155 (hq issue 345): a restart never loses an unsent message unasked, a
+// preference's value never carries an address, a log never a credential, the reminder test never
+// counts as the operator's Betterbird and leaves nothing behind, and a restart answers within the
+// runtime's call limit.
+
+const composerTree = `
+     0x2a00003 "Inbox - Betterbird": ("Mail" "eu.betterbird.Betterbird")  2560x1388+0+0  +0+26
+     0x2a01234 "Write: hello - Betterbird": ("Msgcompose" "eu.betterbird.Betterbird")  900x700+10+10  +10+10
+`
+
+func TestARestartRefusesWhileAComposerIsOpenUnlessTheUnsentMayGo(t *testing.T) {
+	f := newFake(t)
+	f.desktopSession()
+	f.betterbird()
+	links(t, nil)
+	f.declare("// none")
+	f.answer = func(name string, args []string) Output {
+		if name == "xwininfo" {
+			return Output{Stdout: composerTree}
+		}
+		return Output{}
+	}
+	if _, err := f.Restart(false); err == nil || !strings.Contains(err.Error(), "composer") {
+		t.Fatalf("refused while a message is being written: %v", err)
+	}
+	if len(f.signals) != 0 {
+		t.Fatalf("nothing was ended: %v", f.signals)
+	}
+	f.onStart = func([]string) {
+		f.proc(5000, 1000, "betterbird-bin", []string{"betterbird"}, "mesh-betterbird.service")
+	}
+	a, err := f.Restart(true)
+	if err != nil || len(a.Ended) != 1 || a.Composers != 1 {
+		t.Fatalf("with discard_unsent it goes ahead and says what was open: %+v %v", a, err)
+	}
+}
+
+func TestARestartAnswersWithinTheCallLimitEvenWhenNothingCooperates(t *testing.T) {
+	f := newFake(t)
+	f.desktopSession()
+	f.betterbird()
+	links(t, nil)
+	f.stubborn[4000] = true
+	slept := time.Duration(0)
+	f.Sleep = func(d time.Duration) { slept += d }
+	a, err := f.Restart(false)
+	if err != nil {
+		t.Fatalf("a start that has not shown yet is said, not failed: %v", err)
+	}
+	if slept > 18*time.Second || len(a.Killed) != 1 || a.Note == "" {
+		t.Fatalf("waited %v of the runtime's 30 s: %+v", slept, a)
+	}
+}
+
+func TestAPreferencesValueCarriesNoAddressHostOrHome(t *testing.T) {
+	f := newFake(t)
+	f.betterbird()
+	links(t, nil)
+	f.write(profileDir+"/prefs.js", strings.Join([]string{
+		`user_pref("mail.last_msg_movecopy_target_uri", "imap://someone%40example.org@mail.example.org/INBOX");`,
+		`user_pref("messenger.account.a1.server", "x");`,
+		`user_pref("mail.cloud_files.accounts.a", "y");`,
+		`user_pref("editor.lastFileLocation", "/home/operator/Documents/a.pdf");`,
+		`user_pref("network.proxy.http", "proxy.corp.example.com");`,
+		`user_pref("mail.compose.default_to", "imap://someone%40example.org@mail.example.org/x and bob@example.org");`,
+		`user_pref("extensions.activeThemeID", "thunderbird-compact-dark@mozilla.org");`,
+		`user_pref("extensions.ui.lastCategory", "addons://list/theme");`,
+		`user_pref("font.size.variable.x-western", 12);`,
+	}, "\n")+"\n")
+	got, err := f.Prefs([]string{"mail.last_msg*", "messenger.account.", "mail.cloud_files.", "editor.", "network.proxy.",
+		"mail.compose.default_to", "extensions.", "font.size."})
+	if err != nil {
+		t.Fatal(err)
+	}
+	by := map[string]Pref{}
+	for _, p := range got["prefs"].([]Pref) {
+		by[p.Name] = p
+	}
+	for _, n := range []string{"mail.last_msg_movecopy_target_uri", "messenger.account.a1.server", "mail.cloud_files.accounts.a", "extensions.ui.lastCategory"} {
+		if p := by[n]; p.Value != "" || p.Withheld == "" {
+			t.Errorf("withheld: %s %+v", n, p)
+		}
+	}
+	if v := by["editor.lastFileLocation"].Value; v != `"~/Documents/a.pdf"` {
+		t.Errorf("home as ~: %s", v)
+	}
+	if v := by["network.proxy.http"].Value; strings.Contains(v, "example") {
+		t.Errorf("a host: %s", v)
+	}
+	if v := by["mail.compose.default_to"].Value; strings.Contains(v, "example") || strings.Contains(v, "someone") {
+		t.Errorf("an address: %s", v)
+	}
+	if v := by["extensions.activeThemeID"].Value; v != `"thunderbird-compact-dark@mozilla.org"` {
+		t.Errorf("a theme's id is answered as it is: %s", v)
+	}
+	if v := by["font.size.variable.x-western"].Value; v != "12" {
+		t.Errorf("%s", v)
+	}
+	for _, bad := range [][]string{{""}, {"*"}, {"."}, {"  "}} {
+		if _, err := f.Prefs(bad); err == nil {
+			t.Errorf("%q is refused: it asks for everything", bad)
+		}
+	}
+}
+
+func TestALogLineKeepsNoBearerOrBasicCredential(t *testing.T) {
+	for _, l := range []string{"Authorization: Bearer eyJhbGciOi.abc.def", "auth header basic dXNlcjpwYXNz sent", "token=abc123"} {
+		got := mask(l)
+		for _, secret := range []string{"eyJhbGciOi", "dXNlcjpwYXNz", "abc123"} {
+			if strings.Contains(got, secret) {
+				t.Errorf("%q → %q", l, got)
+			}
+		}
+	}
+}
+
+func TestTheReminderTestIsNeverTheOperatorsBetterbirdAndCleansUpWhenItEnds(t *testing.T) {
+	f := newFake(t)
+	f.desktopSession()
+	f.betterbird()
+	links(t, nil)
+	f.proc(6000, 1000, "betterbird-bin", []string{"/usr/bin/betterbird", "--no-remote", "--profile", "/run/user/1000/" + testUnit + "/profile"}, testUnit+".service")
+	if ps := f.mainProcs(); len(ps) != 1 || ps[0].PID != 4000 {
+		t.Fatalf("only the operator's: %+v", ps)
+	}
+	f.declare("// none")
+	if _, err := f.ReminderTest(40*time.Second, nil, false); err != nil {
+		t.Fatal(err)
+	}
+	if !f.called("systemd-run --user --collect --quiet --unit=" + testUnit + " -p RuntimeMaxSec=600 -p ExecStopPost=/usr/bin/rm -rf /run/user/1000/" + testUnit) {
+		t.Fatalf("the unit removes its profile when it ends: %v", f.calls)
+	}
+}