From de37305c796f26fb6ec68be55dbe2e63102a1dfd Mon Sep 17 00:00:00 2001 From: prasanna8585 Date: Thu, 17 Sep 2026 18:15:37 +0530 Subject: [PATCH] fix(copilot): reject a metrics download URL whose host differs from the client's DownloadCopilotMetrics, DownloadDailyMetrics, DownloadPeriodicMetrics, DownloadUserDailyMetrics, DownloadUserPeriodicMetrics, DownloadRepositoryDailyMetrics, and DownloadUserTeamsDailyMetrics all take a download URL that the caller is documented to read out of a prior Get*MetricsReport response's DownloadLinks field, not one it constructs itself. Both fetchMetricsReport (the shared path for the six current methods) and the deprecated DownloadCopilotMetrics built the outgoing request directly from that URL and sent it through the client's own credentialed HTTP client (fetchMetricsReport via s.client.client.Do, DownloadCopilotMetrics via s.client.BareDo) with no check that the URL's host matched the client's configured one. That client's auth transport attaches the caller's Authorization header to every request it sends, regardless of destination host -- confirmed by reading github.go's WithAuthToken, which installs a RoundTripper that unconditionally sets the header before delegating to the underlying transport. A report response naming a foreign host (from a compromised or malicious GitHub Enterprise Server instance, or any position able to influence that response) would have had its bearer token, and the downloaded report body, sent to that host. This is the same vulnerability class this repository already fixed twice elsewhere: bareDoUntilFound's cross-host check on 301 redirects ("a cross-host target would leak credentials"), and UploadReleaseAssetFromRelease's host check on the release object's UploadURL field (also server-provided, also merged this same week). Neither mitigation covers this code path, since both fetchMetricsReport and DownloadCopilotMetrics build and send their request independently of bareDoUntilFound and of UploadReleaseAssetFromRelease's check. Fix: parse the download URL and compare its host against the client's configured baseURL.Host before building the request, in both fetchMetricsReport and DownloadCopilotMetrics, following the same pattern (url.Parse + strings.EqualFold + a descriptive error) already established by the UploadURL fix. Verified: - gofmt -l reports no issues on either changed file. - The full module requires go >= 1.26.0, which this sandbox's network policy could not download (proxy.golang.org is not reachable), so a full `go build`/`go test` of the module itself was not possible here. In its place: (1) extracted the exact fix logic (URL parsing, host comparison, error construction) into a minimal, dependency-free Go program and ran it standalone, confirming (a) the vulnerability is real -- a simulated transport-level auth header genuinely reaches an attacker-controlled server when no check is present; (b) the fix rejects the foreign-host request before it is sent, with zero header leakage; (c) a legitimate same-host request still succeeds normally. (2) Added real Go tests to copilot_test.go (TestCopilotService_fetchMetricsReport_ForeignHostIsRejected, TestCopilotService_DownloadCopilotMetrics_ForeignHostIsRejected, TestCopilotService_fetchMetricsReport_MalformedDownloadURL) using the same httptest.NewServer + Transport-wrapping pattern this file already uses in TestCopilotService_fetchMetricsReport_closesOriginalBodyOnErrorResponse, confirmed syntactically valid via gofmt, but not run against the toolchain for the reason above -- these should be run in CI as part of review. - Confirmed the fix's host-comparison logic matches how every existing test in this file already constructs download URLs (client.baseURL.String() + path), so no existing test's behavior should change. --- github/copilot.go | 47 +++++++++++++++++++++--- github/copilot_test.go | 81 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 124 insertions(+), 4 deletions(-) diff --git a/github/copilot.go b/github/copilot.go index e53a80e38eb..88109cc5c68 100644 --- a/github/copilot.go +++ b/github/copilot.go @@ -12,6 +12,8 @@ import ( "fmt" "io" "net/http" + "net/url" + "strings" "time" ) @@ -1230,8 +1232,25 @@ func (s *CopilotService) GetOrganizationUserTeamsDailyMetricsReport(ctx context. // (see https://github.com/google/go-github/issues/4136). // This method is retained // for GitHub Enterprise Server installations that may still serve the legacy shape. -func (s *CopilotService) DownloadCopilotMetrics(ctx context.Context, url string) ([]*CopilotMetrics, *Response, error) { - req, err := http.NewRequestWithContext(ctx, "GET", url, nil) +// +// downloadURL is a value the caller reads out of a prior Get*MetricsReport +// response, not one it constructs itself. BareDo's auth transport attaches +// the caller's Authorization header to every request it sends, regardless +// of host, so a report response naming a foreign host must be rejected +// before the request goes out. See fetchMetricsReport for the same guard. +func (s *CopilotService) DownloadCopilotMetrics(ctx context.Context, downloadURL string) ([]*CopilotMetrics, *Response, error) { + parsed, err := url.Parse(downloadURL) + if err != nil { + return nil, nil, err + } + if !strings.EqualFold(parsed.Host, s.client.baseURL.Host) { + return nil, nil, fmt.Errorf( + "download URL host %v does not match the client's configured host %v", + parsed.Host, s.client.baseURL.Host, + ) + } + + req, err := http.NewRequestWithContext(ctx, "GET", downloadURL, nil) if err != nil { return nil, nil, err } @@ -1617,8 +1636,28 @@ type CopilotUserPeriodicMetrics struct { // fetchMetricsReport performs a GET against the provided download URL and returns the raw // http.Response. The caller is responsible for closing the body. -func (s *CopilotService) fetchMetricsReport(ctx context.Context, url string) (*http.Response, *Response, error) { - req, err := http.NewRequestWithContext(ctx, "GET", url, nil) +// +// downloadURL is documented as a value the caller reads out of a prior +// Get*MetricsReport response's DownloadLinks, not one it constructs itself. +// The request below goes out through s.client.client, whose auth transport +// attaches the caller's Authorization header to every request it sends, +// regardless of host. Left unchecked, a report response naming a foreign +// host would receive that header. Compare against the client's configured +// host before the request is built, the same guard applied to UploadURL in +// UploadReleaseAssetFromRelease and to redirects in bareDoUntilFound. +func (s *CopilotService) fetchMetricsReport(ctx context.Context, downloadURL string) (*http.Response, *Response, error) { + parsed, err := url.Parse(downloadURL) + if err != nil { + return nil, nil, err + } + if !strings.EqualFold(parsed.Host, s.client.baseURL.Host) { + return nil, nil, fmt.Errorf( + "download URL host %v does not match the client's configured host %v", + parsed.Host, s.client.baseURL.Host, + ) + } + + req, err := http.NewRequestWithContext(ctx, "GET", downloadURL, nil) if err != nil { return nil, nil, err } diff --git a/github/copilot_test.go b/github/copilot_test.go index 3eb26dda7ff..d44083c0bd7 100644 --- a/github/copilot_test.go +++ b/github/copilot_test.go @@ -10,6 +10,7 @@ import ( "fmt" "log" "net/http" + "net/http/httptest" "testing" "github.com/google/go-cmp/cmp" @@ -4159,6 +4160,86 @@ func TestCopilotService_fetchMetricsReport_closesOriginalBodyOnErrorResponse(t * } } +func TestCopilotService_fetchMetricsReport_ForeignHostIsRejected(t *testing.T) { + t.Parallel() + client, _, _ := setup(t) + + // Simulate the auth transport that attaches the caller's credentials to + // every outgoing request, regardless of host: DownloadDailyMetrics takes + // a download link read out of a prior report response, not one the + // caller constructs, so a report response naming a foreign host must + // not be able to redirect that request - and the Authorization header + // riding on it - away from the client's configured host. + base := client.client.Transport + if base == nil { + base = http.DefaultTransport + } + client.client.Transport = roundTripperFunc(func(req *http.Request) (*http.Response, error) { + req.Header.Set("Authorization", "Bearer super-secret-token") + return base.RoundTrip(req) + }) + + var leakedAuth string + evil := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + leakedAuth = r.Header.Get("Authorization") + fmt.Fprint(w, `[]`) + })) + t.Cleanup(evil.Close) + + ctx := t.Context() + _, _, err := client.Copilot.DownloadDailyMetrics(ctx, evil.URL+"/path/to/daily") + if err == nil { + t.Fatal("Copilot.DownloadDailyMetrics expected an error for a foreign-host download URL, got nil") + } + if leakedAuth != "" { + t.Fatalf("Authorization header %q reached the foreign host; it must never be sent there", leakedAuth) + } +} + +func TestCopilotService_DownloadCopilotMetrics_ForeignHostIsRejected(t *testing.T) { + t.Parallel() + client, _, _ := setup(t) + + base := client.client.Transport + if base == nil { + base = http.DefaultTransport + } + client.client.Transport = roundTripperFunc(func(req *http.Request) (*http.Response, error) { + req.Header.Set("Authorization", "Bearer super-secret-token") + return base.RoundTrip(req) + }) + + var leakedAuth string + evil := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + leakedAuth = r.Header.Get("Authorization") + fmt.Fprint(w, `[]`) + })) + t.Cleanup(evil.Close) + + ctx := t.Context() + _, _, err := client.Copilot.DownloadCopilotMetrics(ctx, evil.URL+"/path/to/download") + if err == nil { + t.Fatal("Copilot.DownloadCopilotMetrics expected an error for a foreign-host download URL, got nil") + } + if leakedAuth != "" { + t.Fatalf("Authorization header %q reached the foreign host; it must never be sent there", leakedAuth) + } +} + +func TestCopilotService_fetchMetricsReport_MalformedDownloadURL(t *testing.T) { + t.Parallel() + client, _, _ := setup(t) + + // net/url rejects ASCII control characters, so a report response naming + // such a URL must surface as an error rather than a panic or a request + // to an unchecked host. + ctx := t.Context() + _, _, err := client.Copilot.DownloadDailyMetrics(ctx, "https://example.com/\x7f/report") + if err == nil { + t.Fatal("Copilot.DownloadDailyMetrics expected an error for an unparsable download URL, got nil") + } +} + func TestCopilotService_DownloadPeriodicMetrics(t *testing.T) { t.Parallel() client, mux, _ := setup(t)