Skip to content
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

[5.5] Verbose TokenMismatch Error #18728

Merged
merged 2 commits into from
Apr 10, 2017
Merged

[5.5] Verbose TokenMismatch Error #18728

merged 2 commits into from
Apr 10, 2017

Conversation

laurencei
Copy link
Contributor

@laurencei laurencei commented Apr 8, 2017

Currently when a user has a TokenMismatch due to an expired session - they get a "Whoops" page and the server throws a 500 error.

This is very very confusing to users. There is no indication of what the error was. And technically a 500 error implies an internal "server" error, when in fact it should be throwing some 4xx error - as it is a client error.

This is an common issue faced by developers, such as here, here, here, here, here and here.

This PR aims to improve the experience for users out of the box.

beforeafter

It is now very obvious to all users what the problem is and how to fix it.

The other benefit is developers can now add their own view/errors/419.blade.php file to their application to specifically target this error message, and include a button there to send to login/dashboard etc as needed. I didnt want to automatically redirect etc - because everyone's routes will be different.

One point of discussion is about what is the most appropriate 4xx header to throw. There is technically no status code for "session expired" in the RFC (as it is part of the application layer and not the HTTP layer).

I went with 419 - which is not part of the official RFC - but it is mentioned on some websites such as here, here and here.

The alternative is use 400 - but that was a little too generic for me. We cant use 401, because that is used for the HTTP Basic Auth. I thought about 408 - but I think that is more for socket connections.

@deleugpn
Copy link
Contributor

deleugpn commented Apr 8, 2017

I think this is absolutely marvelous.

CSRF Attack - The user knows what he's doing and the fact that the message doesn't necessarily reflects the reality, we shouldn't care.
Developer forgot csrf_field - Software bug and still easy for the developer to know it.
Session expiration - User friendly message.

It's a win-win-win change.

@laurencei
Copy link
Contributor Author

laurencei commented Apr 8, 2017

Apart from an attempted CSRF attack - the only situation I can think of where the message might be erroneous is if you had multiple tabs open - and the session changed after the fact.

But that would realistically only affect a small subset of users - and it wont be the end of the world if they log out and log back in (because they would have got a whoops page anyway before)

@brunogaspar
Copy link
Contributor

It's a nice addition but in my opinion the message itself should be changed to something else rather than stating that the user has been logged out due to inactivity.

The reason for that is because you don't need to be logged in or even logged out for a CSRF token to expire.

@deleugpn
Copy link
Contributor

deleugpn commented Apr 8, 2017

@brunogaspar I guess the message is the least important subject here because you can override it by creating your own errors.419.blade.php file

@taylorotwell
Copy link
Member

What if it was an AJAX request expecting JSON?

@deleugpn
Copy link
Contributor

deleugpn commented Apr 8, 2017

#18731 solves the JSON problem?

@laurencei
Copy link
Contributor Author

laurencei commented Apr 8, 2017

@taylorotwell - the only difference I can see is it will give a 419 status code (which developers can catch) instead of a 500 code that it does now.

Currently the framework returns the full 500 html page for a TokenMismatch when coming via AJAX anyway - so there is no change in behavior from an AJAX perspective apart from the status code.

I had a look into the Exception class - and I have an idea how to solve the ajax issue for all errors - but I'll do it as another PR for that issue.

Edit: I've done a separate PR around AJAX + exceptions: #18732

@devcircus
Copy link
Contributor

Thanks for attempting to simplify this. This is something that has to be addressed in every project I start. Should be handled better out-of-the-box. Many developers overlook this and don't customize for csrftoken errors due to session timeout.

@taylorotwell
Copy link
Member

I kind of agree the message here doesn't quite fit the scenario. As @brunogaspar said.

@laurencei
Copy link
Contributor Author

laurencei commented Apr 10, 2017

What about

"The page has expired due to inactivity. Please refresh the page and try again."

or even

"The page expired due to inactivity. Please refresh the page and try again."

or

"Your request expired due to inactivity. Please refresh the page and try again."

That way - when they "refresh" - if they have been logged out the application will redirect them to the login screen. And if it is just a form - they'll have a new token ready to go?

@taylorotwell
Copy link
Member

"The page has expired due to inactivity. Please refresh the page and try again."

I think that is fine.

@laurencei
Copy link
Contributor Author

@taylorotwell - ok - done.

@taylorotwell taylorotwell merged commit 79df141 into laravel:master Apr 10, 2017
@taylorotwell
Copy link
Member

taylorotwell commented Apr 10, 2017

Sometimes I wonder if the default Vue setup should have an interceptor that catches such errors and redirects to /login if caught. I usually add that to every project. Or, I guess it would be Axios now that vue-resource is deprecated.

@laurencei laurencei deleted the tokenMismatch branch April 10, 2017 19:29
@johanobergman
Copy link

@taylorotwell That would be a great addition, everyone has to do it anyway.

I wonder if we need an error page at all, even when doing a regular form post. It isn't really necessary to differentiate between form submission and regular link navigation when the session has expired. Just redirect the user to the login page, with a flash message telling them they have to log in again - would really improve the ux and still prevent the hacker.

@tillkruss
Copy link
Contributor

@laurencei: What about a default 401 page?

@laurencei
Copy link
Contributor Author

laurencei commented May 21, 2017 via email

@johanobergman
Copy link

Agreed. Redirecting back to the previous page (and consequently automatically to login if needed) might be a better option. Laravel provides good hooks in the ExceptionHandler for such cases, but it seems like having an error by default will impact the user experience on the many sites that'll leave the exception config to its default.

Consider an auth registration form that has been left open for hours. When the person who's about to register enters all the details and clicks "register", Laravel's default behavior to throw an error might be enough to lose a customer. A redirect back with all input plus flash message telling them the session had expired might be a better way to tackle it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants