Skip to content

Conversation

@robscllc
Copy link
Contributor

No description provided.

@vimalloc
Copy link
Owner

Good catch on the and statements really needing to be or ones in the examples, I really messed those up.

Per looking at the csrf_protect token in a cookie instead of the headers, that actually would not work. The cookies are always going to be sent to the server, regardless of if the request comes from your site, or if it comes from a malicious site attempting to do a CSRF attack. If we check for the token in the cookies (which, it should be there), we are not actually preventing CSRF attacks at all.

In order to prevent them, we are relying on the header having the CSRF protection token, and the only way the header should be able to get the token is if the person making the request can use javascript on the given domain to access that CSRF token from the cookie, which is what actually provides the CSRF protection.

This article has a good write up of these ideas: http://www.redotheweb.com/2015/11/09/api-security.html. If you wanted more information or clarification, you can also hit me up and I would be more then happy to go over this with you.

If you want to revert the csrf changes, and update the pull request message, I would be happy to merge this. Thanks for contributing! 👍

@robscllc
Copy link
Contributor Author

Ah, got it. That's a great article. Updating PR now. Thanks!

@robscllc robscllc changed the title check csrf_protect cookie, not header, in _decode_jwt_from_cookies verify both username and password in examples Jan 16, 2017
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 2818923 on robscllc:master into cea3c4b on vimalloc:master.

1 similar comment
@coveralls
Copy link

coveralls commented Jan 16, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling 2818923 on robscllc:master into cea3c4b on vimalloc:master.

@vimalloc vimalloc merged commit 19094aa into vimalloc:master Jan 16, 2017
@vimalloc
Copy link
Owner

Fantastic. Thanks :)

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.

3 participants