New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Redirect issue with CreateRelease() or UpdateRelease() #804

Open
clarsonneur opened this Issue Dec 4, 2017 · 9 comments

Comments

Projects
None yet
3 participants
@clarsonneur

clarsonneur commented Dec 4, 2017

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

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur Dec 4, 2017

Member

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?

Member

dmitshur commented Dec 4, 2017

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

This comment has been minimized.

Show comment
Hide comment
@willnorris

willnorris Dec 4, 2017

Member

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 😕

Member

willnorris commented Dec 4, 2017

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

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur Dec 4, 2017

Member

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:

// 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

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.

Member

dmitshur commented Dec 4, 2017

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:

// 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

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

This comment has been minimized.

Show comment
Hide comment
@willnorris

willnorris Dec 4, 2017

Member

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:

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.

Member

willnorris commented Dec 4, 2017

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:

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

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur Dec 4, 2017

Member

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.

Member

dmitshur commented Dec 4, 2017

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

This comment has been minimized.

Show comment
Hide comment
@willnorris

willnorris Dec 4, 2017

Member

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

Member

willnorris commented Dec 4, 2017

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

@clarsonneur

This comment has been minimized.

Show comment
Hide comment
@clarsonneur

clarsonneur Dec 11, 2017

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

clarsonneur commented Dec 11, 2017

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

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur Dec 11, 2017

Member

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.

Member

dmitshur commented Dec 11, 2017

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

This comment has been minimized.

Show comment
Hide comment
@clarsonneur

clarsonneur Dec 12, 2017

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

Thanks

clarsonneur commented Dec 12, 2017

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

Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment