config: refuse to save a config file that failed to load

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() {

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)

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")
 	}