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)