From 7f8691ed10f3a279a73da0b30a53059d32540598 Mon Sep 17 00:00:00 2001 From: Sidartha Rakuram Date: Sun, 26 Nov 2023 18:59:20 +0800 Subject: [PATCH] fix: ignore CSRF middleware on Apple OIDC callback --- internal/testhelpers/session.go | 10 ++++++++++ selfservice/strategy/oidc/strategy.go | 4 ++-- selfservice/strategy/oidc/strategy_test.go | 6 ++++-- session/handler_test.go | 14 ++------------ 4 files changed, 18 insertions(+), 16 deletions(-) diff --git a/internal/testhelpers/session.go b/internal/testhelpers/session.go index 91a2da5abda2..e6e9da316ffd 100644 --- a/internal/testhelpers/session.go +++ b/internal/testhelpers/session.go @@ -251,3 +251,13 @@ func (ct *TransportWithHeader) RoundTrip(req *http.Request) (*http.Response, err } return ct.RoundTripper.RoundTrip(req) } + +func AssertNoCSRFCookieInResponse(t *testing.T, _ *httptest.Server, _ *http.Client, r *http.Response) { + found := false + for _, c := range r.Cookies() { + if strings.HasPrefix(c.Name, "csrf_token") { + found = true + } + } + require.False(t, found) +} diff --git a/selfservice/strategy/oidc/strategy.go b/selfservice/strategy/oidc/strategy.go index 861547874cb5..a2afff3ea425 100644 --- a/selfservice/strategy/oidc/strategy.go +++ b/selfservice/strategy/oidc/strategy.go @@ -210,9 +210,9 @@ func (s *Strategy) setRoutes(r *x.RouterPublic) { // Apple can use the POST request method when calling the callback if handle, _, _ := r.Lookup("POST", RouteCallback); handle == nil { // Hardcoded path to Apple provider, I don't have a better way of doing it right now. - // Also this exempt disables CSRF checks for both GET and POST requests. Unfortunately + // Also this ignore disables CSRF checks for both GET and POST requests. Unfortunately // CSRF handler does not allow to define a rule based on the request method, at least not yet. - s.d.CSRFHandler().ExemptPath(RouteBase + "/callback/apple") + s.d.CSRFHandler().IgnorePath(RouteBase + "/callback/apple") // When handler is called using POST method, the cookies are not attached to the request // by the browser. So here we just redirect the request to the same location rewriting the diff --git a/selfservice/strategy/oidc/strategy_test.go b/selfservice/strategy/oidc/strategy_test.go index c5d441bbd766..0b4ca95c6e9e 100644 --- a/selfservice/strategy/oidc/strategy_test.go +++ b/selfservice/strategy/oidc/strategy_test.go @@ -1417,14 +1417,13 @@ func TestPostEndpointRedirect(t *testing.T) { remoteAdmin, remotePublic, _ := newHydra(t, &subject, &claims, &scope) - publicTS, adminTS := testhelpers.NewKratosServers(t) + publicTS, _, _, _ := testhelpers.NewKratosServerWithCSRFAndRouters(t, reg) viperSetProviderConfig( t, conf, newOIDCProvider(t, publicTS, remotePublic, remoteAdmin, "apple"), ) - testhelpers.InitKratosServers(t, reg, publicTS, adminTS) t.Run("case=should redirect to GET and preserve parameters"+publicTS.URL, func(t *testing.T) { // create a client that does not follow redirects @@ -1441,5 +1440,8 @@ func TestPostEndpointRedirect(t *testing.T) { location, err := res.Location() require.NoError(t, err) assert.Equal(t, publicTS.URL+"/self-service/methods/oidc/callback/apple?state=foo&test=3", location.String()) + + // We don't want to add/override CSRF cookie when redirecting + testhelpers.AssertNoCSRFCookieInResponse(t, publicTS, c, res) }) } diff --git a/session/handler_test.go b/session/handler_test.go index 286943796927..b08fb4d1ecb5 100644 --- a/session/handler_test.go +++ b/session/handler_test.go @@ -51,16 +51,6 @@ func send(code int) httprouter.Handle { } } -func assertNoCSRFCookieInResponse(t *testing.T, _ *httptest.Server, _ *http.Client, r *http.Response) { - found := false - for _, c := range r.Cookies() { - if strings.HasPrefix(c.Name, "csrf_token") { - found = true - } - } - require.False(t, found) -} - func TestSessionWhoAmI(t *testing.T) { conf, reg := internal.NewFastRegistryWithMocks(t) ts, _, r, _ := testhelpers.NewKratosServerWithCSRFAndRouters(t, reg) @@ -156,7 +146,7 @@ func TestSessionWhoAmI(t *testing.T) { // No cookie yet -> 401 res, err := client.Get(ts.URL + RouteWhoami) require.NoError(t, err) - assertNoCSRFCookieInResponse(t, ts, client, res) // Test that no CSRF cookie is ever set here. + testhelpers.AssertNoCSRFCookieInResponse(t, ts, client, res) // Test that no CSRF cookie is ever set here. if cacheEnabled { assert.NotEmpty(t, res.Header.Get("Ory-Session-Cache-For")) @@ -183,7 +173,7 @@ func TestSessionWhoAmI(t *testing.T) { require.NoError(t, err) body, err := io.ReadAll(res.Body) require.NoError(t, err) - assertNoCSRFCookieInResponse(t, ts, client, res) // Test that no CSRF cookie is ever set here. + testhelpers.AssertNoCSRFCookieInResponse(t, ts, client, res) // Test that no CSRF cookie is ever set here. assert.EqualValues(t, http.StatusOK, res.StatusCode) assert.NotEmpty(t, res.Header.Get("X-Kratos-Authenticated-Identity-Id"))