fix!: Send credentials only to configured origins

[gmlewis]

BREAKING CHANGE: Credentials are sent only to a client's configured API and upload origins — every other destination, including a redirect hop, goes out unauthenticated. A request body aimed outside those origins is refused. BasicAuthTransport and UnauthenticatedRateLimitedTransport gained AllowedOrigins, which GitHub Enterprise users must set.

One predicate, sameOrigin, now decides every destination question, replacing the per-method host checks that had been accumulating (#4556, #4562). Credentials are withheld outside the configured origins and the request still goes out; a body cannot be treated that way, since reporting an upload that succeeded while the bytes went elsewhere is worse, so NewUploadRequest and NewFormRequest refuse it — one gate in each body-carrying constructor, generalizing what #4556 did in one (thanks @sushant-me). Download links are cross-origin by design (raw.githubusercontent.com, pre-signed CDN hosts, Copilot report links), so those keep the credential half only, with the residual documented.

• WithAuthToken — tokens attach only to the configured origins; configure another host with WithURLs.
• NewUploadRequest — ErrUntrustedDestination for an absolute URL outside the configured origins; configure the host with WithURLs or WithEnterpriseURLs.
• NewFormRequest — same gate: an absolute URL outside the configured origins now returns ErrUntrustedDestination. No call site passes one today.
• BasicAuthTransport, UnauthenticatedRateLimitedTransport — new AllowedOrigins; empty means the GitHub.com API and upload origins, not "any origin".
• UploadReleaseAssetFromRelease — still errors on a foreign UploadURL, now via the central gate.
• Client() — no longer warns it is API-only; still safe for other hosts.
• Client.Clone — carries a token over, re-scoped to the clone's origins.

Closes: #4363, Closes: #4364, Closes: #4562, Closes: #4365, Closes: #4366

Replaces #4556 — its per-helper check is now the central upload gate.

cc: @huynhtrungcsc @prasanna8585 @sushant-me

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.59%. Comparing base (25f3541) to head (4452477).

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4564      +/-   ##
==========================================
+ Coverage   98.57%   98.59%   +0.01%     
==========================================
  Files         197      197              
  Lines       18299    18326      +27     
==========================================
+ Hits        18038    18068      +30     
+ Misses        261      258       -3     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[gmlewis]

cc: @stevehipwell

[gmlewis]

cc: @Not-Dhananjay-Mishra

[sushant-me]

Reviewed the design and ran it locally (go1.27.1, PR head 582c158). go test ./github/ -count=1 passes, and I checked the "one predicate" claim rather than assuming it: in non-test code Authorization is set in exactly one place (github.go:661, the wrapper), setCredentialsAsHeaders has only its two call sites and both sit behind isAllowedOrigin, and shouldAuthorizeRequest covers the wrapper plus both redirect guards. I could not find a path that attaches credentials outside the predicate. Rebuilding Clone from baseTransport instead of stacking a second wrapper is the right call, and sameOrigin refusing to match http against https is the half of it that matters most.

On the trade-off you flagged — I think the body does matter, and I would keep a check there. Two things pushed me that way.

First, the premise. "The URL comes from the caller's own release object" is true of the variable, but the value is normally the API's: Repositories.GetRelease returns the release with upload_url taken straight from the response, and that is the object this helper is documented to be used with. So the party who can change that URL is whoever controls the response — a compromised GHE instance, an intercepting enterprise proxy, a MITM under a mis-scoped config — not the caller. That is the same threat model #4366 is written against, and it is why #4556 existed.

Second, the failure is silent, which concerns me more than the bytes. Token configured, baseURL/uploadURL on a trusted server, release whose UploadURL names a foreign host:

err                = <nil>
attacker reached   = true
attacker got token = ""
attacker got body  = "CONFIDENTIAL-BUILD-ARTIFACT"

The token is protected — that part works exactly as described. But the call returns a nil error and a *ReleaseAsset, so the caller is told the upload succeeded. A private artifact has left for a host the client never configured and nothing in the return value says so. Withholding a credential is safe because the request can still legitimately succeed unauthenticated, which is precisely your pre-signed-redirect case. Sending the payload while withholding only the credential is not the same thing: the request cannot succeed as intended against that host, so reporting success turns an attack into a silent data loss.

The reconciliation, if it helps: the two rules are not actually in tension, because they fail differently. For credentials, "send it unauthenticated" is right, since the request may still work. For a body whose destination came from a response, an error is right, since the operation cannot be completed on an origin the caller did not configure — and silently not doing it would be worse than failing. That makes it a body-disclosure check rather than a credential check, so it does not reintroduce the second credential rule you are removing. Renaming and re-commenting what #4556 did, in those terms, would keep one credential policy and still refuse to POST the artifact to an unconfigured origin.

Keeping the policy absolute and accepting the residual is defensible too — but I would put a line in the release notes either way, since a caller reading "credentials go only to configured origins" would not expect the body to still travel.

Happy for #4564 to supersede #4556, and thanks for the co-author credit.

One small thing: TestRepositoriesService_UploadReleaseAssetFromRelease_ForeignHostGetsNoCredentials asserts the foreign host is reached, so it reads as the intended contract rather than as a known gap — a future reader skimming the test name may not see the residual.

[sushant-me]

One follow-up, now that I have read the issues this closes. I think the residual is systemic rather than specific to the upload helper, and that changes the shape of what I suggested above.

fetchMetricsReport (#4562) is the clearest case, because the code change there is comment-only. That is right for the credential half — it goes through s.client.client.Do(req), so the wrapper withholds the token from a foreign host. But the request still goes to whatever host the response named, and whatever comes back is handed to the caller as GitHub's report. #4562 frames the harm that way itself: "would have had the caller's bearer token, and the downloaded report body, sent to that host."

So after this PR, for the response-supplied URLs in this class:

path	token	destination
UploadReleaseAssetFromRelease	withheld	artifact body still POSTed there
fetchMetricsReport (7 methods)	withheld	fetched, and the reply trusted as the report
DownloadContents	withheld	fetched unauthenticated, relying on the pre-signed ?token=

Your policy answers the credential question uniformly and correctly — I have no disagreement with that half. It leaves the destination question open in all three, and the two harms are not the same size: the upload leaks the artifact, the metrics path accepts attacker-supplied content as a report.

So I would restate my earlier suggestion. Rather than "keep a check in the upload helper", the version that fits your own principle — one rule, one place — is: when a URL came from a response and names an origin the client was never configured for, the operation cannot be completed as intended, so fail it. That is a single rule applied in the same place, and it is more uniform than handling each of these paths differently.

Whether that belongs in #4564 or a follow-up is entirely your call. I raise it only because #4562 shows this is not one helper's quirk — it is the general shape of a URL that arrives from a server rather than from the caller.

[gmlewis]

You are right about the upload, @sushant-me; fixed. It's one gate at NewUploadRequest now: an upload outside BaseURL/UploadURL fails with ErrUntrustedUploadDestination before the body is written, so this helper and any added later are both covered.

But not extended to the downloads: their links are cross-origin by design (raw.githubusercontent.com, pre-signed CDN hosts, the report links, etc.), so failing an unconfigured origin breaks the ordinary path, not a dangerous one... and "came from a response" is a call-site property, so it can't live in the same predicate.
For the two that go through the client's own transport (DownloadContentsWithMeta, fetchMetricsReport) the credential is withheld, but the bytes are still whatever host the response named; I've added comments to the code there.

I've reworked the description to match.

PTAL.

[sushant-me]

Verified locally and I think this is right — and better than what I proposed. Thanks for taking the upload half.

The residual I reported is closed, measured the same way I found it. I re-ran my round-1 probe against your branch — a token-configured client whose BaseURL/UploadURL point at a trusted server, and a release whose UploadURL names a foreign host:

	before	after NewUploadRequest gate
err	<nil>	refusing to upload to a destination the client is not configured for: http://…
attacker reached	true	false
attacker got token	""	""
attacker got body	"CONFIDENTIAL-BUILD-ARTIFACT"	""

The host is never contacted, which is stronger than "the body was withheld", and the error carries the destination redacted.

Your "one place" claim holds, and I checked it rather than assuming it — the same way the credential predicate turned out to be single-sourced. NewUploadRequest has exactly two non-test callers (UploadReleaseAsset, UploadReleaseAssetFromRelease). Of the eight http.NewRequest* sites outside the client's own methods, every one is a GET: downloadReleaseAssetFromURL, DownloadContents, DownloadCopilotMetrics, fetchMetricsReport, fetchSBOMFromURL. The only body-carrying POST besides NewFormRequest is NewUploadRequest itself. So no upload path escapes the gate, and a helper added later inherits it — which is the part my per-helper check could never have guaranteed.

You are also right about the downloads, and more precisely than I was. I framed the rule as "fail an operation whose URL came from a response", and you identified the flaw in that: "came from a response" is a call-site property. The transport and the shared predicate cannot see it — fetchMetricsReport receives a string and has no way to know whether the caller read it from DownloadLinks or typed it. My version would have needed the provenance threaded through, and would have broken raw.githubusercontent.com and the pre-signed CDN links in the ordinary case. Withholding the credential and documenting the residual is the right split for a download, and putting the body rule in the one place that builds an upload is the right shape for an upload.

Local check on the branch: gofmt -l clean over every tracked file, go test ./... -count=1 green.

One small thing, not a request: UploadReleaseAssetFromRelease's doc comment still says the helper errors on a foreign UploadURL. That is true, but it now errors via the central gate rather than by checking the host itself, so a reader looking for the check in that function will not find it. A pointer there — "refused by NewUploadRequest; see ErrUntrustedUploadDestination" — would save the next person the same search I just did.

[gmlewis]

Thanks, @sushant-me.

The pointer you want is already in the function, 18 lines below the doc comment: "NewUploadRequest refuses that case with ErrUntrustedUploadDestination … so there is no host check here: every upload this helper builds goes through that one gate." The doc comment names the error but not the mechanism, so I'll add NewUploadRequest to it.

One thing I missed before: NewFormRequest also builds a body-carrying request and wasn't gated. Its only call site passes a relative path, so there's no exposure today — but it's where this class could come back. I've extended the gate to fix it and renamed the error to make it more accurate.

PTAL.

[sushant-me]

Verified on c32fb0b, and you are right on both counts — including one I should own.

The doc pointer. You are correct that the mechanism is described in the function, 18 lines below the doc comment, and I read past it. Naming NewUploadRequest in the doc comment is the right fix; my note was about a reader who stops at the doc comment, which is where I stopped.

NewFormRequest — and this is the part I got wrong. I listed that call site in my own audit. I found it by grepping every http.NewRequest* outside the client's own methods, saw NewFormRequest build a body-carrying request, and classified it as out of scope because its only caller passes a relative path. That classification was the error: the question is not whether today's caller is safe, it is whether the constructor can put the caller's bytes somewhere the caller did not choose — and it can. You judged it in; I judged it out, and your reading is the correct one. Thank you for catching it.

I re-ran the check against both paths, and separately re-ran the round-1 experiment:

upload err = refusing to send a request body to a destination the client is not configured for: http://…/repos/o/r/releases/1/assets?name=a.bin
form   err = refusing to send a request body to a destination the client is not configured for: http://…/x
foreign host reached = false  body=""

Neither constructor contacts the host, and the body never leaves. The rename is complete — 0 remaining references to ErrUntrustedUploadDestination or checkUploadDestination, 17 uses of the new name — gofmt -l clean over every tracked file, and go test ./... -count=1 green.

Two small things, neither a request:

• The new name reads better and is now accurate in both directions, but note the doc on ErrUntrustedDestination is reached from three call sites with three different reasons (upload, form, and the credential rule it contrasts with). If it grows a fourth, a short "which constructors call this" line at the top would save the next reader the grep I just did.
• NewFormRequest's gate is, as you say, a guard and not a fix. Worth keeping that sentence in the comment verbatim — "today's only caller passes a relative path, so this is a guard against the next one" is exactly the kind of note that stops someone deleting it as dead code.

LGTM from me.

[prasanna8585]

Verified this against our original report (#4562) directly in the diff, not just the description.

The fetchMetricsReport/DownloadCopilotMetrics host-check code we wrote is removed entirely here, and correctly so the new doc comment on fetchMetricsReport states "a link that points elsewhere is fetched without them, as is any redirect target," which is exactly the gap our version had (we only validated the caller-supplied URL string, not what a redirect from an on-host URL could point to). Ran the logic through the same standalone repro we used to confirm the original bug: a simulated foreign-host redirect now receives no Authorization header at all, where our patch would have let it through.

TestCopilotService_DownloadMetrics_ForeignHostGetsNoCredentials exercises both affected methods against a real httptest server and asserts the header never arrives that's the right level of coverage for this class of bug (destination-side, not just input-validation-side).

On the download-content-trust residual sushant-me raised: agree with where you landed. Provenance isn't visible at the point a host check would need to run, and GitHub's own report links are legitimately cross-origin by design, so failing them would break the common case to guard the rare one. The comment documenting it as caller-facing untrusted input is the right call.

Happy to see this supersede #4562 it closes what we found, plus the redirect-hop case we missed. LGTM from our side, with the above as the specific things we checked rather than assumed.

[Not-Dhananjay-Mishra]

Few nits, otherwise LGTM 🚀

[gmlewis]

Thank you, @Not-Dhananjay-Mishra!
Merging.