From 18d0ee174de159661f40d895d9079f6f0b1492f7 Mon Sep 17 00:00:00 2001 From: Basavaraj S m Date: Sun, 18 Oct 2026 15:50:00 +0530 Subject: [PATCH] fix(mailer): reject incoming email tokens of users who cannot sign in (#39833) Incoming email reply tokens are embedded in every notification email and stay valid for a year. `DecodeToken` verifies them with `user.Rands`, which is not rotated when an admin disables sign-in or the account is deactivated, and `ReplyHandler` only checks repo read permission. A suspended user could therefore keep posting comments, review comments and attachments on any public repo by replying to old notifications, while the web, API, git and package paths all refuse them. `DecodeToken` now rejects tokens of users who are inactive or prohibited from signing in, which covers every incoming mail handler. --------- Co-authored-by: wxiaoguang --- models/user/user.go | 30 +++++++++++++++++++++++-- services/mailer/token/token.go | 2 +- services/mailer/token/token_test.go | 34 +++++++++++++++++++++++++++++ 3 files changed, 63 insertions(+), 3 deletions(-) create mode 100644 services/mailer/token/token_test.go diff --git a/models/user/user.go b/models/user/user.go index 869b31d5d28..c839e41d170 100644 --- a/models/user/user.go +++ b/models/user/user.go @@ -1021,7 +1021,8 @@ func GetInactiveUsers(ctx context.Context, olderThan time.Duration) ([]*User, er Find(&users) } -// GetUserByID returns the user object by given ID if exists. +// GetUserByID returns the user object by given ID if exists. TODO: for most cases, this function should be avoided. +// Use correct GetXxxUser functions instead to handle user types & states correctly. func GetUserByID(ctx context.Context, id int64) (*User, error) { u := new(User) has, err := db.GetEngine(ctx).ID(id).Get(u) @@ -1033,7 +1034,8 @@ func GetUserByID(ctx context.Context, id int64) (*User, error) { return u, nil } -// GetUserByIDs returns the user objects by given IDs if exists. +// GetUserByIDs returns the user objects by given IDs if exists. TODO: for most cases, this function should be avoided. +// Use correct GetXxxUser functions instead to handle user types & states correctly. func GetUserByIDs(ctx context.Context, ids []int64) ([]*User, error) { if len(ids) == 0 { return nil, nil @@ -1046,6 +1048,13 @@ func GetUserByIDs(ctx context.Context, ids []int64) ([]*User, error) { return users, err } +// Glossaries for XxxUser: +// * Possible: The user may or may not exist in the database, but we can still return a valid user object (e.g. Ghost user) for the given ID. +// * Individual: The user is a real person, not an organization or a bot. +// * Doer: The user is the one performing an action +// * Valid: The user is an active user and not prohibited from signing in +// * System: Negative ID (e.g. Ghost, Actions, DeployKey, etc.) and not a real person, Ghost is not included if there is no "Possible" keyword + // GetPossibleUserByID returns the possible user and its ID. If the user doesn't exist, it returns Ghost user func GetPossibleUserByID(ctx context.Context, id int64) (_ int64, u *User, err error) { if id < 0 { @@ -1082,6 +1091,23 @@ func GetPossibleUserByIDs(ctx context.Context, ids []int64) ([]*User, error) { return users, nil } +func GetValidDoerUserByID(ctx context.Context, id int64) (*User, error) { + u, err := GetUserByID(ctx, id) + if err != nil { + return nil, err + } + if !u.IsIndividual() && !u.IsTypeBot() { + return nil, util.NewNotExistErrorf("user %d is not a doer", id) + } + if !u.IsActive { + return nil, util.NewPermissionDeniedErrorf("user %d is inactive", id) + } + if u.ProhibitLogin { + return nil, util.NewPermissionDeniedErrorf("user %d is prohibited from login", id) + } + return u, nil +} + func getUserByNameWithTypes(ctx context.Context, name string, types ...UserType) (*User, error) { u := &User{} sess := db.GetEngine(ctx).Where(builder.Eq{"lower_name": strings.ToLower(name)}) diff --git a/services/mailer/token/token.go b/services/mailer/token/token.go index de9a60dd4c7..58c3fab9818 100644 --- a/services/mailer/token/token.go +++ b/services/mailer/token/token.go @@ -98,7 +98,7 @@ func DecodeToken(ctx context.Context, token string) (HandlerType, *user_model.Us return UnknownHandlerType, nil, nil, err } - user, err := user_model.GetUserByID(ctx, userID) + user, err := user_model.GetValidDoerUserByID(ctx, userID) if err != nil { return UnknownHandlerType, nil, nil, err } diff --git a/services/mailer/token/token_test.go b/services/mailer/token/token_test.go new file mode 100644 index 00000000000..940357c495a --- /dev/null +++ b/services/mailer/token/token_test.go @@ -0,0 +1,34 @@ +// Copyright 2026 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package token + +import ( + "testing" + + "gitea.dev/models/unittest" + user_model "gitea.dev/models/user" + + _ "gitea.dev/models" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestMain(m *testing.M) { + unittest.MainTest(m) +} + +func TestDecodeTokenRejectsUserWhoCannotSignIn(t *testing.T) { + require.NoError(t, unittest.PrepareTestDatabase()) + for id, expectedErr := range map[int64]string{2: "", 3: "not a doer", 9: "inactive", 37: "prohibited"} { + token, err := CreateToken(ReplyHandlerType, unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: id}), []byte{1}) + require.NoError(t, err) + _, _, _, err = DecodeToken(t.Context(), token) + if expectedErr == "" { + assert.NoError(t, err) + } else { + assert.ErrorContains(t, err, expectedErr) + } + } +}