-
-
Notifications
You must be signed in to change notification settings - Fork 964
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: settings should persist return_to
after required mfa login flow
#3263
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3263 +/- ##
==========================================
+ Coverage 77.82% 77.84% +0.02%
==========================================
Files 325 325
Lines 21086 21100 +14
==========================================
+ Hits 16410 16426 +16
+ Misses 3446 3444 -2
Partials 1230 1230
|
31d0c8c
to
c42963f
Compare
f0d39f2
to
2c3a277
Compare
return_to
when redirecting to aal2return_to
after required mfa login flow
test/e2e/cypress/integration/profiles/recovery/return-to/success.spec.ts
Outdated
Show resolved
Hide resolved
7ffa1e1
to
0edd60a
Compare
a801e28
to
a2af146
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Just some comments
68d0539
to
e9d8de0
Compare
…flow return_to unless it contains a return_to parameter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Related issue(s)
fixes ory/network#222
fixes #2832
This PR fixes the incorrect behavior after completing the required login (MFA) required by the settings flow. Users would incorrectly be redirected to the
/welcome
page or default UI route after submitting a successful MFA credential instead of being redirected back to the original settings flow.possible breaking change
Observed through the E2E tests, if the host from which the request to Kratos is proxied and the
serve.public.base_url
is set to point to the proxy URL, then Kratos will use the current host which is the Kratos URL. MFA required flows such as settings will attempt to redirect back to the Kratos Host instead of the proxy URL which is not automatically on the allow list for browser redirects - especially since ourserve.public.base_url
now points to the proxy URL instead of Kratos. This will cause the flow to fail and the user to be redirected to the/error
page.To avoid this, allow the proxy to forward the host through the
X-Forwarded-Host
header. Kratos will then correctly redirect back.Before Merge:
navigate to settings immediately after password login with MFA required Identity.We have such a test here https://github.com/ory/kratos/blob/master/test/e2e/cypress/integration/profiles/mfa/totp.spec.ts#L48Checklist
introduces a new feature.
contributing code guidelines.
vulnerability. If this pull request addresses a security vulnerability, I
confirm that I got the approval (please contact
security@ory.sh) from the maintainers to push
the changes.
works.
Further Comments