From 60a296f1afcac880cd7e397ef374aa76ab03fda2 Mon Sep 17 00:00:00 2001 From: Mike Ball Date: Tue, 17 Dec 2024 05:35:17 -0500 Subject: [PATCH 1/2] fix: DownloadReleaseAsset handles renamed repository This fixes issue #3081, such that `Repositories.DownloadReleaseAsset` correctly handles scenarios where a non-`nil` `followRedirectsClient` argument has been passed, but the specified repository has been renamed to a new name. Previously, in such scenarios... 1. The initial `GET /repos///releases/assets/` responds 301 w/ a `Location: /repositories//releases/assets/`. 2. The `followRedirectsClient` issues a `GET /repositories//releases/assets/` with a `Accept: */*` header. 3. `GET /repositories//releases/assets/` responds 200 with a JSON response body (rather than 302 w/ a `Location: ` header) due to the presence of the `Accept: */*` header (and the absence of a `Accept: application/octet-stream` header)`from step 2. The following `curl` offers an example of correct redirect follows & correct `Accept: application/octet-stream`-preservation in this scenario: ``` curl \ --verbose \ --location \ --header "Accept: application/octet-stream" \ https://api.github.com/repos/mterwill/go-github-issue-demo/releases/assets/151970555 ``` Now, as per this change: 1. The initial `GET /repos///releases/assets/` responds 301 w/ a `Location: /repositories//releases/assets/`. 2. The `followRedirectsClient` issues a `GET /repositories//releases/assets/` with a `Accept: application/octet-stream` header [as required by the GH API](https://docs.github.com/en/enterprise-cloud@latest/rest/releases/assets?apiVersion=2022-11-28) 3. `GET /repositories//releases/assets/` responds 302 w/ a `Location: `. 4. The `followRedirectsClient` follows the redirects to `GET ` w/ a `Accept: application/octet-stream` header, and `GET ` responds 200 w/ the asset contents. Signed-off-by: Mike Ball --- github/repos_releases.go | 2 +- github/repos_releases_test.go | 46 +++++++++++++++++++++++++++++++++-- 2 files changed, 45 insertions(+), 3 deletions(-) diff --git a/github/repos_releases.go b/github/repos_releases.go index 7231db6d9ea..9014e3628af 100644 --- a/github/repos_releases.go +++ b/github/repos_releases.go @@ -387,7 +387,7 @@ func (s *RepositoriesService) downloadReleaseAssetFromURL(ctx context.Context, f return nil, err } req = withContext(ctx, req) - req.Header.Set("Accept", "*/*") + req.Header.Set("Accept", defaultMediaType) resp, err := followRedirectsClient.Do(req) if err != nil { return nil, err diff --git a/github/repos_releases_test.go b/github/repos_releases_test.go index 7c9b9e96bab..011e8cf67bf 100644 --- a/github/repos_releases_test.go +++ b/github/repos_releases_test.go @@ -489,7 +489,7 @@ func TestRepositoriesService_DownloadReleaseAsset_FollowRedirect(t *testing.T) { }) mux.HandleFunc("/yo", func(w http.ResponseWriter, r *http.Request) { testMethod(t, r, "GET") - testHeader(t, r, "Accept", "*/*") + testHeader(t, r, "Accept", defaultMediaType) w.Header().Set("Content-Type", "application/octet-stream") w.Header().Set("Content-Disposition", "attachment; filename=hello-world.txt") fmt.Fprint(w, "Hello World") @@ -511,6 +511,48 @@ func TestRepositoriesService_DownloadReleaseAsset_FollowRedirect(t *testing.T) { } } +func TestRepositoriesService_DownloadReleaseAsset_FollowMultipleRedirects(t *testing.T) { + t.Parallel() + client, mux, _ := setup(t) + + mux.HandleFunc("/repos/o/r/releases/assets/1", func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json; charset=utf-8") + testMethod(t, r, "GET") + testHeader(t, r, "Accept", defaultMediaType) + // /yo, below will be served as baseURLPath/yo + http.Redirect(w, r, baseURLPath+"/yo", http.StatusMovedPermanently) + }) + mux.HandleFunc("/yo", func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "text/html;charset=utf-8") + testMethod(t, r, "GET") + testHeader(t, r, "Accept", defaultMediaType) + // /yo2, below will be served as baseURLPath/yo2 + http.Redirect(w, r, baseURLPath+"/yo2", http.StatusFound) + }) + mux.HandleFunc("/yo2", func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/octet-stream") + w.Header().Set("Content-Disposition", "attachment; filename=hello-world.txt") + testMethod(t, r, "GET") + testHeader(t, r, "Accept", defaultMediaType) + fmt.Fprint(w, "Hello World") + }) + + ctx := context.Background() + reader, _, err := client.Repositories.DownloadReleaseAsset(ctx, "o", "r", 1, http.DefaultClient) + if err != nil { + t.Errorf("Repositories.DownloadReleaseAsset returned error: %v", err) + } + content, err := io.ReadAll(reader) + if err != nil { + t.Errorf("Reading Repositories.DownloadReleaseAsset returned error: %v", err) + } + reader.Close() + want := []byte("Hello World") + if !bytes.Equal(want, content) { + t.Errorf("Repositories.DownloadReleaseAsset returned %+v, want %+v", content, want) + } +} + func TestRepositoriesService_DownloadReleaseAsset_FollowRedirectToError(t *testing.T) { t.Parallel() client, mux, _ := setup(t) @@ -523,7 +565,7 @@ func TestRepositoriesService_DownloadReleaseAsset_FollowRedirectToError(t *testi }) mux.HandleFunc("/yo", func(w http.ResponseWriter, r *http.Request) { testMethod(t, r, "GET") - testHeader(t, r, "Accept", "*/*") + testHeader(t, r, "Accept", defaultMediaType) w.WriteHeader(http.StatusNotFound) }) From 94578ef597b6f661d4034c5fddf61e513697920d Mon Sep 17 00:00:00 2001 From: Mike Ball Date: Tue, 17 Dec 2024 08:56:41 -0500 Subject: [PATCH 2/2] chore: improve DownloadReleaseAsset documentation This improves `RepositoriesService.DownloadReleaseAsset` documentation to be a bit more specific and accurate, as per: https://github.com/google/go-github/issues/3081#issuecomment-1972467566 Signed-off-by: Mike Ball --- github/repos_releases.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/github/repos_releases.go b/github/repos_releases.go index 9014e3628af..6023f632716 100644 --- a/github/repos_releases.go +++ b/github/repos_releases.go @@ -333,9 +333,10 @@ func (s *RepositoriesService) GetReleaseAsset(ctx context.Context, owner, repo s // of the io.ReadCloser. Exactly one of rc and redirectURL will be zero. // // followRedirectsClient can be passed to download the asset from a redirected -// location. Passing http.DefaultClient is recommended unless special circumstances -// exist, but it's possible to pass any http.Client. If nil is passed the -// redirectURL will be returned instead. +// location. Specifying any http.Client is possible, but passing http.DefaultClient +// is recommended, except when the specified repository is private, in which case +// it's necessary to pass an http.Client that performs authenticated requests. +// If nil is passed the redirectURL will be returned instead. // // GitHub API docs: https://docs.github.com/rest/releases/assets#get-a-release-asset //