diff --git a/internal/web/service/tgbot/tgbot_client.go b/internal/web/service/tgbot/tgbot_client.go index ca9f48351..243ce08ad 100644 --- a/internal/web/service/tgbot/tgbot_client.go +++ b/internal/web/service/tgbot/tgbot_client.go @@ -537,6 +537,21 @@ func (t *Tgbot) clientInfoMsg( return output } +// clientOwnedByTgUser reports whether email belongs to a client bound to this +// Telegram account, the same list the self-service usage command reads. +func (t *Tgbot) clientOwnedByTgUser(tgUserID int64, email string) bool { + traffics, err := t.inboundService.GetClientTrafficTgBot(tgUserID) + if err != nil { + return false + } + for _, traffic := range traffics { + if traffic.Email == email { + return true + } + } + return false +} + // getClientUsage retrieves and sends client usage information to the chat. func (t *Tgbot) getClientUsage(chatId int64, tgUserID int64, email ...string) { traffics, err := t.inboundService.GetClientTrafficTgBot(tgUserID) diff --git a/internal/web/service/tgbot/tgbot_client_links_authz_test.go b/internal/web/service/tgbot/tgbot_client_links_authz_test.go new file mode 100644 index 000000000..e8549d0b0 --- /dev/null +++ b/internal/web/service/tgbot/tgbot_client_links_authz_test.go @@ -0,0 +1,125 @@ +package tgbot + +import ( + "path/filepath" + "testing" + "time" + + "github.com/mhsanaei/3x-ui/v3/internal/database" + "github.com/mhsanaei/3x-ui/v3/internal/database/model" + "github.com/mhsanaei/3x-ui/v3/internal/web/global" + "github.com/mhsanaei/3x-ui/v3/internal/xray" + + "github.com/mymmrac/telego" +) + +const ( + ownerTgID = int64(4242) + ownerMail = "owner@x" +) + +// newLinksCallbackTgbot seeds one inbound whose settings bind email to +// ownerTgID, the traffic row the ownership lookup joins on, and a mocked API. +func newLinksCallbackTgbot(t *testing.T, email string) (*Tgbot, func(string) int) { + t.Helper() + mock, calls := staleButtonServer(t, map[string]any{ + "answerCallbackQuery": map[string]any{"ok": true, "result": true}, + "sendMessage": map[string]any{"ok": true, "result": map[string]any{ + "message_id": 1, + "date": 0, + "chat": map[string]any{"id": ownerTgID, "type": "private"}, + }}, + }) + swapTestBot(t, mock.URL) + t.Cleanup(mock.Close) + + if err := database.InitDB(filepath.Join(t.TempDir(), "x-ui.db")); err != nil { + t.Fatalf("InitDB: %v", err) + } + t.Cleanup(func() { _ = database.CloseDB() }) + + inbound := &model.Inbound{ + UserId: 1, + Remark: "in", + Port: 443, + Protocol: model.VLESS, + Enable: true, + Settings: `{"clients":[{"email":"` + email + `","tgId":4242,"subId":"sub-owned"}]}`, + } + if err := database.GetDB().Create(inbound).Error; err != nil { + t.Fatalf("seed inbound: %v", err) + } + if err := database.GetDB().Create(&xray.ClientTraffic{ + InboundId: inbound.Id, + Email: email, + Enable: true, + }).Error; err != nil { + t.Fatalf("seed traffic: %v", err) + } + + origRunning := isRunning + t.Cleanup(func() { isRunning = origRunning }) + isRunning = true + + return &Tgbot{}, calls +} + +func tapClientLinks(t *testing.T, tb *Tgbot, tgUserID int64, data string) { + t.Helper() + tb.answerCallback(&telego.CallbackQuery{ + ID: "q1", + From: telego.User{ID: tgUserID}, + Data: data, + Message: &telego.Message{Chat: telego.Chat{ID: tgUserID}}, + }, false) +} + +// Regression test: a non-admin tapping a link callback carrying another +// client's email must be refused; without the ownership check it is served. +func TestClientLinkCallbackRefusesForeignClient(t *testing.T) { + tb, calls := newLinksCallbackTgbot(t, ownerMail) + + tapClientLinks(t, tb, ownerTgID, "client_sub_links someone-else@x") + + if n := calls("sendMessage"); n != 0 { + t.Errorf("sendMessage calls = %d, want 0: a non-admin received a foreign client's links", n) + } + if n := calls("answerCallbackQuery"); n != 1 { + t.Errorf("answerCallbackQuery calls = %d, want 1: the refused tap must be answered", n) + } +} + +// The same guard must not lock the owner out of their own links. +func TestClientLinkCallbackServesOwnClient(t *testing.T) { + tb, calls := newLinksCallbackTgbot(t, ownerMail) + + tapClientLinks(t, tb, ownerTgID, "client_sub_links "+ownerMail) + + if n := calls("sendMessage"); n != 1 { + t.Errorf("sendMessage calls = %d, want 1: the owner must still get its links", n) + } + if n := calls("answerCallbackQuery"); n != 0 { + t.Errorf("answerCallbackQuery calls = %d, want 0: an allowed tap is not refused", n) + } +} + +// Regression test: a payload past 64 chars arrives as its hash, so an email long +// enough to be hashed must still be decoded and served to its owner. +func TestHashedLinkCallbackServesOwnClient(t *testing.T) { + const longMail = "very-long-owner-address-for-hashed-buttons@example.com" + tb, calls := newLinksCallbackTgbot(t, longMail) + + origStorage := hashStorage + hashStorage = global.NewHashStorage(20 * time.Minute) + t.Cleanup(func() { hashStorage = origStorage }) + + data := tb.encodeQuery("client_sub_links " + longMail) + if data == "client_sub_links "+longMail { + t.Fatalf("encodeQuery left %q unhashed; the test needs a hashed payload", data) + } + tapClientLinks(t, tb, ownerTgID, data) + + if n := calls("sendMessage"); n != 1 { + t.Errorf("sendMessage calls = %d, want 1: the owner's hashed button must still be served", n) + } +} diff --git a/internal/web/service/tgbot/tgbot_router.go b/internal/web/service/tgbot/tgbot_router.go index 3d074ca53..161e8ea61 100644 --- a/internal/web/service/tgbot/tgbot_router.go +++ b/internal/web/service/tgbot/tgbot_router.go @@ -922,8 +922,15 @@ func (t *Tgbot) answerCallback(callbackQuery *telego.CallbackQuery, isAdmin bool } } - if !isAdmin && !isClientSelfCallback(callbackQuery.Data) { - return + if !isAdmin { + // encodeQuery hashes any payload past 64 chars, so a long email's button + // must be decoded before the gate can see which client it names. + if decoded, err := t.decodeQuery(callbackQuery.Data); err == nil { + callbackQuery.Data = decoded + } + if !isClientSelfCallback(callbackQuery.Data) { + return + } } switch callbackQuery.Data { @@ -1302,20 +1309,23 @@ func (t *Tgbot) answerCallback(callbackQuery *telego.CallbackQuery, isAdmin bool } default: - if after, ok := strings.CutPrefix(callbackQuery.Data, "client_sub_links "); ok { - email := after + action, email, ok := splitClientLinkCallback(callbackQuery.Data) + if !ok { + return + } + // The keyboard outlives the chat it was sent to, so the email in it + // cannot authorise itself: a non-admin only reaches their own clients. + if !isAdmin && !t.clientOwnedByTgUser(callbackQuery.From.ID, email) { + t.sendCallbackAnswerTgBot(callbackQuery.ID, t.I18nBot("tgbot.answers.errorOperation")) + return + } + switch action { + case "client_sub_links": t.sendClientSubLinks(chatId, email) - return - } - if after, ok := strings.CutPrefix(callbackQuery.Data, "client_individual_links "); ok { - email := after + case "client_individual_links": t.sendClientIndividualLinks(chatId, email) - return - } - if after, ok := strings.CutPrefix(callbackQuery.Data, "client_qr_links "); ok { - email := after + case "client_qr_links": t.sendClientQRLinks(chatId, email) - return } } } @@ -1325,16 +1335,25 @@ func checkAdmin(tgId int64) bool { return slices.Contains(adminIds, tgId) } -// isClientSelfCallback reports whether a callback is one of the per-user client -// actions that resolve their own data from the caller's Telegram id, and so are -// safe to run for a non-admin. Every other callback is admin-only (default-deny). +// isClientSelfCallback reports whether a callback is per-user rather than +// admin-only; the caller still has to prove the client is its own. func isClientSelfCallback(data string) bool { switch data { case "client_traffic", "client_commands", "client_sub_links", "client_individual_links", "client_qr_links": return true } - return strings.HasPrefix(data, "client_sub_links ") || - strings.HasPrefix(data, "client_individual_links ") || - strings.HasPrefix(data, "client_qr_links ") + _, _, ok := splitClientLinkCallback(data) + return ok +} + +// splitClientLinkCallback splits " " for the per-client link +// callbacks; ok is false for every other data. +func splitClientLinkCallback(data string) (action, email string, ok bool) { + for _, candidate := range []string{"client_sub_links", "client_individual_links", "client_qr_links"} { + if rest, found := strings.CutPrefix(data, candidate+" "); found && rest != "" { + return candidate, rest, true + } + } + return "", "", false }