From ce48dc0ec64e5ddbc0243bce590357b734e221ab Mon Sep 17 00:00:00 2001 From: claw Date: Wed, 13 May 2026 10:31:17 -0700 Subject: [PATCH] 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 | 81 +++++++++++++++++++++++++-- github/client_test.go | 124 +++++++++++++++++++++++++++++++++++++++--- 2 files changed, 190 insertions(+), 15 deletions(-) diff --git a/github/client.go b/github/client.go index 041a49c..d5ffbc0 100644 --- a/github/client.go +++ b/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...) diff --git a/github/client_test.go b/github/client_test.go index 6477f6f..88d8a1e 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) + 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") + } +}