Address review feedback on PR #113

- Fix AllowInsecureHTTP doc comment: say '_test.go file in the same
  package' instead of 'export_test.go' (MINOR #1)
- Remove dead u.Fragment = "" from redactURL: HTTP requests never
  carry fragments over the wire per RFC 7230 §5.1 (MINOR #2)
- Use 127.0.0.1:1 in scheme-rejection tests to make intent clearer
  that no network call should occur (NIT #3)

github/client.go

@@ -10,6 +10,7 @@ import (
 	"io"
 	"log/slog"
 	"net/http"
+	"net/url"
 	"os"
 	"strconv"
 	"strings"
@@ -95,8 +96,8 @@ 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.
@@ -151,26 +152,16 @@ type clientConfig struct {
 
 // 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 silently ignored
+// environment variable. Without the env var set, the option is ignored
 // and a warning is logged.
 //
-// For tests, prefer AllowInsecureHTTPForTest which bypasses the env gate.
+// 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
 	}
 }
 
-// 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.
-func AllowInsecureHTTPForTest() ClientOption {
-	return func(cfg *clientConfig) {
-		cfg.allowInsecureHTTP = true
-		cfg.insecureIsTestBypass = 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).
@@ -195,8 +186,8 @@ func NewClient(token, baseURL string, opts ...ClientOption) *Client {
 	}
 
 	return &Client{
-		baseURL:          strings.TrimRight(baseURL, "/"),
-		token:            token,
+		baseURL:           strings.TrimRight(baseURL, "/"),
+		token:             token,
 		allowInsecureHTTP: cfg.allowInsecureHTTP,
 		httpClient: &http.Client{
 			Timeout:       30 * time.Second,
@@ -268,21 +259,37 @@ func (c *Client) parseRetryAfter(value string) (time.Duration, bool) {
 	return 0, false
 }
 
-// redactURL redacts query parameters from a URL for safe inclusion in error
-// messages and log output.
+// 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 {
-	if idx := strings.IndexByte(rawURL, '?'); idx != -1 {
-		return rawURL[:idx] + "?<redacted>"
+	u, err := url.Parse(rawURL)
+	if err != nil {
+		return "<unparseable URL>"
 	}
-	return rawURL
+	u.User = nil
+
+	if u.RawQuery != "" {
+		u.RawQuery = "<redacted>"
+	}
+	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) {
-	if !c.allowInsecureHTTP && strings.HasPrefix(reqURL, "http://") {
-		return nil, fmt.Errorf("refusing HTTP request to %s: use HTTPS or set AllowInsecureHTTP option", redactURL(reqURL))
+	// 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
@@ -303,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()

github/client_test.go

@@ -545,6 +545,30 @@ func TestNoInsecureOption_RejectsHTTP(t *testing.T) {
 	}
 }
 
+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")
@@ -616,3 +640,19 @@ func TestRedactURL_NoQuery(t *testing.T) {
 		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?<redacted>"
+	if got != want {
+		t.Errorf("redactURL = %q, want %q", got, want)
+	}
+}

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
+	}
+}