betterbird: never report the theme as fixed by clearing prefs.js, and never manage an account's name (hq issue 345, review 96)
mesh/merge-gate head c4b4bac757c5 merged into trunk 0863ec2a059d — pass: builds betterbird, sent nowhere; no bus step; 1 wait(s) for a person; every machin…
mesh/repo-check head c4b4bac757c5 merged into trunk 0863ec2a059d — pass: its merge-check.sh passed, each part in its toolchain: merge-check.sh (go), merge-…
mesh/delivery delivered

Betterbird keeps the active theme in its add-on database, and extensions.activeThemeID only mirrors it.
So clearing the preference changed nothing, and the drift report then hid that. The theme is now read
from extensions.json against default-theme@mozilla.org. Another theme is drift, and its remedy is the
add-on manager: config_apply never clears it and keep refuses it.

A credential's, an account's or a server's name in the defaults file is refused by code: it is never
answered or cleared, and the check names it. Apply also refuses while the profile's lock is there, and
syncs prefs.js before the rename. A saved default of a built-in preference is no longer drift.
This commit is contained in:
jochen
2026-10-12 21:09:25 +02:00
parent 854c85393c
commit c4b4bac757
5 changed files with 261 additions and 31 deletions
+11 -2
View File
@@ -117,8 +117,17 @@ the layers, and flags **drift**: a saved value that beats the module's default w
`betterbird_check` reports it. There are two remedies:
- `betterbird_config_apply` with `apply` takes the drifting values out of `prefs.js`, so the module's value
holds at the next start. It acts only while Betterbird is quit, because Betterbird rewrites `prefs.js` when it
quits, and it says what it cleared. Without `apply` it lists what it would clear.
holds at the next start. It acts only while Betterbird is quit (no main process, and no `lock` in the profile),
because Betterbird rewrites `prefs.js` when it quits, and it says what it cleared. Without `apply` it lists
what it would clear.
- **The theme is the exception.** Betterbird keeps the active theme in its add-on database (`extensions.json`),
and `extensions.activeThemeID` only mirrors it. So the tools read the theme from the add-on database, and the
module's value is `default-theme@mozilla.org`. Another theme is drift whose remedy is to enable the default
theme in the add-on manager. `betterbird_config_apply` never clears it, and `keep` refuses it.
**Never an account.** A name the tools withhold (a credential's, an account's, a server's), or an account's own
preference, is never managed, even when the defaults file lists it. `betterbird_config` leaves it out and names it
in `refused`, `betterbird_config_apply` never clears it, and `betterbird_check` says to take it out of the file.
- `betterbird_config` with `keep` proposes, through the controller's settings verb, a node layer of `prefs` that
holds the profile's value for this machine. The operator approves it on a channel that proves who answers.
The tool never writes `user.js` itself. The setting is one string, so the layer carries what `prefs` says on
@@ -908,11 +908,27 @@ func (m *Machine) Check() (CheckAnswer, error) {
if in.From == "the mesh" && !exists(m.path(meshDefaults)) {
add("the module's defaults are not placed ("+meshDefaults+")", "push the module to this node")
}
if c, _, err := m.config(); err == nil && len(c.Drifting) > 0 {
add(fmt.Sprintf("%d managed preference(s) drift from the module's defaults: the profile saved its own value and no "+
"node layer says so (%s)", len(c.Drifting), strings.Join(c.Drifting, ", ")),
"betterbird_config shows each; to keep a value on this node, betterbird_config with keep proposes a node "+
"layer; otherwise quit Betterbird and call betterbird_config_apply with apply")
if c, _, err := m.config(); err == nil {
var cleared []string
for _, cp := range c.Prefs {
switch {
case cp.Drift == "":
case cp.Remedy != "":
add(cp.Name+" drifts from the module's: "+cp.Drift, cp.Remedy)
default:
cleared = append(cleared, cp.Name)
}
}
if len(cleared) > 0 {
add(fmt.Sprintf("%d managed preference(s) drift from the module's defaults: the profile saved its own value and no "+
"node layer says so (%s)", len(cleared), strings.Join(cleared, ", ")),
"betterbird_config shows each; to keep a value on this node, betterbird_config with keep proposes a node "+
"layer; otherwise quit Betterbird and call betterbird_config_apply with apply")
}
if len(c.Refused) > 0 {
add(fmt.Sprintf("the module's defaults list %d preference(s) the tools never manage (%s)", len(c.Refused), strings.Join(c.Refused, "; ")),
"take them out of install/defaults-pref/mesh-defaults.js in a reviewed catalogue change: an account is declared in the setting accounts")
}
}
if why := m.defaultClientAsk(profilePath); why != "" && in.From != "not installed" {
add("Betterbird asks at its start to be the default mail client: "+why,
+134 -14
View File
@@ -48,6 +48,71 @@ var (
keepsDefault = regexp.MustCompile(`^//\s*keeps Betterbird's default:\s*([A-Za-z0-9_.@{}\-]+)\s*$`)
)
// builtIn are the defaults compiled into Betterbird of the preferences the module keeps at Betterbird's default,
// which no defaults file gives: so a value saved equal to them is not drift.
var builtIn = map[string]string{
themePref: `"` + defaultTheme + `"`,
"browser.display.use_system_colors": "false",
"layout.css.always_underline_links": "false",
}
const (
themePref = "extensions.activeThemeID"
defaultTheme = "default-theme@mozilla.org"
// themeRemedy is how the theme comes back: Betterbird keeps the active theme in its add-on database
// (extensions.json), and the preference only mirrors it (XPIDatabase addonChanged), so clearing it changes
// nothing and betterbird_config_apply never touches it.
themeRemedy = "enable the default theme in Betterbird's add-on manager (Add-ons and Themes, Themes): the active " +
"theme is kept in the add-on database, and its preference only mirrors it, so clearing prefs.js would not change it"
)
// activeTheme is the theme the profile's add-on database marks active, or "" when it cannot be read.
func activeTheme(profile string) string {
raw, err := readBounded(filepath.Join(profile, "extensions.json"))
if err != nil {
return ""
}
var ext struct {
Addons []struct {
ID string `json:"id"`
Type string `json:"type"`
Active bool `json:"active"`
} `json:"addons"`
}
if json.Unmarshal(raw, &ext) != nil {
return ""
}
for _, a := range ext.Addons {
if a.Type == "theme" && a.Active {
return a.ID
}
}
return ""
}
// unmanageable says why a name in the module's defaults is never managed by these tools, or "": a credential's,
// an account's or a server's preference, which betterbird_config never answers and betterbird_config_apply never
// clears, whatever the defaults file lists.
func unmanageable(name string) string {
if why := withheld(name); why != "" {
return why
}
for _, p := range accountOwned {
if strings.HasPrefix(name, p) {
return "an account's preference, the setting accounts' alone"
}
}
return ""
}
// answer is a value as these tools answer it: masked, but for the theme's id.
func (m *Machine) answer(name, v string) string {
if allowedExtensions[name] {
return v
}
return m.maskValue(v)
}
// nodeName is the machine this runtime serves, as the mesh names it: a variable so a test answers.
var nodeName = func() string { return os.Getenv("MESH_NODE") }
@@ -269,6 +334,8 @@ type ConfigPref struct {
From string `json:"effective_from"`
// Drift says how the profile's own value beats the module's without a node layer saying so.
Drift string `json:"drift,omitempty"`
// Remedy is how drift that clearing prefs.js cannot fix is fixed (the theme).
Remedy string `json:"remedy,omitempty"`
// module is the value the module means: its default, or Betterbird's.
module string
}
@@ -280,6 +347,8 @@ type ConfigAnswer struct {
Prefs []ConfigPref `json:"prefs"`
// Drifting are the names that drift.
Drifting []string `json:"drifting"`
// Refused are names the module's defaults list that the tools never manage: an account's or a credential's.
Refused []string `json:"refused,omitempty"`
// Proposed is the controller's answer to a node layer proposed with keep.
Proposed string `json:"proposed,omitempty"`
Note string `json:"note,omitempty"`
@@ -320,11 +389,23 @@ func (m *Machine) config() (ConfigAnswer, Profile, error) {
names = append(names, n)
}
}
theme := activeTheme(p.path)
for _, n := range names {
if why := unmanageable(n); why != "" {
a.Refused = append(a.Refused, n+": "+why)
continue
}
c := ConfigPref{Name: n, NodeLayer: node[n], Profile: saved[n]}
if n == themePref && theme != "" {
// The add-on database says which theme is active; the preference only mirrors it.
c.Profile = quoted(theme)
}
bb, hasBB := own[n]
if hasBB {
c.BetterbirdDefault = bb.Value
} else if v, ok := builtIn[n]; ok {
c.BetterbirdDefault = v
bb.From = "Betterbird's built-in default"
}
switch v, isLocked := locked[n]; {
case isLocked:
@@ -351,9 +432,12 @@ func (m *Machine) config() (ConfigAnswer, Profile, error) {
}
if why := drift(c); why != "" {
c.Drift = why
if n == themePref {
c.Remedy = themeRemedy
}
a.Drifting = append(a.Drifting, n)
}
c.NodeLayer, c.Profile, c.Effective = m.maskValue(c.NodeLayer), m.maskValue(c.Profile), m.maskValue(c.Effective)
c.NodeLayer, c.Profile, c.Effective = m.answer(n, c.NodeLayer), m.answer(n, c.Profile), m.answer(n, c.Effective)
a.Prefs = append(a.Prefs, c)
}
return a, p, nil
@@ -369,7 +453,11 @@ func drift(c ConfigPref) string {
if want == "" {
want = "Betterbird's built-in default"
}
return fmt.Sprintf("the profile's prefs.js holds %s, which beats the module's %s, and no node layer says so", c.Profile, want)
where := "the profile's prefs.js holds"
if c.Name == themePref {
where = "the profile's active theme is"
}
return fmt.Sprintf("%s %s, which beats the module's %s, and no node layer says so", where, c.Profile, want)
}
// Config is what betterbird_config answers; with keep it proposes a node layer of the setting prefs that holds
@@ -385,20 +473,12 @@ func (m *Machine) Config(keep []string) (ConfigAnswer, error) {
if len(keep) == 0 {
return a, nil
}
node := nodeName()
if node == "" {
return a, errors.New("nothing was proposed: this runtime does not say which machine it serves (MESH_NODE)")
}
by := map[string]ConfigPref{}
for _, c := range a.Prefs {
by[c.Name] = c
}
prefsRaw, _ := readBounded(filepath.Join(p.path, "prefs.js"))
saved := readPrefs(prefsRaw)
declaredRaw, err := readBounded(m.home(declared))
if err != nil {
return a, fmt.Errorf("nothing was proposed: the module's user.js is not on this machine (~/%s)", declared)
}
kept := map[string]string{}
for _, n := range keep {
c, managed := by[n]
@@ -407,11 +487,21 @@ func (m *Machine) Config(keep []string) (ConfigAnswer, error) {
return a, fmt.Errorf("nothing was proposed: %s is not a preference the module manages (betterbird_config lists them)", n)
case c.Locked != "":
return a, fmt.Errorf("nothing was proposed: %s is locked by the module's policy; a lock changes only in the catalogue", n)
case n == themePref:
return a, fmt.Errorf("nothing was proposed: a node layer cannot choose the theme; the add-on manager does (%s)", themeRemedy)
case saved[n] == "":
return a, fmt.Errorf("nothing was proposed: the profile saved no value of its own for %s", n)
}
kept[n] = saved[n]
}
declaredRaw, err := readBounded(m.home(declared))
if err != nil {
return a, fmt.Errorf("nothing was proposed: the module's user.js is not on this machine (~/%s)", declared)
}
node := nodeName()
if node == "" {
return a, errors.New("nothing was proposed: this runtime does not say which machine it serves (MESH_NODE)")
}
// The setting is one string, so a node layer replaces it whole: it carries what the setting says on this
// machine today, with the kept values in place of any the same names had.
var parts []string
@@ -464,7 +554,9 @@ type ConfigApplyAnswer struct {
Profile string `json:"profile"`
Applied bool `json:"applied"`
Clears []Cleared `json:"clears"`
Note string `json:"note"`
// NotCleared is drift that clearing prefs.js cannot fix, each with how it is fixed.
NotCleared []string `json:"not_cleared,omitempty"`
Note string `json:"note"`
}
// ConfigApply takes the drifting values out of the profile's prefs.js, so the module's value holds at the next
@@ -481,9 +573,13 @@ func (m *Machine) ConfigApply(apply bool) (ConfigApplyAnswer, error) {
}
drifting := map[string]bool{}
for _, cp := range c.Prefs {
if cp.Drift != "" && cp.Remedy != "" {
a.NotCleared = append(a.NotCleared, cp.Name+": "+cp.Remedy)
continue
}
if cp.Drift != "" {
drifting[cp.Name] = true
now := m.maskValue(cp.module)
now := m.answer(cp.Name, cp.module)
if now == "" {
now = "Betterbird's built-in default"
}
@@ -497,8 +593,13 @@ func (m *Machine) ConfigApply(apply bool) (ConfigApplyAnswer, error) {
if ps := m.mainProcs(); len(ps) > 0 {
return a, fmt.Errorf("Betterbird is running (pid %d): it rewrites prefs.js when it quits, so nothing was cleared; quit it, then apply again", ps[0].PID)
}
// The profile's own lock is Gecko's sign that a Betterbird holds it, whichever installation started it.
if _, err := os.Lstat(filepath.Join(p.path, "lock")); err == nil {
return a, errors.New("the profile is locked (its lock file is there): a Betterbird holds it, or one ended " +
"without quitting; nothing was cleared. Quit Betterbird (or start and quit it once), then apply again")
}
if len(drifting) == 0 {
a.Applied, a.Note = true, "nothing drifts: prefs.js was left as it was"
a.Applied, a.Note = true, "nothing in prefs.js drifts: it was left as it was"
return a, nil
}
target := filepath.Join(p.path, "prefs.js")
@@ -519,7 +620,8 @@ func (m *Machine) ConfigApply(apply bool) (ConfigApplyAnswer, error) {
kept.WriteString(l)
}
tmp := target + ".mesh-new"
if err := os.WriteFile(tmp, []byte(kept.String()), st.Mode().Perm()); err != nil {
if err := writeSynced(tmp, []byte(kept.String()), st.Mode().Perm()); err != nil {
os.Remove(tmp)
return a, err
}
if err := os.Rename(tmp, target); err != nil {
@@ -530,3 +632,21 @@ func (m *Machine) ConfigApply(apply bool) (ConfigApplyAnswer, error) {
a.Note = fmt.Sprintf("cleared %d value(s) from prefs.js; the module's values hold from Betterbird's next start", len(a.Clears))
return a, nil
}
// writeSynced writes a file and syncs it to disk before it is renamed into place, so a crash never leaves an
// empty prefs.js.
func writeSynced(path string, data []byte, mode os.FileMode) error {
f, err := os.OpenFile(path, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, mode)
if err != nil {
return err
}
if _, err := f.Write(data); err != nil {
f.Close()
return err
}
if err := f.Sync(); err != nil {
f.Close()
return err
}
return f.Close()
}
@@ -269,8 +269,9 @@ func TestTheConfigShowsTheLayersAndTheDrift(t *testing.T) {
p.Profile != "true" || p.Effective != "true" || p.From != "the profile's prefs.js" || !strings.Contains(p.Drift, "beats the module's false") {
t.Errorf("%+v", p)
}
if p := by["extensions.activeThemeID"]; p.Layer != "Betterbird's default" || !strings.Contains(p.Drift, "built-in default") {
t.Errorf("a kept default with a saved value is drift: %+v", p)
if p := by["extensions.activeThemeID"]; p.Layer != "Betterbird's default" || !strings.Contains(p.Drift, `"default-theme@mozilla.org"`) ||
!strings.Contains(p.Remedy, "add-on manager") || p.Profile != `"thunderbird-compact-dark@mozilla.org"` {
t.Errorf("another theme is drift whose remedy is the add-on manager, its id answered whole: %+v", p)
}
if p := by["mail.spam.manualMark"]; p.Drift != "" || p.BetterbirdDefault != "false" {
t.Errorf("a saved value equal to Betterbird's default is not drift: %+v", p)
@@ -291,10 +292,15 @@ func TestTheConfigShowsTheLayersAndTheDrift(t *testing.T) {
}
found := false
for _, x := range ch.Findings {
found = found || (strings.Contains(x.What, "2 managed preference(s) drift") && strings.Contains(x.What, "mail.biff.show_alert") &&
found = found || (strings.Contains(x.What, "1 managed preference(s) drift") && strings.Contains(x.What, "mail.biff.show_alert") &&
strings.Contains(x.Do, "betterbird_config_apply") && strings.Contains(x.Do, "keep"))
}
if !found {
theme := false
for _, x := range ch.Findings {
theme = theme || (strings.HasPrefix(x.What, "extensions.activeThemeID drifts") && strings.Contains(x.Do, "add-on manager") &&
!strings.Contains(x.Do, "betterbird_config_apply"))
}
if !found || !theme {
t.Fatalf("the check reports the drift: %+v", ch.Findings)
}
os.Remove(filepath.Join(f.Root, meshDefaults))
@@ -324,7 +330,7 @@ func TestConfigApplyClearsTheDriftOnlyWhileBetterbirdIsQuit(t *testing.T) {
`user_pref("mail.uidensity", 0);` + "\n"
f.write(profileDir+"/prefs.js", prefs)
dry, err := f.ConfigApply(false)
if err != nil || dry.Applied || len(dry.Clears) != 2 {
if err != nil || dry.Applied || len(dry.Clears) != 1 || len(dry.NotCleared) != 1 {
t.Fatalf("a dry run lists and changes nothing: %+v %v", dry, err)
}
if f.read(profileDir+"/prefs.js") != prefs {
@@ -337,11 +343,19 @@ func TestConfigApplyClearsTheDriftOnlyWhileBetterbirdIsQuit(t *testing.T) {
t.Fatal("prefs.js changed while Betterbird runs")
}
stopBetterbird(t, f)
f.write(profileDir+"/lock", "")
if _, err := f.ConfigApply(true); err == nil || !strings.Contains(err.Error(), "locked") {
t.Fatalf("refused while the profile's own lock is there: %v", err)
}
if err := os.Remove(filepath.Join(f.Root, profileDir, "lock")); err != nil {
t.Fatal(err)
}
a, err := f.ConfigApply(true)
if err != nil || !a.Applied {
t.Fatalf("%+v %v", a, err)
}
want := "// Mozilla User Preferences\n" +
`user_pref("extensions.activeThemeID", "thunderbird-compact-dark@mozilla.org");` + "\n" +
`user_pref("mail.identity.id1.useremail", "someone@example.org");` + "\n" +
`user_pref("mail.uidensity", 0);` + "\n"
if got := f.read(profileDir + "/prefs.js"); got != want {
@@ -354,15 +368,15 @@ func TestConfigApplyClearsTheDriftOnlyWhileBetterbirdIsQuit(t *testing.T) {
if c := said["mail.biff.show_alert"]; c.Was != "true" || c.Now != "false" {
t.Errorf("it says what it cleared and what holds now: %+v", c)
}
if c := said["extensions.activeThemeID"]; c.Now != "Betterbird's built-in default" {
t.Errorf("%+v", c)
if _, cleared := said["extensions.activeThemeID"]; cleared || len(a.NotCleared) != 1 || !strings.Contains(a.NotCleared[0], "add-on manager") {
t.Errorf("the theme is never cleared, and said with its remedy: %+v", a)
}
raw, _ := json.Marshal(a)
if strings.Contains(string(raw), "example.org") {
t.Fatalf("no account value leaves: %s", raw)
}
if c, _ := f.Config(nil); len(c.Drifting) != 0 {
t.Fatalf("no drift once applied: %v", c.Drifting)
if c, _ := f.Config(nil); !reflect.DeepEqual(c.Drifting, []string{"extensions.activeThemeID"}) {
t.Fatalf("once applied, only the theme drifts, until the add-on manager changes it: %v", c.Drifting)
}
os.Remove(filepath.Join(f.Root, meshDefaults))
if _, err := f.ConfigApply(true); err == nil || !strings.Contains(err.Error(), "not placed") {
@@ -435,3 +449,72 @@ func TestTheDefaultClientAskIsDecidedFromThePreferencesEntry(t *testing.T) {
t.Fatalf("the lock holds whatever else the policy lacks: %s", why)
}
}
// The review of pull request 256: the theme is the add-on database's, and a saved default is not drift.
func TestTheThemeIsTheAddOnDatabasesAndASavedDefaultIsNotDrift(t *testing.T) {
f := newFake(t)
f.betterbird()
f.placedByTheMesh()
f.withDefaults()
links(t, nil)
f.write(profileDir+"/prefs.js", `user_pref("browser.display.use_system_colors", false);`+"\n"+
`user_pref("layout.css.always_underline_links", false);`+"\n")
f.write(profileDir+"/extensions.json", `{"addons":[{"id":"thunderbird-compact-dark@mozilla.org","type":"theme","active":true,"location":"app-builtin"},`+
`{"id":"default-theme@mozilla.org","type":"theme","active":false,"location":"app-builtin"}]}`)
c, err := f.Config(nil)
if err != nil {
t.Fatal(err)
}
if !reflect.DeepEqual(c.Drifting, []string{"extensions.activeThemeID"}) {
t.Fatalf("the active theme from the add-on database, with no preference saved, is drift; a saved default is not: %v", c.Drifting)
}
f.write(profileDir+"/prefs.js", `user_pref("extensions.activeThemeID", "default-theme@mozilla.org");`+"\n")
f.write(profileDir+"/extensions.json", `{"addons":[{"id":"default-theme@mozilla.org","type":"theme","active":true,"location":"app-builtin"}]}`)
if c, _ := f.Config(nil); len(c.Drifting) != 0 {
t.Fatalf("a profile back on the default theme does not drift: %v", c.Drifting)
}
if _, err := f.Config([]string{"extensions.activeThemeID"}); err == nil || !strings.Contains(err.Error(), "add-on manager") {
t.Fatalf("a node layer cannot choose the theme: %v", err)
}
}
// The review of pull request 256: a credential's or an account's name in the defaults file is never managed.
func TestAnAccountsNameInTheDefaultsIsNeverManaged(t *testing.T) {
f := newFake(t)
f.betterbird()
f.placedByTheMesh()
links(t, nil)
f.write(meshDefaults, string(repoFile(t, "install/defaults-pref/mesh-defaults.js"))+
`pref("mail.identity.id1.useremail", "x");`+"\n"+"// keeps Betterbird's default: mail.server.server1.hostname\n"+
`pref("mail.smtpservers", "");`+"\n")
prefs := `user_pref("mail.identity.id1.useremail", "someone@example.org");` + "\n" +
`user_pref("mail.server.server1.hostname", "imap.example.org");` + "\n" +
`user_pref("mail.smtpservers", "smtp1");` + "\n"
f.write(profileDir+"/prefs.js", prefs)
c, err := f.Config(nil)
if err != nil {
t.Fatal(err)
}
if len(c.Refused) != 3 || len(c.Drifting) != 0 {
t.Fatalf("refused, never managed: %+v", c)
}
raw, _ := json.Marshal(c)
if strings.Contains(string(raw), "example.org") {
t.Fatalf("no account value is answered: %s", raw)
}
stopBetterbird(t, f)
if _, err := f.ConfigApply(true); err != nil {
t.Fatal(err)
}
if f.read(profileDir+"/prefs.js") != prefs {
t.Fatal("prefs.js keeps every account's value")
}
ch, _ := f.Check()
found := false
for _, x := range ch.Findings {
found = found || (strings.Contains(x.What, "3 preference(s) the tools never manage") && strings.Contains(x.Do, "mesh-defaults.js"))
}
if !found {
t.Fatalf("the check says so: %+v", ch.Findings)
}
}
@@ -11,7 +11,9 @@
// --- Theme and display ---
// keeps Betterbird's default: extensions.activeThemeID
// The default theme follows the desktop's dark mode; compactness comes from mail.uidensity, not a theme.
// The default theme (default-theme@mozilla.org) follows the desktop's dark mode; compactness comes from
// mail.uidensity, not a theme. Betterbird keeps the active theme in its add-on database, so another theme is
// changed back in the add-on manager, never by clearing prefs.js.
// keeps Betterbird's default: browser.display.use_system_colors
// Betterbird's own colours, which the default theme is drawn for.
// keeps Betterbird's default: layout.css.always_underline_links