Repository: ory/kratos
Pull request #2496 (2022-05-30 15:15:38)

fix: return browser to 'return_to' when logging in without registered account using oidc. 
When handling the callback from the oidc provider and starting a new registration flow to sign up the user we create a new temp request that contains the initial request parameters from the login flow.

## Related issue(s)

Proposal to close https://github.com/ory/kratos/issues/2444.

## Checklist

- [x] I have read the [contributing guidelines](../blob/master/CONTRIBUTING.md).
- [x] I have referenced an issue containing the design document if my change
      introduces a new feature.
- [x] I am following the
      [contributing code guidelines](../blob/master/CONTRIBUTING.md#contributing-code).
- [x] I have read the [security policy](../security/policy).
- [x] I confirm that this pull request does not address a security
      vulnerability. If this pull request addresses a security. vulnerability, I
      confirm that I got green light (please contact
      [security@ory.sh](mailto:security@ory.sh)) from the maintainers to push
      the changes.
- [x] I have added tests that prove my fix is effective or that my feature
      works.
- [x] I have added or changed [the documentation](https://github.com/ory/docs).

## Further Comments

CLAassistant: [![CLA assistant check](https://cla-assistant.io/pull/badge/not_signed)](https://cla-assistant.io/ory/kratos?pullRequest=2496) <br/>Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our [Contributor License Agreement](https://cla-assistant.io/ory/kratos?pullRequest=2496) before we can accept your contribution.<br/><sub>You have signed the CLA already but the status is still pending? Let us [recheck](https://cla-assistant.io/check/ory/kratos?pullRequest=2496) it.</sub>
codecov[bot]: # [Codecov](https://codecov.io/gh/ory/kratos/pull/2496?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ory) Report
> Merging [#2496](https://codecov.io/gh/ory/kratos/pull/2496?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ory) (f344da7) into [master](https://codecov.io/gh/ory/kratos/commit/12ceae005749c5dd01959720925418d643f13070?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ory) (12ceae0) will **decrease** coverage by `0.01%`.
> The diff coverage is `100.00%`.

```diff
@@            Coverage Diff             @@
##           master    #2496      +/-   ##
==========================================
- Coverage   76.65%   76.63%   -0.02%     
==========================================
  Files         317      317              
  Lines       17636    17640       +4     
==========================================
  Hits        13519    13519              
- Misses       3182     3184       +2     
- Partials      935      937       +2     
```


| [Impacted Files](https://codecov.io/gh/ory/kratos/pull/2496?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ory) | Coverage Δ | |
|---|---|---|
| [selfservice/strategy/oidc/strategy\_login.go](https://codecov.io/gh/ory/kratos/pull/2496/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ory#diff-c2VsZnNlcnZpY2Uvc3RyYXRlZ3kvb2lkYy9zdHJhdGVneV9sb2dpbi5nbw==) | `69.33% <100.00%> (+1.72%)` | :arrow_up: |
| [persistence/sql/persister\_courier.go](https://codecov.io/gh/ory/kratos/pull/2496/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ory#diff-cGVyc2lzdGVuY2Uvc3FsL3BlcnNpc3Rlcl9jb3VyaWVyLmdv) | `89.70% <0.00%> (-2.95%)` | :arrow_down: |
| [persistence/sql/persister.go](https://codecov.io/gh/ory/kratos/pull/2496/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ory#diff-cGVyc2lzdGVuY2Uvc3FsL3BlcnNpc3Rlci5nbw==) | `76.28% <0.00%> (-2.07%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/ory/kratos/pull/2496?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ory).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ory)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/ory/kratos/pull/2496?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ory). Last update [0290986...f344da7](https://codecov.io/gh/ory/kratos/pull/2496?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ory). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ory).

Urbansson: Nice work! 

LGTM!

Patch:
diff --git a/selfservice/flow/login/flow.go b/selfservice/flow/login/flow.go
index 13eb1ae86..ff631ddc0 100644
--- a/selfservice/flow/login/flow.go
+++ b/selfservice/flow/login/flow.go
@@ -192,6 +192,10 @@ func (f Flow) MarshalJSON() ([]byte, error) {
 }
 
 func (f *Flow) SetReturnTo() {
+	// Return to is already set, do not overwrite it.
+	if len(f.ReturnTo) > 0 {
+		return
+	}
 	if u, err := url.Parse(f.RequestURL); err == nil {
 		f.ReturnTo = u.Query().Get("return_to")
 	}
diff --git a/selfservice/flow/login/handler.go b/selfservice/flow/login/handler.go
index 33f937f74..9e400f1e9 100644
--- a/selfservice/flow/login/handler.go
+++ b/selfservice/flow/login/handler.go
@@ -86,12 +86,23 @@ func (h *Handler) RegisterAdminRoutes(admin *x.RouterAdmin) {
 	admin.GET(RouteSubmitFlow, x.RedirectToPublicRoute(h.d))
 }
 
-func (h *Handler) NewLoginFlow(w http.ResponseWriter, r *http.Request, ft flow.Type) (*Flow, error) {
+type FlowOption func(f *Flow)
+
+func WithFlowReturnTo(returnTo string) FlowOption {
+	return func(f *Flow) {
+		f.ReturnTo = returnTo
+	}
+}
+
+func (h *Handler) NewLoginFlow(w http.ResponseWriter, r *http.Request, ft flow.Type, opts ...FlowOption) (*Flow, error) {
 	conf := h.d.Config(r.Context())
 	f, err := NewFlow(conf, conf.SelfServiceFlowLoginRequestLifespan(), h.d.GenerateCSRFToken(r), r, ft)
 	if err != nil {
 		return nil, err
 	}
+	for _, o := range opts {
+		o(f)
+	}
 
 	if f.RequestedAAL == "" {
 		f.RequestedAAL = identity.AuthenticatorAssuranceLevel1
diff --git a/selfservice/flow/login/hook.go b/selfservice/flow/login/hook.go
index 53bf0c37d..8ecf6eae2 100644
--- a/selfservice/flow/login/hook.go
+++ b/selfservice/flow/login/hook.go
@@ -82,6 +82,7 @@ func (e *HookExecutor) PostLoginHook(w http.ResponseWriter, r *http.Request, a *
 	// Verify the redirect URL before we do any other processing.
 	c := e.d.Config(r.Context())
 	returnTo, err := x.SecureRedirectTo(r, c.SelfServiceBrowserDefaultReturnTo(),
+		x.SecureRedirectReturnTo(a.ReturnTo),
 		x.SecureRedirectUseSourceURL(a.RequestURL),
 		x.SecureRedirectAllowURLs(c.SelfServiceBrowserAllowedReturnToDomains()),
 		x.SecureRedirectAllowSelfServiceURLs(c.SelfPublicURL()),
diff --git a/selfservice/flow/recovery/error.go b/selfservice/flow/recovery/error.go
index a560fc408..c39b960dd 100644
--- a/selfservice/flow/recovery/error.go
+++ b/selfservice/flow/recovery/error.go
@@ -95,6 +95,7 @@ func (s *ErrorHandler) WriteFlowError(
 		return
 	}
 
+	f.UI.ResetMessages()
 	if err := f.UI.ParseError(group, err); err != nil {
 		s.forward(w, r, f, err)
 		return
diff --git a/selfservice/flow/recovery/flow.go b/selfservice/flow/recovery/flow.go
index 48d8f0de1..5842708fa 100644
--- a/selfservice/flow/recovery/flow.go
+++ b/selfservice/flow/recovery/flow.go
@@ -191,6 +191,10 @@ func (f Flow) MarshalJSON() ([]byte, error) {
 }
 
 func (f *Flow) SetReturnTo() {
+	// Return to is already set, do not overwrite it.
+	if len(f.ReturnTo) > 0 {
+		return
+	}
 	if u, err := url.Parse(f.RequestURL); err == nil {
 		f.ReturnTo = u.Query().Get("return_to")
 	}
diff --git a/selfservice/flow/registration/flow.go b/selfservice/flow/registration/flow.go
index b2d21cad4..da7cff04d 100644
--- a/selfservice/flow/registration/flow.go
+++ b/selfservice/flow/registration/flow.go
@@ -159,6 +159,10 @@ func (f Flow) MarshalJSON() ([]byte, error) {
 }
 
 func (f *Flow) SetReturnTo() {
+	// Return to is already set, do not overwrite it.
+	if len(f.ReturnTo) > 0 {
+		return
+	}
 	if u, err := url.Parse(f.RequestURL); err == nil {
 		f.ReturnTo = u.Query().Get("return_to")
 	}
diff --git a/selfservice/flow/registration/handler.go b/selfservice/flow/registration/handler.go
index 0f22c21f2..490d6ea98 100644
--- a/selfservice/flow/registration/handler.go
+++ b/selfservice/flow/registration/handler.go
@@ -91,8 +91,15 @@ func (h *Handler) RegisterAdminRoutes(admin *x.RouterAdmin) {
 	admin.GET(RouteSubmitFlow, x.RedirectToPublicRoute(h.d))
 }
 
-func (h *Handler) NewRegistrationFlow(w http.ResponseWriter, r *http.Request, ft flow.Type) (*Flow, error) {
+type FlowOption func(f *Flow)
 
+func WithFlowReturnTo(returnTo string) FlowOption {
+	return func(f *Flow) {
+		f.ReturnTo = returnTo
+	}
+}
+
+func (h *Handler) NewRegistrationFlow(w http.ResponseWriter, r *http.Request, ft flow.Type, opts ...FlowOption) (*Flow, error) {
 	if !h.d.Config(r.Context()).SelfServiceFlowRegistrationEnabled() {
 		return nil, errors.WithStack(ErrRegistrationDisabled)
 	}
@@ -101,6 +108,9 @@ func (h *Handler) NewRegistrationFlow(w http.ResponseWriter, r *http.Request, ft
 	if err != nil {
 		return nil, err
 	}
+	for _, o := range opts {
+		o(f)
+	}
 
 	for _, s := range h.d.RegistrationStrategies(r.Context()) {
 		if err := s.PopulateRegistrationMethod(r, f); err != nil {
diff --git a/selfservice/flow/registration/hook.go b/selfservice/flow/registration/hook.go
index 2ab457c76..e311ae187 100644
--- a/selfservice/flow/registration/hook.go
+++ b/selfservice/flow/registration/hook.go
@@ -128,6 +128,7 @@ func (e *HookExecutor) PostRegistrationHook(w http.ResponseWriter, r *http.Reque
 	// Verify the redirect URL before we do any other processing.
 	c := e.d.Config(r.Context())
 	returnTo, err := x.SecureRedirectTo(r, c.SelfServiceBrowserDefaultReturnTo(),
+		x.SecureRedirectReturnTo(a.ReturnTo),
 		x.SecureRedirectUseSourceURL(a.RequestURL),
 		x.SecureRedirectAllowURLs(c.SelfServiceBrowserAllowedReturnToDomains()),
 		x.SecureRedirectAllowSelfServiceURLs(c.SelfPublicURL()),
diff --git a/selfservice/flow/settings/flow.go b/selfservice/flow/settings/flow.go
index 7a3b783f6..f3beff8b1 100644
--- a/selfservice/flow/settings/flow.go
+++ b/selfservice/flow/settings/flow.go
@@ -203,6 +203,10 @@ func (f Flow) MarshalJSON() ([]byte, error) {
 }
 
 func (f *Flow) SetReturnTo() {
+	// Return to is already set, do not overwrite it.
+	if len(f.ReturnTo) > 0 {
+		return
+	}
 	if u, err := url.Parse(f.RequestURL); err == nil {
 		f.ReturnTo = u.Query().Get("return_to")
 	}
diff --git a/selfservice/strategy/oidc/strategy_login.go b/selfservice/strategy/oidc/strategy_login.go
index 8ddc75422..54b98e143 100644
--- a/selfservice/strategy/oidc/strategy_login.go
+++ b/selfservice/strategy/oidc/strategy_login.go
@@ -85,11 +85,16 @@ func (s *Strategy) processLogin(w http.ResponseWriter, r *http.Request, a *login
 			// not need additional consent/login.
 
 			// This is kinda hacky but the only way to ensure seamless login/registration flows when using OIDC.
-
 			s.d.Logger().WithField("provider", provider.Config().ID).WithField("subject", claims.Subject).Debug("Received successful OpenID Connect callback but user is not registered. Re-initializing registration flow now.")
 
+			// If return_to was set before, we need to preserve it.
+			var opts []registration.FlowOption
+			if len(a.ReturnTo) > 0 {
+				opts = append(opts, registration.WithFlowReturnTo(a.ReturnTo))
+			}
+
 			// This flow only works for browsers anyways.
-			aa, err := s.d.RegistrationHandler().NewRegistrationFlow(w, r, flow.TypeBrowser)
+			aa, err := s.d.RegistrationHandler().NewRegistrationFlow(w, r, flow.TypeBrowser, opts...)
 			if err != nil {
 				return nil, s.handleError(w, r, a, provider.Config().ID, nil, err)
 			}
diff --git a/selfservice/strategy/oidc/strategy_registration.go b/selfservice/strategy/oidc/strategy_registration.go
index 9df483827..18673a76a 100644
--- a/selfservice/strategy/oidc/strategy_registration.go
+++ b/selfservice/strategy/oidc/strategy_registration.go
@@ -177,8 +177,14 @@ func (s *Strategy) processRegistration(w http.ResponseWriter, r *http.Request, a
 			WithField("subject", claims.Subject).
 			Debug("Received successful OpenID Connect callback but user is already registered. Re-initializing login flow now.")
 
+		// If return_to was set before, we need to preserve it.
+		var opts []login.FlowOption
+		if len(a.ReturnTo) > 0 {
+			opts = append(opts, login.WithFlowReturnTo(a.ReturnTo))
+		}
+
 		// This endpoint only handles browser flow at the moment.
-		ar, err := s.d.LoginHandler().NewLoginFlow(w, r, flow.TypeBrowser)
+		ar, err := s.d.LoginHandler().NewLoginFlow(w, r, flow.TypeBrowser, opts...)
 		if err != nil {
 			return nil, s.handleError(w, r, a, provider.Config().ID, nil, err)
 		}
diff --git a/x/http_secure_redirect.go b/x/http_secure_redirect.go
index 63fa88ddc..6b46a9d7e 100644
--- a/x/http_secure_redirect.go
+++ b/x/http_secure_redirect.go
@@ -20,6 +20,7 @@ import (
 type secureRedirectOptions struct {
 	allowlist       []url.URL
 	defaultReturnTo *url.URL
+	returnTo        string
 	sourceURL       string
 }
 
@@ -40,6 +41,13 @@ func SecureRedirectUseSourceURL(source string) SecureRedirectOption {
 	}
 }
 
+// SecureRedirectReturnTo uses the provided URL to redirect the user to it.
+func SecureRedirectReturnTo(returnTo string) SecureRedirectOption {
+	return func(o *secureRedirectOptions) {
+		o.returnTo = returnTo
+	}
+}
+
 // SecureRedirectAllowSelfServiceURLs allows the caller to define `?return_to=` values
 // which contain the server's URL and `/self-service` path prefix. Useful for redirecting
 // to the login endpoint, for example.
@@ -81,14 +89,18 @@ func SecureRedirectTo(r *http.Request, defaultReturnTo *url.URL, opts ...SecureR
 	if o.sourceURL != "" {
 		source, err = url.ParseRequestURI(o.sourceURL)
 		if err != nil {
-			return nil, herodot.ErrInternalServerError.WithWrap(err).WithReasonf("Unable to parse the original request URL: %s", err)
+			return nil, errors.WithStack(herodot.ErrInternalServerError.WithWrap(err).WithReasonf("Unable to parse the original request URL: %s", err))
 		}
 	}
 
-	if len(source.Query().Get("return_to")) == 0 {
+	rawReturnTo := stringsx.Coalesce(o.returnTo, source.Query().Get("return_to"))
+	if rawReturnTo == "" {
 		return o.defaultReturnTo, nil
-	} else if returnTo, err = url.Parse(source.Query().Get("return_to")); err != nil {
-		return nil, herodot.ErrInternalServerError.WithWrap(err).WithReasonf("Unable to parse the return_to query parameter as an URL: %s", err)
+	}
+
+	returnTo, err = url.Parse(rawReturnTo)
+	if err != nil {
+		return nil, errors.WithStack(herodot.ErrInternalServerError.WithWrap(err).WithReasonf("Unable to parse the return_to query parameter as an URL: %s", err))
 	}
 
 	returnTo.Host = stringsx.Coalesce(returnTo.Host, o.defaultReturnTo.Host)


Test patch:
diff --git a/selfservice/flow/login/flow_test.go b/selfservice/flow/login/flow_test.go
index b8c549e85..d82a2b508 100644
--- a/selfservice/flow/login/flow_test.go
+++ b/selfservice/flow/login/flow_test.go
@@ -158,3 +158,13 @@ func TestFlowEncodeJSON(t *testing.T) {
 	assert.EqualValues(t, "/bar", gjson.Get(jsonx.TestMarshalJSONString(t, &login.Flow{RequestURL: "https://foo.bar?return_to=/bar"}), "return_to").String())
 	assert.EqualValues(t, "/bar", gjson.Get(jsonx.TestMarshalJSONString(t, login.Flow{RequestURL: "https://foo.bar?return_to=/bar"}), "return_to").String())
 }
+
+func TestFlowDontOverrideReturnTo(t *testing.T) {
+	f := &login.Flow{ReturnTo: "/foo"}
+	f.SetReturnTo()
+	assert.Equal(t, "/foo", f.ReturnTo)
+
+	f = &login.Flow{RequestURL: "https://foo.bar?return_to=/bar"}
+	f.SetReturnTo()
+	assert.Equal(t, "/bar", f.ReturnTo)
+}
diff --git a/selfservice/flow/recovery/flow_test.go b/selfservice/flow/recovery/flow_test.go
index 3510a1937..ba08845fb 100644
--- a/selfservice/flow/recovery/flow_test.go
+++ b/selfservice/flow/recovery/flow_test.go
@@ -101,3 +101,13 @@ func TestFromOldFlow(t *testing.T) {
 		})
 	}
 }
+
+func TestFlowDontOverrideReturnTo(t *testing.T) {
+	f := &recovery.Flow{ReturnTo: "/foo"}
+	f.SetReturnTo()
+	assert.Equal(t, "/foo", f.ReturnTo)
+
+	f = &recovery.Flow{RequestURL: "https://foo.bar?return_to=/bar"}
+	f.SetReturnTo()
+	assert.Equal(t, "/bar", f.ReturnTo)
+}
diff --git a/selfservice/flow/registration/flow_test.go b/selfservice/flow/registration/flow_test.go
index c9970d6fc..acda1c8ee 100644
--- a/selfservice/flow/registration/flow_test.go
+++ b/selfservice/flow/registration/flow_test.go
@@ -125,3 +125,13 @@ func TestFlowEncodeJSON(t *testing.T) {
 	assert.EqualValues(t, "/bar", gjson.Get(jsonx.TestMarshalJSONString(t, &registration.Flow{RequestURL: "https://foo.bar?return_to=/bar"}), "return_to").String())
 	assert.EqualValues(t, "/bar", gjson.Get(jsonx.TestMarshalJSONString(t, registration.Flow{RequestURL: "https://foo.bar?return_to=/bar"}), "return_to").String())
 }
+
+func TestFlowDontOverrideReturnTo(t *testing.T) {
+	f := &registration.Flow{ReturnTo: "/foo"}
+	f.SetReturnTo()
+	assert.Equal(t, "/foo", f.ReturnTo)
+
+	f = &registration.Flow{RequestURL: "https://foo.bar?return_to=/bar"}
+	f.SetReturnTo()
+	assert.Equal(t, "/bar", f.ReturnTo)
+}
diff --git a/selfservice/flow/settings/flow_test.go b/selfservice/flow/settings/flow_test.go
index a5dd468d8..8935d0ca2 100644
--- a/selfservice/flow/settings/flow_test.go
+++ b/selfservice/flow/settings/flow_test.go
@@ -166,3 +166,13 @@ func TestFlowEncodeJSON(t *testing.T) {
 	assert.EqualValues(t, "/bar", gjson.Get(jsonx.TestMarshalJSONString(t, &settings.Flow{RequestURL: "https://foo.bar?return_to=/bar"}), "return_to").String())
 	assert.EqualValues(t, "/bar", gjson.Get(jsonx.TestMarshalJSONString(t, settings.Flow{RequestURL: "https://foo.bar?return_to=/bar"}), "return_to").String())
 }
+
+func TestFlowDontOverrideReturnTo(t *testing.T) {
+	f := &settings.Flow{ReturnTo: "/foo"}
+	f.SetReturnTo()
+	assert.Equal(t, "/foo", f.ReturnTo)
+
+	f = &settings.Flow{RequestURL: "https://foo.bar?return_to=/bar"}
+	f.SetReturnTo()
+	assert.Equal(t, "/bar", f.ReturnTo)
+}
diff --git a/selfservice/strategy/oidc/strategy_test.go b/selfservice/strategy/oidc/strategy_test.go
index 405f2eb47..b21e96289 100644
--- a/selfservice/strategy/oidc/strategy_test.go
+++ b/selfservice/strategy/oidc/strategy_test.go
@@ -58,9 +58,9 @@ func TestStrategy(t *testing.T) {
 		claims    idTokenClaims
 		scope     []string
 	)
-
 	remoteAdmin, remotePublic, hydraIntegrationTSURL := newHydra(t, &subject, &claims, &scope)
 	returnTS := newReturnTs(t, reg)
+	conf.MustSet(config.ViperKeyURLsAllowedReturnToDomains, []string{returnTS.URL})
 	uiTS := newUI(t, reg)
 	errTS := testhelpers.NewErrorTestServer(t, reg)
 	routerP := x.NewRouterPublic()
@@ -368,6 +368,20 @@ func TestStrategy(t *testing.T) {
 		})
 	})
 
+	t.Run("case=login without registered account with return_to", func(t *testing.T) {
+		subject = "login-without-register-return-to@ory.sh"
+		scope = []string{"openid"}
+		returnTo := "/foo"
+
+		t.Run("case=should pass login", func(t *testing.T) {
+			r := newLoginFlow(t, fmt.Sprintf("%s?return_to=%s", returnTS.URL, returnTo), time.Minute)
+			action := afv(t, r.ID, "valid")
+			res, body := makeRequest(t, "valid", action, url.Values{})
+			assert.True(t, strings.HasSuffix(res.Request.URL.String(), returnTo))
+			ai(t, res, body)
+		})
+	})
+
 	t.Run("case=register and register again but login", func(t *testing.T) {
 		subject = "register-twice@ory.sh"
 		scope = []string{"openid"}
@@ -385,6 +399,15 @@ func TestStrategy(t *testing.T) {
 			res, body := makeRequest(t, "valid", action, url.Values{})
 			ai(t, res, body)
 		})
+
+		t.Run("case=should pass third time registration with return to", func(t *testing.T) {
+			returnTo := "/foo"
+			r := newLoginFlow(t, fmt.Sprintf("%s?return_to=%s", returnTS.URL, returnTo), time.Minute)
+			action := afv(t, r.ID, "valid")
+			res, body := makeRequest(t, "valid", action, url.Values{})
+			assert.True(t, strings.HasSuffix(res.Request.URL.String(), returnTo))
+			ai(t, res, body)
+		})
 	})
 
 	t.Run("case=register, merge, and complete data", func(t *testing.T) {
diff --git a/test/e2e/cypress/integration/profiles/recovery/recovery/errors.spec.ts b/test/e2e/cypress/integration/profiles/recovery/recovery/errors.spec.ts
index 8678e44e5..0d98f2563 100644
--- a/test/e2e/cypress/integration/profiles/recovery/recovery/errors.spec.ts
+++ b/test/e2e/cypress/integration/profiles/recovery/recovery/errors.spec.ts
@@ -114,6 +114,22 @@ context('Account Recovery Errors', () => {
         cy.get('[name="method"][value="link"]').should('exist')
       })
 
+      it('should cause non-repeating form errors after submitting empty form twice. see: #2512', () => {
+        cy.visit(recovery)
+        cy.get('button[value="link"]').click()
+        cy.location('pathname').should('eq', '/recovery')
+
+        cy.get('button[value="link"]').click()
+        cy.get('[data-testid="ui/message/4000002"]').should(
+          'contain.text',
+          'Property email is missing.'
+        )
+        cy.get('form')
+          .find('[data-testid="ui/message/4000002"]')
+          .should('have.length', 1)
+        cy.get('[name="method"][value="link"]').should('exist')
+      })
+
       it('is unable to recover the email address if the code is expired', () => {
         cy.shortLinkLifespan()
         const identity = gen.identityWithWebsite()