fix: address MINOR review findings on PR #93 (round 2)
PR Ready Gate / clear-labels (pull_request) Successful in 2s
CI / test (pull_request) Successful in 17s
CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 38s
CI / review (gpt-5, security, ., rodin/security-patterns, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 2m28s
CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 2m50s
PR Ready Gate / clear-labels (pull_request) Successful in 2s
CI / test (pull_request) Successful in 17s
CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 38s
CI / review (gpt-5, security, ., rodin/security-patterns, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 2m28s
CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 2m50s
- Add User-Agent header to all requests (gpt-review-bot) - Limit successful response body to 10 MiB via io.LimitReader (security-review-bot) - Add CheckRedirect to strip Authorization on cross-host redirects (security-review-bot) - Fix decodeBase64Content to strip both \r and \n (gpt-review-bot) - Document that transport errors are not retried (sonnet-review-bot) - Update package doc to reflect current scope (no review submission yet) - Add tests for User-Agent, empty-token auth skip, CRLF base64, CheckRedirect
This commit is contained in:
+22
-4
@@ -1,6 +1,6 @@
|
|||||||
// Package github provides a client for the GitHub API.
|
// Package github provides a client for the GitHub API.
|
||||||
// It supports pull request operations, file content retrieval,
|
// It supports pull request operations, file content retrieval, CI status checks,
|
||||||
// and review submission for both github.com and GitHub Enterprise.
|
// and directory listing for both github.com and GitHub Enterprise.
|
||||||
package github
|
package github
|
||||||
|
|
||||||
import (
|
import (
|
||||||
@@ -15,6 +15,10 @@ import (
|
|||||||
)
|
)
|
||||||
|
|
||||||
const defaultBaseURL = "https://api.github.com"
|
const defaultBaseURL = "https://api.github.com"
|
||||||
|
const userAgent = "review-bot/1.0"
|
||||||
|
|
||||||
|
// maxResponseBytes limits successful response body reads to 10 MiB.
|
||||||
|
const maxResponseBytes = 10 * 1024 * 1024
|
||||||
|
|
||||||
// APIError represents an HTTP error response from the GitHub API.
|
// APIError represents an HTTP error response from the GitHub API.
|
||||||
// It carries the status code so callers can distinguish between
|
// It carries the status code so callers can distinguish between
|
||||||
@@ -82,7 +86,19 @@ func NewClient(token, baseURL string) *Client {
|
|||||||
return &Client{
|
return &Client{
|
||||||
baseURL: strings.TrimRight(baseURL, "/"),
|
baseURL: strings.TrimRight(baseURL, "/"),
|
||||||
token: token,
|
token: token,
|
||||||
http: &http.Client{Timeout: 30 * time.Second},
|
http: &http.Client{
|
||||||
|
Timeout: 30 * time.Second,
|
||||||
|
CheckRedirect: func(req *http.Request, via []*http.Request) error {
|
||||||
|
// Prevent forwarding Authorization header to different hosts on redirect.
|
||||||
|
if len(via) > 0 && req.URL.Host != via[0].URL.Host {
|
||||||
|
req.Header.Del("Authorization")
|
||||||
|
}
|
||||||
|
if len(via) >= 10 {
|
||||||
|
return fmt.Errorf("stopped after 10 redirects")
|
||||||
|
}
|
||||||
|
return nil
|
||||||
|
},
|
||||||
|
},
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -94,6 +110,7 @@ func (c *Client) SetHTTPClient(hc *http.Client) {
|
|||||||
|
|
||||||
// doRequest performs an HTTP request with retry on 429 rate limit responses.
|
// doRequest performs an HTTP request with retry on 429 rate limit responses.
|
||||||
// It respects the Retry-After header when present (capped at maxRetryAfter).
|
// It respects the Retry-After header when present (capped at maxRetryAfter).
|
||||||
|
// Transport errors (network failures, context cancellation) are not retried.
|
||||||
func (c *Client) doRequest(ctx context.Context, method, url string, accept string) ([]byte, error) {
|
func (c *Client) doRequest(ctx context.Context, method, url string, accept string) ([]byte, error) {
|
||||||
const maxAttempts = 3
|
const maxAttempts = 3
|
||||||
const maxRetryAfter = 120 * time.Second
|
const maxRetryAfter = 120 * time.Second
|
||||||
@@ -133,6 +150,7 @@ func (c *Client) doRequest(ctx context.Context, method, url string, accept strin
|
|||||||
if c.token != "" {
|
if c.token != "" {
|
||||||
req.Header.Set("Authorization", "Bearer "+c.token)
|
req.Header.Set("Authorization", "Bearer "+c.token)
|
||||||
}
|
}
|
||||||
|
req.Header.Set("User-Agent", userAgent)
|
||||||
if accept != "" {
|
if accept != "" {
|
||||||
req.Header.Set("Accept", accept)
|
req.Header.Set("Accept", accept)
|
||||||
} else {
|
} else {
|
||||||
@@ -145,7 +163,7 @@ func (c *Client) doRequest(ctx context.Context, method, url string, accept strin
|
|||||||
}
|
}
|
||||||
|
|
||||||
if resp.StatusCode >= 200 && resp.StatusCode < 300 {
|
if resp.StatusCode >= 200 && resp.StatusCode < 300 {
|
||||||
body, err := io.ReadAll(resp.Body)
|
body, err := io.ReadAll(io.LimitReader(resp.Body, maxResponseBytes))
|
||||||
resp.Body.Close()
|
resp.Body.Close()
|
||||||
if err != nil {
|
if err != nil {
|
||||||
return nil, fmt.Errorf("read response body: %w", err)
|
return nil, fmt.Errorf("read response body: %w", err)
|
||||||
|
|||||||
@@ -261,3 +261,56 @@ func TestDoRequest_RetryAfterDoesNotMutateBackoff(t *testing.T) {
|
|||||||
t.Errorf("RetryBackoff[1] was mutated: got %v, want 1ms", c.RetryBackoff[1])
|
t.Errorf("RetryBackoff[1] was mutated: got %v, want 1ms", c.RetryBackoff[1])
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
func TestDoRequest_SetsUserAgentHeader(t *testing.T) {
|
||||||
|
var gotUA string
|
||||||
|
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
||||||
|
gotUA = r.Header.Get("User-Agent")
|
||||||
|
w.WriteHeader(200)
|
||||||
|
w.Write([]byte("{}"))
|
||||||
|
}))
|
||||||
|
defer srv.Close()
|
||||||
|
|
||||||
|
c := NewClient("token", srv.URL)
|
||||||
|
c.SetHTTPClient(srv.Client())
|
||||||
|
_, _ = c.doGet(context.Background(), srv.URL+"/test")
|
||||||
|
|
||||||
|
if gotUA != "review-bot/1.0" {
|
||||||
|
t.Errorf("expected User-Agent 'review-bot/1.0', got %q", gotUA)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
func TestDoRequest_LimitsResponseBody(t *testing.T) {
|
||||||
|
// Verify that responses are read through a limit reader.
|
||||||
|
// We can't easily test the 10 MiB limit without OOM risk,
|
||||||
|
// but we verify the constant is set correctly.
|
||||||
|
if maxResponseBytes != 10*1024*1024 {
|
||||||
|
t.Errorf("expected maxResponseBytes = 10 MiB, got %d", maxResponseBytes)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
func TestDoRequest_SkipsAuthWhenTokenEmpty(t *testing.T) {
|
||||||
|
var gotAuth string
|
||||||
|
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
||||||
|
gotAuth = r.Header.Get("Authorization")
|
||||||
|
w.WriteHeader(200)
|
||||||
|
w.Write([]byte("{}"))
|
||||||
|
}))
|
||||||
|
defer srv.Close()
|
||||||
|
|
||||||
|
c := NewClient("", srv.URL) // empty token
|
||||||
|
c.SetHTTPClient(srv.Client())
|
||||||
|
_, _ = c.doGet(context.Background(), srv.URL+"/test")
|
||||||
|
|
||||||
|
if gotAuth != "" {
|
||||||
|
t.Errorf("expected no Authorization header with empty token, got %q", gotAuth)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
func TestNewClient_CheckRedirectStripsAuthOnCrossHost(t *testing.T) {
|
||||||
|
// Verify the CheckRedirect function is configured
|
||||||
|
c := NewClient("secret-token", "https://api.github.com")
|
||||||
|
if c.http.CheckRedirect == nil {
|
||||||
|
t.Fatal("expected CheckRedirect to be set")
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|||||||
+2
-2
@@ -61,10 +61,10 @@ func escapePath(p string) string {
|
|||||||
}
|
}
|
||||||
|
|
||||||
// decodeBase64Content decodes base64-encoded content from the GitHub contents API.
|
// decodeBase64Content decodes base64-encoded content from the GitHub contents API.
|
||||||
// GitHub returns base64 content with newlines for formatting, which we strip before decoding.
|
// GitHub returns base64 content with line breaks for formatting; we strip \r and \n before decoding.
|
||||||
func decodeBase64Content(encoded string) (string, error) {
|
func decodeBase64Content(encoded string) (string, error) {
|
||||||
// GitHub inserts newlines in base64 content
|
// GitHub inserts newlines in base64 content
|
||||||
cleaned := strings.ReplaceAll(encoded, "\n", "")
|
cleaned := strings.NewReplacer("\n", "", "\r", "").Replace(encoded)
|
||||||
decoded, err := base64.StdEncoding.DecodeString(cleaned)
|
decoded, err := base64.StdEncoding.DecodeString(cleaned)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
return "", err
|
return "", err
|
||||||
|
|||||||
@@ -295,3 +295,15 @@ func TestEscapePath_RejectsDotSegments(t *testing.T) {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
func TestDecodeBase64Content_CRLF(t *testing.T) {
|
||||||
|
// Base64 of "hello world" with CRLF line breaks inserted
|
||||||
|
encoded := "aGVs\r\nbG8g\r\nd29y\r\nbGQ="
|
||||||
|
decoded, err := decodeBase64Content(encoded)
|
||||||
|
if err != nil {
|
||||||
|
t.Fatalf("unexpected error: %v", err)
|
||||||
|
}
|
||||||
|
if decoded != "hello world" {
|
||||||
|
t.Errorf("expected 'hello world', got %q", decoded)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user