silverstripe-framework: [SECURITY] Bad UX for lost password / password reset
Upon successful reset of password through lost password email, ChangePasswordForm will redirect you to whatever location happens to be stored in Session::get('BackURL'). This may be desired in some cases but I think it leads to a a bad user experience on two counts
- Unpredictability about what page the user will end up on
- No feedback / success message for the user may leave them in doubt as to whether the password reset was successful
Think it would be better to ignore the BackURL and instead redirect people back to the change password form every time with a success message. Also think it’s a bad idea to ever store a BackURL in Session with no context. If a user starts a process that causes this value to be set but doesn’t finish it, then that value could be picked up much later and create an unpredictable/undesirable result, in the same process or a completely unrelated one.
Related: https://github.com/silverstripe/silverstripe-framework/issues/3757 (As of SS4, the result of resetting your password and successfully logging in is an offer to log in as someone else, with no way to get back to the page you were trying to log in to.)
Edit - improvements being pitched:
- Upon successful password change, drop instant redirect and replace with confirmation message step which includes a link to continue to the redirect destination.
- Alternative idea: Redirect + Toast notification. Could work for /admin destinations but not other protected URLs
- Alternative idea: Warning a user in the form that they’re going to be redirected after the change. This is better than nothing but not as good as an explicit success message.
- Ensure that
BackURL,default_reset_password_dest, anddefault_login_destare persisted and respected in this workflow.
About this issue
- Original URL
- State: open
- Created 8 years ago
- Comments: 32 (32 by maintainers)
Yeah I was assuming that the destination (could be /admin or any other restricted page or URL) was already known to the user because typically you end up at a login screen because you’ve tried to access that URL. E.g. if you’re trying to access the CMS, the workflow would be
user tries to access /admin->user is redirected to /Security/login.I would rather have a ‘friendly name’ than a URL (e.g. “the CMS” as you suggested) but a restricted URL on a SilverStripe app could be anything and isn’t necessarily going to be easily resolvable to a nice name as is the case with the CMS. Also, if you did give away the name then presumably that’s a form of the same security vulnerability being discussed - namely revealing that the endpoint does exist (and by the way, here’s a friendly title for it…)
Ultimately for simplicity probably just saying something vague like “we’ll take you straight to the URL you were trying to access” (preparing users to expect a redirect) would be fine. It’s a moot point though if we do the better fix which (as @Firesphere originally suggested) is to ditch the redirect and instead display a confirmation message with a link to whatever the redirect destination would have been.
I left a comment very relevant to this issue here: https://github.com/silverstripe/silverstripe-framework/issues/3757#issuecomment-683987939. TLDR; The result of resetting your password and successfully logging in is an offer to log in as someone else, with no way to get back to the page you were trying to log in to, or the
default_login_destas set by the developer. That sucks a lot.