rodin/review-bot
2
1
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases 6 Wiki Activity

feat: implement GitHub API methods and VCS routing (issue #130) #131

Merged
rodin merged 3 commits from issue-130 into main 2026-05-14 22:06:21 +00:00
Conversation 8 Commits 3 Files Changed 5
+91 -48
2 changed files with 91 additions and 48 deletions
Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit d545abe392 - Show all commits
github/client.go
+55 -48
View File
@@ -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 ---
security-review-bot marked this conversation as resolved
security-review-bot commented 2026-05-14 20:46:02 +00:00
Review
Copy Link
Copy Source

[MAJOR] PostReview, DeleteReview, and RequestReviewer construct and execute HTTP requests directly (c.httpClient.Do) without the HTTPS-only guard used in doRequest. If baseURL is accidentally configured with an http:// scheme (e.g., GHES on HTTP), these methods will send the Authorization header over plaintext, exposing tokens. Enforce scheme checks consistently or reuse a common request helper for non-GET methods.

**[MAJOR]** PostReview, DeleteReview, and RequestReviewer construct and execute HTTP requests directly (c.httpClient.Do) without the HTTPS-only guard used in doRequest. If baseURL is accidentally configured with an http:// scheme (e.g., GHES on HTTP), these methods will send the Authorization header over plaintext, exposing tokens. Enforce scheme checks consistently or reuse a common request helper for non-GET methods.
// PullRequest holds relevant PR metadata.
sonnet-review-bot commented 2026-05-14 20:45:20 +00:00
Review
Copy Link
Copy Source

[NIT] getFileContentAtRef uses filepath as a parameter name, which shadows the standard library package path/filepath. While filepath is not imported in this file, the naming is potentially confusing. The gitea client likely uses a different name; consider filePath or path for consistency.

**[NIT]** getFileContentAtRef uses `filepath` as a parameter name, which shadows the standard library package `path/filepath`. While `filepath` is not imported in this file, the naming is potentially confusing. The gitea client likely uses a different name; consider `filePath` or `path` for consistency.
1
 @@ -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
}
github/client_test.go
+36
View File
2
 @@ -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