feat(github): add safeguards against accidental AllowInsecureHTTP use (#96) #113
@@ -10,6 +10,7 @@ import (
|
||||
"io"
|
||||
"log/slog"
|
||||
"net/http"
|
||||
"net/url"
|
||||
"os"
|
||||
"strconv"
|
||||
"strings"
|
||||
@@ -268,21 +269,36 @@ 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
|
||||
sonnet-review-bot
commented
[MINOR] hasHTTPSScheme uses strings.EqualFold for the scheme prefix check. Since "https://" contains only ASCII characters, EqualFold is correct but adds a small amount of unnecessary overhead compared to a direct strings.ToLower or a case-sensitive check (HTTP schemes per RFC 3986 are case-insensitive but in practice always lowercase in this codebase). This is purely a performance NIT on a non-hot path and does not affect correctness. **[MINOR]** hasHTTPSScheme uses strings.EqualFold for the scheme prefix check. Since "https://" contains only ASCII characters, EqualFold is correct but adds a small amount of unnecessary overhead compared to a direct strings.ToLower or a case-sensitive check (HTTP schemes per RFC 3986 are case-insensitive but in practice always lowercase in this codebase). This is purely a performance NIT on a non-hot path and does not affect correctness.
|
||||
// query parameters with a placeholder.
|
||||
|
security-review-bot marked this conversation as resolved
Outdated
[MINOR] The refusal error includes the full request URL (including query string) which may carry sensitive data in some edge cases; if upstream logs this error verbatim, it could leak information. Consider redacting query parameters or logging a sanitized URL. **[MINOR]** The refusal error includes the full request URL (including query string) which may carry sensitive data in some edge cases; if upstream logs this error verbatim, it could leak information. Consider redacting query parameters or logging a sanitized URL.
|
||||
func redactURL(rawURL string) string {
|
||||
if idx := strings.IndexByte(rawURL, '?'); idx != -1 {
|
||||
return rawURL[:idx] + "?<redacted>"
|
||||
u, err := url.Parse(rawURL)
|
||||
|
[MAJOR] The HTTP-scheme guard in doRequest uses a case-sensitive prefix check (strings.HasPrefix(reqURL, "http://"). URI schemes are case-insensitive (RFC 3986), so a URL like "HTTP://..." bypasses the guard and may send credentials over plaintext. Parse the URL and compare scheme case-insensitively (e.g., url.Parse + strings.EqualFold(u.Scheme, "http")). **[MAJOR]** The HTTP-scheme guard in doRequest uses a case-sensitive prefix check (strings.HasPrefix(reqURL, "http://"). URI schemes are case-insensitive (RFC 3986), so a URL like "HTTP://..." bypasses the guard and may send credentials over plaintext. Parse the URL and compare scheme case-insensitively (e.g., url.Parse + strings.EqualFold(u.Scheme, "http")).
|
||||
if err != nil {
|
||||
return "<unparseable URL>"
|
||||
}
|
||||
|
sonnet-review-bot
commented
[NIT] The **[NIT]** The `redactURL` function has a dead code path: `if u.User != nil { u.User = nil }`. The nil check is unnecessary because assigning nil to a nil pointer is a no-op. This works correctly but is slightly misleading. Could simplify to `u.User = nil`.
sonnet-review-bot
commented
[NIT] The HTTP rejection error message says 'use HTTPS or set AllowInsecureHTTP option'. For users who call AllowInsecureHTTP() and see it silently fall back (env var not set), this message may be confusing — they did set the option but it was ignored. A slightly more helpful message might mention the REVIEW_BOT_ALLOW_INSECURE env var. Minor UX nit. **[NIT]** The HTTP rejection error message says 'use HTTPS or set AllowInsecureHTTP option'. For users who call AllowInsecureHTTP() and see it silently fall back (env var not set), this message may be confusing — they did set the option but it was ignored. A slightly more helpful message might mention the REVIEW_BOT_ALLOW_INSECURE env var. Minor UX nit.
|
||||
return rawURL
|
||||
if u.User != nil {
|
||||
u.User = nil
|
||||
}
|
||||
gpt-review-bot
commented
[MAJOR] The insecure-HTTP guard uses strings.HasPrefix(reqURL, "http://") which is case-sensitive. URL schemes are case-insensitive; a URL like "HTTP://..." would bypass this check and send credentials over plaintext. Parse the URL and compare the scheme case-insensitively (e.g., url.Parse + strings.EqualFold(u.Scheme, "http")) or normalize to lower-case before checking. **[MAJOR]** The insecure-HTTP guard uses strings.HasPrefix(reqURL, "http://") which is case-sensitive. URL schemes are case-insensitive; a URL like "HTTP://..." would bypass this check and send credentials over plaintext. Parse the URL and compare the scheme case-insensitively (e.g., url.Parse + strings.EqualFold(u.Scheme, "http")) or normalize to lower-case before checking.
|
||||
if u.RawQuery != "" {
|
||||
u.RawQuery = "<redacted>"
|
||||
}
|
||||
u.Fragment = ""
|
||||
return u.String()
|
||||
}
|
||||
|
[MINOR] The error message includes user-influenced URL data via redactURL(reqURL) without explicit newline/carriage-return sanitization. If upstream code logs this error directly, it could allow limited log injection if a crafted URL with control characters is accepted by url.Parse. Consider sanitizing \n/\r in the formatted error or ensuring redactURL strips such characters. **[MINOR]** The error message includes user-influenced URL data via redactURL(reqURL) without explicit newline/carriage-return sanitization. If upstream code logs this error directly, it could allow limited log injection if a crafted URL with control characters is accepted by url.Parse. Consider sanitizing \n/\r in the formatted error or ensuring redactURL strips such characters.
|
||||
|
||||
// doRequest performs an HTTP request with retry on 429 rate limit responses.
|
||||
|
sonnet-review-bot
commented
[NIT] The HTTP scheme check in **[NIT]** The HTTP scheme check in `doRequest` parses the URL twice when the URL is also valid for the request — once here in the guard and implicitly again by `http.NewRequestWithContext`. This is a minor inefficiency in the hot path but not a correctness issue.
|
||||
// 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))
|
||||
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 +319,6 @@ func (c *Client) doRequest(ctx context.Context, method, reqURL string, accept st
|
||||
timer := time.NewTimer(delay)
|
||||
|
sonnet-review-bot
commented
[MINOR] The diff removes **[MINOR]** The diff removes `timer.Stop()` from the `case <-timer.C:` branch. When the timer fires normally, the timer's goroutine is already done, but calling `timer.Stop()` after it fires is a no-op and not harmful — the original code was actually correct in calling it (the resource is already freed, but it's a safe call). The real concern is the missing call on the successful timer path: after `<-timer.C` fires, the GC will eventually collect the timer, but `timer.Stop()` on the fired case is idiomatic cleanup. This is extremely minor since a fired timer has no goroutine leak, only a small GC delay. Consider adding `timer.Stop()` back on both branches for explicitness, or using `defer timer.Stop()` before the select.
|
||||
select {
|
||||
case <-timer.C:
|
||||
timer.Stop()
|
||||
case <-ctx.Done():
|
||||
timer.Stop()
|
||||
return nil, ctx.Err()
|
||||
|
||||
@@ -544,6 +544,30 @@ func TestNoInsecureOption_RejectsHTTP(t *testing.T) {
|
||||
t.Errorf("unexpected error message: %v", err)
|
||||
}
|
||||
}
|
||||
func TestNoInsecureOption_RejectsUppercaseHTTP(t *testing.T) {
|
||||
// Verify case-insensitive scheme check (RFC 3986).
|
||||
c := NewClient("tok", "HTTP://example.com")
|
||||
_, err := c.doGet(context.Background(), "HTTP://example.com/test")
|
||||
if err == nil {
|
||||
t.Fatal("expected error for uppercase HTTP scheme")
|
||||
}
|
||||
if !strings.Contains(err.Error(), "refusing HTTP request") {
|
||||
|
sonnet-review-bot
commented
[NIT] Missing blank line between **[NIT]** Missing blank line between `TestNoInsecureOption_RejectsHTTP` and `TestNoInsecureOption_RejectsUppercaseHTTP` — minor formatting inconsistency compared to the rest of the file, but `gofmt` doesn't enforce blank lines between top-level declarations in the same way.
|
||||
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://example.com")
|
||||
_, err := c.doGet(context.Background(), "Http://example.com/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) {
|
||||
@@ -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)
|
||||
}
|
||||
}
|
||||
|
||||
[NIT]
redactURLusesstrings.IndexBytewhich is perfectly fine, but since the project already importsnet/url, usingurl.Parse+ clearingRawQuerywould handle edge cases like fragment identifiers (#) and malformed URLs more robustly. Not a bug given the controlled usage context (internal URLs), just a note.[NIT] redactURL redacts userinfo and query parameters; consider also clearing URL fragments (u.Fragment) to avoid leaking anchor components in logs, even though they’re generally not sent to servers.