fix: use case-insensitive HTTP scheme check and redact userinfo
PR Ready Gate / clear-labels (pull_request) Successful in 2s
CI / test (pull_request) Successful in 17s
CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 30s
CI / review (gpt-5, security, ., rodin/security-patterns, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 56s
CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 1m53s

Address review feedback on PR #113:

- MAJOR (both reviews): Replace strings.HasPrefix(reqURL, "http://")
  with url.Parse + strings.EqualFold for case-insensitive scheme
  comparison per RFC 3986. Prevents bypass via HTTP:// or Http://.

- MINOR (security): Enhance redactURL to strip userinfo component
  (user:pass@host) in addition to query params, preventing credential
  leakage in error messages and logs.

- NIT (gpt): Remove redundant timer.Stop() after timer.C fires —
  it's a no-op and the comment was misleading.

- Add tests for uppercase/mixed-case HTTP scheme rejection and
  userinfo redaction.
This commit is contained in:
claw
2026-05-13 11:35:10 -07:00
parent 0232343126
commit db7b7e66bf
2 changed files with 63 additions and 8 deletions
+23 -8
View File
@@ -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
// 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
if u.User != nil {
u.User = nil
}
if u.RawQuery != "" {
u.RawQuery = "<redacted>"
}
u.Fragment = ""
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))
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)
select {
case <-timer.C:
timer.Stop()
case <-ctx.Done():
timer.Stop()
return nil, ctx.Err()
+40
View File
@@ -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") {
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)
}
}