From 53233ddc536d6a06c5e940b743356c86395d39e2 Mon Sep 17 00:00:00 2001 From: Michael McQuade Date: Sun, 25 Oct 2026 16:24:54 -0500 Subject: [PATCH 1/3] feat!: generated clients keep credentials on the origin a request was sent to across redirects NewClient now follows redirects through the exported CheckRedirect policy, applied to any http.Client without a policy of its own: - A redirect on the origin that was asked (or from http up to https on the same host) is followed as before. - A redirect to another origin is followed only for GET and HEAD, carrying only Accept, Accept-Language and User-Agent. net/http alone would carry Authorization to another port or a subdomain of the same host, and every custom header (an API key header, for one) anywhere. - Any other method gets the 3xx back instead of resending its body elsewhere. - A redirect from https to plain http is refused with ErrInsecureRedirect. The retry loop no longer matches the "stopped after" text of a redirect error: it declines ErrInsecureRedirect and ErrTooManyRedirects by errors.Is. BREAKING CHANGE: a non-GET request redirected to another origin returns the 3xx as an APIError instead of following it, and an https to http redirect fails instead of being followed. --- README.md | 2 + internal/generator/e2e_redirect_test.go | 155 ++++++++++++++++++++++++ internal/generator/e2e_test.go | 1 + internal/generator/generator.go | 1 + internal/templates/client.go.tmpl | 1 + internal/templates/redirect.go.tmpl | 86 +++++++++++++ internal/templates/reserved.go | 4 + internal/templates/retry.go.tmpl | 6 +- 8 files changed, 254 insertions(+), 2 deletions(-) create mode 100644 internal/generator/e2e_redirect_test.go create mode 100644 internal/templates/redirect.go.tmpl diff --git a/README.md b/README.md index e5520a1..dbded7d 100644 --- a/README.md +++ b/README.md @@ -16,6 +16,7 @@ Given any OpenAPI 3.1 (or 3.0) spec, it outputs a complete, idiomatic Go client - **Error handling** — `APIError` with sentinel errors (`errors.Is`), typed error wrappers with parsed response bodies (`errors.As`), readable messages via `x-ms-primary-error-message`, RFC 9457 problem details via `Problem()` - **Pagination**: auto-detected cursor, offset, and page pagination with a generic `PageIterator[T]` - **Retries**: configurable exponential backoff with jitter, honoring `Retry-After` in both forms and declining a wait past `MaxDelay` +- **Redirects**: followed on the origin that was asked; elsewhere only a GET or HEAD, without its credentials, and never down to plain http - **Middleware** — composable request/response middleware chain - **OpenAPI 3.1**: JSON Schema 2020-12, nullable type arrays, `$ref` resolution ([what is not generated](#not-supported)) @@ -72,6 +73,7 @@ gen/petstore/ ├── operations.go # ListPets(), CreatePet(), GetPetByID(), DeletePet() ├── options.go # WithHTTPClient(), WithUserAgent() ├── pagination.go # PageIterator[T], ListPetsIter() +├── redirect.go # CheckRedirect, SafeRedirects(), ErrInsecureRedirect ├── retry.go # RetryConfig, WithRetry(), WithDefaultRetry() └── types.go # Pet, PetStatus, PetList, CreatePetRequest, Error ``` diff --git a/internal/generator/e2e_redirect_test.go b/internal/generator/e2e_redirect_test.go new file mode 100644 index 0000000..020c66d --- /dev/null +++ b/internal/generator/e2e_redirect_test.go @@ -0,0 +1,155 @@ +package generator + +import "testing" + +const redirectSpec = `openapi: 3.1.0 +info: { title: redirects, version: "1" } +components: + securitySchemes: + bearer: { type: http, scheme: bearer } +security: + - bearer: [] +paths: + /thing: + get: + operationId: getThing + responses: + "200": + description: ok + content: + application/json: { schema: { type: string } } + post: + operationId: createThing + responses: + "200": + description: ok + content: + application/json: { schema: { type: string } } +` + +// TestE2E_Redirects pins the generated client's redirect policy: no credential +// leaves the origin that was asked, and nothing is sent over plain http. +func TestE2E_Redirects(t *testing.T) { + files, _ := generateFromSpec(t, redirectSpec, "redirectapi") + + runGeneratedWireTest(t, files, "redirects", `package redirectapi + +import ( + "errors" + "net/http" + "net/http/httptest" + "sync/atomic" + "testing" + "time" +) + +func fastRetry() ClientOption { + cfg := DefaultRetryConfig() + cfg.BaseDelay = time.Millisecond + cfg.MaxDelay = time.Millisecond + return WithRetry(cfg) +} + +// elsewhere is another origin on the same host, to which net/http itself would carry Authorization. +func elsewhere(t *testing.T) (*httptest.Server, *atomic.Value, *atomic.Int32) { + var authorization atomic.Value + authorization.Store("") + var hits atomic.Int32 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + hits.Add(1) + authorization.Store(r.Header.Get("Authorization")) + w.Header().Set("Content-Type", "application/json") + w.Write([]byte(`+"`"+`"moved"`+"`"+`)) + })) + t.Cleanup(srv.Close) + return srv, &authorization, &hits +} + +func redirecting(t *testing.T, target string) *httptest.Server { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + http.Redirect(w, r, target+r.URL.Path, http.StatusTemporaryRedirect) + })) + t.Cleanup(srv.Close) + return srv +} + +func TestAGetFollowsARedirectElsewhereWithoutItsCredential(t *testing.T) { + other, authorization, _ := elsewhere(t) + client := NewClient(redirecting(t, other.URL).URL, WithAuth(&BearerAuth{Token: "secret"}), WithHTTPClient(&http.Client{})) + + got, err := client.GetThing(t.Context()) + if err != nil { + t.Fatalf("GetThing: %v", err) + } + if *got != "moved" { + t.Errorf("got %q", *got) + } + if a := authorization.Load(); a != "" { + t.Errorf("the other origin received Authorization %q", a) + } +} + +func TestAPostStopsAtARedirectElsewhere(t *testing.T) { + other, _, hits := elsewhere(t) + client := NewClient(redirecting(t, other.URL).URL, WithAuth(&BearerAuth{Token: "secret"})) + + _, err := client.CreateThing(t.Context()) + var apiErr *APIError + if !errors.As(err, &apiErr) || apiErr.StatusCode != http.StatusTemporaryRedirect { + t.Fatalf("got %v, want the redirect handed back", err) + } + if hits.Load() != 0 { + t.Error("the request was sent to the other origin") + } +} + +func TestARedirectToPlainHTTPIsRefusedAndNotRetried(t *testing.T) { + var calls atomic.Int32 + srv := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + calls.Add(1) + http.Redirect(w, r, "http://"+r.Host+r.URL.Path, http.StatusFound) + })) + t.Cleanup(srv.Close) + client := NewClient(srv.URL, fastRetry(), WithHTTPClient(srv.Client())) + + _, err := client.GetThing(t.Context()) + if !errors.Is(err, ErrInsecureRedirect) { + t.Fatalf("got %v, want ErrInsecureRedirect", err) + } + if n := calls.Load(); n != 1 { + t.Errorf("calls = %d, want a single attempt", n) + } +} + +func TestTooManyRedirectsAreNotRetried(t *testing.T) { + var calls atomic.Int32 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + calls.Add(1) + http.Redirect(w, r, r.URL.Path, http.StatusFound) + })) + t.Cleanup(srv.Close) + client := NewClient(srv.URL, fastRetry()) + + _, err := client.GetThing(t.Context()) + if !errors.Is(err, ErrTooManyRedirects) { + t.Fatalf("got %v, want ErrTooManyRedirects", err) + } + if n := calls.Load(); n != 10 { + t.Errorf("calls = %d, want one chain of redirects", n) + } +} + +func TestAClientsOwnRedirectPolicyIsKept(t *testing.T) { + other, _, hits := elsewhere(t) + own := &http.Client{CheckRedirect: func(*http.Request, []*http.Request) error { return http.ErrUseLastResponse }} + client := NewClient(redirecting(t, other.URL).URL, WithHTTPClient(own)) + + if _, err := client.GetThing(t.Context()); err == nil { + t.Fatal("expected the redirect to be handed back") + } + if hits.Load() != 0 { + t.Error("the client's own policy was replaced") + } +} +`) +} diff --git a/internal/generator/e2e_test.go b/internal/generator/e2e_test.go index 8ff291c..e0d972a 100644 --- a/internal/generator/e2e_test.go +++ b/internal/generator/e2e_test.go @@ -64,6 +64,7 @@ func TestE2E_PetstoreGeneration(t *testing.T) { "operations.go": false, "pagination.go": false, "retry.go": false, + "redirect.go": false, "middleware.go": false, "auth.go": false, "errors.go": false, diff --git a/internal/generator/generator.go b/internal/generator/generator.go index 513b391..b7ceedd 100644 --- a/internal/generator/generator.go +++ b/internal/generator/generator.go @@ -50,6 +50,7 @@ func (g *Generator) Generate() ([]GeneratedFile, error) { {"operations.go.tmpl", "operations.go"}, {"pagination.go.tmpl", "pagination.go"}, {"retry.go.tmpl", "retry.go"}, + {"redirect.go.tmpl", "redirect.go"}, {"middleware.go.tmpl", "middleware.go"}, {"auth.go.tmpl", "auth.go"}, {"errors.go.tmpl", "errors.go"}, diff --git a/internal/templates/client.go.tmpl b/internal/templates/client.go.tmpl index 879663b..75e043b 100644 --- a/internal/templates/client.go.tmpl +++ b/internal/templates/client.go.tmpl @@ -58,6 +58,7 @@ func NewClient(baseURL string, opts ...ClientOption) *Client { for _, opt := range opts { opt(c) } + c.httpClient = SafeRedirects(c.httpClient) return c } diff --git a/internal/templates/redirect.go.tmpl b/internal/templates/redirect.go.tmpl new file mode 100644 index 0000000..ef8e43c --- /dev/null +++ b/internal/templates/redirect.go.tmpl @@ -0,0 +1,86 @@ +// Code generated by openapi-client-generator. DO NOT EDIT. + +package {{ .Name }} + +import ( + "errors" + "net" + "net/http" + "net/url" + "strings" +) + +// ErrInsecureRedirect is a redirect from https to plain http, which the client never follows. +var ErrInsecureRedirect = errors.New("stopped after a redirect from https to plain http") + +// ErrTooManyRedirects is a request redirected more than 10 times. +var ErrTooManyRedirects = errors.New("stopped after 10 redirects") + +// redirectHeaders are the only headers a redirect to another origin carries, since any other may hold a credential. +var redirectHeaders = []string{"Accept", "Accept-Language", "User-Agent"} + +// CheckRedirect follows a redirect on its origin, and elsewhere only a GET or HEAD stripped of its credentials. +func CheckRedirect(req *http.Request, via []*http.Request) error { + if len(via) >= 10 { + return ErrTooManyRedirects + } + if strings.EqualFold(via[len(via)-1].URL.Scheme, "https") && !strings.EqualFold(req.URL.Scheme, "https") { + return ErrInsecureRedirect + } + if sameOrigin(via[0].URL, req.URL) { + return nil + } + if method := via[0].Method; method != http.MethodGet && method != http.MethodHead && method != "" { + return http.ErrUseLastResponse + } + // net/http copies Authorization to a subdomain or another port of the same host, and every other header anywhere. + for name := range req.Header { + keep := false + for _, allowed := range redirectHeaders { + keep = keep || http.CanonicalHeaderKey(name) == allowed + } + if !keep { + req.Header.Del(name) + } + } + return nil +} + +// SafeRedirects is c following redirects by CheckRedirect, unless it has a redirect policy of its own. +func SafeRedirects(c *http.Client) *http.Client { + if c == nil { + c = http.DefaultClient + } + if c.CheckRedirect != nil { + return c + } + safe := *c + safe.CheckRedirect = CheckRedirect + return &safe +} + +// sameOrigin compares scheme, host and port (RFC 6454), counting http up to https on the same host as staying. +func sameOrigin(from, to *url.URL) bool { + if to.User != nil { + return false + } + fromScheme, toScheme := strings.ToLower(from.Scheme), strings.ToLower(to.Scheme) + switch { + case fromScheme == toScheme: + return redirectAuthority(from) == redirectAuthority(to) + case fromScheme == "http" && toScheme == "https": + return strings.EqualFold(from.Hostname(), to.Hostname()) + } + return false +} + +func redirectAuthority(u *url.URL) string { + port := u.Port() + if port == "" { + port = "443" + if strings.EqualFold(u.Scheme, "http") { + port = "80" + } + } + return net.JoinHostPort(strings.ToLower(u.Hostname()), port) +} diff --git a/internal/templates/reserved.go b/internal/templates/reserved.go index 1b3bbf6..6120611 100644 --- a/internal/templates/reserved.go +++ b/internal/templates/reserved.go @@ -13,6 +13,7 @@ var ReservedIdentifiers = []string{ "BasicAuth", "BearerAuth", "CallbackNames", + "CheckRedirect", "Client", "ClientOption", "DefaultBaseURL", @@ -22,9 +23,11 @@ var ReservedIdentifiers = []string{ "ErrConflict", "ErrForbidden", "ErrGatewayTimeout", + "ErrInsecureRedirect", "ErrInternalServerError", "ErrNotFound", "ErrServiceUnavailable", + "ErrTooManyRedirects", "ErrTooManyRequests", "ErrUnauthorized", "EventStream", @@ -38,6 +41,7 @@ var ReservedIdentifiers = []string{ "ResponseMeta", "RetryConfig", "RoundTripFunc", + "SafeRedirects", "ServerURL", "WebhookNames", "WithAuth", diff --git a/internal/templates/retry.go.tmpl b/internal/templates/retry.go.tmpl index bdd9b2d..6de6574 100644 --- a/internal/templates/retry.go.tmpl +++ b/internal/templates/retry.go.tmpl @@ -155,10 +155,12 @@ func isRetryableNetworkError(err error) bool { if errors.As(err, &recordErr) { return false } + if errors.Is(err, ErrInsecureRedirect) || errors.Is(err, ErrTooManyRedirects) { + return false + } var urlErr *url.Error if errors.As(err, &urlErr) { - msg := urlErr.Err.Error() - return !strings.Contains(msg, "unsupported protocol scheme") && !strings.HasPrefix(msg, "stopped after ") + return !strings.Contains(urlErr.Err.Error(), "unsupported protocol scheme") } return true } From b1f8382be6d6016b8f197d2ff6a5f05b39274dd8 Mon Sep 17 00:00:00 2001 From: Michael McQuade Date: Sun, 25 Oct 2026 16:51:25 -0500 Subject: [PATCH 2/3] fix(generator): decide each redirect on the redirected request, and keep more of a request on the same origin - CheckRedirect decides on the redirected request (RFC 9110 section 15.4): a 301, 302 or 303 after a POST is a GET without a body and may follow to another origin; a 307 or 308 of any other method, or any request with a body, gets the 3xx back. - A redirect to another origin rebuilds its headers from an allowlist, so a header set under a non-canonical key no longer survives. The allowlist keeps the conditional and range headers (Range, If-Range, If-Match, If-None-Match, If-Modified-Since, If-Unmodified-Since) and Accept-Encoding, which carry no credential. - The origin compares scheme, host and port with default ports; http counts as upgrading to https only from 80 to 443. Userinfo makes a redirect another origin only when it differs from the request's. - send applies SafeRedirects on each request, so a change to the caller's http.Client after NewClient takes effect. - The retry loop declines ErrInsecureRedirect and ErrTooManyRedirects by errors.Is and still declines a "stopped after" error, which a caller's own CheckRedirect may return. --- README.md | 2 +- internal/generator/e2e_redirect_test.go | 211 +++++++++++++++++++++--- internal/templates/client.go.tmpl | 6 +- internal/templates/redirect.go.tmpl | 57 ++++--- internal/templates/retry.go.tmpl | 4 +- 5 files changed, 231 insertions(+), 49 deletions(-) diff --git a/README.md b/README.md index dbded7d..14c89ac 100644 --- a/README.md +++ b/README.md @@ -16,7 +16,7 @@ Given any OpenAPI 3.1 (or 3.0) spec, it outputs a complete, idiomatic Go client - **Error handling** — `APIError` with sentinel errors (`errors.Is`), typed error wrappers with parsed response bodies (`errors.As`), readable messages via `x-ms-primary-error-message`, RFC 9457 problem details via `Problem()` - **Pagination**: auto-detected cursor, offset, and page pagination with a generic `PageIterator[T]` - **Retries**: configurable exponential backoff with jitter, honoring `Retry-After` in both forms and declining a wait past `MaxDelay` -- **Redirects**: followed on the origin that was asked; elsewhere only a GET or HEAD, without its credentials, and never down to plain http +- **Redirects**: followed on the origin that was asked; elsewhere only a GET or HEAD without a body, stripped of its credentials, and never down to plain http - **Middleware** — composable request/response middleware chain - **OpenAPI 3.1**: JSON Schema 2020-12, nullable type arrays, `$ref` resolution ([what is not generated](#not-supported)) diff --git a/internal/generator/e2e_redirect_test.go b/internal/generator/e2e_redirect_test.go index 020c66d..1b5af8a 100644 --- a/internal/generator/e2e_redirect_test.go +++ b/internal/generator/e2e_redirect_test.go @@ -25,6 +25,17 @@ paths: description: ok content: application/json: { schema: { type: string } } + /search: + get: + operationId: search + requestBody: + content: + application/json: { schema: { type: string } } + responses: + "200": + description: ok + content: + application/json: { schema: { type: string } } ` // TestE2E_Redirects pins the generated client's redirect policy: no credential @@ -38,6 +49,7 @@ import ( "errors" "net/http" "net/http/httptest" + "strings" "sync/atomic" "testing" "time" @@ -50,19 +62,31 @@ func fastRetry() ClientOption { return WithRetry(cfg) } -// elsewhere is another origin on the same host, to which net/http itself would carry Authorization. -func elsewhere(t *testing.T) (*httptest.Server, *atomic.Value, *atomic.Int32) { - var authorization atomic.Value - authorization.Store("") - var hits atomic.Int32 - srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - hits.Add(1) - authorization.Store(r.Header.Get("Authorization")) +type received struct { + header atomic.Value + hits atomic.Int32 +} + +func (rc *received) Header() http.Header { + h, _ := rc.header.Load().(http.Header) + return h +} + +func recording(rc *received) http.HandlerFunc { + return func(w http.ResponseWriter, r *http.Request) { + rc.hits.Add(1) + rc.header.Store(r.Header.Clone()) w.Header().Set("Content-Type", "application/json") w.Write([]byte(`+"`"+`"moved"`+"`"+`)) - })) + } +} + +// elsewhere is another origin on the same host, to which net/http itself would carry Authorization. +func elsewhere(t *testing.T) (*httptest.Server, *received) { + var rc received + srv := httptest.NewServer(recording(&rc)) t.Cleanup(srv.Close) - return srv, &authorization, &hits + return srv, &rc } func redirecting(t *testing.T, target string) *httptest.Server { @@ -73,9 +97,15 @@ func redirecting(t *testing.T, target string) *httptest.Server { return srv } -func TestAGetFollowsARedirectElsewhereWithoutItsCredential(t *testing.T) { - other, authorization, _ := elsewhere(t) - client := NewClient(redirecting(t, other.URL).URL, WithAuth(&BearerAuth{Token: "secret"}), WithHTTPClient(&http.Client{})) +func withHeaders(req *http.Request, next RoundTripFunc) (*http.Response, error) { + req.Header.Set("X-Api-Key", "secret") + req.Header["x-secret"] = []string{"secret"} + return next(req) +} + +func TestAGetFollowsARedirectElsewhereWithoutItsCredentials(t *testing.T) { + other, rc := elsewhere(t) + client := NewClient(redirecting(t, other.URL).URL, WithAuth(&BearerAuth{Token: "secret"}), WithMiddleware(withHeaders), WithHTTPClient(&http.Client{})) got, err := client.GetThing(t.Context()) if err != nil { @@ -84,13 +114,18 @@ func TestAGetFollowsARedirectElsewhereWithoutItsCredential(t *testing.T) { if *got != "moved" { t.Errorf("got %q", *got) } - if a := authorization.Load(); a != "" { - t.Errorf("the other origin received Authorization %q", a) + for _, name := range []string{"Authorization", "X-Api-Key", "X-Secret"} { + if v := rc.Header().Get(name); v != "" { + t.Errorf("the other origin received %s %q", name, v) + } + } + if rc.Header().Get("Accept") == "" { + t.Error("the other origin did not receive Accept") } } func TestAPostStopsAtARedirectElsewhere(t *testing.T) { - other, _, hits := elsewhere(t) + other, rc := elsewhere(t) client := NewClient(redirecting(t, other.URL).URL, WithAuth(&BearerAuth{Token: "secret"})) _, err := client.CreateThing(t.Context()) @@ -98,11 +133,127 @@ func TestAPostStopsAtARedirectElsewhere(t *testing.T) { if !errors.As(err, &apiErr) || apiErr.StatusCode != http.StatusTemporaryRedirect { t.Fatalf("got %v, want the redirect handed back", err) } - if hits.Load() != 0 { + if rc.hits.Load() != 0 { t.Error("the request was sent to the other origin") } } +func TestAPostSeeOtherElsewhereFollowsAsAGetWithoutItsCredentials(t *testing.T) { + other, rc := elsewhere(t) + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + http.Redirect(w, r, other.URL+r.URL.Path, http.StatusSeeOther) + })) + t.Cleanup(srv.Close) + client := NewClient(srv.URL, WithAuth(&BearerAuth{Token: "secret"}), WithMiddleware(withHeaders)) + + got, err := client.CreateThing(t.Context()) + if err != nil { + t.Fatalf("CreateThing: %v", err) + } + if *got != "moved" { + t.Errorf("got %q", *got) + } + for _, name := range []string{"Authorization", "X-Api-Key", "X-Secret"} { + if v := rc.Header().Get(name); v != "" { + t.Errorf("the other origin received %s %q", name, v) + } + } +} + +func TestARangedGetKeepsItsRangeElsewhere(t *testing.T) { + other, rc := elsewhere(t) + ranged := func(req *http.Request, next RoundTripFunc) (*http.Response, error) { + req.Header.Set("Range", "bytes=0-1") + req.Header.Set("If-Range", `+"`"+`"v1"`+"`"+`) + req.Header.Set("If-None-Match", `+"`"+`"v0"`+"`"+`) + return next(req) + } + client := NewClient(redirecting(t, other.URL).URL, WithAuth(&BearerAuth{Token: "secret"}), WithMiddleware(ranged)) + + if _, err := client.GetThing(t.Context()); err != nil { + t.Fatalf("GetThing: %v", err) + } + for name, want := range map[string]string{"Range": "bytes=0-1", "If-Range": `+"`"+`"v1"`+"`"+`, "If-None-Match": `+"`"+`"v0"`+"`"+`} { + if got := rc.Header().Get(name); got != want { + t.Errorf("%s = %q, want %q", name, got, want) + } + } + if a := rc.Header().Get("Authorization"); a != "" { + t.Errorf("the other origin received Authorization %q", a) + } +} + +func TestAClientsOwnRedirectRefusalIsNotRetried(t *testing.T) { + var calls atomic.Int32 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + calls.Add(1) + http.Redirect(w, r, "/elsewhere", http.StatusFound) + })) + t.Cleanup(srv.Close) + own := &http.Client{CheckRedirect: func(*http.Request, []*http.Request) error { return errors.New("stopped after a redirect this caller refuses") }} + client := NewClient(srv.URL, fastRetry(), WithHTTPClient(own)) + + if _, err := client.GetThing(t.Context()); err == nil { + t.Fatal("expected the caller's refusal") + } + if n := calls.Load(); n != 1 { + t.Errorf("calls = %d, want a single attempt", n) + } +} + +func TestAGetWithABodyStopsAtARedirectElsewhere(t *testing.T) { + other, rc := elsewhere(t) + client := NewClient(redirecting(t, other.URL).URL) + + query := "secret" + _, err := client.Search(t.Context(), &query) + var apiErr *APIError + if !errors.As(err, &apiErr) || apiErr.StatusCode != http.StatusTemporaryRedirect { + t.Fatalf("got %v, want the redirect handed back", err) + } + if rc.hits.Load() != 0 { + t.Error("the body was sent to the other origin") + } +} + +func TestAPostFollowsARedirectOnItsOrigin(t *testing.T) { + var rc received + mux := http.NewServeMux() + mux.HandleFunc("POST /thing", func(w http.ResponseWriter, r *http.Request) { + http.Redirect(w, r, "/moved", http.StatusTemporaryRedirect) + }) + mux.HandleFunc("/moved", recording(&rc)) + srv := httptest.NewServer(mux) + t.Cleanup(srv.Close) + + for i, base := range []string{srv.URL, strings.Replace(srv.URL, "://", "://user:pass@", 1)} { + client := NewClient(base, WithAuth(&BearerAuth{Token: "secret"})) + if _, err := client.CreateThing(t.Context()); err != nil { + t.Fatalf("CreateThing on %s: %v", base, err) + } + if n := rc.hits.Load(); n != int32(i+1) { + t.Fatalf("the redirect on %s was not followed", base) + } + if a := rc.Header().Get("Authorization"); a != "Bearer secret" { + t.Errorf("Authorization = %q on %s, want the credential kept on its origin", a, base) + } + } +} + +func TestAnUpgradeToHTTPSOnAnotherPortIsAnotherOrigin(t *testing.T) { + var rc received + secure := httptest.NewTLSServer(recording(&rc)) + t.Cleanup(secure.Close) + client := NewClient(redirecting(t, secure.URL).URL, WithAuth(&BearerAuth{Token: "secret"}), WithHTTPClient(secure.Client())) + + if _, err := client.GetThing(t.Context()); err != nil { + t.Fatalf("GetThing: %v", err) + } + if a := rc.Header().Get("Authorization"); a != "" { + t.Errorf("another port received Authorization %q", a) + } +} + func TestARedirectToPlainHTTPIsRefusedAndNotRetried(t *testing.T) { var calls atomic.Int32 srv := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { @@ -140,16 +291,38 @@ func TestTooManyRedirectsAreNotRetried(t *testing.T) { } func TestAClientsOwnRedirectPolicyIsKept(t *testing.T) { - other, _, hits := elsewhere(t) + other, rc := elsewhere(t) own := &http.Client{CheckRedirect: func(*http.Request, []*http.Request) error { return http.ErrUseLastResponse }} client := NewClient(redirecting(t, other.URL).URL, WithHTTPClient(own)) if _, err := client.GetThing(t.Context()); err == nil { t.Fatal("expected the redirect to be handed back") } - if hits.Load() != 0 { + if rc.hits.Load() != 0 { t.Error("the client's own policy was replaced") } } + +type countingTransport struct{ n atomic.Int32 } + +func (c *countingTransport) RoundTrip(r *http.Request) (*http.Response, error) { + c.n.Add(1) + return http.DefaultTransport.RoundTrip(r) +} + +func TestTheHTTPClientIsReadAtEachRequest(t *testing.T) { + other, _ := elsewhere(t) + hc := &http.Client{} + client := NewClient(other.URL, WithHTTPClient(hc)) + var transport countingTransport + hc.Transport = &transport + + if _, err := client.GetThing(t.Context()); err != nil { + t.Fatalf("GetThing: %v", err) + } + if transport.n.Load() != 1 { + t.Error("a Transport set after NewClient was not used") + } +} `) } diff --git a/internal/templates/client.go.tmpl b/internal/templates/client.go.tmpl index 75e043b..9062b9b 100644 --- a/internal/templates/client.go.tmpl +++ b/internal/templates/client.go.tmpl @@ -58,7 +58,6 @@ func NewClient(baseURL string, opts ...ClientOption) *Client { for _, opt := range opts { opt(c) } - c.httpClient = SafeRedirects(c.httpClient) return c } @@ -122,9 +121,10 @@ func (c *Client) send(ctx context.Context, method string, path string, body any, } } - // Build the transport function. + // Applied per call, not in NewClient, so a change to the caller's http.Client after NewClient still takes effect. + httpClient := SafeRedirects(c.httpClient) transport := func(req *http.Request) (*http.Response, error) { - return c.httpClient.Do(req) + return httpClient.Do(req) } // Wrap with middleware chain (applied in reverse order so first middleware runs first). diff --git a/internal/templates/redirect.go.tmpl b/internal/templates/redirect.go.tmpl index ef8e43c..fb316f3 100644 --- a/internal/templates/redirect.go.tmpl +++ b/internal/templates/redirect.go.tmpl @@ -4,7 +4,6 @@ package {{ .Name }} import ( "errors" - "net" "net/http" "net/url" "strings" @@ -13,13 +12,16 @@ import ( // ErrInsecureRedirect is a redirect from https to plain http, which the client never follows. var ErrInsecureRedirect = errors.New("stopped after a redirect from https to plain http") -// ErrTooManyRedirects is a request redirected more than 10 times. +// ErrTooManyRedirects is a request redirected 10 times. var ErrTooManyRedirects = errors.New("stopped after 10 redirects") // redirectHeaders are the only headers a redirect to another origin carries, since any other may hold a credential. -var redirectHeaders = []string{"Accept", "Accept-Language", "User-Agent"} +var redirectHeaders = []string{ + "Accept", "Accept-Encoding", "Accept-Language", "User-Agent", + "Range", "If-Range", "If-Match", "If-None-Match", "If-Modified-Since", "If-Unmodified-Since", +} -// CheckRedirect follows a redirect on its origin, and elsewhere only a GET or HEAD stripped of its credentials. +// CheckRedirect follows a redirect on its origin, and elsewhere only a GET or HEAD without a body, stripped of its credentials. func CheckRedirect(req *http.Request, via []*http.Request) error { if len(via) >= 10 { return ErrTooManyRedirects @@ -30,19 +32,21 @@ func CheckRedirect(req *http.Request, via []*http.Request) error { if sameOrigin(via[0].URL, req.URL) { return nil } - if method := via[0].Method; method != http.MethodGet && method != http.MethodHead && method != "" { + // req is the redirected request (RFC 9110 section 15.4), so a 303 after a POST arrives here as a GET without a body. + if req.Method != http.MethodGet && req.Method != http.MethodHead && req.Method != "" { return http.ErrUseLastResponse } - // net/http copies Authorization to a subdomain or another port of the same host, and every other header anywhere. - for name := range req.Header { - keep := false - for _, allowed := range redirectHeaders { - keep = keep || http.CanonicalHeaderKey(name) == allowed - } - if !keep { - req.Header.Del(name) + if req.Body != nil && req.Body != http.NoBody { + return http.ErrUseLastResponse + } + // net/http copies every header anywhere under the key it was set with, which Header.Del would miss when not canonical. + kept := http.Header{} + for _, name := range redirectHeaders { + if values := req.Header.Values(name); len(values) > 0 { + kept[name] = values } } + req.Header = kept return nil } @@ -59,28 +63,31 @@ func SafeRedirects(c *http.Client) *http.Client { return &safe } -// sameOrigin compares scheme, host and port (RFC 6454), counting http up to https on the same host as staying. +// sameOrigin compares scheme, host and port (RFC 6454), counting http on 80 up to https on 443 of the same host as staying. func sameOrigin(from, to *url.URL) bool { - if to.User != nil { + if to.User != nil && to.User.String() != from.User.String() { + return false + } + if !strings.EqualFold(from.Hostname(), to.Hostname()) { return false } fromScheme, toScheme := strings.ToLower(from.Scheme), strings.ToLower(to.Scheme) + fromPort, toPort := redirectPort(from), redirectPort(to) switch { case fromScheme == toScheme: - return redirectAuthority(from) == redirectAuthority(to) + return fromPort == toPort case fromScheme == "http" && toScheme == "https": - return strings.EqualFold(from.Hostname(), to.Hostname()) + return fromPort == "80" && toPort == "443" } return false } -func redirectAuthority(u *url.URL) string { - port := u.Port() - if port == "" { - port = "443" - if strings.EqualFold(u.Scheme, "http") { - port = "80" - } +func redirectPort(u *url.URL) string { + if port := u.Port(); port != "" { + return port + } + if strings.EqualFold(u.Scheme, "http") { + return "80" } - return net.JoinHostPort(strings.ToLower(u.Hostname()), port) + return "443" } diff --git a/internal/templates/retry.go.tmpl b/internal/templates/retry.go.tmpl index 6de6574..632f4ff 100644 --- a/internal/templates/retry.go.tmpl +++ b/internal/templates/retry.go.tmpl @@ -160,7 +160,9 @@ func isRetryableNetworkError(err error) bool { } var urlErr *url.Error if errors.As(err, &urlErr) { - return !strings.Contains(urlErr.Err.Error(), "unsupported protocol scheme") + // A caller's own CheckRedirect refuses the way net/http's does, with an error that starts "stopped after". + msg := urlErr.Err.Error() + return !strings.Contains(msg, "unsupported protocol scheme") && !strings.HasPrefix(msg, "stopped after ") } return true } From 6b8ed9a481196af9930b611701be7eb4e307e0e9 Mon Sep 17 00:00:00 2001 From: Michael McQuade Date: Sun, 25 Oct 2026 17:19:44 -0500 Subject: [PATCH 3/3] fix(generator): a write redirected to another origin with a 301 or 302 no longer takes that origin's page for its answer net/http turns a POST, PUT, PATCH or DELETE into a GET after a 301 or 302 as well as a 303. Followed to another origin, that GET's 200 (a proxy's login page, say) became the write's answer. CheckRedirect now lets a write follow to another origin only after a 303, which asks for a GET of another resource (RFC 9110 section 15.4.4); a 301 or 302 is handed back. --- internal/generator/e2e_redirect_test.go | 20 ++++++++++++++++++++ internal/templates/redirect.go.tmpl | 10 ++++++++-- 2 files changed, 28 insertions(+), 2 deletions(-) diff --git a/internal/generator/e2e_redirect_test.go b/internal/generator/e2e_redirect_test.go index 1b5af8a..6a5f7f7 100644 --- a/internal/generator/e2e_redirect_test.go +++ b/internal/generator/e2e_redirect_test.go @@ -160,6 +160,26 @@ func TestAPostSeeOtherElsewhereFollowsAsAGetWithoutItsCredentials(t *testing.T) } } +func TestAPostRedirectedElsewhereBecomesAGetOnlyForASeeOther(t *testing.T) { + for _, status := range []int{http.StatusMovedPermanently, http.StatusFound} { + other, rc := elsewhere(t) + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + http.Redirect(w, r, other.URL+r.URL.Path, status) + })) + t.Cleanup(srv.Close) + client := NewClient(srv.URL, WithAuth(&BearerAuth{Token: "secret"})) + + _, err := client.CreateThing(t.Context()) + var apiErr *APIError + if !errors.As(err, &apiErr) || apiErr.StatusCode != status { + t.Fatalf("a POST answered %d: got %v, want the redirect handed back", status, err) + } + if rc.hits.Load() != 0 { + t.Errorf("a POST answered %d took another origin's page for its answer", status) + } + } +} + func TestARangedGetKeepsItsRangeElsewhere(t *testing.T) { other, rc := elsewhere(t) ranged := func(req *http.Request, next RoundTripFunc) (*http.Response, error) { diff --git a/internal/templates/redirect.go.tmpl b/internal/templates/redirect.go.tmpl index fb316f3..45deca2 100644 --- a/internal/templates/redirect.go.tmpl +++ b/internal/templates/redirect.go.tmpl @@ -21,7 +21,7 @@ var redirectHeaders = []string{ "Range", "If-Range", "If-Match", "If-None-Match", "If-Modified-Since", "If-Unmodified-Since", } -// CheckRedirect follows a redirect on its origin, and elsewhere only a GET or HEAD without a body, stripped of its credentials. +// CheckRedirect follows a redirect on its origin; elsewhere only a bodiless GET or HEAD, or a 303's GET, without credentials. func CheckRedirect(req *http.Request, via []*http.Request) error { if len(via) >= 10 { return ErrTooManyRedirects @@ -33,7 +33,9 @@ func CheckRedirect(req *http.Request, via []*http.Request) error { return nil } // req is the redirected request (RFC 9110 section 15.4), so a 303 after a POST arrives here as a GET without a body. - if req.Method != http.MethodGet && req.Method != http.MethodHead && req.Method != "" { + // net/http turns a write into a GET after a 301 or 302 too, which would take another origin's page for its answer. + sent := via[len(via)-1].Method + if !bodilessMethod(req.Method) || (!bodilessMethod(sent) && (req.Response == nil || req.Response.StatusCode != http.StatusSeeOther)) { return http.ErrUseLastResponse } if req.Body != nil && req.Body != http.NoBody { @@ -50,6 +52,10 @@ func CheckRedirect(req *http.Request, via []*http.Request) error { return nil } +func bodilessMethod(method string) bool { + return method == http.MethodGet || method == http.MethodHead || method == "" +} + // SafeRedirects is c following redirects by CheckRedirect, unless it has a redirect policy of its own. func SafeRedirects(c *http.Client) *http.Client { if c == nil {