fix: Reject a metrics download URL whose host differs from the client's - #4562
prasanna8585 wants to merge 1 commit into
Conversation
…he 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.
gmlewis
changed the title
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #4562 +/- ##
==========================================
- Coverage 98.57% 98.55% -0.02%
==========================================
Files 197 197
Lines 18299 18315 +16
==========================================
+ Hits 18038 18051 +13
- Misses 261 264 +3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Thanks for the PR. The underlying observation is right — fetchMetricsReport and DownloadCopilotMetrics do send the caller's token to whatever host is named, since they bypass bareDoUntilFound's guard entirely, and it's fair to note that. I don't want to land it as written, for three reasons:
Also worth noting on the tests: url.Parse rejecting \x7f is already covered at copilot_test.go:4114 with "\n" (and failed before your change too), and the leakedAuth assertions can't fire once the function returns before sending. And the "fixed twice this week" framing is off — the redirect check is #4171 from April. My suggestion is to hold this until #4366 settles the policy, then implement it once at the injection point. |
|
See: #4366 (comment) |
|
Thanks for the detailed review, and for catching the redirect-hop gap specifically that's a real miss on my part. I'd traced the RoundTripper injection at github.go:612-615 for the report and still didn't carry it through to what happens after a redirect, which is exactly the case that matters here. Fair catch. Agreed on all three points:
Also noted: the \x7f test does duplicate copilot_test.go:4114, and the "fixed twice this week" framing was wrong #4171 is from April, not this week. Thanks for the correction. Happy to hold this per your suggestion. I'll watch #4366 and come back with a single implementation at the injection point once the origin-set policy is settled, matching whatever bareDoUntilFound/DownloadReleaseAsset end up doing. Closing/holding this PR in the meantime let me know if you'd rather I close it now or leave it open as a reference until #4366 lands. |
Please take a look at #4564 and make sure you are happy with that impementation, as I believe it should solve the problem across the entire client library. |
2 participants
Problem
DownloadCopilotMetricsand the six currentDownload*Metricsmethods (via the sharedfetchMetricsReport) take a download URL that callers are documented to read out of a priorGet*MetricsReportresponse'sDownloadLinksfield, not one they construct themselves. Both built the outgoing request directly from that URL and sent it through the client's own credentialedhttp.Client, with no check that the URL's host matched the client's configured host.That client's auth transport (
WithAuthToken'sRoundTripper) attaches the caller'sAuthorizationheader to every request it sends, regardless of destination host. A report response naming a foreign host — from a compromised/malicious GitHub Enterprise Server instance, or any position able to influence that response — would have had the caller's bearer token, and the downloaded report body, sent to that host.This is the same vulnerability class this repo already fixed twice this week:
bareDoUntilFound's cross-host check on 301 redirects, andUploadReleaseAssetFromRelease's host check on the release object'sUploadURLfield. Neither mitigation covers this path, since both functions here build and send their request independently of those.Fix
Parse the download URL and compare its host against
client.baseURL.Hostbefore building the request, in bothfetchMetricsReportandDownloadCopilotMetrics, following the same pattern already established by theUploadURLfix (url.Parse+strings.EqualFold+ a descriptive error).Testing
gofmt -lclean on both changed files.go.modrequires (go >= 1.26.0, network-restricted), so I verified the exact fix logic (URL parsing, host comparison) in a standalone, dependency-free Go program: confirmed the leak is real pre-fix (a simulated auth header reaches an attacker-controlled server), confirmed the fix blocks it with zero header leakage, and confirmed a legitimate same-host request still succeeds.TestCopilotService_fetchMetricsReport_ForeignHostIsRejected,TestCopilotService_DownloadCopilotMetrics_ForeignHostIsRejected, andTestCopilotService_fetchMetricsReport_MalformedDownloadURLtocopilot_test.go, matching this file's existinghttptest.NewServer+Transport-wrapping pattern. These are syntactically valid (gofmt-checked) but I couldn't run them against the toolchain for the reason above — please run in CI as part of review.client.baseURL.String() + path, so this shouldn't change any existing test's behavior.