flask-security
flask-security copied to clipboard
mattupstate
Logout view is not idempotent and vulnerable to CSRF
Now that I've thought about it, it's really wrong that a GET request to logout view can log user out, especially since browsers now send GET requests whenever they want. Also logout view doesn't do CSRF checking that is done in all other views.
So I propose that we do what Github for instance does: on GET request show a form that is then sent in a POST request with csrf checking and everything.
Logout does not manipulate any data on the backend so CSRF is not required.
I guess it's pretty minor but still it's not nice when any random page has ability to log you out.
We had this discussion WRT Flask-Social (but there were issues with deleting data on the backend), but I'd still +1 this. Since CSRF protection is built into Flask-WTF and Flask-Security is opinionated to use Flask-WTF, I think adding this wouldn't cause any bloat. I'm going to work on implementing CSRF checking on Flask-Social routes and will follow up here when I come up with a reasonable solution.
@swarmer revisiting this again...how is logout not idempotent? The result of performing a GET request to /logout results in the same thing every time, does it not?
@swarmer Would you mind if I closed this? CSRF is not required on GET requests. And I'm not sure I see a reason to make logout a POST at the moment.
Hmm, I think @swarmer has a point here, since right now a CSRF attack could log a user out, and that's suboptimal.
It's your call if you want to close this. As for reason, here's an example: you have a website where users can embed pictures in comments. Someone posts a big goatse and next to it a fake image linked to /logout. A moderator tries to remove this but every time they try they are immediately being logged out.
Ha! Had to use a goatse ref, didn't you? I hear your point. However, I think this would be a rather significant change for current users. I could imagine it as a feature flag, though.
So I'd recommend using the CSRF_ENABLED flag that Flask-WTF creates when you use CsrfProtect(app). I think it's fair enough to assume that if a user is using this extension, they should be comfortable changing their calls to /logout to include a token.
In my application, logging out might result in involuntary data-loss (users are only session-based, and do not necessarily have a password for re-login). In this scenario, logging out via GET is critical. I would much prefer a POST logout action, protected by a CSRF token.
Please see the following for an interesting related discussion (and example) from the Django context: https://code.djangoproject.com/ticket/15619
Log out should definitely be a form post. Destruction of a session should count as a mutation just as much as creation of a session should.
The tricky part is backward compatibility, since we don't know how application developers have customized their use of flask security. Post to log out should definitely be the default, but you could add a deprecation notice and a flag for people to easily get the old behavior back if they upgraded.
Workaround (to use POST instead of GET for logout):
bp = Blueprint('my_security', __name__, url_prefix='/my-security')
@bp.route('/logout', methods=['POST'])
def my_logout():
from flask_security.views import logout
return logout()
Then in the header (logout button):
<form id="LogoutForm" method="post" action="{{ url_for('my_security.my_logout') }}">
SUBMIT BUTTON
</form>
