Redirect issue with CreateRelease() or UpdateRelease()

[clarsonneur]

Hi,

I built a small code to create or update release in a github entreprise url thanks to your project.
But I made a small mistake by forgotten the s in the http url string. My fault ;-)

Because of that, all queries that I made (ListReleases, ListTags, and login) got the 301 return code and redirect appropriately to the https equivalent.

But not for CreateRelease or UpdateRelease.

I'm wondering if this behavior is normal one. I'm wondering if those functiona should redirect as well to the appropriate link, like others.

I would consider this as a bug, but I'd like to see if you do consider it as well.

If so, I could spend time to review the code and propose a patch.

Thanks

[dmitshur]

Hmm, supporting redirects transparent like that sounds like a double-edged sword. On the up side, it allows things to work, but on the (much more significant IMO) down side, it makes it harder to realize it's happening. Going through http -> https redirects like that means there are 2 HTTP requests being made for every single API request. More work for both the client and server, more bandwidth used, higher latency for responses, etc.

I wonder if we can disable such redirects in the first place and return an error right away, in turn making such an unintended error much more visible and impossible to accidentally make? Would it break any of our existing endpoints, is there anything that requires redirects even if https is hit directly?

In terms of implementation, I believe we can set the http.Client's CheckRedirect field to a function that instantly returns http.ErrUseLastResponse.

If we can do that, I think that's the best solution. What do you think?

[willnorris]

if I remember correctly, there are a number of API calls that should follow redirects (for example, fetching release assets, which are typically stored on Amazon S3 so GitHub sends a redirect to that URL). So I don't think we'd want to disable redirects entirely.

Also, does GitHub Enterprise allow you to host on an http URL or do they require https? I'm guessing they do allow it, in which case we couldn't simply enforce that you always use an https URL. I'm honestly not sure that there is a good fix for this 😕

[dmitshur]

if I remember correctly, there are a number of API calls that should follow redirects (for example, fetching release assets, which are typically stored on Amazon S3 so GitHub sends a redirect to that URL). So I don't think we'd want to disable redirects entirely.

I was just looking at that, and I found that they explicitly don't follow the redirects. Instead, they return the redirect URL to caller, so the caller can then fetch the asset from AWS, etc. See:

go-github/github/migrations.go

Lines 169 to 185 in fbfee05

	// Disable the redirect mechanism because AWS fails if the GitHub auth token is provided.
	var loc string
	saveRedirect := s.client.client.CheckRedirect
	s.client.client.CheckRedirect = func(req *http.Request, via []*http.Request) error {
	loc = req.URL.String()
	return errors.New("disable redirect")
	}
	defer func() { s.client.client.CheckRedirect = saveRedirect }()
	_, err = s.client.Do(ctx, req, nil) // expect error from disable redirect
	if err == nil {
	return "", errors.New("expected redirect, none provided")
	}
	if !strings.Contains(err.Error(), "disable redirect") {
	return "", err
	}
	return loc, nil

go-github/github/repos_releases.go

Lines 239 to 254 in fbfee05

	var loc string
	saveRedirect := s.client.client.CheckRedirect
	s.client.client.CheckRedirect = func(req *http.Request, via []*http.Request) error {
	loc = req.URL.String()
	return errors.New("disable redirect")
	}
	defer func() { s.client.client.CheckRedirect = saveRedirect }()
	req = withContext(ctx, req)
	resp, err := s.client.client.Do(req)
	if err != nil {
	if !strings.Contains(err.Error(), "disable redirect") {
	return nil, "", err
	}
	return nil, loc, nil // Intentionally return no error with valid redirect URL.
	}

Is that right @willnorris? Or are there some other API calls that I haven't considered?

Also, does GitHub Enterprise allow you to host on an http URL or do they require https? I'm guessing they do allow it, in which case we couldn't simply enforce that you always use an https URL. I'm honestly not sure that there is a good fix for this 😕

Yeah, I was thinking that too. Bummer. We could consider doing a log.Printf("warning: http protocol is specified in URL") call.

[willnorris]

I found that they explicitly don't follow the redirects [...] Is that right @willnorris? Or are there some other API calls that I haven't considered?

oh, right! I had completely forgotten about that :) There was some specific mention about needing to follow redirects some time ago:

• https://developer.github.com/changes/2015-04-17-preview-repository-redirects/
• https://developer.github.com/v3/#http-redirects

I don't know what all endpoints utilize redirects, but since they specifically call it out in the API docs, doesn't sounds like something we'd want to disable.

[dmitshur]

Oh, good point. There are definitely (wanted) redirects involved when hitting a repository that was moved/renamed, etc.

What if we check the scheme before and after in the redirect. If the original scheme is "http" and redirect scheme is "https", then we return an error or log a warning. (Optionally, only do this if the scheme is the only part of the URL that has changed.)

I'm not necessarily in favor of doing that, because it's turning out to be more complex than "just disable all redirects", but wanted to throw the idea out for consideration.

[willnorris]

yeah, that sounds like more complexity than I'd want to add.

[clarsonneur]

Thank you @shurcooL and @willnorris for your answer.

On my side, I'm now detecting the http scheme and I set a warning. But nothing more.

I like the idea that if we get a redirect to the same url, with a simple change on the scheme, we could fail to ask to use the appropriate link.
But I understand it to be a little complex to update in the code, everywhere. Do you want me to have a look?
Or, you do not consider it as useful?

Thanks

[dmitshur]

I don't think it'd be hard to do. It can probably be centralized in the Do method.

However, I agree with @willnorris and feel unconvinced that the potential benefits justify the cost of having this extra code run on every single API call, needlessly so in projects that use go-github correctly.

I'd be more okay with a check in NewClient that runs once, but currently I don't see what can be done there.

[clarsonneur]

Agree. Make more sense to me too. I will have a look in the NewClient.

Thanks

[gmlewis]

Fixed by #1772. Closing.