From ca7cf1162ff429070e4672f6b221386c1db2c376 Mon Sep 17 00:00:00 2001 From: Gani Georgiev Date: Sun, 26 Apr 2026 11:40:09 +0300 Subject: [PATCH] added App.DeleteAllExternalAuthsByRecord --- CHANGELOG.md | 7 +- apis/record_auth_email_change_confirm_test.go | 41 ++- ...record_auth_password_reset_confirm_test.go | 36 ++- apis/record_auth_verification_confirm_test.go | 45 ++++ apis/record_auth_with_oauth2.go | 29 ++- apis/record_auth_with_oauth2_test.go | 246 +++++++++++++++++- apis/record_auth_with_otp.go | 11 +- apis/record_auth_with_otp_test.go | 41 ++- core/app.go | 5 + core/external_auth_query.go | 24 ++ core/external_auth_query_test.go | 66 +++++ core/record_model.go | 58 ++++- 12 files changed, 567 insertions(+), 42 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7dfa5fd3..bef3f4e1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,9 +8,12 @@ - Fixed autocomplete selection not properly updating the underlying input value ([#7664](https://github.com/pocketbase/pocketbase/issues/7664)). -- Adjusted Bitbucket, GitHub and Gitea/Forgejo OAuth2 providers to better reflect recent API updates and doc references. - _The providers also now always send an extra emails_list request to fetch only the explicitly verified primary email in order to eliminate eventual security issues caused by misconfigured onpremise setups._ +- Added dummy bcrypt password check for the failure auth path to minimize enumaration timing attacks. +- Adjusted Bitbucket, GitHub and Gitea/Forgejo OAuth2 providers to better reflect recent API updates and doc references. + _The providers also now always send their respective dedicated emails_list request to fetch only the verified primary email in order to minimize eventual linking security issues caused by specific onpremise setups._ + +- ⚠️ Fixed a pre-hijacking OAuth2 linking vulnerability ([#7662](https://github.com/pocketbase/pocketbase/discussions/7662); thanks @Alardiians for reporting it privately). ## v0.37.3 diff --git a/apis/record_auth_email_change_confirm_test.go b/apis/record_auth_email_change_confirm_test.go index bf56be76..2744dddf 100644 --- a/apis/record_auth_email_change_confirm_test.go +++ b/apis/record_auth_email_change_confirm_test.go @@ -111,12 +111,51 @@ func TestRecordConfirmEmailChange(t *testing.T) { "OnRecordUpdateExecute": 1, "OnRecordAfterUpdateSuccess": 1, "OnRecordValidate": 1, + // unverified->verified external auths removal + "OnModelDelete": 2, + "OnModelDeleteExecute": 2, + "OnModelAfterDeleteSuccess": 2, + "OnRecordDelete": 2, + "OnRecordDeleteExecute": 2, + "OnRecordAfterDeleteSuccess": 2, + }, + BeforeTestFunc: func(t testing.TB, app *tests.TestApp, e *core.ServeEvent) { + user, err := app.FindAuthRecordByEmail("users", "test@example.com") + if err != nil { + t.Fatal(err) + } + + if user.Verified() { + t.Fatalf("Expected the user to be unverified before the confirmation") + } + + // ensure that there is at least one pre-existing OAuth2 link + externalAuths, err := app.FindAllExternalAuthsByRecord(user) + if err != nil { + t.Fatal(err) + } + if len(externalAuths) == 0 { + t.Fatal("Expected at least one external auths") + } }, AfterTestFunc: func(t testing.TB, app *tests.TestApp, res *http.Response) { - _, err := app.FindAuthRecordByEmail("users", "change@example.com") + user, err := app.FindAuthRecordByEmail("users", "change@example.com") if err != nil { t.Fatalf("Expected to find user with email %q, got error: %v", "change@example.com", err) } + + if !user.Verified() { + t.Fatalf("Expected the user to be verified after the confirmation") + } + + // ensure that all pre-existing OAuth2 links are cleared + externalAuths, err := app.FindAllExternalAuthsByRecord(user) + if err != nil { + t.Fatal(err) + } + if len(externalAuths) > 0 { + t.Fatalf("Expected all external auths to be cleared, found %d", len(externalAuths)) + } }, }, { diff --git a/apis/record_auth_password_reset_confirm_test.go b/apis/record_auth_password_reset_confirm_test.go index fff8f2b3..8b87e6d9 100644 --- a/apis/record_auth_password_reset_confirm_test.go +++ b/apis/record_auth_password_reset_confirm_test.go @@ -114,11 +114,18 @@ func TestRecordConfirmPasswordReset(t *testing.T) { "OnModelUpdate": 1, "OnModelUpdateExecute": 1, "OnModelAfterUpdateSuccess": 1, - "OnModelValidate": 1, "OnRecordUpdate": 1, "OnRecordUpdateExecute": 1, "OnRecordAfterUpdateSuccess": 1, + "OnModelValidate": 1, "OnRecordValidate": 1, + // --- + "OnModelDelete": 2, // pre-existing OAuth2 links + "OnModelDeleteExecute": 2, + "OnModelAfterDeleteSuccess": 2, + "OnRecordDelete": 2, + "OnRecordDeleteExecute": 2, + "OnRecordAfterDeleteSuccess": 2, }, BeforeTestFunc: func(t testing.TB, app *tests.TestApp, e *core.ServeEvent) { user, err := app.FindAuthRecordByEmail("users", "test@example.com") @@ -151,6 +158,15 @@ func TestRecordConfirmPasswordReset(t *testing.T) { if !user.ValidatePassword("1234567!") { t.Fatal("Password wasn't changed") } + + // ensure that all pre-existing OAuth2 links are cleared + externalAuths, err := app.FindAllExternalAuthsByRecord(user) + if err != nil { + t.Fatal(err) + } + if len(externalAuths) > 0 { + t.Fatalf("Expected all external auths to be cleared, found %d", len(externalAuths)) + } }, }, { @@ -221,6 +237,15 @@ func TestRecordConfirmPasswordReset(t *testing.T) { if !user.ValidatePassword("1234567!") { t.Fatal("Password wasn't changed") } + + // ensure that all pre-existing OAuth2 were NOT deleted + externalAuths, err := app.FindAllExternalAuthsByRecord(user) + if err != nil { + t.Fatal(err) + } + if len(externalAuths) != 2 { + t.Fatalf("Expected 2 external auths, found %d", len(externalAuths)) + } }, }, { @@ -251,11 +276,20 @@ func TestRecordConfirmPasswordReset(t *testing.T) { t.Fatalf("Failed to fetch confirm password user: %v", err) } + oldTokenKey := user.TokenKey() + // ensure that the user is already verified user.SetVerified(true) if err := app.Save(user); err != nil { t.Fatalf("Failed to update user verified state") } + + // resave with the old token key since the verified change above + // would refresh it and will make the password token invalid + user.SetTokenKey(oldTokenKey) + if err = app.Save(user); err != nil { + t.Fatalf("Failed to restore original user tokenKey: %v", err) + } }, AfterTestFunc: func(t testing.TB, app *tests.TestApp, res *http.Response) { _, err := app.FindAuthRecordByToken( diff --git a/apis/record_auth_verification_confirm_test.go b/apis/record_auth_verification_confirm_test.go index ef48c7c6..d291b056 100644 --- a/apis/record_auth_verification_confirm_test.go +++ b/apis/record_auth_verification_confirm_test.go @@ -105,6 +105,51 @@ func TestRecordConfirmVerification(t *testing.T) { "OnRecordValidate": 1, "OnRecordUpdateExecute": 1, "OnRecordAfterUpdateSuccess": 1, + // unverified->verified external auths removal + "OnModelDelete": 2, + "OnModelDeleteExecute": 2, + "OnModelAfterDeleteSuccess": 2, + "OnRecordDelete": 2, + "OnRecordDeleteExecute": 2, + "OnRecordAfterDeleteSuccess": 2, + }, + BeforeTestFunc: func(t testing.TB, app *tests.TestApp, e *core.ServeEvent) { + user, err := app.FindAuthRecordByEmail("users", "test@example.com") + if err != nil { + t.Fatal(err) + } + + if user.Verified() { + t.Fatalf("Expected the user to be unverified before the confirmation") + } + + // ensure that there is at least one pre-existing OAuth2 link + externalAuths, err := app.FindAllExternalAuthsByRecord(user) + if err != nil { + t.Fatal(err) + } + if len(externalAuths) == 0 { + t.Fatal("Expected at least one external auths") + } + }, + AfterTestFunc: func(t testing.TB, app *tests.TestApp, res *http.Response) { + user, err := app.FindAuthRecordByEmail("users", "test@example.com") + if err != nil { + t.Fatal(err) + } + + if !user.Verified() { + t.Fatalf("Expected the user to be verified after the confirmation") + } + + // ensure that all pre-existing OAuth2 links are cleared + externalAuths, err := app.FindAllExternalAuthsByRecord(user) + if err != nil { + t.Fatal(err) + } + if len(externalAuths) > 0 { + t.Fatalf("Expected all external auths to be cleared, found %d", len(externalAuths)) + } }, }, { diff --git a/apis/record_auth_with_oauth2.go b/apis/record_auth_with_oauth2.go index 6bdd5d36..9611ad79 100644 --- a/apis/record_auth_with_oauth2.go +++ b/apis/record_auth_with_oauth2.go @@ -338,26 +338,43 @@ func oauth2Submit(e *core.RecordAuthWithOAuth2RequestEvent, optExternalAuth *cor e.Auth.Id == e.Record.Id && e.Auth.Collection().Id == e.Record.Collection().Id - // set random password for users with unverified email - // (this is in case a malicious actor has registered previously with the user email) - if !isLoggedAuthRecord && e.Record.Email() != "" && !e.Record.Verified() { - e.Record.SetRandomPassword() + // prevent pre-hijacking with password auth + // + // reset the unverified user password in case the record was precreated by a malicious actor + if !isLoggedAuthRecord && !e.Record.Verified() { needUpdate = true + e.Record.SetRandomPassword() + } + + // prevent pre-hijacking with different OAuth2 provider + // + // delete all other previous OAuth2 record links for the cases + // when the user was precreated by malicious OAuth2 auth with custom payload data + // + // while this would be also done automatically on unverified -> verified upgrade, + // doing it manually here ensures that a single unverified record could have + // max 1 OAuth2 link to prevent further abuse when mixed with other auth flows + if !e.Record.Verified() { + err := txApp.DeleteAllExternalAuthsByRecord(e.Record) + if err != nil { + return err + } + optExternalAuth = nil // clear to allow recreate below } // update the existing auth record empty email if the data.OAuth2User has one // (this is in case previously the auth record was created // with an OAuth2 provider that didn't return an email address) if e.Record.Email() == "" && e.OAuth2User.Email != "" { - e.Record.SetEmail(e.OAuth2User.Email) needUpdate = true + e.Record.SetEmail(e.OAuth2User.Email) } // update the existing auth record verified state // (only if the auth record doesn't have an email or the auth record email match with the one in data.OAuth2User) if !e.Record.Verified() && (e.Record.Email() == "" || e.Record.Email() == e.OAuth2User.Email) { - e.Record.SetVerified(true) needUpdate = true + e.Record.SetVerified(true) } if needUpdate { diff --git a/apis/record_auth_with_oauth2_test.go b/apis/record_auth_with_oauth2_test.go index d07a3091..72357cf1 100644 --- a/apis/record_auth_with_oauth2_test.go +++ b/apis/record_auth_with_oauth2_test.go @@ -178,6 +178,20 @@ func TestRecordAuthWithOAuth2(t *testing.T) { t.Fatal(err) } + // ensure that there is at least one other external auth different than test + // so that later we can verify that it was deleted + var hasAtLeastOneOtherEA = false + externalAuths, _ := app.FindAllExternalAuthsByRecord(user) + for _, rel := range externalAuths { + if rel.Id != ea.Id { + hasAtLeastOneOtherEA = true + break + } + } + if !hasAtLeastOneOtherEA { + t.Fatal("Expected at least one non-test external auth linked") + } + // test at least once that the correct request info context is properly loaded app.OnRecordAuthRequest().BindFunc(func(e *core.RecordAuthRequestEvent) error { info, err := e.RequestInfo() @@ -213,12 +227,12 @@ func TestRecordAuthWithOAuth2(t *testing.T) { "OnRecordAuthRequest": 1, "OnRecordEnrich": 1, // --- - "OnModelCreate": 1, - "OnModelCreateExecute": 1, - "OnModelAfterCreateSuccess": 1, - "OnRecordCreate": 1, - "OnRecordCreateExecute": 1, - "OnRecordAfterCreateSuccess": 1, + "OnModelCreate": 2, // user + recreated external auth + "OnModelCreateExecute": 2, + "OnModelAfterCreateSuccess": 2, + "OnRecordCreate": 2, + "OnRecordCreateExecute": 2, + "OnRecordAfterCreateSuccess": 2, // --- "OnModelUpdate": 1, "OnModelUpdateExecute": 1, @@ -227,8 +241,15 @@ func TestRecordAuthWithOAuth2(t *testing.T) { "OnRecordUpdateExecute": 1, "OnRecordAfterUpdateSuccess": 1, // --- - "OnModelValidate": 2, // create + update - "OnRecordValidate": 2, + "OnModelDelete": 3, // pre-existing external auths + "OnModelDeleteExecute": 3, + "OnModelAfterDeleteSuccess": 3, + "OnRecordDelete": 3, + "OnRecordDeleteExecute": 3, + "OnRecordAfterDeleteSuccess": 3, + // --- + "OnModelValidate": 3, // user create/update + recreated external auth + "OnRecordValidate": 3, }, AfterTestFunc: func(t testing.TB, app *tests.TestApp, res *http.Response) { user, err := app.FindAuthRecordByEmail("users", "test@example.com") @@ -248,6 +269,24 @@ func TestRecordAuthWithOAuth2(t *testing.T) { if len(devices) != 1 { t.Fatalf("Expected only 1 auth origin to be created, got %d (%v)", len(devices), err) } + + // ensure that other linked external auths have been deleted + externalAuths, _ := app.FindAllExternalAuthsByRecord(user) + if len(externalAuths) != 1 { + t.Fatalf("Expected only 1 external auth to remain, got %d", len(externalAuths)) + } + if provider := externalAuths[0].Provider(); provider != "test" { + t.Fatalf("Expected %q external auth, got %q", "test", provider) + } + if providerId := externalAuths[0].ProviderId(); providerId != "test_id" { + t.Fatalf("Expected %q providerId, got %q", "test_id", providerId) + } + if recordRef := externalAuths[0].RecordRef(); recordRef != user.Id { + t.Fatalf("Expected %q recordRef, got %q", user.Id, recordRef) + } + if collectionRef := externalAuths[0].CollectionRef(); collectionRef != user.Collection().Id { + t.Fatalf("Expected %q collectionRef, got %q", user.Collection().Id, collectionRef) + } }, }, { @@ -343,7 +382,7 @@ func TestRecordAuthWithOAuth2(t *testing.T) { } if !user.ValidatePassword("1234567890") { - t.Fatalf("Expected old password %q to be valid", "1234567890") + t.Fatalf("Expected old password %q to remain valid", "1234567890") } devices, err := app.FindAllAuthOriginsByRecord(user) @@ -353,7 +392,7 @@ func TestRecordAuthWithOAuth2(t *testing.T) { }, }, { - Name: "link by email", + Name: "link by email (unverified user)", Method: http.MethodPost, URL: "/api/collections/users/auth-with-oauth2", Body: strings.NewReader(`{ @@ -376,6 +415,20 @@ func TestRecordAuthWithOAuth2(t *testing.T) { t.Fatalf("Expected password %q to be valid", "1234567890") } + // ensure that there is at least one other external auth different than test + // so that later we can verify that it was deleted + var hasAtLeastOneOtherEA = false + externalAuths, _ := app.FindAllExternalAuthsByRecord(user) + for _, rel := range externalAuths { + if rel.Provider() != "test" { + hasAtLeastOneOtherEA = true + break + } + } + if !hasAtLeastOneOtherEA { + t.Fatal("Expected at least one non-test external auth linked") + } + // register the test provider auth.Providers["test"] = func() auth.Provider { return &oauth2MockProvider{ @@ -432,6 +485,13 @@ func TestRecordAuthWithOAuth2(t *testing.T) { "OnRecordUpdateExecute": 1, "OnRecordAfterUpdateSuccess": 1, // --- + "OnModelDelete": 2, // pre-existing external auths + "OnModelDeleteExecute": 2, + "OnModelAfterDeleteSuccess": 2, + "OnRecordDelete": 2, + "OnRecordDeleteExecute": 2, + "OnRecordAfterDeleteSuccess": 2, + // --- "OnModelValidate": 3, // record + authOrigins + externalAuths "OnRecordValidate": 3, }, @@ -449,6 +509,145 @@ func TestRecordAuthWithOAuth2(t *testing.T) { if len(devices) != 1 { t.Fatalf("Expected only 1 auth origin to be created, got %d (%v)", len(devices), err) } + + // ensure that other linked external auths have been deleted + externalAuths, _ := app.FindAllExternalAuthsByRecord(user) + if len(externalAuths) != 1 { + t.Fatalf("Expected only 1 external auth to remain, got %d", len(externalAuths)) + } + if provider := externalAuths[0].Provider(); provider != "test" { + t.Fatalf("Expected %q external auth, got %q", "test", provider) + } + if providerId := externalAuths[0].ProviderId(); providerId != "test_id" { + t.Fatalf("Expected %q providerId, got %q", "test_id", providerId) + } + if recordRef := externalAuths[0].RecordRef(); recordRef != user.Id { + t.Fatalf("Expected %q recordRef, got %q", user.Id, recordRef) + } + if collectionRef := externalAuths[0].CollectionRef(); collectionRef != user.Collection().Id { + t.Fatalf("Expected %q collectionRef, got %q", user.Collection().Id, collectionRef) + } + }, + }, + { + Name: "link by email (verified user)", + Method: http.MethodPost, + URL: "/api/collections/users/auth-with-oauth2", + Body: strings.NewReader(`{ + "provider": "test", + "code":"123", + "redirectURL": "https://example.com" + }`), + BeforeTestFunc: func(t testing.TB, app *tests.TestApp, e *core.ServeEvent) { + user, err := app.FindAuthRecordByEmail("users", "test3@example.com") + if err != nil { + t.Fatal(err) + } + + if !user.Verified() { + t.Fatalf("Expected user %q to be verified", user.Email()) + } + + // ensure that the old password works + if !user.ValidatePassword("1234567890") { + t.Fatalf("Expected password %q to be valid", "1234567890") + } + + // register the test provider + auth.Providers["test"] = func() auth.Provider { + return &oauth2MockProvider{ + AuthUser: &auth.AuthUser{Id: "test_id", Email: "test3@example.com"}, + Token: &oauth2.Token{AccessToken: "abc"}, + } + } + + // ensure that there is at least one other external auth different than test + // so that later we can verify that they are not deleted + var hasAtLeastOneOtherEA = false + externalAuths, _ := app.FindAllExternalAuthsByRecord(user) + for _, rel := range externalAuths { + if rel.Provider() != "test" { + hasAtLeastOneOtherEA = true + break + } + } + if !hasAtLeastOneOtherEA { + t.Fatal("Expected at least one non-test external auth linked") + } + + // add the test provider in the collection + user.Collection().MFA.Enabled = false + user.Collection().OAuth2.Enabled = true + user.Collection().OAuth2.Providers = []core.OAuth2ProviderConfig{{ + Name: "test", + ClientId: "123", + ClientSecret: "456", + }} + if err := app.Save(user.Collection()); err != nil { + t.Fatal(err) + } + }, + ExpectedStatus: 200, + ExpectedContent: []string{ + `"record":{`, + `"token":"`, + `"meta":{`, + `"isNew":false`, + `"email":"test3@example.com"`, + `"id":"bgs820n361vj1qd"`, + `"id":"test_id"`, + `"verified":true`, + }, + NotExpectedContent: []string{ + // hidden fields + `"tokenKey"`, + `"password"`, + }, + ExpectedEvents: map[string]int{ + "*": 0, + "OnRecordAuthWithOAuth2Request": 1, + "OnRecordAuthRequest": 1, + "OnRecordEnrich": 1, + // --- + "OnModelCreate": 2, // authOrigins + externalAuths + "OnModelCreateExecute": 2, + "OnModelAfterCreateSuccess": 2, + "OnRecordCreate": 2, + "OnRecordCreateExecute": 2, + "OnRecordAfterCreateSuccess": 2, + // --- + "OnModelValidate": 2, // authOrigins + externalAuths + "OnRecordValidate": 2, + }, + AfterTestFunc: func(t testing.TB, app *tests.TestApp, res *http.Response) { + user, err := app.FindAuthRecordByEmail("users", "test3@example.com") + if err != nil { + t.Fatal(err) + } + + if !user.ValidatePassword("1234567890") { + t.Fatalf("Expected old password %q to remain valid", "1234567890") + } + + devices, err := app.FindAllAuthOriginsByRecord(user) + if len(devices) != 1 { + t.Fatalf("Expected only 1 auth origin to be created, got %d (%v)", len(devices), err) + } + + var hasTestEA = false + externalAuths, err := app.FindAllExternalAuthsByRecord(user) + if len(externalAuths) <= 1 { + t.Fatalf("Expected to have 2+ ExternalAuth records, got %d (%v)", len(externalAuths), err) + } + for _, rel := range externalAuths { + if rel.Provider() == "test" { + hasTestEA = true + break + } + } + if !hasTestEA { + t.Fatal("Expected test external auth to be linked") + } }, }, { @@ -531,6 +730,13 @@ func TestRecordAuthWithOAuth2(t *testing.T) { "OnRecordCreateExecute": 2, "OnRecordAfterCreateSuccess": 2, // --- + "OnModelDelete": 2, // pre-existing external auths + "OnModelDeleteExecute": 2, + "OnModelAfterDeleteSuccess": 2, + "OnRecordDelete": 2, + "OnRecordDeleteExecute": 2, + "OnRecordAfterDeleteSuccess": 2, + // --- "OnModelValidate": 2, "OnRecordValidate": 2, }, @@ -541,7 +747,7 @@ func TestRecordAuthWithOAuth2(t *testing.T) { } if !user.ValidatePassword("1234567890") { - t.Fatalf("Expected password %q not to be changed", "1234567890") + t.Fatalf("Expected old password %q to remain valid", "1234567890") } devices, err := app.FindAllAuthOriginsByRecord(user) @@ -652,6 +858,13 @@ func TestRecordAuthWithOAuth2(t *testing.T) { "OnRecordUpdateExecute": 1, "OnRecordAfterUpdateSuccess": 1, // --- + "OnModelDelete": 2, // pre-existing external auths + "OnModelDeleteExecute": 2, + "OnModelAfterDeleteSuccess": 2, + "OnRecordDelete": 2, + "OnRecordDeleteExecute": 2, + "OnRecordAfterDeleteSuccess": 2, + // --- "OnModelValidate": 3, // record + authOrigins + externalAuths "OnRecordValidate": 3, }, @@ -662,7 +875,7 @@ func TestRecordAuthWithOAuth2(t *testing.T) { } if !user.ValidatePassword("1234567890") { - t.Fatalf("Expected password %q not to be changed", "1234567890") + t.Fatalf("Expected old password %q to remain valid", "1234567890") } devices, err := app.FindAllAuthOriginsByRecord(user) @@ -758,6 +971,13 @@ func TestRecordAuthWithOAuth2(t *testing.T) { "OnRecordUpdateExecute": 1, "OnRecordAfterUpdateSuccess": 1, // --- + "OnModelDelete": 2, // pre-existing external auths + "OnModelDeleteExecute": 2, + "OnModelAfterDeleteSuccess": 2, + "OnRecordDelete": 2, + "OnRecordDeleteExecute": 2, + "OnRecordAfterDeleteSuccess": 2, + // --- "OnModelValidate": 3, // record + authOrigins + externalAuths "OnRecordValidate": 3, }, @@ -768,7 +988,7 @@ func TestRecordAuthWithOAuth2(t *testing.T) { } if !user.ValidatePassword("1234567890") { - t.Fatalf("Expected password %q not to be changed", "1234567890") + t.Fatalf("Expected old password %q to remain valid", "1234567890") } devices, err := app.FindAllAuthOriginsByRecord(user) diff --git a/apis/record_auth_with_otp.go b/apis/record_auth_with_otp.go index b7dffa60..742c6bfc 100644 --- a/apis/record_auth_with_otp.go +++ b/apis/record_auth_with_otp.go @@ -71,8 +71,15 @@ func recordAuthWithOTP(e *core.RequestEvent) error { otpSentTo := e.OTP.SentTo() if !e.Record.Verified() && otpSentTo != "" && e.Record.Email() == otpSentTo { e.Record.SetVerified(true) - err = e.App.Save(e.Record) - if err != nil { + + // this is technically not required but we enforce password + // reset on verified upgrades in case the OTP is used on its own + // since this makes it less error prone to pre-hijacking attacks + if !e.Record.Collection().MFA.Enabled { + e.Record.SetRandomPassword() + } + + if err := e.App.Save(e.Record); err != nil { e.App.Logger().Error("Failed to update record verified state after successful OTP validation", "error", err, "otpId", e.OTP.Id, diff --git a/apis/record_auth_with_otp_test.go b/apis/record_auth_with_otp_test.go index bc514422..8dafbc27 100644 --- a/apis/record_auth_with_otp_test.go +++ b/apis/record_auth_with_otp_test.go @@ -327,6 +327,15 @@ func TestRecordAuthWithOTP(t *testing.T) { if user.Verified() { t.Fatal("Expected the user to remain unverified because sentTo != email") } + + // ensure that all pre-existing OAuth2 were NOT deleted + externalAuths, err := app.FindAllExternalAuthsByRecord(user) + if err != nil { + t.Fatal(err) + } + if len(externalAuths) != 2 { + t.Fatalf("Expected 2 external auths, found %d", len(externalAuths)) + } }, }, { @@ -364,6 +373,15 @@ func TestRecordAuthWithOTP(t *testing.T) { if err := app.Save(otp); err != nil { t.Fatal(err) } + + // verify that there are at least one pre-existing OAuth2 link + externalAuths, err := app.FindAllExternalAuthsByRecord(user) + if err != nil { + t.Fatal(err) + } + if len(externalAuths) == 0 { + t.Fatal("Expected at least one external auth") + } }, ExpectedStatus: 200, ExpectedContent: []string{ @@ -388,10 +406,10 @@ func TestRecordAuthWithOTP(t *testing.T) { "OnModelCreate": 1, "OnModelCreateExecute": 1, "OnModelAfterCreateSuccess": 1, - // OTP delete - "OnModelDelete": 1, - "OnModelDeleteExecute": 1, - "OnModelAfterDeleteSuccess": 1, + // OTP delete + 2 ExternalAuth delete + "OnModelDelete": 3, + "OnModelDeleteExecute": 3, + "OnModelAfterDeleteSuccess": 3, // user verified update "OnModelUpdate": 1, "OnModelUpdateExecute": 1, @@ -401,9 +419,9 @@ func TestRecordAuthWithOTP(t *testing.T) { "OnRecordCreate": 1, "OnRecordCreateExecute": 1, "OnRecordAfterCreateSuccess": 1, - "OnRecordDelete": 1, - "OnRecordDeleteExecute": 1, - "OnRecordAfterDeleteSuccess": 1, + "OnRecordDelete": 3, + "OnRecordDeleteExecute": 3, + "OnRecordAfterDeleteSuccess": 3, "OnRecordUpdate": 1, "OnRecordUpdateExecute": 1, "OnRecordAfterUpdateSuccess": 1, @@ -417,6 +435,15 @@ func TestRecordAuthWithOTP(t *testing.T) { if !user.Verified() { t.Fatal("Expected the user to be marked as verified") } + + // ensure that all pre-existing OAuth2 links are cleared + externalAuths, err := app.FindAllExternalAuthsByRecord(user) + if err != nil { + t.Fatal(err) + } + if len(externalAuths) > 0 { + t.Fatalf("Expected all external auths to be cleared, found %d", len(externalAuths)) + } }, }, { diff --git a/core/app.go b/core/app.go index 729f1175..089c4e07 100644 --- a/core/app.go +++ b/core/app.go @@ -502,6 +502,11 @@ type App interface { // ExternalAuth model that satisfies the non-nil expression. FindFirstExternalAuthByExpr(expr dbx.Expression) (*ExternalAuth, error) + // DeleteAllExternalAuthsByRecord deletes all ExternalAuth models associated with the provided record. + // + // Returns a combined error with the failed deletes. + DeleteAllExternalAuthsByRecord(authRecord *Record) error + // --------------------------------------------------------------- // FindAllMFAsByRecord returns all MFA models linked to the provided auth record. diff --git a/core/external_auth_query.go b/core/external_auth_query.go index 9331cfde..b5cb4cec 100644 --- a/core/external_auth_query.go +++ b/core/external_auth_query.go @@ -1,6 +1,8 @@ package core import ( + "errors" + "github.com/pocketbase/dbx" ) @@ -59,3 +61,25 @@ func (app *BaseApp) FindFirstExternalAuthByExpr(expr dbx.Expression) (*ExternalA return model, nil } + +// DeleteAllExternalAuthsByRecord deletes all ExternalAuth models associated with the provided record. +// +// Returns a combined error with the failed deletes. +func (app *BaseApp) DeleteAllExternalAuthsByRecord(authRecord *Record) error { + models, err := app.FindAllExternalAuthsByRecord(authRecord) + if err != nil { + return err + } + + var errs []error + for _, m := range models { + if err := app.Delete(m); err != nil { + errs = append(errs, err) + } + } + if len(errs) > 0 { + return errors.Join(errs...) + } + + return nil +} diff --git a/core/external_auth_query_test.go b/core/external_auth_query_test.go index 868ba665..ccc97cb4 100644 --- a/core/external_auth_query_test.go +++ b/core/external_auth_query_test.go @@ -2,6 +2,7 @@ package core_test import ( "fmt" + "slices" "testing" "github.com/pocketbase/dbx" @@ -174,3 +175,68 @@ func TestFindFirstExternalAuthByExpr(t *testing.T) { }) } } + +func TestDeleteAllExternalAuthsByRecord(t *testing.T) { + t.Parallel() + + testApp, _ := tests.NewTestApp() + defer testApp.Cleanup() + + demo1, err := testApp.FindRecordById("demo1", "84nmscqy84lsi1t") + if err != nil { + t.Fatal(err) + } + + user1, err := testApp.FindAuthRecordByEmail("users", "test@example.com") + if err != nil { + t.Fatal(err) + } + + client1, err := testApp.FindAuthRecordByEmail("clients", "test@example.com") + if err != nil { + t.Fatal(err) + } + + client2, err := testApp.FindAuthRecordByEmail("clients", "test2@example.com") + if err != nil { + t.Fatal(err) + } + + scenarios := []struct { + record *core.Record + deletedIds []string + }{ + {demo1, nil}, // non-auth record + {user1, []string{"dlmflokuq1xl342", "clmflokuq1xl341"}}, + {client1, []string{"f1z5b3843pzc964"}}, + {client2, nil}, + } + + for i, s := range scenarios { + t.Run(fmt.Sprintf("%d_%s_%s", i, s.record.Collection().Name, s.record.Id), func(t *testing.T) { + app, _ := tests.NewTestApp() + defer app.Cleanup() + + deletedIds := []string{} + app.OnRecordDelete().BindFunc(func(e *core.RecordEvent) error { + deletedIds = append(deletedIds, e.Record.Id) + return e.Next() + }) + + err := app.DeleteAllExternalAuthsByRecord(s.record) + if err != nil { + t.Fatal(err) + } + + if len(deletedIds) != len(s.deletedIds) { + t.Fatalf("Expected deleted ids\n%v\ngot\n%v", s.deletedIds, deletedIds) + } + + for _, id := range s.deletedIds { + if !slices.Contains(deletedIds, id) { + t.Errorf("Expected to find deleted id %q in %v", id, deletedIds) + } + } + }) + } +} diff --git a/core/record_model.go b/core/record_model.go index 0b06a210..443d06ef 100644 --- a/core/record_model.go +++ b/core/record_model.go @@ -1427,22 +1427,31 @@ func onRecordValidate(e *RecordEvent) error { } func onRecordSaveExecute(e *RecordEvent) error { + var needToDeleteExternalAuths bool + if e.Record.Collection().IsAuth() { - // ensure that the token key is regenerated on password change or email change + // auth resets to prevent (pre)hijacking vulnerabilities if !e.Record.IsNew() { lastSavedRecord, err := e.App.FindRecordById(e.Record.Collection(), e.Record.Id) if err != nil { return err } + // ensure that the token key is regenerated on password change or email change if lastSavedRecord.TokenKey() == e.Record.TokenKey() && (lastSavedRecord.Get(FieldNamePassword) != e.Record.Get(FieldNamePassword) || lastSavedRecord.Email() != e.Record.Email()) { e.Record.RefreshTokenKey() } + + // in case upgrading from "unverified" -> "verified" mark all pre-existing OAuth2 links + // for deletion since there is no reliable way to verify that they weren't created by an attacker + if !lastSavedRecord.Verified() && e.Record.Verified() { + needToDeleteExternalAuths = true + } } - // cross-check that the auth record id is unique across all auth collections. + // cross-check that the auth record id is unique across all auth collections authCollections, err := e.App.FindAllCollections(CollectionTypeAuth) if err != nil { return fmt.Errorf("unable to fetch the auth collections for cross-id unique check: %w", err) @@ -1460,16 +1469,45 @@ func onRecordSaveExecute(e *RecordEvent) error { } } - err := e.Next() - if err == nil { - return nil + finalizer := func() error { + err := e.Next() + if err == nil { + return nil + } + + return validators.NormalizeUniqueIndexError( + err, + e.Record.Collection().Name, + e.Record.Collection().Fields.FieldNames(), + ) } - return validators.NormalizeUniqueIndexError( - err, - e.Record.Collection().Name, - e.Record.Collection().Fields.FieldNames(), - ) + if needToDeleteExternalAuths { + originalApp := e.App + + return e.App.RunInTransaction(func(txApp App) error { + e.App = txApp + defer func() { e.App = originalApp }() + + externalAuths, err := txApp.FindAllExternalAuthsByRecord(e.Record) + if err != nil { + return err + } + if len(externalAuths) > 0 { + // delete all pre-existing external auths + if err := txApp.DeleteAllExternalAuthsByRecord(e.Record); err != nil { + return err + } + + // force refresh tokens reset (if not already) + e.Record.RefreshTokenKey() + } + + return finalizer() + }) + } + + return finalizer() } func onRecordDeleteExecute(e *RecordEvent) error {