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...)