fix: use HTTP 303 instead of 302 for selfservice redirects #2215
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2215 +/- ##
==========================================
+ Coverage 75.84% 75.88% +0.04%
==========================================
Files 297 297
Lines 15905 15905
==========================================
+ Hits 12063 12070 +7
+ Misses 2984 2977 -7
Partials 858 858
Continue to review full report at Codecov.
|
|
The end-to-end tests failing seems to be an issue with the current version of yq. Running
:/ |
There was a problem hiding this comment.
yq seems to be broken since #2206, so blame @aeneasr 😉
This looks very good, now all redirects seem consistent. Is it expected that anyone depends on a 302 code and we need to mark this as a breaking change? I would assume not...
It would be nice if you could add some tests (or probably just extend existing ones) that ensure no regression will happen.
Handling the 302/303 redirect is the user agent's responsibility. According to MDN, both are supported by all browsers but 302 handling seems to be inconsistently implemented, whereas with 303 the subsequent request method is always GET, which is probably what we want when redirecting to the application UI. So if anything there is a very slim chance this could fix otherwise broken redirects if some obscure browser decides to handle 302 weirdly. In any case it makes user agent behavior more predictable I guess. I'll look into writing tests, I guess this would be a case for the e2e tests, right? |
|
@maurice-freitag I agree, the response code should be 303. I believe you can write some tests in each flow I can take a look today to direct you properly on this :) |
|
So after looking at this a bit, we don't have any tests currently covering if the redirect takes place within the flows' So I wrote a quick sample for you to add and which you can reuse in the other flows. under t.Run("lifecycle=init", func(t *testing.T) {
...
t.Run("flow=browser", func(t *testing.T) {
...
t.Run("case=redirects with 303", func(t *testing.T) {
c := ts.Client()
// prevent the redirect
c.CheckRedirect = func(req *http.Request, via []*http.Request) error {
return http.ErrUseLastResponse
}
req, err := http.NewRequest("GET", ts.URL+login.RouteInitBrowserFlow, nil)
require.NoError(t, err)
res, err := c.Do(req)
require.NoError(t, err)
// here we check that the redirect status is 303
require.Equal(t, http.StatusSeeOther, res.StatusCode)
defer res.Body.Close()
}) |
|
@maurice-freitag I have opened a PR against your branch implementing the |
Status code tests
ty! I merged your changes and will get going with the other tests asap :) |
|
Seems the docs have moved 🙈 I'll look into it when I get the chance |
|
Ah yes @maurice-freitag you can add the docs to https://github.com/ory/docs Sorry about that, we've been doing some major changes to the structure of our documentation. |
|
|
||
| t.Run("case=redirects with 303", func(t *testing.T) { | ||
| c := http.DefaultClient | ||
| // don't get the reference, instead copy the values, so we don't alter the client directly. | ||
| *c = *publicTS.Client() | ||
| // prevent the redirect | ||
| c.CheckRedirect = func(req *http.Request, via []*http.Request) error { | ||
| return http.ErrUseLastResponse | ||
| } | ||
| req, err := http.NewRequest("GET", publicTS.URL+settings.RouteInitBrowserFlow, nil) | ||
| require.NoError(t, err) | ||
|
|
||
| res, err := c.Do(req) | ||
| require.NoError(t, err) | ||
| // here we check that the redirect status is 303 | ||
| require.Equal(t, http.StatusSeeOther, res.StatusCode) | ||
| defer res.Body.Close() | ||
| }) | ||
| }) |
There was a problem hiding this comment.
Not sure if this will work, since we need a session first before we initialize a settings flow
Also shown in one of the tests above:
t.Run("description=success", func(t *testing.T) {
user1 := testhelpers.NewHTTPClientWithArbitrarySessionCookie(t, reg)
res, body := initFlow(t, user1, false)
assert.Contains(t, res.Request.URL.String(), reg.Config(context.Background()).SelfServiceFlowSettingsUI().String())
assertion(t, body, false)
})There was a problem hiding this comment.
The test ran through, I switched the default client with the one including the session cookie anyways since all other tests here use it as well
There was a problem hiding this comment.
I also instantiated a new http client instead of using http.DefaultClient because some subsequent tests where failing due to the changed redirect behavior on the default instance.
|
The PR for the docs is now open as well: ory/docs#598 |
|
Thank you! :) |
|
Hello @maurice-freitag |
Closes ory#1969 Co-authored-by: Alano Terblanche <[email protected]>
The issue mentions a discrepancy between the documented response code and the actual response code sent by Kratos during selfservice flow redirects. Upon further inspection I noticed that some handlers actually did return 302 which I have changed to 303 where appropriate. Fixed docs & rebuilt sdk as well.
Related issue(s)
#1969
Checklist
introduces a new feature.
contributing code guidelines.
vulnerability. If this pull request addresses a security. vulnerability, I
confirm that I got green light (please contact
[email protected]) from the maintainers to push
the changes.
addedadjusted tests that prove my fix is effective or that my featureworks.