diff --git a/pkg/api/client_options.go b/pkg/api/client_options.go index 3464aac..2e25f48 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 c2f0d79..e3a837a 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 e7cb4df..ac42ac5 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) }