Skip to content

revert: Update user password on each valid signup attempt #2912

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
cstockton merged 1 commit into from
Nov 6, 2026
Merged

revert: Update user password on each valid signup attempt #2912

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 11 additions & 52 deletions internal/api/signup.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,72 +175,32 @@ func (a *API) Signup(w http.ResponseWriter, r *http.Request) error {
return apierrors.NewInternalServerError("Database error finding user").WithInternalError(err)
}

if user != nil {
if (params.Provider == EmailProvider && user.IsConfirmed()) || (params.Provider == PhoneProvider && user.IsPhoneConfirmed()) {
err = db.Transaction(func(tx *storage.Connection) error {
if terr := models.NewAuditLogEntry(config.AuditLog, r, tx, user, models.UserRepeatedSignUpAction, "", map[string]any{
"provider": params.Provider,
}); terr != nil {
return terr
}
return nil
})
if err != nil {
return err
}
if config.Mailer.Autoconfirm || config.Sms.Autoconfirm {
return apierrors.NewUnprocessableEntityError(apierrors.ErrorCodeUserAlreadyExists, "User already registered")
}
sanitizedUser, err := sanitizeUser(user, params)
if err != nil {
return err
}
return sendJSON(w, http.StatusOK, sanitizedUser)
var signupUser *models.User
if user == nil {
// always call this outside of a database transaction as this method
// can be computationally hard and block due to password hashing
signupUser, err = params.ToUserModel(false /* <- isSSOUser */)
if err != nil {
return err
}
if err := a.triggerBeforeUserCreated(r, db, signupUser); err != nil {
return err
}
}

// always call this outside of a database transaction as this method
// can be computationally hard and block due to password hashing
signupUser, err := params.ToUserModel(false /* <- isSSOUser */)
if err != nil {
return err
}
if err := a.triggerBeforeUserCreated(r, db, signupUser); err != nil {
return err
}

err = db.Transaction(func(tx *storage.Connection) error {
var terr error

if user != nil {
tuser, terr := models.FindUserByIDForUpdate(tx, user.ID)
if terr == nil {
user = tuser
} else if !models.IsNotFoundError(terr) {
return terr
}
}

if user != nil {
if (params.Provider == EmailProvider && user.IsConfirmed()) || (params.Provider == PhoneProvider && user.IsPhoneConfirmed()) {
return UserExistsError
}

// Check user hasn't finished registration.
if !user.IsConfirmed() && !user.IsPhoneConfirmed() {
user.EncryptedPassword = signupUser.EncryptedPassword
terr = user.UpdatePassword(tx, nil)
if terr != nil {
return terr
}
}
// do not update the user because we can't be sure of their claimed identity
} else {
user, terr = a.signupNewUser(tx, signupUser)
if terr != nil {
return terr
}
}

identity, terr := models.FindIdentityByIdAndProvider(tx, user.ID.String(), params.Provider)
if terr != nil {
if !models.IsNotFoundError(terr) {
Expand Down Expand Up @@ -397,7 +357,6 @@ func sanitizeUser(u *models.User, params *SignupParams) (*models.User, error) {
u.Identities = make([]models.Identity, 0)
u.UserMetaData = params.Data
u.Aud = params.Aud
u.IsSuperAdmin = false

// sanitize app_metadata
u.AppMetaData = map[string]interface{}{
Expand Down
99 changes: 0 additions & 99 deletions internal/api/signup_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ import (
"testing"
"time"

"github.com/supabase/auth/internal/crypto"
mail "github.com/supabase/auth/internal/mailer"

"github.com/stretchr/testify/assert"
Expand Down Expand Up @@ -170,101 +169,3 @@ func (ts *SignupTestSuite) TestSignupRequestBodyTooLarge() {
require.NoError(ts.T(), json.NewDecoder(w.Body).Decode(&data))
require.Equal(ts.T(), "request_entity_too_large", data["error_code"])
}

// Check that each new signup attempt updates the unregistered user's password.
func (ts *SignupTestSuite) TestConsecutiveSignupAttempts() {
var attempt1Buffer bytes.Buffer
require.NoError(ts.T(), json.NewEncoder(&attempt1Buffer).Encode(map[string]interface{}{
"email": "test@example.com",
"password": "password1",
}))

// Setup request
req := httptest.NewRequest(http.MethodPost, "/signup", &attempt1Buffer)
req.Header.Set("Content-Type", "application/json")

// Setup response recorder
w := httptest.NewRecorder()
ts.API.handler.ServeHTTP(w, req)

require.Equal(ts.T(), http.StatusOK, w.Code)

var attempt2Buffer bytes.Buffer
require.NoError(ts.T(), json.NewEncoder(&attempt2Buffer).Encode(map[string]interface{}{
"email": "test@example.com",
"password": "password2",
}))

// Setup request
req = httptest.NewRequest(http.MethodPost, "/signup", &attempt2Buffer)
req.Header.Set("Content-Type", "application/json")

w = httptest.NewRecorder()
ts.API.handler.ServeHTTP(w, req)
require.Equal(ts.T(), http.StatusOK, w.Code)

user, err := models.FindUserByEmailAndAudience(ts.API.db, "test@example.com", ts.Config.JWT.Aud)
require.NoError(ts.T(), err)

err = crypto.CompareHashAndPassword(ts.T().Context(), *user.EncryptedPassword, "password1")
require.Error(ts.T(), err)

err = crypto.CompareHashAndPassword(ts.T().Context(), *user.EncryptedPassword, "password2")
require.NoError(ts.T(), err)
}

// Check an email signup attempt doesn't overwrite password of an email-confirmed user.
func (ts *SignupTestSuite) TestEmailSignupForEmailConfirmedUser() {
user, err := models.NewUser("", "test@example.com", "password1", ts.Config.JWT.Aud, nil)
require.NoError(ts.T(), err)
require.NoError(ts.T(), ts.API.db.Create(user))
require.NoError(ts.T(), user.Confirm(ts.API.db))

var buffer bytes.Buffer
require.NoError(ts.T(), json.NewEncoder(&buffer).Encode(map[string]interface{}{
"email": "test@example.com",
"password": "password2",
}))

req := httptest.NewRequest(http.MethodPost, "/signup", &buffer)
req.Header.Set("Content-Type", "application/json")

w := httptest.NewRecorder()
ts.API.handler.ServeHTTP(w, req)

require.Equal(ts.T(), http.StatusOK, w.Code)

user, err = models.FindUserByEmailAndAudience(ts.API.db, "test@example.com", ts.Config.JWT.Aud)
require.NoError(ts.T(), err)

err = crypto.CompareHashAndPassword(ts.T().Context(), *user.EncryptedPassword, "password1")
require.NoError(ts.T(), err)
}

// Check an email signup attempt doesn't overwrite password of an phone-confirmed user.
func (ts *SignupTestSuite) TestEmailSignupForPhoneConfirmedUser() {
user, err := models.NewUser("123456789", "test@example.com", "password1", ts.Config.JWT.Aud, nil)
require.NoError(ts.T(), err)
require.NoError(ts.T(), ts.API.db.Create(user))
require.NoError(ts.T(), user.ConfirmPhone(ts.API.db))

var buffer bytes.Buffer
require.NoError(ts.T(), json.NewEncoder(&buffer).Encode(map[string]interface{}{
"email": "test@example.com",
"password": "password2",
}))

req := httptest.NewRequest(http.MethodPost, "/signup", &buffer)
req.Header.Set("Content-Type", "application/json")

w := httptest.NewRecorder()
ts.API.handler.ServeHTTP(w, req)

require.Equal(ts.T(), http.StatusOK, w.Code)

user, err = models.FindUserByEmailAndAudience(ts.API.db, "test@example.com", ts.Config.JWT.Aud)
require.NoError(ts.T(), err)

err = crypto.CompareHashAndPassword(ts.T().Context(), *user.EncryptedPassword, "password1")
require.NoError(ts.T(), err)
}
17 changes: 1 addition & 16 deletions internal/models/user.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,8 +73,6 @@ type User struct {
DeletedAt *time.Time `json:"deleted_at,omitempty" db:"deleted_at"`
IsAnonymous bool `json:"is_anonymous" db:"is_anonymous"`

IsSuperAdmin bool `json:"-" db:"is_super_admin"`

DONTUSEINSTANCEID uuid.UUID `json:"-" db:"instance_id"`
}

Expand Down Expand Up @@ -188,7 +186,7 @@ func (u *User) BeforeSave(tx *pop.Connection) error {
return nil
}

// IsConfirmed checks if a user's email has already been
// IsConfirmed checks if a user has already been
// registered and confirmed.
func (u *User) IsConfirmed() bool {
return u.EmailConfirmedAt != nil
Expand Down Expand Up @@ -774,19 +772,6 @@ func FindUserByID(tx *storage.Connection, id uuid.UUID) (*User, error) {
return findUser(tx, "instance_id = ? and id = ?", uuid.Nil, id)
}

func FindUserByIDForUpdate(tx *storage.Connection, id uuid.UUID) (*User, error) {
obj := &User{}
query := fmt.Sprintf(`SELECT * FROM %q WHERE id = ? FOR UPDATE`, obj.TableName())
if err := tx.RawQuery(query, id).First(obj); err != nil {
if errors.Cause(err) == sql.ErrNoRows {
return nil, UserNotFoundError{}
}
return nil, errors.Wrap(err, "error finding user")
}

return obj, nil
}

// FindUserWithRefreshToken finds a user from the provided refresh token. If
// forUpdate is set to true, then the SELECT statement used by the query has
// the form SELECT ... FOR UPDATE SKIP LOCKED. This means that a FOR UPDATE
Expand Down
Loading