From 26ce8b127055eb20676bcccfc8e3384cdf223b1d Mon Sep 17 00:00:00 2001 From: Ryan Fowler Date: Sun, 30 Aug 2026 21:20:20 +0000 Subject: [PATCH] fix(http): keep request URL normalization reusable --- integration/integration_test.go | 20 ++++++++++++ internal/client/client.go | 10 ++++++ internal/client/client_test.go | 31 +++++++++++++++++++ internal/fetch/fetch.go | 21 +++++++++++-- internal/fetch/fetch_test.go | 54 +++++++++++++++++++++++++++++++++ internal/fetch/retry_test.go | 4 +++ 6 files changed, 137 insertions(+), 3 deletions(-) create mode 100644 internal/fetch/fetch_test.go diff --git a/integration/integration_test.go b/integration/integration_test.go index 7b27970f..947a4ce9 100644 --- a/integration/integration_test.go +++ b/integration/integration_test.go @@ -94,6 +94,26 @@ func TestMain(t *testing.T) { assertBufContains(t, res.stderr, "POST / HTTP/1.1") }) + t.Run("schemeless plaintext hint uses the effective URL", func(t *testing.T) { + t.Parallel() + server := startServer(func(http.ResponseWriter, *http.Request) {}) + defer server.Close() + + _, port, err := net.SplitHostPort(strings.TrimPrefix(server.URL, "http://")) + if err != nil { + t.Fatal(err) + } + host := "plaintext-hint.example.test" + target := host + ":" + port + "/path?existing=one" + res := runFetchOpts(t, fetchPath, fetchOpts{env: []string{ + "HTTP_PROXY=", "http_proxy=", "HTTPS_PROXY=", "https_proxy=", "ALL_PROXY=", "all_proxy=", + "NO_PROXY=*", "no_proxy=*", + }}, target, "--query", "added=two words", "--resolve", host+":"+port+":127.0.0.1") + assertExitCode(t, 1, res) + assertBufEmpty(t, res.stdout) + assertBufContains(t, res.stderr, "If this is a plaintext service, use http://"+host+":"+port+"/path?existing=one&added=two%20words.") + }) + t.Run("invalid flag", func(t *testing.T) { t.Parallel() res := runFetch(t, fetchPath, "--invalid") diff --git a/internal/client/client.go b/internal/client/client.go index 3925fcb9..be1b67da 100644 --- a/internal/client/client.go +++ b/internal/client/client.go @@ -1001,6 +1001,10 @@ type RequestConfig struct { // NewRequest returns an *http.Request given the provided configuration. func (c *Client) NewRequest(ctx context.Context, cfg RequestConfig) (*http.Request, error) { + if cfg.URL == nil { + return nil, errors.New("request URL is required") + } + // URL userinfo is an authentication source, not part of the request URL. // Convert it to Basic auth before constructing the request so diagnostics, // redirects, and signatures never retain credentials in the URL. Explicit @@ -1016,6 +1020,12 @@ func (c *Client) NewRequest(ctx context.Context, cfg RequestConfig) (*http.Reque cfg.URL.User = nil } + // Scheme defaults and query parameters belong to this request. Apply them + // to a copy so callers can safely reuse their parsed URL without accumulating + // query parameters. URL userinfo is intentionally scrubbed above. + requestURL := *cfg.URL + cfg.URL = &requestURL + // Append query params directly to RawQuery. url.Values.Encode sorts keys, // which loses the user's ordering even though duplicate parameters are // valid and meaningful to many servers. diff --git a/internal/client/client_test.go b/internal/client/client_test.go index 2d6020f2..84328d79 100644 --- a/internal/client/client_test.go +++ b/internal/client/client_test.go @@ -210,6 +210,37 @@ func TestNewRequestUsesLazyReplayableFileBody(t *testing.T) { } } +func TestNewRequestDoesNotAccumulateURLDefaults(t *testing.T) { + u := &url.URL{Host: "example.com", Path: "/path", RawQuery: "existing=one"} + wantURL := u.String() + c := NewClient(ClientConfig{}) + cfg := RequestConfig{ + QueryParams: []core.KeyVal[string]{{Key: "added", Val: "two words"}}, + URL: u, + } + + for range 2 { + req, err := c.NewRequest(context.Background(), cfg) + if err != nil { + t.Fatal(err) + } + if got := req.URL.String(); got != "https://example.com/path?existing=one&added=two%20words" { + t.Fatalf("request URL = %q", got) + } + } + + if got := u.String(); got != wantURL { + t.Fatalf("input URL mutated: got %q, want %q", got, wantURL) + } +} + +func TestNewRequestRejectsNilURL(t *testing.T) { + _, err := NewClient(ClientConfig{}).NewRequest(context.Background(), RequestConfig{}) + if err == nil || err.Error() != "request URL is required" { + t.Fatalf("error = %v, want request URL is required", err) + } +} + func TestCLI003RequestDefaultsAndOrdering(t *testing.T) { u, err := url.Parse("https://example.com/path?z=old&space=hello+world") if err != nil { diff --git a/internal/fetch/fetch.go b/internal/fetch/fetch.go index c673c3d9..51588661 100644 --- a/internal/fetch/fetch.go +++ b/internal/fetch/fetch.go @@ -122,6 +122,7 @@ type Request struct { // responseDescriptor is set internally after proto setup for response formatting. responseDescriptor protoreflect.MessageDescriptor + effectiveURL *url.URL // harRecorder is reserved before the request starts and records the final // response exchange. It remains private so callers only provide a path. @@ -168,6 +169,7 @@ func Fetch(ctx context.Context, r *Request) int { } func fetch(ctx context.Context, r *Request) (int, error) { + r.effectiveURL = nil if r.HAR != "" { if r.WS || r.GRPCList || r.GRPCDescribe != "" || r.DryRun { return 0, errors.New("--har cannot be used with WebSocket, gRPC discovery, or --dry-run") @@ -292,6 +294,10 @@ func fetch(ctx context.Context, r *Request) (int, error) { if err != nil { return 0, err } + // Keep the effective URL separately for diagnostics that run after the + // request has failed. The caller's input must remain reusable without + // accumulating query parameters. + r.effectiveURL = req.URL defer func() { if req.Body != nil { req.Body.Close() @@ -867,6 +873,8 @@ func formatArticleResponse(ctx context.Context, r *Request, resp *http.Response, pageURL := "" if resp.Request != nil && resp.Request.URL != nil { pageURL = resp.Request.URL.String() + } else if r.effectiveURL != nil { + pageURL = r.effectiveURL.String() } else if r.URL != nil { pageURL = r.URL.String() } @@ -1059,11 +1067,18 @@ func addHeader(headers []core.KeyVal[string], h core.KeyVal[string]) []core.KeyV // schemeless HTTPS connection when the failure is not a certificate or timeout // error. func schemelessPlaintextHint(r *Request, err error) string { - if r == nil || !r.SchemelessURL || r.URL == nil || r.URL.Scheme != "https" || isCertificateErr(err) { + if r == nil || !r.SchemelessURL || isCertificateErr(err) { + return "" + } + effectiveURL := r.effectiveURL + if effectiveURL == nil { + effectiveURL = r.URL + } + if effectiveURL == nil || effectiveURL.Scheme != "https" { return "" } var recordErr tls.RecordHeaderError - if !errors.As(err, &recordErr) { + if !errors.As(err, &recordErr) && !errors.Is(err, http.ErrSchemeMismatch) { return "" } if errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) { @@ -1078,7 +1093,7 @@ func schemelessPlaintextHint(r *Request, err error) string { return "" } - hintURL := *r.URL + hintURL := *effectiveURL hintURL.Scheme = "http" return core.RedactedURL(&hintURL) } diff --git a/internal/fetch/fetch_test.go b/internal/fetch/fetch_test.go new file mode 100644 index 00000000..d05c1378 --- /dev/null +++ b/internal/fetch/fetch_test.go @@ -0,0 +1,54 @@ +package fetch + +import ( + "context" + "io" + "net/http" + "net/http/httptest" + "net/url" + "testing" + + "github.com/ryanfowler/fetch/internal/core" +) + +func TestFetchCanReuseRequestWithoutAccumulatingQueryParams(t *testing.T) { + queries := make(chan string, 2) + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + queries <- r.URL.RawQuery + _, _ = io.WriteString(w, "ok") + })) + defer server.Close() + + u, err := url.Parse(server.URL + "/path?existing=one") + if err != nil { + t.Fatal(err) + } + wantURL := u.String() + r := &Request{ + Compression: core.CompressionOff, + Discard: true, + PrinterHandle: core.NewHandle(core.ColorOff), + QueryParams: []core.KeyVal[string]{{Key: "added", Val: "two words"}}, + URL: u, + Verbosity: core.VSilent, + } + + for range 2 { + code, err := fetch(context.Background(), r) + if err != nil { + t.Fatal(err) + } + if code != 0 { + t.Fatalf("exit code = %d, want 0", code) + } + } + + for range 2 { + if got := <-queries; got != "existing=one&added=two%20words" { + t.Fatalf("query = %q", got) + } + } + if got := r.URL.String(); got != wantURL { + t.Fatalf("input URL mutated: got %q, want %q", got, wantURL) + } +} diff --git a/internal/fetch/retry_test.go b/internal/fetch/retry_test.go index 7cf59455..2f89dd3a 100644 --- a/internal/fetch/retry_test.go +++ b/internal/fetch/retry_test.go @@ -196,6 +196,10 @@ func TestSchemelessPlaintextHint(t *testing.T) { if got := schemelessPlaintextHint(r, connectErr); got != "http://example.com:8080/path?debug=true" { t.Fatalf("hint = %q", got) } + schemeErr := &url.Error{Op: "Get", URL: u.String(), Err: http.ErrSchemeMismatch} + if got := schemelessPlaintextHint(r, schemeErr); got != "http://example.com:8080/path?debug=true" { + t.Fatalf("scheme mismatch hint = %q", got) + } r.SchemelessURL = false if got := schemelessPlaintextHint(r, connectErr); got != "" {