From 70167821bb06ee575c7627dbc8ecd00df4472bea Mon Sep 17 00:00:00 2001 From: riccardom Date: Mon, 7 Sep 2026 17:12:12 +0200 Subject: [PATCH] [client] Stop the last config write that skipped normalization MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every path that creates or updates a profile config goes through apply(), which resolves an optional field to its default — except RenameProfile, which read the file with a bare json.Unmarshal, set the name, and wrote it straight back. That copied whatever the file held, so a config written by a client that stored these fields as null kept them null. It could not introduce a null, only carry one forward, but renaming a profile is a poor place to leave a half-resolved config behind. It now reads through GetExistingConfig, which normalizes what it hands out. The tests state the invariant the fix completes, over the *bool fields of Config listed by reflection so a field added later is covered without touching them: none may come out of apply() unset, and no write may store one as null. An optional bool that can be nil, true or false forces every reader to invent the meaning of nil, and makes a diff of the config compare presence rather than value — which is exactly what refused `netbird up` for a client restating its own defaults. SyncMessageVersion stays a genuine three-state field and is not covered: it is an *int whose absence means the client pins no version, and it travels to management that way. --- .../config_optional_fields_test.go | 131 ++++++++++++++++++ client/internal/profilemanager/service.go | 14 +- 2 files changed, 138 insertions(+), 7 deletions(-) create mode 100644 client/internal/profilemanager/config_optional_fields_test.go diff --git a/client/internal/profilemanager/config_optional_fields_test.go b/client/internal/profilemanager/config_optional_fields_test.go new file mode 100644 index 000000000..9b74e2217 --- /dev/null +++ b/client/internal/profilemanager/config_optional_fields_test.go @@ -0,0 +1,131 @@ +package profilemanager + +import ( + "encoding/json" + "os" + "path/filepath" + "reflect" + "testing" + + "github.com/stretchr/testify/require" +) + +// optionalBoolFields lists the *bool fields of Config by name, derived from the +// type so a field added later is covered without touching these tests. +func optionalBoolFields() []string { + pointerToBool := reflect.TypeOf((*bool)(nil)) + + var fields []string + configType := reflect.TypeOf(Config{}) + for i := range configType.NumField() { + field := configType.Field(i) + if field.Type == pointerToBool && field.Tag.Get("json") != "-" { + fields = append(fields, field.Name) + } + } + return fields +} + +func requireNoUnsetOptionalBool(t *testing.T, config *Config, context string) { + t.Helper() + + value := reflect.ValueOf(*config) + for _, name := range optionalBoolFields() { + require.False(t, value.FieldByName(name).IsNil(), + "%s left %s unset, so its readers have to invent a default and a diff of it compares presence instead of value", context, name) + } +} + +// An optional bool must not be tristate. While one can be nil, true or false, +// every reader has to invent the meaning of nil, and — the reason this test +// exists — a diff of the config ends up comparing presence rather than value: +// that is what made the update-settings gate refuse `netbird up` for a client +// restating its own defaults. apply() is where a config becomes complete, so +// the invariant belongs to it: no *bool may come out of apply() unset. +func TestApplyLeavesNoOptionalBoolUnset(t *testing.T) { + require.NotEmpty(t, optionalBoolFields(), "the invariant is only meaningful while Config has optional bools") + + t.Run("a config built from scratch", func(t *testing.T) { + config := newConfigSkeleton() + _, err := config.apply(ConfigInput{}) + require.NoError(t, err) + + requireNoUnsetOptionalBool(t, config, "apply on a new config") + }) + + t.Run("a config file that predates every optional field", func(t *testing.T) { + path := filepath.Join(t.TempDir(), "legacy.json") + require.NoError(t, os.WriteFile(path, []byte(`{"WgIface":"wt0"}`), 0o600)) + + config, err := GetExistingConfig(path) + require.NoError(t, err) + + requireNoUnsetOptionalBool(t, config, "a read of a legacy config") + }) + + t.Run("a config file that stores them as null", func(t *testing.T) { + path := filepath.Join(t.TempDir(), "null.json") + _, err := UpdateOrCreateConfig(ConfigInput{ConfigPath: path}) + require.NoError(t, err) + unsetOnDisk(t, path, optionalBoolFields()...) + + config, err := GetExistingConfig(path) + require.NoError(t, err) + + requireNoUnsetOptionalBool(t, config, "a read of a config storing nulls") + }) +} + +// The same invariant on disk: what a write leaves in the file is what the next +// client to read it starts from, so no write may store a null. +func TestNoWriteStoresAnUnsetOptionalBool(t *testing.T) { + requireNoNullOnDisk := func(t *testing.T, path string, context string) { + t.Helper() + + raw, err := os.ReadFile(path) + require.NoError(t, err) + + var stored map[string]json.RawMessage + require.NoError(t, json.Unmarshal(raw, &stored)) + + for _, name := range optionalBoolFields() { + value, present := stored[name] + require.True(t, present, "%s did not store %s at all", context, name) + require.NotEqual(t, "null", string(value), "%s stored %s as null", context, name) + } + } + + t.Run("UpdateOrCreateConfig", func(t *testing.T) { + path := filepath.Join(t.TempDir(), "created.json") + _, err := UpdateOrCreateConfig(ConfigInput{ConfigPath: path, ManagementURL: DefaultManagementURL}) + require.NoError(t, err) + + requireNoNullOnDisk(t, path, "UpdateOrCreateConfig") + }) + + t.Run("UpdateConfig over a config storing nulls", func(t *testing.T) { + path := filepath.Join(t.TempDir(), "stored.json") + _, err := UpdateOrCreateConfig(ConfigInput{ConfigPath: path}) + require.NoError(t, err) + unsetOnDisk(t, path, optionalBoolFields()...) + + _, err = UpdateConfig(ConfigInput{ConfigPath: path, ManagementURL: "https://mgmt.example.com"}) + require.NoError(t, err) + + requireNoNullOnDisk(t, path, "UpdateConfig") + }) + + // Renaming used to copy the file back through a bare Unmarshal, which + // preserved the nulls a pre-fix client had written. + t.Run("RenameProfile", func(t *testing.T) { + withTestSM(t, func(sm *ServiceManager, username string) { + created, err := sm.AddProfile("work", username) + require.NoError(t, err) + unsetOnDisk(t, created.Path, optionalBoolFields()...) + + require.NoError(t, sm.RenameProfile(created.ID, username, "office")) + + requireNoNullOnDisk(t, created.Path, "RenameProfile") + }) + }) +} diff --git a/client/internal/profilemanager/service.go b/client/internal/profilemanager/service.go index ec287f01a..26b1d58fe 100644 --- a/client/internal/profilemanager/service.go +++ b/client/internal/profilemanager/service.go @@ -356,17 +356,17 @@ func (s *ServiceManager) RenameProfile(id ID, username string, newName string) e return ErrProfileNotFound } - data, err := os.ReadFile(target.Path) + // Through the reader, not a bare Unmarshal: this was the one write that + // skipped apply(), so it copied back whatever the file held — including an + // optional field left unset, which every other write resolves to its + // default. Renaming a profile is a poor place to leave that behind. + cfg, err := GetExistingConfig(target.Path) if err != nil { - return err - } - var cfg Config - if err := json.Unmarshal(data, &cfg); err != nil { - return err + return fmt.Errorf("read profile config: %w", err) } cfg.Name = displayName - if err := util.WriteJson(context.Background(), target.Path, cfg); err != nil { + if err := WriteOutConfig(target.Path, cfg); err != nil { return fmt.Errorf("failed to write profile name: %w", err) } return nil