diff --git a/acceptance/acceptance_test.go b/acceptance/acceptance_test.go index 9030e050611..c8ae750a047 100644 --- a/acceptance/acceptance_test.go +++ b/acceptance/acceptance_test.go @@ -85,6 +85,15 @@ func TestAuth(t *testing.T) { testscript.Run(t, testScriptParamsFor(tsEnv, "auth")) } +func TestGists(t *testing.T) { + var tsEnv testScriptEnv + if err := tsEnv.fromEnv(); err != nil { + t.Fatal(err) + } + + testscript.Run(t, testScriptParamsFor(tsEnv, "gist")) +} + func TestGPGKeys(t *testing.T) { var tsEnv testScriptEnv if err := tsEnv.fromEnv(); err != nil { diff --git a/acceptance/testdata/gist/gist-create-view-delete.txtar b/acceptance/testdata/gist/gist-create-view-delete.txtar new file mode 100644 index 00000000000..86ed08f53fb --- /dev/null +++ b/acceptance/testdata/gist/gist-create-view-delete.txtar @@ -0,0 +1,40 @@ +# Gists are owned by the authenticated user rather than an org, so unlike most +# other acceptance scripts there is no repository to create or clean up. +# +# Not covered here: gists whose files exceed the API's 1MB inline limit come +# back truncated, and the full content is then fetched from a raw URL on +# gist.githubusercontent.com rather than the API host. Triggering that needs a +# file too large to keep in a txtar, so it is left to unit tests. + +# Setup useful env vars +env GIST_DESC=${SCRIPT_NAME}-${RANDOM_STRING} + +# Create a gist from a file, capturing its URL +exec gh gist create gist-file.txt --desc ${GIST_DESC} +stdout 'https://gist.github.com/' +stdout2env GIST_URL + +# View the gist and check the description and content are both rendered +exec gh gist view ${GIST_URL} +stdout ${GIST_DESC} +stdout 'hello from the acceptance tests' + +# List the file names in the gist +exec gh gist view ${GIST_URL} --files +stdout 'gist-file.txt' + +# View a single file raw, which must be the content we uploaded and nothing else +exec gh gist view ${GIST_URL} --filename gist-file.txt --raw +stdout 'hello from the acceptance tests' +! stdout ${GIST_DESC} + +# Delete the gist. This is deliberately not deferred because deletion is what +# the script is testing, and a deferred delete would fail on the second attempt. +exec gh gist delete --yes ${GIST_URL} + +# Check the gist is gone +! exec gh gist view ${GIST_URL} +stderr 'not found' + +-- gist-file.txt -- +hello from the acceptance tests diff --git a/acceptance/testdata/gist/gist-edit-rename-list.txtar b/acceptance/testdata/gist/gist-edit-rename-list.txtar new file mode 100644 index 00000000000..537e63b9295 --- /dev/null +++ b/acceptance/testdata/gist/gist-edit-rename-list.txtar @@ -0,0 +1,60 @@ +# Gists are owned by the authenticated user rather than an org, so unlike most +# other acceptance scripts there is no repository to create or clean up. +# +# Only --add is exercised here. Editing a file's content, and editing the +# description on its own, both fall through to an interactive editor, so they +# cannot be driven from a script. --remove is left out deliberately, because it +# issues the same update request as --add and so covers no further ground. +# +# Every update to an existing gist is followed by a sleep. A read that comes +# straight after an update serves the previous state around twice in ten +# attempts, and it settles within about a second and a half. The sleeps matter +# most before the next gh command rather than before an assertion: rename reads +# the gist to build its update, so a stale read there renames nothing at all and +# still exits 0, leaving the gist permanently wrong rather than briefly behind. +# Creating a gist reads back consistently, so it needs no sleep. + +# Setup useful env vars +env GIST_DESC=${SCRIPT_NAME}-${RANDOM_STRING} + +# Create a gist from a file, capturing its URL +exec gh gist create first.txt --desc ${GIST_DESC} +stdout2env GIST_URL + +# Defer gist cleanup +defer gh gist delete --yes ${GIST_URL} + +# Add a second file to the gist +exec gh gist edit ${GIST_URL} --add second.txt +sleep 5 + +# Check both files are now in the gist +exec gh gist view ${GIST_URL} --files +stdout 'first.txt' +stdout 'second.txt' + +# Rename the second file. Checking that the new name is present is enough to +# prove the rename happened, and asserting the absence of the old name would +# only add another chance to read the gist before the write has settled. +exec gh gist rename ${GIST_URL} second.txt renamed.txt +sleep 5 + +# Check the rename took effect +exec gh gist view ${GIST_URL} --files +stdout 'first.txt' +stdout 'renamed.txt' + +# Check the renamed file kept its content +exec gh gist view ${GIST_URL} --filename renamed.txt --raw +stdout 'the second file' + +# Find the gist by description, which lists secret gists by default +exec gh gist list --filter ${GIST_DESC} +stdout ${GIST_DESC} +stdout '2 files' +stdout 'secret' + +-- first.txt -- +the first file +-- second.txt -- +the second file diff --git a/api/client.go b/api/client.go index 27a747995c9..07027cabb94 100644 --- a/api/client.go +++ b/api/client.go @@ -52,12 +52,50 @@ func (err HTTPError) ScopesSuggestion() string { return err.scopesSuggestion } +// RequestOption configures a single request made through Request, RequestWithContext or GraphQL. +type RequestOption func(*requestConfig) + +// requestConfig accumulates the effect of the RequestOptions applied to one request. +type requestConfig struct { + headers map[string]string + endpointScopes string +} + +// WithHeader sets a header on the request, taking precedence over any header of the same name +// that the underlying transport would otherwise supply. +func WithHeader(name, value string) RequestOption { + return func(cfg *requestConfig) { + cfg.headers[name] = value + } +} + +// WithEndpointScopes adds OAuth scopes to a 4xx error as if the server endpoint had returned them. +// This improves error messaging for endpoints that do not explicitly list the scopes they need, and +// is the Request equivalent of calling EndpointNeedsScopes on a raw response. +func WithEndpointScopes(scopes string) RequestOption { + return func(cfg *requestConfig) { + cfg.endpointScopes = scopes + } +} + +func newRequestConfig(opts []RequestOption) requestConfig { + cfg := requestConfig{headers: map[string]string{}} + for _, opt := range opts { + opt(&cfg) + } + return cfg +} + // GraphQL performs a GraphQL request using the query string and parses the response into data receiver. If there are errors in the response, // GraphQLError will be returned, but the receiver will also be partially populated. -func (c Client) GraphQL(hostname string, query string, variables map[string]interface{}, data interface{}) error { - opts := clientOptions(hostname, c.http.Transport) - opts.Headers[graphqlFeatures] = features - gqlClient, err := ghAPI.NewGraphQLClient(opts) +func (c Client) GraphQL(hostname string, query string, variables map[string]interface{}, data interface{}, opts ...RequestOption) error { + cfg := newRequestConfig(opts) + clientOpts := clientOptions(hostname, c.http.Transport) + clientOpts.Headers[graphqlFeatures] = features + for name, value := range cfg.headers { + clientOpts.Headers[name] = value + } + gqlClient, err := ghAPI.NewGraphQLClient(clientOpts) if err != nil { return err } @@ -147,6 +185,82 @@ func (c Client) RESTWithNext(hostname string, method string, p string, body io.R return next, nil } +// Request performs a REST request and returns the response for the caller to consume. +// +// See RequestWithContext for the full semantics. +func (c Client) Request(hostname string, method string, p string, body io.Reader, opts ...RequestOption) (*http.Response, error) { + return c.RequestWithContext(context.Background(), hostname, method, p, body, opts...) +} + +// RequestWithContext performs a REST request and returns the response for the caller to consume, +// rather than decoding it into a receiver as REST does. It exists for call sites that need access +// to the response itself: streaming bodies, non-JSON bodies, response headers, or the status code +// of a successful response. +// +// p may be a path relative to the host's REST endpoint, or an absolute URL. Prefer a relative path. +// Absolute URLs are requested as given, which is correct for URLs supplied by the API itself, such +// as asset and upload URLs, but means they do not benefit from host-level endpoint resolution. +// +// Responses outside the 2xx range are returned as an HTTPError and their body is closed. On success +// the response is returned unread and the caller is responsible for closing its body. +func (c Client) RequestWithContext(ctx context.Context, hostname string, method string, p string, body io.Reader, opts ...RequestOption) (*http.Response, error) { + cfg := newRequestConfig(opts) + clientOpts := clientOptions(hostname, c.http.Transport) + for name, value := range cfg.headers { + clientOpts.Headers[name] = value + } + + restClient, err := ghAPI.NewRESTClient(clientOpts) + if err != nil { + return nil, err + } + + resp, err := restClient.RequestWithContext(ctx, method, p, body) + if err != nil { + return nil, handleRequestError(err, cfg.endpointScopes) + } + + return resp, nil +} + +// DoRequest sends a request that the caller has already built, and returns the response for the +// caller to consume. It applies the same error handling as Request. +// +// Prefer Request. DoRequest exists for the few call sites that must control something Request +// cannot express. That is either a field of the request which is neither method, path, body nor +// header, such as ContentLength and GetBody when uploading a release asset, or a property of the +// client itself, such as CheckRedirect. Request delegates to go-gh, which builds a client of its +// own from the transport alone, so a redirect policy set on the client passed to +// NewClientFromHTTP only survives when the request is sent here. +// +// Because the caller supplies the URL, requests sent this way do not benefit from host-level +// endpoint resolution, so it is only appropriate for absolute URLs. +// +// Only the endpoint scopes of any RequestOption are applied; headers are the caller's own to set +// on the request they built. +// +// Responses outside the 2xx range are returned as an HTTPError and their body is closed. On success +// the response is returned unread and the caller is responsible for closing its body. +func (c Client) DoRequest(req *http.Request, opts ...RequestOption) (*http.Response, error) { + cfg := newRequestConfig(opts) + + resp, err := c.http.Do(req) + if err != nil { + return nil, err + } + + success := resp.StatusCode >= 200 && resp.StatusCode < 300 + if !success { + defer resp.Body.Close() + if cfg.endpointScopes != "" { + EndpointNeedsScopes(resp, cfg.endpointScopes) + } + return nil, HandleHTTPError(resp) + } + + return resp, nil +} + // HandleHTTPError parses a http.Response into a HTTPError. // // The caller is responsible to close the response body stream. @@ -154,6 +268,21 @@ func HandleHTTPError(resp *http.Response) error { return handleResponse(ghAPI.HandleHTTPError(resp)) } +// handleRequestError converts a request error into an HTTPError or GraphQLError, first adding any +// endpoint scopes to the error's headers so that the scopes suggestion generated by handleResponse +// accounts for them. This is the error-path equivalent of calling EndpointNeedsScopes on a response +// before HandleHTTPError, which call sites can no longer do when the response never reaches them. +func handleRequestError(err error, endpointScopes string) error { + if endpointScopes != "" { + var restErr *ghAPI.HTTPError + if errors.As(err, &restErr) && restErr.Headers != nil && restErr.StatusCode >= 400 && restErr.StatusCode < 500 { + oldScopes := restErr.Headers.Get("X-Accepted-Oauth-Scopes") + restErr.Headers.Set("X-Accepted-Oauth-Scopes", fmt.Sprintf("%s, %s", oldScopes, endpointScopes)) + } + } + return handleResponse(err) +} + // handleResponse takes a ghAPI.HTTPError or ghAPI.GraphQLError and converts it into an // HTTPError or GraphQLError respectively. func handleResponse(err error) error { diff --git a/api/queries_repo.go b/api/queries_repo.go index 3e0b75648cb..6544b8ea548 100644 --- a/api/queries_repo.go +++ b/api/queries_repo.go @@ -13,7 +13,6 @@ import ( "time" "github.com/cli/cli/v2/internal/gh" - "github.com/cli/cli/v2/internal/ghinstance" "golang.org/x/sync/errgroup" "github.com/cli/cli/v2/internal/ghrepo" @@ -647,13 +646,13 @@ func RenameRepo(client *Client, repo ghrepo.Interface, newRepoName string) (*Rep return nil, err } - path, err := safeurl.JoinPathWithHostPrefix(ghinstance.RESTPrefix(repo.RepoHost()), "repos", repo.RepoOwner(), repo.RepoName()) + path, err := safeurl.JoinPath("repos", repo.RepoOwner(), repo.RepoName()) if err != nil { return nil, err } result := repositoryV3{} - err = client.REST(repo.RepoHost(), "PATCH", path.String(), body, &result) + err = client.REST(repo.RepoHost(), http.MethodPatch, path.String(), body, &result) if err != nil { return nil, err } @@ -1671,26 +1670,31 @@ func GetRepoIDs(client *Client, host string, repositories []ghrepo.Interface) ([ } func RepoExists(client *Client, repo ghrepo.Interface) (bool, error) { - u, err := safeurl.JoinPathWithHostPrefix(ghinstance.RESTPrefix(repo.RepoHost()), "repos", repo.RepoOwner(), repo.RepoName()) + path, err := safeurl.JoinPath("repos", repo.RepoOwner(), repo.RepoName()) if err != nil { return false, err } - resp, err := client.HTTP().Head(u.String()) + // A HEAD request has no body to decode, so Request is used rather than REST. Existence is + // decided by the status alone. + resp, err := client.Request(repo.RepoHost(), http.MethodHead, path.String(), nil) if err != nil { + var httpErr HTTPError + if errors.As(err, &httpErr) && httpErr.StatusCode == http.StatusNotFound { + return false, nil + } return false, err } defer resp.Body.Close() - switch resp.StatusCode { - case 200: - return true, nil - case 404: - return false, nil - default: - return false, ghAPI.HandleHTTPError(resp) + // Only 200 means the repository exists. Any other success status is unexpected here and is + // reported as an error rather than being taken as existence. + if resp.StatusCode != http.StatusOK { + return false, HandleHTTPError(resp) } + + return true, nil } // RepoLicenses fetches available repository licenses. diff --git a/api/queries_repo_test.go b/api/queries_repo_test.go index 4fe7074f187..73edfbabd6d 100644 --- a/api/queries_repo_test.go +++ b/api/queries_repo_test.go @@ -798,6 +798,20 @@ func TestRepoExists(t *testing.T) { existCheck: false, wantErrMsg: "HTTP 500 (https://api.github.com/repos/OWNER/REPO)", }, + { + // Only 200 counts as existence. A 2xx other than 200 is unexpected for this + // endpoint and must be reported rather than quietly taken as a yes. + name: "unexpected success status", + httpStub: func(r *httpmock.Registry) { + r.Register( + httpmock.REST("HEAD", "repos/OWNER/REPO"), + httpmock.StatusStringResponse(201, ""), + ) + }, + repo: ghrepo.New("OWNER", "REPO"), + existCheck: false, + wantErrMsg: "HTTP 201 (https://api.github.com/repos/OWNER/REPO)", + }, } for _, tt := range tests { reg := &httpmock.Registry{} diff --git a/api/request_test.go b/api/request_test.go new file mode 100644 index 00000000000..b0eea6de3b8 --- /dev/null +++ b/api/request_test.go @@ -0,0 +1,443 @@ +package api + +import ( + "bytes" + "context" + + "io" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/cli/cli/v2/pkg/httpmock" + "github.com/cli/cli/v2/pkg/iostreams" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestRequestResolvesPathAgainstHost(t *testing.T) { + tests := []struct { + name string + hostname string + path string + wantHostname string + wantPath string + }{ + { + name: "relative path resolves against github.com", + hostname: "github.com", + path: "repos/OWNER/REPO", + wantHostname: "api.github.com", + wantPath: "/repos/OWNER/REPO", + }, + { + name: "relative path resolves against an enterprise host", + hostname: "example.com", + path: "repos/OWNER/REPO", + wantHostname: "example.com", + wantPath: "/api/v3/repos/OWNER/REPO", + }, + { + // Absolute URLs are how API-supplied locations such as asset and upload URLs + // reach this method, so they must be requested exactly as given. + name: "absolute URL is requested as given", + hostname: "github.com", + path: "https://uploads.github.com/assets/1234", + wantHostname: "uploads.github.com", + wantPath: "/assets/1234", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + reg := &httpmock.Registry{} + defer reg.Verify(t) + client := newTestClient(reg) + + reg.Register(httpmock.MatchAny, httpmock.StatusStringResponse(200, "{}")) + + resp, err := client.Request(tt.hostname, http.MethodGet, tt.path, nil) + require.NoError(t, err) + defer resp.Body.Close() + + assert.Equal(t, tt.wantHostname, reg.Requests[0].URL.Hostname()) + assert.Equal(t, tt.wantPath, reg.Requests[0].URL.Path) + }) + } +} + +func TestRequestWithHeader(t *testing.T) { + reg := &httpmock.Registry{} + defer reg.Verify(t) + client := newTestClient(reg) + + reg.Register(httpmock.MatchAny, httpmock.StatusStringResponse(200, "{}")) + + resp, err := client.Request("github.com", http.MethodGet, "repos/OWNER/REPO", nil, + WithHeader("Accept", "application/vnd.github.raw")) + require.NoError(t, err) + defer resp.Body.Close() + + assert.Equal(t, "application/vnd.github.raw", reg.Requests[0].Header.Get("Accept")) +} + +// The transport supplies default headers of its own, so a per-request header is only useful if it +// takes precedence over them. +func TestRequestHeaderOverridesTransportDefault(t *testing.T) { + reg := &httpmock.Registry{} + defer reg.Verify(t) + client := newTestClient(reg) + + reg.Register(httpmock.MatchAny, httpmock.StatusStringResponse(200, "{}")) + + resp, err := client.Request("github.com", http.MethodPost, "repos/OWNER/REPO/releases", nil, + WithHeader("Content-Type", "application/zip")) + require.NoError(t, err) + defer resp.Body.Close() + + assert.Equal(t, "application/zip", reg.Requests[0].Header.Get("Content-Type")) +} + +// Pre-auth call sites supply their own token because it is not yet in config, so an explicit +// Authorization header must survive rather than be replaced by the transport. +func TestRequestExplicitAuthorizationHeaderIsPreserved(t *testing.T) { + reg := &httpmock.Registry{} + defer reg.Verify(t) + + httpClient := &http.Client{} + httpmock.ReplaceTripper(httpClient, reg) + httpClient.Transport = AddAuthTokenHeader(httpClient.Transport, tinyConfig{"github.com:oauth_token": "config-token"}) + client := NewClientFromHTTP(httpClient) + + reg.Register(httpmock.MatchAny, httpmock.StatusStringResponse(200, "{}")) + + resp, err := client.Request("github.com", http.MethodGet, "user", nil, + WithHeader("Authorization", "token explicit-token")) + require.NoError(t, err) + defer resp.Body.Close() + + assert.Equal(t, "token explicit-token", reg.Requests[0].Header.Get("Authorization")) +} + +func TestRequestReturnsBodyUnread(t *testing.T) { + reg := &httpmock.Registry{} + defer reg.Verify(t) + client := newTestClient(reg) + + reg.Register(httpmock.MatchAny, httpmock.StatusStringResponse(200, "the raw body")) + + resp, err := client.Request("github.com", http.MethodGet, "repos/OWNER/REPO/contents/f", nil) + require.NoError(t, err) + defer resp.Body.Close() + + body, err := io.ReadAll(resp.Body) + require.NoError(t, err) + assert.Equal(t, "the raw body", string(body)) +} + +// REST treats 204 and 205 specially and decodes every other 2xx as JSON. Request must do neither, +// so that bodiless successes and non-JSON successes both work. +func TestRequestDoesNotDecodeSuccessfulResponses(t *testing.T) { + tests := []struct { + name string + statusCode int + body string + }{ + {name: "bodiless 200, as returned by HEAD", statusCode: 200, body: ""}, + {name: "no content", statusCode: 204, body: ""}, + {name: "non-JSON body", statusCode: 200, body: "d3f9c4a"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + reg := &httpmock.Registry{} + defer reg.Verify(t) + client := newTestClient(reg) + + reg.Register(httpmock.MatchAny, httpmock.StatusStringResponse(tt.statusCode, tt.body)) + + resp, err := client.Request("github.com", http.MethodGet, "repos/OWNER/REPO", nil) + require.NoError(t, err) + defer resp.Body.Close() + + assert.Equal(t, tt.statusCode, resp.StatusCode) + }) + } +} + +func TestRequestError(t *testing.T) { + reg := &httpmock.Registry{} + defer reg.Verify(t) + client := newTestClient(reg) + + reg.Register(httpmock.MatchAny, httpmock.StatusJSONResponse(404, map[string]interface{}{ + "message": "Not Found", + })) + + resp, err := client.Request("github.com", http.MethodGet, "repos/OWNER/REPO", nil) + assert.Nil(t, resp) + + var httpErr HTTPError + require.ErrorAs(t, err, &httpErr) + assert.Equal(t, 404, httpErr.StatusCode) +} + +func TestRequestErrorPreservesScopesSuggestion(t *testing.T) { + reg := &httpmock.Registry{} + defer reg.Verify(t) + client := newTestClient(reg) + + reg.Register(httpmock.MatchAny, func(req *http.Request) (*http.Response, error) { + return &http.Response{ + Request: req, + StatusCode: 404, + Body: io.NopCloser(bytes.NewBufferString(`{"message": "Not Found"}`)), + Header: map[string][]string{ + "Content-Type": {"application/json; charset=utf-8"}, + "X-Accepted-Oauth-Scopes": {"delete_repo"}, + "X-Oauth-Scopes": {"repo"}, + }, + }, nil + }) + + _, err := client.Request("github.com", http.MethodDelete, "repos/OWNER/REPO", nil) + + var httpErr HTTPError + require.ErrorAs(t, err, &httpErr) + assert.Contains(t, httpErr.ScopesSuggestion(), "delete_repo") +} + +// Call sites used to mutate the response with EndpointNeedsScopes before converting it to an error. +// Request converts internally, so WithEndpointScopes has to reproduce that suggestion. +func TestRequestWithEndpointScopes(t *testing.T) { + tests := []struct { + name string + statusCode int + acceptedScopes string + wantSuggestion string + }{ + { + name: "adds scopes the endpoint did not report", + statusCode: 403, + acceptedScopes: "", + wantSuggestion: "delete_repo", + }, + { + name: "appends to scopes the endpoint did report", + statusCode: 403, + acceptedScopes: "repo", + wantSuggestion: "delete_repo", + }, + { + name: "leaves 5xx errors alone", + statusCode: 500, + acceptedScopes: "", + wantSuggestion: "", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + reg := &httpmock.Registry{} + defer reg.Verify(t) + client := newTestClient(reg) + + reg.Register(httpmock.MatchAny, func(req *http.Request) (*http.Response, error) { + header := map[string][]string{ + "Content-Type": {"application/json; charset=utf-8"}, + "X-Oauth-Scopes": {"repo"}, + } + if tt.acceptedScopes != "" { + header["X-Accepted-Oauth-Scopes"] = []string{tt.acceptedScopes} + } + return &http.Response{ + Request: req, + StatusCode: tt.statusCode, + Body: io.NopCloser(bytes.NewBufferString(`{"message": "Forbidden"}`)), + Header: header, + }, nil + }) + + _, err := client.Request("github.com", http.MethodDelete, "repos/OWNER/REPO", nil, + WithEndpointScopes("delete_repo")) + + var httpErr HTTPError + require.ErrorAs(t, err, &httpErr) + if tt.wantSuggestion == "" { + assert.Empty(t, httpErr.ScopesSuggestion()) + } else { + assert.Contains(t, httpErr.ScopesSuggestion(), tt.wantSuggestion) + } + }) + } +} + +type ctxKey struct{} + +// Call sites such as release asset upload run inside a retry loop that owns a context, so the +// context has to reach the outgoing request. +func TestRequestWithContextPropagatesContext(t *testing.T) { + reg := &httpmock.Registry{} + defer reg.Verify(t) + client := newTestClient(reg) + + var gotValue interface{} + reg.Register(httpmock.MatchAny, func(req *http.Request) (*http.Response, error) { + gotValue = req.Context().Value(ctxKey{}) + return httpmock.StatusStringResponse(200, "{}")(req) + }) + + ctx := context.WithValue(context.Background(), ctxKey{}, "carried") + + resp, err := client.RequestWithContext(ctx, "github.com", http.MethodGet, "repos/OWNER/REPO", nil) + require.NoError(t, err) + defer resp.Body.Close() + + assert.Equal(t, "carried", gotValue) +} + +func TestDoRequest(t *testing.T) { + reg := &httpmock.Registry{} + defer reg.Verify(t) + client := newTestClient(reg) + + reg.Register(httpmock.MatchAny, httpmock.StatusStringResponse(201, `{"id": 1}`)) + + // Fields such as ContentLength are the reason DoRequest exists, so assert one survives. + req, err := http.NewRequest(http.MethodPost, "https://uploads.github.com/assets", strings.NewReader("asset")) + require.NoError(t, err) + req.ContentLength = 5 + + resp, err := client.DoRequest(req) + require.NoError(t, err) + defer resp.Body.Close() + + assert.Equal(t, 201, resp.StatusCode) + assert.Equal(t, "uploads.github.com", reg.Requests[0].URL.Hostname()) + assert.Equal(t, int64(5), reg.Requests[0].ContentLength) + + body, err := io.ReadAll(resp.Body) + require.NoError(t, err) + assert.Equal(t, `{"id": 1}`, string(body)) +} + +func TestDoRequestError(t *testing.T) { + reg := &httpmock.Registry{} + defer reg.Verify(t) + client := newTestClient(reg) + + reg.Register(httpmock.MatchAny, httpmock.StatusJSONResponse(422, map[string]interface{}{ + "message": "Validation Failed", + })) + + req, err := http.NewRequest(http.MethodPost, "https://uploads.github.com/assets", nil) + require.NoError(t, err) + + resp, err := client.DoRequest(req) + assert.Nil(t, resp) + + var httpErr HTTPError + require.ErrorAs(t, err, &httpErr) + assert.Equal(t, 422, httpErr.StatusCode) + assert.Equal(t, "Validation Failed", httpErr.Message) +} + +// The transport built by NewHTTPClient supplies default Accept, Content-Type and User-Agent +// headers. Per-request headers are only useful if they win against that real transport, so this +// exercises it rather than a bare test tripper. +func TestRequestHeadersAgainstRealTransport(t *testing.T) { + var gotReq *http.Request + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + gotReq = r + w.WriteHeader(http.StatusOK) + })) + defer ts.Close() + + ios, _, _, _ := iostreams.Test() + httpClient, err := NewHTTPClient(HTTPClientOptions{ + AppVersion: "v1.2.3", + Config: tinyConfig{ts.URL[7:] + ":oauth_token": "MYTOKEN"}, + Log: ios.ErrOut, + }) + require.NoError(t, err) + client := NewClientFromHTTP(httpClient) + + resp, err := client.Request(ts.URL, http.MethodGet, ts.URL+"/user/repos", nil, + WithHeader("Accept", "application/vnd.github.raw"), + WithHeader("Content-Type", "application/zip")) + require.NoError(t, err) + defer resp.Body.Close() + + assert.Equal(t, "application/vnd.github.raw", gotReq.Header.Get("Accept")) + assert.Equal(t, "application/zip", gotReq.Header.Get("Content-Type")) + + // Headers the caller did not override must still be supplied by the transport. + assert.Equal(t, "token MYTOKEN", gotReq.Header.Get("Authorization")) + assert.Equal(t, "GitHub CLI v1.2.3", gotReq.Header.Get("User-Agent")) +} + +func TestGraphQLWithHeader(t *testing.T) { + reg := &httpmock.Registry{} + defer reg.Verify(t) + client := newTestClient(reg) + + reg.Register(httpmock.MatchAny, httpmock.StatusStringResponse(200, `{"data": {}}`)) + + var response struct{} + err := client.GraphQL("github.com", "query{}", nil, &response, + WithHeader("Authorization", "token explicit-token")) + require.NoError(t, err) + + assert.Equal(t, "token explicit-token", reg.Requests[0].Header.Get("Authorization")) +} + +// TestDoRequestPreservesCheckRedirect pins the difference between Request and DoRequest that +// motivates DoRequest existing for redirect-sensitive call sites. Request delegates to go-gh, +// which builds an http.Client of its own from the transport alone, so a redirect policy set on +// the client cannot survive. DoRequest sends through that client and so keeps it. +func TestDoRequestPreservesCheckRedirect(t *testing.T) { + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == "/start" { + http.Redirect(w, r, "/final", http.StatusFound) + return + } + w.WriteHeader(http.StatusOK) + })) + defer ts.Close() + + newClient := func(called *bool) *http.Client { + return &http.Client{CheckRedirect: func(req *http.Request, via []*http.Request) error { + *called = true + return http.ErrUseLastResponse + }} + } + + t.Run("DoRequest honours it", func(t *testing.T) { + var called bool + req, err := http.NewRequest(http.MethodGet, ts.URL+"/start", nil) + require.NoError(t, err) + + // The redirect is not followed, so the 3xx is the final response and, being outside + // the 2xx range, is reported as an error. Call sites such as gh repo delete rely on + // seeing that status to explain that a repo was renamed or transferred. + _, err = NewClientFromHTTP(newClient(&called)).DoRequest(req) + require.Error(t, err) + + var httpErr HTTPError + require.ErrorAs(t, err, &httpErr) + assert.True(t, called, "CheckRedirect should have been consulted") + assert.Equal(t, http.StatusFound, httpErr.StatusCode) + }) + + t.Run("Request cannot honour it", func(t *testing.T) { + var called bool + + resp, err := NewClientFromHTTP(newClient(&called)).Request("github.com", http.MethodGet, ts.URL+"/start", nil) + require.NoError(t, err) + defer resp.Body.Close() + + assert.False(t, called, "go-gh builds its own client, so the policy cannot apply") + assert.Equal(t, http.StatusOK, resp.StatusCode) + }) +} diff --git a/internal/update/update.go b/internal/update/update.go index 27a7d8a248f..d0af16a632f 100644 --- a/internal/update/update.go +++ b/internal/update/update.go @@ -13,6 +13,7 @@ import ( "strings" "time" + "github.com/cli/cli/v2/api" "github.com/cli/cli/v2/internal/ci" "github.com/cli/cli/v2/internal/safeurl" "github.com/cli/cli/v2/pkg/extensions" @@ -121,11 +122,12 @@ func getLatestReleaseInfo(ctx context.Context, client *http.Client, repo string) if err != nil { return nil, err } - req, err := http.NewRequestWithContext(ctx, "GET", u.String(), nil) - if err != nil { - return nil, err - } - res, err := client.Do(req) + // The URL stays absolute on purpose: CLI releases live on github.com regardless of the + // host the user has configured, so this must not be rewritten to their API host. + // TODO(api-client-rollout) + // This line of code is part of a mechanical roll out of the api client. + // As a follow up, consider whether the api client can be injected to this call site, rather than constructed + res, err := api.NewClientFromHTTP(client).RequestWithContext(ctx, "github.com", http.MethodGet, u.String(), nil) if err != nil { return nil, err } diff --git a/pkg/cmd/auth/shared/login_flow.go b/pkg/cmd/auth/shared/login_flow.go index c76dc5fb84c..a43f919b050 100644 --- a/pkg/cmd/auth/shared/login_flow.go +++ b/pkg/cmd/auth/shared/login_flow.go @@ -1,8 +1,6 @@ package shared import ( - "bytes" - "encoding/json" "fmt" "net/http" "os" @@ -13,8 +11,6 @@ import ( "github.com/cli/cli/v2/api" "github.com/cli/cli/v2/internal/authflow" "github.com/cli/cli/v2/internal/browser" - "github.com/cli/cli/v2/internal/ghinstance" - "github.com/cli/cli/v2/internal/safeurl" "github.com/cli/cli/v2/pkg/cmd/ssh-key/add" "github.com/cli/cli/v2/pkg/iostreams" "github.com/cli/cli/v2/pkg/ssh" @@ -250,36 +246,21 @@ func sshKeyUpload(httpClient *http.Client, hostname, keyFile string, title strin return add.SSHKeyUpload(httpClient, hostname, f, title) } -func GetCurrentLogin(httpClient httpClient, hostname, authToken string) (string, error) { - query := `query UserCurrent{viewer{login}}` - reqBody, err := json.Marshal(map[string]interface{}{"query": query}) +// GetCurrentLogin returns the login of the user the token belongs to. +// +// The token is passed explicitly because this runs before the token is stored in config, so +// the transport has nothing to attach. The transport only sets Authorization when it is +// absent, so the header set here wins. +func GetCurrentLogin(httpClient *http.Client, hostname, authToken string) (string, error) { + var result struct{ Viewer struct{ Login string } } + + // TODO(api-client-rollout) + // This line of code is part of a mechanical roll out of the api client. + // As a follow up, consider whether the api client can be injected to this call site, rather than constructed + err := api.NewClientFromHTTP(httpClient).GraphQL(hostname, `query UserCurrent{viewer{login}}`, nil, &result, + api.WithHeader("Authorization", "token "+authToken)) if err != nil { return "", err } - result := struct { - Data struct{ Viewer struct{ Login string } } - }{} - apiEndpoint, err := safeurl.JoinPathWithHostPrefix(ghinstance.GraphQLEndpoint(hostname)) - if err != nil { - return "", err - } - req, err := http.NewRequest("POST", apiEndpoint.String(), bytes.NewBuffer(reqBody)) - if err != nil { - return "", err - } - req.Header.Set("Authorization", "token "+authToken) - res, err := httpClient.Do(req) - if err != nil { - return "", err - } - defer res.Body.Close() - if res.StatusCode > 299 { - return "", api.HandleHTTPError(res) - } - decoder := json.NewDecoder(res.Body) - err = decoder.Decode(&result) - if err != nil { - return "", err - } - return result.Data.Viewer.Login, nil + return result.Viewer.Login, nil } diff --git a/pkg/cmd/auth/shared/oauth_scopes.go b/pkg/cmd/auth/shared/oauth_scopes.go index bc5e611163a..67886b96b85 100644 --- a/pkg/cmd/auth/shared/oauth_scopes.go +++ b/pkg/cmd/auth/shared/oauth_scopes.go @@ -7,8 +7,6 @@ import ( "strings" "github.com/cli/cli/v2/api" - "github.com/cli/cli/v2/internal/ghinstance" - "github.com/cli/cli/v2/internal/safeurl" ) type MissingScopesError struct { @@ -28,25 +26,17 @@ func (e MissingScopesError) Error() string { return "missing required scopes " + scopes } -type httpClient interface { - Do(*http.Request) (*http.Response, error) -} - // GetScopes performs a GitHub API request and returns the value of the X-Oauth-Scopes header. -func GetScopes(httpClient httpClient, hostname, authToken string) (string, error) { - apiEndpoint, err := safeurl.JoinPathWithHostPrefix(ghinstance.RESTPrefix(hostname)) - if err != nil { - return "", err - } - - req, err := http.NewRequest("GET", apiEndpoint.String(), nil) - if err != nil { - return "", err - } - - req.Header.Set("Authorization", "token "+authToken) - - res, err := httpClient.Do(req) +// +// The token is passed explicitly because this runs before the token is stored in config, so +// the transport has nothing to attach. The transport only sets Authorization when it is +// absent, so the header set here wins. +func GetScopes(httpClient *http.Client, hostname, authToken string) (string, error) { + // TODO(api-client-rollout) + // This line of code is part of a mechanical roll out of the api client. + // As a follow up, consider whether the api client can be injected to this call site, rather than constructed + res, err := api.NewClientFromHTTP(httpClient).Request(hostname, http.MethodGet, "", nil, + api.WithHeader("Authorization", "token "+authToken)) if err != nil { return "", err } @@ -67,7 +57,7 @@ func GetScopes(httpClient httpClient, hostname, authToken string) (string, error // HasMinimumScopes performs a GitHub API request and returns an error if the token used in the request // lacks the minimum required scopes for performing API operations with gh. -func HasMinimumScopes(httpClient httpClient, hostname, authToken string) error { +func HasMinimumScopes(httpClient *http.Client, hostname, authToken string) error { scopesHeader, err := GetScopes(httpClient, hostname, authToken) if err != nil { return err diff --git a/pkg/cmd/extension/http.go b/pkg/cmd/extension/http.go index 333564d29a4..740698871f5 100644 --- a/pkg/cmd/extension/http.go +++ b/pkg/cmd/extension/http.go @@ -8,38 +8,38 @@ import ( "os" "github.com/cli/cli/v2/api" - "github.com/cli/cli/v2/internal/ghinstance" "github.com/cli/cli/v2/internal/ghrepo" "github.com/cli/cli/v2/internal/safeurl" ) func repoExists(httpClient *http.Client, repo ghrepo.Interface) (bool, error) { - url, err := safeurl.JoinPathWithHostPrefix(ghinstance.RESTPrefix(repo.RepoHost()), "repos", repo.RepoOwner(), repo.RepoName()) - if err != nil { - return false, err - } - - req, err := http.NewRequest(http.MethodGet, url.String(), nil) + path, err := safeurl.JoinPath("repos", repo.RepoOwner(), repo.RepoName()) if err != nil { return false, err } + // The response body is deliberately not read. Existence is decided by the status alone, + // so Request is used rather than REST, which would try to decode the body. // TODO(api-client-rollout) - // This has been deferred from moving to api.Client due to its exact-status contract and body-blind response handling. - resp, err := httpClient.Do(req) + // This line of code is part of a mechanical roll out of the api client. + // As a follow up, consider whether the api client can be injected to this call site, rather than constructed + resp, err := api.NewClientFromHTTP(httpClient).Request(repo.RepoHost(), http.MethodGet, path.String(), nil) if err != nil { + var httpErr api.HTTPError + if errors.As(err, &httpErr) && httpErr.StatusCode == http.StatusNotFound { + return false, nil + } return false, err } defer resp.Body.Close() - switch resp.StatusCode { - case http.StatusOK: - return true, nil - case http.StatusNotFound: - return false, nil - default: + // Only 200 means the repository exists. Any other success status is unexpected here and is + // reported as an error rather than being taken as existence. + if resp.StatusCode != http.StatusOK { return false, api.HandleHTTPError(resp) } + + return true, nil } func hasScript(httpClient *http.Client, repo ghrepo.Interface) (bool, error) { @@ -76,27 +76,22 @@ type release struct { } // downloadAsset downloads a single asset to the given file path. -func downloadAsset(httpClient *http.Client, assetURL safeurl.SafeURL, destPath string) (downloadErr error) { - var req *http.Request - if req, downloadErr = http.NewRequest("GET", assetURL.String(), nil); downloadErr != nil { - return - } - - req.Header.Set("Accept", "application/octet-stream") - +// +// assetURL is an absolute URL supplied by the API, so it is requested as given rather than +// being resolved against the host's REST endpoint. hostname is still needed so the request +// is made through a client configured for the right host. +func downloadAsset(httpClient *http.Client, hostname string, assetURL safeurl.SafeURL, destPath string) (downloadErr error) { var resp *http.Response // TODO(api-client-rollout) - // This has been deferred from moving to api.Client due to its custom Accept header and binary response streaming. - if resp, downloadErr = httpClient.Do(req); downloadErr != nil { + // This line of code is part of a mechanical roll out of the api client. + // As a follow up, consider whether the api client can be injected to this call site, rather than constructed + resp, downloadErr = api.NewClientFromHTTP(httpClient).Request(hostname, http.MethodGet, assetURL.String(), nil, + api.WithHeader("Accept", "application/octet-stream")) + if downloadErr != nil { return } defer resp.Body.Close() - if resp.StatusCode > 299 { - downloadErr = api.HandleHTTPError(resp) - return - } - var f *os.File if f, downloadErr = os.OpenFile(destPath, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, 0755); downloadErr != nil { return @@ -173,30 +168,25 @@ func fetchReleaseFromTag(httpClient *http.Client, baseRepo ghrepo.Interface, tag // fetchCommitSHA finds full commit SHA from a target ref in a repo func fetchCommitSHA(httpClient *http.Client, baseRepo ghrepo.Interface, targetRef string) (string, error) { - url, err := safeurl.JoinPathWithHostPrefix(ghinstance.RESTPrefix(baseRepo.RepoHost()), "repos", baseRepo.RepoOwner(), baseRepo.RepoName(), "commits", targetRef) - if err != nil { - return "", err - } - req, err := http.NewRequest("GET", url.String(), nil) + path, err := safeurl.JoinPath("repos", baseRepo.RepoOwner(), baseRepo.RepoName(), "commits", targetRef) if err != nil { return "", err } - req.Header.Set("Accept", "application/vnd.github.v3.sha") + // The response body is a bare SHA rather than JSON, so Request is used instead of REST. // TODO(api-client-rollout) - // This has been deferred from moving to api.Client due to its custom Accept header and bare SHA response body. - resp, err := httpClient.Do(req) + // This line of code is part of a mechanical roll out of the api client. + // As a follow up, consider whether the api client can be injected to this call site, rather than constructed + resp, err := api.NewClientFromHTTP(httpClient).Request(baseRepo.RepoHost(), http.MethodGet, path.String(), nil, + api.WithHeader("Accept", "application/vnd.github.v3.sha")) if err != nil { + var httpErr api.HTTPError + if errors.As(err, &httpErr) && httpErr.StatusCode == http.StatusUnprocessableEntity { + return "", commitNotFoundErr + } return "", err } - defer resp.Body.Close() - if resp.StatusCode == 422 { - return "", commitNotFoundErr - } - if resp.StatusCode > 299 { - return "", api.HandleHTTPError(resp) - } body, err := io.ReadAll(resp.Body) if err != nil { diff --git a/pkg/cmd/extension/http_test.go b/pkg/cmd/extension/http_test.go index 7a262f78026..e1e9ceed0e2 100644 --- a/pkg/cmd/extension/http_test.go +++ b/pkg/cmd/extension/http_test.go @@ -3,10 +3,13 @@ package extension import ( "fmt" "net/http" + "os" + "path/filepath" "testing" "github.com/cli/cli/v2/api" "github.com/cli/cli/v2/internal/ghrepo" + "github.com/cli/cli/v2/internal/safeurl" "github.com/cli/cli/v2/pkg/httpmock" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -230,3 +233,84 @@ func TestFetchReleaseFromTag(t *testing.T) { }) } } + +// TestFetchCommitSHA covers the two things that kept this site on the raw client: the custom +// Accept media type that makes the API answer with a bare SHA, and the 422 sentinel. +func TestFetchCommitSHA(t *testing.T) { + repo := ghrepo.New("OWNER", "REPO") + + t.Run("sends the sha media type and returns the bare body", func(t *testing.T) { + reg := &httpmock.Registry{} + defer reg.Verify(t) + + reg.Register( + func(req *http.Request) bool { + return req.URL.Path == "/repos/OWNER/REPO/commits/main" && + req.Header.Get("Accept") == "application/vnd.github.v3.sha" + }, + httpmock.StatusStringResponse(http.StatusOK, "0123456789abcdef"), + ) + + sha, err := fetchCommitSHA(&http.Client{Transport: reg}, repo, "main") + + require.NoError(t, err) + assert.Equal(t, "0123456789abcdef", sha) + }) + + t.Run("unprocessable entity means the commit was not found", func(t *testing.T) { + client := extensionHTTPClient(t, "repos/OWNER/REPO/commits/nope", http.StatusUnprocessableEntity, `{"message":"No commit found"}`) + + _, err := fetchCommitSHA(client, repo, "nope") + + require.ErrorIs(t, err, commitNotFoundErr) + }) + + t.Run("other errors are reported", func(t *testing.T) { + client := extensionHTTPClient(t, "repos/OWNER/REPO/commits/main", http.StatusInternalServerError, `{"message":"Internal Server Error"}`) + + _, err := fetchCommitSHA(client, repo, "main") + + requireExtensionHTTPError(t, err, http.StatusInternalServerError) + }) +} + +// TestDownloadAsset pins the octet-stream media type, without which the API returns asset +// metadata as JSON rather than the binary itself. +func TestDownloadAsset(t *testing.T) { + t.Run("sends the octet-stream media type and writes the body", func(t *testing.T) { + reg := &httpmock.Registry{} + defer reg.Verify(t) + + reg.Register( + func(req *http.Request) bool { + return req.URL.String() == "https://example.com/release/cool" && + req.Header.Get("Accept") == "application/octet-stream" + }, + httpmock.StatusStringResponse(http.StatusOK, "BINARY"), + ) + + destPath := filepath.Join(t.TempDir(), "gh-cool") + err := downloadAsset(&http.Client{Transport: reg}, "github.com", safeurl.NewImmutableSafeURL("https://example.com/release/cool"), destPath) + + require.NoError(t, err) + contents, err := os.ReadFile(destPath) + require.NoError(t, err) + assert.Equal(t, "BINARY", string(contents)) + }) + + t.Run("errors are reported", func(t *testing.T) { + reg := &httpmock.Registry{} + defer reg.Verify(t) + + reg.Register( + httpmock.REST(http.MethodGet, "release/cool"), + httpmock.StatusStringResponse(http.StatusNotFound, `{"message":"Not Found"}`), + ) + + destPath := filepath.Join(t.TempDir(), "gh-cool") + err := downloadAsset(&http.Client{Transport: reg}, "github.com", safeurl.NewImmutableSafeURL("https://example.com/release/cool"), destPath) + + requireExtensionHTTPError(t, err, http.StatusNotFound) + assert.NoFileExists(t, destPath) + }) +} diff --git a/pkg/cmd/extension/manager.go b/pkg/cmd/extension/manager.go index 82b7fbfe0f8..112efad876b 100644 --- a/pkg/cmd/extension/manager.go +++ b/pkg/cmd/extension/manager.go @@ -351,7 +351,7 @@ func (m *Manager) installBin(repo ghrepo.Interface, target string) error { binPath := filepath.Join(targetDir, name) binPath += ext - err = downloadAsset(m.client, safeurl.NewImmutableSafeURL(asset.APIURL), binPath) + err = downloadAsset(m.client, repo.RepoHost(), safeurl.NewImmutableSafeURL(asset.APIURL), binPath) if err != nil { return fmt.Errorf("failed to download asset %s: %w", asset.Name, err) } diff --git a/pkg/cmd/extension/manager_test.go b/pkg/cmd/extension/manager_test.go index 567f3dba3ba..d09585a85a4 100644 --- a/pkg/cmd/extension/manager_test.go +++ b/pkg/cmd/extension/manager_test.go @@ -540,7 +540,7 @@ func TestManager_MigrateToBinaryExtension(t *testing.T) { Assets: []releaseAsset{ { Name: "gh-remote-windows-amd64.exe", - APIURL: "/release/cool", + APIURL: "https://example.com/release/cool", }, }, })) @@ -552,7 +552,7 @@ func TestManager_MigrateToBinaryExtension(t *testing.T) { Assets: []releaseAsset{ { Name: "gh-remote-windows-amd64.exe", - APIURL: "/release/cool", + APIURL: "https://example.com/release/cool", }, }, })) diff --git a/pkg/cmd/gist/create/create.go b/pkg/cmd/gist/create/create.go index 6ed4f66263d..76f918bee93 100644 --- a/pkg/cmd/gist/create/create.go +++ b/pkg/cmd/gist/create/create.go @@ -17,7 +17,6 @@ import ( "github.com/cli/cli/v2/api" "github.com/cli/cli/v2/internal/browser" "github.com/cli/cli/v2/internal/gh" - "github.com/cli/cli/v2/internal/ghinstance" "github.com/cli/cli/v2/internal/safeurl" "github.com/cli/cli/v2/internal/text" "github.com/cli/cli/v2/pkg/cmd/gist/shared" @@ -273,27 +272,23 @@ func createGist(client *http.Client, hostname, description string, public bool, return nil, err } - u, err := safeurl.JoinPathWithHostPrefix(ghinstance.RESTPrefix(hostname), "gists") + path, err := safeurl.JoinPath("gists") if err != nil { return nil, err } - req, err := http.NewRequest(http.MethodPost, u.String(), requestBody) - if err != nil { - return nil, err - } - req.Header.Set("Content-Type", "application/json; charset=utf-8") - resp, err := client.Do(req) + // The Content-Type this site used to set explicitly is the one the client already sends, + // so it is no longer stated here. + // TODO(api-client-rollout) + // This line of code is part of a mechanical roll out of the api client. + // As a follow up, consider whether the api client can be injected to this call site, rather than constructed + resp, err := api.NewClientFromHTTP(client).Request(hostname, http.MethodPost, path.String(), requestBody, + api.WithEndpointScopes("gist")) if err != nil { return nil, err } defer resp.Body.Close() - if resp.StatusCode > 299 { - api.EndpointNeedsScopes(resp, "gist") - return nil, api.HandleHTTPError(resp) - } - result := &shared.Gist{} dec := json.NewDecoder(resp.Body) if err := dec.Decode(result); err != nil { diff --git a/pkg/cmd/gist/create/create_test.go b/pkg/cmd/gist/create/create_test.go index 44f0ba284ef..0885082926b 100644 --- a/pkg/cmd/gist/create/create_test.go +++ b/pkg/cmd/gist/create/create_test.go @@ -11,6 +11,7 @@ import ( "testing" "github.com/MakeNowJust/heredoc" + "github.com/cli/cli/v2/api" "github.com/cli/cli/v2/internal/browser" "github.com/cli/cli/v2/internal/config" "github.com/cli/cli/v2/internal/gh" @@ -21,6 +22,7 @@ import ( "github.com/cli/cli/v2/pkg/iostreams" "github.com/google/shlex" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func Test_processFiles(t *testing.T) { @@ -415,3 +417,32 @@ func Test_detectEmptyFiles(t *testing.T) { assert.Equal(t, tt.isEmptyFile, isEmptyFile) } } + +// Test_createGist_missingScope pins the scopes suggestion, which is the reason this call site +// declares the gist scope rather than relying on what the API reports. +func Test_createGist_missingScope(t *testing.T) { + reg := &httpmock.Registry{} + defer reg.Verify(t) + + reg.Register( + httpmock.REST("POST", "gists"), + func(req *http.Request) (*http.Response, error) { + resp, err := httpmock.StatusJSONResponse(404, map[string]string{"message": "Not Found"})(req) + if err != nil { + return nil, err + } + resp.Header.Set("X-Oauth-Scopes", "repo") + resp.Header.Set("X-Accepted-Oauth-Scopes", "") + return resp, nil + }, + ) + + _, err := createGist(&http.Client{Transport: reg}, "github.com", "", true, map[string]*shared.GistFile{ + "file.txt": {Filename: "file.txt", Content: "hello"}, + }) + require.Error(t, err) + + var httpErr api.HTTPError + require.ErrorAs(t, err, &httpErr) + assert.Contains(t, httpErr.ScopesSuggestion(), "gist") +} diff --git a/pkg/cmd/gist/edit/edit.go b/pkg/cmd/gist/edit/edit.go index 8195c6aa0f9..36ea2b0e301 100644 --- a/pkg/cmd/gist/edit/edit.go +++ b/pkg/cmd/gist/edit/edit.go @@ -288,7 +288,7 @@ func editRun(opts *EditOptions) error { file := gist.Files[filename] if file.Truncated { if _, alreadyEdited := filesToUpdate[filename]; !alreadyEdited { - fullContent, err := shared.GetRawGistFile(client, safeurl.NewImmutableSafeURL(file.RawURL)) + fullContent, err := shared.GetRawGistFile(client, host, safeurl.NewImmutableSafeURL(file.RawURL)) if err != nil { return err } diff --git a/pkg/cmd/gist/shared/shared.go b/pkg/cmd/gist/shared/shared.go index 7c0a7c07565..ee51a902294 100644 --- a/pkg/cmd/gist/shared/shared.go +++ b/pkg/cmd/gist/shared/shared.go @@ -255,13 +255,13 @@ func PromptGists(prompter prompter.Prompter, client *http.Client, host string, c // GetRawGistFile fetches the full content of a gist file from its raw URL. The // bytes are external content, so they are returned as iostreams.Untrusted to // force callers to choose between sanitized display and raw round-tripping. -func GetRawGistFile(httpClient *http.Client, rawURL safeurl.SafeURL) (iostreams.Untrusted, error) { - req, err := http.NewRequest("GET", rawURL.String(), nil) - if err != nil { - return iostreams.Untrusted{}, err - } - - resp, err := httpClient.Do(req) +// The rawURL is supplied by the API and points at gist.githubusercontent.com rather than the +// API host, so it is requested as an absolute URL. The hostname only configures the client. +func GetRawGistFile(httpClient *http.Client, hostname string, rawURL safeurl.SafeURL) (iostreams.Untrusted, error) { + // TODO(api-client-rollout) + // This line of code is part of a mechanical roll out of the api client. + // As a follow up, consider whether the api client can be injected to this call site, rather than constructed + resp, err := api.NewClientFromHTTP(httpClient).Request(hostname, http.MethodGet, rawURL.String(), nil) if err != nil { return iostreams.Untrusted{}, err } diff --git a/pkg/cmd/gist/shared/shared_test.go b/pkg/cmd/gist/shared/shared_test.go index 8b891d8b9fe..1f24a9011e5 100644 --- a/pkg/cmd/gist/shared/shared_test.go +++ b/pkg/cmd/gist/shared/shared_test.go @@ -299,7 +299,7 @@ func TestGetRawGistFile(t *testing.T) { ) client := &http.Client{Transport: reg} - result, err := GetRawGistFile(client, safeurl.NewImmutableSafeURL("https://gist.githubusercontent.com/raw-url")) + result, err := GetRawGistFile(client, "github.com", safeurl.NewImmutableSafeURL("https://gist.githubusercontent.com/raw-url")) if tt.wantErr { assert.Error(t, err) diff --git a/pkg/cmd/gist/view/view.go b/pkg/cmd/gist/view/view.go index 51ec38d0443..36822e45985 100644 --- a/pkg/cmd/gist/view/view.go +++ b/pkg/cmd/gist/view/view.go @@ -150,7 +150,7 @@ func viewRun(opts *ViewOptions) error { // path fetches the full content from the raw URL. content := iostreams.NewUntrusted(gf.Content) if gf.Truncated { - fullContent, err := shared.GetRawGistFile(client, safeurl.NewImmutableSafeURL(gf.RawURL)) + fullContent, err := shared.GetRawGistFile(client, hostname, safeurl.NewImmutableSafeURL(gf.RawURL)) if err != nil { return err } diff --git a/pkg/cmd/pr/diff/diff.go b/pkg/cmd/pr/diff/diff.go index 3e627b8b806..0a08b6cad53 100644 --- a/pkg/cmd/pr/diff/diff.go +++ b/pkg/cmd/pr/diff/diff.go @@ -15,7 +15,6 @@ import ( "github.com/MakeNowJust/heredoc" "github.com/cli/cli/v2/api" "github.com/cli/cli/v2/internal/browser" - "github.com/cli/cli/v2/internal/ghinstance" "github.com/cli/cli/v2/internal/ghrepo" "github.com/cli/cli/v2/internal/safeurl" "github.com/cli/cli/v2/internal/text" @@ -211,7 +210,7 @@ func diffRun(opts *DiffOptions) error { } func fetchDiff(httpClient *http.Client, baseRepo ghrepo.Interface, prNumber int, asPatch bool) (io.ReadCloser, error) { - url, err := safeurl.JoinPathWithHostPrefix(ghinstance.RESTPrefix(baseRepo.RepoHost()), "repos", baseRepo.RepoOwner(), baseRepo.RepoName(), "pulls", strconv.Itoa(prNumber)) + path, err := safeurl.JoinPath("repos", baseRepo.RepoOwner(), baseRepo.RepoName(), "pulls", strconv.Itoa(prNumber)) if err != nil { return nil, err } @@ -220,18 +219,16 @@ func fetchDiff(httpClient *http.Client, baseRepo ghrepo.Interface, prNumber int, acceptType = "application/vnd.github.v3.patch" } - req, err := http.NewRequest("GET", url.String(), nil) - if err != nil { - return nil, err - } - - req.Header.Set("Accept", acceptType) - - resp, err := httpClient.Do(req) + // TODO(api-client-rollout) + // This line of code is part of a mechanical roll out of the api client. + // As a follow up, consider whether the api client can be injected to this call site, rather than constructed + resp, err := api.NewClientFromHTTP(httpClient).Request(baseRepo.RepoHost(), http.MethodGet, path.String(), nil, + api.WithHeader("Accept", acceptType)) if err != nil { return nil, err } if resp.StatusCode != 200 { + defer resp.Body.Close() return nil, api.HandleHTTPError(resp) } diff --git a/pkg/cmd/release/create/create.go b/pkg/cmd/release/create/create.go index 79d92bdf153..56e69a4158a 100644 --- a/pkg/cmd/release/create/create.go +++ b/pkg/cmd/release/create/create.go @@ -548,7 +548,7 @@ func createRun(opts *CreateOptions) error { } opts.IO.StartProgressIndicator() - err = shared.ConcurrentUpload(httpClient, safeurl.NewImmutableSafeURL(uploadURL), opts.Concurrency, opts.Assets) + err = shared.ConcurrentUpload(httpClient, baseRepo.RepoHost(), safeurl.NewImmutableSafeURL(uploadURL), opts.Concurrency, opts.Assets) opts.IO.StopProgressIndicator() if err != nil { return cleanupDraftRelease(err) diff --git a/pkg/cmd/release/create/http.go b/pkg/cmd/release/create/http.go index a2dc4372cda..ff07a9b93e6 100644 --- a/pkg/cmd/release/create/http.go +++ b/pkg/cmd/release/create/http.go @@ -11,7 +11,6 @@ import ( "strings" "github.com/cli/cli/v2/api" - "github.com/cli/cli/v2/internal/ghinstance" "github.com/cli/cli/v2/internal/ghrepo" "github.com/cli/cli/v2/internal/safeurl" "github.com/cli/cli/v2/pkg/cmd/release/shared" @@ -110,32 +109,33 @@ func generateReleaseNotes(httpClient *http.Client, repo ghrepo.Interface, tagNam } func publishedReleaseExists(httpClient *http.Client, repo ghrepo.Interface, tagName string) (bool, error) { - url, err := safeurl.JoinPathWithHostPrefix(ghinstance.RESTPrefix(repo.RepoHost()), "repos", repo.RepoOwner(), repo.RepoName(), "releases", "tags", tagName) - if err != nil { - return false, err - } - req, err := http.NewRequest("HEAD", url.String(), nil) + path, err := safeurl.JoinPath("repos", repo.RepoOwner(), repo.RepoName(), "releases", "tags", tagName) if err != nil { return false, err } + // A HEAD response has no body, so Request is used rather than REST, which would try to + // decode one. // TODO(api-client-rollout) - // This has been deferred from moving to api.Client due to HEAD responses having no body while REST decodes non-204/205 2xx responses as JSON. - resp, err := httpClient.Do(req) + // This line of code is part of a mechanical roll out of the api client. + // As a follow up, consider whether the api client can be injected to this call site, rather than constructed + resp, err := api.NewClientFromHTTP(httpClient).Request(repo.RepoHost(), http.MethodHead, path.String(), nil) if err != nil { + var httpErr api.HTTPError + if errors.As(err, &httpErr) && httpErr.StatusCode == http.StatusNotFound { + return false, nil + } return false, err } if resp.Body != nil { defer resp.Body.Close() } - if resp.StatusCode == 200 { - return true, nil - } else if resp.StatusCode == 404 { - return false, nil - } else { + if resp.StatusCode != http.StatusOK { return false, api.HandleHTTPError(resp) } + + return true, nil } func createRelease(httpClient *http.Client, repo ghrepo.Interface, params map[string]interface{}) (*shared.Release, error) { diff --git a/pkg/cmd/release/delete-asset/delete_asset.go b/pkg/cmd/release/delete-asset/delete_asset.go index 3aedc3e3a46..7528741c152 100644 --- a/pkg/cmd/release/delete-asset/delete_asset.go +++ b/pkg/cmd/release/delete-asset/delete_asset.go @@ -97,7 +97,7 @@ func deleteAssetRun(opts *DeleteAssetOptions) error { return fmt.Errorf("asset %s not found in release %s", opts.AssetName, release.TagName) } - err = deleteAsset(httpClient, safeurl.NewImmutableSafeURL(assetURL)) + err = deleteAsset(httpClient, baseRepo.RepoHost(), safeurl.NewImmutableSafeURL(assetURL)) if err != nil { return err } @@ -112,20 +112,16 @@ func deleteAssetRun(opts *DeleteAssetOptions) error { return nil } -func deleteAsset(httpClient *http.Client, assetURL safeurl.SafeURL) error { - req, err := http.NewRequest("DELETE", assetURL.String(), nil) - if err != nil { - return err - } - - resp, err := httpClient.Do(req) +func deleteAsset(httpClient *http.Client, hostname string, assetURL safeurl.SafeURL) error { + // The asset URL is supplied by the API, so it is absolute and requested as given. + // TODO(api-client-rollout) + // This line of code is part of a mechanical roll out of the api client. + // As a follow up, consider whether the api client can be injected to this call site, rather than constructed + resp, err := api.NewClientFromHTTP(httpClient).Request(hostname, http.MethodDelete, assetURL.String(), nil) if err != nil { return err } defer resp.Body.Close() - if resp.StatusCode > 299 { - return api.HandleHTTPError(resp) - } return nil } diff --git a/pkg/cmd/release/delete/delete.go b/pkg/cmd/release/delete/delete.go index 108ebae7ece..264d1d677bc 100644 --- a/pkg/cmd/release/delete/delete.go +++ b/pkg/cmd/release/delete/delete.go @@ -7,7 +7,6 @@ import ( "github.com/cli/cli/v2/api" "github.com/cli/cli/v2/git" - "github.com/cli/cli/v2/internal/ghinstance" "github.com/cli/cli/v2/internal/ghrepo" "github.com/cli/cli/v2/internal/safeurl" "github.com/cli/cli/v2/pkg/cmd/release/shared" @@ -93,7 +92,7 @@ func deleteRun(opts *DeleteOptions) error { } } - err = deleteRelease(httpClient, safeurl.NewImmutableSafeURL(release.APIURL)) + err = deleteRelease(httpClient, baseRepo.RepoHost(), safeurl.NewImmutableSafeURL(release.APIURL)) if err != nil { return err } @@ -122,42 +121,34 @@ func deleteRun(opts *DeleteOptions) error { return nil } -func deleteRelease(httpClient *http.Client, releaseURL safeurl.SafeURL) error { - req, err := http.NewRequest("DELETE", releaseURL.String(), nil) - if err != nil { - return err - } - - resp, err := httpClient.Do(req) +func deleteRelease(httpClient *http.Client, hostname string, releaseURL safeurl.SafeURL) error { + // The release URL is supplied by the API, so it is absolute and requested as given. + // TODO(api-client-rollout) + // This line of code is part of a mechanical roll out of the api client. + // As a follow up, consider whether the api client can be injected to this call site, rather than constructed + resp, err := api.NewClientFromHTTP(httpClient).Request(hostname, http.MethodDelete, releaseURL.String(), nil) if err != nil { return err } defer resp.Body.Close() - if resp.StatusCode > 299 { - return api.HandleHTTPError(resp) - } return nil } func deleteTag(httpClient *http.Client, baseRepo ghrepo.Interface, tagName string) error { - url, err := safeurl.JoinPathWithHostPrefix(ghinstance.RESTPrefix(baseRepo.RepoHost()), "repos", baseRepo.RepoOwner(), baseRepo.RepoName(), "git", "refs", fmt.Sprintf("tags/%s", tagName)) - if err != nil { - return err - } - req, err := http.NewRequest("DELETE", url.String(), nil) + path, err := safeurl.JoinPath("repos", baseRepo.RepoOwner(), baseRepo.RepoName(), "git", "refs", fmt.Sprintf("tags/%s", tagName)) if err != nil { return err } - resp, err := httpClient.Do(req) + // TODO(api-client-rollout) + // This line of code is part of a mechanical roll out of the api client. + // As a follow up, consider whether the api client can be injected to this call site, rather than constructed + resp, err := api.NewClientFromHTTP(httpClient).Request(baseRepo.RepoHost(), http.MethodDelete, path.String(), nil) if err != nil { return err } defer resp.Body.Close() - if resp.StatusCode > 299 { - return api.HandleHTTPError(resp) - } return nil } diff --git a/pkg/cmd/release/download/download.go b/pkg/cmd/release/download/download.go index 688d94c1472..c3d29362f6c 100644 --- a/pkg/cmd/release/download/download.go +++ b/pkg/cmd/release/download/download.go @@ -302,7 +302,7 @@ func downloadAsset(dest *destinationWriter, httpClient *http.Client, assetURL sa return err } - req, err := http.NewRequest("GET", assetURL.String(), nil) + req, err := http.NewRequest(http.MethodGet, assetURL.String(), nil) if err != nil { return err } @@ -312,7 +312,9 @@ func downloadAsset(dest *destinationWriter, httpClient *http.Client, assetURL sa // adding application/json to Accept header due to a bug in the zipball/tarball API endpoint that makes it mandatory req.Header.Set("Accept", "application/octet-stream, application/json") - // override HTTP redirect logic to avoid "legacy" Codeload resources + // override HTTP redirect logic to avoid "legacy" Codeload resources. This is client + // level rather than request level, so the client is copied before being handed over + // rather than mutating the one shared with every other caller. oldClient := *httpClient httpClient = &oldClient httpClient.CheckRedirect = func(req *http.Request, via []*http.Request) error { @@ -323,16 +325,20 @@ func downloadAsset(dest *destinationWriter, httpClient *http.Client, assetURL sa } } - resp, err := httpClient.Do(req) + // The asset URL is supplied by the API, so it is absolute and requested as given. DoRequest + // rather than Request because the CheckRedirect above is a property of the client, and + // Request delegates to go-gh, which builds a client of its own from the transport alone and + // so silently drops it. Without it the "legacy" Codeload path is never rewritten and the + // archive is saved under the wrong name. + // TODO(api-client-rollout) + // This line of code is part of a mechanical roll out of the api client. + // As a follow up, consider whether the api client can be injected to this call site, rather than constructed + resp, err := api.NewClientFromHTTP(httpClient).DoRequest(req) if err != nil { return err } defer resp.Body.Close() - if resp.StatusCode > 299 { - return api.HandleHTTPError(resp) - } - if len(fileName) == 0 { contentDisposition := resp.Header.Get("Content-Disposition") diff --git a/pkg/cmd/release/download/download_test.go b/pkg/cmd/release/download/download_test.go index 855380c3856..cc5ac482b76 100644 --- a/pkg/cmd/release/download/download_test.go +++ b/pkg/cmd/release/download/download_test.go @@ -4,18 +4,22 @@ import ( "bytes" "io" "net/http" + "net/http/httptest" "os" "path/filepath" "runtime" + "strings" "testing" "github.com/cli/cli/v2/internal/ghrepo" + "github.com/cli/cli/v2/internal/safeurl" "github.com/cli/cli/v2/pkg/cmd/release/shared" "github.com/cli/cli/v2/pkg/cmdutil" "github.com/cli/cli/v2/pkg/httpmock" "github.com/cli/cli/v2/pkg/iostreams" "github.com/google/shlex" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func Test_NewCmdDownload(t *testing.T) { @@ -978,3 +982,42 @@ func listFiles(dir string) ([]string, error) { }) return files, err } + +// Test_downloadAsset_archiveAvoidsLegacyCodeload pins that the redirect policy which rewrites +// "legacy" Codeload paths is actually applied to the request. +// +// The policy lives on the http.Client rather than the request, so it only survives if the +// request is sent through the client we hand over. A real server is used because httpmock is a +// RoundTripper and redirects are followed by the client, above the transport, so a stubbed +// transport cannot exercise this at all. +func Test_downloadAsset_archiveAvoidsLegacyCodeload(t *testing.T) { + var requestedPaths []string + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + requestedPaths = append(requestedPaths, r.URL.Path) + switch { + case r.URL.Path == "/asset": + http.Redirect(w, r, "/OWNER/REPO/legacy.zip/refs/tags/v1.2.3", http.StatusFound) + case strings.Contains(r.URL.Path, "legacy."): + t.Errorf("legacy Codeload path was requested: %s", r.URL.Path) + w.WriteHeader(http.StatusNotFound) + default: + w.Header().Set("Content-Disposition", `attachment; filename=REPO-1.2.3.zip`) + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte("archive contents")) + } + })) + defer ts.Close() + + tempDir := t.TempDir() + dest := destinationWriter{dir: tempDir} + + err := downloadAsset(&dest, ts.Client(), safeurl.NewImmutableSafeURL(ts.URL+"/asset"), "", true) + require.NoError(t, err) + + // The file name is derived from the response, so it also proves which resource was served. + body, err := os.ReadFile(filepath.Join(tempDir, "REPO-1.2.3.zip")) + require.NoError(t, err) + assert.Equal(t, "archive contents", string(body)) + + assert.Equal(t, []string{"/asset", "/OWNER/REPO/zip/refs/tags/v1.2.3"}, requestedPaths) +} diff --git a/pkg/cmd/release/edit/http.go b/pkg/cmd/release/edit/http.go index 291123ad3ea..55f3100fbad 100644 --- a/pkg/cmd/release/edit/http.go +++ b/pkg/cmd/release/edit/http.go @@ -9,7 +9,6 @@ import ( "strconv" "github.com/cli/cli/v2/api" - "github.com/cli/cli/v2/internal/ghinstance" "github.com/cli/cli/v2/internal/ghrepo" "github.com/cli/cli/v2/internal/safeurl" "github.com/cli/cli/v2/pkg/cmd/release/shared" @@ -22,28 +21,23 @@ func editRelease(httpClient *http.Client, repo ghrepo.Interface, releaseID int64 return nil, err } - url, err := safeurl.JoinPathWithHostPrefix(ghinstance.RESTPrefix(repo.RepoHost()), "repos", repo.RepoOwner(), repo.RepoName(), "releases", strconv.FormatInt(releaseID, 10)) - if err != nil { - return nil, err - } - req, err := http.NewRequest("PATCH", url.String(), bytes.NewBuffer(bodyBytes)) + path, err := safeurl.JoinPath("repos", repo.RepoOwner(), repo.RepoName(), "releases", strconv.FormatInt(releaseID, 10)) if err != nil { return nil, err } - req.Header.Set("Content-Type", "application/json; charset=utf-8") - - resp, err := httpClient.Do(req) + // Request rather than REST because REST returns without decoding on a 204, which would + // yield a zero valued release instead of the error this site reports today. + // TODO(api-client-rollout) + // This line of code is part of a mechanical roll out of the api client. + // As a follow up, consider whether the api client can be injected to this call site, rather than constructed + resp, err := api.NewClientFromHTTP(httpClient).Request(repo.RepoHost(), http.MethodPatch, path.String(), bytes.NewBuffer(bodyBytes), + api.WithHeader("Content-Type", "application/json; charset=utf-8")) if err != nil { return nil, err } defer resp.Body.Close() - success := resp.StatusCode >= 200 && resp.StatusCode < 300 - if !success { - return nil, api.HandleHTTPError(resp) - } - b, err := io.ReadAll(resp.Body) if err != nil { return nil, err diff --git a/pkg/cmd/release/shared/fetch.go b/pkg/cmd/release/shared/fetch.go index 74bf06657a1..bfc5a9fb667 100644 --- a/pkg/cmd/release/shared/fetch.go +++ b/pkg/cmd/release/shared/fetch.go @@ -5,7 +5,6 @@ import ( "encoding/json" "errors" "fmt" - "io" "net/http" "reflect" "strconv" @@ -14,7 +13,6 @@ import ( "time" "github.com/cli/cli/v2/api" - "github.com/cli/cli/v2/internal/ghinstance" "github.com/cli/cli/v2/internal/ghrepo" "github.com/cli/cli/v2/internal/safeurl" "github.com/cli/cli/v2/pkg/httpmock" @@ -138,30 +136,23 @@ type fetchResult struct { } func FetchRefSHA(ctx context.Context, httpClient *http.Client, repo ghrepo.Interface, tagName string) (string, error) { - url, err := safeurl.JoinPathWithHostPrefix(ghinstance.RESTPrefix(repo.RepoHost()), "repos", repo.RepoOwner(), repo.RepoName(), "git", "ref", fmt.Sprintf("tags/%s", tagName)) - if err != nil { - return "", err - } - req, err := http.NewRequestWithContext(ctx, "GET", url.String(), nil) + path, err := safeurl.JoinPath("repos", repo.RepoOwner(), repo.RepoName(), "git", "ref", fmt.Sprintf("tags/%s", tagName)) if err != nil { return "", err } - resp, err := httpClient.Do(req) + // TODO(api-client-rollout) + // This line of code is part of a mechanical roll out of the api client. + // As a follow up, consider whether the api client can be injected to this call site, rather than constructed + resp, err := api.NewClientFromHTTP(httpClient).RequestWithContext(ctx, repo.RepoHost(), http.MethodGet, path.String(), nil) if err != nil { + if isNotFound(err) { + return "", ErrReleaseNotFound + } return "", err } defer resp.Body.Close() - if resp.StatusCode == http.StatusNotFound { - _, _ = io.Copy(io.Discard, resp.Body) - return "", ErrReleaseNotFound - } - - if resp.StatusCode > 299 { - return "", api.HandleHTTPError(resp) - } - var ref struct { Object struct { SHA string `json:"sha"` @@ -190,7 +181,7 @@ func DigestAlgForRef(digest string) string { // FetchRelease finds a published repository release by its tagName, or a draft release by its pending tag name. func FetchRelease(ctx context.Context, httpClient *http.Client, repo ghrepo.Interface, tagName string) (*Release, error) { - publishedURL, err := safeurl.JoinPathWithHostPrefix(ghinstance.RESTPrefix(repo.RepoHost()), "repos", repo.RepoOwner(), repo.RepoName(), "releases", "tags", tagName) + publishedPath, err := safeurl.JoinPath("repos", repo.RepoOwner(), repo.RepoName(), "releases", "tags", tagName) if err != nil { return nil, err } @@ -200,7 +191,7 @@ func FetchRelease(ctx context.Context, httpClient *http.Client, repo ghrepo.Inte // published release lookup go func() { - release, err := fetchReleasePath(cc, httpClient, publishedURL) + release, err := fetchReleasePath(cc, httpClient, repo.RepoHost(), publishedPath) results <- fetchResult{release: release, error: err} }() @@ -235,11 +226,11 @@ func FetchRelease(ctx context.Context, httpClient *http.Client, repo ghrepo.Inte // FetchLatestRelease finds the latest published release for a repository. func FetchLatestRelease(ctx context.Context, httpClient *http.Client, repo ghrepo.Interface) (*Release, error) { - url, err := safeurl.JoinPathWithHostPrefix(ghinstance.RESTPrefix(repo.RepoHost()), "repos", repo.RepoOwner(), repo.RepoName(), "releases", "latest") + path, err := safeurl.JoinPath("repos", repo.RepoOwner(), repo.RepoName(), "releases", "latest") if err != nil { return nil, err } - return fetchReleasePath(ctx, httpClient, url) + return fetchReleasePath(ctx, httpClient, repo.RepoHost(), path) } // fetchDraftRelease returns the first draft release that has tagName as its pending tag. @@ -271,32 +262,26 @@ func fetchDraftRelease(ctx context.Context, httpClient *http.Client, repo ghrepo // Then, use REST to get information about the draft release. In theory, we could have fetched // all the necessary information via GraphQL, but REST is safer for backwards compatibility. - path, err := safeurl.JoinPathWithHostPrefix(ghinstance.RESTPrefix(repo.RepoHost()), "repos", repo.RepoOwner(), repo.RepoName(), "releases", strconv.FormatInt(query.Repository.Release.DatabaseID, 10)) + path, err := safeurl.JoinPath("repos", repo.RepoOwner(), repo.RepoName(), "releases", strconv.FormatInt(query.Repository.Release.DatabaseID, 10)) if err != nil { return nil, err } - return fetchReleasePath(ctx, httpClient, path) + return fetchReleasePath(ctx, httpClient, repo.RepoHost(), path) } -func fetchReleasePath(ctx context.Context, httpClient *http.Client, url safeurl.SafeURL) (*Release, error) { - req, err := http.NewRequestWithContext(ctx, "GET", url.String(), nil) - if err != nil { - return nil, err - } - - resp, err := httpClient.Do(req) +func fetchReleasePath(ctx context.Context, httpClient *http.Client, host string, path safeurl.SafeURL) (*Release, error) { + // TODO(api-client-rollout) + // This line of code is part of a mechanical roll out of the api client. + // As a follow up, consider whether the api client can be injected to this call site, rather than constructed + resp, err := api.NewClientFromHTTP(httpClient).RequestWithContext(ctx, host, http.MethodGet, path.String(), nil) if err != nil { + if isNotFound(err) { + return nil, ErrReleaseNotFound + } return nil, err } defer resp.Body.Close() - if resp.StatusCode == http.StatusNotFound { - _, _ = io.Copy(io.Discard, resp.Body) - return nil, ErrReleaseNotFound - } else if resp.StatusCode > 299 { - return nil, api.HandleHTTPError(resp) - } - var release Release if err := json.NewDecoder(resp.Body).Decode(&release); err != nil { return nil, err @@ -305,6 +290,14 @@ func fetchReleasePath(ctx context.Context, httpClient *http.Client, url safeurl. return &release, nil } +// isNotFound reports whether err is an API error carrying a 404 status. The release lookups +// treat a missing release as a sentinel rather than an error, and the api client reports the +// status through an HTTPError rather than through a response the call site can inspect. +func isNotFound(err error) bool { + var httpErr api.HTTPError + return errors.As(err, &httpErr) && httpErr.StatusCode == http.StatusNotFound +} + func StubFetchRelease(t *testing.T, reg *httpmock.Registry, owner, repoName, tagName, responseBody string) { path := "repos/OWNER/REPO/releases/tags/v1.2.3" if tagName == "" { diff --git a/pkg/cmd/release/shared/fetch_test.go b/pkg/cmd/release/shared/fetch_test.go index e68278f1ed1..97e0968ea34 100644 --- a/pkg/cmd/release/shared/fetch_test.go +++ b/pkg/cmd/release/shared/fetch_test.go @@ -93,6 +93,94 @@ func TestFetchRefSHA(t *testing.T) { } } +func TestFetchLatestRelease(t *testing.T) { + tests := []struct { + name string + responseStatus int + responseBody string + expectedTag string + expectedErr error + errorMessage string + }{ + { + name: "found (200)", + responseStatus: 200, + responseBody: `{"tag_name": "v1.2.3"}`, + expectedTag: "v1.2.3", + }, + { + name: "not found (404)", + responseStatus: 404, + expectedErr: ErrReleaseNotFound, + }, + { + name: "server error (500)", + responseStatus: 500, + errorMessage: "HTTP 500", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + fakeHTTP := &httpmock.Registry{} + defer fakeHTTP.Verify(t) + + repo, err := ghrepo.FromFullName("owner/repo") + require.NoError(t, err) + + if tt.responseStatus > 299 { + fakeHTTP.Register( + httpmock.REST("GET", "repos/owner/repo/releases/latest"), + httpmock.JSONErrorResponse(tt.responseStatus, api.HTTPError{ + StatusCode: tt.responseStatus, + Message: "some error", + }), + ) + } else { + fakeHTTP.Register( + httpmock.REST("GET", "repos/owner/repo/releases/latest"), + httpmock.StatusStringResponse(tt.responseStatus, tt.responseBody), + ) + } + + release, err := FetchLatestRelease(context.Background(), &http.Client{Transport: fakeHTTP}, repo) + + switch { + case tt.expectedErr != nil: + require.ErrorIs(t, err, tt.expectedErr) + case tt.errorMessage != "": + require.Error(t, err) + assert.Contains(t, err.Error(), tt.errorMessage) + default: + require.NoError(t, err) + assert.Equal(t, tt.expectedTag, release.TagName) + } + }) + } +} + +// TestFetchReleaseNotFoundWhenBothLookupsMiss pins the sentinel that FetchRelease reports when +// neither the published nor the draft lookup finds a release, since the published lookup now +// recognises a miss from an HTTPError rather than from the response status directly. +func TestFetchReleaseNotFoundWhenBothLookupsMiss(t *testing.T) { + fakeHTTP := &httpmock.Registry{} + + repo, err := ghrepo.FromFullName("owner/repo") + require.NoError(t, err) + + fakeHTTP.Register( + httpmock.REST("GET", "repos/owner/repo/releases/tags/v9.9.9"), + httpmock.JSONErrorResponse(404, api.HTTPError{StatusCode: 404, Message: "Not Found"}), + ) + fakeHTTP.Register( + httpmock.GraphQL(`query RepositoryReleaseByTag\b`), + httpmock.StringResponse(`{"data": {"repository": {"release": null}}}`), + ) + + _, err = FetchRelease(context.Background(), &http.Client{Transport: fakeHTTP}, repo, "v9.9.9") + require.ErrorIs(t, err, ErrReleaseNotFound) +} + func TestDigestAlgForRef(t *testing.T) { tests := []struct { name string diff --git a/pkg/cmd/release/shared/upload.go b/pkg/cmd/release/shared/upload.go index 9307c8c01c2..1f2f0076580 100644 --- a/pkg/cmd/release/shared/upload.go +++ b/pkg/cmd/release/shared/upload.go @@ -20,10 +20,6 @@ import ( "golang.org/x/sync/errgroup" ) -type httpDoer interface { - Do(*http.Request) (*http.Response, error) -} - type errNetwork struct{ error } type AssetForUpload struct { @@ -112,7 +108,7 @@ func fileExt(fn string) string { return path.Ext(fn) } -func ConcurrentUpload(httpClient httpDoer, uploadURL safeurl.SafeURL, numWorkers int, assets []*AssetForUpload) error { +func ConcurrentUpload(httpClient *http.Client, hostname string, uploadURL safeurl.SafeURL, numWorkers int, assets []*AssetForUpload) error { if numWorkers == 0 { return errors.New("the number of concurrent workers needs to be greater than 0") } @@ -124,7 +120,7 @@ func ConcurrentUpload(httpClient httpDoer, uploadURL safeurl.SafeURL, numWorkers for _, a := range assets { asset := *a g.Go(func() error { - return uploadWithDelete(gctx, httpClient, uploadURL, asset) + return uploadWithDelete(gctx, httpClient, hostname, uploadURL, asset) }) } @@ -143,9 +139,9 @@ func shouldRetry(err error) bool { // Allow injecting backoff interval in tests. var retryInterval = time.Millisecond * 200 -func uploadWithDelete(ctx context.Context, httpClient httpDoer, uploadURL safeurl.SafeURL, a AssetForUpload) error { +func uploadWithDelete(ctx context.Context, httpClient *http.Client, hostname string, uploadURL safeurl.SafeURL, a AssetForUpload) error { if a.ExistingURL != nil && a.ExistingURL.String() != "" { - if err := deleteAsset(ctx, httpClient, a.ExistingURL); err != nil { + if err := deleteAsset(ctx, httpClient, hostname, a.ExistingURL); err != nil { return err } } @@ -159,7 +155,7 @@ func uploadWithDelete(ctx context.Context, httpClient httpDoer, uploadURL safeur }, backoff.WithContext(backoff.WithMaxRetries(bo, 3), ctx)) } -func uploadAsset(ctx context.Context, httpClient httpDoer, uploadURL safeurl.SafeURL, asset AssetForUpload) (*ReleaseAsset, error) { +func uploadAsset(ctx context.Context, httpClient *http.Client, uploadURL safeurl.SafeURL, asset AssetForUpload) (*ReleaseAsset, error) { u, err := url.Parse(uploadURL.String()) if err != nil { return nil, err @@ -186,17 +182,24 @@ func uploadAsset(ctx context.Context, httpClient httpDoer, uploadURL safeurl.Saf req.Header.Set("Content-Type", asset.MIMEType) req.GetBody = asset.Open - resp, err := httpClient.Do(req) + // DoRequest rather than Request because ContentLength and GetBody are set on the request + // itself, and neither can be expressed as a header. The upload URL is supplied by the API + // and points at the uploads host, so there is no endpoint resolution to delegate here. + // TODO(api-client-rollout) + // This line of code is part of a mechanical roll out of the api client. + // As a follow up, consider whether the api client can be injected to this call site, rather than constructed + resp, err := api.NewClientFromHTTP(httpClient).DoRequest(req) if err != nil { + // Only transport failures are retryable as network errors. A response that arrived and + // carried a failing status is reported as is, so shouldRetry can judge it by status. + var httpErr api.HTTPError + if errors.As(err, &httpErr) { + return nil, err + } return nil, errNetwork{err} } defer resp.Body.Close() - success := resp.StatusCode >= 200 && resp.StatusCode < 300 - if !success { - return nil, api.HandleHTTPError(resp) - } - var newAsset ReleaseAsset dec := json.NewDecoder(resp.Body) if err := dec.Decode(&newAsset); err != nil { @@ -206,22 +209,16 @@ func uploadAsset(ctx context.Context, httpClient httpDoer, uploadURL safeurl.Saf return &newAsset, nil } -func deleteAsset(ctx context.Context, httpClient httpDoer, assetURL safeurl.SafeURL) error { - req, err := http.NewRequestWithContext(ctx, "DELETE", assetURL.String(), nil) - if err != nil { - return err - } - - resp, err := httpClient.Do(req) +func deleteAsset(ctx context.Context, httpClient *http.Client, hostname string, assetURL safeurl.SafeURL) error { + // The asset URL is supplied by the API, so it is absolute and requested as given. + // TODO(api-client-rollout) + // This line of code is part of a mechanical roll out of the api client. + // As a follow up, consider whether the api client can be injected to this call site, rather than constructed + resp, err := api.NewClientFromHTTP(httpClient).RequestWithContext(ctx, hostname, http.MethodDelete, assetURL.String(), nil) if err != nil { return err } defer resp.Body.Close() - success := resp.StatusCode >= 200 && resp.StatusCode < 300 - if !success { - return api.HandleHTTPError(resp) - } - return nil } diff --git a/pkg/cmd/release/shared/upload_test.go b/pkg/cmd/release/shared/upload_test.go index 8271fa6ae6e..9038abb21f9 100644 --- a/pkg/cmd/release/shared/upload_test.go +++ b/pkg/cmd/release/shared/upload_test.go @@ -82,7 +82,7 @@ func Test_uploadWithDelete_retry(t *testing.T) { ctx := context.Background() tries := 0 - client := funcClient(func(req *http.Request) (*http.Response, error) { + client := &http.Client{Transport: funcTripper(func(req *http.Request) (*http.Response, error) { tries++ if tries == 1 { return nil, errors.New("made up exception") @@ -98,8 +98,8 @@ func Test_uploadWithDelete_retry(t *testing.T) { StatusCode: 200, Body: io.NopCloser(bytes.NewBufferString(`{}`)), }, nil - }) - err := uploadWithDelete(ctx, client, safeurl.NewImmutableSafeURL("http://example.com/upload"), AssetForUpload{ + })} + err := uploadWithDelete(ctx, client, "github.com", safeurl.NewImmutableSafeURL("http://example.com/upload"), AssetForUpload{ Name: "asset", Label: "", Size: 8, @@ -116,8 +116,10 @@ func Test_uploadWithDelete_retry(t *testing.T) { } } -type funcClient func(*http.Request) (*http.Response, error) +// funcTripper drives uploadWithDelete from a plain function. It is a RoundTripper rather than +// a Do-er because the upload path takes a concrete *http.Client, which is what api.Client needs. +type funcTripper func(*http.Request) (*http.Response, error) -func (f funcClient) Do(req *http.Request) (*http.Response, error) { +func (f funcTripper) RoundTrip(req *http.Request) (*http.Response, error) { return f(req) } diff --git a/pkg/cmd/release/upload/upload.go b/pkg/cmd/release/upload/upload.go index 35b10860d75..84345adddb2 100644 --- a/pkg/cmd/release/upload/upload.go +++ b/pkg/cmd/release/upload/upload.go @@ -113,7 +113,7 @@ func uploadRun(opts *UploadOptions) error { } opts.IO.StartProgressIndicator() - err = shared.ConcurrentUpload(httpClient, safeurl.NewImmutableSafeURL(uploadURL), opts.Concurrency, opts.Assets) + err = shared.ConcurrentUpload(httpClient, baseRepo.RepoHost(), safeurl.NewImmutableSafeURL(uploadURL), opts.Concurrency, opts.Assets) opts.IO.StopProgressIndicator() if err != nil { return err diff --git a/pkg/cmd/repo/delete/delete_test.go b/pkg/cmd/repo/delete/delete_test.go index 64a8ee30161..55a249d047a 100644 --- a/pkg/cmd/repo/delete/delete_test.go +++ b/pkg/cmd/repo/delete/delete_test.go @@ -194,9 +194,14 @@ func Test_deleteRun(t *testing.T) { errMsg: "SilentError", wantStderr: "X Failed to delete repository: OWNER/REPO has changed name or transferred ownership\n", httpStubs: func(reg *httpmock.Registry) { + // The Location header is what makes this a real redirect. Without it Go declines + // to follow the 307 regardless of the client's CheckRedirect policy, so the stub + // would pass even if deleteRepo stopped suppressing redirects. reg.Register( httpmock.REST("DELETE", "repos/OWNER/REPO"), - httpmock.StatusStringResponse(307, "{}")) + httpmock.WithHeader( + httpmock.StatusStringResponse(307, "{}"), + "Location", "https://api.github.com/repos/OWNER/RENAMED")) }, }, } @@ -230,6 +235,7 @@ func Test_deleteRun(t *testing.T) { if tt.wantErr { assert.Error(t, err) assert.Equal(t, tt.errMsg, err.Error()) + assert.Equal(t, tt.wantStderr, stderr.String()) return } assert.NoError(t, err) diff --git a/pkg/cmd/repo/delete/http.go b/pkg/cmd/repo/delete/http.go index faffa006e3d..60d7a150798 100644 --- a/pkg/cmd/repo/delete/http.go +++ b/pkg/cmd/repo/delete/http.go @@ -21,21 +21,24 @@ func deleteRepo(client *http.Client, repo ghrepo.Interface) error { return err } - request, err := http.NewRequest("DELETE", url.String(), nil) + request, err := http.NewRequest(http.MethodDelete, url.String(), nil) if err != nil { return err } - resp, err := client.Do(request) + // DoRequest rather than Request because the CheckRedirect set above is a property of the + // client, and Request delegates to go-gh, which builds a client of its own from the + // transport alone and so silently drops it. Following the redirect would delete nothing + // and report success. The cost is that this site keeps an absolute URL and so will not + // pick up api_host resolution until go-gh can carry a redirect policy. + // TODO(api-client-rollout) + // This line of code is part of a mechanical roll out of the api client. + // As a follow up, consider whether the api client can be injected to this call site, rather than constructed + resp, err := api.NewClientFromHTTP(client).DoRequest(request, api.WithEndpointScopes("delete_repo")) if err != nil { return err } defer resp.Body.Close() - if resp.StatusCode > 299 { - api.EndpointNeedsScopes(resp, "delete_repo") - return api.HandleHTTPError(resp) - } - return nil } diff --git a/pkg/cmd/repo/delete/http_test.go b/pkg/cmd/repo/delete/http_test.go new file mode 100644 index 00000000000..fde93823c05 --- /dev/null +++ b/pkg/cmd/repo/delete/http_test.go @@ -0,0 +1,81 @@ +package delete + +import ( + "net/http" + "testing" + + "github.com/cli/cli/v2/api" + "github.com/cli/cli/v2/internal/ghrepo" + "github.com/cli/cli/v2/pkg/httpmock" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestDeleteRepo(t *testing.T) { + reg := &httpmock.Registry{} + defer reg.Verify(t) + + reg.Register( + httpmock.REST("DELETE", "repos/OWNER/REPO"), + httpmock.StatusStringResponse(204, ""), + ) + + require.NoError(t, deleteRepo(&http.Client{Transport: reg}, ghrepo.New("OWNER", "REPO"))) +} + +// TestDeleteRepoMissingScope pins the scopes suggestion, which is the reason this call site +// needs the delete_repo scope declared rather than relying on what the API reports. +func TestDeleteRepoMissingScope(t *testing.T) { + reg := &httpmock.Registry{} + defer reg.Verify(t) + + reg.Register( + httpmock.REST("DELETE", "repos/OWNER/REPO"), + func(req *http.Request) (*http.Response, error) { + resp, err := httpmock.StatusJSONResponse(403, map[string]string{"message": "Must have admin rights to Repository."})(req) + if err != nil { + return nil, err + } + resp.Header.Set("X-Oauth-Scopes", "repo") + resp.Header.Set("X-Accepted-Oauth-Scopes", "") + return resp, nil + }, + ) + + err := deleteRepo(&http.Client{Transport: reg}, ghrepo.New("OWNER", "REPO")) + require.Error(t, err) + + var httpErr api.HTTPError + require.ErrorAs(t, err, &httpErr) + assert.Contains(t, httpErr.ScopesSuggestion(), "delete_repo") +} + +// TestDeleteRepoDoesNotFollowRedirect pins that a renamed or transferred repo surfaces the 3xx +// to the caller rather than being followed. deleteRun relies on seeing that status to explain +// what happened, so following the redirect would report success while deleting nothing. +// +// The Location header matters: without it Go cannot follow a redirect at all, so a stub that +// omits it passes whatever the redirect policy is. +func TestDeleteRepoDoesNotFollowRedirect(t *testing.T) { + reg := &httpmock.Registry{} + defer reg.Verify(t) + + reg.Register( + httpmock.REST("DELETE", "repos/OWNER/REPO"), + func(req *http.Request) (*http.Response, error) { + resp, err := httpmock.StatusStringResponse(301, "")(req) + if err != nil { + return nil, err + } + resp.Header.Set("Location", "https://api.github.com/repos/OWNER/RENAMED") + return resp, nil + }, + ) + + err := deleteRepo(&http.Client{Transport: reg}, ghrepo.New("OWNER", "REPO")) + require.Error(t, err) + + var httpErr api.HTTPError + require.ErrorAs(t, err, &httpErr) + assert.Equal(t, 301, httpErr.StatusCode) +} diff --git a/pkg/cmd/repo/edit/edit.go b/pkg/cmd/repo/edit/edit.go index c215f182ccc..ac9a443e6fd 100644 --- a/pkg/cmd/repo/edit/edit.go +++ b/pkg/cmd/repo/edit/edit.go @@ -14,7 +14,6 @@ import ( "github.com/MakeNowJust/heredoc" "github.com/cli/cli/v2/api" fd "github.com/cli/cli/v2/internal/featuredetection" - "github.com/cli/cli/v2/internal/ghinstance" "github.com/cli/cli/v2/internal/ghrepo" "github.com/cli/cli/v2/internal/safeurl" "github.com/cli/cli/v2/internal/text" @@ -567,18 +566,17 @@ func parseTopics(s string) []string { } func getTopics(ctx context.Context, httpClient *http.Client, repo ghrepo.Interface) ([]string, error) { - url, err := safeurl.JoinPathWithHostPrefix(ghinstance.RESTPrefix(repo.RepoHost()), "repos", repo.RepoOwner(), repo.RepoName(), "topics") - if err != nil { - return nil, err - } - req, err := http.NewRequestWithContext(ctx, "GET", url.String(), nil) + path, err := safeurl.JoinPath("repos", repo.RepoOwner(), repo.RepoName(), "topics") if err != nil { return nil, err } // "mercy-preview" is still needed for some GitHub Enterprise versions - req.Header.Set("Accept", "application/vnd.github.mercy-preview+json") - res, err := httpClient.Do(req) + // TODO(api-client-rollout) + // This line of code is part of a mechanical roll out of the api client. + // As a follow up, consider whether the api client can be injected to this call site, rather than constructed + res, err := api.NewClientFromHTTP(httpClient).RequestWithContext(ctx, repo.RepoHost(), http.MethodGet, path.String(), nil, + api.WithHeader("Accept", "application/vnd.github.mercy-preview+json")) if err != nil { return nil, err } @@ -608,19 +606,18 @@ func setTopics(ctx context.Context, httpClient *http.Client, repo ghrepo.Interfa return err } - url, err := safeurl.JoinPathWithHostPrefix(ghinstance.RESTPrefix(repo.RepoHost()), "repos", repo.RepoOwner(), repo.RepoName(), "topics") - if err != nil { - return err - } - req, err := http.NewRequestWithContext(ctx, "PUT", url.String(), body) + path, err := safeurl.JoinPath("repos", repo.RepoOwner(), repo.RepoName(), "topics") if err != nil { return err } - req.Header.Set("Content-type", "application/json") // "mercy-preview" is still needed for some GitHub Enterprise versions - req.Header.Set("Accept", "application/vnd.github.mercy-preview+json") - res, err := httpClient.Do(req) + // TODO(api-client-rollout) + // This line of code is part of a mechanical roll out of the api client. + // As a follow up, consider whether the api client can be injected to this call site, rather than constructed + res, err := api.NewClientFromHTTP(httpClient).RequestWithContext(ctx, repo.RepoHost(), http.MethodPut, path.String(), body, + api.WithHeader("Content-Type", "application/json"), + api.WithHeader("Accept", "application/vnd.github.mercy-preview+json")) if err != nil { return err } diff --git a/pkg/cmd/repo/garden/http.go b/pkg/cmd/repo/garden/http.go index 903de787632..9aeeb72c740 100644 --- a/pkg/cmd/repo/garden/http.go +++ b/pkg/cmd/repo/garden/http.go @@ -2,14 +2,13 @@ package garden import ( "encoding/json" - "errors" "io" "net/http" "strconv" "strings" "time" - "github.com/cli/cli/v2/internal/ghinstance" + "github.com/cli/cli/v2/api" "github.com/cli/cli/v2/internal/ghrepo" "github.com/cli/cli/v2/internal/safeurl" ) @@ -27,7 +26,7 @@ func getCommits(client *http.Client, repo ghrepo.Interface, maxCommits int) ([]* commits := []*Commit{} pathF := func(page int) (*safeurl.MutableSafeURL, error) { - u, err := safeurl.JoinPathWithHostPrefix(ghinstance.RESTPrefix(repo.RepoHost()), "repos", repo.RepoOwner(), repo.RepoName(), "commits") + u, err := safeurl.JoinPath("repos", repo.RepoOwner(), repo.RepoName(), "commits") if err != nil { return nil, err } @@ -47,7 +46,7 @@ func getCommits(client *http.Client, repo ghrepo.Interface, maxCommits int) ([]* if err != nil { return nil, err } - links, err := getResponse(client, path, &result) + links, err := getResponse(client, repo.RepoHost(), path, &result) if err != nil { return nil, err } @@ -80,24 +79,18 @@ func getCommits(client *http.Client, repo ghrepo.Interface, maxCommits int) ([]* // getResponse performs the API call and returns the response's link header values. // If the "Link" header is missing, the returned slice will be nil. -func getResponse(client *http.Client, url safeurl.SafeURL, data interface{}) ([]string, error) { - req, err := http.NewRequest("GET", url.String(), nil) - if err != nil { - return nil, err - } - - req.Header.Set("Content-Type", "application/json; charset=utf-8") - resp, err := client.Do(req) +func getResponse(client *http.Client, hostname string, path safeurl.SafeURL, data interface{}) ([]string, error) { + // The Content-Type this site used to set explicitly is the one the client already sends, + // so it is no longer stated here. + // TODO(api-client-rollout) + // This line of code is part of a mechanical roll out of the api client. + // As a follow up, consider whether the api client can be injected to this call site, rather than constructed + resp, err := api.NewClientFromHTTP(client).Request(hostname, http.MethodGet, path.String(), nil) if err != nil { return nil, err } defer resp.Body.Close() - success := resp.StatusCode >= 200 && resp.StatusCode < 300 - if !success { - return nil, errors.New("api call failed") - } - links := resp.Header["Link"] if resp.StatusCode == http.StatusNoContent { diff --git a/pkg/cmd/repo/garden/http_test.go b/pkg/cmd/repo/garden/http_test.go new file mode 100644 index 00000000000..44b4e42d5cf --- /dev/null +++ b/pkg/cmd/repo/garden/http_test.go @@ -0,0 +1,55 @@ +package garden + +import ( + "net/http" + "testing" + + "github.com/cli/cli/v2/api" + "github.com/cli/cli/v2/internal/ghrepo" + "github.com/cli/cli/v2/pkg/httpmock" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestGetCommits(t *testing.T) { + reg := &httpmock.Registry{} + defer reg.Verify(t) + + reg.Register( + httpmock.REST("GET", "repos/OWNER/REPO/commits"), + httpmock.JSONResponse([]map[string]interface{}{ + {"sha": "abc123def456", "author": map[string]string{"login": "monalisa"}}, + {"sha": "def456abc123", "author": map[string]string{"login": ""}}, + }), + ) + + commits, err := getCommits(&http.Client{Transport: reg}, ghrepo.New("OWNER", "REPO"), 10) + require.NoError(t, err) + + require.Len(t, commits, 2) + // commits are reversed so that older ones come first + assert.Equal(t, "def456abc123", commits[0].Sha) + assert.Equal(t, "a mysterious stranger", commits[0].Handle) + assert.Equal(t, "abc123def456", commits[1].Sha) + assert.Equal(t, "monalisa", commits[1].Handle) +} + +// TestGetCommitsHTTPError pins the error returned on a non-2xx response. This site used to +// return a bare "api call failed", and now surfaces the API's own message via api.HTTPError. +func TestGetCommitsHTTPError(t *testing.T) { + reg := &httpmock.Registry{} + defer reg.Verify(t) + + reg.Register( + httpmock.REST("GET", "repos/OWNER/REPO/commits"), + httpmock.StatusJSONResponse(404, map[string]string{"message": "Not Found"}), + ) + + _, err := getCommits(&http.Client{Transport: reg}, ghrepo.New("OWNER", "REPO"), 10) + require.Error(t, err) + + var httpErr api.HTTPError + require.ErrorAs(t, err, &httpErr) + assert.Equal(t, 404, httpErr.StatusCode) + assert.Equal(t, "Not Found", httpErr.Message) +} diff --git a/pkg/cmd/repo/read-file/http.go b/pkg/cmd/repo/read-file/http.go index 20c1ed0e9f1..5f4b8eb23b0 100644 --- a/pkg/cmd/repo/read-file/http.go +++ b/pkg/cmd/repo/read-file/http.go @@ -9,7 +9,6 @@ import ( "strings" "github.com/cli/cli/v2/api" - "github.com/cli/cli/v2/internal/ghinstance" "github.com/cli/cli/v2/internal/ghrepo" "github.com/cli/cli/v2/internal/safeurl" ) @@ -88,25 +87,19 @@ func fetchContent(httpClient *http.Client, repo ghrepo.Interface, filePath, ref return nil, err } - req, err := http.NewRequest("GET", apiPath.String(), nil) - if err != nil { - return nil, err - } // We use the "application/vnd.github.object+json" media type to request a unified object // representation from the Contents API. Without this, the API returns a JSON array for // directories and a JSON object for files. - req.Header.Set("Accept", "application/vnd.github.object+json") - - resp, err := httpClient.Do(req) + // TODO(api-client-rollout) + // This line of code is part of a mechanical roll out of the api client. + // As a follow up, consider whether the api client can be injected to this call site, rather than constructed + resp, err := api.NewClientFromHTTP(httpClient).Request(repo.RepoHost(), http.MethodGet, apiPath.String(), nil, + api.WithHeader("Accept", "application/vnd.github.object+json")) if err != nil { return nil, err } defer resp.Body.Close() - if resp.StatusCode > 299 { - return nil, api.HandleHTTPError(resp) - } - body, err := io.ReadAll(resp.Body) if err != nil { return nil, err @@ -179,30 +172,25 @@ func fetchRawFile(httpClient *http.Client, repo ghrepo.Interface, filePath, ref return nil, err } - req, err := http.NewRequest("GET", apiPath.String(), nil) - if err != nil { - return nil, err - } - req.Header.Set("Accept", "application/vnd.github.raw") - - resp, err := httpClient.Do(req) + // TODO(api-client-rollout) + // This line of code is part of a mechanical roll out of the api client. + // As a follow up, consider whether the api client can be injected to this call site, rather than constructed + resp, err := api.NewClientFromHTTP(httpClient).Request(repo.RepoHost(), http.MethodGet, apiPath.String(), nil, + api.WithHeader("Accept", "application/vnd.github.raw")) if err != nil { return nil, err } defer resp.Body.Close() - if resp.StatusCode > 299 { - return nil, api.HandleHTTPError(resp) - } - return io.ReadAll(resp.Body) } -// contentsAPIPath builds the absolute Contents API URL for a path and optional ref. +// contentsAPIPath builds the Contents API path for a path and optional ref, relative to the +// host's REST endpoint. func contentsAPIPath(repo ghrepo.Interface, filePath, ref string) (safeurl.SafeURL, error) { // The Contents API accepts a fully percent-encoded path, including path separators // encoded as %2F, so spaces and other special characters are handled transparently. - u, err := safeurl.JoinPathWithHostPrefix(ghinstance.RESTPrefix(repo.RepoHost()), "repos", repo.RepoOwner(), repo.RepoName(), "contents", strings.TrimPrefix(filePath, "/")) + u, err := safeurl.JoinPath("repos", repo.RepoOwner(), repo.RepoName(), "contents", strings.TrimPrefix(filePath, "/")) if err != nil { return nil, err } diff --git a/pkg/cmd/repo/read-file/read_file_test.go b/pkg/cmd/repo/read-file/read_file_test.go index 59d3856e40b..422cb00c425 100644 --- a/pkg/cmd/repo/read-file/read_file_test.go +++ b/pkg/cmd/repo/read-file/read_file_test.go @@ -724,28 +724,28 @@ func Test_contentsAPIPath(t *testing.T) { { name: "simple path", filePath: "README.md", - want: "https://api.github.com/repos/OWNER/REPO/contents/README.md", + want: "repos/OWNER/REPO/contents/README.md", }, { name: "leading slash trimmed", filePath: "/README.md", - want: "https://api.github.com/repos/OWNER/REPO/contents/README.md", + want: "repos/OWNER/REPO/contents/README.md", }, { name: "nested path encodes separators", filePath: "pkg/cmd/create.go", - want: "https://api.github.com/repos/OWNER/REPO/contents/pkg%2Fcmd%2Fcreate.go", + want: "repos/OWNER/REPO/contents/pkg%2Fcmd%2Fcreate.go", }, { name: "spaces are encoded", filePath: "dir with spaces/file.txt", - want: "https://api.github.com/repos/OWNER/REPO/contents/dir%20with%20spaces%2Ffile.txt", + want: "repos/OWNER/REPO/contents/dir%20with%20spaces%2Ffile.txt", }, { name: "ref is appended", filePath: "README.md", ref: "feature/branch", - want: "https://api.github.com/repos/OWNER/REPO/contents/README.md?ref=feature%2Fbranch", + want: "repos/OWNER/REPO/contents/README.md?ref=feature%2Fbranch", }, } diff --git a/pkg/cmd/run/download/http.go b/pkg/cmd/run/download/http.go index a832f924b20..ad15984c67c 100644 --- a/pkg/cmd/run/download/http.go +++ b/pkg/cmd/run/download/http.go @@ -25,27 +25,24 @@ func (p *apiPlatform) List(runID string) ([]shared.Artifact, error) { } func (p *apiPlatform) Download(url safeurl.SafeURL, dir safepaths.Absolute) error { - return downloadArtifact(p.client, url, dir) + return downloadArtifact(p.client, p.repo.RepoHost(), url, dir) } -func downloadArtifact(httpClient *http.Client, url safeurl.SafeURL, destDir safepaths.Absolute) error { - req, err := http.NewRequest("GET", url.String(), nil) - if err != nil { - return err - } +func downloadArtifact(httpClient *http.Client, hostname string, url safeurl.SafeURL, destDir safepaths.Absolute) error { // The server rejects this :( - //req.Header.Set("Accept", "application/zip") - - resp, err := httpClient.Do(req) + //api.WithHeader("Accept", "application/zip") + // + // The artifact download URL is supplied by the API, so it is absolute and is requested as + // given rather than being resolved against the host's REST endpoint. + // TODO(api-client-rollout) + // This line of code is part of a mechanical roll out of the api client. + // As a follow up, consider whether the api client can be injected to this call site, rather than constructed + resp, err := api.NewClientFromHTTP(httpClient).Request(hostname, http.MethodGet, url.String(), nil) if err != nil { return err } defer resp.Body.Close() - if resp.StatusCode > 299 { - return api.HandleHTTPError(resp) - } - tmpfile, err := os.CreateTemp("", "gh-artifact.*.zip") if err != nil { return fmt.Errorf("error initializing temporary file: %w", err) diff --git a/pkg/cmd/run/download/http_test.go b/pkg/cmd/run/download/http_test.go index 5b68dff82d4..33874388621 100644 --- a/pkg/cmd/run/download/http_test.go +++ b/pkg/cmd/run/download/http_test.go @@ -72,6 +72,7 @@ func Test_Download(t *testing.T) { api := &apiPlatform{ client: &http.Client{Transport: reg}, + repo: ghrepo.New("OWNER", "REPO"), } require.NoError(t, api.Download(safeurl.NewImmutableSafeURL("https://api.github.com/repos/OWNER/REPO/actions/artifacts/12345/zip"), destDir)) diff --git a/pkg/cmd/run/shared/artifacts.go b/pkg/cmd/run/shared/artifacts.go index 36d0b39e73c..064b62ee8b1 100644 --- a/pkg/cmd/run/shared/artifacts.go +++ b/pkg/cmd/run/shared/artifacts.go @@ -1,13 +1,10 @@ package shared import ( - "encoding/json" "net/http" - "regexp" "strconv" "github.com/cli/cli/v2/api" - "github.com/cli/cli/v2/internal/ghinstance" "github.com/cli/cli/v2/internal/ghrepo" "github.com/cli/cli/v2/internal/safeurl" ) @@ -26,14 +23,13 @@ type artifactsPayload struct { func ListArtifacts(httpClient *http.Client, repo ghrepo.Interface, runID string) ([]Artifact, error) { var results []Artifact - restPrefix := ghinstance.RESTPrefix(repo.RepoHost()) perPage := 100 - u, err := safeurl.JoinPathWithHostPrefix(restPrefix, "repos", repo.RepoOwner(), repo.RepoName(), "actions", "artifacts") + u, err := safeurl.JoinPath("repos", repo.RepoOwner(), repo.RepoName(), "actions", "artifacts") if err != nil { return nil, err } if runID != "" { - u, err = safeurl.JoinPathWithHostPrefix(restPrefix, "repos", repo.RepoOwner(), repo.RepoName(), "actions", "runs", runID, "artifacts") + u, err = safeurl.JoinPath("repos", repo.RepoOwner(), repo.RepoName(), "actions", "runs", runID, "artifacts") if err != nil { return nil, err } @@ -41,9 +37,14 @@ func ListArtifacts(httpClient *http.Client, repo ghrepo.Interface, runID string) u.SetQuery("per_page", strconv.Itoa(perPage)) var pageURL safeurl.SafeURL = u + // TODO(api-client-rollout) + // This line of code is part of a mechanical roll out of the api client. + // As a follow up, consider whether the api client can be injected to this call site, rather than constructed + client := api.NewClientFromHTTP(httpClient) + for { var payload artifactsPayload - nextURL, err := apiGet(httpClient, pageURL, &payload) + nextURL, err := client.RESTWithNext(repo.RepoHost(), http.MethodGet, pageURL.String(), nil, &payload) if err != nil { return nil, err } @@ -57,38 +58,3 @@ func ListArtifacts(httpClient *http.Client, repo ghrepo.Interface, runID string) return results, nil } - -func apiGet(httpClient *http.Client, url safeurl.SafeURL, data interface{}) (string, error) { - req, err := http.NewRequest("GET", url.String(), nil) - if err != nil { - return "", err - } - - resp, err := httpClient.Do(req) - if err != nil { - return "", err - } - defer resp.Body.Close() - - if resp.StatusCode > 299 { - return "", api.HandleHTTPError(resp) - } - - dec := json.NewDecoder(resp.Body) - if err := dec.Decode(data); err != nil { - return "", err - } - - return findNextPage(resp), nil -} - -var linkRE = regexp.MustCompile(`<([^>]+)>;\s*rel="([^"]+)"`) - -func findNextPage(resp *http.Response) string { - for _, m := range linkRE.FindAllStringSubmatch(resp.Header.Get("Link"), -1) { - if len(m) > 2 && m[2] == "next" { - return m[1] - } - } - return "" -} diff --git a/pkg/cmd/run/view/logs.go b/pkg/cmd/run/view/logs.go index ab1232837bb..113dc871526 100644 --- a/pkg/cmd/run/view/logs.go +++ b/pkg/cmd/run/view/logs.go @@ -14,7 +14,6 @@ import ( "unicode/utf16" "github.com/cli/cli/v2/api" - "github.com/cli/cli/v2/internal/ghinstance" "github.com/cli/cli/v2/internal/ghrepo" "github.com/cli/cli/v2/internal/safeurl" "github.com/cli/cli/v2/pkg/cmd/run/shared" @@ -40,24 +39,26 @@ type apiLogFetcher struct { } func (f *apiLogFetcher) GetLog() (io.ReadCloser, error) { - logURL, err := safeurl.JoinPathWithHostPrefix(ghinstance.RESTPrefix(f.repo.RepoHost()), "repos", f.repo.RepoOwner(), f.repo.RepoName(), "actions", "jobs", strconv.FormatInt(f.jobID, 10), "logs") + logPath, err := safeurl.JoinPath("repos", f.repo.RepoOwner(), f.repo.RepoName(), "actions", "jobs", strconv.FormatInt(f.jobID, 10), "logs") if err != nil { return nil, err } - req, err := http.NewRequest("GET", logURL.String(), nil) - if err != nil { - return nil, err - } - - resp, err := f.httpClient.Do(req) + // The response body is the log stream, which is handed to the caller rather than decoded. + // TODO(api-client-rollout) + // This line of code is part of a mechanical roll out of the api client. + // As a follow up, consider whether the api client can be injected to this call site, rather than constructed + resp, err := api.NewClientFromHTTP(f.httpClient).Request(f.repo.RepoHost(), http.MethodGet, logPath.String(), nil) if err != nil { + var httpErr api.HTTPError + if errors.As(err, &httpErr) && httpErr.StatusCode == http.StatusNotFound { + return nil, fmt.Errorf("log not found: %v", f.jobID) + } return nil, err } - if resp.StatusCode == 404 { - return nil, fmt.Errorf("log not found: %v", f.jobID) - } else if resp.StatusCode != 200 { + if resp.StatusCode != http.StatusOK { + defer resp.Body.Close() return nil, api.HandleHTTPError(resp) } diff --git a/pkg/cmd/run/view/view.go b/pkg/cmd/run/view/view.go index 95cdc891291..f913ce86015 100644 --- a/pkg/cmd/run/view/view.go +++ b/pkg/cmd/run/view/view.go @@ -16,7 +16,6 @@ import ( "github.com/MakeNowJust/heredoc" "github.com/cli/cli/v2/api" "github.com/cli/cli/v2/internal/browser" - "github.com/cli/cli/v2/internal/ghinstance" "github.com/cli/cli/v2/internal/ghrepo" "github.com/cli/cli/v2/internal/safeurl" "github.com/cli/cli/v2/internal/text" @@ -470,20 +469,22 @@ func shouldFetchJobs(opts *ViewOptions) bool { return false } -func getLog(httpClient *http.Client, logURL safeurl.SafeURL) (io.ReadCloser, error) { - req, err := http.NewRequest("GET", logURL.String(), nil) - if err != nil { - return nil, err - } - - resp, err := httpClient.Do(req) +func getLog(httpClient *http.Client, hostname string, logPath safeurl.SafeURL) (io.ReadCloser, error) { + // The response body is the log archive, which is handed to the caller rather than decoded. + // TODO(api-client-rollout) + // This line of code is part of a mechanical roll out of the api client. + // As a follow up, consider whether the api client can be injected to this call site, rather than constructed + resp, err := api.NewClientFromHTTP(httpClient).Request(hostname, http.MethodGet, logPath.String(), nil) if err != nil { + var httpErr api.HTTPError + if errors.As(err, &httpErr) && httpErr.StatusCode == http.StatusNotFound { + return nil, errors.New("log not found") + } return nil, err } - if resp.StatusCode == 404 { - return nil, errors.New("log not found") - } else if resp.StatusCode != 200 { + if resp.StatusCode != http.StatusOK { + defer resp.Body.Close() return nil, api.HandleHTTPError(resp) } @@ -499,19 +500,19 @@ func getRunLog(cache RunLogCache, httpClient *http.Client, repo ghrepo.Interface if !isCached { // Run log does not exist in cache so retrieve and store it - logURL, err := safeurl.JoinPathWithHostPrefix(ghinstance.RESTPrefix(repo.RepoHost()), "repos", repo.RepoOwner(), repo.RepoName(), "actions", "runs", strconv.FormatInt(run.ID, 10), "logs") + logPath, err := safeurl.JoinPath("repos", repo.RepoOwner(), repo.RepoName(), "actions", "runs", strconv.FormatInt(run.ID, 10), "logs") if err != nil { return nil, err } if attempt > 0 { - logURL, err = safeurl.JoinPathWithHostPrefix(ghinstance.RESTPrefix(repo.RepoHost()), "repos", repo.RepoOwner(), repo.RepoName(), "actions", "runs", strconv.FormatInt(run.ID, 10), "attempts", strconv.FormatUint(attempt, 10), "logs") + logPath, err = safeurl.JoinPath("repos", repo.RepoOwner(), repo.RepoName(), "actions", "runs", strconv.FormatInt(run.ID, 10), "attempts", strconv.FormatUint(attempt, 10), "logs") if err != nil { return nil, err } } - resp, err := getLog(httpClient, logURL) + resp, err := getLog(httpClient, repo.RepoHost(), logPath) if err != nil { return nil, err } diff --git a/pkg/search/searcher.go b/pkg/search/searcher.go index dd8dd590cac..ab822517a48 100644 --- a/pkg/search/searcher.go +++ b/pkg/search/searcher.go @@ -2,16 +2,16 @@ package search import ( "encoding/json" + "errors" "fmt" - "io" "net/http" "net/url" "regexp" "strconv" "strings" + "github.com/cli/cli/v2/api" fd "github.com/cli/cli/v2/internal/featuredetection" - "github.com/cli/cli/v2/internal/ghinstance" "github.com/cli/cli/v2/internal/safeurl" ) @@ -24,7 +24,6 @@ const ( var linkRE = regexp.MustCompile(`<([^>]+)>;\s*rel="([^"]+)"`) var pageRE = regexp.MustCompile(`(\?|&)page=(\d*)`) -var jsonTypeRE = regexp.MustCompile(`[/+]json($|;)`) //go:generate moq -rm -out searcher_mock.go . Searcher type Searcher interface { @@ -198,7 +197,7 @@ func (s searcher) Issues(query Query) (IssuesResult, error) { // // For more information, see https://docs.github.com/en/rest/search/search?apiVersion=2022-11-28. func (s searcher) search(query Query, result interface{}) (string, error) { - u, err := safeurl.JoinPathWithHostPrefix(ghinstance.RESTPrefix(s.host), "search", string(query.Kind)) + u, err := safeurl.JoinPath("search", string(query.Kind)) if err != nil { return "", err } @@ -236,28 +235,31 @@ func (s searcher) search(query Query, result interface{}) (string, error) { if query.Sort != "" { u.SetQuery(sortKey, query.Sort) } - req, err := http.NewRequest("GET", u.String(), nil) - if err != nil { - return "", err - } - req.Header.Set("Content-Type", "application/json; charset=utf-8") - req.Header.Set("Accept", "application/vnd.github.v3+json") + accept := "application/vnd.github.v3+json" if query.Kind == KindCode { - req.Header.Set("Accept", "application/vnd.github.text-match+json") + accept = "application/vnd.github.text-match+json" } - resp, err := s.client.Do(req) + // The Content-Type this site used to set explicitly is the one the client already sends, + // so it is no longer stated here. + // TODO(api-client-rollout) + // This line of code is part of a mechanical roll out of the api client. + // As a follow up, consider whether the api client can be injected to this call site, rather than constructed + resp, err := api.NewClientFromHTTP(s.client).Request(s.host, http.MethodGet, u.String(), nil, + api.WithHeader("Accept", accept)) if err != nil { + // Search reports query errors in a shape of its own, so the generic API error is + // translated back rather than surfaced directly. + var apiErr api.HTTPError + if errors.As(err, &apiErr) { + return apiErr.Headers.Get("Link"), asSearchError(apiErr) + } return "", err } defer resp.Body.Close() link := resp.Header.Get("Link") - success := resp.StatusCode >= 200 && resp.StatusCode < 300 - if !success { - return link, handleHTTPError(resp) - } decoder := json.NewDecoder(resp.Body) err = decoder.Decode(result) if err != nil { @@ -297,23 +299,27 @@ func (err httpError) Error() string { return fmt.Sprintf("Invalid search query %q.\n%s", query, err.Errors[0].Message) } -func handleHTTPError(resp *http.Response) error { - httpError := httpError{ - RequestURL: resp.Request.URL, - StatusCode: resp.StatusCode, - } - if !jsonTypeRE.MatchString(resp.Header.Get("Content-Type")) { - httpError.Message = resp.Status - return httpError +// asSearchError converts an api.HTTPError into search's own error type, which formats +// invalid queries differently from the rest of the CLI. +func asSearchError(apiErr api.HTTPError) error { + searchErr := httpError{ + Message: apiErr.Message, + RequestURL: apiErr.RequestURL, + StatusCode: apiErr.StatusCode, } - body, err := io.ReadAll(resp.Body) - if err != nil { - return err + // A non-JSON response leaves Message empty, where this package used to report the status line. + if searchErr.Message == "" { + searchErr.Message = fmt.Sprintf("%d %s", apiErr.StatusCode, http.StatusText(apiErr.StatusCode)) } - if err := json.Unmarshal(body, &httpError); err != nil { - return err + for _, item := range apiErr.Errors { + searchErr.Errors = append(searchErr.Errors, httpErrorItem{ + Code: item.Code, + Field: item.Field, + Message: item.Message, + Resource: item.Resource, + }) } - return httpError + return searchErr } // nextPage extracts the next page number from an API response's link header. if