diff --git a/README.md b/README.md index e5520a1..14c89ac 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 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)) @@ -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..6a5f7f7 --- /dev/null +++ b/internal/generator/e2e_redirect_test.go @@ -0,0 +1,348 @@ +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 } } + /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 +// 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" + "strings" + "sync/atomic" + "testing" + "time" +) + +func fastRetry() ClientOption { + cfg := DefaultRetryConfig() + cfg.BaseDelay = time.Millisecond + cfg.MaxDelay = time.Millisecond + return WithRetry(cfg) +} + +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, &rc +} + +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 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 { + t.Fatalf("GetThing: %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) + } + } + if rc.Header().Get("Accept") == "" { + t.Error("the other origin did not receive Accept") + } +} + +func TestAPostStopsAtARedirectElsewhere(t *testing.T) { + other, rc := 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 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 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) { + 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) { + 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, 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 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/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..9062b9b 100644 --- a/internal/templates/client.go.tmpl +++ b/internal/templates/client.go.tmpl @@ -121,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 new file mode 100644 index 0000000..45deca2 --- /dev/null +++ b/internal/templates/redirect.go.tmpl @@ -0,0 +1,99 @@ +// Code generated by openapi-client-generator. DO NOT EDIT. + +package {{ .Name }} + +import ( + "errors" + "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 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-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; 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 + } + 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 + } + // req is the redirected request (RFC 9110 section 15.4), so a 303 after a POST arrives here as a GET without a body. + // 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 { + 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 +} + +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 { + 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 on 80 up to https on 443 of the same host as staying. +func sameOrigin(from, to *url.URL) bool { + 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 fromPort == toPort + case fromScheme == "http" && toScheme == "https": + return fromPort == "80" && toPort == "443" + } + return false +} + +func redirectPort(u *url.URL) string { + if port := u.Port(); port != "" { + return port + } + if strings.EqualFold(u.Scheme, "http") { + return "80" + } + return "443" +} 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..632f4ff 100644 --- a/internal/templates/retry.go.tmpl +++ b/internal/templates/retry.go.tmpl @@ -155,8 +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) { + // 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 ") }