From 1194bc758ce8608c9f4fd79442f1ce6c27aedc84 Mon Sep 17 00:00:00 2001 From: claw Date: Tue, 12 May 2026 19:29:06 -0700 Subject: [PATCH] fix(github): address review findings from rounds 2884/2885/2887 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 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 | 21 ++++++++---- github/client_test.go | 80 +++++++++++++++++++++++++++++++++++++++++++ github/pr.go | 1 + 3 files changed, 96 insertions(+), 6 deletions(-) diff --git a/github/client.go b/github/client.go index 75f4596..eedfdc7 100644 --- a/github/client.go +++ b/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 diff --git a/github/client_test.go b/github/client_test.go index d94bf8e..a8ccc06 100644 --- a/github/client_test.go +++ b/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) diff --git a/github/pr.go b/github/pr.go index e21eb0e..89a3d99 100644 --- a/github/pr.go +++ b/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) {