Question on `DownloadReleaseAsset` redirect logic

[stevehipwell]

I'm just looking at the implementation of DownloadReleaseAsset based on some code that calls is with a copy of the GitHub client as the followRedirectsClient and I'm trying to understand why the redirect is always disabled? Based on what I've seen on GitHub it looks like some assets were being server from AWS (9 years ago) which in turn was having issues with the headers.

I'm wondering if this implementation is outdated? At a minimum shouldn't redirects to https://release-assets.githubusercontent.com (or github.com) be handled by the primary client if no additional client is passed in?

I think we'd also need to look at how GHES and GHEC work, but I'd expect that they both need authentication to access artifacts.

CC @gmlewis

[gmlewis]

Without doing a deep dive into the history of this code using "Blame" to follow the thread of development over the years, I would guess that it is not enabled by default for backward compatibility, such that only those who wish to actually follow the redirects will do so. If a client just wants to know what the link is, they wouldn't enable this option.

So if changes were made to this behavior, we would have to make it clear that there is a breaking change, and still be able to support those who do not wish to follow the redirects, but instead only wish to know that they exist.

[stevehipwell]

@gmlewis I saw mention of AWS and S3 failing when the GH token was in the headers but the linked GH docs appear to be missing. I agree that just getting the URL may be useful functionality but I'd expect a function called DownloadReleaseAsset to actually download the release asset. Would you be happy if I "fixed" DownloadReleaseAsset and added a new GetReleaseDownloadURL.

Also is there a reason why we don't copy the client to disable redirects rather than lock the active client and disable redirects on it?

[gmlewis]

Also is there a reason why we don't copy the client to disable redirects rather than lock the active client and disable redirects on it?

Yes, absolutely there was a leak of client credentials when we used to copy the client, and I would have to go back and search for the issue to explain it more fully.

As for your other question, I will have to devote more time into looking back at the history of DownloadReleaseAsset to answer your question about "fixing" it before I can confidently answer. I know it has had quite a history of it being "fixed" many times over the years, so this will probably take some time.

[stevehipwell]

Yes, absolutely there was a leak of client credentials when we used to copy the client, and I would have to go back and search for the issue to explain it more fully.

I'd be interested in the issue; we currently support copying the client for external usage which is the pattern I'm trying to avoid. I can't think of any security implications of copying the client given that it's internal and has a limited scope (single request). I can think of a number of issues that locking the default client could cause.

[gmlewis]

Yes, absolutely there was a leak of client credentials when we used to copy the client, and I would have to go back and search for the issue to explain it more fully.

I'd be interested in the issue; we currently support copying the client for external usage which is the pattern I'm trying to avoid. I can't think of any security implications of copying the client given that it's internal and has a limited scope (single request). I can think of a number of issues that locking the default client could cause.

httpClient issues:

• Unsafe concurrent modification of http.DefaultClient #1173
• Access token leakage when reusing the client between users #3043
• Proposal: client.WithOptions #2897
• Is NewClientWithEnvProxy necessary? #2898
• Passing the http.DefaultClient to a New client can lead to breakdowns #3028
• Add NewTokenClient constructor #2636
• Add convenience helper for creating client with custom (enterprise) URL. #756

DownloadReleaseAsset issues:

• DownloadReleaseAsset breaks with renamed repository #3081
• DownloadReleaseAsset should follow the redirect and download the resource #1378
• Incorrect Accept header in DownloadReleaseAsset. #870
• DownloadReleaseAsset swallows API errors & returns io.ReadCloser of error payload #343
• DownloadReleaseAsset fails.  #246