From 879319cd2a8c3ea8c4be1967d7bccebda2cff4c0 Mon Sep 17 00:00:00 2001 From: Trong Huu Nguyen Date: Fri, 7 Jan 2022 11:37:17 +0100 Subject: [PATCH] fix(router/login): alleviate SameSite issues for login cookie A login cookie is set as part of the redirection flow between the RP and OP, and thus inherently involves cross-site requests. Our client uses the response_mode=query parameter for authorization requests, which should work with the SameSite attribute set to Lax. However, there are certain versions of user agents on certain operating systems (e.g. Safari 12.2 on iOS<12.2, MacOS<10.14.4, Android WebView<72) that do not properly handle cookies with the SameSite attribute set. This commit attempts to alleviate this issue for legacy browsers by introducing a fallback cookie without the SameSite attribute set. Additionally, we also set the SameSite value for the original login cookie to None to ensure that the cookie persists through the cross-origin redirection requests. --- pkg/router/cookies.go | 4 ++-- pkg/router/handler_callback.go | 2 +- pkg/router/handler_login.go | 24 ++++++++++++++++++++---- pkg/router/router_test.go | 6 ++++++ 4 files changed, 29 insertions(+), 7 deletions(-) diff --git a/pkg/router/cookies.go b/pkg/router/cookies.go index 413d03a..e970648 100644 --- a/pkg/router/cookies.go +++ b/pkg/router/cookies.go @@ -1,7 +1,6 @@ package router import ( - "fmt" "net/http" "github.com/nais/wonderwall/pkg/cookie" @@ -10,6 +9,7 @@ import ( const ( SessionCookieName = "io.nais.wonderwall.session" LoginCookieName = "io.nais.wonderwall.callback" + LoginLegacyCookieName = "io.nais.wonderwall.callback.legacy" ) func (h *Handler) setEncryptedCookie(w http.ResponseWriter, key string, plaintext string, opts cookie.Options) error { @@ -25,7 +25,7 @@ func (h *Handler) setEncryptedCookie(w http.ResponseWriter, key string, plaintex func (h *Handler) getDecryptedCookie(r *http.Request, key string) (string, error) { encryptedCookie, err := cookie.Get(r, key) if err != nil { - return "", fmt.Errorf("no cookie named '%s': %w", key, err) + return "", err } return encryptedCookie.Decrypt(h.Crypter) diff --git a/pkg/router/handler_callback.go b/pkg/router/handler_callback.go index 6eef211..772b480 100644 --- a/pkg/router/handler_callback.go +++ b/pkg/router/handler_callback.go @@ -57,7 +57,7 @@ func (h *Handler) Callback(w http.ResponseWriter, r *http.Request) { return } - h.clearLoginCookie(w) + h.clearLoginCookies(w) http.Redirect(w, r, loginCookie.Referer, http.StatusTemporaryRedirect) } diff --git a/pkg/router/handler_login.go b/pkg/router/handler_login.go index 1967aa7..85d582b 100644 --- a/pkg/router/handler_login.go +++ b/pkg/router/handler_login.go @@ -7,6 +7,8 @@ import ( "net/http" "time" + log "github.com/sirupsen/logrus" + "github.com/nais/wonderwall/pkg/cookie" "github.com/nais/wonderwall/pkg/openid" "github.com/nais/wonderwall/pkg/router/request" @@ -53,7 +55,12 @@ func (h *Handler) Login(w http.ResponseWriter, r *http.Request) { func (h *Handler) getLoginCookie(r *http.Request) (*openid.LoginCookie, error) { loginCookieJson, err := h.getDecryptedCookie(r, LoginCookieName) if err != nil { - return nil, err + log.Warnf("failed to fetch login cookie; falling back to legacy cookie: %+v", err) + log.Debugf("debug: user-agent: %s", r.UserAgent()) + loginCookieJson, err = h.getDecryptedCookie(r, LoginLegacyCookieName) + if err != nil { + return nil, err + } } var loginCookie openid.LoginCookie @@ -71,7 +78,9 @@ func (h *Handler) setLoginCookies(w http.ResponseWriter, loginCookie *openid.Log return fmt.Errorf("marshalling login cookie: %w", err) } - opts := h.Cookies.WithExpiresIn(LoginCookieLifetime) + opts := h.Cookies. + WithExpiresIn(LoginCookieLifetime). + WithSameSite(http.SameSiteNoneMode) value := string(loginCookieJson) err = h.setEncryptedCookie(w, LoginCookieName, value, opts) @@ -79,10 +88,17 @@ func (h *Handler) setLoginCookies(w http.ResponseWriter, loginCookie *openid.Log return err } + // set a duplicate cookie without the SameSite value set for user agents that do not properly handle SameSite + err = h.setEncryptedCookie(w, LoginLegacyCookieName, value, opts.WithSameSite(http.SameSiteDefaultMode)) + if err != nil { + return err + } + return nil } -func (h *Handler) clearLoginCookie(w http.ResponseWriter) { +func (h *Handler) clearLoginCookies(w http.ResponseWriter) { opts := h.Cookies - cookie.Clear(w, LoginCookieName, opts) + cookie.Clear(w, LoginCookieName, opts.WithSameSite(http.SameSiteNoneMode)) + cookie.Clear(w, LoginLegacyCookieName, opts.WithSameSite(http.SameSiteDefaultMode)) } diff --git a/pkg/router/router_test.go b/pkg/router/router_test.go index 8093ef5..e923d6a 100644 --- a/pkg/router/router_test.go +++ b/pkg/router/router_test.go @@ -69,6 +69,8 @@ func TestHandler_Login(t *testing.T) { cookies := client.Jar.Cookies(loginURL) loginCookie := getCookieFromJar(router.LoginCookieName, cookies) assert.NotNil(t, loginCookie) + loginLegacyCookie := getCookieFromJar(router.LoginLegacyCookieName, cookies) + assert.NotNil(t, loginLegacyCookie) location := resp.Header.Get("location") u, err := url.Parse(location) @@ -126,9 +128,11 @@ func TestHandler_Callback_and_Logout(t *testing.T) { cookies := client.Jar.Cookies(loginURL) sessionCookie := getCookieFromJar(router.SessionCookieName, cookies) loginCookie := getCookieFromJar(router.LoginCookieName, cookies) + loginLegacyCookie := getCookieFromJar(router.LoginLegacyCookieName, cookies) assert.Nil(t, sessionCookie) assert.NotNil(t, loginCookie) + assert.NotNil(t, loginLegacyCookie) // Get authorization URL location := resp.Header.Get("location") @@ -154,9 +158,11 @@ func TestHandler_Callback_and_Logout(t *testing.T) { cookies = client.Jar.Cookies(callbackURL) sessionCookie = getCookieFromJar(router.SessionCookieName, cookies) loginCookie = getCookieFromJar(router.LoginCookieName, cookies) + loginLegacyCookie = getCookieFromJar(router.LoginLegacyCookieName, cookies) assert.NotNil(t, sessionCookie) assert.Nil(t, loginCookie) + assert.Nil(t, loginLegacyCookie) // Request self-initiated logout logoutURL, err := url.Parse(server.URL + "/oauth2/logout")