Code review

server/errors.go

@@ -167,7 +167,7 @@ var (
 	errHTTPTooManyRequestsLimitSubscriptions         = &errHTTP{42903, http.StatusTooManyRequests, "limit reached: too many active subscriptions", "https://ntfy.sh/docs/publish/#limitations", nil}
 	errHTTPTooManyRequestsLimitTotalTopics           = &errHTTP{42904, http.StatusTooManyRequests, "limit reached: the total number of topics on the server has been reached, please contact the admin", "https://ntfy.sh/docs/publish/#limitations", nil}
 	errHTTPTooManyRequestsLimitAttachmentBandwidth   = &errHTTP{42905, http.StatusTooManyRequests, "limit reached: daily bandwidth reached", "https://ntfy.sh/docs/publish/#limitations", nil}
-	errHTTPTooManyRequestsLimitAccountActions       = &errHTTP{42906, http.StatusTooManyRequests, "limit reached: too many account requests", "https://ntfy.sh/docs/publish/#limitations", nil} // FIXME document limit
+	errHTTPTooManyRequestsLimitAccountActions        = &errHTTP{42906, http.StatusTooManyRequests, "limit reached: too many account requests", "https://ntfy.sh/docs/publish/#limitations", nil} // FIXME document limit
 	errHTTPTooManyRequestsLimitReservations          = &errHTTP{42907, http.StatusTooManyRequests, "limit reached: too many topic reservations for this user", "", nil}
 	errHTTPTooManyRequestsLimitMessages              = &errHTTP{42908, http.StatusTooManyRequests, "limit reached: daily message quota reached", "https://ntfy.sh/docs/publish/#limitations", nil}
 	errHTTPTooManyRequestsLimitAuthFailure           = &errHTTP{42909, http.StatusTooManyRequests, "limit reached: too many auth failures", "https://ntfy.sh/docs/publish/#limitations", nil} // FIXME document limit

server/server_account.go

@@ -825,21 +825,20 @@ func (s *Server) handleAccountPasswordResetRequest(w http.ResponseWriter, r *htt
 	return s.writeJSON(w, newSuccessResponse())
 }
 
-// resolveResetTarget resolves a reset identifier to a single account and its primary email.
-// The identifier is tried first as a username, then as a primary email address. It returns
-// ok=false if no account with a primary email matches (reset requires a verified primary email).
+// resolveResetTarget resolves a reset identifier (username or primary email) to a single account
+// and its primary email. It applies the reset policy on top of the lookup: provisioned users are
+// excluded, and ok=false is returned unless the account has a verified primary email (reset
+// requires one, and that is where the link is sent).
 func (s *Server) resolveResetTarget(identifier string) (userID string, email string, ok bool) {
-	if u, err := s.userManager.User(identifier); err == nil && u != nil && !u.Provisioned {
-		if primary, perr := s.userManager.PrimaryEmail(u.ID); perr == nil && primary != "" {
-			return u.ID, primary, true
-		}
+	u, err := s.userManager.UserByEmailOrUsername(identifier)
+	if err != nil || u == nil || u.Provisioned {
+		return "", "", false
 	}
-	if uid, err := s.userManager.UserIDByPrimaryEmail(identifier); err == nil {
-		if u, uerr := s.userManager.UserByID(uid); uerr == nil && !u.Provisioned {
-			return uid, identifier, true
-		}
+	primary, err := s.userManager.PrimaryEmail(u.ID)
+	if err != nil || primary == "" {
+		return "", "", false
 	}
-	return "", "", false
+	return u.ID, primary, true
 }
 
 // handleAccountPasswordReset performs the reset (POST /v1/account/password/reset, unauthenticated):

server/server_account_email_test.go

@@ -259,6 +259,37 @@ func TestAccount_PasswordReset_ByEmail(t *testing.T) {
 	})
 }
 
+func TestAccount_PasswordReset_EmailLookalikeUsernameDoesNotShadow(t *testing.T) {
+	forEachBackend(t, func(t *testing.T, databaseURL string) {
+		s, mailer, auth := newEmailTestServer(t, databaseURL)
+		defer s.closeDatabases()
+
+		// Account A (the email owner): user "ben" with verified primary email "phil@example.com"
+		verifyEmailFor(t, s, mailer, auth, "phil@example.com")
+
+		// Account B (the squatter): a different account whose USERNAME looks like A's email, with
+		// its own, different verified primary email
+		require.Nil(t, s.userManager.AddUser("phil@example.com", "squatterpass", user.RoleUser, false))
+		squatter, err := s.userManager.User("phil@example.com")
+		require.Nil(t, err)
+		require.Nil(t, s.userManager.AddEmail(squatter.ID, "squatter@example.com"))
+		require.Nil(t, s.userManager.SetPrimaryEmail(squatter.ID, "squatter@example.com"))
+
+		// Reset by the ambiguous identifier: the verified email must win over the look-alike username
+		rr := request(t, s, "POST", "/v1/account/password/reset/request", `{"identifier":"phil@example.com"}`, nil)
+		require.Equal(t, 200, rr.Code)
+		require.NotEmpty(t, mailer.resetLinks["phil@example.com"])  // sent to the email owner (account A)
+		require.Empty(t, mailer.resetLinks["squatter@example.com"]) // NOT the username squatter (account B)
+
+		// The token resets account A (ben); the squatter's password is untouched
+		token := tokenFromLink(t, mailer.resetLinks["phil@example.com"], "https://ntfy.example.com/account/password/reset/")
+		rr = request(t, s, "POST", "/v1/account/password/reset", fmt.Sprintf(`{"token":"%s","password":"brandnew"}`, token), nil)
+		require.Equal(t, 200, rr.Code)
+		require.True(t, canLogin(t, s, "ben", "brandnew"))                  // account A was reset
+		require.True(t, canLogin(t, s, "phil@example.com", "squatterpass")) // account B unaffected
+	})
+}
+
 func TestAccount_PasswordReset_UnknownIdentifierUniform(t *testing.T) {
 	forEachBackend(t, func(t *testing.T, databaseURL string) {
 		s, mailer, _ := newEmailTestServer(t, databaseURL)

user/manager.go

@@ -515,6 +515,19 @@ func (a *Manager) UserByID(id string) (*User, error) {
 	return a.readUser(rows)
 }
 
+// UserByEmailOrUsername resolves an identifier to a single user, trying it first as a primary
+// email address and then as a username. A verified, owned email takes precedence over a
+// freely-chosen username, so a look-alike username cannot shadow the email's real owner. Returns
+// ErrUserNotFound if neither matches.
+func (a *Manager) UserByEmailOrUsername(identifier string) (*User, error) {
+	if userID, err := a.UserIDByPrimaryEmail(identifier); err == nil {
+		if u, err := a.UserByID(userID); err == nil {
+			return u, nil
+		}
+	}
+	return a.User(identifier)
+}
+
 // userByToken returns the user with the given token if it exists and is not expired, or ErrUserNotFound otherwise
 func (a *Manager) userByToken(token string) (*User, error) {
 	rows, err := a.db.Query(a.queries.selectUserByToken, token, time.Now().Unix())