From 06b92a68348a8f3341552c7beee1ca9afb3d01e8 Mon Sep 17 00:00:00 2001 From: claw Date: Wed, 13 May 2026 11:09:57 -0700 Subject: [PATCH] address review feedback: rename to With* convention, extract env const, redact query params, fix misleading test --- github/client.go | 30 +++++++++++++-------- github/client_test.go | 61 +++++++++++++++++++++++++++++-------------- github/export_test.go | 4 +-- 3 files changed, 62 insertions(+), 33 deletions(-) diff --git a/github/client.go b/github/client.go index 9eaad24..5520fe4 100644 --- a/github/client.go +++ b/github/client.go @@ -33,6 +33,9 @@ const ( // maxResponseBodyBytes limits how much of a successful response body we read // for defense-in-depth against servers returning excessively large payloads. maxResponseBodyBytes = 10 * 1024 * 1024 // 10 MB + + // envAllowInsecure is the environment variable that gates the WithAllowInsecureHTTP option. + envAllowInsecure = "REVIEW_BOT_ALLOW_INSECURE" ) // APIError represents an HTTP error response from the GitHub API. @@ -95,13 +98,13 @@ type clientConfig struct { // ClientOption configures optional behavior of NewClient. type ClientOption func(*clientConfig) -// AllowInsecureHTTP permits the client to send credentials over HTTP (non-TLS) URLs. +// WithAllowInsecureHTTP permits the client to send credentials over HTTP (non-TLS) URLs. // This is gated by the REVIEW_BOT_ALLOW_INSECURE=1 environment variable as a // defense-in-depth safeguard. If the env var is not set, the option is ignored // and a warning is logged. // // For production use on trusted internal networks only. -func AllowInsecureHTTP() ClientOption { +func WithAllowInsecureHTTP() ClientOption { return func(c *clientConfig) { c.allowInsecureHTTP = true } @@ -162,7 +165,7 @@ func defaultCheckRedirect(req *http.Request, via []*http.Request) error { // 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). -// The baseURL must use HTTPS unless AllowInsecureHTTP() or AllowInsecureHTTPForTest() (test-only) +// The baseURL must use HTTPS unless WithAllowInsecureHTTP() or WithAllowInsecureHTTPForTest() (test-only) // is passed as an option. func NewClient(token, baseURL string, opts ...ClientOption) *Client { if baseURL == "" { @@ -173,18 +176,18 @@ func NewClient(token, baseURL string, opts ...ClientOption) *Client { o(&cfg) } - // Environment gate for AllowInsecureHTTP (defense-in-depth). - // AllowInsecureHTTPForTest bypasses this gate for test convenience. + // Environment gate for WithAllowInsecureHTTP (defense-in-depth). + // WithAllowInsecureHTTPForTest bypasses this gate for test convenience. allowInsecure := false if cfg.allowInsecureHTTP { if cfg.testBypass { allowInsecure = true - } else if os.Getenv("REVIEW_BOT_ALLOW_INSECURE") == "1" { + } else if os.Getenv(envAllowInsecure) == "1" { allowInsecure = true - slog.Warn("AllowInsecureHTTP enabled — credentials may be sent over plaintext", - "env", "REVIEW_BOT_ALLOW_INSECURE=1") + slog.Warn("WithAllowInsecureHTTP enabled — credentials may be sent over plaintext", + "env", envAllowInsecure+"=1") } else { - slog.Warn("AllowInsecureHTTP option ignored: set REVIEW_BOT_ALLOW_INSECURE=1 to enable") + slog.Warn("WithAllowInsecureHTTP option ignored: set "+envAllowInsecure+"=1 to enable") } } @@ -266,7 +269,7 @@ func (c *Client) parseRetryAfter(value string) (time.Duration, bool) { // It avoids the allocation of url.Parse for a simple scheme check. func hasHTTPSScheme(rawURL string) bool { const prefix = "https://" - return len(rawURL) >= len(prefix) && strings.EqualFold(rawURL[:len(prefix)], prefix) + return len(rawURL) >= len(prefix) && strings.HasPrefix(strings.ToLower(rawURL[:len(prefix)]), prefix) } // doRequest performs an HTTP request with retry on 429 rate limit responses. @@ -277,7 +280,12 @@ func (c *Client) doRequest(ctx context.Context, method, reqURL string, accept st // Uses a string prefix check instead of url.Parse to avoid per-request allocations. if c.token != "" && !c.allowInsecureHTTP { if !hasHTTPSScheme(reqURL) { - return nil, fmt.Errorf("refusing to send credentials over non-HTTPS URL %q (use AllowInsecureHTTP option for trusted networks)", reqURL) + // Redact query parameters to avoid leaking sensitive data in error messages. + sanitized := reqURL + if i := strings.Index(reqURL, "?"); i >= 0 { + sanitized = reqURL[:i] + "?[REDACTED]" + } + return nil, fmt.Errorf("refusing to send credentials over non-HTTPS URL %q (use WithAllowInsecureHTTP option for trusted networks)", sanitized) } } diff --git a/github/client_test.go b/github/client_test.go index 88d8a1e..87fbcab 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, AllowInsecureHTTPForTest()) + c := NewClient("test-token", srv.URL, WithAllowInsecureHTTPForTest()) 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, AllowInsecureHTTPForTest()) + c := NewClient("tok", srv.URL, WithAllowInsecureHTTPForTest()) 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, AllowInsecureHTTPForTest()) + c := NewClient("tok", srv.URL, WithAllowInsecureHTTPForTest()) 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, AllowInsecureHTTPForTest()) + c := NewClient("tok", srv.URL, WithAllowInsecureHTTPForTest()) 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, AllowInsecureHTTPForTest()) + c := NewClient("tok", srv.URL, WithAllowInsecureHTTPForTest()) 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, AllowInsecureHTTPForTest()) + c := NewClient("tok", srv.URL, WithAllowInsecureHTTPForTest()) 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, AllowInsecureHTTPForTest()) + c := NewClient("tok", srv.URL, WithAllowInsecureHTTPForTest()) _, 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, AllowInsecureHTTPForTest()) + c := NewClient("tok", srv.URL, WithAllowInsecureHTTPForTest()) _, 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, AllowInsecureHTTPForTest()) + c := NewClient("tok", srv.URL, WithAllowInsecureHTTPForTest()) c.SetRetryBackoff([]time.Duration{10 * time.Second, 10 * time.Second}) ctx, cancel := context.WithCancel(context.Background()) @@ -519,7 +519,7 @@ func TestDoRequest_RejectsHTTPWithToken(t *testing.T) { })) defer srv.Close() - // Without AllowInsecureHTTP, should refuse to send token over HTTP + // Without WithAllowInsecureHTTP, should refuse to send token over HTTP c := NewClient("secret-token", srv.URL) _, err := c.doGet(context.Background(), srv.URL+"/test") if err == nil { @@ -530,6 +530,27 @@ func TestDoRequest_RejectsHTTPWithToken(t *testing.T) { } } + +func TestDoRequest_RejectsHTTPWithToken_RedactsQueryParams(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(200) + w.Write([]byte("{}")) + })) + defer srv.Close() + + c := NewClient("secret-token", srv.URL) + _, err := c.doGet(context.Background(), srv.URL+"/test?secret=hunter2&token=abc") + if err == nil { + t.Fatal("expected error when sending token over HTTP") + } + errMsg := err.Error() + if strings.Contains(errMsg, "hunter2") || strings.Contains(errMsg, "token=abc") { + t.Errorf("error message should not contain query params, got: %v", errMsg) + } + if !strings.Contains(errMsg, "?[REDACTED]") { + t.Errorf("error message should contain redacted marker, got: %v", errMsg) + } +} func TestDoRequest_AllowsHTTPWithoutToken(t *testing.T) { srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(200) @@ -538,7 +559,7 @@ func TestDoRequest_AllowsHTTPWithoutToken(t *testing.T) { defer srv.Close() // Without token, HTTP should be fine (no credentials to leak) - c := NewClient("", srv.URL, AllowInsecureHTTPForTest()) + c := NewClient("", srv.URL) body, err := c.doGet(context.Background(), srv.URL+"/test") if err != nil { t.Fatalf("unexpected error: %v", err) @@ -555,7 +576,7 @@ func TestDoRequest_AllowsHTTPWithInsecureTestOption(t *testing.T) { })) defer srv.Close() - c := NewClient("secret-token", srv.URL, AllowInsecureHTTPForTest()) + c := NewClient("secret-token", srv.URL, WithAllowInsecureHTTPForTest()) body, err := c.doGet(context.Background(), srv.URL+"/test") if err != nil { t.Fatalf("unexpected error: %v", err) @@ -565,7 +586,7 @@ func TestDoRequest_AllowsHTTPWithInsecureTestOption(t *testing.T) { } } -func TestAllowInsecureHTTP_RequiresEnvVar(t *testing.T) { +func TestWithAllowInsecureHTTP_RequiresEnvVar(t *testing.T) { srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(200) w.Write([]byte(`{"ok":true}`)) @@ -574,17 +595,17 @@ func TestAllowInsecureHTTP_RequiresEnvVar(t *testing.T) { // Without the env var, AllowInsecureHTTP should be ignored t.Setenv("REVIEW_BOT_ALLOW_INSECURE", "") - c := NewClient("secret-token", srv.URL, AllowInsecureHTTP()) + c := NewClient("secret-token", srv.URL, WithAllowInsecureHTTP()) _, err := c.doGet(context.Background(), srv.URL+"/test") if err == nil { - t.Fatal("expected error: AllowInsecureHTTP without env var should be rejected") + t.Fatal("expected error: WithAllowInsecureHTTP without env var should be rejected") } if !strings.Contains(err.Error(), "refusing to send credentials") { t.Errorf("unexpected error: %v", err) } } -func TestAllowInsecureHTTP_WithEnvVar(t *testing.T) { +func TestWithAllowInsecureHTTP_WithEnvVar(t *testing.T) { srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(200) w.Write([]byte(`{"ok":true}`)) @@ -592,7 +613,7 @@ func TestAllowInsecureHTTP_WithEnvVar(t *testing.T) { defer srv.Close() t.Setenv("REVIEW_BOT_ALLOW_INSECURE", "1") - c := NewClient("secret-token", srv.URL, AllowInsecureHTTP()) + c := NewClient("secret-token", srv.URL, WithAllowInsecureHTTP()) body, err := c.doGet(context.Background(), srv.URL+"/test") if err != nil { t.Fatalf("unexpected error: %v", err) @@ -602,7 +623,7 @@ func TestAllowInsecureHTTP_WithEnvVar(t *testing.T) { } } -func TestAllowInsecureHTTP_EnvVarMustBeExactlyOne(t *testing.T) { +func TestWithAllowInsecureHTTP_EnvVarMustBeExactlyOne(t *testing.T) { srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(200) w.Write([]byte(`{"ok":true}`)) @@ -611,9 +632,9 @@ func TestAllowInsecureHTTP_EnvVarMustBeExactlyOne(t *testing.T) { // "true" is not "1" — should be rejected t.Setenv("REVIEW_BOT_ALLOW_INSECURE", "true") - c := NewClient("secret-token", srv.URL, AllowInsecureHTTP()) + c := NewClient("secret-token", srv.URL, WithAllowInsecureHTTP()) _, err := c.doGet(context.Background(), srv.URL+"/test") if err == nil { - t.Fatal("expected error: REVIEW_BOT_ALLOW_INSECURE=true should not satisfy gate") + t.Fatal("expected error: env var must be exactly \"1\" to satisfy gate") } } diff --git a/github/export_test.go b/github/export_test.go index a609c84..89e5340 100644 --- a/github/export_test.go +++ b/github/export_test.go @@ -1,12 +1,12 @@ package github -// AllowInsecureHTTPForTest permits the client to send credentials over HTTP +// WithAllowInsecureHTTPForTest permits the client to send credentials over HTTP // without requiring the REVIEW_BOT_ALLOW_INSECURE environment variable. // This is intended exclusively for tests using httptest.Server. // // This function lives in export_test.go so it is only available to test // binaries and does not appear in the production API surface. -func AllowInsecureHTTPForTest() ClientOption { +func WithAllowInsecureHTTPForTest() ClientOption { return func(c *clientConfig) { c.allowInsecureHTTP = true c.testBypass = true