From 80af5037b220d3f6221197e4b4e50b59c4252bf7 Mon Sep 17 00:00:00 2001 From: claw Date: Tue, 12 May 2026 18:41:44 -0700 Subject: [PATCH] fix(github): address review findings from round 2880/2883 Sonnet MINOR #1: Stop timer after <-timer.C fires for idiomatic cleanup. Sonnet MINOR #2: Document that empty array from contents API is valid (empty dir). Sonnet MINOR #3: Document that GetPullRequestFiles returns nil for no files. Sonnet NIT #4: Strengthen SetHTTPClient/SetRetryBackoff docs to clarify test-only intent. Sonnet NIT #5: Document GetCommitStatuses fail-fast behavior. Sonnet NIT #6: Document double-slash collapsing in escapePath. Security MINOR #1: Document redirect policy responsibility when providing custom client. Security MINOR #2: Reduce maxErrorBodyBytes from 64KB to 4KB to limit sensitive data exposure. --- github/client.go | 28 ++++++++++++++++++++-------- github/files.go | 13 ++++++++----- github/pr.go | 4 ++++ 3 files changed, 32 insertions(+), 13 deletions(-) diff --git a/github/client.go b/github/client.go index 2a4569e..75f4596 100644 --- a/github/client.go +++ b/github/client.go @@ -27,9 +27,10 @@ const ( // It carries the status code so callers can distinguish between // different failure modes (e.g. 404 vs 500). // -// Note: Error() includes up to 200 bytes of the response body for debugging. -// Callers should avoid logging raw error messages in production if the upstream -// server may return sensitive details in error responses. +// The Body field stores up to 4 KiB of the raw response for programmatic +// inspection. Error() truncates to 200 bytes for safe logging, but callers +// should avoid logging or propagating Body directly in production since it may +// contain sensitive details from the upstream server. type APIError struct { StatusCode int Body string @@ -87,8 +88,9 @@ func AllowInsecureHTTP() ClientOption { } // Client interacts with the GitHub API. -// A Client is safe for concurrent use by multiple goroutines; -// however, SetHTTPClient and SetRetryBackoff must not be called concurrently with requests. +// A Client is safe for concurrent use by multiple goroutines. +// SetHTTPClient and SetRetryBackoff are intended for test setup only and must +// be called before any goroutines issue requests; they have no synchronization. type Client struct { baseURL string token string @@ -141,9 +143,15 @@ func NewClient(token, baseURL string, opts ...ClientOption) *Client { } // SetHTTPClient sets the underlying HTTP client used for requests. -// This is intended for testing to inject mock transports. +// This is intended for test setup only to inject mock transports; it must be +// called before any goroutines issue requests. +// // Passing nil restores the default client (30s timeout + auth-stripping // CheckRedirect policy matching NewClient). +// +// Callers providing a non-nil client are responsible for configuring a safe +// CheckRedirect policy. Without one, the default net/http behavior will follow +// redirects and may forward the Authorization header to untrusted hosts. func (c *Client) SetHTTPClient(hc *http.Client) { if hc == nil { hc = &http.Client{ @@ -155,6 +163,7 @@ func (c *Client) SetHTTPClient(hc *http.Client) { } // SetRetryBackoff configures the retry backoff durations for testing. +// It must be called before any goroutines issue requests. // In production the default {1s, 2s} applies. func (c *Client) SetRetryBackoff(d []time.Duration) { c.retryBackoff = d @@ -175,7 +184,10 @@ func (c *Client) doRequest(ctx context.Context, method, reqURL string, accept st backoff = []time.Duration{1 * time.Second, 2 * time.Second} } - const maxErrorBodyBytes = 64 * 1024 + // maxErrorBodyBytes limits how much of an error response body is stored. + // Kept small (4 KiB) to reduce the risk of sensitive data leakage if callers + // log APIError.Body directly. Error() further truncates to 200 bytes. + const maxErrorBodyBytes = 4 * 1024 // Reject non-HTTPS URLs early since the URL is immutable across retries. if c.token != "" && !c.allowInsecureHTTP { @@ -199,7 +211,7 @@ func (c *Client) doRequest(ctx context.Context, method, reqURL string, accept st timer := time.NewTimer(delay) select { case <-timer.C: - // Backoff elapsed, proceed with retry. + timer.Stop() // no-op after fire, releases runtime resources promptly case <-ctx.Done(): timer.Stop() return nil, ctx.Err() diff --git a/github/files.go b/github/files.go index 25fd697..0b12c4e 100644 --- a/github/files.go +++ b/github/files.go @@ -42,6 +42,8 @@ func (c *Client) ListContents(ctx context.Context, owner, repo, path string) ([] // The GitHub contents API returns an array for directories and an object // for single files. Try array first (common case), then fall back to object. + // An empty array ([]) is valid — it represents an empty directory — and + // results in a zero-length slice returned without error. var entries []entry if err := json.Unmarshal(body, &entries); err != nil { var single entry @@ -69,11 +71,12 @@ func (c *Client) ListContents(ctx context.Context, owner, repo, path string) ([] // escapePath escapes each segment of a relative file path for use in URLs. // Slashes are preserved as path separators; other special characters are escaped. -// Dot-segments ("." and "..") are silently removed to prevent path traversal. -// This is intentional: callers may receive a different path than requested without -// error. The function is package-private, and all callers (GetFileContentAtRef, -// ListContents) already handle missing-file errors from the API if the cleaned -// path doesn't match what the caller intended. +// Dot-segments ("." and "..") and empty segments (from consecutive slashes like +// "a//b") are silently removed to prevent path traversal and produce canonical +// paths. This is intentional: callers may receive a different path than requested +// without error. The function is package-private, and all callers +// (GetFileContentAtRef, ListContents) already handle missing-file errors from the +// API if the cleaned path doesn't match what the caller intended. func escapePath(p string) string { parts := strings.Split(p, "/") var clean []string diff --git a/github/pr.go b/github/pr.go index c088f2b..e21eb0e 100644 --- a/github/pr.go +++ b/github/pr.go @@ -89,6 +89,8 @@ const maxPages = 100 // GetPullRequestFiles fetches the list of files changed in a PR. // Paginates through all pages (100 per page) to collect all files. +// Returns nil (not an empty slice) when the PR has no changed files. +// Callers can safely range over or check len() on a nil slice. func (c *Client) GetPullRequestFiles(ctx context.Context, owner, repo string, number int) ([]vcs.ChangedFile, error) { var allFiles []vcs.ChangedFile @@ -156,6 +158,8 @@ 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. +// 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) { var result []vcs.CommitStatus