Skip to content

Commit 397c949

Browse files
kangmingtaykangmingtay
authored
fix: shorten email otp length (#513)
* fix: add migrations to hash email * add email otp length to config * remove email hash migration * send email hash & otp in email link * verify post should check for token hash * fix verify tests * fix tests * update generate_link endpoint * remove magic number * use sum224 instead of md5
1 parent bc44b28 commit 397c949

19 files changed

+269
-195
lines changed

‎api/external.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -235,7 +235,7 @@ func (a *API) internalExternalProviderCallback(w http.ResponseWriter, r *http.Re
235235
if !emailData.Verified && !config.Mailer.Autoconfirm {
236236
mailer := a.Mailer(ctx)
237237
referrer := a.getReferrer(r)
238-
if terr = sendConfirmation(tx, user, mailer, config.SMTP.MaxFrequency, referrer); terr != nil {
238+
if terr = sendConfirmation(tx, user, mailer, config.SMTP.MaxFrequency, referrer, config.Mailer.OtpLength); terr != nil {
239239
if errors.Is(terr, MaxFrequencyLimitError) {
240240
return tooManyRequestsError("For security purposes, you can only request this once every minute")
241241
}

‎api/invite.go‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ type InviteParams struct {
1717
// Invite is the endpoint for inviting a new user
1818
func (a *API) Invite(w http.ResponseWriter, r *http.Request) error {
1919
ctx := r.Context()
20+
config := a.getConfig(ctx)
2021
instanceID := getInstanceID(ctx)
2122
adminUser := getAdminUser(ctx)
2223
params := &InviteParams{}
@@ -64,7 +65,7 @@ func (a *API) Invite(w http.ResponseWriter, r *http.Request) error {
6465

6566
mailer := a.Mailer(ctx)
6667
referrer := a.getReferrer(r)
67-
if err := sendInvite(tx, user, mailer, referrer); err != nil {
68+
if err := sendInvite(tx, user, mailer, referrer, config.Mailer.OtpLength); err != nil {
6869
return internalServerError("Error inviting user").WithInternalError(err)
6970
}
7071
return nil

‎api/invite_test.go‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ package api
22

33
import (
44
"bytes"
5+
"crypto/sha256"
56
"encoding/json"
67
"fmt"
78
"net/http"
@@ -126,6 +127,7 @@ func (ts *InviteTestSuite) TestVerifyInvite() {
126127
"Verify invite with password",
127128
"[email protected]",
128129
map[string]interface{}{
130+
"email": "[email protected]",
129131
"type": "invite",
130132
"token": "asdf",
131133
"password": "testing",
@@ -136,6 +138,7 @@ func (ts *InviteTestSuite) TestVerifyInvite() {
136138
"Verify invite with no password",
137139
"[email protected]",
138140
map[string]interface{}{
141+
"email": "[email protected]",
139142
"type": "invite",
140143
"token": "asdf",
141144
},
@@ -150,7 +153,7 @@ func (ts *InviteTestSuite) TestVerifyInvite() {
150153
user.InvitedAt = &now
151154
user.ConfirmationSentAt = &now
152155
user.EncryptedPassword = ""
153-
user.ConfirmationToken = c.requestBody["token"].(string)
156+
user.ConfirmationToken = fmt.Sprintf("%x", sha256.Sum224([]byte(c.email+c.requestBody["token"].(string))))
154157
require.NoError(ts.T(), err)
155158
require.NoError(ts.T(), ts.API.db.Create(user))
156159

‎api/magic_link.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -82,7 +82,7 @@ func (a *API) MagicLink(w http.ResponseWriter, r *http.Request) error {
8282

8383
mailer := a.Mailer(ctx)
8484
referrer := a.getReferrer(r)
85-
return a.sendMagicLink(tx, user, mailer, config.SMTP.MaxFrequency, referrer)
85+
return a.sendMagicLink(tx, user, mailer, config.SMTP.MaxFrequency, referrer, config.Mailer.OtpLength)
8686
})
8787
if err != nil {
8888
if errors.Is(err, MaxFrequencyLimitError) {

‎api/mail.go‎

Lines changed: 42 additions & 50 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ package api
22

33
import (
44
"context"
5+
"crypto/sha256"
56
"encoding/json"
67
"fmt"
78
"net/http"
@@ -14,7 +15,6 @@ import (
1415
"github.com/netlify/gotrue/storage"
1516
"github.com/pkg/errors"
1617
"github.com/sethvargo/go-password/password"
17-
"github.com/sirupsen/logrus"
1818
)
1919

2020
var (
@@ -69,14 +69,18 @@ func (a *API) GenerateLink(w http.ResponseWriter, r *http.Request) error {
6969
var url string
7070
referrer := a.getRedirectURLOrReferrer(r, params.RedirectTo)
7171
now := time.Now()
72+
otp, err := crypto.GenerateOtp(config.Mailer.OtpLength)
73+
if err != nil {
74+
return err
75+
}
7276
err = a.db.Transaction(func(tx *storage.Connection) error {
7377
var terr error
7478
switch params.Type {
7579
case "magiclink", "recovery":
7680
if terr = models.NewAuditLogEntry(tx, instanceID, user, models.UserRecoveryRequestedAction, "", nil); terr != nil {
7781
return terr
7882
}
79-
user.RecoveryToken = crypto.SecureToken()
83+
user.RecoveryToken = fmt.Sprintf("%x", sha256.Sum224([]byte(user.GetEmail()+otp)))
8084
user.RecoverySentAt = &now
8185
terr = errors.Wrap(tx.UpdateOnly(user, "recovery_token", "recovery_sent_at"), "Database error updating user for recovery")
8286
case "invite":
@@ -102,7 +106,7 @@ func (a *API) GenerateLink(w http.ResponseWriter, r *http.Request) error {
102106
}); terr != nil {
103107
return terr
104108
}
105-
user.ConfirmationToken = crypto.SecureToken()
109+
user.ConfirmationToken = fmt.Sprintf("%x", sha256.Sum224([]byte(user.GetEmail()+otp)))
106110
user.ConfirmationSentAt = &now
107111
user.InvitedAt = &now
108112
terr = errors.Wrap(tx.UpdateOnly(user, "confirmation_token", "confirmation_sent_at", "invited_at"), "Database error updating user for invite")
@@ -133,7 +137,7 @@ func (a *API) GenerateLink(w http.ResponseWriter, r *http.Request) error {
133137
return terr
134138
}
135139
}
136-
user.ConfirmationToken = crypto.SecureToken()
140+
user.ConfirmationToken = fmt.Sprintf("%x", sha256.Sum224([]byte(user.GetEmail()+otp)))
137141
user.ConfirmationSentAt = &now
138142
terr = errors.Wrap(tx.UpdateOnly(user, "confirmation_token", "confirmation_sent_at"), "Database error updating user for confirmation")
139143
default:
@@ -168,34 +172,36 @@ func (a *API) GenerateLink(w http.ResponseWriter, r *http.Request) error {
168172
return sendJSON(w, http.StatusOK, resp)
169173
}
170174

171-
func sendConfirmation(tx *storage.Connection, u *models.User, mailer mailer.Mailer, maxFrequency time.Duration, referrerURL string) error {
175+
func sendConfirmation(tx *storage.Connection, u *models.User, mailer mailer.Mailer, maxFrequency time.Duration, referrerURL string, otpLength int) error {
172176
var err error
173177
if u.ConfirmationSentAt != nil && !u.ConfirmationSentAt.Add(maxFrequency).Before(time.Now()) {
174178
return MaxFrequencyLimitError
175179
}
176180
oldToken := u.ConfirmationToken
177-
u.ConfirmationToken, err = generateUniqueEmailOtp(tx, confirmationToken)
181+
otp, err := crypto.GenerateOtp(otpLength)
178182
if err != nil {
179183
return err
180184
}
185+
u.ConfirmationToken = fmt.Sprintf("%x", sha256.Sum224([]byte(u.GetEmail()+otp)))
181186
now := time.Now()
182-
if err := mailer.ConfirmationMail(u, referrerURL); err != nil {
187+
if err := mailer.ConfirmationMail(u, otp, referrerURL); err != nil {
183188
u.ConfirmationToken = oldToken
184189
return errors.Wrap(err, "Error sending confirmation email")
185190
}
186191
u.ConfirmationSentAt = &now
187192
return errors.Wrap(tx.UpdateOnly(u, "confirmation_token", "confirmation_sent_at"), "Database error updating user for confirmation")
188193
}
189194

190-
func sendInvite(tx *storage.Connection, u *models.User, mailer mailer.Mailer, referrerURL string) error {
195+
func sendInvite(tx *storage.Connection, u *models.User, mailer mailer.Mailer, referrerURL string, otpLength int) error {
191196
var err error
192197
oldToken := u.ConfirmationToken
193-
u.ConfirmationToken, err = generateUniqueEmailOtp(tx, confirmationToken)
198+
otp, err := crypto.GenerateOtp(otpLength)
194199
if err != nil {
195200
return err
196201
}
202+
u.ConfirmationToken = fmt.Sprintf("%x", sha256.Sum224([]byte(u.GetEmail()+otp)))
197203
now := time.Now()
198-
if err := mailer.InviteMail(u, referrerURL); err != nil {
204+
if err := mailer.InviteMail(u, otp, referrerURL); err != nil {
199205
u.ConfirmationToken = oldToken
200206
return errors.Wrap(err, "Error sending invite email")
201207
}
@@ -204,60 +210,66 @@ func sendInvite(tx *storage.Connection, u *models.User, mailer mailer.Mailer, re
204210
return errors.Wrap(tx.UpdateOnly(u, "confirmation_token", "confirmation_sent_at", "invited_at"), "Database error updating user for invite")
205211
}
206212

207-
func (a *API) sendPasswordRecovery(tx *storage.Connection, u *models.User, mailer mailer.Mailer, maxFrequency time.Duration, referrerURL string) error {
213+
func (a *API) sendPasswordRecovery(tx *storage.Connection, u *models.User, mailer mailer.Mailer, maxFrequency time.Duration, referrerURL string, otpLength int) error {
208214
var err error
209215
if u.RecoverySentAt != nil && !u.RecoverySentAt.Add(maxFrequency).Before(time.Now()) {
210216
return MaxFrequencyLimitError
211217
}
212218

213219
oldToken := u.RecoveryToken
214-
u.RecoveryToken, err = generateUniqueEmailOtp(tx, recoveryToken)
220+
otp, err := crypto.GenerateOtp(otpLength)
215221
if err != nil {
216222
return err
217223
}
224+
u.RecoveryToken = fmt.Sprintf("%x", sha256.Sum224([]byte(u.GetEmail()+otp)))
218225
now := time.Now()
219-
if err := mailer.RecoveryMail(u, referrerURL); err != nil {
226+
if err := mailer.RecoveryMail(u, otp, referrerURL); err != nil {
220227
u.RecoveryToken = oldToken
221228
return errors.Wrap(err, "Error sending recovery email")
222229
}
223230
u.RecoverySentAt = &now
224231
return errors.Wrap(tx.UpdateOnly(u, "recovery_token", "recovery_sent_at"), "Database error updating user for recovery")
225232
}
226233

227-
func (a *API) sendReauthenticationOtp(tx *storage.Connection, u *models.User, mailer mailer.Mailer, maxFrequency time.Duration) error {
234+
func (a *API) sendReauthenticationOtp(tx *storage.Connection, u *models.User, mailer mailer.Mailer, maxFrequency time.Duration, otpLength int) error {
228235
var err error
229236
if u.ReauthenticationSentAt != nil && !u.ReauthenticationSentAt.Add(maxFrequency).Before(time.Now()) {
230237
return MaxFrequencyLimitError
231238
}
232239

233240
oldToken := u.ReauthenticationToken
234-
u.ReauthenticationToken, err = generateUniqueEmailOtp(tx, reauthenticationToken)
241+
otp, err := crypto.GenerateOtp(otpLength)
242+
if err != nil {
243+
return err
244+
}
245+
u.ReauthenticationToken = fmt.Sprintf("%x", sha256.Sum224([]byte(u.GetEmail()+otp)))
235246
if err != nil {
236247
return err
237248
}
238249
now := time.Now()
239-
if err := mailer.ReauthenticateMail(u); err != nil {
250+
if err := mailer.ReauthenticateMail(u, otp); err != nil {
240251
u.ReauthenticationToken = oldToken
241252
return errors.Wrap(err, "Error sending reauthentication email")
242253
}
243254
u.ReauthenticationSentAt = &now
244255
return errors.Wrap(tx.UpdateOnly(u, "reauthentication_token", "reauthentication_sent_at"), "Database error updating user for reauthentication")
245256
}
246257

247-
func (a *API) sendMagicLink(tx *storage.Connection, u *models.User, mailer mailer.Mailer, maxFrequency time.Duration, referrerURL string) error {
258+
func (a *API) sendMagicLink(tx *storage.Connection, u *models.User, mailer mailer.Mailer, maxFrequency time.Duration, referrerURL string, otpLength int) error {
248259
var err error
249260
// since Magic Link is just a recovery with a different template and behaviour
250261
// around new users we will reuse the recovery db timer to prevent potential abuse
251262
if u.RecoverySentAt != nil && !u.RecoverySentAt.Add(maxFrequency).Before(time.Now()) {
252263
return MaxFrequencyLimitError
253264
}
254265
oldToken := u.RecoveryToken
255-
u.RecoveryToken, err = generateUniqueEmailOtp(tx, recoveryToken)
266+
otp, err := crypto.GenerateOtp(otpLength)
256267
if err != nil {
257268
return err
258269
}
270+
u.RecoveryToken = fmt.Sprintf("%x", sha256.Sum224([]byte(u.GetEmail()+otp)))
259271
now := time.Now()
260-
if err := mailer.MagicLinkMail(u, referrerURL); err != nil {
272+
if err := mailer.MagicLinkMail(u, otp, referrerURL); err != nil {
261273
u.RecoveryToken = oldToken
262274
return errors.Wrap(err, "Error sending magic link email")
263275
}
@@ -266,22 +278,29 @@ func (a *API) sendMagicLink(tx *storage.Connection, u *models.User, mailer maile
266278
}
267279

268280
// sendEmailChange sends out an email change token to the new email.
269-
func (a *API) sendEmailChange(tx *storage.Connection, config *conf.Configuration, u *models.User, mailer mailer.Mailer, email string, referrerURL string) error {
281+
func (a *API) sendEmailChange(tx *storage.Connection, config *conf.Configuration, u *models.User, mailer mailer.Mailer, email string, referrerURL string, otpLength int) error {
270282
var err error
271-
u.EmailChangeTokenNew, err = generateUniqueEmailOtp(tx, emailChangeTokenNew)
283+
otpNew, err := crypto.GenerateOtp(otpLength)
272284
if err != nil {
273285
return err
274286
}
287+
u.EmailChangeTokenNew = fmt.Sprintf("%x", sha256.Sum224([]byte(u.EmailChange+otpNew)))
288+
289+
otpCurrent := ""
275290
if config.Mailer.SecureEmailChangeEnabled && u.GetEmail() != "" {
276-
u.EmailChangeTokenCurrent, err = generateUniqueEmailOtp(tx, emailChangeTokenCurrent)
291+
otpCurrent, err = crypto.GenerateOtp(otpLength)
292+
if err != nil {
293+
return err
294+
}
295+
u.EmailChangeTokenCurrent = fmt.Sprintf("%x", sha256.Sum224([]byte(u.GetEmail()+otpCurrent)))
277296
if err != nil {
278297
return err
279298
}
280299
}
281300
u.EmailChange = email
282301
u.EmailChangeConfirmStatus = zeroConfirmation
283302
now := time.Now()
284-
if err := mailer.EmailChangeMail(u, referrerURL); err != nil {
303+
if err := mailer.EmailChangeMail(u, otpNew, otpCurrent, referrerURL); err != nil {
285304
return err
286305
}
287306

@@ -306,30 +325,3 @@ func (a *API) validateEmail(ctx context.Context, email string) error {
306325
}
307326
return nil
308327
}
309-
310-
// generateUniqueEmailOtp returns a unique otp
311-
func generateUniqueEmailOtp(tx *storage.Connection, tokenType tokenType) (string, error) {
312-
maxRetries := 5
313-
otpLength := 20
314-
var otp string
315-
var err error
316-
for i := 0; i < maxRetries; i++ {
317-
otp, err = crypto.GenerateEmailOtp(otpLength)
318-
if err != nil {
319-
return "", err
320-
}
321-
_, err = models.FindUserByTokenAndTokenType(tx, otp, string(tokenType))
322-
if err != nil {
323-
if models.IsNotFoundError(err) {
324-
return otp, nil
325-
}
326-
return "", err
327-
}
328-
logrus.Warn("otp generated is not unique, retrying.")
329-
err = errors.New("Could not generate a unique email otp")
330-
}
331-
if err != nil {
332-
return "", err
333-
}
334-
return "", errors.New("Could not generate a unique email otp")
335-
}

‎api/phone.go‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ package api
22

33
import (
44
"context"
5+
"crypto/sha256"
56
"fmt"
67
"regexp"
78
"strings"
@@ -77,13 +78,13 @@ func (a *API) sendPhoneConfirmation(ctx context.Context, tx *storage.Connection,
7778
if err != nil {
7879
return internalServerError("error generating otp").WithInternalError(err)
7980
}
80-
*token = otp
81+
*token = fmt.Sprintf("%x", sha256.Sum224([]byte(phone+otp)))
8182

8283
var message string
8384
if config.Sms.Template == "" {
84-
message = fmt.Sprintf(defaultSmsMessage, *token)
85+
message = fmt.Sprintf(defaultSmsMessage, otp)
8586
} else {
86-
message = strings.Replace(config.Sms.Template, "{{ .Code }}", *token, -1)
87+
message = strings.Replace(config.Sms.Template, "{{ .Code }}", otp, -1)
8788
}
8889

8990
if serr := smsProvider.SendSms(phone, message); serr != nil {

‎api/reauthenticate.go‎

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,9 @@
11
package api
22

33
import (
4+
"crypto/sha256"
45
"errors"
6+
"fmt"
57
"net/http"
68

79
"github.com/gofrs/uuid"
@@ -54,7 +56,7 @@ func (a *API) Reauthenticate(w http.ResponseWriter, r *http.Request) error {
5456
}
5557
if email != "" {
5658
mailer := a.Mailer(ctx)
57-
return a.sendReauthenticationOtp(tx, user, mailer, config.SMTP.MaxFrequency)
59+
return a.sendReauthenticationOtp(tx, user, mailer, config.SMTP.MaxFrequency, config.Mailer.OtpLength)
5860
} else if phone != "" {
5961
smsProvider, terr := sms_provider.GetSmsProvider(*config)
6062
if terr != nil {
@@ -81,9 +83,11 @@ func (a *API) verifyReauthentication(nonce string, tx *storage.Connection, confi
8183
}
8284
var isValid bool
8385
if user.GetEmail() != "" {
84-
isValid = isOtpValid(nonce, user.ReauthenticationToken, user.ReauthenticationSentAt, config.Mailer.OtpExp)
86+
tokenHash := fmt.Sprintf("%x", sha256.Sum224([]byte(user.GetEmail()+nonce)))
87+
isValid = isOtpValid(tokenHash, user.ReauthenticationToken, user.ReauthenticationSentAt, config.Mailer.OtpExp)
8588
} else if user.GetPhone() != "" {
86-
isValid = isOtpValid(nonce, user.ReauthenticationToken, user.ReauthenticationSentAt, config.Sms.OtpExp)
89+
tokenHash := fmt.Sprintf("%x", sha256.Sum224([]byte(user.GetPhone()+nonce)))
90+
isValid = isOtpValid(tokenHash, user.ReauthenticationToken, user.ReauthenticationSentAt, config.Sms.OtpExp)
8791
} else {
8892
return unprocessableEntityError("Reauthentication requires an email or a phone number")
8993
}

‎api/recover.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -51,7 +51,7 @@ func (a *API) Recover(w http.ResponseWriter, r *http.Request) error {
5151
}
5252
mailer := a.Mailer(ctx)
5353
referrer := a.getReferrer(r)
54-
return a.sendPasswordRecovery(tx, user, mailer, config.SMTP.MaxFrequency, referrer)
54+
return a.sendPasswordRecovery(tx, user, mailer, config.SMTP.MaxFrequency, referrer, config.Mailer.OtpLength)
5555
})
5656
if err != nil {
5757
if errors.Is(err, MaxFrequencyLimitError) {

‎api/signup.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -132,7 +132,7 @@ func (a *API) Signup(w http.ResponseWriter, r *http.Request) error {
132132
}); terr != nil {
133133
return terr
134134
}
135-
if terr = sendConfirmation(tx, user, mailer, config.SMTP.MaxFrequency, referrer); terr != nil {
135+
if terr = sendConfirmation(tx, user, mailer, config.SMTP.MaxFrequency, referrer, config.Mailer.OtpLength); terr != nil {
136136
if errors.Is(terr, MaxFrequencyLimitError) {
137137
now := time.Now()
138138
left := user.ConfirmationSentAt.Add(config.SMTP.MaxFrequency).Sub(now) / time.Second

0 commit comments

Comments
 (0)