diff --git a/internal/auth/reset_handlers.go b/internal/auth/reset_handlers.go
index ef3680f..d0fb7f7 100644
--- a/internal/auth/reset_handlers.go
+++ b/internal/auth/reset_handlers.go
@@ -86,9 +86,20 @@ func (s *Handlers) handleReset(w http.ResponseWriter, r *http.Request) {
token := chi.URLParam(r, "token")
password := r.FormValue("new_password")
- // Validated BEFORE the token is spent: a too-short password must cost the user a
- // correction, not their only link.
- if !ValidPassword(password) {
+ // Both checks run BEFORE the token is spent: a correctable mistake — too short,
+ // or a mistyped confirmation — must cost the user a correction, not their only
+ // link. Whoever is here can't sign in, so a dead link is the expensive failure.
+ errMsg := ""
+ switch {
+ case !ValidPassword(password):
+ errMsg = fmt.Sprintf("Password must be at least %d characters.", MinPasswordLen)
+ case r.FormValue("confirm_password") != password:
+ // Worth confirming here more than anywhere else: it's a value the user can't
+ // see, typed by someone already locked out, and the next thing they do with it
+ // is sign in.
+ errMsg = "The passwords don't match."
+ }
+ if errMsg != "" {
u, ok, err := s.Auth.PeekResetToken(ctx, token)
if err != nil {
s.ServerError(w, r, "could not check reset link", err)
@@ -98,8 +109,7 @@ func (s *Handlers) handleReset(w http.ResponseWriter, r *http.Request) {
s.renderResetInvalid(w, r)
return
}
- s.renderReset(w, r, token, u, fmt.Sprintf(
- "Password must be at least %d characters.", MinPasswordLen))
+ s.renderReset(w, r, token, u, errMsg)
return
}
diff --git a/internal/auth/templates/reset.html b/internal/auth/templates/reset.html
index 2c41704..585deb7 100644
--- a/internal/auth/templates/reset.html
+++ b/internal/auth/templates/reset.html
@@ -36,6 +36,17 @@
+
+
+
+
+
+
+
+
+
diff --git a/internal/core/password_reset_test.go b/internal/core/password_reset_test.go
index e49d552..eab51d5 100644
--- a/internal/core/password_reset_test.go
+++ b/internal/core/password_reset_test.go
@@ -222,7 +222,7 @@ func TestResetHappyPath(t *testing.T) {
}
const newPassword = "an-entirely-new-password"
- resp := post(t, ts, h.auth, path, url.Values{"new_password": {newPassword}})
+ resp := post(t, ts, h.auth, path, url.Values{"new_password": {newPassword}, "confirm_password": {newPassword}})
resp.Body.Close()
loc, _ := url.Parse(resp.Header.Get("Location"))
if resp.StatusCode != http.StatusSeeOther || loc.Path != "/login" || loc.Query().Get("ok") == "" {
@@ -254,10 +254,10 @@ func TestResetLinkIsSingleUse(t *testing.T) {
forgot(t, ts, h, "single@example.test").Body.Close()
path := linkPath(t, sender.last(t))
- first := post(t, ts, h.auth, path, url.Values{"new_password": {"first-new-password"}})
+ first := post(t, ts, h.auth, path, url.Values{"new_password": {"first-new-password"}, "confirm_password": {"first-new-password"}})
first.Body.Close()
- second := post(t, ts, h.auth, path, url.Values{"new_password": {"second-new-password"}})
+ second := post(t, ts, h.auth, path, url.Values{"new_password": {"second-new-password"}, "confirm_password": {"second-new-password"}})
body := readAll(t, second)
if !strings.Contains(body, "This link doesn't work") {
t.Error("a spent reset link was accepted a second time")
@@ -284,7 +284,7 @@ func TestResetGetDoesNotSpendToken(t *testing.T) {
do(t, ts, h.auth, path).Body.Close()
do(t, ts, h.auth, path).Body.Close()
- resp := post(t, ts, h.auth, path, url.Values{"new_password": {"still-works-password"}})
+ resp := post(t, ts, h.auth, path, url.Values{"new_password": {"still-works-password"}, "confirm_password": {"still-works-password"}})
resp.Body.Close()
loc, _ := url.Parse(resp.Header.Get("Location"))
if loc.Path != "/login" || loc.Query().Get("ok") == "" {
@@ -302,7 +302,7 @@ func TestResetRejectsShortPasswordWithoutSpendingToken(t *testing.T) {
forgot(t, ts, h, "shortpw@example.test").Body.Close()
path := linkPath(t, sender.last(t))
- bad := post(t, ts, h.auth, path, url.Values{"new_password": {"short"}})
+ bad := post(t, ts, h.auth, path, url.Values{"new_password": {"short"}, "confirm_password": {"short"}})
body := readAll(t, bad)
if !strings.Contains(body, "at least") {
t.Errorf("no length error shown:\n%s", body)
@@ -312,7 +312,7 @@ func TestResetRejectsShortPasswordWithoutSpendingToken(t *testing.T) {
}
// The same link still works with an acceptable password.
- good := post(t, ts, h.auth, path, url.Values{"new_password": {"now-long-enough-password"}})
+ good := post(t, ts, h.auth, path, url.Values{"new_password": {"now-long-enough-password"}, "confirm_password": {"now-long-enough-password"}})
good.Body.Close()
loc, _ := url.Parse(good.Header.Get("Location"))
if loc.Query().Get("ok") == "" {
@@ -320,6 +320,51 @@ func TestResetRejectsShortPasswordWithoutSpendingToken(t *testing.T) {
}
}
+// TestResetRejectsMismatchedConfirmationWithoutSpendingToken: the confirmation
+// field is worth having on this page above all others — the person typing is
+// already locked out, can't see what they're typing, and the next thing they do
+// with it is sign in. Like the length check, a mismatch has to be caught before
+// the token is consumed, or a typo costs them the link rather than a retry.
+func TestResetRejectsMismatchedConfirmationWithoutSpendingToken(t *testing.T) {
+ t.Parallel()
+ st, ctx, ts, h, sender := splitServerMail(t)
+ u := recoverable(t, st, ctx, "typopw", "typopw@example.test")
+ forgot(t, ts, h, "typopw@example.test").Body.Close()
+ path := linkPath(t, sender.last(t))
+
+ bad := post(t, ts, h.auth, path, url.Values{
+ "new_password": {"a-long-enough-password"},
+ "confirm_password": {"a-long-enough-passwrod"},
+ })
+ body := readAll(t, bad)
+ if !strings.Contains(body, "passwords don't match") && !strings.Contains(body, "passwords don't match") {
+ t.Errorf("no mismatch error shown:\n%s", body)
+ }
+ if strings.Contains(body, "This link doesn't work") {
+ t.Fatal("the token was spent by a mistyped confirmation")
+ }
+
+ // Nothing was written, so the old password still signs in.
+ before, err := st.GetUserByID(ctx, u.ID)
+ if err != nil {
+ t.Fatalf("reload user: %v", err)
+ }
+ if before.PasswordHash == nil {
+ t.Fatal("a rejected reset cleared the password")
+ }
+
+ // The same link still works once the two fields agree.
+ good := post(t, ts, h.auth, path, url.Values{
+ "new_password": {"a-long-enough-password"},
+ "confirm_password": {"a-long-enough-password"},
+ })
+ good.Body.Close()
+ loc, _ := url.Parse(good.Header.Get("Location"))
+ if loc.Query().Get("ok") == "" {
+ t.Errorf("retry after a mismatch failed: %q", good.Header.Get("Location"))
+ }
+}
+
// TestResetRevokesExistingSessions: if an attacker got in with the stolen password,
// the reset is the moment they're evicted. A reset that left their session alive would
// leave the account compromised while looking recovered.
@@ -355,7 +400,7 @@ func TestResetRevokesExistingSessions(t *testing.T) {
forgot(t, ts, h, "evict@example.test").Body.Close()
path := linkPath(t, sender.last(t))
- post(t, ts, h.auth, path, url.Values{"new_password": {"brand-new-password-here"}}).Body.Close()
+ post(t, ts, h.auth, path, url.Values{"new_password": {"brand-new-password-here"}, "confirm_password": {"brand-new-password-here"}}).Body.Close()
after := do(t, ts, h.app, "/", victim)
after.Body.Close()
@@ -463,7 +508,7 @@ func TestResetRefusedIfPasswordRemovedAfterSending(t *testing.T) {
t.Fatalf("clear password: %v", err)
}
- resp := post(t, ts, h.auth, path, url.Values{"new_password": {"should-not-apply-pw"}})
+ resp := post(t, ts, h.auth, path, url.Values{"new_password": {"should-not-apply-pw"}, "confirm_password": {"should-not-apply-pw"}})
resp.Body.Close()
loc, _ := url.Parse(resp.Header.Get("Location"))
if loc.Query().Get("error") == "" {
diff --git a/internal/e2e/password_reset_test.go b/internal/e2e/password_reset_test.go
index 8b1f3be..e83ed26 100644
--- a/internal/e2e/password_reset_test.go
+++ b/internal/e2e/password_reset_test.go
@@ -117,6 +117,7 @@ func TestE2EPasswordRecoveryRoundTrip(t *testing.T) {
// own link in the same message.
chromedp.Text(`.card-body`, &resetHeading, chromedp.ByQuery),
chromedp.SendKeys(`#new_pw`, newPassword, chromedp.ByQuery),
+ chromedp.SendKeys(`#confirm_pw`, newPassword, chromedp.ByQuery),
chromedp.Click(`[data-testid="reset-submit"]`, chromedp.ByQuery),
// Lands back on sign-in with the success flash.
chromedp.WaitVisible(`.alert-success`, chromedp.ByQuery),