fix(github): address sonnet review feedback on PR #113

- hasHTTPSScheme: use strings.EqualFold instead of ToLower+HasPrefix
- slog.Warn: move hint into structured attribute for idiomatic usage
- client_test.go: fix blank line formatting between test functions
- clientConfig.testBypass: strengthen comment to reference export_test.go

github/client.go

@@ -8,7 +8,9 @@ import (
 	"errors"
 	"fmt"
 	"io"
+	"log/slog"
 	"net/http"
+	"os"
 	"strconv"
 	"strings"
 	"time"
@@ -31,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.
@@ -84,6 +89,27 @@ func asAPIError(err error) (*APIError, bool) {
 	return nil, false
 }
 
+// clientConfig holds optional configuration for NewClient.
+type clientConfig struct {
+	allowInsecureHTTP bool
+	testBypass        bool // skip env gate; only WithAllowInsecureHTTPForTest (export_test.go) should set this
+}
+
+// ClientOption configures optional behavior of NewClient.
+type ClientOption func(*clientConfig)
+
+// 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 WithAllowInsecureHTTP() ClientOption {
+	return func(c *clientConfig) {
+		c.allowInsecureHTTP = true
+	}
+}
+
 // Client interacts with the GitHub API.
 // A Client is safe for concurrent use by multiple goroutines.
 // SetHTTPClient and SetRetryBackoff are intended for test setup only and must
@@ -93,9 +119,10 @@ 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
-	httpClient *http.Client
+	baseURL           string
+	token             string
+	allowInsecureHTTP bool
+	httpClient        *http.Client
 
 	// retryBackoff defines the delays between retry attempts for 429 responses.
 	// retryBackoff[i] is the delay before attempt i+1 (after attempt i fails).
@@ -138,13 +165,36 @@ 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).
-func NewClient(token, baseURL string) *Client {
+// 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 == "" {
 		baseURL = defaultBaseURL
 	}
+	var cfg clientConfig
+	for _, o := range opts {
+		o(&cfg)
+	}
+
+	// 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(envAllowInsecure) == "1" {
+			allowInsecure = true
+			slog.Warn("WithAllowInsecureHTTP enabled — credentials may be sent over plaintext",
+				"env", envAllowInsecure+"=1")
+		} else {
+			slog.Warn("WithAllowInsecureHTTP option ignored", "hint", "set "+envAllowInsecure+"=1 to enable")
+		}
+	}
+
 	return &Client{
-		baseURL: strings.TrimRight(baseURL, "/"),
-		token:   token,
+		baseURL:           strings.TrimRight(baseURL, "/"),
+		token:             token,
+		allowInsecureHTTP: allowInsecure,
 		httpClient: &http.Client{
 			Timeout:       30 * time.Second,
 			CheckRedirect: defaultCheckRedirect,
@@ -215,10 +265,30 @@ func (c *Client) parseRetryAfter(value string) (time.Duration, bool) {
 	return 0, false
 }
 
+// hasHTTPSScheme reports whether rawURL starts with "https://" (case-insensitive).
+// 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)
+}
+
 // 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) {
+	// Reject non-HTTPS URLs when credentials are present and insecure mode is not enabled.
+	// Uses a string prefix check instead of url.Parse to avoid per-request allocations.
+	if c.token != "" && !c.allowInsecureHTTP {
+		if !hasHTTPSScheme(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)
+		}
+	}
+
 	var backoff []time.Duration
 	if c.retryBackoff != nil {
 		backoff = append([]time.Duration(nil), c.retryBackoff...)

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, 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)
+	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)
+	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)
+	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)
+	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)
+	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)
+	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)
+	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)
+	c := NewClient("tok", srv.URL, WithAllowInsecureHTTPForTest())
 	c.SetRetryBackoff([]time.Duration{10 * time.Second, 10 * time.Second})
 
 	ctx, cancel := context.WithCancel(context.Background())
@@ -511,3 +511,130 @@ func TestSetHTTPClient_NilRestoresDefault(t *testing.T) {
 		t.Fatal("expected CheckRedirect policy after SetHTTPClient(nil)")
 	}
 }
+
+func TestDoRequest_RejectsHTTPWithToken(t *testing.T) {
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		w.WriteHeader(200)
+		w.Write([]byte("{}"))
+	}))
+	defer srv.Close()
+
+	// 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 {
+		t.Fatal("expected error when sending token over HTTP")
+	}
+	if !strings.Contains(err.Error(), "refusing to send credentials") {
+		t.Errorf("unexpected error message: %v", err)
+	}
+}
+
+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)
+		w.Write([]byte(`{"ok":true}`))
+	}))
+	defer srv.Close()
+
+	// Without token, HTTP should be fine (no credentials to leak)
+	c := NewClient("", srv.URL)
+	body, err := c.doGet(context.Background(), srv.URL+"/test")
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	if string(body) != `{"ok":true}` {
+		t.Errorf("unexpected body: %s", body)
+	}
+}
+
+func TestDoRequest_AllowsHTTPWithInsecureTestOption(t *testing.T) {
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		w.WriteHeader(200)
+		w.Write([]byte(`{"ok":true}`))
+	}))
+	defer srv.Close()
+
+	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)
+	}
+	if string(body) != `{"ok":true}` {
+		t.Errorf("unexpected body: %s", body)
+	}
+}
+
+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}`))
+	}))
+	defer srv.Close()
+
+	// Without the env var, AllowInsecureHTTP should be ignored
+	t.Setenv("REVIEW_BOT_ALLOW_INSECURE", "")
+	c := NewClient("secret-token", srv.URL, WithAllowInsecureHTTP())
+	_, err := c.doGet(context.Background(), srv.URL+"/test")
+	if err == nil {
+		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 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}`))
+	}))
+	defer srv.Close()
+
+	t.Setenv("REVIEW_BOT_ALLOW_INSECURE", "1")
+	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)
+	}
+	if string(body) != `{"ok":true}` {
+		t.Errorf("unexpected body: %s", body)
+	}
+}
+
+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}`))
+	}))
+	defer srv.Close()
+
+	// "true" is not "1" — should be rejected
+	t.Setenv("REVIEW_BOT_ALLOW_INSECURE", "true")
+	c := NewClient("secret-token", srv.URL, WithAllowInsecureHTTP())
+	_, err := c.doGet(context.Background(), srv.URL+"/test")
+	if err == nil {
+		t.Fatal("expected error: env var must be exactly \"1\" to satisfy gate")
+	}
+}

github/export_test.go

@@ -0,0 +1,14 @@
+package github
+
+// 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 WithAllowInsecureHTTPForTest() ClientOption {
+	return func(c *clientConfig) {
+		c.allowInsecureHTTP = true
+		c.testBypass = true
+	}
+}