From e414471a162d4fd83160c0059f4bd33e8dd8a1c5 Mon Sep 17 00:00:00 2001 From: claw Date: Wed, 13 May 2026 05:54:06 -0700 Subject: [PATCH] fix(github): address review feedback on Retry-After implementation - Fix data race: copy retryBackoff slice per-request to prevent concurrent doRequest calls from racing on shared state - Fix parseRetryAfter: trim whitespace before parsing for robustness - Fix parseRetryAfter: treat delta-seconds of 0 as valid per RFC 7231 - Add bounded read on success path (10 MB limit) for defense-in-depth - Clean up TestDoRequest_429_RetryAfter_IntegerSeconds: remove dead code block and commented-out reasoning - Fix import ordering: context before errors (goimports compliance) --- github/client.go | 17 ++++++++++--- github/client_test.go | 58 +++++++------------------------------------ 2 files changed, 22 insertions(+), 53 deletions(-) diff --git a/github/client.go b/github/client.go index e908f69..db55266 100644 --- a/github/client.go +++ b/github/client.go @@ -27,6 +27,10 @@ const ( // maxErrorBodyBytes limits how much of an error response body we read // to protect against malicious servers sending unbounded data. maxErrorBodyBytes = 64 * 1024 // 64 KB + + // maxResponseBodyBytes limits how much of a successful response body we read + // for defense-in-depth against servers returning excessively large payloads. + maxResponseBodyBytes = 10 * 1024 * 1024 // 10 MB ) // APIError represents an HTTP error response from the GitHub API. @@ -136,8 +140,11 @@ func (c *Client) SetRetryBackoff(backoff []time.Duration) { // // Returns (0, false) if the value cannot be parsed as either format. func (c *Client) parseRetryAfter(value string) (time.Duration, bool) { + value = strings.TrimSpace(value) + // Try integer seconds first (most common from GitHub). - if seconds, err := strconv.Atoi(value); err == nil && seconds > 0 { + // RFC 7231 allows delta-seconds of 0 to indicate immediate retry. + if seconds, err := strconv.Atoi(value); err == nil && seconds >= 0 { return time.Duration(seconds) * time.Second, true } @@ -158,8 +165,10 @@ func (c *Client) parseRetryAfter(value string) (time.Duration, bool) { // It respects the Retry-After header when present, supporting both integer // seconds and HTTP-date formats (capped at maxRetryAfter). func (c *Client) doRequest(ctx context.Context, method, reqURL string, accept string) ([]byte, error) { - backoff := c.retryBackoff - if backoff == nil { + var backoff []time.Duration + if c.retryBackoff != nil { + backoff = append([]time.Duration(nil), c.retryBackoff...) + } else { backoff = []time.Duration{1 * time.Second, 2 * time.Second} } @@ -198,7 +207,7 @@ func (c *Client) doRequest(ctx context.Context, method, reqURL string, accept st } if resp.StatusCode >= 200 && resp.StatusCode < 300 { - body, err := io.ReadAll(resp.Body) + body, err := io.ReadAll(io.LimitReader(resp.Body, maxResponseBodyBytes)) resp.Body.Close() if err != nil { return nil, fmt.Errorf("read response body: %w", err) diff --git a/github/client_test.go b/github/client_test.go index 1a37191..e679704 100644 --- a/github/client_test.go +++ b/github/client_test.go @@ -1,8 +1,8 @@ package github import ( - "errors" "context" + "errors" "net/http" "net/http/httptest" "testing" @@ -48,7 +48,7 @@ func TestDoRequest_429_RetryAfter_IntegerSeconds(t *testing.T) { srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { attempts++ if attempts == 1 { - w.Header().Set("Retry-After", "3") + w.Header().Set("Retry-After", "1") w.WriteHeader(http.StatusTooManyRequests) w.Write([]byte("rate limited")) return @@ -59,52 +59,9 @@ func TestDoRequest_429_RetryAfter_IntegerSeconds(t *testing.T) { defer srv.Close() c := NewClient("tok", srv.URL) - // Use zero backoff so test doesn't wait — the Retry-After override only - // affects backoff[attempt] which is used on the NEXT iteration. Since - // we only have one retry, we set backoff[0] to 0 initially, then - // the 429 handler overrides it. To avoid waiting, we cancel quickly. - // Actually: the flow is attempt=0 gets 429, handler overrides backoff[0], - // then attempt=1 reads backoff[0]. So we need backoff[0] to be small after override. - // With Retry-After: 3, backoff[0] becomes 3s. Let's use context timeout. - // Better approach: just set backoff large enough and use very short timeout. - // Simplest: verify parsing works via parseRetryAfter unit tests and keep - // the integration test fast by not actually waiting. - - // For integration: set backoff to 0 initially. The 429 handler will override - // backoff[0] to 3s. To avoid waiting 3s, we'll just verify it retried. - // Actually we need to accept the 3s wait OR use a different test strategy. - - // Best approach: use a 1ms initial backoff that gets overridden, but we - // check correctness via the parseRetryAfter unit tests. For the integration - // test, use Retry-After: 0 edge case OR just test that retry happens. c.SetRetryBackoff([]time.Duration{0, 0}) - // The handler sets Retry-After: 3, which will override backoff[0] to 3s. - // But since we start with backoff[0]=0, the first attempt runs immediately, - // then on 429 the code does: backoff[0] = 3s. The retry loop then uses - // backoff[attempt-1] = backoff[0] = 3s for the delay before attempt 1. - // To keep the test fast, let's just test a small value. - srv.Close() - - // Recreate with small Retry-After - attempts = 0 - srv2 := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - attempts++ - if attempts == 1 { - w.Header().Set("Retry-After", "1") - w.WriteHeader(http.StatusTooManyRequests) - w.Write([]byte("rate limited")) - return - } - w.WriteHeader(http.StatusOK) - w.Write([]byte("success")) - })) - defer srv2.Close() - - c2 := NewClient("tok", srv2.URL) - c2.SetRetryBackoff([]time.Duration{0, 0}) - - body, err := c2.doGet(context.Background(), srv2.URL+"/test") + body, err := c.doGet(context.Background(), srv.URL+"/test") if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -320,9 +277,12 @@ func TestParseRetryAfter_IntegerSeconds(t *testing.T) { func TestParseRetryAfter_ZeroSeconds(t *testing.T) { c := NewClient("tok", "") - _, ok := c.parseRetryAfter("0") - if ok { - t.Error("expected ok=false for zero seconds") + delay, ok := c.parseRetryAfter("0") + if !ok { + t.Fatal("expected ok=true for zero seconds (RFC 7231 allows immediate retry)") + } + if delay != 0 { + t.Errorf("delay = %v, want 0", delay) } }