laravel / framework

The Laravel Framework.
https://laravel.com
MIT License
32.38k stars 10.97k forks source link

Password reset e-mail missing e-mail in URL #15733

Closed rafaelrenanpacheco closed 8 years ago

rafaelrenanpacheco commented 8 years ago

When sending the password reset e-mail in Laravel 5.3, the reset link have a token, but doesn't have the user e-mail. This way, the reset form will not load the user e-mail. In 5.2, the reset link had the user e-mail.

Without the user e-mail in the URL, Illuminate\Foundation\Auth\ResetsPasswords.php will send a null e-mail in showResetForm, because $request->email will evaluate to null. Then, the reset form provided by the framework from Illuminate\Auth\Console\stubs\make\views\auth\passwords\email.stub will show a blank e-mail in the following input:

<input id="email" type="email" class="form-control" name="email" value="{{ old('email') }}" required>

Digging up who missed sending the e-mail in the URL, we can found a Illuminate\Auth\Notifications\ResetPassword.php which does the following action:

->action('Reset Password', url('password/reset', $this->token))

As you can see, by default Laravel does not add the e-mail in the request URL. Since this trait usually is overwritten, we can add the e-mail to the URL to make things working again. The issue here is that, by default, Laravel is missing this e-mail in the request url.

Steps To Reproduce:

Send a reset e-mail link and open the link provided in the e-mail.

zaknesler commented 8 years ago

This may have been on purpose. Not completely sure, though.

themsaid commented 8 years ago

In 5.3 you get asked for the email while providing your new password.

rafaelrenanpacheco commented 8 years ago

Which does not make sense. If I open a reset url, the form should be opened with the user's e-mail.

Otherwise, why there are these pieces of codes in the framework:

Illuminate\Foundation\Auth\ResetsPasswords.php

public function showResetForm(Request $request, $token = null)
{
    return view('auth.passwords.reset')->with(
        ['token' => $token, 'email' => $request->email]
    );
}

Illuminate\Auth\Console\stubs\make\views\auth\passwords\reset.stub

<input id="email" type="email" class="form-control" name="email" value="{{ $email or old('email') }}" required autofocus>

If the desired behavior in 5.3 is to force the user to input it's e-mail again, why the showResetForm and the view are trying to send and display it?

gogromat commented 7 years ago

Thank you @chekovsky, I have also just stumbled upon it. As I can guess $request->email was left from older 5.2 version. Question is why the reset password link/functionality have changed and why user has to type in the email address again (form is used elsewhere?).

rafaelrenanpacheco commented 7 years ago

Hi there!

Those lines are leftovers from 5.2, and they won't fix it in 5.3.

In 5.3 the best solution is to override it and show your own form with your own data, like the user's e-mail.

Best regards, Rafael Pacheco.

safydy commented 7 years ago

Hi there, Do we have update on this guys? (the mail field still remains in L5.4)

khalilst commented 7 years ago

Same issue here in Laravel 5.4

kjdion84 commented 7 years ago

@khalist It's been removed since 5.2. If it bothers you so much simply remove $email from the view file.

zmonteca commented 7 years ago

@kjdion84 does the form actually work if you do that?

kjdion84 commented 7 years ago

Yes.

RadBoris commented 7 years ago

@kjdion84 It doesn't seem to work if you are logging in the user with an email (or with a username, in fact). @retrieveByCredentials from the UserProvider interface cannot locate the user without an email

khalilst commented 7 years ago

@kjdion84 Your solution won't work, because the validator raises error for missing email field. In other hand there is no email field in the view and you want to delete nothing!

@zmonteca I solved the problem by adding hidden email field to view by locating the token in password_resets table.

Vrutin commented 7 years ago

@khalilst how are you locating the token since its now hashed in 5.4?

khalilst commented 7 years ago

@Vrutin My mistake. Actually I forgot how to solve this problem before. Email souldn't be a hidden field, it must be a simple input field. In fact I asked the user to enter his/her email in password reset form.

Vrutin commented 7 years ago

@khalilst No worries, I have that currently but it kind of doesn't make sense from the users' point of view that they get a password reset link but then having to enter email again.

khalilst commented 7 years ago

@Vrutin I know this is an UX issue and of course resolving a bug by adding another smaller bug. The best way to solve this problem, can be done by our dear friend @taylorotwell by removing required email validation from core controllers and maybe by removing token hashing. I hope this have been done in 5.5

ashwani23 commented 7 years ago

@khalilst, well the process has been kept same in Laravel 5.5 too. We still need to do this in a custom way

zmonteca commented 7 years ago

Agreed. It's terrible as is.

Mayonado commented 7 years ago

Did someone solve this problem? I am currently using laravel 5.4 with same issue.

devcircus commented 7 years ago

What's your particular issue? There have been several mentioned here.

Mayonado commented 7 years ago

return view('auth.passwords.reset')->with( ['token' => $token, 'email' => $request->email] );

The email here returns empty

Vrutin commented 7 years ago

@khalilst @Mayonado @zmonteca

I have fixed it by editing/hacking ResetPassword.php notification & reset.blade.php file. Now this does mean you are editing vendor file, so it's your choice to do this or not.

ResetPassword.php

The file can be found at (5.4):

../vendor/laravel/framework/src/Illuminate/Auth/Notifications/ResetPassword.php

Amend the toMail method:

From:

public function toMail($notifiable)
    {
        return (new MailMessage)
            ->line('You are receiving this email because we received a password reset request for your account.')
            ->action('Reset Password', url(config('app.url').route('password.reset', $this->token, false)))
            ->line('If you did not request a password reset, no further action is required.');
    }

To

public function toMail($notifiable)
    {
        return (new MailMessage)
            ->line('You are receiving this email because we received a password reset request for your account.')
            ->action('Reset Password', url(config('app.url') . route('password.reset', [$this->token, 'email=' . $notifiable->email], false)))
            ->line('If you did not request a password reset, no further action is required.');
    }

Notice the updated parameters argument, $notifiable resolves to User object hence you can access the email property:

[$this->token, 'email=' . $notifiable->email]

reset.blade.php

The file can be found at (5.4):

../resources/views/auth/passwords/reset.blade.php

The HTML may slightly differ here based on your CSS framework. In addition, you may want to show email field as read-only or make it hidden input. I prefer the user seeing the email they requested the password reset for.

From

<input type="text" id="email" name="email" class="input {{ $errors->has('email') ? ' is-danger' : '' }}" value="{{ old('email') }}">

To

<input type="text" id="email" name="email" class="input {{ $errors->has('email') ? ' is-danger' : '' }}" value="{{ $email }}" readonly>

Notice the updated value attribute, added the readonly attribute to stop user amending the email.

... value="{{ $email }}" readonly>

Hope this helps others.

ashwani23 commented 7 years ago

@Vrutin your work around this issue is appreciated. In your solution, you have added email in the password reset link (being shared in email)

This appears to be a bad way to handle the functionality as displaying email in the URL is not a suggested and adopted practice.

Vrutin commented 7 years ago

@ashwani23 What if we encrypt the email and then decrypt back. Would that be okay?

iranianpep commented 7 years ago

@Vrutin how does your route look like? What I did was to change the route to: reset/{token}/{email}

And changed toMail function body to include this: ->action('Reset Password', url('reset', [$this->token, $notifiable->email]))

Cheers,

Vrutin commented 7 years ago

@iranianpep I didn't change the route, as I am passing it as a query parameter and the default implementation uses $request->email.

Though your implementation would work well too 👍

I ended up encrypting the email using the encrypt() method and then used the decrypt() method to get back users email back.

iranianpep commented 7 years ago

@Vrutin because for some reason when I used your approach the reset link was like /reset/THE_GENERATED_TOKEN/email=example@example.com

and since it didn't find any route I got page not found error. Anyway, I'll use my own solution. But thanks for the encryption suggestion.

khalilst commented 7 years ago

Another simple approach:

  1. Store email in session or even simpler in cookie or LocalStorage.
  2. Reload it from previous storage to reset form.
  3. Delete the email key from the storage.

Majority of users reset their passwords instantly after requesting. Others should enter their emails again because they are very little percentage ;)

iranianpep commented 7 years ago

I don't think storing email in the session is a good idea because it is mainly for temporary sensitive data. Also if a website has got loads of visitors you don't wanna use the session for storing emails.

khalilst commented 7 years ago

@iranianpep Session is for temporary sensitive data?! Did u use flash in laravel? It stores simple messages in Session. BTW read this and consider item 3.

However as I said before, this problem would be solved from here Illuminate\Foundation\Auth\ResetsPasswords by @taylorotwell, @JeffreyWay , but apparently they are too busy

iranianpep commented 7 years ago

As I said, it is mainly for temporary sensitive data e.g. logged in user otherwise you're free to overwhelm it with a bunch of emails! Also wikipedia is not recommended as a solid programming reference.

khalilst commented 7 years ago

@iranianpep So you didn't use flash. Or you're using session mainly in a wrong way. And of course my approach is not for bunch of emails. (Don't play with words and don't go off topic)

Simply don't use mine if you don't like it.

devcircus commented 7 years ago

Think he's confusing session for cache. There wouldn't be a "bunch" of emails.

iranianpep commented 7 years ago

Well this is going nowhere! This is the last that I'm trying to justify myself: Yes, I have used flash messages. As I said you can store anything on the session however because it's stored on the server it is (again!) mainly for storing sensitive data or in another word it's the best option if you need for example store authenticated user, shopping cart details, etc.

@devcircus: in a high traffic website you never know how many users are requested to reset their passwords and the size of session files can grow quickly.

khalilst commented 7 years ago

@iranianpep +1 for your last off topic

georgecpacheco commented 6 years ago

@Vrutin I use your solution, and works great!! I use encrypt() and decrypt() to hide the values.

rizqyhi commented 6 years ago

Hi guys, I've just realize this issue since I was working with 5.2. I love @Vrutin answer and found a better way rather than editing vendor.

Since the notification email is sent from CanResetPassword trait, and our user model extends Illuminate\Foundation\Auth\User which use the trait. We can just override sendPasswordResetNotification() in our user model and create our own notification class like so:

public function sendPasswordResetNotification($token)
{
    $this->notify(new App\Notifications\ResetPasswordNotification($token));
}

Hope it helps. :)

devcircus commented 6 years ago

That's why I've never really understood this issue. Laravel doesn't use it and these traits are designed to be overwritten.

shiroamada commented 6 years ago

From User point of view:

  1. Since the system already know my email, why I need to retype again? From Developer point of view: Email is sensitive data??

From my point of view, I think it will be great if it is auto fill, I know Laravel Open Source, you can do everything you want to do it. When you propose and show the password reset flow, I am sure your client will ask you why I need to retype again the password since the system know it. My answer will be for security purpose. Maybe we need more clear on this move?

JohnnyWalkerDigital commented 6 years ago

Solution

Here's how to do it without altering vendor/core - tested with Laravel 5.6 (should be fine with later versions, too):

1. Create own notification

Firstly create a new notification for your app:

php artisan make:notification ResetPassword

Then open the newly created file: app\Notifications\ResetPassword.php and make the following changes:

Add public $token; to the beginning of the class (ie. after use Queueable).

Add $this->token = $token; to the body of your __construct() method.

Add $token as parameter to the __construct() method (so it reads __construct($token)).

Add the following to the body of your toMail() method (replacing what's there by default):

        return (new MailMessage)
            ->line('You are receiving this email because we received a password reset request for your account.')
            ->action('Reset Password', url(config('app.url').route('password.reset', [$this->token, $notifiable->email], false)))
            ->line('If you did not request a password reset, no further action is required.');

What you've done is create a copy of the original vendor/core version of the ResetPassword notification (found in /vendor/laravel/framework/src/Illuminate/Auth/Notifications/ResetPassword.php). This allows you to make customisations to it without changing the core Laravel files.

(One difference we've made is adding $notifiable->email to the body of the email.)

2. Point to your notification

Now we need to tell Laravel to call our notification instead of the one found in the CanResetPassword trait. To do this, just edit your App/User.php model:

Add use App\Notifications\ResetPassword; to the top of the file (ie. a link to the notification you just created) and then add a new method:

    public function sendPasswordResetNotification($token)
    {
        $this->notify(new ResetPassword($token));
    }

3. Update your routes

Finally we need to update our routes to include the new {email} parameter:

Route::get('/password/reset/{token}/{email}', 'Auth\ResetPasswordController@showResetForm')->name('password.reset');

Now if the user fills in the /password/reset form, they will be sent your notification, with the additional email parameter.

This now works perfectly!

Additional: Handle errors

You can improve on the above by adding a bit of error handling. To do this, just pass the email to your form, to handle things if there's a problem:

Add the following method to Auth/ForgotPasswordController.php:

    protected function sendResetLinkFailedResponse(Request $request, $response)
    {
        return back()->withErrors(
            ['email' => trans($response)]
        )->withInput($request->only('email'));
    }

And then place the following at the top: use Illuminate\Http\Request;.

And you're done!

Additional: Finesse the UX

If you want, I'd also recommend modifying the semantic HTML in your views (regardless of your design). Go to resources/views/auth/passwords/reset.blade.php and remove the autofocus attribute from the email input element, and add readonly. (I move the autofocus to the first password input element myself.)

Additional: Encrypt the email address in the URL

You can still tidy things up a bit more by encrypting the email in the URL. This isn't for security, it's to make it look prettier (so to speak) and make it less likely for the user to mess with the URL and break something.

To do this, just go to the notification we created in step 1 and change $notifiable->email to encrypt($notifiable->email).

Then go to app/Http/Controllers/Auth/ResetPasswordController.php and add the following method:

    public function showResetForm(Request $request, $token = null)
    {
        return view('auth.passwords.reset')->with(
            ['token' => $token, 'email' => decrypt($request->email)]
        );
    }

Don't forget to place use Illuminate\Http\Request; at the top, too.

And you're done! (Again.)


I've tested all this fully, and it works perfectly, but if you have an issue, let me know.

Good luck!

whitehorse0 commented 6 years ago

or you want to overide route reset

in ResetPassword notifiation: use URL;

$link = URL::to("password-reset?token={$this->token}&email={$notifiable->email}");

return (new MailMessage)
                    ->line('You are receiving this email because we received a password reset request for your account.')
                    ->line('Click the button below to reset your password:')
                    ->action('Reset Password', $link)
                    ->line('If you did not request a password reset, no further action is required.');

and then you create route Route::get('password-reset', YourController); acces on your controller $request->email or $request->token

simioluwatomi commented 6 years ago

I had this working in a project but I made a lot of changes to the project but I never touched this. I wanted to test if everything works out but got the surprise of my life that it didn't.

Sending the reset link response was part one, resetting the passwords was part two.

If you are a newbie like me, you won't be getting the token and e-mail field in your post request if you do not add it as parameters in your route and method. I ended up doing this

Route::post('/password/reset/{token}/{email}', 'ResetPasswordController@reset')->name('password.reset');
public function reset(Request $request, $token, $email)
    {
        $request->request->add(
            [
                'token' => $token,
                'email' => $email,
            ]
        );
        $this->validate($request, $this->rules(), $this->validationErrorMessages());
        $response = $this->broker()->reset(
            $this->credentials($request), function ($user, $password) {
                $this->resetPassword($user, $password);
            }
        );

        return $response == Password::PASSWORD_RESET
                    ? $this->sendResetResponse($response)
                    : $this->sendResetFailedResponse($request, $response);
    }

For the security guys, I'll think about security later; let this work first!!! If you think this could be better, kindly give suggestions

ryanrapini commented 6 years ago

Am I missing something big here? If the email is being stored in the database alongside the token, can it not just be retrieved and auto-populated into the form? Storing it in the URL seems silly, you have to check the token when you follow the email, right?

The token is already supposed to be a secure identifier, if the user has the token they surely know the email. So why can't we just look it up from the database?

khalilst commented 6 years ago

@ryanrapini, Yes, ur missing a point. The token hashed in url! So u need email to find the token itself in db (and not vice versa)

devcircus commented 6 years ago

Password reset flow works perfectly since this change. I still haven't understood the issue. Plus it's simple to add the old functionality back if you want.🤔

mtsunu commented 6 years ago

here's a hack for finding the email with token only

public function showResetForm(Request $request, $token = null)
    {
        $dbTokens = DB::table('password_resets')->get();
        $email = '';
        foreach($dbTokens as $dbToken) {
            if($this->hasher->check($token, $dbToken->token)) {
                $email = $dbToken->email;
                break;
            }
        }

        return view('auth.passwords.reset')->with(
            ['token' => $token, 'email' => $email]
        );
    }

don't know about the performance though..

cyth319 commented 6 years ago

i am using 5.6 and this problem still continues. default e-mail provided by laravel does not include an email field in its URL. however it wants an email to reset the password. in the documentation it shows that email field should be filled by old method. like that => value="{{ old( 'email' ) }}" . however it does not work for me.

now, they used old() method in documentation already says us they never expect mail form url. They want to get it from a session which makes sense but why its not working?

on the other hand if they expect it from a session why they have this code in Illuminate\Foundation\Auth\ResetsPasswords.php

public function showResetForm(Request $request, $token = null)
{
    return view('auth.passwords.reset')->with(
        ['token' => $token, 'email' => $request->email]
    );
}

i am confused.

devcircus commented 6 years ago

That line just wasn't removed when the implementation changed. If you decide to pass an email, the helper may come in handy. Passing the email is completely unnecessary though. Honestly that line should just be removed.

cyth319 commented 6 years ago

Passing the email is completely unnecessary though

so why my controller ResetPasswordController@reset shows "email field is required" ?

devcircus commented 6 years ago

It's definitely required to type the email into the form. If my memory is correct, the logic changed a while back so as to not send the email in the url, for security reasons. The 'old' method was just never removed from the view. However, if you wish to send the email in the url, you are free to change the reset functionality as you see fit.