From cac306dda0e7074ded7b434ea20316e4711e64c5 Mon Sep 17 00:00:00 2001 From: Sushant Poudel Date: Tue, 15 Sep 2026 14:46:17 +0545 Subject: [PATCH 1/3] Refuse to upload a release asset to a host named by the API response UploadReleaseAssetFromRelease takes release.UploadURL from the server's response, and NewUploadRequest lets an absolute URL replace the client's configured upload host entirely. A response naming a different host was therefore able to receive the artifact together with the caller's Authorization header, while the caller's own WithEnterpriseURLs/WithURLs configuration was silently ignored. Resolve the value against the configured upload origin and reject a differing host, so an upload can only go where the client was told to send it. Relative URLs (the normal case, and what GitHub returns in practice) are unaffected, and the comparison is against uploadURL rather than baseURL because uploads.github.com differs from api.github.com by design. --- github/repos_releases.go | 20 ++++++++++++++++++++ github/repos_releases_test.go | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 52 insertions(+) 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..4292d62d60f 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,37 @@ 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_NilRelease(t *testing.T) { t.Parallel() client, _, _ := setup(t) From e7d857e58b4d0f31506e4625dbc09c485e1073e2 Mon Sep 17 00:00:00 2001 From: Sushant Poudel Date: Tue, 15 Sep 2026 19:41:55 +0545 Subject: [PATCH 2/3] Cover the unparsable upload URL branch Exercises the url.Parse error return in UploadReleaseAssetFromRelease so every line of the new host check is covered. A URL containing an ASCII control character makes net/url refuse the value, and the helper must surface that as an error rather than uploading to an unchecked host. --- github/repos_releases_test.go | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/github/repos_releases_test.go b/github/repos_releases_test.go index 4292d62d60f..491917fe338 100644 --- a/github/repos_releases_test.go +++ b/github/repos_releases_test.go @@ -882,6 +882,22 @@ func TestRepositoriesService_UploadReleaseAssetFromRelease_ForeignHostIsRejected } } +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) From 8590220c394a1ed61f64f734f40daba77db94389 Mon Sep 17 00:00:00 2001 From: Sushant Poudel Date: Wed, 16 Sep 2026 21:15:21 +0545 Subject: [PATCH 3/3] Use %v for the host and count verbs in the upload-host error Applies the wording suggestions from review on #4556: the error message formats both hosts with %v, and the test failure formats the leak counter with %v rather than %d. Verified: gofmt clean, go build ./... ok, and all nine TestRepositoriesService_UploadReleaseAssetFromRelease subtests pass. --- github/repos_releases.go | 2 +- github/repos_releases_test.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/github/repos_releases.go b/github/repos_releases.go index be9d53b3735..8fb4ebea611 100644 --- a/github/repos_releases.go +++ b/github/repos_releases.go @@ -520,7 +520,7 @@ func (s *RepositoriesService) UploadReleaseAssetFromRelease( } 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", + "upload URL host %v does not match the client's configured upload host %v", u.Host, s.client.uploadURL.Host, ) } diff --git a/github/repos_releases_test.go b/github/repos_releases_test.go index 491917fe338..67012cf3245 100644 --- a/github/repos_releases_test.go +++ b/github/repos_releases_test.go @@ -878,7 +878,7 @@ func TestRepositoriesService_UploadReleaseAssetFromRelease_ForeignHostIsRejected 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) + t.Fatalf("upload reached the foreign host %v time(s); the token and body must never be sent there", leaked) } }