(backported from v0.37.4) deleted all previous external auths on verified upgrade

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`.

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,
 			},
 		},
 	}

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.

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)

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},
 	}