diff --git a/github/repos_releases.go b/github/repos_releases.go index 3361a5d79d1..be9d53b3735 100644 --- a/github/repos_releases.go +++ b/github/repos_releases.go @@ -12,6 +12,7 @@ import ( "io" "mime" "net/http" + "net/url" "os" "path/filepath" "strings" @@ -504,6 +505,25 @@ func (s *RepositoriesService) UploadReleaseAssetFromRelease( // so it works with Client.BaseURL path prefixes (e.g. "/api-v3/"). if !strings.HasPrefix(uploadURL, "http://") && !strings.HasPrefix(uploadURL, "https://") { uploadURL = strings.TrimPrefix(uploadURL, "/") + } else { + // This helper is the one upload entry point whose URL comes from a server + // response rather than from the caller, and an absolute URL replaces the + // client's configured upload host entirely. Left unchecked, a response could + // name any host and receive the artifact together with the caller's + // Authorization header. Keep the upload on the host the client was + // configured with; that is uploads.github.com rather than api.github.com in + // the default configuration, which is why the comparison is against + // uploadURL and not baseURL. + u, err := url.Parse(uploadURL) + if err != nil { + return nil, nil, err + } + if !strings.EqualFold(u.Host, s.client.uploadURL.Host) { + return nil, nil, fmt.Errorf( + "upload URL host %q does not match the client's configured upload host %q", + u.Host, s.client.uploadURL.Host, + ) + } } // addOptions will append name/label query params (same behavior as UploadReleaseAsset). diff --git a/github/repos_releases_test.go b/github/repos_releases_test.go index 1c94d043f30..491917fe338 100644 --- a/github/repos_releases_test.go +++ b/github/repos_releases_test.go @@ -10,6 +10,7 @@ import ( "fmt" "io" "net/http" + "net/http/httptest" "strings" "testing" @@ -850,6 +851,53 @@ func TestRepositoriesService_UploadReleaseAssetFromRelease_AbsoluteTemplate(t *t } } +func TestRepositoriesService_UploadReleaseAssetFromRelease_ForeignHostIsRejected(t *testing.T) { + t.Parallel() + client, _, _ := setup(t) + + // A server that hands out an absolute upload URL naming a different host must not + // be able to redirect the upload - and therefore the caller's Authorization header + // and the artifact body - away from the host the client was configured with. + var leaked int + evil := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + leaked++ + fmt.Fprint(w, `{"id":1}`) + })) + t.Cleanup(evil.Close) + + body := []byte("private artifact\n") + reader := bytes.NewReader(body) + size := int64(len(body)) + + release := &RepositoryRelease{UploadURL: evil.URL + "/upload{?name,label}"} + ctx := t.Context() + _, _, err := client.Repositories.UploadReleaseAssetFromRelease( + ctx, release, &UploadOptions{Name: "n.txt"}, reader, size, + ) + if err == nil { + t.Fatal("expected an error for an upload URL naming a foreign host, got nil") + } + if leaked != 0 { + t.Fatalf("upload reached the foreign host %d time(s); the token and body must never be sent there", leaked) + } +} + +func TestRepositoriesService_UploadReleaseAssetFromRelease_MalformedUploadURL(t *testing.T) { + t.Parallel() + client, _, _ := setup(t) + + // net/url rejects ASCII control characters, so a response naming such a URL must + // surface as an error rather than a panic or an upload to an unchecked host. + release := &RepositoryRelease{UploadURL: "https://uploads.github.com/\x7f/upload{?name,label}"} + ctx := t.Context() + _, _, err := client.Repositories.UploadReleaseAssetFromRelease( + ctx, release, &UploadOptions{Name: "n.txt"}, bytes.NewReader([]byte("x")), 1, + ) + if err == nil { + t.Fatal("expected an error for an unparsable upload URL, got nil") + } +} + func TestRepositoriesService_UploadReleaseAssetFromRelease_NilRelease(t *testing.T) { t.Parallel() client, _, _ := setup(t)