From d8da2f49e453649172e05d5fb8928062c229763e Mon Sep 17 00:00:00 2001 From: Earl Warren Date: Mon, 20 Jan 2025 22:42:26 +0100 Subject: [PATCH] fix: ignore orphaned two_factor failing upgrade to v10 with invalid decrypted base64 If a row in the two_factor table references a non existent user, it may contain a secret that has an invalid format. Such an orphaned row is never used and should be removed. Instead of blocking an upgrade when a database migration fails to convert the secret because it does not contain a base64 string, it is ignored. A warning is displayed to suggest the row is removed and clarify that it does not require action. It is safe to ignore. --- models/forgejo_migrations/v25.go | 10 ++++++++++ models/forgejo_migrations/v25_test.go | 10 ++++++++-- .../Test_MigrateTwoFactorToKeying/two_factor.yml | 5 +++++ 3 files changed, 23 insertions(+), 2 deletions(-) diff --git a/models/forgejo_migrations/v25.go b/models/forgejo_migrations/v25.go index e2316007cf..03df2d32df 100644 --- a/models/forgejo_migrations/v25.go +++ b/models/forgejo_migrations/v25.go @@ -10,6 +10,8 @@ import ( "code.gitea.io/gitea/models/auth" "code.gitea.io/gitea/models/db" + "code.gitea.io/gitea/models/user" + "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/secret" "code.gitea.io/gitea/modules/setting" @@ -58,6 +60,14 @@ func MigrateTwoFactorToKeying(x *xorm.Engine) error { oldEncryptionKey := md5.Sum([]byte(setting.SecretKey)) return db.Iterate(context.Background(), nil, func(ctx context.Context, bean *auth.TwoFactor) error { + if _, err := user.GetUserByID(context.Background(), bean.UID); err != nil { + if user.IsErrUserNotExist(err) { + log.Warn("two_factor.id = %d references non existent user id %d. It is harmless and was ignored but should be removed", bean.ID, bean.UID) + return nil + } + return err + } + decodedStoredSecret, err := base64.StdEncoding.DecodeString(string(bean.Secret)) if err != nil { return err diff --git a/models/forgejo_migrations/v25_test.go b/models/forgejo_migrations/v25_test.go index 43c5885c39..fff5aaab61 100644 --- a/models/forgejo_migrations/v25_test.go +++ b/models/forgejo_migrations/v25_test.go @@ -8,6 +8,7 @@ import ( "code.gitea.io/gitea/models/auth" migration_tests "code.gitea.io/gitea/models/migrations/test" + "code.gitea.io/gitea/models/user" "code.gitea.io/gitea/modules/keying" "code.gitea.io/gitea/modules/timeutil" @@ -28,7 +29,7 @@ func Test_MigrateTwoFactorToKeying(t *testing.T) { } // Prepare and load the testing database - x, deferable := migration_tests.PrepareTestEnv(t, 0, new(TwoFactor)) + x, deferable := migration_tests.PrepareTestEnv(t, 0, new(TwoFactor), new(user.User)) defer deferable() if x == nil || t.Failed() { return @@ -36,7 +37,7 @@ func Test_MigrateTwoFactorToKeying(t *testing.T) { cnt, err := x.Table("two_factor").Count() require.NoError(t, err) - assert.EqualValues(t, 1, cnt) + assert.EqualValues(t, 2, cnt) require.NoError(t, MigrateTwoFactorToKeying(x)) @@ -47,4 +48,9 @@ func Test_MigrateTwoFactorToKeying(t *testing.T) { secretBytes, err := keying.DeriveKey(keying.ContextTOTP).Decrypt(twofactor.Secret, keying.ColumnAndID("secret", twofactor.ID)) require.NoError(t, err) assert.Equal(t, []byte("AVDYS32OPIAYSNBG2NKYV4AHBVEMKKKIGBQ46OXTLMJO664G4TIECOGEANMSNBLS"), secretBytes) + + var twofactorOrphaned auth.TwoFactor + _, err = x.Table("two_factor").ID(2).Get(&twofactorOrphaned) + require.NoError(t, err) + assert.Equal(t, []byte("CORRUPTED_SECRET"), twofactorOrphaned.Secret) } diff --git a/models/migrations/fixtures/Test_MigrateTwoFactorToKeying/two_factor.yml b/models/migrations/fixtures/Test_MigrateTwoFactorToKeying/two_factor.yml index ef6c158659..a08014261c 100644 --- a/models/migrations/fixtures/Test_MigrateTwoFactorToKeying/two_factor.yml +++ b/models/migrations/fixtures/Test_MigrateTwoFactorToKeying/two_factor.yml @@ -7,3 +7,8 @@ last_used_passcode: created_unix: 1564253724 updated_unix: 1564253724 + +- + id: 2 + uid: 2000 + secret: CORRUPTED_SECRET