address review feedback: rename to With* convention, extract env const, redact query params, fix misleading test

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).
+	// Environment gate for WithAllowInsecureHTTP (defense-in-depth).
-	// AllowInsecureHTTPForTest bypasses this gate for test convenience.
+	// 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",
+			slog.Warn("WithAllowInsecureHTTP enabled — credentials may be sent over plaintext",
-				"env", "REVIEW_BOT_ALLOW_INSECURE=1")
+				"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)
 		}
 	}
 

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")
 	}
 }

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