Repository: supabase/gotrue
Pull request #509 (2022-06-23 12:58:02)

fix(oauth): Remove password, phone on subsequent OAuth login
## What kind of change does this PR introduce?

An unconfirmed user account using email + password login, that is then connected to an OAuth account presents a security risk since the user logging in with OAuth does not expect for there to be a (potentially malicious) email + password combination (or unconfirmed phone number). Therefore, the password / phone is removed, the email and/or phone providers removed from the user.

## What is the current behavior?

Email + password combination can be used to log-in after linking with an OAuth account occurs.




Patch:
diff --git a/api/external.go b/api/external.go
index 32e3c69b..d8516e8a 100644
--- a/api/external.go
+++ b/api/external.go
@@ -231,6 +231,16 @@ func (a *API) internalExternalProviderCallback(w http.ResponseWriter, r *http.Re
 				return unauthorizedError("User is unauthorized")
 			}
 
+			// an account with a previously unconfirmed email + password
+			// combination or phone may exist. so now that there is an
+			// OAuth identity bound to this user, and since they have not
+			// confirmed their email or phone, they are unaware that a
+			// potentially malicious door exists into their account; thus
+			// the password and phone needs to be removed.
+			if terr = user.RemoveUnconfirmedIdentities(tx); terr != nil {
+				return internalServerError("Error updating user").WithInternalError(terr)
+			}
+
 			if !user.IsConfirmed() {
 				if !emailData.Verified && !config.Mailer.Autoconfirm {
 					mailer := a.Mailer(ctx)
diff --git a/api/recover.go b/api/recover.go
index 17cc3676..b2679b13 100644
--- a/api/recover.go
+++ b/api/recover.go
@@ -32,7 +32,7 @@ func (a *API) Recover(w http.ResponseWriter, r *http.Request) error {
 
 	var user *models.User
 	aud := a.requestAud(ctx, r)
-	recoverErrorMessage := "If a user exists, you will receive an email with instructions on how to reset your password."
+
 	if err := a.validateEmail(ctx, params.Email); err != nil {
 		return err
 	}
@@ -40,9 +40,9 @@ func (a *API) Recover(w http.ResponseWriter, r *http.Request) error {
 
 	if err != nil {
 		if models.IsNotFoundError(err) {
-			return notFoundError(err.Error())
+			return sendJSON(w, http.StatusOK, map[string]string{})
 		}
-		return internalServerError(recoverErrorMessage).WithInternalError(err)
+		return internalServerError("Unable to process request").WithInternalError(err)
 	}
 
 	err = a.db.Transaction(func(tx *storage.Connection) error {
@@ -57,8 +57,8 @@ func (a *API) Recover(w http.ResponseWriter, r *http.Request) error {
 		if errors.Is(err, MaxFrequencyLimitError) {
 			return tooManyRequestsError("For security purposes, you can only request this once every 60 seconds")
 		}
-		return internalServerError(recoverErrorMessage).WithInternalError(err)
+		return internalServerError("Unable to process request").WithInternalError(err)
 	}
 
-	return sendJSON(w, http.StatusOK, &map[string]string{})
+	return sendJSON(w, http.StatusOK, map[string]string{})
 }
diff --git a/api/token.go b/api/token.go
index 581b3834..53259638 100644
--- a/api/token.go
+++ b/api/token.go
@@ -479,6 +479,7 @@ func (a *API) IdTokenGrant(ctx context.Context, w http.ResponseWriter, r *http.R
 				isEmailVerified = getEmailVerified(emailVerified)
 			}
 			if (!ok || !isEmailVerified) && !config.Mailer.Autoconfirm {
+
 				mailer := a.Mailer(ctx)
 				referrer := a.getReferrer(r)
 				if terr = sendConfirmation(tx, user, mailer, config.SMTP.MaxFrequency, referrer, config.Mailer.OtpLength); terr != nil {
diff --git a/models/user.go b/models/user.go
index 46bba3c4..9d7efdef 100644
--- a/models/user.go
+++ b/models/user.go
@@ -520,5 +520,60 @@ func (u *User) IsBanned() bool {
 
 func (u *User) UpdateBannedUntil(tx *storage.Connection) error {
 	return tx.UpdateOnly(u, "banned_until")
+}
+
+// RemoveUnconfirmedIdentities removes potentially malicious unconfirmed identities from a user (if any)
+func (u *User) RemoveUnconfirmedIdentities(tx *storage.Connection) error {
+	if u.IsConfirmed() {
+		return nil
+	}
+
+	u.EncryptedPassword = ""
+
+	if terr := tx.UpdateOnly(u, "encrypted_password"); terr != nil {
+		return terr
+	}
+
+	if providersList, ok := u.AppMetaData["providers"].([]string); ok {
+		// user has "providers" metadata, and the "email" provider
+		// should be removed from it
+
+		var confirmedProviders []string
+
+		for _, provider := range providersList {
+			if provider != "email" {
+				confirmedProviders = append(confirmedProviders, provider)
+			}
+		}
+
+		u.AppMetaData["providers"] = confirmedProviders
 
+		if len(confirmedProviders) > 0 {
+			u.AppMetaData["provider"] = confirmedProviders[0]
+		} else {
+			u.AppMetaData["provider"] = nil
+		}
+
+		if terr := u.UpdateAppMetaData(tx, u.AppMetaData); terr != nil {
+			return terr
+		}
+	}
+
+	// finally, remove any identity with the "email" provider
+
+	var confirmedProviders []Identity
+
+	for _, identity := range u.Identities {
+		if identity.Provider == "email" {
+			if terr := tx.Destroy(&identity); terr != nil {
+				return terr
+			}
+		} else {
+			confirmedProviders = append(confirmedProviders, identity)
+		}
+	}
+
+	u.Identities = confirmedProviders
+
+	return nil
 }


Test patch:
diff --git a/api/recover_test.go b/api/recover_test.go
index 9838e3f2..26e674f1 100644
--- a/api/recover_test.go
+++ b/api/recover_test.go
@@ -133,3 +133,25 @@ func (ts *RecoverTestSuite) TestRecover_NewEmailSent() {
 	// ensure it sent a new email
 	assert.WithinDuration(ts.T(), time.Now(), *u.RecoverySentAt, 1*time.Second)
 }
+
+func (ts *RecoverTestSuite) TestRecover_NoSideChannelLeak() {
+	email := "doesntexist@example.com"
+
+	_, err := models.FindUserByEmailAndAudience(ts.API.db, ts.instanceID, email, ts.Config.JWT.Aud)
+	require.True(ts.T(), models.IsNotFoundError(err), "User with email %s does exist", email)
+
+	// Request body
+	var buffer bytes.Buffer
+	require.NoError(ts.T(), json.NewEncoder(&buffer).Encode(map[string]interface{}{
+		"email": email,
+	}))
+
+	// Setup request
+	req := httptest.NewRequest(http.MethodPost, "http://localhost/recover", &buffer)
+	req.Header.Set("Content-Type", "application/json")
+
+	// Setup response recorder
+	w := httptest.NewRecorder()
+	ts.API.handler.ServeHTTP(w, req)
+	assert.Equal(ts.T(), http.StatusOK, w.Code)
+}
diff --git a/models/user_test.go b/models/user_test.go
index c15b40f7..f87d55fe 100644
--- a/models/user_test.go
+++ b/models/user_test.go
@@ -194,3 +194,59 @@ func (ts *UserTestSuite) createUserWithEmail(email string) *User {
 
 	return user
 }
+
+func (ts *UserTestSuite) TestRemoveUnconfirmedIdentities() {
+	uid, err := uuid.NewV4()
+	require.NoError(ts.T(), err)
+
+	user, err := NewUser(uid, "+29382983298", "someone@example.com", "abcdefgh", "test", nil)
+	require.NoError(ts.T(), err)
+
+	user.AppMetaData = map[string]interface{}{
+		"provider":  "email",
+		"providers": []string{"email", "phone", "twitter"},
+	}
+
+	require.NoError(ts.T(), ts.db.Create(user))
+
+	idEmail, err := NewIdentity(user, "email", map[string]interface{}{
+		"sub": "someone@example.com",
+	})
+	require.NoError(ts.T(), err)
+	require.NoError(ts.T(), ts.db.Create(idEmail))
+
+	user.Identities = append(user.Identities, *idEmail)
+
+	idPhone, err := NewIdentity(user, "phone", map[string]interface{}{
+		"sub": "+29382983298",
+	})
+	require.NoError(ts.T(), err)
+	require.NoError(ts.T(), ts.db.Create(idPhone))
+
+	user.Identities = append(user.Identities, *idPhone)
+
+	idTwitter, err := NewIdentity(user, "twitter", map[string]interface{}{
+		"sub": "test_twitter_user_id",
+	})
+	require.NoError(ts.T(), err)
+	require.NoError(ts.T(), ts.db.Create(idTwitter))
+
+	user.Identities = append(user.Identities, *idTwitter)
+
+	// reload the user
+	require.NoError(ts.T(), ts.db.Load(user))
+
+	require.False(ts.T(), user.IsConfirmed(), "user's email must not be confirmed")
+
+	require.NoError(ts.T(), user.RemoveUnconfirmedIdentities(ts.db))
+
+	require.Empty(ts.T(), user.EncryptedPassword, "password still remains in user")
+
+	require.Len(ts.T(), user.Identities, 2, "only two identity must be remaining")
+	require.Equal(ts.T(), idPhone.ID, user.Identities[0].ID, "remaining identity is not the expected one")
+	require.Equal(ts.T(), idTwitter.ID, user.Identities[1].ID, "remaining identity is not the expected one")
+
+	require.NotNil(ts.T(), user.AppMetaData)
+	require.Equal(ts.T(), user.AppMetaData["provider"], "phone")
+	require.Equal(ts.T(), user.AppMetaData["providers"], []string{"phone", "twitter"})
+}