From 3b3c37dd0ce87184b2f67b90864cf9288f8727b1 Mon Sep 17 00:00:00 2001 From: Roland <33993199+rolznz@users.noreply.github.com> Date: Wed, 12 Aug 2026 14:48:06 +0700 Subject: [PATCH] fix: remove legacy acceptance of empty unlock password check (#2534) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix: remove legacy acceptance of empty unlock password check CheckUnlockPassword previously treated a missing or empty UnlockPasswordCheck value as a match — a legacy compatibility path from before the canary was always written. It now requires the stored value to be present and to equal the expected string. StartApp checks for the canary up front and, if it is missing, stops with a message asking the user to restore from a backup rather than continuing. A new IsUnlockPasswordCheckSet helper reports whether the value is present. keys.Init now returns the error from reading NostrSecretKey instead of ignoring it, so a read failure aborts instead of generating and saving a new key. Co-Authored-By: Claude Fable 5 * chore: add operation context to unlock password check errors Co-Authored-By: Claude Fable 5 --------- Co-authored-by: Claude Fable 5 --- api/backup_test.go | 8 +++-- config/config.go | 13 +++++++- config/config_test.go | 2 -- config/models.go | 1 + config/tests/config_test.go | 61 +++++++++++++++++++++++++++++-------- service/keys/keys.go | 8 +++-- service/keys/keys_test.go | 32 +++++++++++++++++++ service/start.go | 9 ++++++ tests/mocks/Config.go | 53 ++++++++++++++++++++++++++++++++ 9 files changed, 167 insertions(+), 20 deletions(-) diff --git a/api/backup_test.go b/api/backup_test.go index 784367f9..27bc112a 100644 --- a/api/backup_test.go +++ b/api/backup_test.go @@ -39,6 +39,12 @@ func TestCreateBackup(t *testing.T) { cfg, err := config.NewConfig(appConfig, gormDB) require.NoError(t, err) + unlockPassword := "" + + // Represent a fully set-up hub: the unlock-password canary is written during + // setup and is required for the password check to pass. + require.NoError(t, cfg.SaveUnlockPasswordCheck(unlockPassword)) + app := &db.App{ Name: "test", AppPubkey: "2b7dea2866958f17c568cf024e113db7a3baa9c253a9016889196b8d0b11c7ae", @@ -64,8 +70,6 @@ func TestCreateBackup(t *testing.T) { albyOAuthSvc: albyOAuthSvc, } - unlockPassword := "" - var buf bytes.Buffer err = theAPI.CreateBackup(unlockPassword, &buf) require.NoError(t, err) diff --git a/config/config.go b/config/config.go index 57a17685..c0434665 100644 --- a/config/config.go +++ b/config/config.go @@ -408,7 +408,18 @@ func (cfg *config) SetAutoUnlockPassword(unlockPassword string) error { func (cfg *config) CheckUnlockPassword(encryptionKey string) bool { decryptedValue, err := cfg.Get("UnlockPasswordCheck", encryptionKey) - return err == nil && (decryptedValue == "" || decryptedValue == unlockPasswordCheck) + // require a non-empty match so an absent or empty canary always fails + return err == nil && decryptedValue != "" && decryptedValue == unlockPasswordCheck +} + +func (cfg *config) IsUnlockPasswordCheckSet() (bool, error) { + // Read the raw value with an empty encryption key so we can detect the + // presence of the canary row without needing the (possibly wrong) password. + value, err := cfg.Get("UnlockPasswordCheck", "") + if err != nil { + return false, fmt.Errorf("read unlock password check: %w", err) + } + return value != "", nil } func (cfg *config) SaveUnlockPasswordCheck(encryptionKey string) error { diff --git a/config/config_test.go b/config/config_test.go index 302c120c..1849e45a 100644 --- a/config/config_test.go +++ b/config/config_test.go @@ -56,8 +56,6 @@ func TestCheckUnlockPasswordCache(t *testing.T) { Workdir: ".test", }, db) require.NoError(t, err) - err = cfg.ChangeUnlockPassword("", unlockPassword) - require.NoError(t, err) err = cfg.SaveUnlockPasswordCheck(unlockPassword) require.NoError(t, err) diff --git a/config/models.go b/config/models.go index 0fb88709..48860104 100644 --- a/config/models.go +++ b/config/models.go @@ -94,6 +94,7 @@ type Config interface { GetMempoolUrl() string GetEnv() *AppConfig CheckUnlockPassword(password string) bool + IsUnlockPasswordCheckSet() (bool, error) ChangeUnlockPassword(currentUnlockPassword string, newUnlockPassword string) error SetAutoUnlockPassword(unlockPassword string) error SaveUnlockPasswordCheck(encryptionKey string) error diff --git a/config/tests/config_test.go b/config/tests/config_test.go index ae1bfd0d..c256f542 100644 --- a/config/tests/config_test.go +++ b/config/tests/config_test.go @@ -16,8 +16,6 @@ func TestCheckUnlockPasswordCache_InvalidSecond(t *testing.T) { require.NoError(t, err) defer svc.Remove() - err = svc.Cfg.ChangeUnlockPassword("", unlockPassword) - require.NoError(t, err) err = svc.Cfg.SaveUnlockPasswordCheck(unlockPassword) require.NoError(t, err) @@ -35,8 +33,6 @@ func TestCheckUnlockPasswordCache_InvalidFirst(t *testing.T) { require.NoError(t, err) defer svc.Remove() - err = svc.Cfg.ChangeUnlockPassword("", unlockPassword) - require.NoError(t, err) err = svc.Cfg.SaveUnlockPasswordCheck(unlockPassword) require.NoError(t, err) @@ -58,8 +54,6 @@ func TestCheckUnlockPassword_ChangePassword(t *testing.T) { require.NoError(t, err) defer svc.Remove() - err = svc.Cfg.ChangeUnlockPassword("", unlockPassword) - require.NoError(t, err) err = svc.Cfg.SaveUnlockPasswordCheck(unlockPassword) require.NoError(t, err) @@ -81,6 +75,50 @@ func TestCheckUnlockPassword_ChangePassword(t *testing.T) { assert.True(t, svc.Cfg.CheckUnlockPassword(newUnlockPassword)) } +func TestCheckUnlockPassword_MissingCanaryFailsClosed(t *testing.T) { + svc, err := tests.CreateTestService(t) + require.NoError(t, err) + defer svc.Remove() + + // A fresh hub has not saved the unlock-password canary yet. + set, err := svc.Cfg.IsUnlockPasswordCheckSet() + require.NoError(t, err) + assert.False(t, set) + + // Without the canary, no password may validate - including an empty one. + assert.False(t, svc.Cfg.CheckUnlockPassword("")) + assert.False(t, svc.Cfg.CheckUnlockPassword("any-password")) + + // After the canary is saved, only the correct password validates. + err = svc.Cfg.SaveUnlockPasswordCheck("correct") + require.NoError(t, err) + + set, err = svc.Cfg.IsUnlockPasswordCheckSet() + require.NoError(t, err) + assert.True(t, set) + + assert.True(t, svc.Cfg.CheckUnlockPassword("correct")) + assert.False(t, svc.Cfg.CheckUnlockPassword("wrong")) + assert.False(t, svc.Cfg.CheckUnlockPassword("")) +} + +func TestCheckUnlockPassword_NoPasswordHub(t *testing.T) { + svc, err := tests.CreateTestService(t) + require.NoError(t, err) + defer svc.Remove() + + // A hub configured without an unlock password stores the canary unencrypted; + // the empty password must still validate after the fail-closed change. + err = svc.Cfg.SaveUnlockPasswordCheck("") + require.NoError(t, err) + + set, err := svc.Cfg.IsUnlockPasswordCheckSet() + require.NoError(t, err) + assert.True(t, set) + + assert.True(t, svc.Cfg.CheckUnlockPassword("")) +} + func TestSetIgnore_NoEncryptionKey(t *testing.T) { svc, err := tests.CreateTestService(t) require.NoError(t, err) @@ -219,7 +257,7 @@ func TestJWTSecret_GeneratedOnLoad(t *testing.T) { cfg, err := config.NewConfig(&config.AppConfig{}, svc.DB) require.NoError(t, err) - err = cfg.ChangeUnlockPassword("", "123") + err = cfg.SaveUnlockPasswordCheck("123") require.NoError(t, err) err = cfg.LoadJWTSecret("123") @@ -251,9 +289,6 @@ func TestJWTSecret_WrongPassword(t *testing.T) { cfg, err := config.NewConfig(&config.AppConfig{}, svc.DB) require.NoError(t, err) - err = cfg.ChangeUnlockPassword("", "123") - require.NoError(t, err) - err = cfg.SaveUnlockPasswordCheck("123") require.NoError(t, err) @@ -272,7 +307,7 @@ func TestJWTSecret_ChangePassword(t *testing.T) { cfg, err := config.NewConfig(&config.AppConfig{}, svc.DB) require.NoError(t, err) - err = cfg.ChangeUnlockPassword("", "123") + err = cfg.SaveUnlockPasswordCheck("123") require.NoError(t, err) err = cfg.LoadJWTSecret("123") @@ -282,7 +317,7 @@ func TestJWTSecret_ChangePassword(t *testing.T) { require.NoError(t, err) assert.NotEmpty(t, jwtSecret) - err = cfg.ChangeUnlockPassword("", "1234") + err = cfg.ChangeUnlockPassword("123", "1234") require.NoError(t, err) newJwtSecret, err := cfg.GetJWTSecret() @@ -306,7 +341,7 @@ func TestJWTSecret_ReplaceUnencryptedSecretOnLoad(t *testing.T) { cfg, err := config.NewConfig(&config.AppConfig{}, svc.DB) require.NoError(t, err) - err = cfg.ChangeUnlockPassword("", "123") + err = cfg.SaveUnlockPasswordCheck("123") require.NoError(t, err) // simulate a hub that had an unencrypted JWT secret diff --git a/service/keys/keys.go b/service/keys/keys.go index 039efc1d..1a437d2d 100644 --- a/service/keys/keys.go +++ b/service/keys/keys.go @@ -46,11 +46,15 @@ func NewKeys() *keys { } func (keys *keys) Init(cfg config.Config, encryptionKey string) error { - nostrSecretKey, _ := cfg.Get("NostrSecretKey", encryptionKey) + nostrSecretKey, err := cfg.Get("NostrSecretKey", encryptionKey) + if err != nil { + logger.Logger.WithError(err).Error("Failed to decrypt nostr secret key") + return err + } if nostrSecretKey == "" { nostrSecretKey = nostr.GeneratePrivateKey() - err := cfg.SetUpdate("NostrSecretKey", nostrSecretKey, encryptionKey) + err = cfg.SetUpdate("NostrSecretKey", nostrSecretKey, encryptionKey) if err != nil { logger.Logger.WithError(err).Error("Failed to save generated nostr secret key") return err diff --git a/service/keys/keys_test.go b/service/keys/keys_test.go index 9e8a5131..252fb650 100644 --- a/service/keys/keys_test.go +++ b/service/keys/keys_test.go @@ -110,6 +110,38 @@ func TestGenerateNewMnemonic(t *testing.T) { assert.Equal(t, encryptedChannelsBackupKey.String(), derivedKeyFromKeys.String()) } +func TestInit_WrongPasswordDoesNotOverwriteNostrKey(t *testing.T) { + logger.Init(strconv.Itoa(int(logrus.DebugLevel))) + gormDb, err := db.NewDB(t) + require.NoError(t, err) + defer db.CloseDB(gormDb) + + unlockPassword := "correct" + + cfg, err := config.NewConfig(&config.AppConfig{}, gormDb) + require.NoError(t, err) + + // initialise keys under the correct password, storing an encrypted NostrSecretKey + keys := NewKeys() + err = keys.Init(cfg, unlockPassword) + require.NoError(t, err) + + originalSecret, err := cfg.Get("NostrSecretKey", unlockPassword) + require.NoError(t, err) + require.NotEmpty(t, originalSecret) + + // a wrong password must abort instead of mistaking the failed decrypt for + // "no key yet" and overwriting the stored key with a freshly generated one + keys2 := NewKeys() + err = keys2.Init(cfg, "wrong") + require.Error(t, err) + + // the stored key, decrypted with the correct password, must be unchanged + secretAfter, err := cfg.Get("NostrSecretKey", unlockPassword) + require.NoError(t, err) + assert.Equal(t, originalSecret, secretAfter) +} + func TestGenerateSwapMnemonic(t *testing.T) { logger.Init(strconv.Itoa(int(logrus.DebugLevel))) gormDb, err := db.NewDB(t) diff --git a/service/start.go b/service/start.go index 7ea158a9..8c9786a7 100644 --- a/service/start.go +++ b/service/start.go @@ -273,6 +273,15 @@ func (svc *service) StartApp(encryptionKey string) error { if svc.lnClient != nil { return errors.New("app already started") } + unlockPasswordCheckSet, err := svc.cfg.IsUnlockPasswordCheckSet() + if err != nil { + logger.Logger.WithError(err).Error("Failed to check unlock password check") + return fmt.Errorf("check unlock password check: %w", err) + } + if !unlockPasswordCheckSet { + logger.Logger.Error("Unlock password check is missing from the database") + return errors.New("your wallet data is incomplete and cannot be unlocked. Please restore from a backup") + } if !svc.cfg.CheckUnlockPassword(encryptionKey) { logger.Logger.Errorf("Invalid password") return errors.New("invalid password") diff --git a/tests/mocks/Config.go b/tests/mocks/Config.go index 47adb71c..d10f83c2 100644 --- a/tests/mocks/Config.go +++ b/tests/mocks/Config.go @@ -531,6 +531,59 @@ func (_c *MockConfig_GetRelayUrls_Call) RunAndReturn(run func() []string) *MockC return _c } +// IsUnlockPasswordCheckSet provides a mock function for the type MockConfig +func (_mock *MockConfig) IsUnlockPasswordCheckSet() (bool, error) { + ret := _mock.Called() + + if len(ret) == 0 { + panic("no return value specified for IsUnlockPasswordCheckSet") + } + + var r0 bool + var r1 error + if returnFunc, ok := ret.Get(0).(func() (bool, error)); ok { + return returnFunc() + } + if returnFunc, ok := ret.Get(0).(func() bool); ok { + r0 = returnFunc() + } else { + r0 = ret.Get(0).(bool) + } + if returnFunc, ok := ret.Get(1).(func() error); ok { + r1 = returnFunc() + } else { + r1 = ret.Error(1) + } + return r0, r1 +} + +// MockConfig_IsUnlockPasswordCheckSet_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'IsUnlockPasswordCheckSet' +type MockConfig_IsUnlockPasswordCheckSet_Call struct { + *mock.Call +} + +// IsUnlockPasswordCheckSet is a helper method to define mock.On call +func (_e *MockConfig_Expecter) IsUnlockPasswordCheckSet() *MockConfig_IsUnlockPasswordCheckSet_Call { + return &MockConfig_IsUnlockPasswordCheckSet_Call{Call: _e.mock.On("IsUnlockPasswordCheckSet")} +} + +func (_c *MockConfig_IsUnlockPasswordCheckSet_Call) Run(run func()) *MockConfig_IsUnlockPasswordCheckSet_Call { + _c.Call.Run(func(args mock.Arguments) { + run() + }) + return _c +} + +func (_c *MockConfig_IsUnlockPasswordCheckSet_Call) Return(b bool, err error) *MockConfig_IsUnlockPasswordCheckSet_Call { + _c.Call.Return(b, err) + return _c +} + +func (_c *MockConfig_IsUnlockPasswordCheckSet_Call) RunAndReturn(run func() (bool, error)) *MockConfig_IsUnlockPasswordCheckSet_Call { + _c.Call.Return(run) + return _c +} + // LoadJWTSecret provides a mock function for the type MockConfig func (_mock *MockConfig) LoadJWTSecret(encryptionKey string) error { ret := _mock.Called(encryptionKey)