fix: don't inherit flow type in recovery and verification flows (ory#…

…2250)

Closes ory#2049

Co-authored-by: Patrik <[email protected]>
Co-authored-by: aeneasr <[email protected]>
Co-authored-by: Patrik <[email protected]>

package.json

@@ -18,12 +18,12 @@
     "@types/node": "^16.9.6",
     "@types/yamljs": "^0.2.31",
     "chrome-remote-interface": "^0.31.0",
-    "cypress": "^9.1.1",
+    "cypress": "^9.5.0",
     "dayjs": "^1.10.4",
     "got": "^11.8.2",
     "ory-prettier-styles": "1.1.2",
-    "prettier": "2.3.2",
     "otplib": "^12.0.1",
+    "prettier": "2.3.2",
     "typescript": "^4.4.3",
     "wait-on": "5.3.0"
   },

selfservice/flow/recovery/error.go

@@ -83,7 +83,10 @@ func (s *ErrorHandler) WriteFlowError(
 			return
 		}
 
-		if f.Type == flow.TypeAPI || x.IsJSONRequest(r) {
+		// We need to use the new flow, as that flow will be a browser flow. Bug fix for:
+		//
+		// https://github.com/ory/kratos/issues/2049!!
+		if a.Type == flow.TypeAPI || x.IsJSONRequest(r) {
 			http.Redirect(w, r, urlx.CopyWithQuery(urlx.AppendPaths(s.d.Config(r.Context()).SelfPublicURL(),
 				RouteGetFlow), url.Values{"id": {a.ID.String()}}).String(), http.StatusSeeOther)
 		} else {

selfservice/flow/recovery/flow.go

@@ -130,7 +130,12 @@ func NewFlow(conf *config.Config, exp time.Duration, csrf string, r *http.Reques
 }
 
 func FromOldFlow(conf *config.Config, exp time.Duration, csrf string, r *http.Request, strategies Strategies, of Flow) (*Flow, error) {
-	nf, err := NewFlow(conf, exp, csrf, r, strategies, of.Type)
+	f := of.Type
+	// Using the same flow in the recovery/verification context can lead to using API flow in a verification/recovery email
+	if of.Type == flow.TypeAPI {
+		f = flow.TypeBrowser
+	}
+	nf, err := NewFlow(conf, exp, csrf, r, strategies, f)
 	if err != nil {
 		return nil, err
 	}

selfservice/flow/recovery/flow_test.go

@@ -84,3 +84,20 @@ func TestFlowEncodeJSON(t *testing.T) {
 	assert.EqualValues(t, "/bar", gjson.Get(jsonx.TestMarshalJSONString(t, &recovery.Flow{RequestURL: "https://foo.bar?return_to=/bar"}), "return_to").String())
 	assert.EqualValues(t, "/bar", gjson.Get(jsonx.TestMarshalJSONString(t, recovery.Flow{RequestURL: "https://foo.bar?return_to=/bar"}), "return_to").String())
 }
+
+func TestFromOldFlow(t *testing.T) {
+	conf := internal.NewConfigurationWithDefaults(t)
+	r := http.Request{URL: &url.URL{Path: "/", RawQuery: "return_to=" + urlx.AppendPaths(conf.SelfPublicURL(), "/self-service/login/browser").String()}, Host: "ory.sh"}
+	for _, ft := range []flow.Type{
+		flow.TypeAPI,
+		flow.TypeBrowser,
+	} {
+		t.Run(fmt.Sprintf("case=original flow is %s", ft), func(t *testing.T) {
+			f, err := recovery.NewFlow(conf, 0, "csrf", &r, nil, ft)
+			require.NoError(t, err)
+			nF, err := recovery.FromOldFlow(conf, time.Duration(time.Hour), f.CSRFToken, &r, []recovery.Strategy{}, *f)
+			require.NoError(t, err)
+			require.Equal(t, flow.TypeBrowser, nF.Type)
+		})
+	}
+}

selfservice/flow/verification/error.go

@@ -81,7 +81,10 @@ func (s *ErrorHandler) WriteFlowError(
 			return
 		}
 
-		if f.Type == flow.TypeAPI || x.IsJSONRequest(r) {
+		// We need to use the new flow, as that flow will be a browser flow. Bug fix for:
+		//
+		// https://github.com/ory/kratos/issues/2049!!
+		if a.Type == flow.TypeAPI || x.IsJSONRequest(r) {
 			http.Redirect(w, r, urlx.CopyWithQuery(urlx.AppendPaths(s.d.Config(r.Context()).SelfPublicURL(),
 				RouteGetFlow), url.Values{"id": {a.ID.String()}}).String(), http.StatusSeeOther)
 		} else {

selfservice/flow/verification/flow.go

@@ -133,7 +133,12 @@ func NewFlow(conf *config.Config, exp time.Duration, csrf string, r *http.Reques
 }
 
 func FromOldFlow(conf *config.Config, exp time.Duration, csrf string, r *http.Request, strategies Strategies, of *Flow) (*Flow, error) {
-	nf, err := NewFlow(conf, exp, csrf, r, strategies, of.Type)
+	f := of.Type
+	// Using the same flow in the recovery/verification context can lead to using API flow in a verification/recovery email
+	if of.Type == flow.TypeAPI {
+		f = flow.TypeBrowser
+	}
+	nf, err := NewFlow(conf, exp, csrf, r, strategies, f)
 	if err != nil {
 		return nil, err
 	}

selfservice/flow/verification/flow_test.go

@@ -119,3 +119,20 @@ func TestFlowEncodeJSON(t *testing.T) {
 	assert.EqualValues(t, "/bar", gjson.Get(jsonx.TestMarshalJSONString(t, &verification.Flow{RequestURL: "https://foo.bar?return_to=/bar"}), "return_to").String())
 	assert.EqualValues(t, "/bar", gjson.Get(jsonx.TestMarshalJSONString(t, verification.Flow{RequestURL: "https://foo.bar?return_to=/bar"}), "return_to").String())
 }
+
+func TestFromOldFlow(t *testing.T) {
+	conf := internal.NewConfigurationWithDefaults(t)
+	r := http.Request{URL: &url.URL{Path: "/", RawQuery: "return_to=" + urlx.AppendPaths(conf.SelfPublicURL(), "/self-service/login/browser").String()}, Host: "ory.sh"}
+	for _, ft := range []flow.Type{
+		flow.TypeAPI,
+		flow.TypeBrowser,
+	} {
+		t.Run(fmt.Sprintf("case=original flow is %s", ft), func(t *testing.T) {
+			f, err := verification.NewFlow(conf, 0, "csrf", &r, nil, ft)
+			require.NoError(t, err)
+			nf, err := verification.FromOldFlow(conf, time.Duration(time.Hour), f.CSRFToken, &r, []verification.Strategy{}, f)
+			require.NoError(t, err)
+			require.Equal(t, flow.TypeBrowser, nf.Type)
+		})
+	}
+}

test/e2e/cypress/integration/profiles/recovery/recovery/errors.spec.ts

@@ -1,4 +1,4 @@
-import { appPrefix, gen, parseHtml } from '../../../../helpers'
+import { APP_URL, appPrefix, gen, parseHtml } from '../../../../helpers'
 import { routes as react } from '../../../../helpers/react'
 import { routes as express } from '../../../../helpers/express'
 
@@ -30,6 +30,56 @@ context('Account Recovery Errors', () => {
         cy.enableRecovery()
       })
 
+      it('responds with a HTML response on link click of an API flow if the link is expired', () => {
+        cy.visit(recovery)
+
+        cy.shortLinkLifespan()
+
+        const identity = gen.identityWithWebsite()
+        cy.registerApi(identity)
+        cy.recoverApi({ email: identity.email })
+        cy.recoverEmailButExpired({ expect: { email: identity.email } })
+
+        cy.get('[data-testid="ui/message/4060005"]').should(
+          'contain.text',
+          'The recovery flow expired'
+        )
+
+        cy.noSession()
+      })
+
+      it('responds with a HTML response on link click of an API flow if the flow is expired', () => {
+        cy.visit(recovery)
+
+        cy.updateConfigFile((config) => {
+          config.selfservice.flows.recovery.lifespan = '1s'
+          return config
+        })
+
+        const identity = gen.identityWithWebsite()
+        cy.registerApi(identity)
+        cy.recoverApi({ email: identity.email })
+        cy.wait(1000)
+
+        cy.getMail().should((message) => {
+          expect(message.subject.trim()).to.equal(
+            'Recover access to your account'
+          )
+          expect(message.toAddresses[0].trim()).to.equal(identity.email)
+
+          const link = parseHtml(message.body).querySelector('a')
+          cy.longRecoveryLifespan()
+          cy.visit(link.href)
+        })
+
+        cy.get('[data-testid="ui/message/4060005"]').should(
+          'contain.text',
+          'The recovery flow expired'
+        )
+
+        cy.noSession()
+      })
+
       it('should receive a stub email when recovering a non-existent account', () => {
         cy.visit(recovery)
 

test/e2e/cypress/integration/profiles/verification/verify/errors.spec.ts

@@ -31,10 +31,6 @@ context('Account Verification Error', () => {
       })
 
       let identity
-      before(() => {
-        cy.deleteMail()
-      })
-
       beforeEach(() => {
         cy.clearAllCookies()
         cy.longVerificationLifespan()
@@ -47,6 +43,54 @@ context('Account Verification Error', () => {
         cy.visit(verification)
       })
 
+      it('responds with a HTML response on link click of an API flow if the flow is expired', () => {
+        cy.updateConfigFile((config) => {
+          config.selfservice.flows.verification.lifespan = '1s'
+          return config
+        })
+
+        cy.verificationApi({
+          email: identity.email
+        })
+
+        cy.wait(1000)
+        cy.shortVerificationLifespan()
+
+        cy.getMail().then((message) => {
+          expect(message.subject.trim()).to.equal(
+            'Please verify your email address'
+          )
+          expect(message.toAddresses[0].trim()).to.equal(identity.email)
+
+          const link = parseHtml(message.body).querySelector('a')
+
+          cy.longVerificationLifespan()
+          cy.visit(link.href)
+          cy.get('[data-testid="ui/message/4070005"]').should(
+            'contain.text',
+            'verification flow expired'
+          )
+
+          cy.getSession().should((session) => {
+            assertVerifiableAddress({
+              isVerified: false,
+              email: identity.email
+            })(session)
+          })
+        })
+      })
+
+      it('responds with a HTML response on link click of an API flow if the link is expired', () => {
+        cy.shortLinkLifespan()
+
+        // Init expired flow
+        cy.verificationApi({
+          email: identity.email
+        })
+
+        cy.verifyEmailButExpired({ expect: { email: identity.email } })
+      })
+
       it('is unable to verify the email address if the code is expired', () => {
         cy.shortLinkLifespan()
 
@@ -58,7 +102,7 @@ context('Account Verification Error', () => {
           'contain.text',
           'An email containing a verification'
         )
-
+        cy.get('[name="method"][value="link"]').should('exist')
         cy.verifyEmailButExpired({ expect: { email: identity.email } })
       })
 

test/e2e/cypress/support/commands.ts

@@ -246,6 +246,10 @@ Cypress.Commands.add('useLaxAal', ({} = {}) => {
   })
 })
 
+Cypress.Commands.add('updateConfigFile', (cb: (arg: any) => any) => {
+  updateConfigFile(cb)
+})
+
 Cypress.Commands.add(
   'register',
   ({
@@ -402,6 +406,71 @@ Cypress.Commands.add('recoverApi', ({ email, returnTo }) => {
     })
 })
 
+Cypress.Commands.add('verificationApi', ({ email, returnTo }) => {
+  let url = APP_URL + '/self-service/verification/api'
+  if (returnTo) {
+    url += '?return_to=' + returnTo
+  }
+  cy.request({ url })
+    .then(({ body }) => {
+      const form = body.ui
+      return cy.request({
+        method: form.method,
+        body: mergeFields(form, { email, method: 'link' }),
+        url: form.action
+      })
+    })
+    .then(({ body }) => {
+      expect(body.state).to.contain('sent_email')
+    })
+})
+
+Cypress.Commands.add('verificationApiExpired', ({ email, returnTo }) => {
+  cy.shortVerificationLifespan()
+  let url = APP_URL + '/self-service/verification/api'
+  if (returnTo) {
+    url += '?return_to=' + returnTo
+  }
+  cy.request({ url })
+    .then(({ body }) => {
+      const form = body.ui
+      return cy.request({
+        method: form.method,
+        body: mergeFields(form, { email, method: 'link' }),
+        url: form.action,
+        failOnStatusCode: false
+      })
+    })
+    .then((response) => {
+      expect(response.status).to.eq(410)
+      expect(response.body.error.reason).to.eq(
+        'The verification flow has expired. Redirect the user to the verification flow init endpoint to initialize a new verification flow.'
+      )
+      expect(response.body.error.details.redirect_to).to.eq(
+        'http://localhost:4455/self-service/verification/browser'
+      )
+    })
+})
+
+Cypress.Commands.add('verificationBrowser', ({ email, returnTo }) => {
+  let url = APP_URL + '/self-service/verification/browser'
+  if (returnTo) {
+    url += '?return_to=' + returnTo
+  }
+  cy.request({ url })
+    .then(({ body }) => {
+      const form = body.ui
+      return cy.request({
+        method: form.method,
+        body: mergeFields(form, { email, method: 'link' }),
+        url: form.action
+      })
+    })
+    .then(({ body }) => {
+      expect(body.state).to.contain('sent_email')
+    })
+})
+
 Cypress.Commands.add(
   'registerOidc',
   ({