fix: remove legacy acceptance of empty unlock password check (#2534)

* 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 <noreply@anthropic.com>

* chore: add operation context to unlock password check errors

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

---------

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>

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)

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 {

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)
 

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

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

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

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)

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

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)