From 23da7eedf5c8ab684591022eef356fa629ba622d Mon Sep 17 00:00:00 2001
From: Rodin <rodin@forgedthought.ai>
Date: Mon, 11 May 2026 01:08:01 -0700
Subject: [PATCH] fix(gitea): address review feedback on retry logic

- Remove dead backoff[0] element; array now only contains retry delays
- Fix time.After timer leak by using time.NewTimer with timer.Stop()
- Add io.LimitReader (64KB) for error body reads to bound memory allocation

Addresses feedback from sonnet-review-bot, security-review-bot, and gpt-review-bot.
---
 gitea/client.go | 32 ++++++++++++++++++++++----------
 1 file changed, 22 insertions(+), 10 deletions(-)

diff --git a/gitea/client.go b/gitea/client.go
index 5feb9a8..0d2ec13 100644
--- a/gitea/client.go
+++ b/gitea/client.go
@@ -215,23 +215,32 @@ func (c *Client) PostReview(ctx context.Context, owner, repo string, number int,
 	}
 	return &review, nil
 }
-
 // doGet performs an HTTP GET request with retry on 5xx errors.
 // Retries up to 3 times with exponential backoff (1s, 2s delays).
 func (c *Client) doGet(ctx context.Context, reqURL string) ([]byte, error) {
 	const maxAttempts = 3
-	backoff := []time.Duration{0, 1 * time.Second, 2 * time.Second}
+	// backoff[i] is the delay before attempt i+1 (i.e., after attempt i fails).
+	// First attempt (i=0) has no delay; retries wait 1s then 2s.
+	backoff := []time.Duration{1 * time.Second, 2 * time.Second}
+
+	// maxErrorBodyBytes limits how much of an error response body we read
+	// to protect against malicious servers sending unbounded data.
+	const maxErrorBodyBytes = 64 * 1024 // 64 KB
 
 	var lastErr error
 	for attempt := 0; attempt < maxAttempts; attempt++ {
 		if attempt > 0 {
+			delay := backoff[attempt-1]
 			slog.Warn("retrying request after server error",
 				"attempt", attempt+1,
 				"url", reqURL,
-				"delay", backoff[attempt].String())
+				"delay", delay.String())
+
+			timer := time.NewTimer(delay)
 			select {
-			case <-time.After(backoff[attempt]):
+			case <-timer.C:
 			case <-ctx.Done():
+				timer.Stop()
 				return nil, ctx.Err()
 			}
 		}
@@ -247,17 +256,20 @@ func (c *Client) doGet(ctx context.Context, reqURL string) ([]byte, error) {
 			return nil, err
 		}
 
-		body, readErr := io.ReadAll(resp.Body)
-		resp.Body.Close()
-
 		if resp.StatusCode >= 200 && resp.StatusCode < 300 {
-			if readErr != nil {
-				return nil, readErr
+			body, err := io.ReadAll(resp.Body)
+			resp.Body.Close()
+			if err != nil {
+				return nil, err
 			}
 			return body, nil
 		}
 
-		lastErr = &APIError{StatusCode: resp.StatusCode, Body: string(body)}
+		// Error path: limit how much we read from potentially malicious server
+		errBody, _ := io.ReadAll(io.LimitReader(resp.Body, maxErrorBodyBytes))
+		resp.Body.Close()
+
+		lastErr = &APIError{StatusCode: resp.StatusCode, Body: string(errBody)}
 
 		// Only retry on 5xx server errors
 		if resp.StatusCode < 500 || resp.StatusCode >= 600 {