fix: force HTTP GET for redirect responses

[svrakitin]

302 Found behavior is not consistent across browsers and my team got into an issue with Safari maintaining HTTP POST across redirects after login through form. This results in a callback executed with HTTP POST instead of HTTP GET.

Changing redirects to 303 See Other to force HTTP GET.

Checklist

• I have read the contributing guidelines
  and signed the CLA.
• I have referenced an issue containing the design document if my change
  introduces a new feature.
• I have read the security policy.
• I confirm that this pull request does not address a security
  vulnerability. If this pull request addresses a security vulnerability, I
  confirm that I got green light (please contact
  security@ory.sh) from the maintainers to push
  the changes.
• I have added tests that prove my fix is effective or that my feature
  works.
• I have added necessary documentation within the code base (if
  appropriate).

[mitar]

Nice catch. This seems to be also recommended best security practice: https://tools.ietf.org/id/draft-ietf-oauth-security-topics-08.html#rfc.section.3.9

[mitar]

I am surprised that oidc-conformity test has not detected this.

[svrakitin]

@mitar I am not sure this is actually a requirement, this is just an implementation detail.

[mitar]

Sure, but it could at least warn about it. I mean, it is in security best practices.

[enj]

@svrakitin @mitar @aeneasr

I believe this change is the correct thing to do, but it has the potential to break non-browser clients that expect a http.StatusFound instead of http.StatusSeeOther. In our case we had to make the following change to override this behavior while we rollout changes to our client code:

w = httpsnoop.Wrap(w, httpsnoop.Hooks{
	WriteHeader: func(delegate httpsnoop.WriteHeaderFunc) httpsnoop.WriteHeaderFunc {
		return func(code int) {
			if code == http.StatusSeeOther {
				code = http.StatusFound
			}
			delegate(code)
		}
	},
})
oauth2Provider.WriteAuthorizeResponse(w, authorizeRequester, authorizeResponder)

[aeneasr]

Sorry about that! Hopefully the migration wasn’t too painful :)

[glnarus]

Hi @aeneasr , we ran into the same situation as @enj ; my current company uses embedded browser in deployed clients that can only accept 302 redirects. Doing updates to these deployed clients faces high customer resistance and long time horizons for uptake (if ever). Most likely meaning that we are stuck on an older version of Hydra due to this change.

What do you all think about making which redirect HTTP action to use a configuration option?

[mitar]

@glnarus What about a workaround from @enj above? It should help your case as well.

[mitar]

You can also do a similar thing with a reverse proxy in front of fosite (which you might already have). I think this is a reasonable place to do such change to handle legacy apps.

[glnarus]

Thank you @mitar - the workaround would help, but if I understand it correctly, the appetite here is not high for the extra support burden of always having to modify the Hydra source; though we may very well be forced to do so. When I brought up the reverse proxy man in the middle solution, I was met with concerns on over complicating the system; but perhaps if this can be done with our existing Apache server, it would be preferable. Will look into this further.

I wanted to share a datapoint that three major identity providers we have been engaged with all utilize 302 redirects in their OAuth2 flow. Something perhaps to consider as an impact for Hydra's ability to address more use cases out of the box.

[james-d-elliott]

I think the 303 should only be sent if the method verb used for the authorize request was POST not when it's originally a GET. Unless I am missing something I suspect this is partially the source of the issues with this particular change and it would most likely also be fixed in most situations if changed to only send 303 if the method was originally POST.

[mitar]

An interesting idea.

[james-d-elliott]

See #724, we probably need to have those who originally had the issue that this fix addressed check this. I'm also looking into some more OAuth 2.0 specific implementation details, it looks like maybe this is not as clear cut as with other things related to OAuth 2.0 however.