Password confirmation on reset page

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
 	}
 

internal/auth/templates/reset.html

@@ -36,6 +36,17 @@
           </span>
         </div>
       </div>
+      <div class="mb-3">
+        <label class="form-label" for="confirm_pw">Confirm new password</label>
+        <div class="input-group input-group-flat">
+          <input type="password" class="form-control" id="confirm_pw" name="confirm_password"
+                 autocomplete="new-password" minlength="{{.MinPasswordLen}}"
+                 placeholder="Type it again" required>
+          <span class="input-group-text">
+            <button type="button" class="link-secondary border-0 bg-transparent p-0" aria-pressed="false" title="Show password" data-pwtoggle="confirm_pw" aria-label="Show password">{{template "icon-eye" ""}}</button>
+          </span>
+        </div>
+      </div>
       <div class="form-footer mt-0">
         <button type="submit" class="btn btn-primary w-100" data-testid="reset-submit">Set new password</button>
       </div>

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") == "" {

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),