fix(http): skip AuthTransport bearer on redirected hops#832
Conversation
AuthTransport.RoundTrip unconditionally re-set the Authorization header, which clobbered stdlib http.Client's cross-host strip on artifact-download redirects to presigned buildkiteartifacts.com / S3 URLs. The presigned URL already carries its own auth via X-Amz-Signature, so the extra header either leaks the bearer or breaks the download with S3's 'Only one auth mechanism allowed' 400. Closes buildkite#830 Signed-off-by: Sai Asish Y <[email protected]>
| token := t.TokenSource.Token() | ||
| if token != "" { | ||
| req.Header.Set("Authorization", fmt.Sprintf("Bearer %s", token)) | ||
| // Skip re-injecting on redirected hops so stdlib's cross-host strip stays in effect. |
There was a problem hiding this comment.
@SAY-5 thanks for the PR! As far as I can tell, this only fixes the normal redirect hop, which is fine (and a nice fix). I'm wondering if you wanted to additionally include a fix for 401 responses, which will still use the RefreshTransport
There was a problem hiding this comment.
Good catch. The 401-retry path is a separate failure mode (refresh + cloned request, where req.Response is nil) so I would rather address it in a follow-up PR with its own test, to keep this one tightly scoped to the redirect leak. I will open that next.
There was a problem hiding this comment.
CI is failing due to an expired token, I think, so I'll look in to fixing that up
|
Kept this PR scoped to the AuthTransport redirect path since that is the leak path #830 reproduces. RefreshTransport only fires on a 401 from the original request, and any 401-then-redirect path would go back through AuthTransport anyway, so the same skip applies. Happy to fold in a separate RefreshTransport hardening if you want. |
2 participants
AuthTransport.RoundTripunconditionally re-set the Authorization header, which clobbered stdlibhttp.Client's cross-host strip on artifact-download redirects to presignedbuildkiteartifacts.com/ S3 URLs. The presigned URL already carries its own auth viaX-Amz-Signature, so the extra header either leaks the bearer or breaks the download with S3's "Only one auth mechanism allowed" 400. New test redirects through twohttptestservers and asserts the second hop sees noAuthorization.Closes #830