fix!: Send credentials only to configured origins

github/copilot.go

@@ -1617,6 +1617,18 @@ 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.
+//
+// The download URL is a value the caller reads out of a report response, which
+// may name any host. No host check belongs here: the client attaches its
+// credentials only to its configured API and upload origins, so a link that
+// points elsewhere is fetched without them, as is any redirect target.
+//
+// This is a GET with no body of the caller's, so only the credential half of the
+// origin rules applies to it; refusing a foreign host would break the ordinary
+// path, since GitHub hands back report links on a host of its own choosing. The
+// residual is that the body returned here is whatever that host served, and the
+// Download*Metrics methods decode it as GitHub's report. A caller that must be
+// sure the report is GitHub's should treat it as untrusted input.
 func (s *CopilotService) fetchMetricsReport(ctx context.Context, url string) (*http.Response, *Response, error) {
 	req, err := http.NewRequestWithContext(ctx, "GET", url, nil)
 	if err != nil {

github/copilot_test.go

@@ -6,10 +6,12 @@
 package github
 
 import (
+	"context"
 	"encoding/json"
 	"fmt"
 	"log"
 	"net/http"
+	"net/http/httptest"
 	"testing"
 
 	"github.com/google/go-cmp/cmp"
@@ -3905,6 +3907,71 @@ func TestCopilotService_DownloadCopilotMetrics(t *testing.T) {
 	}
 }
 
+// downloadFunc adapts a Download*Metrics method whose decoded payload the table
+// below does not inspect into the error-only shape it runs. Each method returns a
+// different type, so the shared signature is what lets one case list them all.
+func downloadFunc[V any](f func(context.Context, string) (V, *Response, error)) func(context.Context, string) error {
+	return func(ctx context.Context, url string) error {
+		_, _, err := f(ctx, url)
+		return err
+	}
+}
+
+// TestCopilotService_DownloadMetrics_ForeignHostGetsNoCredentials covers the
+// download helpers whose URL comes straight out of a report response, and which
+// therefore may name any host: DownloadCopilotMetrics, and the fetchMetricsReport
+// backed Download*Metrics methods. The client attaches its token only to its own
+// configured origins, so a download link naming some other host is fetched
+// unauthenticated. That is a property of the client's credential wrapper rather
+// than of any one of these methods, which is why none of them needs a host check
+// of its own.
+func TestCopilotService_DownloadMetrics_ForeignHostGetsNoCredentials(t *testing.T) {
+	t.Parallel()
+	client, _, _ := setup(t)
+
+	authedClient, err := client.Clone(WithAuthToken("secret-token"))
+	if err != nil {
+		t.Fatalf("Client.Clone returned error: %v", err)
+	}
+
+	tests := []struct {
+		name string
+		// payload is what the foreign host serves. The methods do not all decode
+		// the same shape, so each case carries one its method can parse: a case
+		// then fails on the header and not on a decode error.
+		payload  string
+		download func(ctx context.Context, url string) error
+	}{
+		{"DownloadCopilotMetrics decodes a JSON array", `[]`, downloadFunc(authedClient.Copilot.DownloadCopilotMetrics)},
+		{"DownloadDailyMetrics decodes a JSON object", `{}`, downloadFunc(authedClient.Copilot.DownloadDailyMetrics)},
+		{"DownloadPeriodicMetrics decodes a JSON object", `{}`, downloadFunc(authedClient.Copilot.DownloadPeriodicMetrics)},
+		{"DownloadUserDailyMetrics decodes NDJSON", `{}`, downloadFunc(authedClient.Copilot.DownloadUserDailyMetrics)},
+		{"DownloadUserPeriodicMetrics decodes NDJSON", `{}`, downloadFunc(authedClient.Copilot.DownloadUserPeriodicMetrics)},
+		{"DownloadRepositoryDailyMetrics decodes NDJSON", `{}`, downloadFunc(authedClient.Copilot.DownloadRepositoryDailyMetrics)},
+		{"DownloadUserTeamsDailyMetrics decodes NDJSON", `{}`, downloadFunc(authedClient.Copilot.DownloadUserTeamsDailyMetrics)},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+			auth := make(chan string, 1)
+			foreign := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+				select {
+				case auth <- r.Header.Get("Authorization"):
+				default:
+				}
+				fmt.Fprint(w, tt.payload)
+			}))
+			t.Cleanup(foreign.Close)
+
+			if err := tt.download(t.Context(), foreign.URL+"/path/to/report"); err != nil {
+				t.Fatalf("download returned error: %v", err)
+			}
+			assertRecordedAuthHeader(t, auth, "")
+		})
+	}
+}
+
 func TestCopilotService_DownloadDailyMetrics(t *testing.T) {
 	t.Parallel()
 	client, mux, _ := setup(t)