From 91f31ff2d7603d3f09fbd5e1492a37829394cd77 Mon Sep 17 00:00:00 2001
From: claw <claw@weiker.me>
Date: Wed, 13 May 2026 10:48:16 -0700
Subject: [PATCH] address review feedback: export_test.go for
 AllowInsecureHTTPForTest, avoid url.Parse in doRequest
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

MINOR #1: Move AllowInsecureHTTPForTest to export_test.go so it is only
available in test binaries and does not pollute the production API surface.

MINOR #2: Replace url.Parse with a strings.EqualFold prefix check in
doRequest's HTTPS enforcement, avoiding a per-request allocation.

NIT #3: Push back — slog.Warn on ignored AllowInsecureHTTP is a deliberate
design choice that helps operators debug 'refusing to send credentials'
errors when the env gate is not set.
---
 github/client.go      | 27 ++++++++++-----------------
 github/export_test.go | 14 ++++++++++++++
 2 files changed, 24 insertions(+), 17 deletions(-)
 create mode 100644 github/export_test.go

diff --git a/github/client.go b/github/client.go
index d5ffbc0..9eaad24 100644
--- a/github/client.go
+++ b/github/client.go
@@ -10,7 +10,6 @@ import (
 	"io"
 	"log/slog"
 	"net/http"
-	"net/url"
 	"os"
 	"strconv"
 	"strings"
@@ -108,16 +107,6 @@ func AllowInsecureHTTP() ClientOption {
 	}
 }
 
-// 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
@@ -173,7 +162,7 @@ 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).
-// The baseURL must use HTTPS unless AllowInsecureHTTP() or AllowInsecureHTTPForTest()
+// The baseURL must use HTTPS unless AllowInsecureHTTP() or AllowInsecureHTTPForTest() (test-only)
 // is passed as an option.
 func NewClient(token, baseURL string, opts ...ClientOption) *Client {
 	if baseURL == "" {
@@ -273,17 +262,21 @@ func (c *Client) parseRetryAfter(value string) (time.Duration, bool) {
 	return 0, false
 }
 
+// hasHTTPSScheme reports whether rawURL starts with "https://" (case-insensitive).
+// It avoids the allocation of url.Parse for a simple scheme check.
+func hasHTTPSScheme(rawURL string) bool {
+	const prefix = "https://"
+	return len(rawURL) >= len(prefix) && strings.EqualFold(rawURL[:len(prefix)], prefix)
+}
+
 // 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) {
 	// Reject non-HTTPS URLs when credentials are present and insecure mode is not enabled.
+	// Uses a string prefix check instead of url.Parse to avoid per-request allocations.
 	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") {
+		if !hasHTTPSScheme(reqURL) {
 			return nil, fmt.Errorf("refusing to send credentials over non-HTTPS URL %q (use AllowInsecureHTTP option for trusted networks)", reqURL)
 		}
 	}
diff --git a/github/export_test.go b/github/export_test.go
new file mode 100644
index 0000000..a609c84
--- /dev/null
+++ b/github/export_test.go
@@ -0,0 +1,14 @@
+package github
+
+// 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.
+//
+// This function lives in export_test.go so it is only available to test
+// binaries and does not appear in the production API surface.
+func AllowInsecureHTTPForTest() ClientOption {
+	return func(c *clientConfig) {
+		c.allowInsecureHTTP = true
+		c.testBypass = true
+	}
+}