Skip to content
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

fix: ignore CSRF middleware on Apple OIDC callback #3643

Merged
merged 3 commits into from
Dec 5, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions internal/testhelpers/session.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
4 changes: 2 additions & 2 deletions selfservice/strategy/oidc/strategy.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
6 changes: 4 additions & 2 deletions selfservice/strategy/oidc/strategy_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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)
})
}
14 changes: 2 additions & 12 deletions session/handler_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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"))
Expand All @@ -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"))
Expand Down
Loading