From 056a0a99bfc9535ed4d6d5374c238c2721ca4a66 Mon Sep 17 00:00:00 2001 From: Gani Georgiev Date: Sun, 26 Apr 2026 15:19:43 +0300 Subject: [PATCH] (backported from v0.37.4) deleted all previous external auths on verified upgrade --- CHANGELOG.md | 5 +++++ apis/record_auth_test.go | 38 +++++++++++++++++++++++++++++------ daos/record.go | 39 +++++++++++++++++++++++++++++++++++- forms/record_oauth2_login.go | 21 +++++++++++++++++++ models/record_test.go | 2 +- 5 files changed, 97 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 724ffd8e..466af472 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,8 @@ +## v0.22.42 + +- (_Backported from v0.37.4_) ⚠️ Fixed a pre-hijacking OAuth2 linking vulnerability ([#7662](https://github.com/pocketbase/pocketbase/discussions/7662); thanks @Alardiians for reporting it privately). + + ## v0.22.41 - (_Backported from v0.36.9_) Updated the Discord `AuthUser.Name` field to use `global_name`. diff --git a/apis/record_auth_test.go b/apis/record_auth_test.go index 112f0f86..dbfe846c 100644 --- a/apis/record_auth_test.go +++ b/apis/record_auth_test.go @@ -663,10 +663,13 @@ func TestRecordAuthConfirmPasswordReset(t *testing.T) { }`), ExpectedStatus: 204, ExpectedEvents: map[string]int{ - "OnModelAfterUpdate": 1, "OnModelBeforeUpdate": 1, + "OnModelAfterUpdate": 1, "OnRecordBeforeConfirmPasswordResetRequest": 1, "OnRecordAfterConfirmPasswordResetRequest": 1, + // external auths deletion + "OnModelBeforeDelete": 2, + "OnModelAfterDelete": 2, }, BeforeTestFunc: func(t *testing.T, app *tests.TestApp, e *echo.Echo) { user, err := app.Dao().FindAuthRecordByEmail("users", "test@example.com") @@ -770,10 +773,18 @@ func TestRecordAuthConfirmPasswordReset(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.Dao().WithoutHooks().SaveRecord(user); err != nil { - t.Fatalf("Failed to update user verified state") + t.Fatal("Failed to update user verified state") + } + + // restore original token key that was reseted after the verified change + user.SetTokenKey(oldTokenKey) + if err := app.Dao().WithoutHooks().SaveRecord(user); err != nil { + t.Fatal("Failed to reset user tokenKey") } }, AfterTestFunc: func(t *testing.T, app *tests.TestApp, res *http.Response) { @@ -812,10 +823,13 @@ func TestRecordAuthConfirmPasswordReset(t *testing.T) { ExpectedStatus: 400, ExpectedContent: []string{`"data":{}`}, ExpectedEvents: map[string]int{ - "OnModelAfterUpdate": 1, "OnModelBeforeUpdate": 1, + "OnModelAfterUpdate": 1, "OnRecordBeforeConfirmPasswordResetRequest": 1, "OnRecordAfterConfirmPasswordResetRequest": 1, + // external auths deletion + "OnModelBeforeDelete": 2, + "OnModelAfterDelete": 2, }, }, } @@ -987,10 +1001,13 @@ func TestRecordAuthConfirmVerification(t *testing.T) { }`), ExpectedStatus: 204, ExpectedEvents: map[string]int{ - "OnModelAfterUpdate": 1, "OnModelBeforeUpdate": 1, + "OnModelAfterUpdate": 1, "OnRecordBeforeConfirmVerificationRequest": 1, "OnRecordAfterConfirmVerificationRequest": 1, + // external auths deletion + "OnModelBeforeDelete": 2, + "OnModelAfterDelete": 2, }, }, { @@ -1037,10 +1054,13 @@ func TestRecordAuthConfirmVerification(t *testing.T) { ExpectedStatus: 400, ExpectedContent: []string{`"data":{}`}, ExpectedEvents: map[string]int{ - "OnModelAfterUpdate": 1, "OnModelBeforeUpdate": 1, + "OnModelAfterUpdate": 1, "OnRecordBeforeConfirmVerificationRequest": 1, "OnRecordAfterConfirmVerificationRequest": 1, + // external auths deletion + "OnModelBeforeDelete": 2, + "OnModelAfterDelete": 2, }, }, } @@ -1227,10 +1247,13 @@ func TestRecordAuthConfirmEmailChange(t *testing.T) { }`), ExpectedStatus: 204, ExpectedEvents: map[string]int{ - "OnModelAfterUpdate": 1, "OnModelBeforeUpdate": 1, + "OnModelAfterUpdate": 1, "OnRecordBeforeConfirmEmailChangeRequest": 1, "OnRecordAfterConfirmEmailChangeRequest": 1, + // external auths deletion + "OnModelBeforeDelete": 2, + "OnModelAfterDelete": 2, }, }, { @@ -1267,6 +1290,9 @@ func TestRecordAuthConfirmEmailChange(t *testing.T) { "OnModelBeforeUpdate": 1, "OnRecordBeforeConfirmEmailChangeRequest": 1, "OnRecordAfterConfirmEmailChangeRequest": 1, + // external auths deletion + "OnModelBeforeDelete": 2, + "OnModelAfterDelete": 2, }, }, } diff --git a/daos/record.go b/daos/record.go index 9e2f532a..af347e07 100644 --- a/daos/record.go +++ b/daos/record.go @@ -584,6 +584,8 @@ func (dao *Dao) CanAccessRecord(record *models.Record, requestInfo *models.Reque // If record.IsNew() is true, the method will perform a create, otherwise an update. // To explicitly mark a record for update you can use record.MarkAsNotNew(). func (dao *Dao) SaveRecord(record *models.Record) error { + var needToDeleteExternalAuths bool + if record.Collection().IsAuth() { if record.Username() == "" { return errors.New("unable to save auth record without username") @@ -604,9 +606,44 @@ func (dao *Dao) SaveRecord(record *models.Record) error { return errors.New("the auth record ID must be unique across all auth collections") } } + + if !record.IsNew() && record.Verified() { + lastSavedRecord, err := dao.FindRecordById(record.Collection().Id, record.Id) + if err != nil { + return err + } + + // 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() && record.Verified() { + needToDeleteExternalAuths = true + } + } } - return dao.Save(record) + if !needToDeleteExternalAuths { + return dao.Save(record) + } + + return dao.RunInTransaction(func(txDao *Dao) error { + externalAuths, err := txDao.FindAllExternalAuthsByRecord(record) + if err != nil { + return err + } + if len(externalAuths) > 0 { + // delete all pre-existing external auths + for _, ea := range externalAuths { + if err := txDao.DeleteExternalAuth(ea); err != nil { + return err + } + } + + // force refresh tokens reset (if not already) + record.RefreshTokenKey() + } + + return txDao.Save(record) + }) } // DeleteRecord deletes the provided Record model. diff --git a/forms/record_oauth2_login.go b/forms/record_oauth2_login.go index 1e31bb94..05b10437 100644 --- a/forms/record_oauth2_login.go +++ b/forms/record_oauth2_login.go @@ -260,6 +260,27 @@ func (form *RecordOAuth2Login) submit(data *RecordOAuth2LoginData) error { } } + // 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 !data.Record.Verified() { + externalAuths, err := txDao.FindAllExternalAuthsByRecord(data.Record) + if err != nil { + return err + } + for _, ea := range externalAuths { + if err := txDao.DeleteExternalAuth(ea); err != nil { + return err + } + } + data.ExternalAuth = 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) diff --git a/models/record_test.go b/models/record_test.go index b43f9203..7383a698 100644 --- a/models/record_test.go +++ b/models/record_test.go @@ -909,7 +909,7 @@ func TestRecordGetInt(t *testing.T) { {123, 123}, {2.4, 2}, {"123", 123}, - {"123.5", 0}, + {"123.5", 123}, {false, 0}, {true, 1}, }