Skip to content

Commit 7c88c25

Browse files
mattwobertsmattwoberts
authored
Merge pull request #1468 from getfider/fix/signin-verification-key
Fix: Use strong key for sign-in email link
2 parents 98bf7d9 + 4df507b commit 7c88c25

File tree

8 files changed

+74
-32
lines changed

8 files changed

+74
-32
lines changed

‎app/actions/signin.go‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,11 +16,13 @@ import (
1616
type SignInByEmail struct {
1717
Email string `json:"email" format:"lower"`
1818
VerificationCode string
19+
LinkKey string
1920
}
2021

2122
func NewSignInByEmail() *SignInByEmail {
2223
return &SignInByEmail{
2324
VerificationCode: entity.GenerateEmailVerificationCode(),
25+
LinkKey: entity.GenerateEmailVerificationKey(),
2426
}
2527
}
2628

@@ -118,11 +120,13 @@ type SignInByEmailWithName struct {
118120
Email string `json:"email" format:"lower"`
119121
Name string `json:"name"`
120122
VerificationCode string
123+
LinkKey string
121124
}
122125

123126
func NewSignInByEmailWithName() *SignInByEmailWithName {
124127
return &SignInByEmailWithName{
125128
VerificationCode: entity.GenerateEmailVerificationCode(),
129+
LinkKey: entity.GenerateEmailVerificationKey(),
126130
}
127131
}
128132

‎app/handlers/signin.go‎

Lines changed: 24 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -87,15 +87,16 @@ func SignInByEmail() web.HandlerFunc {
8787
// Only send code if user exists
8888
if userExists {
8989
err := bus.Dispatch(c, &cmd.SaveVerificationKey{
90-
Key: action.VerificationCode,
90+
Key: action.LinkKey,
91+
Code: action.VerificationCode,
9192
Duration: 15 * time.Minute,
9293
Request: action,
9394
})
9495
if err != nil {
9596
return c.Failure(err)
9697
}
9798

98-
c.Enqueue(tasks.SendSignInEmail(action.Email, action.VerificationCode))
99+
c.Enqueue(tasks.SendSignInEmail(action.Email, action.LinkKey, action.VerificationCode))
99100
}
100101

101102
return c.Ok(web.Map{
@@ -129,15 +130,16 @@ func SignInByEmailWithName() web.HandlerFunc {
129130

130131
// Save verification with name
131132
err = bus.Dispatch(c, &cmd.SaveVerificationKey{
132-
Key: action.VerificationCode,
133+
Key: action.LinkKey,
134+
Code: action.VerificationCode,
133135
Duration: 15 * time.Minute,
134136
Request: action,
135137
})
136138
if err != nil {
137139
return c.Failure(err)
138140
}
139141

140-
c.Enqueue(tasks.SendSignInEmail(action.Email, action.VerificationCode))
142+
c.Enqueue(tasks.SendSignInEmail(action.Email, action.LinkKey, action.VerificationCode))
141143

142144
return c.Ok(web.Map{})
143145
}
@@ -169,18 +171,20 @@ func VerifySignInCode() web.HandlerFunc {
169171

170172
result := verification.Result
171173

172-
// Check if already verified (with grace period)
174+
// Code is single-use: reject if already verified
173175
if result.VerifiedAt != nil {
174-
if time.Since(*result.VerifiedAt) > 5*time.Minute {
175-
return c.Gone()
176-
}
177-
} else {
178-
// Check if expired
179-
if time.Now().After(result.ExpiresAt) {
180-
// Mark as verified to prevent reuse
181-
_ = bus.Dispatch(c, &cmd.SetKeyAsVerified{Key: action.Code})
182-
return c.Gone()
183-
}
176+
return c.BadRequest(web.Map{
177+
"code": "Invalid or expired verification code",
178+
})
179+
}
180+
181+
// Check if expired
182+
if time.Now().After(result.ExpiresAt) {
183+
// Mark as verified to prevent reuse
184+
_ = bus.Dispatch(c, &cmd.SetKeyAsVerified{Key: result.Key})
185+
return c.BadRequest(web.Map{
186+
"code": "Invalid or expired verification code",
187+
})
184188
}
185189

186190
// Check if user exists
@@ -207,7 +211,7 @@ func VerifySignInCode() web.HandlerFunc {
207211
}
208212

209213
// Mark code as verified
210-
err = bus.Dispatch(c, &cmd.SetKeyAsVerified{Key: action.Code})
214+
err = bus.Dispatch(c, &cmd.SetKeyAsVerified{Key: result.Key})
211215
if err != nil {
212216
return c.Failure(err)
213217
}
@@ -226,7 +230,7 @@ func VerifySignInCode() web.HandlerFunc {
226230
}
227231

228232
// Mark code as verified
229-
err = bus.Dispatch(c, &cmd.SetKeyAsVerified{Key: action.Code})
233+
err = bus.Dispatch(c, &cmd.SetKeyAsVerified{Key: result.Key})
230234
if err != nil {
231235
return c.Failure(err)
232236
}
@@ -254,7 +258,8 @@ func ResendSignInCode() web.HandlerFunc {
254258

255259
// Save new verification code
256260
err := bus.Dispatch(c, &cmd.SaveVerificationKey{
257-
Key: action.VerificationCode,
261+
Key: action.LinkKey,
262+
Code: action.VerificationCode,
258263
Duration: 15 * time.Minute,
259264
Request: action,
260265
})
@@ -263,7 +268,7 @@ func ResendSignInCode() web.HandlerFunc {
263268
}
264269

265270
// Send new email
266-
c.Enqueue(tasks.SendSignInEmail(action.Email, action.VerificationCode))
271+
c.Enqueue(tasks.SendSignInEmail(action.Email, action.LinkKey, action.VerificationCode))
267272

268273
return c.Ok(web.Map{})
269274
}

‎app/handlers/signin_test.go‎

Lines changed: 30 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,8 @@ func TestSignInByEmailHandler_ExistingUser(t *testing.T) {
5757
ExecutePost(handlers.SignInByEmail(), `{ "email": "jon.snow@got.com" }`)
5858

5959
Expect(code).Equals(http.StatusOK)
60-
Expect(saveKeyCmd.Key).HasLen(6)
60+
Expect(saveKeyCmd.Key).HasLen(64)
61+
Expect(saveKeyCmd.Code).HasLen(6)
6162
Expect(saveKeyCmd.Request.GetKind()).Equals(enum.EmailVerificationKindSignIn)
6263
Expect(saveKeyCmd.Request.GetEmail()).Equals("jon.snow@got.com")
6364
Expect(response.Body.String()).ContainsSubstring(`"userExists":true`)
@@ -98,7 +99,8 @@ func TestSignInByEmailWithNameHandler_NewUser(t *testing.T) {
9899
ExecutePost(handlers.SignInByEmailWithName(), `{ "email": "new.user@got.com", "name": "New User" }`)
99100

100101
Expect(code).Equals(http.StatusOK)
101-
Expect(saveKeyCmd.Key).HasLen(6)
102+
Expect(saveKeyCmd.Key).HasLen(64)
103+
Expect(saveKeyCmd.Code).HasLen(6)
102104
Expect(saveKeyCmd.Request.GetKind()).Equals(enum.EmailVerificationKindSignIn)
103105
Expect(saveKeyCmd.Request.GetEmail()).Equals("new.user@got.com")
104106
Expect(saveKeyCmd.Request.GetName()).Equals("New User")
@@ -820,7 +822,7 @@ func TestVerifySignInCodeHandler_ExpiredCode(t *testing.T) {
820822
OnTenant(mock.DemoTenant).
821823
ExecutePost(handlers.VerifySignInCode(), `{ "email": "jon.snow@got.com", "code": "123456" }`)
822824

823-
Expect(code).Equals(http.StatusGone)
825+
Expect(code).Equals(http.StatusBadRequest)
824826
}
825827

826828
func TestVerifySignInCodeHandler_CorrectCode_ExistingUser(t *testing.T) {
@@ -951,6 +953,29 @@ func TestVerifySignInCodeHandler_CorrectCode_NewUser_PrivateTenant(t *testing.T)
951953
Expect(code).Equals(http.StatusForbidden)
952954
}
953955

956+
func TestVerifySignInCodeHandler_AlreadyVerifiedCode_ShouldReject(t *testing.T) {
957+
RegisterT(t)
958+
959+
verifiedAt := time.Now().Add(-1 * time.Minute)
960+
bus.AddHandler(func(ctx context.Context, q *query.GetVerificationByEmailAndCode) error {
961+
q.Result = &entity.EmailVerification{
962+
Email: "jon.snow@got.com",
963+
Key: "some-long-link-key",
964+
CreatedAt: time.Now().Add(-10 * time.Minute),
965+
ExpiresAt: time.Now().Add(5 * time.Minute),
966+
VerifiedAt: &verifiedAt,
967+
}
968+
return nil
969+
})
970+
971+
server := mock.NewServer()
972+
code, _ := server.
973+
OnTenant(mock.DemoTenant).
974+
ExecutePost(handlers.VerifySignInCode(), `{ "email": "jon.snow@got.com", "code": "123456" }`)
975+
976+
Expect(code).Equals(http.StatusBadRequest)
977+
}
978+
954979
func TestResendSignInCodeHandler_ValidEmail(t *testing.T) {
955980
RegisterT(t)
956981

@@ -966,7 +991,8 @@ func TestResendSignInCodeHandler_ValidEmail(t *testing.T) {
966991
ExecutePost(handlers.ResendSignInCode(), `{ "email": "jon.snow@got.com" }`)
967992

968993
Expect(code).Equals(http.StatusOK)
969-
Expect(saveKeyCmd.Key).HasLen(6)
994+
Expect(saveKeyCmd.Key).HasLen(64)
995+
Expect(saveKeyCmd.Code).HasLen(6)
970996
Expect(saveKeyCmd.Request.GetKind()).Equals(enum.EmailVerificationKindSignIn)
971997
Expect(saveKeyCmd.Request.GetEmail()).Equals("jon.snow@got.com")
972998
}

‎app/models/cmd/tenant.go‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,7 @@ type ActivateTenant struct {
4747

4848
type SaveVerificationKey struct {
4949
Key string
50+
Code string
5051
Duration time.Duration
5152
Request NewEmailVerification
5253
}

‎app/services/sqlstore/postgres/tenant.go‎

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -145,7 +145,7 @@ func getVerificationByEmailAndCode(ctx context.Context, q *query.GetVerification
145145
return using(ctx, func(trx *dbx.Trx, tenant *entity.Tenant, user *entity.User) error {
146146
verification := dbEntities.EmailVerification{}
147147

148-
query := "SELECT id, email, name, key, created_at, verified_at, expires_at, kind, user_id FROM email_verifications WHERE tenant_id = $1 AND email = $2 AND key = $3 AND kind = $4 LIMIT 1"
148+
query := "SELECT id, email, name, key, created_at, verified_at, expires_at, kind, user_id FROM email_verifications WHERE tenant_id = $1 AND email = $2 AND code = $3 AND kind = $4 LIMIT 1"
149149
err := trx.Get(&verification, query, tenant.ID, q.Email, q.Code, q.Kind)
150150
if err != nil {
151151
return errors.Wrap(err, "failed to get email verification by email and code")
@@ -163,8 +163,13 @@ func saveVerificationKey(ctx context.Context, c *cmd.SaveVerificationKey) error
163163
userID = c.Request.GetUser().ID
164164
}
165165

166-
query := "INSERT INTO email_verifications (tenant_id, email, created_at, expires_at, key, name, kind, user_id) VALUES ($1, $2, $3, $4, $5, $6, $7, $8)"
167-
_, err := trx.Execute(query, tenant.ID, c.Request.GetEmail(), time.Now(), time.Now().Add(c.Duration), c.Key, c.Request.GetName(), c.Request.GetKind(), userID)
166+
var code any
167+
if c.Code != "" {
168+
code = c.Code
169+
}
170+
171+
query := "INSERT INTO email_verifications (tenant_id, email, created_at, expires_at, key, name, kind, user_id, code) VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9)"
172+
_, err := trx.Execute(query, tenant.ID, c.Request.GetEmail(), time.Now(), time.Now().Add(c.Duration), c.Key, c.Request.GetName(), c.Request.GetKind(), userID, code)
168173
if err != nil {
169174
return errors.Wrap(err, "failed to save verification key for kind '%d'", c.Request.GetKind())
170175
}

‎app/tasks/signin.go‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -9,12 +9,12 @@ import (
99
)
1010

1111
// SendSignInEmail is used to send the sign in email to requestor
12-
func SendSignInEmail(email, verificationCode string) worker.Task {
12+
func SendSignInEmail(email, linkKey, code string) worker.Task {
1313
return describe("Send sign in email", func(c *worker.Context) error {
1414
to := dto.NewRecipient("", email, dto.Props{
1515
"siteName": c.Tenant().Name,
16-
"code": verificationCode,
17-
"link": link(web.BaseURL(c), "/signin/verify?k=%s", verificationCode),
16+
"code": code,
17+
"link": link(web.BaseURL(c), "/signin/verify?k=%s", linkKey),
1818
})
1919

2020
bus.Publish(c, &cmd.SendMail{

‎app/tasks/signin_test.go‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@ func TestSendSignInEmailTask(t *testing.T) {
1616
bus.Init(emailmock.Service{})
1717

1818
worker := mock.NewWorker()
19-
task := tasks.SendSignInEmail("jon@got.com", "9876")
19+
task := tasks.SendSignInEmail("jon@got.com", "theLinkKey123", "987654")
2020

2121
err := worker.
2222
OnTenant(mock.DemoTenant).
@@ -39,8 +39,8 @@ func TestSendSignInEmailTask(t *testing.T) {
3939
Address: "jon@got.com",
4040
Props: dto.Props{
4141
"siteName": mock.DemoTenant.Name,
42-
"code": "9876",
43-
"link": "<a href='http://domain.com/signin/verify?k=9876'>http://domain.com/signin/verify?k=9876</a>",
42+
"code": "987654",
43+
"link": "<a href='http://domain.com/signin/verify?k=theLinkKey123'>http://domain.com/signin/verify?k=theLinkKey123</a>",
4444
},
4545
})
4646
}

‎migrations/202602250001_add_verification_code_column.sql‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
ALTER TABLE email_verifications ADD COLUMN code VARCHAR(6) NULL;

0 commit comments

Comments
 (0)