feat: immutable cookie session values (#2761)

Closes #2701

Author: Ajay Kelkar

selfservice/flow/login/handler_test.go

@@ -11,6 +11,8 @@ import (
 	"testing"
 	"time"
 
+	"github.com/ory/x/urlx"
+
 	"github.com/ory/x/sqlxx"
 
 	"github.com/ory/kratos/selfservice/flow"
@@ -233,6 +235,73 @@ func TestFlowLifecycle(t *testing.T) {
 					assert.NotEqual(t, gjson.Get(b, "session.id").String(), gjson.Get(a, "id").String())
 				})
 			})
+
+			t.Run("case=changed kratos session identifiers when refresh is true", func(t *testing.T) {
+				t.Cleanup(func() {
+					conf.MustSet(ctx, config.ViperKeySelfServiceBrowserDefaultReturnTo, "https://www.ory.sh")
+				})
+
+				t.Run("type=browser", func(t *testing.T) {
+					// Setup flow
+					f := login.Flow{Type: flow.TypeBrowser, ExpiresAt: time.Now().Add(time.Minute), IssuedAt: time.Now(), UI: container.New(""), Refresh: false, RequestedAAL: "aal1"}
+					require.NoError(t, reg.LoginFlowPersister().CreateLoginFlow(context.Background(), &f))
+
+					// Submit Login
+					hc := testhelpers.NewClientWithCookies(t)
+					res, err := hc.PostForm(ts.URL+login.RouteSubmitFlow+"?flow="+f.ID.String(), url.Values{"method": {"password"}, "password_identifier": {id1mail}, "password": {"foobar"}, "csrf_token": {x.FakeCSRFToken}})
+					require.NoError(t, err)
+
+					// Check response and session cookie presence
+					assert.Equal(t, http.StatusOK, res.StatusCode)
+					require.Len(t, hc.Jar.Cookies(urlx.ParseOrPanic(ts.URL+login.RouteGetFlow)), 1)
+					require.Contains(t, fmt.Sprintf("%v", hc.Jar.Cookies(urlx.ParseOrPanic(ts.URL))), "ory_kratos_session")
+					cookies1 := hc.Jar.Cookies(urlx.ParseOrPanic(ts.URL + login.RouteGetFlow))
+
+					req, err := http.NewRequest("GET", ts.URL+"/sessions/whoami", nil)
+					require.NoError(t, err)
+
+					res, err = hc.Do(req)
+					require.NoError(t, err)
+					assert.Equal(t, http.StatusOK, res.StatusCode)
+					firstSession := x.MustReadAll(res.Body)
+					require.NoError(t, res.Body.Close())
+
+					// Refresh
+					f = login.Flow{Type: flow.TypeBrowser, ExpiresAt: time.Now().Add(time.Minute), IssuedAt: time.Now(), UI: container.New(""), Refresh: true, RequestedAAL: "aal1"}
+					require.NoError(t, reg.LoginFlowPersister().CreateLoginFlow(context.Background(), &f))
+
+					vv := testhelpers.EncodeFormAsJSON(t, false, url.Values{"method": {"password"}, "password_identifier": {id1mail}, "password": {"foobar"}, "csrf_token": {x.FakeCSRFToken}})
+
+					req, err = http.NewRequest("POST", ts.URL+login.RouteSubmitFlow+"?flow="+f.ID.String(), strings.NewReader(vv))
+					require.NoError(t, err)
+					req.Header.Set("Content-Type", "application/x-www-form-urlencoded")
+
+					// Submit Login
+					res, err = hc.Do(req)
+					require.NoError(t, err)
+
+					// Check response and session cookie presence
+					assert.Equal(t, http.StatusOK, res.StatusCode)
+					require.Len(t, hc.Jar.Cookies(urlx.ParseOrPanic(ts.URL+login.RouteGetFlow)), 1)
+					require.Contains(t, fmt.Sprintf("%v", hc.Jar.Cookies(urlx.ParseOrPanic(ts.URL))), "ory_kratos_session")
+					cookies2 := hc.Jar.Cookies(urlx.ParseOrPanic(ts.URL + login.RouteGetFlow))
+
+					req, err = http.NewRequest("GET", ts.URL+"/sessions/whoami", nil)
+					require.NoError(t, err)
+
+					res, err = hc.Do(req)
+					require.NoError(t, err)
+					assert.Equal(t, http.StatusOK, res.StatusCode)
+					secondSession := x.MustReadAll(res.Body)
+					require.NoError(t, res.Body.Close())
+
+					// Sessions should still be resolvable despite different kratos session identifier due to nonce
+					assert.NotEqual(t, cookies1[0].String(), cookies2[0].String())
+					assert.Equal(t, id1mail, gjson.Get(string(firstSession), "identity.traits.username").String())
+					assert.Equal(t, id1mail, gjson.Get(string(secondSession), "identity.traits.username").String())
+					assert.Equal(t, gjson.Get(string(secondSession), "id").String(), gjson.Get(string(firstSession), "id").String())
+				})
+			})
 		})
 
 		t.Run("case=ensure aal is checked for upgradeability on session", func(t *testing.T) {

selfservice/flow/settings/handler_test.go

@@ -402,6 +402,32 @@ func TestHandler(t *testing.T) {
 				assert.Equal(t, "You must restart the flow because the resumable session was initiated by another person.", gjson.Get(actual, "ui.messages.0.text").String(), actual)
 			})
 		})
+
+		t.Run("description=submit - kratos session cookie issued", func(t *testing.T) {
+			t.Run("type=spa", func(t *testing.T) {
+				_, body := initFlow(t, primaryUser, false)
+				var f kratos.SelfServiceSettingsFlow
+				require.NoError(t, json.Unmarshal(body, &f))
+
+				actual, res := testhelpers.SettingsMakeRequest(t, false, true, &f, primaryUser, fmt.Sprintf(`{"method":"profile", "numby": 15, "csrf_token": "%s"}`, x.FakeCSRFToken))
+				assert.Equal(t, http.StatusOK, res.StatusCode)
+				require.Len(t, primaryUser.Jar.Cookies(urlx.ParseOrPanic(publicTS.URL+login.RouteGetFlow)), 1)
+				require.Contains(t, fmt.Sprintf("%v", primaryUser.Jar.Cookies(urlx.ParseOrPanic(publicTS.URL))), "ory_kratos_session")
+				assert.Equal(t, "Your changes have been saved!", gjson.Get(actual, "ui.messages.0.text").String(), actual)
+			})
+
+			t.Run("type=browser", func(t *testing.T) {
+				_, body := initFlow(t, primaryUser, false)
+				var f kratos.SelfServiceSettingsFlow
+				require.NoError(t, json.Unmarshal(body, &f))
+
+				actual, res := testhelpers.SettingsMakeRequest(t, false, false, &f, primaryUser, `method=profile&traits.numby=15&csrf_token=`+x.FakeCSRFToken)
+				assert.Equal(t, http.StatusOK, res.StatusCode)
+				require.Len(t, primaryUser.Jar.Cookies(urlx.ParseOrPanic(publicTS.URL+login.RouteGetFlow)), 1)
+				require.Contains(t, fmt.Sprintf("%v", primaryUser.Jar.Cookies(urlx.ParseOrPanic(publicTS.URL))), "ory_kratos_session")
+				assert.Equal(t, "Your changes have been saved!", gjson.Get(actual, "ui.messages.0.text").String(), actual)
+			})
+		})
 	})
 
 	t.Run("case=relative redirect when self-service settings ui is a relative url", func(t *testing.T) {

selfservice/flow/settings/hook.go

@@ -6,6 +6,8 @@ import (
 	"net/http"
 	"time"
 
+	"github.com/ory/kratos/session"
+
 	"github.com/ory/kratos/text"
 	"github.com/ory/kratos/ui/container"
 	"github.com/ory/kratos/ui/node"
@@ -48,6 +50,7 @@ type (
 	executorDependencies interface {
 		identity.ManagementProvider
 		identity.ValidationProvider
+		session.ManagementProvider
 		config.Provider
 
 		HandlerProvider
@@ -271,7 +274,21 @@ func (e *HookExecutor) PostSettingsHook(w http.ResponseWriter, r *http.Request,
 		WithField("flow_method", settingsType).
 		Debug("Completed all PostSettingsPrePersistHooks and PostSettingsPostPersistHooks.")
 
-	if ctxUpdate.Flow.Type == flow.TypeAPI || x.IsJSONRequest(r) {
+	if ctxUpdate.Flow.Type == flow.TypeAPI {
+		updatedFlow, err := e.d.SettingsFlowPersister().GetSettingsFlow(r.Context(), ctxUpdate.Flow.ID)
+		if err != nil {
+			return err
+		}
+
+		e.d.Writer().Write(w, r, updatedFlow)
+		return nil
+	}
+
+	if err := e.d.SessionManager().IssueCookie(r.Context(), w, r, ctxUpdate.Session); err != nil {
+		return errors.WithStack(err)
+	}
+
+	if x.IsJSONRequest(r) {
 		updatedFlow, err := e.d.SettingsFlowPersister().GetSettingsFlow(r.Context(), ctxUpdate.Flow.ID)
 		if err != nil {
 			return err

session/manager_http.go

@@ -6,6 +6,8 @@ import (
 	"net/url"
 	"time"
 
+	"github.com/ory/x/randx"
+
 	"github.com/gorilla/sessions"
 
 	"github.com/ory/x/urlx"
@@ -129,6 +131,7 @@ func (s *ManagerHTTP) IssueCookie(ctx context.Context, w http.ResponseWriter, r
 
 	cookie.Values["session_token"] = session.Token
 	cookie.Values["expires_at"] = session.ExpiresAt.UTC().Format(time.RFC3339Nano)
+	cookie.Values["nonce"] = randx.MustString(8, randx.Alpha) // Guarantee new kratos session identifier
 
 	if err := cookie.Save(r, w); err != nil {
 		return errors.WithStack(err)

session/manager_http_test.go

@@ -104,6 +104,13 @@ func TestManagerHTTP(t *testing.T) {
 			return rec.Result().Cookies()[0]
 		}
 
+		t.Run("case=immutability", func(t *testing.T) {
+			cookie1 := getCookie(t, x.NewTestHTTPRequest(t, "GET", "https://baseurl.com/bar", nil))
+			cookie2 := getCookie(t, x.NewTestHTTPRequest(t, "GET", "https://baseurl.com/bar", nil))
+
+			assert.NotEqual(t, cookie1.Value, cookie2.Value)
+		})
+
 		t.Run("case=with default options", func(t *testing.T) {
 			actual := getCookie(t, httptest.NewRequest("GET", "https://baseurl.com/bar", nil))
 			assert.EqualValues(t, "", actual.Domain, "Domain is empty because unset as a config option")