Email verification should work across devices #117
Conversation
|
|
||
| $user = User::findOrFail($id); | ||
|
|
||
| if (! hash_equals($hash, sha1($user->getEmailForVerification()))) { |
There was a problem hiding this comment.
Should this be calling sha1 directly? Isn't there a Laravel abstraction for this? Also bearing this in mind.
| event(new Verified($user)); | ||
| } | ||
|
|
||
| // Always log the user in, regardless of verification status | ||
| Auth::login($user); |
There was a problem hiding this comment.
This makes verification links a little dangerous – anyone who gets hold of one is instantly logged in with no other auth required. Usual practice after verification is to require a login, which solves that problem, though at a mild UX expense.
/cc @valorin
Note that this additional login doesn't really apply for OAuth, since verification and login are essentially the same thing there.
|
Hello! Have you considered that many email servers now (and have for many years) click links in emails to prevent phishing? (Example: https://old.reddit.com/r/webdev/comments/1i861ej/ms_and_other_antivirus_now_click_on_links_in/ but if you search around there are many other articles). This would cause users to be verified automatically without user action. Which may be ok if you only care that the email exists, but otherwise I'm not sure it should be the default action. |
|
To be fair, that's all we are actually interested in when verifying, but it does also suggest that logging the user in in response to a verification link is a bad idea, as I mentioned in a review comment. This is the same problem that faced email subscription headers, where one-click unsubscribe links were automatically opened by mail filters, automatically unsubscribing everyone, leading to the introduction of the |
|
I need to have a deeper look into this, but I wanted to quickly respond to this:
You shouldn't blindly accept email verification if the link is clicked because then you're only verifying if the email exists, rather than if the owner of the email address signed up for the service. This means you could sign up for a service, submit the email address of someone whose email provider automatically clicks link, and then you've got a verified account with their email address. You need to ensure the email verification click is intentional, to prevent abuse of automated link clicking. |
That's true. In that case it means that opening the verification link by any means is insufficient; we need to display a page with a verify button on it which must be clicked to verify the account. Of course this means that we are still subject to very stupid mail filters (which sadly exist) that also click every link on a destination page to check it for malware, but there's only so much we can do! Even so, that verification step doesn't obviate the need to require a post-verify login. While this UX may seem a little clunky, it's an extremely common pattern, and not as user-hostile as many auth flows (e.g. AWS and eBay). |
The current Verify Email functionality utilizes the EmailVerificationRequest here: https://github.com/laravel/framework/blob/12.x/src/Illuminate/Foundation/Auth/EmailVerificationRequest.php, which fails when a user clicks the verification email from a different device or browser.
This update removes the need for the user to be logged in when verifying their email. Instead, we check the id and hash directly from the signed URL. We use hasValidSignature() to make sure the link hasn’t been tampered with or expired, and then compare the hash to a SHA-1 version of the user’s email to confirm it’s legit.
With this update, the email verification flow is still secure and has a better user experience.
@taylorotwell, let me know if this looks good. I have PR's for this in the other starter kits but wanted to get your thoughts. Should I instead update the EmailVerificationRequest from inside the Laravel framework to work this way (https://github.com/laravel/framework/blob/12.x/src/Illuminate/Foundation/Auth/EmailVerificationRequest.php), or is it fine to add it in each starter kit?
Let me know 👍 Thanks!