diff --git a/cli/config/config.go b/cli/config/config.go index a7ea0acdd7e6..63eab640f50c 100644 --- a/cli/config/config.go +++ b/cli/config/config.go @@ -143,12 +143,15 @@ func load(configDir string) (*configfile.ConfigFile, error) { return configFile, nil } // Any other error happening when failing to read the file must be returned. - return configFile, fmt.Errorf("loading config file: %w", err) + err = fmt.Errorf("loading config file: %w", err) + configFile.LoadErr = err + return configFile, err } defer func() { _ = file.Close() }() err = configFile.LoadFromReader(file) if err != nil { err = fmt.Errorf("parsing config file (%s): %w", filename, err) + configFile.LoadErr = err } return configFile, err } @@ -159,15 +162,12 @@ func load(configDir string) (*configfile.ConfigFile, error) { // credentials-store is set in the configuration file, it attempts to discover // the default store to use for the current platform. // -// Important: LoadDefaultConfigFile prints a warning to stderr when failing to -// load the configuration file, but otherwise ignores errors. Consumers should -// consider using [Load] (and [credentials.DetectDefaultStore]) to detect errors -// when updating the configuration file, to prevent discarding a (malformed) -// configuration file. +// LoadDefaultConfigFile prints a warning when the file cannot be read and +// still returns a config so read-only commands keep working. Save refuses +// to overwrite that file. See https://github.com/docker/cli/issues/5075. func LoadDefaultConfigFile(stderr io.Writer) *configfile.ConfigFile { configFile, err := load(Dir()) if err != nil { - // FIXME(thaJeztah): we should not proceed here to prevent overwriting existing (but malformed) config files; see https://github.com/docker/cli/issues/5075 _, _ = fmt.Fprintln(stderr, "WARNING: Error", err) } if !configFile.ContainsAuth() { diff --git a/cli/config/config_test.go b/cli/config/config_test.go index 922641a726ad..b39bf6d73bdd 100644 --- a/cli/config/config_test.go +++ b/cli/config/config_test.go @@ -362,6 +362,26 @@ func TestJSONSaveWithNoFile(t *testing.T) { } } +func TestSaveRefusesInvalidConfigFile(t *testing.T) { + dir := setupConfigDir(t) + filename := filepath.Join(dir, ConfigFileName) + original := []byte(`{"imagesFormat":"some special format I have carefully hand-crafted, artisinally"`) + assert.NilError(t, os.WriteFile(filename, original, 0o600)) + + buffer := new(bytes.Buffer) + configFile := LoadDefaultConfigFile(buffer) + assert.Check(t, is.Contains(buffer.String(), "WARNING:")) + assert.Check(t, configFile.LoadErr != nil) + + configFile.CurrentContext = "other" + err := configFile.Save() + assert.Check(t, is.ErrorContains(err, "refusing to overwrite invalid config file")) + + got, readErr := os.ReadFile(filename) + assert.NilError(t, readErr) + assert.Check(t, is.DeepEqual(original, got)) +} + func TestLoadDefaultConfigFile(t *testing.T) { dir := setupConfigDir(t) buffer := new(bytes.Buffer) diff --git a/cli/config/configfile/file.go b/cli/config/configfile/file.go index e32b1e767cbb..b9f02bb71e73 100644 --- a/cli/config/configfile/file.go +++ b/cli/config/configfile/file.go @@ -75,6 +75,10 @@ type ConfigFile struct { Plugins map[string]map[string]string `json:"plugins,omitempty"` Aliases map[string]string `json:"aliases,omitempty"` Features map[string]string `json:"features,omitempty"` + + // LoadErr is set when an existing config file could not be read or parsed. + // Save refuses to overwrite the file while this is set. + LoadErr error `json:"-"` } type configEnvAuth struct { @@ -193,8 +197,13 @@ func (c *ConfigFile) SaveToWriter(writer io.Writer) error { return err } -// Save encodes and writes out all the authorization information +// Save encodes and writes out all the authorization information. +// It refuses to write when the existing file could not be loaded, so a +// malformed config.json is not replaced with an empty one. func (c *ConfigFile) Save() (retErr error) { + if c.LoadErr != nil { + return fmt.Errorf("refusing to overwrite invalid config file %s: %w", c.Filename, c.LoadErr) + } if c.Filename == "" { return errors.New("can't save config with empty filename") }