From 023234312617dcc456f4b62a05989126c8693503 Mon Sep 17 00:00:00 2001 From: claw Date: Wed, 13 May 2026 11:24:07 -0700 Subject: [PATCH 1/6] feat(github): add safeguards against accidental AllowInsecureHTTP use in production Three-layer defense for the AllowInsecureHTTP client option: 1. Environment gate: AllowInsecureHTTP() requires REVIEW_BOT_ALLOW_INSECURE=1 env var. Without it, the option is silently ignored with a slog.Warn. 2. Warning log on activation: When the env gate IS satisfied, a slog.Warn fires at client construction time so operators notice in production logs. 3. Test bypass: AllowInsecureHTTPForTest() skips the env gate entirely, keeping test code clean (no t.Setenv needed in every test). Additionally, doRequest now rejects HTTP URLs unless allowInsecureHTTP is set on the client, providing defense-in-depth against credential leakage. Closes #96 --- github/client.go | 72 +++++++++++++++++++++++-- github/client_test.go | 123 ++++++++++++++++++++++++++++++++++++++---- 2 files changed, 183 insertions(+), 12 deletions(-) diff --git a/github/client.go b/github/client.go index 041a49c..e786275 100644 --- a/github/client.go +++ b/github/client.go @@ -8,7 +8,9 @@ import ( "errors" "fmt" "io" + "log/slog" "net/http" + "os" "strconv" "strings" "time" @@ -97,6 +99,10 @@ type Client struct { token string httpClient *http.Client + // allowInsecureHTTP permits requests to HTTP (non-TLS) endpoints. + // When false, doRequest rejects URLs with an http:// scheme. + allowInsecureHTTP bool + // retryBackoff defines the delays between retry attempts for 429 responses. // retryBackoff[i] is the delay before attempt i+1 (after attempt i fails). // If nil, defaults to {1s, 2s}. @@ -135,16 +141,63 @@ func defaultCheckRedirect(req *http.Request, via []*http.Request) error { return nil } +// ClientOption configures optional behavior of a Client. +type ClientOption func(*clientConfig) + +type clientConfig struct { + allowInsecureHTTP bool + insecureIsTestBypass bool +} + +// AllowInsecureHTTP permits sending credentials over plaintext HTTP connections. +// In production, this option is gated by the REVIEW_BOT_ALLOW_INSECURE=1 +// environment variable. Without the env var set, the option is silently ignored +// and a warning is logged. +// +// For tests, prefer AllowInsecureHTTPForTest which bypasses the env gate. +func AllowInsecureHTTP() ClientOption { + return func(cfg *clientConfig) { + cfg.allowInsecureHTTP = true + } +} + +// AllowInsecureHTTPForTest permits sending credentials over plaintext HTTP +// without requiring the REVIEW_BOT_ALLOW_INSECURE environment variable. +// This is intended exclusively for test code using httptest.Server. +func AllowInsecureHTTPForTest() ClientOption { + return func(cfg *clientConfig) { + cfg.allowInsecureHTTP = true + cfg.insecureIsTestBypass = true + } +} + // NewClient creates a new GitHub API client. // If baseURL is empty, it defaults to https://api.github.com. // For GitHub Enterprise, pass the API base URL (e.g. https://github.concur.com/api/v3). -func NewClient(token, baseURL string) *Client { +func NewClient(token, baseURL string, opts ...ClientOption) *Client { if baseURL == "" { baseURL = defaultBaseURL } + + var cfg clientConfig + for _, opt := range opts { + opt(&cfg) + } + + if cfg.allowInsecureHTTP && !cfg.insecureIsTestBypass { + if os.Getenv("REVIEW_BOT_ALLOW_INSECURE") != "1" { + slog.Warn("AllowInsecureHTTP ignored: set REVIEW_BOT_ALLOW_INSECURE=1 to enable") + cfg.allowInsecureHTTP = false + } else { + slog.Warn("AllowInsecureHTTP enabled โ€” credentials may be sent over plaintext", + "env", "REVIEW_BOT_ALLOW_INSECURE=1") + } + } + return &Client{ - baseURL: strings.TrimRight(baseURL, "/"), - token: token, + baseURL: strings.TrimRight(baseURL, "/"), + token: token, + allowInsecureHTTP: cfg.allowInsecureHTTP, httpClient: &http.Client{ Timeout: 30 * time.Second, CheckRedirect: defaultCheckRedirect, @@ -215,10 +268,23 @@ 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. +func redactURL(rawURL string) string { + if idx := strings.IndexByte(rawURL, '?'); idx != -1 { + return rawURL[:idx] + "?" + } + return rawURL +} + // 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)) + } + var backoff []time.Duration if c.retryBackoff != nil { backoff = append([]time.Duration(nil), c.retryBackoff...) diff --git a/github/client_test.go b/github/client_test.go index 6477f6f..a7d8fd7 100644 --- a/github/client_test.go +++ b/github/client_test.go @@ -35,7 +35,7 @@ func TestDoRequest_Success(t *testing.T) { })) defer srv.Close() - c := NewClient("test-token", srv.URL) + c := NewClient("test-token", srv.URL, AllowInsecureHTTPForTest()) body, err := c.doGet(context.Background(), srv.URL+"/test") if err != nil { t.Fatalf("unexpected error: %v", err) @@ -60,7 +60,7 @@ func TestDoRequest_429_RetryAfter_IntegerSeconds(t *testing.T) { })) defer srv.Close() - c := NewClient("tok", srv.URL) + c := NewClient("tok", srv.URL, AllowInsecureHTTPForTest()) c.SetRetryBackoff([]time.Duration{0, 0}) body, err := c.doGet(context.Background(), srv.URL+"/test") @@ -94,7 +94,7 @@ func TestDoRequest_429_RetryAfter_HTTPDate(t *testing.T) { })) defer srv.Close() - c := NewClient("tok", srv.URL) + c := NewClient("tok", srv.URL, AllowInsecureHTTPForTest()) c.now = func() time.Time { return fixedNow } // Initial backoff is 0; the HTTP-date parser will compute 1s and override. c.SetRetryBackoff([]time.Duration{0, 0}) @@ -130,7 +130,7 @@ func TestDoRequest_429_RetryAfter_HTTPDate_InPast(t *testing.T) { })) defer srv.Close() - c := NewClient("tok", srv.URL) + c := NewClient("tok", srv.URL, AllowInsecureHTTPForTest()) c.now = func() time.Time { return fixedNow } c.SetRetryBackoff([]time.Duration{0, 0}) @@ -157,7 +157,7 @@ func TestDoRequest_429_NoRetryAfter_UsesDefaultBackoff(t *testing.T) { })) defer srv.Close() - c := NewClient("tok", srv.URL) + c := NewClient("tok", srv.URL, AllowInsecureHTTPForTest()) c.SetRetryBackoff([]time.Duration{0, 0}) body, err := c.doGet(context.Background(), srv.URL+"/test") @@ -187,7 +187,7 @@ func TestDoRequest_429_InvalidRetryAfter_UsesDefaultBackoff(t *testing.T) { })) defer srv.Close() - c := NewClient("tok", srv.URL) + c := NewClient("tok", srv.URL, AllowInsecureHTTPForTest()) c.SetRetryBackoff([]time.Duration{0, 0}) body, err := c.doGet(context.Background(), srv.URL+"/test") @@ -208,7 +208,7 @@ func TestDoRequest_404_NoRetry(t *testing.T) { })) defer srv.Close() - c := NewClient("tok", srv.URL) + c := NewClient("tok", srv.URL, AllowInsecureHTTPForTest()) _, err := c.doGet(context.Background(), srv.URL+"/test") if err == nil { t.Fatal("expected error, got nil") @@ -230,7 +230,7 @@ func TestDoRequest_401_NoRetry(t *testing.T) { })) defer srv.Close() - c := NewClient("tok", srv.URL) + c := NewClient("tok", srv.URL, AllowInsecureHTTPForTest()) _, err := c.doGet(context.Background(), srv.URL+"/test") if err == nil { t.Fatal("expected error, got nil") @@ -260,7 +260,7 @@ func TestDoRequest_ContextCanceled(t *testing.T) { })) defer srv.Close() - c := NewClient("tok", srv.URL) + c := NewClient("tok", srv.URL, AllowInsecureHTTPForTest()) c.SetRetryBackoff([]time.Duration{10 * time.Second, 10 * time.Second}) ctx, cancel := context.WithCancel(context.Background()) @@ -511,3 +511,108 @@ func TestSetHTTPClient_NilRestoresDefault(t *testing.T) { t.Fatal("expected CheckRedirect policy after SetHTTPClient(nil)") } } + +func TestAllowInsecureHTTPForTest_PermitsHTTP(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + w.Write([]byte("ok")) + })) + defer srv.Close() + + c := NewClient("tok", srv.URL, AllowInsecureHTTPForTest()) + body, err := c.doGet(context.Background(), srv.URL+"/test") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if string(body) != "ok" { + t.Errorf("body = %q, want %q", body, "ok") + } +} + +func TestNoInsecureOption_RejectsHTTP(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + t.Fatal("request should not have been sent") + })) + defer srv.Close() + + c := NewClient("tok", srv.URL) + _, err := c.doGet(context.Background(), srv.URL+"/test") + if err == nil { + t.Fatal("expected error for HTTP request without AllowInsecureHTTP") + } + 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) { + t.Fatal("request should not have been sent") + })) + defer srv.Close() + + t.Setenv("REVIEW_BOT_ALLOW_INSECURE", "") + + c := NewClient("tok", srv.URL, AllowInsecureHTTP()) + _, err := c.doGet(context.Background(), srv.URL+"/test") + if err == nil { + t.Fatal("expected error: AllowInsecureHTTP without env var should be rejected") + } + if !strings.Contains(err.Error(), "refusing HTTP request") { + t.Errorf("unexpected error message: %v", err) + } +} + +func TestAllowInsecureHTTP_WithEnvVar_Permitted(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + w.Write([]byte("insecure-ok")) + })) + defer srv.Close() + + t.Setenv("REVIEW_BOT_ALLOW_INSECURE", "1") + + c := NewClient("tok", srv.URL, AllowInsecureHTTP()) + body, err := c.doGet(context.Background(), srv.URL+"/test") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if string(body) != "insecure-ok" { + t.Errorf("body = %q, want %q", body, "insecure-ok") + } +} + +func TestAllowInsecureHTTP_EnvVarNotOne_Rejected(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + t.Fatal("request should not have been sent") + })) + defer srv.Close() + + // "true" is not "1" โ€” strict check + t.Setenv("REVIEW_BOT_ALLOW_INSECURE", "true") + + c := NewClient("tok", srv.URL, AllowInsecureHTTP()) + _, err := c.doGet(context.Background(), srv.URL+"/test") + if err == nil { + t.Fatal("expected error: env var 'true' is not '1'") + } + if !strings.Contains(err.Error(), "refusing HTTP request") { + t.Errorf("unexpected error message: %v", err) + } +} + +func TestRedactURL_WithQuery(t *testing.T) { + got := redactURL("http://localhost:1234/path?secret=token&foo=bar") + want := "http://localhost:1234/path?" + if got != want { + t.Errorf("redactURL = %q, want %q", got, want) + } +} + +func TestRedactURL_NoQuery(t *testing.T) { + got := redactURL("http://localhost:1234/path") + want := "http://localhost:1234/path" + if got != want { + t.Errorf("redactURL = %q, want %q", got, want) + } +} -- 2.47.3 From db7b7e66bf055972647eead67b072b9c999cc214 Mon Sep 17 00:00:00 2001 From: claw Date: Wed, 13 May 2026 11:35:10 -0700 Subject: [PATCH 2/6] fix: use case-insensitive HTTP scheme check and redact userinfo MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address review feedback on PR #113: - MAJOR (both reviews): Replace strings.HasPrefix(reqURL, "http://") with url.Parse + strings.EqualFold for case-insensitive scheme comparison per RFC 3986. Prevents bypass via HTTP:// or Http://. - MINOR (security): Enhance redactURL to strip userinfo component (user:pass@host) in addition to query params, preventing credential leakage in error messages and logs. - NIT (gpt): Remove redundant timer.Stop() after timer.C fires โ€” it's a no-op and the comment was misleading. - Add tests for uppercase/mixed-case HTTP scheme rejection and userinfo redaction. --- github/client.go | 31 +++++++++++++++++++++++-------- github/client_test.go | 40 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 63 insertions(+), 8 deletions(-) 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) + } +} -- 2.47.3 From 64c9d551ba85f4b431989ae3c049a6876a65132e Mon Sep 17 00:00:00 2001 From: claw Date: Wed, 13 May 2026 11:44:28 -0700 Subject: [PATCH 3/6] =?UTF-8?q?fix:=20address=20review=20feedback=20?= =?UTF-8?q?=E2=80=94=20restore=20timer.Stop()=20and=20fix=20test=20spacing?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Restore timer.Stop() no-op in case <-timer.C for symmetry with ctx.Done - Add missing blank line between TestNoInsecureOption_RejectsHTTP and TestNoInsecureOption_RejectsUppercaseHTTP - Remove double blank line before TestAllowInsecureHTTP_WithoutEnvVar_Rejected Resolves review comments from sonnet-review-bot on PR #113. --- github/client.go | 1 + github/client_test.go | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/github/client.go b/github/client.go index a778d9c..dabb51d 100644 --- a/github/client.go +++ b/github/client.go @@ -319,6 +319,7 @@ func (c *Client) doRequest(ctx context.Context, method, reqURL string, accept st timer := time.NewTimer(delay) select { case <-timer.C: + timer.Stop() // no-op after fire; kept for symmetry with the ctx.Done case case <-ctx.Done(): timer.Stop() return nil, ctx.Err() diff --git a/github/client_test.go b/github/client_test.go index 3d5fa4e..6d2afa6 100644 --- a/github/client_test.go +++ b/github/client_test.go @@ -544,6 +544,7 @@ 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") @@ -568,7 +569,6 @@ func TestNoInsecureOption_RejectsMixedCaseHTTP(t *testing.T) { } } - func TestAllowInsecureHTTP_WithoutEnvVar_Rejected(t *testing.T) { srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { t.Fatal("request should not have been sent") -- 2.47.3 From 4c032a3b5354b2ee58cc03a22297c941a3e0d2e8 Mon Sep 17 00:00:00 2001 From: claw Date: Wed, 13 May 2026 11:58:25 -0700 Subject: [PATCH 4/6] fix: move AllowInsecureHTTPForTest to export_test.go, fix gofmt alignment --- github/client.go | 20 +++++--------------- github/export_test.go | 13 +++++++++++++ 2 files changed, 18 insertions(+), 15 deletions(-) create mode 100644 github/export_test.go diff --git a/github/client.go b/github/client.go index dabb51d..718935a 100644 --- a/github/client.go +++ b/github/client.go @@ -96,8 +96,8 @@ type Client struct { // Higher-level exported methods (GetPullRequest, etc.) will use it to // construct request URLs; remove this field if those methods end up // accepting full URLs instead. - baseURL string - token string + baseURL string + token string httpClient *http.Client // allowInsecureHTTP permits requests to HTTP (non-TLS) endpoints. @@ -155,23 +155,13 @@ type clientConfig struct { // environment variable. Without the env var set, the option is silently ignored // and a warning is logged. // -// For tests, prefer AllowInsecureHTTPForTest which bypasses the env gate. +// For tests, use AllowInsecureHTTPForTest (defined in export_test.go) which bypasses the env gate. func AllowInsecureHTTP() ClientOption { return func(cfg *clientConfig) { cfg.allowInsecureHTTP = true } } -// AllowInsecureHTTPForTest permits sending credentials over plaintext HTTP -// without requiring the REVIEW_BOT_ALLOW_INSECURE environment variable. -// This is intended exclusively for test code using httptest.Server. -func AllowInsecureHTTPForTest() ClientOption { - return func(cfg *clientConfig) { - cfg.allowInsecureHTTP = true - cfg.insecureIsTestBypass = true - } -} - // NewClient creates a new GitHub API client. // If baseURL is empty, it defaults to https://api.github.com. // For GitHub Enterprise, pass the API base URL (e.g. https://github.concur.com/api/v3). @@ -196,8 +186,8 @@ func NewClient(token, baseURL string, opts ...ClientOption) *Client { } return &Client{ - baseURL: strings.TrimRight(baseURL, "/"), - token: token, + baseURL: strings.TrimRight(baseURL, "/"), + token: token, allowInsecureHTTP: cfg.allowInsecureHTTP, httpClient: &http.Client{ Timeout: 30 * time.Second, diff --git a/github/export_test.go b/github/export_test.go new file mode 100644 index 0000000..9b82083 --- /dev/null +++ b/github/export_test.go @@ -0,0 +1,13 @@ +package github + +// AllowInsecureHTTPForTest permits sending credentials over plaintext HTTP +// without requiring the REVIEW_BOT_ALLOW_INSECURE environment variable. +// This is intended exclusively for test code using httptest.Server. +// +// Defined in a _test.go file so it is only available to test binaries. +func AllowInsecureHTTPForTest() ClientOption { + return func(cfg *clientConfig) { + cfg.allowInsecureHTTP = true + cfg.insecureIsTestBypass = true + } +} -- 2.47.3 From 6b7f3f6924e384f36d38e1c638672c02e784ed91 Mon Sep 17 00:00:00 2001 From: claw Date: Wed, 13 May 2026 12:48:02 -0700 Subject: [PATCH 5/6] fix: address NIT findings from sonnet review (#113) - Fix AllowInsecureHTTP doc comment: 'silently ignored' -> 'ignored' (it logs a warning) - Remove unnecessary nil guard in redactURL (assigning nil to nil is a no-op) - Add clarifying comment about acceptable double URL parse in doRequest --- github/client.go | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/github/client.go b/github/client.go index 718935a..41ecea8 100644 --- a/github/client.go +++ b/github/client.go @@ -152,7 +152,7 @@ type clientConfig struct { // AllowInsecureHTTP permits sending credentials over plaintext HTTP connections. // In production, this option is gated by the REVIEW_BOT_ALLOW_INSECURE=1 -// environment variable. Without the env var set, the option is silently ignored +// environment variable. Without the env var set, the option is ignored // and a warning is logged. // // For tests, use AllowInsecureHTTPForTest (defined in export_test.go) which bypasses the env gate. @@ -267,9 +267,8 @@ func redactURL(rawURL string) string { if err != nil { return "" } - if u.User != nil { - u.User = nil - } + u.User = nil + if u.RawQuery != "" { u.RawQuery = "" } @@ -281,6 +280,9 @@ func redactURL(rawURL string) string { // 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) { + // NOTE: This parses reqURL a second time (http.NewRequestWithContext parses it + // again internally). Acceptable cost: URL parsing is cheap and threading the + // parsed *url.URL through would complicate the interface for negligible gain. if !c.allowInsecureHTTP { parsed, err := url.Parse(reqURL) if err != nil { -- 2.47.3 From ab2a6c8aeff8d5afbf8aea4cbcbdeab774750ed4 Mon Sep 17 00:00:00 2001 From: claw Date: Wed, 13 May 2026 13:04:23 -0700 Subject: [PATCH 6/6] Address review feedback on PR #113 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Fix AllowInsecureHTTP doc comment: say '_test.go file in the same package' instead of 'export_test.go' (MINOR #1) - Remove dead u.Fragment = "" from redactURL: HTTP requests never carry fragments over the wire per RFC 7230 ยง5.1 (MINOR #2) - Use 127.0.0.1:1 in scheme-rejection tests to make intent clearer that no network call should occur (NIT #3) --- github/client.go | 3 +-- github/client_test.go | 8 ++++---- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/github/client.go b/github/client.go index 41ecea8..227df06 100644 --- a/github/client.go +++ b/github/client.go @@ -155,7 +155,7 @@ type clientConfig struct { // environment variable. Without the env var set, the option is ignored // and a warning is logged. // -// For tests, use AllowInsecureHTTPForTest (defined in export_test.go) which bypasses the env gate. +// For tests, use AllowInsecureHTTPForTest (defined in a _test.go file in the same package) which bypasses the env gate. func AllowInsecureHTTP() ClientOption { return func(cfg *clientConfig) { cfg.allowInsecureHTTP = true @@ -272,7 +272,6 @@ func redactURL(rawURL string) string { if u.RawQuery != "" { u.RawQuery = "" } - u.Fragment = "" return u.String() } diff --git a/github/client_test.go b/github/client_test.go index 6d2afa6..bb2f9f9 100644 --- a/github/client_test.go +++ b/github/client_test.go @@ -547,8 +547,8 @@ func TestNoInsecureOption_RejectsHTTP(t *testing.T) { 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") + c := NewClient("tok", "HTTP://127.0.0.1:1") + _, err := c.doGet(context.Background(), "HTTP://127.0.0.1:1/test") if err == nil { t.Fatal("expected error for uppercase HTTP scheme") } @@ -559,8 +559,8 @@ func TestNoInsecureOption_RejectsUppercaseHTTP(t *testing.T) { 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") + c := NewClient("tok", "Http://127.0.0.1:1") + _, err := c.doGet(context.Background(), "Http://127.0.0.1:1/test") if err == nil { t.Fatal("expected error for mixed-case HTTP scheme") } -- 2.47.3