From d59138deddd98128b64a8c673745a619ae9ba01e Mon Sep 17 00:00:00 2001 From: William Martin Date: Fri, 7 Aug 2026 09:53:50 +0200 Subject: [PATCH] Let clients configure a redirect policy NewHTTPClient built its http.Client from the transport alone, so a CheckRedirect supplied by the caller was silently discarded. There was no way to express a redirect policy through this package. That silence has teeth. Go's default policy converts a DELETE into a GET when it follows a 301, so deleting a resource that has since been renamed follows the redirect, issues a GET against the new location, and returns its success status. The caller is told the delete succeeded when nothing was deleted. cli/cli hits exactly this in `gh repo delete`, and works around it today by building its own client and bypassing this package. Add CheckRedirect to ClientOptions and pass it through, mirroring the field of the same name on http.Client. Leaving it nil keeps the existing behaviour of following up to 10 redirects. The test drives a stub transport that answers the first request with a 301. Redirect handling belongs to http.Client rather than the transport, so the stub still exercises the real policy: the client only asks it for the redirected request if the policy allows the redirect. Without the change, the second case sees the DELETE arrive as a GET and reports 204. --- pkg/api/client_options.go | 10 ++++++ pkg/api/http_client.go | 2 +- pkg/api/http_client_test.go | 68 +++++++++++++++++++++++++++++++++++++ 3 files changed, 79 insertions(+), 1 deletion(-) diff --git a/pkg/api/client_options.go b/pkg/api/client_options.go index 3464aacc..2e25f489 100644 --- a/pkg/api/client_options.go +++ b/pkg/api/client_options.go @@ -25,6 +25,16 @@ type ClientOptions struct { // Default is 24 hours. CacheTTL time.Duration + // CheckRedirect specifies the policy for handling redirects, matching the + // field of the same name on http.Client. If nil, the default policy of + // following up to 10 redirects is used. + // + // This matters for requests where following a redirect silently changes + // the meaning of the request. Go's default policy converts a DELETE into a + // GET when it follows a 301, so a caller deleting a renamed resource can + // receive a success response having deleted nothing. + CheckRedirect func(*http.Request, []*http.Request) error + // EnableCache specifies if API requests will be cached or not. // Default is no caching. EnableCache bool diff --git a/pkg/api/http_client.go b/pkg/api/http_client.go index c2f0d792..e3a837a8 100644 --- a/pkg/api/http_client.go +++ b/pkg/api/http_client.go @@ -118,7 +118,7 @@ func NewHTTPClient(opts ClientOptions) (*http.Client, error) { } transport = newHeaderRoundTripper(opts.Host, opts.AuthToken, opts.Headers, transport) - return &http.Client{Transport: transport, Timeout: opts.Timeout}, nil + return &http.Client{Transport: transport, Timeout: opts.Timeout, CheckRedirect: opts.CheckRedirect}, nil } func inspectableMIMEType(t string) bool { diff --git a/pkg/api/http_client_test.go b/pkg/api/http_client_test.go index e7cb4dfb..ac42ac5c 100644 --- a/pkg/api/http_client_test.go +++ b/pkg/api/http_client_test.go @@ -158,6 +158,74 @@ func TestNewHTTPClient(t *testing.T) { } } +func TestNewHTTPClientCheckRedirect(t *testing.T) { + // Redirect handling belongs to http.Client rather than the transport, so a + // stub transport still exercises the real policy: the client asks it for the + // redirected request only if the policy allows the redirect. + newRecordingTransport := func(methods *[]string) tripper { + return tripper{ + roundTrip: func(req *http.Request) (*http.Response, error) { + *methods = append(*methods, req.Method) + if len(*methods) == 1 { + return &http.Response{ + StatusCode: http.StatusMovedPermanently, + Header: http.Header{"Location": []string{"https://api.github.com/repos/OWNER/NEW"}}, + Body: io.NopCloser(bytes.NewBufferString("")), + }, nil + } + return &http.Response{ + StatusCode: http.StatusNoContent, + Body: io.NopCloser(bytes.NewBufferString("")), + }, nil + }, + } + } + + t.Run("follows redirects by default, downgrading DELETE to GET", func(t *testing.T) { + var methods []string + client, err := NewHTTPClient(ClientOptions{ + Host: "github.com", + AuthToken: "oauth_token", + Transport: newRecordingTransport(&methods), + }) + assert.NoError(t, err) + + req, err := http.NewRequest(http.MethodDelete, "https://api.github.com/repos/OWNER/OLD", nil) + assert.NoError(t, err) + res, err := client.Do(req) + assert.NoError(t, err) + defer res.Body.Close() + + // This is the behaviour that makes the option necessary. Go turns the + // DELETE into a GET when it follows the redirect, so the caller is told + // the request succeeded while nothing was deleted. + assert.Equal(t, []string{http.MethodDelete, http.MethodGet}, methods) + assert.Equal(t, http.StatusNoContent, res.StatusCode) + }) + + t.Run("honours a CheckRedirect that stops at the redirect", func(t *testing.T) { + var methods []string + client, err := NewHTTPClient(ClientOptions{ + Host: "github.com", + AuthToken: "oauth_token", + Transport: newRecordingTransport(&methods), + CheckRedirect: func(req *http.Request, via []*http.Request) error { + return http.ErrUseLastResponse + }, + }) + assert.NoError(t, err) + + req, err := http.NewRequest(http.MethodDelete, "https://api.github.com/repos/OWNER/OLD", nil) + assert.NoError(t, err) + res, err := client.Do(req) + assert.NoError(t, err) + defer res.Body.Close() + + assert.Equal(t, []string{http.MethodDelete}, methods) + assert.Equal(t, http.StatusMovedPermanently, res.StatusCode) + }) +} + type tripper struct { roundTrip func(*http.Request) (*http.Response, error) }