From fac90093982047443f34d8f4e181a91a9cca7f5e Mon Sep 17 00:00:00 2001 From: MHSanaei Date: Sun, 1 Nov 2026 08:58:43 +0100 Subject: [PATCH] fix(tgbot): escape template parameters once in I18nBot Every bot message is sent HTML-parsed while the intended markup lives in the translation templates, yet the values filled into them were escaped only at some call sites. Remarks and e-mails, which a node chooses for the inbounds it reports, reached the scheduled report and the client info unescaped and turned into markup, and the reports that escaped their whole text instead showed the templates' own tags. I18nBot now escapes every parameter value, the call sites drop their own escaping and the whole-report escapes, the two values concatenated outside a template are escaped where they are written, and callback answers, which are plain text, are unescaped again. --- .../web/service/tgbot/screen_client_views.go | 8 +- internal/web/service/tgbot/screen_views.go | 2 +- internal/web/service/tgbot/tgbot.go | 15 +- internal/web/service/tgbot/tgbot_client.go | 2 +- internal/web/service/tgbot/tgbot_invite.go | 4 +- internal/web/service/tgbot/tgbot_report.go | 3 +- .../tgbot/tgbot_report_batching_test.go | 174 ++---------------- internal/web/service/tgbot/tgbot_router.go | 12 +- internal/web/service/tgbot/tgbot_send.go | 4 +- internal/web/service/tgbot/tgbot_send_test.go | 36 ++++ 10 files changed, 88 insertions(+), 172 deletions(-) diff --git a/internal/web/service/tgbot/screen_client_views.go b/internal/web/service/tgbot/screen_client_views.go index 3e82b1a01..9a2136a2d 100644 --- a/internal/web/service/tgbot/screen_client_views.go +++ b/internal/web/service/tgbot/screen_client_views.go @@ -43,7 +43,7 @@ func (t *Tgbot) screenClientIps(chatID int64, email string) { if err != nil || len(ips) == 0 { body += t.I18nBot("tgbot.noIpRecord") } else { - body += t.I18nBot("tgbot.messages.ips", "IPs=="+html.EscapeString(formatIPLog(ips))) + body += t.I18nBot("tgbot.messages.ips", "IPs=="+formatIPLog(ips)) } body += t.I18nBot("tgbot.messages.refreshedOn", "Time=="+time.Now().Format("2006-01-02 15:04:05")) rows := [][]telego.InlineKeyboardButton{ @@ -158,9 +158,9 @@ func (t *Tgbot) screenClientLinks(chatID int64, email string) { return } body := t.clientScreenBody(email, "") - body += t.I18nBot("tgbot.messages.subURL", "URL=="+html.EscapeString(subURL)) + body += t.I18nBot("tgbot.messages.subURL", "URL=="+subURL) if subJSON != "" { - body += t.I18nBot("tgbot.messages.jsonURL", "URL=="+html.EscapeString(subJSON)) + body += t.I18nBot("tgbot.messages.jsonURL", "URL=="+subJSON) } body += t.I18nBot("tgbot.messages.refreshedOn", "Time=="+time.Now().Format("2006-01-02 15:04:05")) rows := [][]telego.InlineKeyboardButton{ @@ -213,7 +213,7 @@ func (t *Tgbot) screenClientQR(chatID int64, email string) { return } body := t.clientScreenBody(email, t.I18nBot("tgbot.answers.qrCodeForClient", "Email=="+email)) - body += t.I18nBot("tgbot.messages.subURL", "URL=="+html.EscapeString(subURL)) + body += t.I18nBot("tgbot.messages.subURL", "URL=="+subURL) rows := [][]telego.InlineKeyboardButton{ tu.InlineKeyboardRow(t.btn("tgbot.buttons.individualLinks", t.encodeQuery("client_individual_links "+email))), t.backRow(), diff --git a/internal/web/service/tgbot/screen_views.go b/internal/web/service/tgbot/screen_views.go index 3303ffa11..d21871b3a 100644 --- a/internal/web/service/tgbot/screen_views.go +++ b/internal/web/service/tgbot/screen_views.go @@ -235,7 +235,7 @@ func (t *Tgbot) screenReport(chatID int64) { tu.InlineKeyboardRow(t.btn("tgbot.buttons.refresh", cbReport)), t.backRow(), } - t.renderScreen(chatID, t.newScreen("reports", html.EscapeString(report.String()), rows...)) + t.renderScreen(chatID, t.newScreen("reports", report.String(), rows...)) } // screenCommands explains the classic commands; kept because muscle memory is diff --git a/internal/web/service/tgbot/tgbot.go b/internal/web/service/tgbot/tgbot.go index 374cf5034..e3065395e 100644 --- a/internal/web/service/tgbot/tgbot.go +++ b/internal/web/service/tgbot/tgbot.go @@ -5,6 +5,7 @@ import ( "crypto/rand" "embed" "fmt" + "html" "math/big" "net" "net/url" @@ -249,9 +250,19 @@ func (t *Tgbot) NewTgbot() *Tgbot { return new(Tgbot) } -// I18nBot retrieves a localized message for the bot interface. +// I18nBot retrieves a localized message for the bot interface. Every message is sent +// HTML-parsed, so param values (remarks, e-mails, errors) are escaped here. func (t *Tgbot) I18nBot(name string, params ...string) string { - return locale.I18n(locale.Bot, name, params...) + escaped := make([]string, len(params)) + for i, param := range params { + key, value, found := strings.Cut(param, "==") + if !found { + escaped[i] = param + continue + } + escaped[i] = key + "==" + html.EscapeString(value) + } + return locale.I18n(locale.Bot, name, escaped...) } // GetHashStorage returns the hash storage instance for callback queries. diff --git a/internal/web/service/tgbot/tgbot_client.go b/internal/web/service/tgbot/tgbot_client.go index 5e9868781..e11e2ce77 100644 --- a/internal/web/service/tgbot/tgbot_client.go +++ b/internal/web/service/tgbot/tgbot_client.go @@ -321,7 +321,7 @@ func (t *Tgbot) clientInfoMsg( output := "" output += t.I18nBot("tgbot.messages.email", "Email=="+traffic.Email) if attachIds, err := t.clientService.GetInboundIdsForEmail(nil, traffic.Email); err == nil && len(attachIds) > 0 { - output += fmt.Sprintf("🔗 Inbounds: %s\r\n", t.describeAttachedInbounds(attachIds)) + output += fmt.Sprintf("🔗 Inbounds: %s\r\n", html.EscapeString(t.describeAttachedInbounds(attachIds))) } if printEnabled { output += t.I18nBot("tgbot.messages.enabled", "Enable=="+enabled) diff --git a/internal/web/service/tgbot/tgbot_invite.go b/internal/web/service/tgbot/tgbot_invite.go index 35f1bf3f4..ac1ff8474 100644 --- a/internal/web/service/tgbot/tgbot_invite.go +++ b/internal/web/service/tgbot/tgbot_invite.go @@ -11,6 +11,7 @@ import ( "github.com/mhsanaei/3x-ui/v3/internal/database/model" "github.com/mhsanaei/3x-ui/v3/internal/logger" "github.com/mhsanaei/3x-ui/v3/internal/util/common" + "github.com/mhsanaei/3x-ui/v3/internal/web/locale" "github.com/mymmrac/telego" ) @@ -197,7 +198,8 @@ func (t *Tgbot) allowInviteAttempt(from *telego.User) bool { inviteAttemptsMu.Unlock() if count == inviteAttemptLimit+1 { - t.SendMsgToTgbotAdmins(t.I18nBot("tgbot.messages.inviteRateLimitedAdmin", + // locale.I18n directly: User is a ready-made mention that I18nBot would escape. + t.SendMsgToTgbotAdmins(locale.I18n(locale.Bot, "tgbot.messages.inviteRateLimitedAdmin", "User=="+tgUserMention(from), "ID=="+strconv.FormatInt(from.ID, 10), "Limit=="+strconv.Itoa(inviteAttemptLimit))) diff --git a/internal/web/service/tgbot/tgbot_report.go b/internal/web/service/tgbot/tgbot_report.go index c1da45bbc..dd7a65c3f 100644 --- a/internal/web/service/tgbot/tgbot_report.go +++ b/internal/web/service/tgbot/tgbot_report.go @@ -2,6 +2,7 @@ package tgbot import ( "fmt" + "html" "net" "os" "strconv" @@ -292,7 +293,7 @@ func (t *Tgbot) notifyExhausted() { output.WriteString(":\r\n") for _, traffic := range disabledClients { output.WriteString(" ") - output.WriteString(traffic.Email) + output.WriteString(html.EscapeString(traffic.Email)) } output.WriteString("\r\n") } diff --git a/internal/web/service/tgbot/tgbot_report_batching_test.go b/internal/web/service/tgbot/tgbot_report_batching_test.go index ba0166ba9..96bb1f03d 100644 --- a/internal/web/service/tgbot/tgbot_report_batching_test.go +++ b/internal/web/service/tgbot/tgbot_report_batching_test.go @@ -2,7 +2,6 @@ package tgbot import ( "encoding/json" - "errors" "fmt" "io" "net/http" @@ -11,18 +10,13 @@ import ( "strings" "sync" "testing" - "time" "github.com/mymmrac/telego" "github.com/nicksnyder/go-i18n/v2/i18n" - "golang.org/x/text/language" - "gorm.io/gorm" "github.com/mhsanaei/3x-ui/v3/internal/database" "github.com/mhsanaei/3x-ui/v3/internal/database/dbtest" "github.com/mhsanaei/3x-ui/v3/internal/database/model" - "github.com/mhsanaei/3x-ui/v3/internal/web/locale" - "github.com/mhsanaei/3x-ui/v3/internal/web/service" "github.com/mhsanaei/3x-ui/v3/internal/xray" ) @@ -173,157 +167,29 @@ func TestResetAllTrafficsAnswersWithNoClients(t *testing.T) { } } -// Regression test: the report leaves as one HTML-parsed message, so a remark -// holding "<" must reach Telegram escaped instead of dropping the whole page. -func TestTrafficUsageReportEscapesHtml(t *testing.T) { - mock, sent := captureReportServer(t) - swapTestBot(t, mock.URL) - defer mock.Close() - +// Every bot message is HTML-parsed, and a node chooses the remark and e-mails the master +// stores for its inbound: filled into a template unescaped, they became markup. +func TestClientInfoEscapesNodeValues(t *testing.T) { tb := initReportDB(t) - seedReportClients(t, "DE ", []string{"a@x"}) - - tb.answerCallback(&telego.CallbackQuery{ - ID: "q1", - From: telego.User{ID: 1}, - Data: "get_sorted_traffic_usage_report", - Message: &telego.Message{Chat: telego.Chat{ID: 1}}, - }, true) // admin - - got := sent() - if len(got) != 1 { - t.Fatalf("messages = %d, want 1", len(got)) + draftLocalizer(t, &i18n.Message{ID: "tgbot.messages.email", Other: "📧 Email: {{ .Email }}\r\n"}) + remark := `Panel session expired` + email := "zz" + seedReportClients(t, remark, []string{email}) + var traffic xray.ClientTraffic + if err := database.GetDB().Where("email = ?", email).First(&traffic).Error; err != nil { + t.Fatalf("read traffic: %v", err) } - if strings.Contains(got[0].body(), "") { - t.Errorf("report text = %q, want the remark escaped", got[0].body()) - } -} -func TestNotifyExhaustedLooksUpEachIDOnce(t *testing.T) { - for _, outcome := range []string{"empty", "failure", "exhausted", "expiry", "admin", "zero", "disabled-inbound"} { - t.Run(outcome, func(t *testing.T) { - tb := initReportDB(t) - clientInfoLocalizer(t) - mock, sent := captureReportServer(t) - swapTestBot(t, mock.URL) - t.Cleanup(mock.Close) - clients := []model.Client{ - {Email: "notify-a", ID: "uuid-a", TgID: 6886, Enable: true}, - {Email: "notify-b", ID: "uuid-b", TgID: 6886, Enable: true}, - } - if outcome == "zero" { - for i := range clients { - clients[i].TgID = 0 - } - } - if outcome == "admin" { - tgBotMutex.Lock() - previous := adminIds - adminIds = append(append([]int64(nil), adminIds...), 6886) - tgBotMutex.Unlock() - t.Cleanup(func() { - tgBotMutex.Lock() - adminIds = previous - tgBotMutex.Unlock() - }) - } - db := database.GetDB() - for i := range 2 { - inbound := &model.Inbound{UserId: 1, Tag: fmt.Sprintf("notify-%d", i), Port: 41890 + i, Enable: true, Protocol: model.VLESS, Settings: `{"clients":[{"email":"notify-a","id":"uuid-a","enable":true,"tgId":6886},{"email":"notify-b","id":"uuid-b","enable":true,"tgId":6886}]}`} - if outcome == "disabled-inbound" { - inbound.Enable = false - } - if err := db.Create(inbound).Error; err != nil { - t.Fatal(err) - } - if err := (&service.ClientService{}).SyncInbound(nil, inbound.Id, clients); err != nil { - t.Fatal(err) - } - // Keep the inbound eligible even when the Telegram account has no traffic. - traffic := &xray.ClientTraffic{Email: fmt.Sprintf("unrelated-%d", i), InboundId: inbound.Id} - if outcome == "exhausted" || outcome == "expiry" { - traffic.Email = clients[i].Email - traffic.Enable = true - traffic.Total = 1 - traffic.Up = 2 - if outcome == "expiry" { - traffic.Total = 0 - traffic.ExpiryTime = time.Now().Add(-time.Hour).UnixMilli() - } - } - if err := db.Create(traffic).Error; err != nil { - t.Fatal(err) - } - } - lookups := 0 - cb := "test:notify_lookup" - if err := db.Callback().Query().Before("gorm:query").Register(cb, func(tx *gorm.DB) { - if tx.Statement.Table == "clients" && strings.Contains(fmt.Sprint(tx.Statement.Clauses["WHERE"]), "tg_id") { - lookups++ - if outcome == "failure" { - tx.AddError(errors.New("injected Telegram lookup failure")) - } - } - }); err != nil { - t.Fatal(err) - } - t.Cleanup(func() { _ = db.Callback().Query().Remove(cb) }) - tb.notifyExhausted() - wantLookups := 1 - if outcome == "admin" || outcome == "zero" || outcome == "disabled-inbound" { - wantLookups = 0 - } - if lookups != wantLookups { - t.Fatalf("lookups = %d, want %d after %s", lookups, wantLookups, outcome) - } - got := sent() - if outcome == "exhausted" || outcome == "expiry" { - if len(got) != 1 || !strings.Contains(got[0].Text, "notify-a") || !strings.Contains(got[0].Text, "notify-b") { - t.Fatalf("want one notification containing both clients, got %+v", got) - } - } else if len(got) != 0 { - t.Fatalf("unexpected notifications: %+v", got) - } - }) - } -} + out := tb.clientInfoMsg(&traffic, true, false, false, true, true, false) -func TestServerUsageInfoShowsUnavailableUptime(t *testing.T) { - bundle := i18n.NewBundle(language.English) - _ = bundle.AddMessages(language.English, &i18n.Message{ID: "tgbot.messages.serverUpTime", Other: "Uptime: {{.UpTime}} {{.Unit}}\n"}, &i18n.Message{ID: "tgbot.days", Other: "days"}) - oldLocale := locale.SwapBotLocalizer(i18n.NewLocalizer(bundle, "en")) - t.Cleanup(func() { locale.SwapBotLocalizer(oldLocale) }) - statusCache.mutex.Lock() - oldStatus, oldTime := statusCache.data, statusCache.timestamp - statusCache.mutex.Unlock() - serverStatsCache.mutex.Lock() - oldStats, oldStatsTime := serverStatsCache.data, serverStatsCache.timestamp - serverStatsCache.mutex.Unlock() - t.Cleanup(func() { - statusCache.mutex.Lock() - statusCache.data, statusCache.timestamp = oldStatus, oldTime - statusCache.mutex.Unlock() - serverStatsCache.mutex.Lock() - serverStatsCache.data, serverStatsCache.timestamp = oldStats, oldStatsTime - serverStatsCache.mutex.Unlock() - }) - for _, unavailable := range []bool{true, false} { - t.Run(map[bool]string{true: "unavailable", false: "valid zero"}[unavailable], func(t *testing.T) { - bot := &Tgbot{} - status := &service.Status{Loads: []float64{0, 0, 0}} - if unavailable { - status.UnavailableMetrics = []string{"uptime"} - } - bot.setCachedStatus(status) - bot.setCachedServerStats("") - info := bot.prepareServerUsageInfo() - want := "Uptime: 0 days" - if unavailable { - want = "Uptime: —" - } - if !strings.Contains(info, want) { - t.Fatalf("report did not contain %q: %s", want, info) - } - }) + for _, raw := range []string{"zz"} { + if strings.Contains(out, raw) { + t.Errorf("client info = %q, want %q escaped", out, raw) + } + } + for _, want := range []string{"📧 Email: <b>zz</b>", "<a href="} { + if !strings.Contains(out, want) { + t.Errorf("client info = %q, want it to contain %q", out, want) + } } } diff --git a/internal/web/service/tgbot/tgbot_router.go b/internal/web/service/tgbot/tgbot_router.go index 107fdb896..e883401bb 100644 --- a/internal/web/service/tgbot/tgbot_router.go +++ b/internal/web/service/tgbot/tgbot_router.go @@ -2,7 +2,6 @@ package tgbot import ( "context" - "html" "slices" "strconv" "strings" @@ -1030,17 +1029,17 @@ func (t *Tgbot) answerCallback(callbackQuery *telego.CallbackQuery, isAdmin bool t.openOwnClientScreen(chatId, callbackQuery.From.ID, callbackQuery.Data) case "add_client_ch_default_email": userStateMgr.set(actor, "awaiting_email") - t.wizardPrompt(chatId, draft, t.I18nBot("tgbot.messages.email_prompt", "ClientEmail=="+html.EscapeString(draft.email))) + t.wizardPrompt(chatId, draft, t.I18nBot("tgbot.messages.email_prompt", "ClientEmail=="+draft.email)) case "add_client_ch_default_comment": userStateMgr.set(actor, "awaiting_comment") - t.wizardPrompt(chatId, draft, t.I18nBot("tgbot.messages.comment_prompt", "ClientComment=="+html.EscapeString(draft.comment))) + t.wizardPrompt(chatId, draft, t.I18nBot("tgbot.messages.comment_prompt", "ClientComment=="+draft.comment)) case "add_client_ch_default_tg_id": userStateMgr.set(actor, "awaiting_tg_id") current := draft.tgID if current == "" { current = "—" } - t.wizardPrompt(chatId, draft, t.I18nBot("tgbot.messages.tgid_prompt", "ClientTGID=="+html.EscapeString(current))) + t.wizardPrompt(chatId, draft, t.I18nBot("tgbot.messages.tgid_prompt", "ClientTGID=="+current)) case "add_client_ch_default_traffic": inlineKeyboard := tu.InlineKeyboard( tu.InlineKeyboardRow( @@ -1192,9 +1191,8 @@ func (t *Tgbot) answerCallback(callbackQuery *telego.CallbackQuery, isAdmin bool } report.WriteString(t.I18nBot("tgbot.messages.FinishProcess")) - // Escaped whole (a stray "<" makes Telegram reject a page) and paged: - // past ~80 clients the report exceeds one message. - t.sendPagedNotice(chatId, html.EscapeString(report.String())) + // Paged: past ~80 clients the report exceeds one message. + t.sendPagedNotice(chatId, report.String()) // The confirmation has served its purpose: leaving it on screen left the // only button being the one just pressed. t.screenHome(chatId, callbackQuery.From.ID) diff --git a/internal/web/service/tgbot/tgbot_send.go b/internal/web/service/tgbot/tgbot_send.go index e134f1c18..8db98c51d 100644 --- a/internal/web/service/tgbot/tgbot_send.go +++ b/internal/web/service/tgbot/tgbot_send.go @@ -3,6 +3,7 @@ package tgbot import ( "context" "fmt" + "html" "strings" "time" @@ -149,9 +150,10 @@ func (t *Tgbot) SendMsgToTgbotAdmins(msg string, replyMarkup ...telego.ReplyMark // sendCallbackAnswerTgBot answers a callback query with a message. func (t *Tgbot) sendCallbackAnswerTgBot(id string, message string) { + // Callback answers are plain text, so the HTML entities I18nBot adds are undone. params := telego.AnswerCallbackQueryParams{ CallbackQueryID: id, - Text: message, + Text: html.UnescapeString(message), } if err := liveBot().AnswerCallbackQuery(context.Background(), ¶ms); err != nil { logger.Warning(err) diff --git a/internal/web/service/tgbot/tgbot_send_test.go b/internal/web/service/tgbot/tgbot_send_test.go index 958ffccf6..b86391725 100644 --- a/internal/web/service/tgbot/tgbot_send_test.go +++ b/internal/web/service/tgbot/tgbot_send_test.go @@ -6,12 +6,14 @@ import ( "net/http" "net/http/httptest" "strings" + "sync" "testing" "github.com/mhsanaei/3x-ui/v3/internal/logger" "github.com/mymmrac/telego" tu "github.com/mymmrac/telego/telegoutil" + "github.com/nicksnyder/go-i18n/v2/i18n" ) func TestIsTelegramNotModifiedError(t *testing.T) { @@ -136,3 +138,37 @@ func TestPageMessageSplitsLinkListWithoutBlankLines(t *testing.T) { t.Errorf("pages contain %d links, want 50", links) } } + +// Regression test: callback answers are shown as plain text, so the entities +// I18nBot escapes for HTML-parsed messages must not surface in them. +func TestCallbackAnswerIsPlainText(t *testing.T) { + var mu sync.Mutex + var texts []string + mock := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path != "/bot"+testBotToken+"/answerCallbackQuery" { + w.WriteHeader(http.StatusNotFound) + return + } + var body struct { + Text string `json:"text"` + } + _ = json.NewDecoder(r.Body).Decode(&body) + mu.Lock() + texts = append(texts, body.Text) + mu.Unlock() + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(map[string]any{"ok": true, "result": true}) + })) + defer mock.Close() + swapTestBot(t, mock.URL) + draftLocalizer(t, &i18n.Message{ID: "tgbot.answers.canceled", Other: "❌ {{ .Email }}: Operation canceled."}) + + tb := &Tgbot{} + tb.sendCallbackAnswerTgBot("q1", tb.I18nBot("tgbot.answers.canceled", "Email==a&b")) + + mu.Lock() + defer mu.Unlock() + if len(texts) != 1 || texts[0] != "❌ a&b: Operation canceled." { + t.Errorf("answerCallbackQuery texts = %q, want the e-mail shown verbatim", texts) + } +}