diff --git a/github/client.go b/github/client.go index e786275..a778d9c 100644 --- a/github/client.go +++ b/github/client.go @@ -10,6 +10,7 @@ import ( "io" "log/slog" "net/http" + "net/url" "os" "strconv" "strings" @@ -268,21 +269,36 @@ func (c *Client) parseRetryAfter(value string) (time.Duration, bool) { return 0, false } -// redactURL redacts query parameters from a URL for safe inclusion in error -// messages and log output. +// redactURL redacts sensitive components from a URL for safe inclusion in error +// messages and log output. It removes userinfo (e.g., user:pass@) and replaces +// query parameters with a placeholder. func redactURL(rawURL string) string { - if idx := strings.IndexByte(rawURL, '?'); idx != -1 { - return rawURL[:idx] + "?" + u, err := url.Parse(rawURL) + if err != nil { + return "" } - return rawURL + if u.User != nil { + u.User = nil + } + if u.RawQuery != "" { + u.RawQuery = "" + } + u.Fragment = "" + return u.String() } // doRequest performs an HTTP request with retry on 429 rate limit responses. // It respects the Retry-After header when present, supporting both integer // seconds and HTTP-date formats (capped at maxRetryAfter). func (c *Client) doRequest(ctx context.Context, method, reqURL string, accept string) ([]byte, error) { - if !c.allowInsecureHTTP && strings.HasPrefix(reqURL, "http://") { - return nil, fmt.Errorf("refusing HTTP request to %s: use HTTPS or set AllowInsecureHTTP option", redactURL(reqURL)) + 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 backoff []time.Duration @@ -303,7 +319,6 @@ func (c *Client) doRequest(ctx context.Context, method, reqURL string, accept st timer := time.NewTimer(delay) select { case <-timer.C: - timer.Stop() case <-ctx.Done(): timer.Stop() return nil, ctx.Err() diff --git a/github/client_test.go b/github/client_test.go index a7d8fd7..3d5fa4e 100644 --- a/github/client_test.go +++ b/github/client_test.go @@ -544,6 +544,30 @@ func TestNoInsecureOption_RejectsHTTP(t *testing.T) { t.Errorf("unexpected error message: %v", err) } } +func TestNoInsecureOption_RejectsUppercaseHTTP(t *testing.T) { + // Verify case-insensitive scheme check (RFC 3986). + c := NewClient("tok", "HTTP://example.com") + _, err := c.doGet(context.Background(), "HTTP://example.com/test") + if err == nil { + t.Fatal("expected error for uppercase HTTP scheme") + } + if !strings.Contains(err.Error(), "refusing HTTP request") { + t.Errorf("unexpected error message: %v", err) + } +} + +func TestNoInsecureOption_RejectsMixedCaseHTTP(t *testing.T) { + // Verify mixed case like "Http://" is also rejected. + c := NewClient("tok", "Http://example.com") + _, err := c.doGet(context.Background(), "Http://example.com/test") + if err == nil { + t.Fatal("expected error for mixed-case HTTP scheme") + } + if !strings.Contains(err.Error(), "refusing HTTP request") { + t.Errorf("unexpected error message: %v", err) + } +} + func TestAllowInsecureHTTP_WithoutEnvVar_Rejected(t *testing.T) { srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { @@ -616,3 +640,19 @@ func TestRedactURL_NoQuery(t *testing.T) { t.Errorf("redactURL = %q, want %q", got, want) } } + +func TestRedactURL_Userinfo(t *testing.T) { + got := redactURL("http://user:pass@localhost:1234/path") + want := "http://localhost:1234/path" + if got != want { + t.Errorf("redactURL = %q, want %q", got, want) + } +} + +func TestRedactURL_UserinfoWithQuery(t *testing.T) { + got := redactURL("http://user:pass@localhost:1234/path?secret=token") + want := "http://localhost:1234/path?" + if got != want { + t.Errorf("redactURL = %q, want %q", got, want) + } +}