diff --git a/cmd/bot/main.go b/cmd/bot/main.go index ddc4c9f..dcd553a 100644 --- a/cmd/bot/main.go +++ b/cmd/bot/main.go @@ -322,6 +322,7 @@ func run() (runErr error) { contextCache.Cleanup() actionCache.Cleanup() webhookServer.DeliverySeen.Cleanup() + clientFactory.Cleanup() } } }() diff --git a/internal/bot/callbacks/callbacks.go b/internal/bot/callbacks/callbacks.go index aa8b12e..f792ede 100644 --- a/internal/bot/callbacks/callbacks.go +++ b/internal/bot/callbacks/callbacks.go @@ -753,6 +753,7 @@ func (h *CallbackHandler) HandlePRAction(b *gotgbot.Bot, ctx *ext.Context) error } _, _ = ctx.CallbackQuery.Answer(b, &gotgbot.AnswerCallbackQueryOpts{Text: msg, ShowAlert: true}) + h.ActionCache.Delete(actionID) return nil } diff --git a/internal/config/config.go b/internal/config/config.go index 934f191..69e67c7 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -34,7 +34,6 @@ func Load() *Config { "MONGODB_URI", "GITHUB_CLIENT_ID", "GITHUB_CLIENT_SECRET", - "GITHUB_WEBHOOK_SECRET", "ENCRYPTION_KEY", } diff --git a/internal/github/client.go b/internal/github/client.go index 6ce150a..55d402f 100644 --- a/internal/github/client.go +++ b/internal/github/client.go @@ -3,13 +3,24 @@ package github import ( "context" "sync" + "time" "github.com/google/go-github/v85/github" "golang.org/x/oauth2" ) +// clientIdleTTL bounds how long an unused authenticated client is kept. The cache is +// keyed by access token and users re-authenticate over time, so without a TTL it would +// grow without bound for a long-running process. +const clientIdleTTL = 30 * time.Minute + +type cachedClient struct { + client *github.Client + lastUsed time.Time +} + type ClientFactory struct { - clients sync.Map // accessToken -> *github.Client + clients sync.Map // accessToken -> *cachedClient } func NewClientFactory() *ClientFactory { @@ -20,13 +31,26 @@ func NewClientFactory() *ClientFactory { // 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) + if v, ok := f.clients.Load(accessToken); ok { + cc := v.(*cachedClient) + cc.lastUsed = time.Now() + return cc.client } ts := oauth2.StaticTokenSource(&oauth2.Token{AccessToken: accessToken}) - tc := oauth2.NewClient(ctx, ts) - c := github.NewClient(tc) - f.clients.Store(accessToken, c) + c := github.NewClient(oauth2.NewClient(ctx, ts)) + f.clients.Store(accessToken, &cachedClient{client: c, lastUsed: time.Now()}) return c } + +// Cleanup drops clients that have not been used within clientIdleTTL. Safe to call +// periodically; in-flight requests on an evicted client are unaffected. +func (f *ClientFactory) Cleanup() { + now := time.Now() + f.clients.Range(func(key, value any) bool { + if now.Sub(value.(*cachedClient).lastUsed) > clientIdleTTL { + f.clients.Delete(key) + } + return true + }) +} diff --git a/internal/github/format.go b/internal/github/format.go index 8d2cdfe..858669a 100644 --- a/internal/github/format.go +++ b/internal/github/format.go @@ -1117,7 +1117,10 @@ func FormatPullRequestReviewThreadEvent(e *github.PullRequestReviewThreadEvent) FormatUser(sender.GetLogin()), ) - return FormatMessageWithButton(msg, "View Thread", e.GetThread().Comments[0].GetHTMLURL()) + if thread := e.GetThread(); thread != nil && len(thread.Comments) > 0 { + return FormatMessageWithButton(msg, "View Thread", thread.Comments[0].GetHTMLURL()) + } + return msg, nil } func FormatPullRequestTargetEvent(e *github.PullRequestTargetEvent) (string, *gotgbot.InlineKeyboardMarkup) { diff --git a/internal/github/webhooks.go b/internal/github/webhooks.go index 7f63982..45cd0b5 100644 --- a/internal/github/webhooks.go +++ b/internal/github/webhooks.go @@ -4,7 +4,6 @@ import ( "context" "errors" "fmt" - "io" "log" "net/http" "regexp" @@ -93,7 +92,17 @@ 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) - payload, err := github.ValidatePayload(r, []byte(s.Config.GitHubWebhookSecret)) + // Read and validate the body exactly once. ValidatePayloadFromBody consumes + // r.Body and returns the payload; it verifies the GitHub HMAC signature only + // when a secret (or a signature header) is present, so the dev-mode path with + // no secret and no signature still yields the real payload. The previous code + // called ValidatePayload (which drains the body) and then tried io.ReadAll on + // the now-empty body in the no-secret branch, producing an empty payload. + sig := r.Header.Get("X-Hub-Signature-256") + if sig == "" { + sig = r.Header.Get("X-Hub-Signature") + } + payload, err := github.ValidatePayloadFromBody(r.Header.Get("Content-Type"), r.Body, sig, []byte(s.Config.GitHubWebhookSecret)) if err != nil { var maxBytesErr *http.MaxBytesError if errors.As(err, &maxBytesErr) { @@ -103,14 +112,12 @@ func (s *WebhookServer) Handler(w http.ResponseWriter, r *http.Request) { } if s.Config.GitHubWebhookSecret == "" { - log.Printf("Webhook Warning: No GITHUB_WEBHOOK_SECRET configured. Validation skipped. event=%s chat=%d", eventType, chatID) - body, _ := io.ReadAll(r.Body) - payload = body + log.Printf("Webhook REJECTED: signature present but GITHUB_WEBHOOK_SECRET is empty; cannot verify. event=%s delivery=%s chat=%d error=%v", eventType, deliveryID, chatID, err) } else { log.Printf("Webhook REJECTED: Signature mismatch. This means the secret in GitHub doesn't match GITHUB_WEBHOOK_SECRET on Render. event=%s delivery=%s chat=%d error=%v", eventType, deliveryID, chatID, err) - http.Error(w, "Invalid signature", http.StatusUnauthorized) - return } + http.Error(w, "Invalid signature", http.StatusUnauthorized) + return } event, err := parseWebhookEvent(eventType, payload) @@ -352,8 +359,9 @@ func (s *WebhookServer) withPRActionButtons(event interface{}, markup *gotgbot.I func (s *WebhookServer) formatMessage(event interface{}) (msg string, markup *gotgbot.InlineKeyboardMarkup) { defer func() { - if recover() != nil { - msg, markup = FormatGenericEvent(event) + if r := recover(); r != nil { + log.Printf("Formatter panic for event %T: %v", event, r) + msg, markup = "", nil } }()