fix(github): address review findings from rounds 2884/2885/2887

- Fix response body limit check: read maxResponseBytes+1 and use > to
  distinguish exactly-at-limit from truncated (sonnet finding #1)
- Reject HTTPS→HTTP redirects outright instead of stripping auth and
  following; prevents plaintext metadata leakage (sonnet #2, security #1)
- Sanitize newlines in APIError.Error to prevent log injection from
  upstream response bodies (security #2)
- Add nil-return documentation to GetCommitStatuses (sonnet #3)
- Gate TestDoRequest_429RetryAfterHTTPDate behind testing.Short (sonnet #6)
- Add tests for redirect policy, exact-at-limit body, and error sanitization

github/client.go

@@ -41,6 +41,9 @@ func (e *APIError) Error() string {
 	if len(body) > 200 {
 		body = body[:200] + "...(truncated)"
 	}
+	// Sanitize newlines to prevent log injection from upstream response bodies.
+	body = strings.ReplaceAll(body, "\n", " ")
+	body = strings.ReplaceAll(body, "\r", " ")
 	return fmt.Sprintf("HTTP %d: %s", e.StatusCode, body)
 }
 
@@ -104,15 +107,21 @@ type Client struct {
 }
 
 // defaultCheckRedirect is the redirect policy used by NewClient and SetHTTPClient(nil).
-// It strips the Authorization header on cross-host redirects or protocol downgrades
-// (HTTPS→HTTP) to prevent credential leakage, while still following the redirect.
+// It rejects HTTPS→HTTP protocol downgrades (to prevent plaintext leakage) and strips
+// the Authorization header on cross-host redirects to prevent credential leakage to
+// third-party hosts (e.g. CDN redirects from GitHub).
 func defaultCheckRedirect(req *http.Request, via []*http.Request) error {
 	if len(via) >= 10 {
 		return fmt.Errorf("stopped after 10 redirects")
 	}
-	// Strip Authorization on cross-host redirect or protocol downgrade (https→http).
 	prev := via[len(via)-1]
-	if req.URL.Host != prev.URL.Host || (prev.URL.Scheme == "https" && req.URL.Scheme == "http") {
+	// Reject protocol downgrade: HTTPS→HTTP leaks request metadata over plaintext.
+	if prev.URL.Scheme == "https" && req.URL.Scheme == "http" {
+		return fmt.Errorf("refusing redirect from HTTPS to HTTP (%s → %s)", prev.URL.Host, req.URL.Host)
+	}
+	// Strip Authorization on cross-host redirect to avoid leaking credentials
+	// to third-party hosts (GitHub legitimately redirects to CDN hosts).
+	if req.URL.Host != prev.URL.Host {
 		req.Header.Del("Authorization")
 	}
 	return nil
@@ -242,12 +251,12 @@ func (c *Client) doRequest(ctx context.Context, method, reqURL string, accept st
 		}
 
 		if resp.StatusCode >= 200 && resp.StatusCode < 300 {
-			body, err := io.ReadAll(io.LimitReader(resp.Body, maxResponseBytes))
+			body, err := io.ReadAll(io.LimitReader(resp.Body, maxResponseBytes+1))
 			resp.Body.Close()
 			if err != nil {
 				return nil, fmt.Errorf("read response body: %w", err)
 			}
-			if len(body) >= maxResponseBytes {
+			if len(body) > maxResponseBytes {
 				return nil, fmt.Errorf("response body exceeded %d bytes (truncated)", maxResponseBytes)
 			}
 			return body, nil

github/client_test.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"net/http"
 	"net/http/httptest"
+	"net/url"
 	"strings"
 	"testing"
 	"time"
@@ -185,6 +186,17 @@ func TestIsUnauthorized(t *testing.T) {
 	}
 }
 
+func TestAPIError_SanitizesNewlines(t *testing.T) {
+	err := &APIError{StatusCode: 500, Body: "line1\ninjected\rmore"}
+	msg := err.Error()
+	if strings.Contains(msg, "\n") || strings.Contains(msg, "\r") {
+		t.Errorf("expected newlines to be sanitized, got: %q", msg)
+	}
+	if !strings.Contains(msg, "line1 injected more") {
+		t.Errorf("expected sanitized body, got: %q", msg)
+	}
+}
+
 func TestDoRequest_429RetryAfterHeader(t *testing.T) {
 	if testing.Short() {
 		t.Skip("skipping slow retry test in short mode")
@@ -264,6 +276,9 @@ func TestDoRequest_RetryAfterDoesNotMutateBackoff(t *testing.T) {
 }
 
 func TestDoRequest_429RetryAfterHTTPDate(t *testing.T) {
+	if testing.Short() {
+		t.Skip("skipping slow Retry-After HTTP-date test in short mode")
+	}
 	attempts := 0
 	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		attempts++
@@ -379,6 +394,26 @@ func TestDoRequest_LimitsResponseBody(t *testing.T) {
 	}
 }
 
+func TestDoRequest_AcceptsExactlyAtLimit(t *testing.T) {
+	// A response body exactly equal to maxResponseBytes should succeed (not error).
+	exactBody := strings.Repeat("x", maxResponseBytes)
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		w.WriteHeader(200)
+		w.Write([]byte(exactBody))
+	}))
+	defer srv.Close()
+
+	c := NewClient("token", srv.URL, AllowInsecureHTTP())
+	c.SetHTTPClient(srv.Client())
+	body, err := c.doGet(context.Background(), srv.URL+"/test")
+	if err != nil {
+		t.Fatalf("unexpected error for exactly-at-limit body: %v", err)
+	}
+	if len(body) != maxResponseBytes {
+		t.Errorf("expected body length %d, got %d", maxResponseBytes, len(body))
+	}
+}
+
 func TestDoRequest_SkipsAuthWhenTokenEmpty(t *testing.T) {
 	var gotAuth string
 	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
@@ -405,6 +440,51 @@ func TestNewClient_CheckRedirectStripsAuthOnCrossHost(t *testing.T) {
 	}
 }
 
+func TestDefaultCheckRedirect_RejectsHTTPSToHTTP(t *testing.T) {
+	prev := &http.Request{URL: &url.URL{Scheme: "https", Host: "api.github.com", Path: "/foo"}}
+	req := &http.Request{
+		URL:    &url.URL{Scheme: "http", Host: "api.github.com", Path: "/foo"},
+		Header: http.Header{"Authorization": []string{"Bearer token"}},
+	}
+	err := defaultCheckRedirect(req, []*http.Request{prev})
+	if err == nil {
+		t.Fatal("expected error on HTTPS→HTTP redirect")
+	}
+	if !strings.Contains(err.Error(), "refusing redirect from HTTPS to HTTP") {
+		t.Errorf("unexpected error message: %v", err)
+	}
+}
+
+func TestDefaultCheckRedirect_StripsAuthOnCrossHost(t *testing.T) {
+	prev := &http.Request{URL: &url.URL{Scheme: "https", Host: "api.github.com", Path: "/foo"}}
+	req := &http.Request{
+		URL:    &url.URL{Scheme: "https", Host: "objects.githubusercontent.com", Path: "/bar"},
+		Header: http.Header{"Authorization": []string{"Bearer token"}},
+	}
+	err := defaultCheckRedirect(req, []*http.Request{prev})
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	if auth := req.Header.Get("Authorization"); auth != "" {
+		t.Errorf("expected Authorization header to be stripped, got %q", auth)
+	}
+}
+
+func TestDefaultCheckRedirect_PreservesAuthOnSameHost(t *testing.T) {
+	prev := &http.Request{URL: &url.URL{Scheme: "https", Host: "api.github.com", Path: "/foo"}}
+	req := &http.Request{
+		URL:    &url.URL{Scheme: "https", Host: "api.github.com", Path: "/bar"},
+		Header: http.Header{"Authorization": []string{"Bearer token"}},
+	}
+	err := defaultCheckRedirect(req, []*http.Request{prev})
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	if auth := req.Header.Get("Authorization"); auth != "Bearer token" {
+		t.Errorf("expected Authorization to be preserved, got %q", auth)
+	}
+}
+
 func TestDoRequest_RejectsHTTPWithToken(t *testing.T) {
 	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		w.WriteHeader(200)

github/pr.go

@@ -158,6 +158,7 @@ func (c *Client) GetFileContentAtRef(ctx context.Context, owner, repo, path, ref
 
 // GetCommitStatuses fetches both commit statuses and check runs for a SHA,
 // merging them into a unified []vcs.CommitStatus slice.
+// Returns nil (not an empty slice) when there are no statuses or check runs.
 // If the commit statuses endpoint fails (e.g. 404 for an unknown SHA), the
 // function returns immediately without attempting the check-runs endpoint.
 func (c *Client) GetCommitStatuses(ctx context.Context, owner, repo, sha string) ([]vcs.CommitStatus, error) {