address review feedback: export_test.go for AllowInsecureHTTPForTest, avoid url.Parse in doRequest
PR Ready Gate / clear-labels (pull_request) Successful in 2s
CI / test (pull_request) Successful in 24s
CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 34s
CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 1m43s
CI / review (gpt-5, security, ., rodin/security-patterns, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 1m49s
PR Ready Gate / clear-labels (pull_request) Successful in 2s
CI / test (pull_request) Successful in 24s
CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 34s
CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 1m43s
CI / review (gpt-5, security, ., rodin/security-patterns, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 1m49s
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.
This commit is contained in:
+10
-17
@@ -10,7 +10,6 @@ import (
|
|||||||
"io"
|
"io"
|
||||||
"log/slog"
|
"log/slog"
|
||||||
"net/http"
|
"net/http"
|
||||||
"net/url"
|
|
||||||
"os"
|
"os"
|
||||||
"strconv"
|
"strconv"
|
||||||
"strings"
|
"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.
|
// Client interacts with the GitHub API.
|
||||||
// A Client is safe for concurrent use by multiple goroutines.
|
// A Client is safe for concurrent use by multiple goroutines.
|
||||||
// SetHTTPClient and SetRetryBackoff are intended for test setup only and must
|
// 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.
|
// NewClient creates a new GitHub API client.
|
||||||
// If baseURL is empty, it defaults to https://api.github.com.
|
// 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).
|
// 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.
|
// is passed as an option.
|
||||||
func NewClient(token, baseURL string, opts ...ClientOption) *Client {
|
func NewClient(token, baseURL string, opts ...ClientOption) *Client {
|
||||||
if baseURL == "" {
|
if baseURL == "" {
|
||||||
@@ -273,17 +262,21 @@ func (c *Client) parseRetryAfter(value string) (time.Duration, bool) {
|
|||||||
return 0, false
|
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.
|
// doRequest performs an HTTP request with retry on 429 rate limit responses.
|
||||||
// It respects the Retry-After header when present, supporting both integer
|
// It respects the Retry-After header when present, supporting both integer
|
||||||
// seconds and HTTP-date formats (capped at maxRetryAfter).
|
// seconds and HTTP-date formats (capped at maxRetryAfter).
|
||||||
func (c *Client) doRequest(ctx context.Context, method, reqURL string, accept string) ([]byte, error) {
|
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.
|
// 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 {
|
if c.token != "" && !c.allowInsecureHTTP {
|
||||||
parsed, err := url.Parse(reqURL)
|
if !hasHTTPSScheme(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)
|
return nil, fmt.Errorf("refusing to send credentials over non-HTTPS URL %q (use AllowInsecureHTTP option for trusted networks)", reqURL)
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -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
|
||||||
|
}
|
||||||
|
}
|
||||||
Reference in New Issue
Block a user