Skip to content

Commit 93722eb

Browse files
committed
fix: add limit to recovery session
A session would be deemed a recovery session indefinitely long. This timeboxes the recovery to 15 minutes.
1 parent ab7c9f9 commit 93722eb

File tree

2 files changed

+49
-6
lines changed

2 files changed

+49
-6
lines changed

internal/api/user.go

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -170,12 +170,16 @@ func (a *API) UserUpdate(w http.ResponseWriter, r *http.Request) error {
170170
if config.Security.UpdatePasswordRequireCurrentPassword {
171171
// user may be in a password reset flow, where they do not have a currentPassword
172172
isRecoverySession := false
173-
for _, claim := range session.AMRClaims {
174-
// password recovery flows can be via otp or a magic link, check if the current session
175-
// was created with one of those
176-
if claim.GetAuthenticationMethod() == "otp" || claim.GetAuthenticationMethod() == "magiclink" {
177-
isRecoverySession = true
178-
break
173+
174+
// it is only a recovery session if it was recently created
175+
if time.Now().Before(session.CreatedAt.Add(15*time.Minute)) {
176+
for _, claim := range session.AMRClaims {
177+
// password recovery flows can be via otp or a magic link, check if the current session
178+
// was created with one of those
179+
if claim.GetAuthenticationMethod() == "otp" || claim.GetAuthenticationMethod() == "magiclink" {
180+
isRecoverySession = true
181+
break
182+
}
179183
}
180184
}
181185
if !isRecoverySession {

internal/api/user_test.go

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -289,6 +289,27 @@ func (ts *UserTestSuite) TestUserUpdatePassword() {
289289
notRecentlyLoggedIn.ID).Exec(),
290290
)
291291

292+
// create a recovery session (OTP) created recently (within 15 minutes)
293+
recentRecoverySession, err := models.NewSession(u.ID, nil)
294+
require.NoError(ts.T(), err)
295+
require.NoError(ts.T(), ts.API.db.Create(recentRecoverySession))
296+
require.NoError(ts.T(), models.AddClaimToSession(ts.API.db, recentRecoverySession.ID, models.OTP))
297+
recentRecoverySession, err = models.FindSessionByID(ts.API.db, recentRecoverySession.ID, true)
298+
require.NoError(ts.T(), err)
299+
300+
// create a recovery session (OTP) whose created_at is older than 15 minutes
301+
staleRecoverySession, err := models.NewSession(u.ID, nil)
302+
require.NoError(ts.T(), err)
303+
require.NoError(ts.T(), ts.API.db.Create(staleRecoverySession))
304+
require.NoError(ts.T(), models.AddClaimToSession(ts.API.db, staleRecoverySession.ID, models.OTP))
305+
require.NoError(ts.T(), ts.API.db.RawQuery(
306+
"update "+staleRecoverySession.TableName()+" set created_at = ? where id = ?",
307+
time.Now().Add(-20*time.Minute),
308+
staleRecoverySession.ID).Exec(),
309+
)
310+
staleRecoverySession, err = models.FindSessionByID(ts.API.db, staleRecoverySession.ID, true)
311+
require.NoError(ts.T(), err)
312+
292313
type expected struct {
293314
code int
294315
isAuthenticated bool
@@ -365,6 +386,24 @@ func (ts *UserTestSuite) TestUserUpdatePassword() {
365386
sessionId: r.SessionId,
366387
expected: expected{code: http.StatusBadRequest, isAuthenticated: false},
367388
},
389+
{
390+
desc: "Current password not required for recent recovery session (OTP, within 15 minutes)",
391+
newPassword: "newpassword123",
392+
nonce: "",
393+
requireReauthentication: false,
394+
requireCurrentPassword: true,
395+
sessionId: &recentRecoverySession.ID,
396+
expected: expected{code: http.StatusOK, isAuthenticated: true},
397+
},
398+
{
399+
desc: "Current password required for stale recovery session (OTP, older than 15 minutes)",
400+
newPassword: "newpassword456",
401+
nonce: "",
402+
requireReauthentication: false,
403+
requireCurrentPassword: true,
404+
sessionId: &staleRecoverySession.ID,
405+
expected: expected{code: http.StatusBadRequest, isAuthenticated: false},
406+
},
368407
}
369408

370409
for _, c := range cases {

0 commit comments

Comments
 (0)