Avoid polluting config file when "save" (#25395) (#25406)

Backport #25395 by @wxiaoguang

That's a longstanding INI package problem: the "MustXxx" calls change
the option values, and the following "Save" will save a lot of garbage
options into the user's config file.

Ideally we should refactor the INI package to a clear solution, but it's
a huge work.

A clear workaround is what this PR does: when "Save", load a clear INI
instance and save it.

Partially fix #25377, the "install" page needs more fine tunes.

Co-authored-by: wxiaoguang <[email protected]>

cmd/web.go

@@ -217,9 +217,15 @@ func setPort(port string) error {
 		defaultLocalURL += ":" + setting.HTTPPort + "/"
 
 		// Save LOCAL_ROOT_URL if port changed
-		setting.CfgProvider.Section("server").Key("LOCAL_ROOT_URL").SetValue(defaultLocalURL)
-		if err := setting.CfgProvider.Save(); err != nil {
-			return fmt.Errorf("Failed to save config file: %v", err)
+		rootCfg := setting.CfgProvider
+		saveCfg, err := rootCfg.PrepareSaving()
+		if err != nil {
+			return fmt.Errorf("failed to save config file: %v", err)
+		}
+		rootCfg.Section("server").Key("LOCAL_ROOT_URL").SetValue(defaultLocalURL)
+		saveCfg.Section("server").Key("LOCAL_ROOT_URL").SetValue(defaultLocalURL)
+		if err = saveCfg.Save(); err != nil {
+			return fmt.Errorf("failed to save config file: %v", err)
 		}
 	}
 	return nil

modules/setting/config_provider.go

@@ -4,6 +4,7 @@
 package setting
 
 import (
+	"errors"
 	"fmt"
 	"os"
 	"path/filepath"
@@ -51,11 +52,17 @@ type ConfigProvider interface {
 	GetSection(name string) (ConfigSection, error)
 	Save() error
 	SaveTo(filename string) error
+
+	DisableSaving()
+	PrepareSaving() (ConfigProvider, error)
 }
 
 type iniConfigProvider struct {
-	opts    *Options
-	ini     *ini.File
+	opts *Options
+	ini  *ini.File
+
+	disableSaving bool
+
 	newFile bool // whether the file has not existed previously
 }
 
@@ -191,7 +198,7 @@ type Options struct {
 // NewConfigProviderFromFile load configuration from file.
 // NOTE: do not print any log except error.
 func NewConfigProviderFromFile(opts *Options) (ConfigProvider, error) {
-	cfg := ini.Empty()
+	cfg := ini.Empty(ini.LoadOptions{KeyValueDelimiterOnWrite: " = "})
 	newFile := true
 
 	if opts.CustomConf != "" {
@@ -252,8 +259,13 @@ func (p *iniConfigProvider) GetSection(name string) (ConfigSection, error) {
 	return &iniConfigSection{sec: sec}, nil
 }
 
+var errDisableSaving = errors.New("this config can't be saved, developers should prepare a new config to save")
+
 // Save saves the content into file
 func (p *iniConfigProvider) Save() error {
+	if p.disableSaving {
+		return errDisableSaving
+	}
 	filename := p.opts.CustomConf
 	if filename == "" {
 		if !p.opts.AllowEmpty {
@@ -285,9 +297,29 @@ func (p *iniConfigProvider) Save() error {
 }
 
 func (p *iniConfigProvider) SaveTo(filename string) error {
+	if p.disableSaving {
+		return errDisableSaving
+	}
 	return p.ini.SaveTo(filename)
 }
 
+// DisableSaving disables the saving function, use PrepareSaving to get clear config options.
+func (p *iniConfigProvider) DisableSaving() {
+	p.disableSaving = true
+}
+
+// PrepareSaving loads the ini from file again to get clear config options.
+// Otherwise, the "MustXxx" calls would have polluted the current config provider,
+// it makes the "Save" outputs a lot of garbage options
+// After the INI package gets refactored, no "MustXxx" pollution, this workaround can be dropped.
+func (p *iniConfigProvider) PrepareSaving() (ConfigProvider, error) {
+	cfgFile := p.opts.CustomConf
+	if cfgFile == "" {
+		return nil, errors.New("no config file to save")
+	}
+	return NewConfigProviderFromFile(p.opts)
+}
+
 func mustMapSetting(rootCfg ConfigProvider, sectionName string, setting any) {
 	if err := rootCfg.Section(sectionName).MapTo(setting); err != nil {
 		log.Fatal("Failed to map %s settings: %v", sectionName, err)

modules/setting/config_provider_test.go

@@ -84,11 +84,11 @@ func TestNewConfigProviderFromFile(t *testing.T) {
 
 	bs, err := os.ReadFile(testFile)
 	assert.NoError(t, err)
-	assert.Equal(t, "[foo]\nk1=a\n", string(bs))
+	assert.Equal(t, "[foo]\nk1 = a\n", string(bs))
 
 	bs, err = os.ReadFile(testFile1)
 	assert.NoError(t, err)
-	assert.Equal(t, "[foo]\nk1=a\nk2=b\n", string(bs))
+	assert.Equal(t, "[foo]\nk1 = a\nk2 = b\n", string(bs))
 
 	// load existing file and save
 	cfg, err = NewConfigProviderFromFile(&Options{CustomConf: testFile, AllowEmpty: true})
@@ -99,7 +99,7 @@ func TestNewConfigProviderFromFile(t *testing.T) {
 	assert.NoError(t, cfg.Save())
 	bs, err = os.ReadFile(testFile)
 	assert.NoError(t, err)
-	assert.Equal(t, "[foo]\nk1=a\n\n[bar]\nk1=b\n", string(bs))
+	assert.Equal(t, "[foo]\nk1 = a\n\n[bar]\nk1 = b\n", string(bs))
 }
 
 func TestNewConfigProviderForLocale(t *testing.T) {
@@ -119,3 +119,27 @@ func TestNewConfigProviderForLocale(t *testing.T) {
 	assert.Equal(t, "foo", cfg.Section("").Key("k1").String())
 	assert.Equal(t, "xxx", cfg.Section("").Key("k2").String())
 }
+
+func TestDisableSaving(t *testing.T) {
+	testFile := t.TempDir() + "/test.ini"
+	_ = os.WriteFile(testFile, []byte("k1=a\nk2=b"), 0o644)
+	cfg, err := NewConfigProviderFromFile(&Options{CustomConf: testFile, AllowEmpty: true})
+	assert.NoError(t, err)
+
+	cfg.DisableSaving()
+	err = cfg.Save()
+	assert.ErrorIs(t, err, errDisableSaving)
+
+	saveCfg, err := cfg.PrepareSaving()
+	assert.NoError(t, err)
+
+	saveCfg.Section("").Key("k1").MustString("x")
+	saveCfg.Section("").Key("k2").SetValue("y")
+	saveCfg.Section("").Key("k3").SetValue("z")
+	err = saveCfg.Save()
+	assert.NoError(t, err)
+
+	bs, err := os.ReadFile(testFile)
+	assert.NoError(t, err)
+	assert.Equal(t, "k1 = a\nk2 = y\nk3 = z\n", string(bs))
+}

modules/setting/lfs.go

@@ -59,13 +59,18 @@ func loadLFSFrom(rootCfg ConfigProvider) error {
 	if err != nil || n != 32 {
 		LFS.JWTSecretBase64, err = generate.NewJwtSecretBase64()
 		if err != nil {
-			return fmt.Errorf("Error generating JWT Secret for custom config: %v", err)
+			return fmt.Errorf("error generating JWT Secret for custom config: %v", err)
 		}
 
 		// Save secret
-		sec.Key("LFS_JWT_SECRET").SetValue(LFS.JWTSecretBase64)
-		if err := rootCfg.Save(); err != nil {
-			return fmt.Errorf("Error saving JWT Secret for custom config: %v", err)
+		saveCfg, err := rootCfg.PrepareSaving()
+		if err != nil {
+			return fmt.Errorf("error saving JWT Secret for custom config: %v", err)
+		}
+		rootCfg.Section("server").Key("LFS_JWT_SECRET").SetValue(LFS.JWTSecretBase64)
+		saveCfg.Section("server").Key("LFS_JWT_SECRET").SetValue(LFS.JWTSecretBase64)
+		if err := saveCfg.Save(); err != nil {
+			return fmt.Errorf("error saving JWT Secret for custom config: %v", err)
 		}
 	}
 

modules/setting/oauth2.go

@@ -130,8 +130,13 @@ func loadOAuth2From(rootCfg ConfigProvider) {
 			}
 
 			secretBase64 := base64.RawURLEncoding.EncodeToString(key)
+			saveCfg, err := rootCfg.PrepareSaving()
+			if err != nil {
+				log.Fatal("save oauth2.JWT_SECRET failed: %v", err)
+			}
 			rootCfg.Section("oauth2").Key("JWT_SECRET").SetValue(secretBase64)
-			if err := rootCfg.Save(); err != nil {
+			saveCfg.Section("oauth2").Key("JWT_SECRET").SetValue(secretBase64)
+			if err := saveCfg.Save(); err != nil {
 				log.Fatal("save oauth2.JWT_SECRET failed: %v", err)
 			}
 		}

modules/setting/security.go

@@ -89,8 +89,13 @@ func generateSaveInternalToken(rootCfg ConfigProvider) {
 	}
 
 	InternalToken = token
+	saveCfg, err := rootCfg.PrepareSaving()
+	if err != nil {
+		log.Fatal("Error saving internal token: %v", err)
+	}
 	rootCfg.Section("security").Key("INTERNAL_TOKEN").SetValue(token)
-	if err := rootCfg.Save(); err != nil {
+	saveCfg.Section("security").Key("INTERNAL_TOKEN").SetValue(token)
+	if err = saveCfg.Save(); err != nil {
 		log.Fatal("Error saving internal token: %v", err)
 	}
 }

modules/setting/setting.go

@@ -202,6 +202,7 @@ func Init(opts *Options) {
 	}
 	var err error
 	CfgProvider, err = NewConfigProviderFromFile(opts)
+	CfgProvider.DisableSaving() // do not allow saving the CfgProvider into file, it will be polluted by the "MustXxx" calls
 	if err != nil {
 		log.Fatal("newConfigProviderFromFile[%v]: %v", opts, err)
 	}
@@ -214,7 +215,7 @@ func Init(opts *Options) {
 
 // loadCommonSettingsFrom loads common configurations from a configuration provider.
 func loadCommonSettingsFrom(cfg ConfigProvider) error {
-	// WARNNING: don't change the sequence except you know what you are doing.
+	// WARNING: don't change the sequence except you know what you are doing.
 	loadRunModeFrom(cfg)
 	loadLogGlobalFrom(cfg)
 	loadServerFrom(cfg)