From 99bc80327140d680fcad0ceb3cd95a29e1f5c320 Mon Sep 17 00:00:00 2001 From: binwiederhier Date: Tue, 16 Jun 2026 21:18:58 -0400 Subject: [PATCH] Code review --- server/errors.go | 2 +- server/server_account.go | 23 ++++++++++----------- server/server_account_email_test.go | 31 +++++++++++++++++++++++++++++ user/manager.go | 13 ++++++++++++ 4 files changed, 56 insertions(+), 13 deletions(-) diff --git a/server/errors.go b/server/errors.go index caf1abc0..0b0f790d 100644 --- a/server/errors.go +++ b/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 diff --git a/server/server_account.go b/server/server_account.go index 517fb938..c1556cf1 100644 --- a/server/server_account.go +++ b/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): diff --git a/server/server_account_email_test.go b/server/server_account_email_test.go index e5168cfe..9bbcccd5 100644 --- a/server/server_account_email_test.go +++ b/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) diff --git a/user/manager.go b/user/manager.go index 40b61647..1c20367c 100644 --- a/user/manager.go +++ b/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())