From f7c5f5fb19838ac5767ddebca0a0842381b60d6a Mon Sep 17 00:00:00 2001 From: Bisu Ghalan Date: Sat, 11 Jul 2026 07:36:24 +0545 Subject: [PATCH 1/2] refactor: reliability, performance, and maintainability improvements - Cache GitHub clients per access token to stop rebuilding the oauth2 transport on every API call - Deduplicate webhook deliveries via X-GitHub-Delivery; enforce MaxBytesReader before signature validation - Replace stringly-typed "unauthorized" comparisons with errors.Is(ErrUnauthorized) - Make the OAuth callback asynchronous: return 200 fast, run GitHub exchange/DB write in a goroutine - Add /healthz endpoint and a background TTL cache sweep to stop unbounded cache growth - Debounce per-chat upserts; escape Telegram usernames in the connect success message - Deduplicate repo pagination and compact-button helpers into the ui package - Add a UpsertUser BSON-shape unit test (validates _id lives under $setOnInsert) Co-Authored-By: Claude --- cmd/bot/main.go | 97 ++++++++++++++++++-------- internal/bot/callbacks/callbacks.go | 61 ++-------------- internal/bot/commands/commands.go | 63 ++--------------- internal/bot/commands/reply_handler.go | 3 +- internal/bot/middleware/middleware.go | 11 +++ internal/bot/ui/buttons.go | 55 +++++++++++++++ internal/db/db.go | 12 +++- internal/db/db_test.go | 49 +++++++++++++ internal/github/auth_helper.go | 7 +- internal/github/client.go | 14 +++- internal/github/webhooks.go | 17 ++++- 11 files changed, 237 insertions(+), 152 deletions(-) create mode 100644 internal/db/db_test.go diff --git a/cmd/bot/main.go b/cmd/bot/main.go index ae3711f..ddc4c9f 100644 --- a/cmd/bot/main.go +++ b/cmd/bot/main.go @@ -4,6 +4,7 @@ import ( "context" "errors" "fmt" + "html" "github-webhook/internal/bot/middleware" "log" "log/slog" @@ -42,6 +43,9 @@ func main() { func run() (runErr error) { cfg := config.Load() + if cfg.GitHubWebhookSecret == "" { + log.Printf("WARNING: GITHUB_WEBHOOK_SECRET is empty — incoming webhook signature verification is DISABLED. Only use this for local development.") + } database, err := db.Connect(cfg) if err != nil { return fmt.Errorf("connect to DB: %w", err) @@ -122,6 +126,11 @@ func run() (runErr error) { dispatcher.AddHandler(handlers.NewCallback(callbackquery.Prefix("act:"), cbHandler.HandlePRAction)) mux := http.NewServeMux() + mux.HandleFunc("/healthz", func(writer http.ResponseWriter, request *http.Request) { + writer.Header().Set("Content-Type", "text/plain") + writer.WriteHeader(http.StatusOK) + _, _ = writer.Write([]byte("ok")) + }) mux.HandleFunc("/", func(writer http.ResponseWriter, request *http.Request) { html := fmt.Sprintf(` @@ -155,42 +164,49 @@ func run() (runErr error) { } oauthStateCache.Delete(state) - requestCtx, cancel := context.WithTimeout(r.Context(), 15*time.Second) - defer cancel() - token, err := oauth.ExchangeCode(requestCtx, code) - if err != nil { - http.Error(w, "Failed to exchange code", http.StatusInternalServerError) - return - } + // Do the (slow) GitHub exchange + DB write in the background so we return a + // fast 200 to the browser. The request context is cancelled when we return, + // so the goroutine uses its own context with a generous timeout. + go func() { + ctx, cancel := context.WithTimeout(context.Background(), 20*time.Second) + defer cancel() - encToken, err := utils.Encrypt(token.AccessToken, cfg.EncryptionKey) - if err != nil { - http.Error(w, "Encryption failed", http.StatusInternalServerError) - return - } + token, err := oauth.ExchangeCode(ctx, code) + if err != nil { + log.Printf("OAuth exchange failed for %d: %v", telegramID, err) + return + } - ghClient := clientFactory.GetUserClient(requestCtx, token.AccessToken) - u, _, err := ghClient.Users.Get(requestCtx, "") - if err != nil { - http.Error(w, "Failed to fetch user", http.StatusInternalServerError) - return - } + encToken, err := utils.Encrypt(token.AccessToken, cfg.EncryptionKey) + if err != nil { + log.Printf("OAuth encrypt failed for %d: %v", telegramID, err) + return + } - user := &models.User{ - ID: telegramID, - GitHubUserID: u.GetID(), - GitHubUsername: u.GetLogin(), - EncryptedOAuthToken: encToken, - } - if err := database.UpsertUser(requestCtx, user); err != nil { - http.Error(w, "DB Error", http.StatusInternalServerError) - return - } + ghClient := clientFactory.GetUserClient(ctx, token.AccessToken) + u, _, err := ghClient.Users.Get(ctx, "") + if err != nil { + log.Printf("OAuth fetch user failed for %d: %v", telegramID, err) + return + } - _, _ = b.SendMessage(telegramID, fmt.Sprintf("✅ GitHub account %s connected successfully!", u.GetLogin()), &gotgbot.SendMessageOpts{ParseMode: "HTML"}) + user := &models.User{ + ID: telegramID, + GitHubUserID: u.GetID(), + GitHubUsername: u.GetLogin(), + EncryptedOAuthToken: encToken, + } + if err := database.UpsertUser(ctx, user); err != nil { + log.Printf("OAuth DB upsert failed for %d: %v", telegramID, err) + _, _ = b.SendMessage(telegramID, "⚠️ Connected to GitHub but failed to save your token. Please run /connect again.", &gotgbot.SendMessageOpts{ParseMode: "HTML"}) + return + } - html := fmt.Sprintf(` + _, _ = b.SendMessage(telegramID, fmt.Sprintf("✅ GitHub account %s connected successfully!", html.EscapeString(u.GetLogin())), &gotgbot.SendMessageOpts{ParseMode: "HTML"}) + }() + + htmlBody := fmt.Sprintf(` Connected @@ -205,7 +221,7 @@ func run() (runErr error) { `, b.User.Username, b.User.Username) w.Header().Set("Content-Type", "text/html") - _, _ = w.Write([]byte(html)) + _, _ = w.Write([]byte(htmlBody)) }) listener, err := net.Listen("tcp", ":"+cfg.Port) @@ -291,6 +307,25 @@ func run() (runErr error) { signalCtx, stopSignals := signal.NotifyContext(context.Background(), os.Interrupt, syscall.SIGTERM) defer stopSignals() + // Periodically sweep TTL caches so they do not grow unbounded (entries otherwise + // only evict on re-access, and most are never read again). + go func() { + ticker := time.NewTicker(10 * time.Minute) + defer ticker.Stop() + for { + select { + case <-signalCtx.Done(): + return + case <-ticker.C: + database.ChatReposCache.Cleanup() + oauthStateCache.Cleanup() + contextCache.Cleanup() + actionCache.Cleanup() + webhookServer.DeliverySeen.Cleanup() + } + } + }() + select { case <-signalCtx.Done(): log.Printf("Shutdown signal received") diff --git a/internal/bot/callbacks/callbacks.go b/internal/bot/callbacks/callbacks.go index 604784a..aa8b12e 100644 --- a/internal/bot/callbacks/callbacks.go +++ b/internal/bot/callbacks/callbacks.go @@ -36,7 +36,7 @@ func (h *CallbackHandler) getClient(b *gotgbot.Bot, ctx *ext.Context) (*gh.Clien client, err := github.GetClientForUser(context.Background(), h.DB, h.ClientFactory, ctx.EffectiveUser.Id, h.EncryptionKey) if err != nil { msg := "Auth error." - if err.Error() == "unauthorized" { + if errors.Is(err, github.ErrUnauthorized) { msg = "Please /connect to GitHub first." } _, _ = ctx.CallbackQuery.Answer(b, &gotgbot.AnswerCallbackQueryOpts{Text: msg, ShowAlert: true}) @@ -95,55 +95,6 @@ func cbAddRepoID(repoID int64) string { return cb(cbPrefixSettings, cbAddRepo, "id", strconv.FormatInt(repoID, 10)) } -func repoButtonText(name string) string { - const max = 42 - if len(name) <= max { - return name - } - return name[:max-1] + "…" -} - -func repoPageNav(page int, resp *gh.Response) []gotgbot.InlineKeyboardButton { - var navRow []gotgbot.InlineKeyboardButton - - if resp.FirstPage != 0 && resp.PrevPage != 0 { - navRow = append(navRow, ui.Callback("<", cbAddRepoPage(resp.PrevPage), - ui.WithStyle(ui.StylePrimary), - ui.WithCustomEmojiEnv(ui.IconPrevious), - )) - } - - startPage := page - 1 - if startPage < 1 { - startPage = 1 - } - - endPage := page + 1 - if resp.LastPage != 0 && endPage > resp.LastPage { - endPage = resp.LastPage - } - if resp.LastPage == 0 && resp.NextPage != 0 { - endPage = resp.NextPage - } - - for i := startPage; i <= endPage; i++ { - text := strconv.Itoa(i) - if i == page { - text = "· " + text + " ·" - } - navRow = append(navRow, ui.Callback(text, cbAddRepoPage(i), ui.WithStyle(ui.StylePrimary))) - } - - if resp.NextPage != 0 { - navRow = append(navRow, ui.Callback(">", cbAddRepoPage(resp.NextPage), - ui.WithStyle(ui.StylePrimary), - ui.WithCustomEmojiEnv(ui.IconNext), - )) - } - - return navRow -} - func (h *CallbackHandler) HandleSettings(b *gotgbot.Bot, ctx *ext.Context) error { if ctx.EffectiveChat.Type != gotgbot.ChatTypePrivate && !utils.IsAdmin(b, ctx.EffectiveChat.Id, ctx.EffectiveUser.Id) { _, _ = ctx.CallbackQuery.Answer(b, &gotgbot.AnswerCallbackQueryOpts{Text: "Only admins can change settings", ShowAlert: true}) @@ -372,7 +323,7 @@ func (h *CallbackHandler) handleStopNotifications(b *gotgbot.Bot, ctx *ext.Conte if l.WebhookID != 0 { client, err := github.GetClientForUser(context.Background(), h.DB, h.ClientFactory, ctx.EffectiveUser.Id, h.EncryptionKey) if err != nil { - if err.Error() == "unauthorized" { + if errors.Is(err, github.ErrUnauthorized) { warning = "\n\nWarning: your GitHub account is not connected, so the GitHub webhook could not be removed automatically." } else { warning = "\n\nWarning: your GitHub token could not be decrypted, so the GitHub webhook was not removed automatically." @@ -486,7 +437,7 @@ func (h *CallbackHandler) handlePresets(b *gotgbot.Bot, ctx *ext.Context, l *mod func (h *CallbackHandler) showIndividualEvents(b *gotgbot.Bot, ctx *ext.Context, l *models.RepoLink, page int) error { client, err := github.GetClientForUser(context.Background(), h.DB, h.ClientFactory, ctx.EffectiveUser.Id, h.EncryptionKey) if err != nil { - if err.Error() == "unauthorized" { + if errors.Is(err, github.ErrUnauthorized) { _, _, _ = ctx.EffectiveMessage.EditText(b, "Error: You must be connected to GitHub to view/edit settings.", nil) } else { _, _, _ = ctx.EffectiveMessage.EditText(b, "Auth error. Please reconnect.", nil) @@ -583,7 +534,7 @@ func (h *CallbackHandler) showRepoList(b *gotgbot.Bot, ctx *ext.Context) error { var kb [][]gotgbot.InlineKeyboardButton for _, l := range links { kb = append(kb, []gotgbot.InlineKeyboardButton{ - ui.Callback(repoButtonText(l.RepoFullName), cbRepo(cbRepoMenu, &l), + ui.Callback(ui.CompactButtonText(l.RepoFullName), cbRepo(cbRepoMenu, &l), ui.WithStyle(ui.StylePrimary), ui.WithCustomEmojiEnv(ui.IconSettings), ), @@ -619,14 +570,14 @@ func (h *CallbackHandler) handleRepoPage(b *gotgbot.Bot, ctx *ext.Context, page var kb [][]gotgbot.InlineKeyboardButton for _, repo := range repos { kb = append(kb, []gotgbot.InlineKeyboardButton{ - ui.Callback(repoButtonText(repo.GetFullName()), cbAddRepoID(repo.GetID()), + ui.Callback(ui.CompactButtonText(repo.GetFullName()), cbAddRepoID(repo.GetID()), ui.WithStyle(ui.StylePrimary), ui.WithCustomEmojiEnv(ui.IconAdd), ), }) } - if navRow := repoPageNav(page, resp); len(navRow) > 0 { + if navRow := ui.RepoPageNav(page, resp, cbAddRepoPage); len(navRow) > 0 { kb = append(kb, navRow) } diff --git a/internal/bot/commands/commands.go b/internal/bot/commands/commands.go index 8f8f820..e96d31d 100644 --- a/internal/bot/commands/commands.go +++ b/internal/bot/commands/commands.go @@ -140,55 +140,6 @@ func repoSettingsCallback(link models.RepoLink) string { return settingsCallback("c", "r", linkID) } -func compactButtonText(name string) string { - const max = 42 - if len(name) <= max { - return name - } - return name[:max-1] + "…" -} - -func repoPageKeyboardNav(page int, resp *github.Response) []gotgbot.InlineKeyboardButton { - var navRow []gotgbot.InlineKeyboardButton - - if resp.FirstPage != 0 && resp.PrevPage != 0 { - navRow = append(navRow, ui.Callback("<", addRepoPageCallback(resp.PrevPage), - ui.WithStyle(ui.StylePrimary), - ui.WithCustomEmojiEnv(ui.IconPrevious), - )) - } - - startPage := page - 1 - if startPage < 1 { - startPage = 1 - } - - endPage := page + 1 - if resp.LastPage != 0 && endPage > resp.LastPage { - endPage = resp.LastPage - } - if resp.LastPage == 0 && resp.NextPage != 0 { - endPage = resp.NextPage - } - - for i := startPage; i <= endPage; i++ { - text := strconv.Itoa(i) - if i == page { - text = "· " + text + " ·" - } - navRow = append(navRow, ui.Callback(text, addRepoPageCallback(i), ui.WithStyle(ui.StylePrimary))) - } - - if resp.NextPage != 0 { - navRow = append(navRow, ui.Callback(">", addRepoPageCallback(resp.NextPage), - ui.WithStyle(ui.StylePrimary), - ui.WithCustomEmojiEnv(ui.IconNext), - )) - } - - return navRow -} - func (h *CommandHandler) AddRepo(b *gotgbot.Bot, ctx *ext.Context) error { if err := requireAdminOrPrivate(b, ctx, "Only admins can add repositories."); err != nil { return err @@ -202,7 +153,7 @@ func (h *CommandHandler) AddRepo(b *gotgbot.Bot, ctx *ext.Context) error { repoFullName := args[1] client, err := gh.GetClientForUser(context.Background(), h.DB, h.ClientFactory, ctx.EffectiveUser.Id, h.EncryptionKey) if err != nil { - if err.Error() == "unauthorized" { + if errors.Is(err, gh.ErrUnauthorized) { url, urlErr := h.loginURLForUser(ctx.EffectiveUser.Id) if urlErr != nil { return urlErr @@ -316,7 +267,7 @@ func (h *CommandHandler) listUserRepos(b *gotgbot.Bot, ctx *ext.Context) error { func (h *CommandHandler) sendRepoList(b *gotgbot.Bot, ctx *ext.Context, page int) error { client, err := gh.GetClientForUser(context.Background(), h.DB, h.ClientFactory, ctx.EffectiveUser.Id, h.EncryptionKey) if err != nil { - if err.Error() == "unauthorized" { + if errors.Is(err, gh.ErrUnauthorized) { _, _ = ctx.EffectiveMessage.Reply(b, "Please /connect your GitHub account first to list repositories.", nil) } else { _, _ = ctx.EffectiveMessage.Reply(b, "Auth error. Reconnect via /connect", nil) @@ -346,14 +297,14 @@ func (h *CommandHandler) sendRepoList(b *gotgbot.Bot, ctx *ext.Context, page int var kb [][]gotgbot.InlineKeyboardButton for _, repo := range repos { kb = append(kb, []gotgbot.InlineKeyboardButton{ - ui.Callback(compactButtonText(repo.GetFullName()), addRepoIDCallback(repo.GetID()), + ui.Callback(ui.CompactButtonText(repo.GetFullName()), addRepoIDCallback(repo.GetID()), ui.WithStyle(ui.StylePrimary), ui.WithCustomEmojiEnv(ui.IconAdd), ), }) } - if navRow := repoPageKeyboardNav(page, resp); len(navRow) > 0 { + if navRow := ui.RepoPageNav(page, resp, addRepoPageCallback); len(navRow) > 0 { kb = append(kb, navRow) } @@ -381,7 +332,7 @@ func (h *CommandHandler) Settings(b *gotgbot.Bot, ctx *ext.Context) error { var kb [][]gotgbot.InlineKeyboardButton for _, l := range links { kb = append(kb, []gotgbot.InlineKeyboardButton{ - ui.Callback(compactButtonText(l.RepoFullName), repoSettingsCallback(l), + ui.Callback(ui.CompactButtonText(l.RepoFullName), repoSettingsCallback(l), ui.WithStyle(ui.StylePrimary), ui.WithCustomEmojiEnv(ui.IconSettings), ), @@ -417,7 +368,7 @@ func (h *CommandHandler) RemoveRepo(b *gotgbot.Bot, ctx *ext.Context) error { if link.WebhookID != 0 { client, err := gh.GetClientForUser(context.Background(), h.DB, h.ClientFactory, ctx.EffectiveUser.Id, h.EncryptionKey) if err != nil { - if err.Error() == "unauthorized" { + if errors.Is(err, gh.ErrUnauthorized) { webhookStatusMsg = "\n\n⚠️ Warning: You are not connected to GitHub. The webhook could not be removed from the repository settings. Please remove it manually." } else { webhookStatusMsg = "\n\n⚠️ Warning: Could not decrypt your access token. Webhook not removed from GitHub." @@ -654,7 +605,7 @@ func (h *CommandHandler) handleIssueAction(b *gotgbot.Bot, ctx *ext.Context, sta func (h *CommandHandler) getAuthenticatedClient(b *gotgbot.Bot, ctx *ext.Context) (*github.Client, error) { client, err := gh.GetClientForUser(context.Background(), h.DB, h.ClientFactory, ctx.EffectiveUser.Id, h.EncryptionKey) if err != nil { - if err.Error() == "unauthorized" { + if errors.Is(err, gh.ErrUnauthorized) { url, urlErr := h.loginURLForUser(ctx.EffectiveUser.Id) if urlErr != nil { return nil, urlErr diff --git a/internal/bot/commands/reply_handler.go b/internal/bot/commands/reply_handler.go index d5dbe8d..85259e1 100644 --- a/internal/bot/commands/reply_handler.go +++ b/internal/bot/commands/reply_handler.go @@ -2,6 +2,7 @@ package commands import ( "context" + "errors" "fmt" "strings" @@ -50,7 +51,7 @@ func (h *ReplyHandler) HandleReply(b *gotgbot.Bot, ctx *ext.Context) error { client, err := github.GetClientForUser(context.Background(), h.DB, h.ClientFactory, ctx.EffectiveUser.Id, h.EncryptionKey) if err != nil { - if err.Error() == "unauthorized" { + if errors.Is(err, github.ErrUnauthorized) { _, _ = msg.Reply(b, "Please /connect your GitHub account in a private chat before replying to GitHub items.", nil) } else { _, _ = msg.Reply(b, "Auth error. Reconnect via /connect", nil) diff --git a/internal/bot/middleware/middleware.go b/internal/bot/middleware/middleware.go index 3d33a9b..5c98201 100644 --- a/internal/bot/middleware/middleware.go +++ b/internal/bot/middleware/middleware.go @@ -2,7 +2,9 @@ package middleware import ( "context" + "time" + "github-webhook/internal/cache" "github-webhook/internal/db" "github-webhook/internal/models" @@ -10,6 +12,10 @@ import ( "github.com/PaulSonOfLars/gotgbot/v2/ext" ) +// chatUpsertSeen debounces chat upserts so we write to the DB at most once per chat +// every 10 minutes instead of on every incoming update. +var chatUpsertSeen = cache.New[int64, struct{}]() + func TrackUserAndChat(database *db.DB) func(b *gotgbot.Bot, ctx *ext.Context) error { return func(b *gotgbot.Bot, ctx *ext.Context) error { if ctx.EffectiveChat != nil { @@ -23,6 +29,11 @@ func TrackUserAndChat(database *db.DB) func(b *gotgbot.Bot, ctx *ext.Context) er dbChat.Title = ctx.EffectiveChat.Username } + if _, seen := chatUpsertSeen.Get(ctx.EffectiveChat.Id); seen { + return nil + } + chatUpsertSeen.Set(ctx.EffectiveChat.Id, struct{}{}, 10*time.Minute) + go func() { _ = database.UpsertChat(context.Background(), dbChat) }() diff --git a/internal/bot/ui/buttons.go b/internal/bot/ui/buttons.go index cca2ed5..a74ad4f 100644 --- a/internal/bot/ui/buttons.go +++ b/internal/bot/ui/buttons.go @@ -3,9 +3,11 @@ package ui import ( "log" "os" + "strconv" "strings" "github.com/PaulSonOfLars/gotgbot/v2" + gh "github.com/google/go-github/v85/github" ) const ( @@ -85,3 +87,56 @@ func validateCallback(data string) { log.Printf("Telegram callback_data exceeds 64 bytes: length=%d data=%q", len(data), data) } } + +// RepoPageNav builds the previous / numbered / next pagination row for the add-repo +// repository picker. pageData maps a page number to its callback data string; commands +// and callbacks pass their own builder so ui stays unaware of the callback protocol. +func RepoPageNav(page int, resp *gh.Response, pageData func(int) string) []gotgbot.InlineKeyboardButton { + var navRow []gotgbot.InlineKeyboardButton + + if resp.FirstPage != 0 && resp.PrevPage != 0 { + navRow = append(navRow, Callback("<", pageData(resp.PrevPage), + WithStyle(StylePrimary), + WithCustomEmojiEnv(IconPrevious), + )) + } + + startPage := page - 1 + if startPage < 1 { + startPage = 1 + } + + endPage := page + 1 + if resp.LastPage != 0 && endPage > resp.LastPage { + endPage = resp.LastPage + } + if resp.LastPage == 0 && resp.NextPage != 0 { + endPage = resp.NextPage + } + + for i := startPage; i <= endPage; i++ { + text := strconv.Itoa(i) + if i == page { + text = "· " + text + " ·" + } + navRow = append(navRow, Callback(text, pageData(i), WithStyle(StylePrimary))) + } + + if resp.NextPage != 0 { + navRow = append(navRow, Callback(">", pageData(resp.NextPage), + WithStyle(StylePrimary), + WithCustomEmojiEnv(IconNext), + )) + } + + return navRow +} + +// CompactButtonText truncates a button label to Telegram-friendly length with an ellipsis. +func CompactButtonText(name string) string { + const max = 42 + if len(name) <= max { + return name + } + return name[:max-1] + "…" +} diff --git a/internal/db/db.go b/internal/db/db.go index 77a51aa..d66a6d4 100644 --- a/internal/db/db.go +++ b/internal/db/db.go @@ -83,7 +83,15 @@ func (d *DB) GetUserByTelegramID(ctx context.Context, telegramID int64) (*models func (d *DB) UpsertUser(ctx context.Context, user *models.User) error { opts := options.UpdateOne().SetUpsert(true) filter := bson.M{"_id": user.ID} - update := bson.M{ + _, err := d.Users.UpdateOne(ctx, filter, buildUserUpsert(user), opts) + return err +} + +// buildUserUpsert returns the update document for UpsertUser. Mutable fields go under +// $set; the immutable _id goes under $setOnInsert so re-upserts of existing users never +// attempt to modify _id (which MongoDB rejects). +func buildUserUpsert(user *models.User) bson.M { + return bson.M{ "$set": bson.M{ "github_user_id": user.GitHubUserID, "github_username": user.GitHubUsername, @@ -94,8 +102,6 @@ func (d *DB) UpsertUser(ctx context.Context, user *models.User) error { "_id": user.ID, }, } - _, err := d.Users.UpdateOne(ctx, filter, update, opts) - return err } func (d *DB) ClearUserToken(ctx context.Context, userID int64) error { diff --git a/internal/db/db_test.go b/internal/db/db_test.go new file mode 100644 index 0000000..58d7c57 --- /dev/null +++ b/internal/db/db_test.go @@ -0,0 +1,49 @@ +package db + +import ( + "testing" + + "go.mongodb.org/mongo-driver/v2/bson" + + "github-webhook/internal/models" +) + +// TestBuildUserUpsertShape locks the UpsertUser BSON shape without requiring a live +// MongoDB: _id must live under $setOnInsert (never $set, which MongoDB rejects for +// existing documents) and the document must be valid BSON. +func TestBuildUserUpsertShape(t *testing.T) { + update := buildUserUpsert(&models.User{ + ID: 42, + GitHubUserID: 7, + GitHubUsername: "alice", + EncryptedOAuthToken: "enc", + Scopes: []string{"repo"}, + }) + + if _, ok := update["$set"]; !ok { + t.Fatal("update missing $set") + } + if _, ok := update["$setOnInsert"]; !ok { + t.Fatal("update missing $setOnInsert") + } + + set := update["$set"].(bson.M) + if _, ok := set["_id"]; ok { + t.Fatal("_id must not appear under $set (immutable field)") + } + + setOnInsert := update["$setOnInsert"].(bson.M) + id, ok := setOnInsert["_id"] + if !ok || id != int64(42) { + t.Fatalf("expected _id=42 under $setOnInsert, got %v (present=%v)", id, ok) + } + + data, err := bson.Marshal(update) + if err != nil { + t.Fatalf("update is not valid BSON: %v", err) + } + var round bson.M + if err := bson.Unmarshal(data, &round); err != nil { + t.Fatalf("bson round-trip failed: %v", err) + } +} diff --git a/internal/github/auth_helper.go b/internal/github/auth_helper.go index 94727d0..a283214 100644 --- a/internal/github/auth_helper.go +++ b/internal/github/auth_helper.go @@ -2,6 +2,7 @@ package github import ( "context" + "errors" "fmt" "github-webhook/internal/db" @@ -10,12 +11,16 @@ import ( "github.com/google/go-github/v85/github" ) +// ErrUnauthorized is returned by GetClientForUser when the user has no linked GitHub +// account or no decrypted OAuth token. Callers should use errors.Is to detect it. +var ErrUnauthorized = errors.New("unauthorized") + // GetClientForUser retrieves the user's OAuth token from the database, decrypts it, // and returns an authenticated GitHub client. func GetClientForUser(ctx context.Context, database *db.DB, factory *ClientFactory, userID int64, encryptionKey string) (*github.Client, error) { user, err := database.GetUserByTelegramID(ctx, userID) if err != nil || user.EncryptedOAuthToken == "" { - return nil, fmt.Errorf("unauthorized") + return nil, ErrUnauthorized } token, err := utils.Decrypt(user.EncryptedOAuthToken, encryptionKey) diff --git a/internal/github/client.go b/internal/github/client.go index 0b5a434..6ce150a 100644 --- a/internal/github/client.go +++ b/internal/github/client.go @@ -2,21 +2,31 @@ package github import ( "context" + "sync" "github.com/google/go-github/v85/github" "golang.org/x/oauth2" ) type ClientFactory struct { + clients sync.Map // accessToken -> *github.Client } func NewClientFactory() *ClientFactory { return &ClientFactory{} } -// GetUserClient returns a GitHub client authenticated as a specific User (via OAuth token) +// GetUserClient returns a GitHub client authenticated as a specific User (via OAuth token). +// Clients are cached per access token to reuse the underlying TCP connection and avoid +// rebuilding the oauth2 transport on every call. func (f *ClientFactory) GetUserClient(ctx context.Context, accessToken string) *github.Client { + if c, ok := f.clients.Load(accessToken); ok { + return c.(*github.Client) + } + ts := oauth2.StaticTokenSource(&oauth2.Token{AccessToken: accessToken}) tc := oauth2.NewClient(ctx, ts) - return github.NewClient(tc) + c := github.NewClient(tc) + f.clients.Store(accessToken, c) + return c } diff --git a/internal/github/webhooks.go b/internal/github/webhooks.go index 9ef3227..a4bad0e 100644 --- a/internal/github/webhooks.go +++ b/internal/github/webhooks.go @@ -38,6 +38,7 @@ type WebhookServer struct { Bot *gotgbot.Bot ContextCache *cache.Cache[string, models.MessageContext] // Key: "chat_id:message_id" ActionCache *cache.Cache[string, models.PRActionContext] // Key: UUID + DeliverySeen *cache.Cache[string, struct{}] // Key: X-GitHub-Delivery (idempotency) Wg sync.WaitGroup } @@ -48,6 +49,7 @@ func NewWebhookServer(cfg *config.Config, database *db.DB, bot *gotgbot.Bot, ctx Bot: bot, ContextCache: ctxCache, ActionCache: actionCache, + DeliverySeen: cache.New[string, struct{}](), } } @@ -58,6 +60,9 @@ func (s *WebhookServer) Handler(w http.ResponseWriter, r *http.Request) { return } + r.Body = http.MaxBytesReader(w, r.Body, maxWebhookPayloadBytes) + defer r.Body.Close() + receivedAt := time.Now() var chatID int64 eventType := github.WebHookType(r) @@ -88,9 +93,6 @@ func (s *WebhookServer) Handler(w http.ResponseWriter, r *http.Request) { log.Printf("Webhook received event=%s delivery=%s hook_id=%s chat=%d remote=%s", eventType, deliveryID, hookIDHeader, chatID, r.RemoteAddr) - r.Body = http.MaxBytesReader(w, r.Body, maxWebhookPayloadBytes) - defer r.Body.Close() - payload, err := github.ValidatePayload(r, []byte(s.Config.GitHubWebhookSecret)) if err != nil { var maxBytesErr *http.MaxBytesError @@ -123,6 +125,15 @@ func (s *WebhookServer) Handler(w http.ResponseWriter, r *http.Request) { hookID, _ = strconv.ParseInt(idStr, 10, 64) } + if deliveryID != "" { + if _, seen := s.DeliverySeen.Get(deliveryID); seen { + log.Printf("Webhook duplicate delivery ignored event=%s delivery=%s chat=%d", eventType, deliveryID, chatID) + w.WriteHeader(http.StatusOK) + return + } + s.DeliverySeen.Set(deliveryID, struct{}{}, 10*time.Minute) + } + s.Wg.Add(1) go func() { defer s.Wg.Done() From c8142ccf5006e5146ceede01381108f9a0136f7d Mon Sep 17 00:00:00 2001 From: Bisu Ghalan Date: Sat, 11 Jul 2026 08:04:26 +0545 Subject: [PATCH 2/2] fix: escape MarkdownV2 link anchors to stop silent plain-text fallback FormatTextWithMarkdown protected links but left the anchor text unescaped. An anchor containing MarkdownV2 special chars (e.g. "release.v2") made Telegram reject the message, so the bot fell back to plain text and dropped all formatting. Link anchors are now escaped; code spans stay verbatim. Also cap the formatted message at Telegram's 4096-rune limit on the primary send path (only the plain-text fallback truncated before, so long events could be lost outright). Co-Authored-By: Claude --- internal/github/markdown_utils.go | 22 +++++++++++++++++++++- internal/github/markdown_utils_test.go | 26 ++++++++++++++++++++++++++ internal/github/webhooks.go | 7 +++++++ 3 files changed, 54 insertions(+), 1 deletion(-) diff --git a/internal/github/markdown_utils.go b/internal/github/markdown_utils.go index dd5577f..561b45b 100644 --- a/internal/github/markdown_utils.go +++ b/internal/github/markdown_utils.go @@ -68,6 +68,8 @@ func EscapeMarkdownV2URL(text string) string { } // FormatTextWithMarkdown preserves Markdown links and code blocks while escaping other special characters. +// Link anchors are also escaped: an anchor such as "foo.bar" would otherwise break Telegram's +// MarkdownV2 parser (unescaped ".") and force a plain-text fallback that drops all formatting. func FormatTextWithMarkdown(text string) string { emailRe := regexp.MustCompile(`<[^> ]+@[^> ]+>`) var emails []string @@ -95,12 +97,30 @@ func FormatTextWithMarkdown(text string) string { for i, original := range originals { placeholder := fmt.Sprintf("___PLACEHOLDER_%d___", i) escapedPlaceholder := EscapeMarkdownV2(placeholder) - escapedBody = strings.Replace(escapedBody, escapedPlaceholder, original, 1) + escapedBody = strings.Replace(escapedBody, escapedPlaceholder, restoreProtectedSegment(original), 1) } return escapedBody } +// restoreProtectedSegment returns a MarkdownV2-safe form of a protected segment. Links get their +// anchor and URL escaped; code spans/fenced blocks are returned verbatim (their contents are literal). +func restoreProtectedSegment(segment string) string { + if text, url, ok := parseMarkdownLink(segment); ok { + return fmt.Sprintf("[%s](%s)", EscapeMarkdownV2(text), EscapeMarkdownV2URL(url)) + } + return segment +} + +func parseMarkdownLink(segment string) (text, url string, ok bool) { + linkRe := regexp.MustCompile(`^\[(.+)\]\((.+)\)$`) + m := linkRe.FindStringSubmatch(segment) + if m == nil { + return "", "", false + } + return m[1], m[2], true +} + func FormatReleaseBody(body string) string { formattedText := FormatTextWithMarkdown(body) lines := strings.Split(formattedText, "\n") diff --git a/internal/github/markdown_utils_test.go b/internal/github/markdown_utils_test.go index 7c24205..595dabe 100644 --- a/internal/github/markdown_utils_test.go +++ b/internal/github/markdown_utils_test.go @@ -159,3 +159,29 @@ func TestFormatReleaseBody(t *testing.T) { } }) } + +// An unescaped link anchor (e.g. a "." in "release.v2") makes Telegram's MarkdownV2 +// parser reject the whole message, forcing a plain-text fallback that drops all styling. +func TestFormatTextWithMarkdownEscapesLinkAnchor(t *testing.T) { + in := "See [release.v2.1](https://github.com/o/r/releases/tag/v2.1) for details." + want := "See [release\\.v2\\.1](https://github.com/o/r/releases/tag/v2.1) for details\\." + if got := FormatTextWithMarkdown(in); got != want { + t.Fatalf("FormatTextWithMarkdown() = %q, want %q", got, want) + } +} + +func TestFormatTextWithMarkdownKeepsCodeVerbatim(t *testing.T) { + in := "Run `go test ./...` and check `a.b` inline." + want := "Run `go test ./...` and check `a.b` inline\\." + if got := FormatTextWithMarkdown(in); got != want { + t.Fatalf("code spans should pass through unchanged, got %q", got) + } +} + +func TestFormatTextWithMarkdownEscapesBodySpecials(t *testing.T) { + in := "Fix a.b and call me (soon)! Use ~strike~." + want := "Fix a\\.b and call me \\(soon\\)\\! Use \\~strike\\~\\." + if got := FormatTextWithMarkdown(in); got != want { + t.Fatalf("FormatTextWithMarkdown() = %q, want %q", got, want) + } +} diff --git a/internal/github/webhooks.go b/internal/github/webhooks.go index a4bad0e..7f63982 100644 --- a/internal/github/webhooks.go +++ b/internal/github/webhooks.go @@ -172,6 +172,13 @@ func (s *WebhookServer) processEvent(event interface{}, chatID int64, hookID int msg = normalizeMessage(msg) + // Telegram rejects messages over 4096 runes. The plain-text fallback already + // truncates; cap here too so a long formatted event is not lost outright. + const maxTelegramText = 4096 + if runes := []rune(msg); len(runes) > maxTelegramText { + msg = string(runes[:maxTelegramText-1]) + "…" + } + var threadID int64 if hookID != 0 { ctx, cancel := context.WithTimeout(context.Background(), webhookDBTimeout)