fix(github): address review findings from rounds 2884/2885/2887
PR Ready Gate / clear-labels (pull_request) Successful in 2s
CI / test (pull_request) Successful in 18s
CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 40s
CI / review (gpt-5, security, ., rodin/security-patterns, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 1m18s
CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 1m44s

- Fix response body limit check: read maxResponseBytes+1 and use > to
  distinguish exactly-at-limit from truncated (sonnet finding #1)
- Reject HTTPS→HTTP redirects outright instead of stripping auth and
  following; prevents plaintext metadata leakage (sonnet #2, security #1)
- Sanitize newlines in APIError.Error to prevent log injection from
  upstream response bodies (security #2)
- Add nil-return documentation to GetCommitStatuses (sonnet #3)
- Gate TestDoRequest_429RetryAfterHTTPDate behind testing.Short (sonnet #6)
- Add tests for redirect policy, exact-at-limit body, and error sanitization
This commit is contained in:
claw
2026-05-12 19:29:06 -07:00
parent 80af5037b2
commit 1194bc758c
3 changed files with 96 additions and 6 deletions
+15 -6
View File
@@ -41,6 +41,9 @@ func (e *APIError) Error() string {
if len(body) > 200 {
body = body[:200] + "...(truncated)"
}
// Sanitize newlines to prevent log injection from upstream response bodies.
body = strings.ReplaceAll(body, "\n", " ")
body = strings.ReplaceAll(body, "\r", " ")
return fmt.Sprintf("HTTP %d: %s", e.StatusCode, body)
}
@@ -104,15 +107,21 @@ type Client struct {
}
// defaultCheckRedirect is the redirect policy used by NewClient and SetHTTPClient(nil).
// It strips the Authorization header on cross-host redirects or protocol downgrades
// (HTTPS→HTTP) to prevent credential leakage, while still following the redirect.
// It rejects HTTPS→HTTP protocol downgrades (to prevent plaintext leakage) and strips
// the Authorization header on cross-host redirects to prevent credential leakage to
// third-party hosts (e.g. CDN redirects from GitHub).
func defaultCheckRedirect(req *http.Request, via []*http.Request) error {
if len(via) >= 10 {
return fmt.Errorf("stopped after 10 redirects")
}
// Strip Authorization on cross-host redirect or protocol downgrade (https→http).
prev := via[len(via)-1]
if req.URL.Host != prev.URL.Host || (prev.URL.Scheme == "https" && req.URL.Scheme == "http") {
// Reject protocol downgrade: HTTPS→HTTP leaks request metadata over plaintext.
if prev.URL.Scheme == "https" && req.URL.Scheme == "http" {
return fmt.Errorf("refusing redirect from HTTPS to HTTP (%s → %s)", prev.URL.Host, req.URL.Host)
}
// Strip Authorization on cross-host redirect to avoid leaking credentials
// to third-party hosts (GitHub legitimately redirects to CDN hosts).
if req.URL.Host != prev.URL.Host {
req.Header.Del("Authorization")
}
return nil
@@ -242,12 +251,12 @@ func (c *Client) doRequest(ctx context.Context, method, reqURL string, accept st
}
if resp.StatusCode >= 200 && resp.StatusCode < 300 {
body, err := io.ReadAll(io.LimitReader(resp.Body, maxResponseBytes))
body, err := io.ReadAll(io.LimitReader(resp.Body, maxResponseBytes+1))
resp.Body.Close()
if err != nil {
return nil, fmt.Errorf("read response body: %w", err)
}
if len(body) >= maxResponseBytes {
if len(body) > maxResponseBytes {
return nil, fmt.Errorf("response body exceeded %d bytes (truncated)", maxResponseBytes)
}
return body, nil