github/client.go

@@ -1,6 +1,6 @@
 // Package github provides a client for the GitHub API.
-// It supports pull request operations, file content retrieval,
-// and review submission for both github.com and GitHub Enterprise.
+// It supports pull request operations, file content retrieval, CI status checks,
+// and directory listing for both github.com and GitHub Enterprise.
 package github
 
 import (
@@ -15,6 +15,10 @@ import (
 )
 
 const defaultBaseURL = "https://api.github.com"
+const userAgent = "review-bot/1.0"
+
+// maxResponseBytes limits successful response body reads to 10 MiB.
+const maxResponseBytes = 10 * 1024 * 1024
 
 // APIError represents an HTTP error response from the GitHub API.
 // It carries the status code so callers can distinguish between
@@ -82,7 +86,19 @@ func NewClient(token, baseURL string) *Client {
 	return &Client{
 		baseURL: strings.TrimRight(baseURL, "/"),
 		token:   token,
-		http:    &http.Client{Timeout: 30 * time.Second},
+		http: &http.Client{
+			Timeout: 30 * time.Second,
+			CheckRedirect: func(req *http.Request, via []*http.Request) error {
+				// Prevent forwarding Authorization header to different hosts on redirect.
+				if len(via) > 0 && req.URL.Host != via[0].URL.Host {
+					req.Header.Del("Authorization")
+				}
+				if len(via) >= 10 {
+					return fmt.Errorf("stopped after 10 redirects")
+				}
+				return nil
+			},
+		},
 	}
 }
 
@@ -94,6 +110,7 @@ func (c *Client) SetHTTPClient(hc *http.Client) {
 
 // doRequest performs an HTTP request with retry on 429 rate limit responses.
 // It respects the Retry-After header when present (capped at maxRetryAfter).
+// Transport errors (network failures, context cancellation) are not retried.
 func (c *Client) doRequest(ctx context.Context, method, url string, accept string) ([]byte, error) {
 	const maxAttempts = 3
 	const maxRetryAfter = 120 * time.Second
@@ -133,6 +150,7 @@ func (c *Client) doRequest(ctx context.Context, method, url string, accept strin
 		if c.token != "" {
 			req.Header.Set("Authorization", "Bearer "+c.token)
 		}
+		req.Header.Set("User-Agent", userAgent)
 		if accept != "" {
 			req.Header.Set("Accept", accept)
 		} else {
@@ -145,7 +163,7 @@ func (c *Client) doRequest(ctx context.Context, method, url string, accept strin
 		}
 
 		if resp.StatusCode >= 200 && resp.StatusCode < 300 {
-			body, err := io.ReadAll(resp.Body)
+			body, err := io.ReadAll(io.LimitReader(resp.Body, maxResponseBytes))
 			resp.Body.Close()
 			if err != nil {
 				return nil, fmt.Errorf("read response body: %w", err)

github/client_test.go

@@ -261,3 +261,56 @@ func TestDoRequest_RetryAfterDoesNotMutateBackoff(t *testing.T) {
 		t.Errorf("RetryBackoff[1] was mutated: got %v, want 1ms", c.RetryBackoff[1])
 	}
 }
+
+func TestDoRequest_SetsUserAgentHeader(t *testing.T) {
+	var gotUA string
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		gotUA = r.Header.Get("User-Agent")
+		w.WriteHeader(200)
+		w.Write([]byte("{}"))
+	}))
+	defer srv.Close()
+
+	c := NewClient("token", srv.URL)
+	c.SetHTTPClient(srv.Client())
+	_, _ = c.doGet(context.Background(), srv.URL+"/test")
+
+	if gotUA != "review-bot/1.0" {
+		t.Errorf("expected User-Agent 'review-bot/1.0', got %q", gotUA)
+	}
+}
+
+func TestDoRequest_LimitsResponseBody(t *testing.T) {
+	// Verify that responses are read through a limit reader.
+	// We can't easily test the 10 MiB limit without OOM risk,
+	// but we verify the constant is set correctly.
+	if maxResponseBytes != 10*1024*1024 {
+		t.Errorf("expected maxResponseBytes = 10 MiB, got %d", maxResponseBytes)
+	}
+}
+
+func TestDoRequest_SkipsAuthWhenTokenEmpty(t *testing.T) {
+	var gotAuth string
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		gotAuth = r.Header.Get("Authorization")
+		w.WriteHeader(200)
+		w.Write([]byte("{}"))
+	}))
+	defer srv.Close()
+
+	c := NewClient("", srv.URL) // empty token
+	c.SetHTTPClient(srv.Client())
+	_, _ = c.doGet(context.Background(), srv.URL+"/test")
+
+	if gotAuth != "" {
+		t.Errorf("expected no Authorization header with empty token, got %q", gotAuth)
+	}
+}
+
+func TestNewClient_CheckRedirectStripsAuthOnCrossHost(t *testing.T) {
+	// Verify the CheckRedirect function is configured
+	c := NewClient("secret-token", "https://api.github.com")
+	if c.http.CheckRedirect == nil {
+		t.Fatal("expected CheckRedirect to be set")
+	}
+}

github/files.go

@@ -61,10 +61,10 @@ func escapePath(p string) string {
 }
 
 // decodeBase64Content decodes base64-encoded content from the GitHub contents API.
-// GitHub returns base64 content with newlines for formatting, which we strip before decoding.
+// GitHub returns base64 content with line breaks for formatting; we strip \r and \n before decoding.
 func decodeBase64Content(encoded string) (string, error) {
 	// GitHub inserts newlines in base64 content
-	cleaned := strings.ReplaceAll(encoded, "\n", "")
+	cleaned := strings.NewReplacer("\n", "", "\r", "").Replace(encoded)
 	decoded, err := base64.StdEncoding.DecodeString(cleaned)
 	if err != nil {
 		return "", err

github/files_test.go

@@ -295,3 +295,15 @@ func TestEscapePath_RejectsDotSegments(t *testing.T) {
 		}
 	}
 }
+
+func TestDecodeBase64Content_CRLF(t *testing.T) {
+	// Base64 of "hello world" with CRLF line breaks inserted
+	encoded := "aGVs\r\nbG8g\r\nd29y\r\nbGQ="
+	decoded, err := decodeBase64Content(encoded)
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	if decoded != "hello world" {
+		t.Errorf("expected 'hello world', got %q", decoded)
+	}
+}