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
[SECURITY] Bad UX for lost password / password reset #5203
Comments
Agreed. The entire password reset function is a bit of a pain from a user point of view |
I would not redirect back to the change password form. I personally hate to see the form again, after submitting. It somehow feels like I did something wrong. |
If the The only issue is that it wouldn't always work (e.g. this form is used when the user's password has expired, which is set whenever the user logs in and their password has expired, so you can't take them back to the login form as that's pointless too. |
I'm not attached to showing them the form again, the important thing to me is that we give users a success message, not blindly throw them to some location they may or may not want to go to with no indication of whether or not the password change was successful. @dhensby do you have any thoughts about SemVer on this one? I'd like to fix this in 3.x but is this a broken enough behaviour that we can stop redirecting people and not consider it a breaking change? I guess it could be an opt-in fix with a config variable but seems convoluted. |
So I whipped up a change that redirects users back to the form (for now, because it's easy) and displays a confirmation message, but also shows a link to the BackURL if available, which I thought might be a nice compromise. But then I wondered if this could be a security vulnerability, because the BackURL is posted in via the form so that could be manipulated and maybe cause this page to display some undesirable HTML instead of a valid URL? |
As long as you validate that it's a site url, and it's properly encoded, then it's fine. |
@jonom you could make the form redirect to that URL after x seconds (if it is valid). Validate URLs by doing |
In terms of SemVer: I think the best process here is to use deduction.. is it a patch: No So, it's definitely not the 3.3 branch, it might be 3 branch if we think that this doesn't "break" an API I think it's reasonable that a unit test would assert that this step moves a user on to the BackURL, so I think, unfortunately, this is a 4.x change. What we could do is add this to 3.x with a config option to turn it on and then just enable it in 4 If you PR against 4, please add it to the changelog/upgrade notes |
There's already a check against Director::is_site_url() but I don't think that's sufficient at all... it falls back on this function: public static function is_relative_url($url) {
return (!Director::is_absolute_url($url));
} So it looks to me like you can pass any string at all to Yeah I had a feeling it would be a 4 thing. I'm not sure it's worth doing an opt-in change for 3. @Firesphere is this overlapping with work you're already doing? I might leave it to you if it is so we're not double handling this issue. have left some thoughts in your dev list post. |
I guess that comes down to casting in the template, then? |
Yeah I guess I would just have to make sure it ends up getting escaped properly and still works properly as a link. Probably a way easier problem than I've made it out to be in my head. I'm going to pause exploring this until I see what @Firesphere comes up with. @dhensby feel free to close this issue if you see it as more of a feature request than a bug. |
You're free to comment and/or help. I've created a separate repository, with a framework version minus security (almost). My current implementation, is showing a text saying password is changed and the user is logged in, and can follow a link to go back wherever they came from (backurl from session) Also done massive restructuring/refactoring, so feedback on there is very welcome 👍 I added ZenHub to the Security repository. I don't know if it'll work out, I just wanted to try it. |
@Firesphere is that going through an RFC process? Does "Security" include If security is pulled out, wouldn't the whole admin have to be pulled out too? |
Pulling Security out doesn't quite sounds feasible so interested to see how it turns out. Does seem like RFC territory, but I have to say based on RFC-2 that if I'd put my energy in to building a proof of concept I could demonstrate instead of writing and defending a theoretical proposal it would probably be all done ages ago. So it's probably a good idea to do some experimenting first but then maybe write an RFC once you're confident about the general form your solution will take? |
@dhensby I am not pulling it out really. I separated it to make it easier to work on and keep track of issues etc. not to be a separate module. Once done, it can be put back in quite easily. |
ah, ok |
The password reset form has substantially changed since we introduced the MFA module which now ships by default with the installer. I've recorded what it looks like: https://youtu.be/V1woJiKHaWk I guess we could have a toast notification when you successfully access the CMS after changing your password. But otherwise the flow looks pretty good to me. @silverstripeux Maybe have a glance at the video and make call on if it's something you would like improved. |
Please, no toast notification, they're useless to the end user, it doesn't help them at all, because it's a tiny notification in the corner of the screen, a proper notification is a lot more useful. |
For the most part the experience seems good. Yup, missing some sort of notification, but I would think a toast notification is ideal in this situation. The user already knows its worked as they have entered the CMS interface. Toast notifications are used when things happen as the user would expect, e.g. continue, everything is good. I think this is a good example of what they have been designed for IMHO. |
I watched the video (thanks @maxime-rainville), it certainly looks prettier than before but I'm not sure it anything has changed apart from a re-skin? No idea if my thoughts on What happens if you were trying to access a restricted page rather than the CMS? Do you still get redirected there immediately? Depending on the design of the site I'd still argue that users might be left unsure whether the password change worked in some circumstances (particularly since a toast notification not really an option in that context out-of-the-box), so I think my original suggestion of showing a success message with a link you can click to go to the location you were trying to log in to (rather than redirecting immediately) is still a decent idea. |
If you're trying to access a restricted page, it seems you still get redirected to that page after resetting your password. That's what it looks like. https://youtu.be/FjgovT_iwX8 |
@maxime-rainville it's not terrible but if we kept the redirect behaviour I think it would be nice if the password reset form at least told you what was going to happen. Like 'Please enter a new password' could become 'Enter a new password, then we'll take you straight to /admin.' BTW should that password reset email have some more meat to it? Here's an example from Slack: @sminnee mentioned the design aspect, would be great to see that cleaned up. |
Being lazy I didn't read the all the threads... having a success message prior to moving into the CMS also works. |
I left a comment very relevant to this issue here: #3757 (comment). 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 |
That literally gives away unwanted information. I would personally not like it if I'd see something like that. It would work for non-admin URLs, but for admin URLs, I would not like to see that. |
@Firesphere Can you clarify what you mean by unwanted information - do you mean that it's cryptic / unhelpful? I just think that if we keep the immediate redirect behaviour (no success message displayed to user) then we should inform the user that that is going to happen. My preferred UX which I outlined here (sorry for the fragmentation) would be to not immediately redirect the user and instead display a success message on a new action like Example: |
It gives information that's not wanted, e.g. giving away the next endpoint |
Is it sensitive information? I guess I was focused on BackURL and assumed we'd just be displaying the URL they were trying to access in the first place (i.e. already known to them). But I suppose that wouldn't always be the case. Maybe it could just say something like "Enter a new password, then we'll take you straight to your destination" instead (if we stick with the redirect behaviour). |
Yeah so putting We can say 'Enter a new password, then we'll take you straight to the CMS.' (or "the admin area" if the CMS module isn't installed), then we're avoiding that problem. |
|
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
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. |
Upon successful reset of password through lost password email,
ChangePasswordForm
will redirect you to whatever location happens to be stored inSession::get('BackURL')
. This may be desired in some cases but I think it leads to a a bad user experience on two countsThink 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: #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:
BackURL
,default_reset_password_dest
, anddefault_login_dest
are persisted and respected in this workflow.The text was updated successfully, but these errors were encountered: