Repository navigation
Auth transports add credentials after cross-origin redirects #4365
Description
Activity
@huynhtrungcsc given that the transport is an implementation detail of go-github do you have an example of a case where a valid GitHub redirect redirects to an untrusted source? From my understanding all cross origin redirects from the GitHub API are "trusted".
@stevehipwell — I don't think "is GitHub's redirect target trusted" is the deciding factor here. These two transports are exported API, not an internal detail: callers build github.NewClient(tp.Client()) and then hand that same client to pre-signed third-party URLs — release assets on S3, download_url on raw.githubusercontent.com, SBOM downloads. Those hosts are trusted today, but the credentials these transports add are long-lived (and BasicAuthTransport also sets X-GitHub-OTP), so I'd rather the guarantee hold by construction than rest on every redirect target staying benign. It's the same reason DownloadReleaseAsset already documents passing an unauthenticated client.
The policy is settled in #4366: #4366 (comment) — credentials go only to the client's configured origins, everything else goes out unauthenticated, and we don't error. One new PR implements that across all three mechanisms and closes this along with #4363, #4364 and #4562.
One thing I want decided deliberately in that PR rather than by accident: what an empty AllowedOrigins means. It must not mean "allow everything" — but if it defaults to the dotcom origins, then a GitHub Enterprise user who constructs these transports sends no credentials at all, silently. I'd like that to be a documented default, not a surprise.
@gmlewis have you evaluated the current behaviour of DotCom, GHEC & GHES as the current logic around redirects appears to be based on behaviour that is certainly no longer present in DotCom? I want to avoid slowing down the client logic for trusted URLs, which I'd categorise as the configured target domain and any returned redirect URLs (which AFAIK for DotCom no longer need the token removing).
@gmlewis have you evaluated the current behaviour of DotCom, GHEC & GHES as the current logic around redirects appears to be based on behaviour that is certainly no longer present in DotCom? I want to avoid slowing down the client logic for trusted URLs, which I'd categorise as the configured target domain and any returned redirect URLs (which AFAIK for DotCom no longer need the token removing).
You're right that DotCom's download targets no longer need the token — and that's what the new behaviour does: redirect hops are never sent credentials. So on DotCom the wire behaviour is unchanged either way.
What I didn't do is encode DotCom's redirect behaviour into the client, which is the part of the old logic you're calling stale. The rule is "credentials go only to the origins the caller configured", with no claim about where GitHub redirects to. "Returned redirect URLs are trusted" is a statement about DotCom today and would go stale the same way.
The cost isn't CPU — it's two string comparisons per round trip, and the common path is unchanged from before. If you have a GHES case where a redirect target genuinely needs the token, that's the gap to fix: WithURLs only names two origins today.
@gmlewis I'm not sure I agree with your statements above but I'd need a bit of time to ingest what you've said and look at the code changes.
I can't say I'm happy that you've pushed through the client changes without giving me the time to review them given that I re-wrote that whole area and have already raised concerns about the approach.
@gmlewis I'm not sure I agree with your statements above but I'd need a bit of time to ingest what you've said and look at the code changes.
I can't say I'm happy that you've pushed through the client changes without giving me the time to review them given that I re-wrote that whole area and have already raised concerns about the approach.
I'm sorry, @stevehipwell, for pushing through without waiting for your feedback.
As maintainer of this repo, I'm getting frequent PRs attempting to solve this issue for individual tiny cases and actually let one such solution completely slip through into the repo that I then had to later back out.
I sincerely believe that this approach now solves the issue across the entire repo and that you will be happy with it.
However, if after trying it out, you are still not happy with the solution, then please do address your concerns in a new issue stating exactly what problems you are having with this new solution and how you recommend those problems be fixed.
@gmlewis the point of this issue was to collect the spec for GitHub URLs in regards to redirects and returned endpoints requiring authentication. Once we had this we would be able to design a consistent implementation that wasn't based on old behaviours and fit the rest of the architecture.
For example release downloads requires an additional client due to a legacy behaviour where the releases were being served out of S3 and the client token caused an error; releases haven't been served from S3 for a long time and if we're saying that the redirects are pre-signed then we should have an internal client that can be used instead of requiring a client as an arg.
The new code has a special cutout for an auth token, but this doesn't address the app use case (anyone caring about this should be using an app). The with token options implementation was just a copy of the previous logic and needs to be updated to be backed in the options struct as a
oauth2.TokenSourcewith a correspondingWithTokenSourceso it'd be possible to model authentication in a consistent way.What I'm saying is that there is still plenty to discuss around this area that may (or may not) have a material impact on the implementation. I don't think rushing this through serves anyone, no matter how urgently they feel that they must have this functionality.
What I'm saying is that there is still plenty to discuss around this area that may (or may not) have a material impact on the implementation. I don't think rushing this through serves anyone, no matter how urgently they feel that they must have this functionality.
OK, thank you, @stevehipwell - reopening for discussion.
Summary
BasicAuthTransportandUnauthenticatedRateLimitedTransportadd credentials inRoundTripfor each outgoing request. When Go'snet/httpclient follows a redirect to a different origin, the redirected request can pass through the same transport and receive those credentials again.Expected behavior
Credentials added by these transports should only be sent to the original origin. Redirected requests that cross origin should continue without these transport-managed credentials.
Reproduction
This can be reproduced with two
httptestservers: one server returns a redirect to a second server, and the second server records whether the redirected request contains the transport-added authorization headers.Proposed fix
PR #4364 avoids adding these credentials on cross-origin redirect requests and includes regression coverage for both affected transports.