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