From 8302b95d6bfdbc40083a72e5d1b727fcb1e00940 Mon Sep 17 00:00:00 2001 From: Giteabot Date: Wed, 21 Jun 2023 00:51:26 -0400 Subject: [PATCH] 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 --- cmd/web.go | 12 ++++++-- modules/setting/config_provider.go | 38 +++++++++++++++++++++++-- modules/setting/config_provider_test.go | 30 +++++++++++++++++-- modules/setting/lfs.go | 13 ++++++--- modules/setting/oauth2.go | 7 ++++- modules/setting/security.go | 7 ++++- modules/setting/setting.go | 3 +- 7 files changed, 94 insertions(+), 16 deletions(-) diff --git a/cmd/web.go b/cmd/web.go index da6c987ff8..5444c80008 100644 --- a/cmd/web.go +++ b/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 diff --git a/modules/setting/config_provider.go b/modules/setting/config_provider.go index 526d69bbdc..deec5cc586 100644 --- a/modules/setting/config_provider.go +++ b/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) diff --git a/modules/setting/config_provider_test.go b/modules/setting/config_provider_test.go index 17650edea4..c5c5196e04 100644 --- a/modules/setting/config_provider_test.go +++ b/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)) +} diff --git a/modules/setting/lfs.go b/modules/setting/lfs.go index d68349be86..140a96f9ed 100644 --- a/modules/setting/lfs.go +++ b/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) } } diff --git a/modules/setting/oauth2.go b/modules/setting/oauth2.go index 836a2bb25f..83c607a416 100644 --- a/modules/setting/oauth2.go +++ b/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) } } diff --git a/modules/setting/security.go b/modules/setting/security.go index ce2e7711f1..c39eb7f3eb 100644 --- a/modules/setting/security.go +++ b/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) } } diff --git a/modules/setting/setting.go b/modules/setting/setting.go index 539eb4b197..6eaddbe2b5 100644 --- a/modules/setting/setting.go +++ b/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)