diff --git a/github/client.go b/github/client.go index 041a49c..227df06 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" @@ -93,10 +96,14 @@ 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 + baseURL string + token string httpClient *http.Client + // allowInsecureHTTP permits requests to HTTP (non-TLS) endpoints. + // When false, doRequest rejects URLs with an http:// scheme. + allowInsecureHTTP bool + // retryBackoff defines the delays between retry attempts for 429 responses. // retryBackoff[i] is the delay before attempt i+1 (after attempt i fails). // If nil, defaults to {1s, 2s}. @@ -135,16 +142,53 @@ func defaultCheckRedirect(req *http.Request, via []*http.Request) error { return nil } +// ClientOption configures optional behavior of a Client. +type ClientOption func(*clientConfig) + +type clientConfig struct { + allowInsecureHTTP bool + insecureIsTestBypass bool +} + +// AllowInsecureHTTP permits sending credentials over plaintext HTTP connections. +// In production, this option is gated by the REVIEW_BOT_ALLOW_INSECURE=1 +// environment variable. Without the env var set, the option is ignored +// and a warning is logged. +// +// For tests, use AllowInsecureHTTPForTest (defined in a _test.go file in the same package) which bypasses the env gate. +func AllowInsecureHTTP() ClientOption { + return func(cfg *clientConfig) { + cfg.allowInsecureHTTP = true + } +} + // 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 { +func NewClient(token, baseURL string, opts ...ClientOption) *Client { if baseURL == "" { baseURL = defaultBaseURL } + + var cfg clientConfig + for _, opt := range opts { + opt(&cfg) + } + + if cfg.allowInsecureHTTP && !cfg.insecureIsTestBypass { + if os.Getenv("REVIEW_BOT_ALLOW_INSECURE") != "1" { + slog.Warn("AllowInsecureHTTP ignored: set REVIEW_BOT_ALLOW_INSECURE=1 to enable") + cfg.allowInsecureHTTP = false + } else { + slog.Warn("AllowInsecureHTTP enabled — credentials may be sent over plaintext", + "env", "REVIEW_BOT_ALLOW_INSECURE=1") + } + } + return &Client{ - baseURL: strings.TrimRight(baseURL, "/"), - token: token, + baseURL: strings.TrimRight(baseURL, "/"), + token: token, + allowInsecureHTTP: cfg.allowInsecureHTTP, httpClient: &http.Client{ Timeout: 30 * time.Second, CheckRedirect: defaultCheckRedirect, @@ -215,10 +259,39 @@ func (c *Client) parseRetryAfter(value string) (time.Duration, bool) { return 0, false } +// redactURL redacts sensitive components from a URL for safe inclusion in error +// messages and log output. It removes userinfo (e.g., user:pass@) and replaces +// query parameters with a placeholder. +func redactURL(rawURL string) string { + u, err := url.Parse(rawURL) + if err != nil { + return "" + } + u.User = nil + + if u.RawQuery != "" { + u.RawQuery = "" + } + return u.String() +} + // 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) { + // NOTE: This parses reqURL a second time (http.NewRequestWithContext parses it + // again internally). Acceptable cost: URL parsing is cheap and threading the + // parsed *url.URL through would complicate the interface for negligible gain. + if !c.allowInsecureHTTP { + parsed, err := url.Parse(reqURL) + if err != nil { + return nil, fmt.Errorf("parse request URL: %w", err) + } + if strings.EqualFold(parsed.Scheme, "http") { + return nil, fmt.Errorf("refusing HTTP request to %s: use HTTPS or set AllowInsecureHTTP option", redactURL(reqURL)) + } + } + var backoff []time.Duration if c.retryBackoff != nil { backoff = append([]time.Duration(nil), c.retryBackoff...) @@ -237,7 +310,7 @@ func (c *Client) doRequest(ctx context.Context, method, reqURL string, accept st timer := time.NewTimer(delay) select { case <-timer.C: - timer.Stop() + timer.Stop() // no-op after fire; kept for symmetry with the ctx.Done case case <-ctx.Done(): timer.Stop() return nil, ctx.Err() diff --git a/github/client_test.go b/github/client_test.go index 6477f6f..bb2f9f9 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,148 @@ func TestSetHTTPClient_NilRestoresDefault(t *testing.T) { t.Fatal("expected CheckRedirect policy after SetHTTPClient(nil)") } } + +func TestAllowInsecureHTTPForTest_PermitsHTTP(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + w.Write([]byte("ok")) + })) + defer srv.Close() + + c := NewClient("tok", 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" { + t.Errorf("body = %q, want %q", body, "ok") + } +} + +func TestNoInsecureOption_RejectsHTTP(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + t.Fatal("request should not have been sent") + })) + defer srv.Close() + + c := NewClient("tok", srv.URL) + _, err := c.doGet(context.Background(), srv.URL+"/test") + if err == nil { + t.Fatal("expected error for HTTP request without AllowInsecureHTTP") + } + if !strings.Contains(err.Error(), "refusing HTTP request") { + t.Errorf("unexpected error message: %v", err) + } +} + +func TestNoInsecureOption_RejectsUppercaseHTTP(t *testing.T) { + // Verify case-insensitive scheme check (RFC 3986). + c := NewClient("tok", "HTTP://127.0.0.1:1") + _, err := c.doGet(context.Background(), "HTTP://127.0.0.1:1/test") + if err == nil { + t.Fatal("expected error for uppercase HTTP scheme") + } + if !strings.Contains(err.Error(), "refusing HTTP request") { + t.Errorf("unexpected error message: %v", err) + } +} + +func TestNoInsecureOption_RejectsMixedCaseHTTP(t *testing.T) { + // Verify mixed case like "Http://" is also rejected. + c := NewClient("tok", "Http://127.0.0.1:1") + _, err := c.doGet(context.Background(), "Http://127.0.0.1:1/test") + if err == nil { + t.Fatal("expected error for mixed-case HTTP scheme") + } + if !strings.Contains(err.Error(), "refusing HTTP request") { + t.Errorf("unexpected error message: %v", err) + } +} + +func TestAllowInsecureHTTP_WithoutEnvVar_Rejected(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + t.Fatal("request should not have been sent") + })) + defer srv.Close() + + t.Setenv("REVIEW_BOT_ALLOW_INSECURE", "") + + c := NewClient("tok", 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 HTTP request") { + t.Errorf("unexpected error message: %v", err) + } +} + +func TestAllowInsecureHTTP_WithEnvVar_Permitted(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + w.Write([]byte("insecure-ok")) + })) + defer srv.Close() + + t.Setenv("REVIEW_BOT_ALLOW_INSECURE", "1") + + c := NewClient("tok", srv.URL, AllowInsecureHTTP()) + body, err := c.doGet(context.Background(), srv.URL+"/test") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if string(body) != "insecure-ok" { + t.Errorf("body = %q, want %q", body, "insecure-ok") + } +} + +func TestAllowInsecureHTTP_EnvVarNotOne_Rejected(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + t.Fatal("request should not have been sent") + })) + defer srv.Close() + + // "true" is not "1" — strict check + t.Setenv("REVIEW_BOT_ALLOW_INSECURE", "true") + + c := NewClient("tok", srv.URL, AllowInsecureHTTP()) + _, err := c.doGet(context.Background(), srv.URL+"/test") + if err == nil { + t.Fatal("expected error: env var 'true' is not '1'") + } + if !strings.Contains(err.Error(), "refusing HTTP request") { + t.Errorf("unexpected error message: %v", err) + } +} + +func TestRedactURL_WithQuery(t *testing.T) { + got := redactURL("http://localhost:1234/path?secret=token&foo=bar") + want := "http://localhost:1234/path?" + if got != want { + t.Errorf("redactURL = %q, want %q", got, want) + } +} + +func TestRedactURL_NoQuery(t *testing.T) { + got := redactURL("http://localhost:1234/path") + want := "http://localhost:1234/path" + if got != want { + t.Errorf("redactURL = %q, want %q", got, want) + } +} + +func TestRedactURL_Userinfo(t *testing.T) { + got := redactURL("http://user:pass@localhost:1234/path") + want := "http://localhost:1234/path" + if got != want { + t.Errorf("redactURL = %q, want %q", got, want) + } +} + +func TestRedactURL_UserinfoWithQuery(t *testing.T) { + got := redactURL("http://user:pass@localhost:1234/path?secret=token") + want := "http://localhost:1234/path?" + if got != want { + t.Errorf("redactURL = %q, want %q", got, want) + } +} diff --git a/github/export_test.go b/github/export_test.go new file mode 100644 index 0000000..9b82083 --- /dev/null +++ b/github/export_test.go @@ -0,0 +1,13 @@ +package github + +// AllowInsecureHTTPForTest permits sending credentials over plaintext HTTP +// without requiring the REVIEW_BOT_ALLOW_INSECURE environment variable. +// This is intended exclusively for test code using httptest.Server. +// +// Defined in a _test.go file so it is only available to test binaries. +func AllowInsecureHTTPForTest() ClientOption { + return func(cfg *clientConfig) { + cfg.allowInsecureHTTP = true + cfg.insecureIsTestBypass = true + } +}