Parcourir la source

fix(tgbot): require admin for privileged callbacks, not just the first switch

answerCallback wraps only its first callback switch in an isAdmin guard; the
second switch (server usage, inbound/online enumeration, database backup export,
ban logs, mass traffic reset, client creation) ran for every caller. Telegram
delivers a callback with the tapping user's id, so a non-admin who can see an
admin's inline keyboard — as when the bot runs in a group — could tap Backup and
receive the full database and config, or reset all traffic. Default-deny before
the second switch: a non-admin may only run the per-user client_* callbacks that
resolve their own data from their Telegram id.
MHSanaei il y a 1 jour
Parent
commit
18349c36ac

+ 46 - 0
internal/web/service/tgbot/tgbot_callback_auth_test.go

@@ -0,0 +1,46 @@
+package tgbot
+
+import (
+	"testing"
+
+	"github.com/mymmrac/telego"
+)
+
+// A non-admin callback must never reach a privileged handler. The second
+// callback switch runs outside the isAdmin guard, so without the default-deny
+// check a non-admin who can tap an admin's inline button (e.g. in a group) could
+// export the database backup or reset all traffic. Here the privileged handler
+// would panic on the nil bot/services of a bare Tgbot; the guard must return
+// first, so no panic occurs.
+func TestAnswerCallbackDeniesPrivilegedActionToNonAdmin(t *testing.T) {
+	defer func() {
+		if r := recover(); r != nil {
+			t.Fatalf("a non-admin callback reached a privileged handler: %v", r)
+		}
+	}()
+
+	tg := &Tgbot{}
+	for _, data := range []string{"get_backup", "reset_all_traffics_c", "add_client", "onlines", "inbounds"} {
+		q := &telego.CallbackQuery{
+			Data:    data,
+			From:    telego.User{ID: 999999},
+			Message: &telego.Message{Chat: telego.Chat{ID: 1}},
+		}
+		tg.answerCallback(q, false)
+	}
+}
+
+func TestIsClientSelfCallback(t *testing.T) {
+	allowed := []string{"client_traffic", "client_sub_links", "client_qr_links", "client_sub_links alice@x"}
+	for _, d := range allowed {
+		if !isClientSelfCallback(d) {
+			t.Errorf("%q should be a per-user client callback", d)
+		}
+	}
+	denied := []string{"get_backup", "reset_all_traffics_c", "add_client", "onlines", "get_banlogs", "get_usage"}
+	for _, d := range denied {
+		if isClientSelfCallback(d) {
+			t.Errorf("%q is an admin-only callback and must not be treated as per-user", d)
+		}
+	}
+}

+ 23 - 0
internal/web/service/tgbot/tgbot_router.go

@@ -1128,6 +1128,15 @@ func (t *Tgbot) answerCallback(callbackQuery *telego.CallbackQuery, isAdmin bool
 		}
 	}
 
+	// The callbacks below sit outside the isAdmin block above, so a non-admin who
+	// can see an admin's inline keyboard (for example when the bot runs in a
+	// group) could otherwise trigger a database backup export, a mass traffic
+	// reset or client creation. Default-deny: a non-admin may only run the
+	// per-user client_* callbacks that key off their own Telegram id.
+	if !isAdmin && !isClientSelfCallback(callbackQuery.Data) {
+		return
+	}
+
 	switch callbackQuery.Data {
 	case "get_usage":
 		t.sendCallbackAnswerTgBot(callbackQuery.ID, t.I18nBot("tgbot.buttons.serverUsage"))
@@ -1526,3 +1535,17 @@ func (t *Tgbot) answerCallback(callbackQuery *telego.CallbackQuery, isAdmin bool
 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).
+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 ")
+}