Repository navigation
WithAuthToken authorizes requests outside configured hosts #4366
Description
Activity
A consolidated view of the auth-scoping problem (to help move #4363/#4364 forward)
@huynhtrungcsc — since the discussion asked for #4363/#4364 to be considered together with the related redirect/download-url behaviors rather than piecemeal, I read through the auth code and tried to consolidate the problem here in #4366, in case it's useful for framing a single coherent approach. This is my reading of the code, so corrections are very welcome.
Root cause (one problem, two symptoms)
All three auth mechanisms attach credentials inside
RoundTrip, on every hop, with no origin check:WithAuthTokenwraps the transport and doesreq.Header.Set("Authorization", "Bearer …")unconditionally (theroundTripperFuncinnewClient);BasicAuthTransport.RoundTripandUnauthenticatedRateLimitedTransport.RoundTripcallsetCredentialsAsHeaders(...)unconditionally.
Because the credential is injected per-hop in the RoundTripper (not on the original
*http.Request), it also bypassesnet/http's built-in cross-origin sensitive-header stripping — the client stripsAuthorizationwhen following a cross-domain redirect, but our transport re-adds it on the redirected hop.That single behavior produces both reported symptoms:
- WithAuthToken authorizes requests outside configured hosts #4366 — an absolute-URL request to a host outside the configured origins still gets the token;
- Auth transports add credentials after cross-origin redirects #4365 — a redirect to a different origin (e.g. a CDN/
objects.githubusercontent.com/S3 download target) receives the credentials again.
This is already handled — but only per-endpoint
The download paths already guard against exactly this, manually. For example
bareDoUntilFoundrefuses cross-host redirects with this comment:// Refuse to follow a permanent redirect to a different host: // req.Clone preserves Authorization headers added by the auth // transport, so a cross-host target would leak credentials. if newURL.Host != c.baseURL.Host { ... }
checkRedirectHostdoes the same, andDownloadReleaseAssetfetches the redirected CDN bytes with a caller-suppliedfollowRedirectsClientthat deliberately has no auth transport. So the "don't leak credentials cross-host" rule already exists — it's just duplicated in the download layer instead of enforced centrally. (Those guards compare only againstbaseURL.Host, notuploadURL.Host, which hints the "what counts as an allowed origin" question is itself unsettled.)A possible unified direction
Move origin-scoping into the auth layer: attach credentials only when the request's origin matches the client's configured origin(s); otherwise send the request unauthenticated. Applied to all three transports, this addresses #4365 and #4366 at the root, and the per-endpoint download guards become a natural consequence rather than duplicated logic.
Open questions a single design would need to settle
- Allowed-origin set —
baseURL+uploadURL(they're different hosts on dotcom:api.vsuploads.), plus whateverWithEnterpriseURLsconfigures. Is that the complete set? - Comparison granularity — scheme + host + port with default-port normalization (the
defaultPorthelper from fix!: Scope auth transports to allowed origins #4364), or host-only? Clone()/WithEnterpriseURLscorrectness — the scope needs to track the current client's origins rather than values captured in a closure, otherwiseClonewith new URLs can silently drop auth (theCloneissue reproduced on fix: Scope token auth to configured origins #4363).- Consistency — the same rule across
WithAuthToken,BasicAuthTransport, andUnauthenticatedRateLimitedTransport. Backward compatibility — it's a behavior change for anyone relying on the token reaching a non-configured host, and it should keep the existing download flows working.
Struck — per the clarification below, never letting a token reach a non-configured host is an explicit goal, so backward compatibility for that behavior is a non-goal. The only requirement that survives is that the legitimate download/upload flows keep working, which is a correctness concern rather than a compatibility one.
Hope this is useful in pulling the pieces together — happy to help however's most useful toward getting #4363/#4364 landed as one coherent change.
- Backward compatibility — it's a behavior change for anyone relying on the token reaching a non-configured host, and it should keep the existing download flows working.
@JamBalaya56562 - thank you for the writeup. I think we need one clarification:
We explicitly want to NEVER allow a token to reach a non-configured host. So "Backward compatibility" is a NON-GOAL.
How does this change your analysis?
Reacted by Jam Balaya@JamBalaya56562 - and specifically, it might be easier to maintain if we have a centralized guarding mechanism that covers all the possibilities... but that almost sounds like an internal RoundTripper that is bolted on, but I'm not sure if that is the best option or if that would cause more problems than it solves.
Thanks — struck point 5 above.
How it changes the analysis: mostly by deleting work. The rule becomes deny-by-default — attach only to provably configured origins, everything else goes out unauthenticated — which also settles question 2 in favor of strict comparison: exact scheme + host + port, no suffix or subdomain allowances. No compat shim or deprecation window is needed; it's just a breaking change for the release notes. Question 3 (
Clone()/WithEnterpriseURLs) is then the main remaining risk, though under deny-by-default a stale scope fails as missing auth rather than as a leak.On a centralized guard: I think the unease is justified, because the three mechanisms aren't symmetric.
WithAuthTokenis installed insidenewClient, so it can consultc.baseURL/c.uploadURLatRoundTriptime — a guard works cleanly there. ButBasicAuthTransportandUnauthenticatedRateLimitedTransportare user-constructed (github.NewClient(tp.Client())) and neither struct has a URL field, so they never learn the configured origins. A client-installed guard would also sit outside them and run before they callsetCredentialsAsHeaders, so it can't strip a header that doesn't exist yet; reaching into theirTransportfield would mean mutating a user-owned struct after type-asserting the two known types.So it may need to be a shared policy helper applied at each injection point rather than one bolted-on RoundTripper — and for those two that means an explicit allowed-origins field. Your clarification decides its default: empty should mean the dotcom origins, not "allow everything", with enterprise users setting it explicitly.
OK, based upon this updated analysis, @JamBalaya56562 - how do we address the remaining open PRs and are we on the right track to accomplish these stated goals so that this GitHub v3 API client library cannot be maliciously (or unknowingly) used as an attack vector for stealing authorized GitHub access tokens?
On the open PRs. #4363 is closest to the direction — I'd adapt it rather than restart. #4364's rule is hop-to-hop rather than origin-based (
isCrossOriginRedirectreturns false whenreq.Responseis nil), so a direct absolute-URL request throughBasicAuthTransportstill gets credentials — the same hole as #4366. Scoping those two transports properly means giving them an explicit allowed-origins field, which is new public API and probably belongs in its own PR. huynhtrungcsc offered to adapt or close both once the direction settled, so I'd defer to him on the disposition.On whether this closes the hole. I think so — scoping at the injection point subsumes both symptoms and the per-endpoint download guards, because every hop is checked rather than only the ones we remembered to guard. The remaining risk is completeness rather than approach:
Authorizationisn't the only credential —BasicAuthTransportsetsheaderOTPimmediately after it, so the check has to skip both rather than sit inside the header-setting helper.- Caller-supplied clients (
WithHTTPClient, and thefollowRedirectsClientargument toDownloadReleaseAsset) are outside anything we can enforce — worth stating as the documented boundary of the guarantee. - One table-driven test asserting that no credential-bearing header reaches a non-configured origin, across all three mechanisms, would make "are we done" checkable.
I've left the implementation-level specifics on the PRs.
Reacted by Glenn LewisReacted by Glenn LewisOK — there has been almost no further discussion here, so I'm going to make the call and we'll move forward.
The rule: a configured credential is attached only to the client's own origins — baseURL and uploadURL, compared as scheme + hostname + normalized port (so https://ghe:443 matches https://ghe). Every other destination, every redirect hop included, goes out with no credential at all. Deny by default: we never attach outside those origins, and we don't error either — the request simply goes unauthenticated. One shared predicate, used everywhere.
That's the right layer for it: there are at least five places in the library that build a request from a URL rather than through NewRequest (the two Copilot metrics downloads, DownloadContents, DownloadReleaseAsset's redirect target, fetchSBOMFromURL), and guarding them one at a time is how we ended up with four different checks with four different rules.
How we get there: one new PR implementing the rule for all three credential mechanisms (WithAuthToken, BasicAuthTransport, UnauthenticatedRateLimitedTransport), which:
- closes fix: Scope token auth to configured origins #4363 (client-level WithAuthToken scoping, plus the Clone re-binding)
- closes fix!: Scope auth transports to allowed origins #4364 (explicit allowed-origins on the two caller-constructed transports; new public API, so it's breaking)
- closes fix: Reject a metrics download URL whose host differs from the client's #4562
- reverts the per-method check I merged in fix: Reject an upload URL whose host differs from the configured upload host #4556, whose error-return contradicts the rule above — my apologies, I merged it while this discussion was open, and I'd rather have one policy than two. Reverting it as part of this PR rather than separately, so no release ever has neither guard.
I'm going with a new PR instead of rebasing the two existing ones because both are a month stale and a lot of breaking changes have landed since. I'll carry their fixes and credit @huynhtrungcsc and @sushant-me as co-authors.
Two consequences I want to be deliberate about:
- DownloadContents (dotcom download_url points at raw.githubusercontent.com) is not added to the allowlist — it relies on the pre-signed ?token= in that URL.
- DownloadReleaseAsset currently documents passing an authenticated client for private repos. That guidance is superseded: the redirect target is pre-signed, so it gets an unauthenticated client — the same requirement fetchSBOMFromURL already has.
The existing redirect guards (bareDoUntilFound, checkRedirectHost) stay, but move to the shared predicate so they stop comparing against baseURL alone.
Reacted by Dhananjay Mishra and Jam BalayaHi @gmlewis — no urgency on this, and please take it as the small thing it is.
You mentioned upthread that you'd carry the fixes and credit @huynhtrungcsc and me as co-authors on the replacement. Looking at the merged commit for #4564 (
8d9197f7) I can't find aCo-authored-bytrailer, and nothing in the release notes names me — so I think it slipped, which is completely understandable given how much landed at once.If you'd still like to, adding the trailer to a follow-up commit is enough for me; I'm not asking you to rewrite history or cut a release. If you'd rather not, or it doesn't fit how you attribute work, that's fine too — say so and I'll drop it, and thank you for generalising the fix properly rather than leaving two policies in place.
Either way, thank you for the review on #4556. I learned something from watching you replace three per-method checks with one predicate.
Hi @gmlewis — no urgency on this, and please take it as the small thing it is.
You mentioned upthread that you'd carry the fixes and credit @huynhtrungcsc and me as co-authors on the replacement. Looking at the merged commit for #4564 (
8d9197f7) I can't find aCo-authored-bytrailer, and nothing in the release notes names me — so I think it slipped, which is completely understandable given how much landed at once.If you'd still like to, adding the trailer to a follow-up commit is enough for me; I'm not asking you to rewrite history or cut a release. If you'd rather not, or it doesn't fit how you attribute work, that's fine too — say so and I'll drop it, and thank you for generalising the fix properly rather than leaving two policies in place.
Either way, thank you for the review on #4556. I learned something from watching you replace three per-method checks with one predicate.
Hi @sushant-me - I apologize for my failure to credit you (other than the description).
Since we have a squash-merge policy in this repo, the cleanest way to move forward is for YOU to create a PR that adds your email to the AUTHORS file and also (in the description) credits yourself for the work that you did. I will approve and merge it.Is that OK with you, or would you prefer some other solution?
Hi @sushant-me - I apologize for my failure to credit you (other than the description).
Since we have a squash-merge policy in this repo, the cleanest way to move forward is for YOU to create a PR that adds your email to the AUTHORS file and also (in the description) credits yourself for the work that you did. I will approve and merge it.@huynhtrungcsc - you are welcome to do the same, obviously.
Hi @gmlewis — that is more than fair, and yes, I'll take it. Thank you for offering it rather than leaving it where it was.
Done: #4579 adds the entry to
AUTHORS, in the file's alphabetical position, and its description records what is being credited — that #4556 was merged and then superseded by your broader #4564, which is whatmasterimplements today.For what it's worth, the description credit on #4564 was never nothing to me, and I would have dropped the trailer request if you had said it didn't fit how you attribute this repo's work. Watching you replace three per-method checks with one predicate was the more useful thing I took from the exchange, and a trailer would not have taught me that.
Nothing needed from you beyond the review on #4579 whenever it suits. Thanks for taking it seriously.
Reacted by Glenn Lewis
Summary
WithAuthTokeninstalls a transport wrapper that addsAuthorization: Bearer <token>to outgoing requests. When callers build a request with an absolute URL outside the client's configured API or upload origins, that wrapper still adds the bearer token before sending the request.Expected behavior
The token configured through
WithAuthTokenshould only be attached to requests targeting the client's configured API or upload origins. Requests to other origins should be sent without this transport-managed authorization header.Reproduction
This can be reproduced with
httptestservers by configuring a client with trusted API/upload URLs, then issuing requests using absolute URLs for another server and checking whether that server receives anAuthorizationheader.Proposed fix
PR #4363 limits token injection to the configured API and upload origins, and includes regression coverage for trusted API/upload requests and cross-host absolute URL requests.