feat(github): add safeguards against accidental AllowInsecureHTTP use (#96)

Add defense-in-depth for the AllowInsecureHTTP client option:

1. HTTPS enforcement: doRequest now rejects non-HTTPS URLs when
   credentials are present and insecure mode is not explicitly enabled.

2. Environment gate: AllowInsecureHTTP() requires REVIEW_BOT_ALLOW_INSECURE=1
   env var. Without it, the option is silently ignored and a warning is logged.
   This prevents accidental enablement from config drift.

3. Warning on activation: When the env gate IS satisfied, a slog.Warn fires
   at client construction time so operators notice in logs.

4. Test bypass: AllowInsecureHTTPForTest() skips the env gate for test
   convenience with httptest.Server, keeping tests clean.

Closes #96

github/client.go

@@ -8,7 +8,10 @@ import (
 	"errors"
 	"fmt"
 	"io"
+	"log/slog"
 	"net/http"
+	"net/url"
+	"os"
 	"strconv"
 	"strings"
 	"time"
@@ -84,6 +87,37 @@ 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 (for tests only)
+}
+
+// ClientOption configures optional behavior of NewClient.
+type ClientOption func(*clientConfig)
+
+// AllowInsecureHTTP 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 {
+	return func(c *clientConfig) {
+		c.allowInsecureHTTP = true
+	}
+}
+
+// AllowInsecureHTTPForTest 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.
+func AllowInsecureHTTPForTest() ClientOption {
+	return func(c *clientConfig) {
+		c.allowInsecureHTTP = true
+		c.testBypass = 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 +127,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 +173,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 AllowInsecureHTTP() or AllowInsecureHTTPForTest()
+// 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 AllowInsecureHTTP (defense-in-depth).
+	// AllowInsecureHTTPForTest 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" {
+			allowInsecure = true
+			slog.Warn("AllowInsecureHTTP enabled — credentials may be sent over plaintext",
+				"env", "REVIEW_BOT_ALLOW_INSECURE=1")
+		} else {
+			slog.Warn("AllowInsecureHTTP option ignored: set REVIEW_BOT_ALLOW_INSECURE=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,
@@ -219,6 +277,17 @@ func (c *Client) parseRetryAfter(value string) (time.Duration, bool) {
 // 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.
+	if c.token != "" && !c.allowInsecureHTTP {
+		parsed, err := url.Parse(reqURL)
+		if err != nil {
+			return nil, fmt.Errorf("parse request URL: %w", err)
+		}
+		if !strings.EqualFold(parsed.Scheme, "https") {
+			return nil, fmt.Errorf("refusing to send credentials over non-HTTPS URL %q (use AllowInsecureHTTP option for trusted networks)", reqURL)
+		}
+	}
+
 	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, 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,109 @@ 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 AllowInsecureHTTP, 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_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, AllowInsecureHTTPForTest())
+	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, AllowInsecureHTTPForTest())
+	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 TestAllowInsecureHTTP_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, 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 to send credentials") {
+		t.Errorf("unexpected error: %v", err)
+	}
+}
+
+func TestAllowInsecureHTTP_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, AllowInsecureHTTP())
+	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 TestAllowInsecureHTTP_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, AllowInsecureHTTP())
+	_, err := c.doGet(context.Background(), srv.URL+"/test")
+	if err == nil {
+		t.Fatal("expected error: REVIEW_BOT_ALLOW_INSECURE=true should not satisfy gate")
+	}
+}