From d545abe39227ea06256134dfe97bd1eb10aeaa89 Mon Sep 17 00:00:00 2001 From: claw Date: Thu, 14 May 2026 14:11:14 -0700 Subject: [PATCH] fix(#130): enforce HTTPS scheme consistently in GitHub client write methods PostReview, DeleteReview, and RequestReviewer were calling c.httpClient.Do directly, bypassing the scheme check in doRequest that rejects http:// URLs unless AllowInsecureHTTP is explicitly enabled. Introduce doRequestWithBody(ctx, method, url, body) with the same HTTPS guard, and refactor all three write methods to use it. This ensures tokens are never sent over plaintext regardless of which API path is exercised. Add scheme validation tests for each method. --- github/client.go | 103 ++++++++++++++++++++++-------------------- github/client_test.go | 36 +++++++++++++++ 2 files changed, 91 insertions(+), 48 deletions(-) diff --git a/github/client.go b/github/client.go index cb566e7..d77aa8c 100644 --- a/github/client.go +++ b/github/client.go @@ -376,6 +376,57 @@ func (c *Client) doGet(ctx context.Context, url string) ([]byte, error) { return c.doRequest(ctx, http.MethodGet, url, "") } +// doRequestWithBody performs an HTTP request with an optional body, applying the +// same HTTPS enforcement as doRequest. It is used by write methods (POST, PUT, +// DELETE) that bypass the retry loop in doRequest because write operations are +// not idempotent. +// +// body may be nil for requests that carry no payload (e.g. DELETE). +// When body is non-nil, Content-Type is set to application/json. +func (c *Client) doRequestWithBody(ctx context.Context, method, reqURL string, body []byte) ([]byte, error) { + if !c.allowInsecureHTTP { + parsed, err := url.Parse(reqURL) + if err != nil { + return nil, fmt.Errorf("parse request URL: %w", err) + } + if strings.EqualFold(parsed.Scheme, "http") { + return nil, fmt.Errorf("refusing HTTP request to %s: use HTTPS or set AllowInsecureHTTP option", redactURL(reqURL)) + } + } + + var reqBody io.Reader + if body != nil { + reqBody = bytes.NewReader(body) + } + + req, err := http.NewRequestWithContext(ctx, method, reqURL, reqBody) + if err != nil { + return nil, fmt.Errorf("create request: %w", err) + } + req.Header.Set("Authorization", "Bearer "+c.token) + req.Header.Set("Accept", "application/vnd.github+json") + if body != nil { + req.Header.Set("Content-Type", "application/json") + } + + resp, err := c.httpClient.Do(req) + if err != nil { + return nil, fmt.Errorf("do request: %w", err) + } + defer resp.Body.Close() + + if resp.StatusCode >= 200 && resp.StatusCode < 300 { + respBody, err := io.ReadAll(io.LimitReader(resp.Body, maxResponseBodyBytes)) + if err != nil { + return nil, fmt.Errorf("read response body: %w", err) + } + return respBody, nil + } + + errBody, _ := io.ReadAll(io.LimitReader(resp.Body, maxErrorBodyBytes)) + return nil, &APIError{StatusCode: resp.StatusCode, Body: string(errBody)} +} + // --- API types --- // PullRequest holds relevant PR metadata. @@ -677,29 +728,11 @@ func (c *Client) PostReview(ctx context.Context, owner, repo string, number int, return nil, fmt.Errorf("marshal review payload: %w", err) } - req, err := http.NewRequestWithContext(ctx, http.MethodPost, reqURL, bytes.NewReader(data)) - if err != nil { - return nil, fmt.Errorf("create review request: %w", err) - } - req.Header.Set("Authorization", "Bearer "+c.token) - req.Header.Set("Content-Type", "application/json") - req.Header.Set("Accept", "application/vnd.github+json") - - resp, err := c.httpClient.Do(req) + respBody, err := c.doRequestWithBody(ctx, http.MethodPost, reqURL, data) if err != nil { return nil, fmt.Errorf("post review: %w", err) } - defer resp.Body.Close() - if resp.StatusCode < 200 || resp.StatusCode >= 300 { - respBody, _ := io.ReadAll(io.LimitReader(resp.Body, maxErrorBodyBytes)) - return nil, &APIError{StatusCode: resp.StatusCode, Body: string(respBody)} - } - - respBody, err := io.ReadAll(io.LimitReader(resp.Body, maxResponseBodyBytes)) - if err != nil { - return nil, fmt.Errorf("read review response: %w", err) - } var review Review if err := json.Unmarshal(respBody, &review); err != nil { return nil, fmt.Errorf("parse review response: %w", err) @@ -740,23 +773,11 @@ func (c *Client) DeleteReview(ctx context.Context, owner, repo string, number in reqURL := fmt.Sprintf("%s/repos/%s/%s/pulls/%d/reviews/%d", c.baseURL, url.PathEscape(owner), url.PathEscape(repo), number, reviewID) - req, err := http.NewRequestWithContext(ctx, http.MethodDelete, reqURL, nil) - if err != nil { - return fmt.Errorf("create delete request: %w", err) - } - req.Header.Set("Authorization", "Bearer "+c.token) - req.Header.Set("Accept", "application/vnd.github+json") - - resp, err := c.httpClient.Do(req) + // nil body: the GitHub DELETE endpoint for reviews requires no request body. + _, err := c.doRequestWithBody(ctx, http.MethodDelete, reqURL, nil) if err != nil { return fmt.Errorf("delete review: %w", err) } - defer resp.Body.Close() - - if resp.StatusCode < 200 || resp.StatusCode >= 300 { - respBody, _ := io.ReadAll(io.LimitReader(resp.Body, maxErrorBodyBytes)) - return &APIError{StatusCode: resp.StatusCode, Body: string(respBody)} - } return nil } @@ -790,24 +811,10 @@ func (c *Client) RequestReviewer(ctx context.Context, owner, repo string, number return fmt.Errorf("marshal reviewer request: %w", err) } - req, err := http.NewRequestWithContext(ctx, http.MethodPost, reqURL, bytes.NewReader(data)) - if err != nil { - return fmt.Errorf("create reviewer request: %w", err) - } - req.Header.Set("Authorization", "Bearer "+c.token) - req.Header.Set("Content-Type", "application/json") - req.Header.Set("Accept", "application/vnd.github+json") - - resp, err := c.httpClient.Do(req) + _, err = c.doRequestWithBody(ctx, http.MethodPost, reqURL, data) if err != nil { return fmt.Errorf("request reviewer: %w", err) } - defer resp.Body.Close() - - if resp.StatusCode != http.StatusOK && resp.StatusCode != http.StatusCreated && resp.StatusCode != http.StatusNoContent { - respBody, _ := io.ReadAll(io.LimitReader(resp.Body, 256)) - return &APIError{StatusCode: resp.StatusCode, Body: string(respBody)} - } return nil } diff --git a/github/client_test.go b/github/client_test.go index 4b944a1..8aa22fb 100644 --- a/github/client_test.go +++ b/github/client_test.go @@ -1077,6 +1077,42 @@ func TestRequestReviewer_Success(t *testing.T) { } } +func TestPostReview_RejectsHTTP(t *testing.T) { + // PostReview must reject http:// base URLs — tokens must not be sent in plaintext. + c := NewClient("tok", "http://127.0.0.1:1") + _, err := c.PostReview(context.Background(), "owner", "repo", 1, "APPROVE", "body", "", nil) + if err == nil { + t.Fatal("expected error for HTTP base URL in PostReview") + } + if !strings.Contains(err.Error(), "refusing HTTP request") { + t.Errorf("unexpected error message: %v", err) + } +} + +func TestDeleteReview_RejectsHTTP(t *testing.T) { + // DeleteReview must reject http:// base URLs — tokens must not be sent in plaintext. + c := NewClient("tok", "http://127.0.0.1:1") + err := c.DeleteReview(context.Background(), "owner", "repo", 1, 42) + if err == nil { + t.Fatal("expected error for HTTP base URL in DeleteReview") + } + if !strings.Contains(err.Error(), "refusing HTTP request") { + t.Errorf("unexpected error message: %v", err) + } +} + +func TestRequestReviewer_RejectsHTTP(t *testing.T) { + // RequestReviewer must reject http:// base URLs — tokens must not be sent in plaintext. + c := NewClient("tok", "http://127.0.0.1:1") + err := c.RequestReviewer(context.Background(), "owner", "repo", 1, "reviewer1") + if err == nil { + t.Fatal("expected error for HTTP base URL in RequestReviewer") + } + if !strings.Contains(err.Error(), "refusing HTTP request") { + t.Errorf("unexpected error message: %v", err) + } +} + func TestEscapePath_SpecialChars(t *testing.T) { tests := []struct { input string