teknik-eksjo / chronos

A scheduling app that helps teachers submit workday outlines
MIT License
5 stars 5 forks source link

Added CSRF-protection on ajax calls #146

Closed hugolundin closed 8 years ago

hugolundin commented 8 years ago

Part of the solution of #141

Greenheart commented 8 years ago

@Limpan Yes, adding a closure is obfuscation. @hugolundin I've read more about their suggested implementation and it seems to handle CSRF-threats pretty well.

My concern is that the global csrftoken will allow XSS-attacks to exploit valid ajax-calls to modify the app data.

Limpan commented 8 years ago

Since CsrfProtect in FlaskWTF checks referrer we're pretty much good on cross-site issues. https://github.com/lepture/flask-wtf/blob/master/flask_wtf/csrf.py#L215-L218 https://en.m.wikipedia.org/wiki/Cross-site_request_forgery#Limitations

Since XSS is only applicable when we have the user enter data it is pretty much a moot point in our app. (It's about the user entering code instead of whatever and we serving it as code.)

I'd say that the solution provided by @hugolundin is as good as it needs to be and that there's not much we can (or should) improve on.

Greenheart commented 8 years ago

https://github.com/lepture/flask-wtf/blob/master/flask_wtf/csrf.py#L215-L218

Didn't know it was possible to link to specific lines, nice feature! :)


@Limpan To answer your comments: This code is good enough to protect from CSRF. Chronos doesn't provide enough good surfaces to be viable for effective XSS.

I do agree that this is ready to be merged - I just want to make clear that this PR will introduce a potential vulnerability that allows malicios js to abuse $.ajax() altough it's very unlikely.