spantaleev / flask-sijax

An extension for the Flask microframework that adds Sijax support.
BSD 3-Clause "New" or "Revised" License
106 stars 22 forks source link

CSRF protection #8

Open Midnighter opened 10 years ago

Midnighter commented 10 years ago

I see that you made an implementation of this in issue #6 but I find nothing about the topic of CSRF protection in the documentation of either Sijax or Flask-Sijax. Which method of using a token of those mentioned in #6 can I now use?

spantaleev commented 10 years ago

You can use the method proposed by @rrb5068 in #6.

To summarize it: it involves passing a 3rd parameter to Sijax.request(), which gets merged with the standard jQuery.ajax parameters. The problem with #6 was that Sijax wasn't merging the data parameter correctly, so you couldn't send custom stuff with the other POST values. The workaround proposed there is to send additional data as custom HTTP headers.

6 has since been fixed (as noted there), so you can pass additional data (the CSRF token) in whatever way is convenient to you.

You have to validate the CSRF token's authenticity on the server yourself - Sijax doesn't handle that for you. The Sijax documentation doesn't mention CSRF tokens, because Sijax is more low-level and doesn't concern itself with that.

Midnighter commented 10 years ago

In my (admittedly limited) experience, if security features like CSRF are not explicitly documented then they simply are not used in a majority of cases. So if the lack of documentation is simply a time issue rather than a decision cut in stone I'd be happy to contribute documentation myself. Maybe @rrb5068 could also provide an illuminating use-case.

spantaleev commented 10 years ago

I'm not at all against documenting the CSRF and Sijax situation.

I agree that it'd be nice if CSRF is mentioned (the least we could do). If a few ways to protect against it are mentioned as well, even better.

If you feel like contributing these additions, I'd be happy to merge them in.

rrb5068 commented 10 years ago

Sorry, I haven’t checked this in quite a while. @Midnighter, the use case comes up almost inevitably if using Flask-Sijax for a production level Flask app.

With a Flask app, I’m going to turn on CSRF protection because it’s going to keep my users safe. If I then decide to enhance my app with the functionality provided by Flask-Sijax, I’m most likely going to do a POST request with AJAX at some point. I'll definitely do a POST request if I need to pass sensitive user data. And when I do, because CSRF protection is turned on, the Flask app is going to complain and give users a 400 Bad Request if their POST request doesn’t contain the CSRF token.

I could decorate my Sijax method and tell the app to not require the CSRF token just to make it work, but I wouldn’t want to do that because the protection is doing an important job.

You said: "In my (admittedly limited) experience, if security features like CSRF are not explicitly documented then they simply are not used in a majority of cases.”

@spantaleev may not have documented it for his Flask extension yet, but CSRF protection is widely documented for Flask and several of its extensions, as well as for web applications in general.

If you want to talk about it more, I’d be happy to.

Midnighter commented 10 years ago

Hi, thanks for checking in :smile:

I've been swamped with other things but this will become relevant to me again in about two to three weeks which is when I will try to improve the documentation as well.

My comment on documentation is mostly due to reading that "proper implementation of a security key" is left as an exercise to the reader, or that passwords need to be salted and hashed and properly checked but that is not shown. I understand the need to convey basic principles first but I think that a full example with best security practices should always be available in addition. Rant over :smiley: I'll try to do my part to improve it.

Midnighter commented 9 years ago

@spantaleev since I started working with Sijax again, I would like to make additions to the documentation now. I'm currently thinking about inserting another section after Setting up the client (browser) and maybe extending the chat, comet and upload examples. What do you think?

spantaleev commented 9 years ago

@Midnighter, that's great. Any changes are welcome!

Midnighter commented 9 years ago

So I've started with #10. Comments, questions, corrections are very welcome. For now I've changed the chat example only but I was also considering simply creating separate versions of each example with CSRF protection included.

I would also like to expand on dealing with Ajax errors. I'm not sure how Sijax handles that at the moment.

What do you think?

spantaleev commented 9 years ago

Looks good. Thanks for your contribution - it points people in the right direction.

We can possibly make it easier to pass the token for all requests and then make Flask-Sijax automatically validate it. The best way to do it may be to patch Sijax itself a bit, so that it could take some custom headers or jQuery AJAX request options and automatically add them to all requests. If the CSRF-protection feature is enabled, the initialization can be done by the g.sijax.get_js() code, ala Sijax.setDefaultOptions({...}). All Sijax.request() calls would then use these options by default (merged with any per-call custom ones provided, if any).

The server-side code can then validate the token for every request and not let it go through at all. The token creation/validation code may need to be pluggable..

What happens whenever an invalid token is encountered needs to be decided. Do we return some JSON back and fire an event on the JS side that the developer can catch? Do we just create an alert() response? (likely not..), etc.

In any case, this sounds like a major change and there's a lot of things to be decided and done about it..

Midnighter commented 9 years ago

It's definitely a lot to consider.

What happens whenever an invalid token is encountered needs to be decided. Do we return some JSON back and fire an event on the JS side that the developer can catch? Do we just create an alert() response? (likely not..), etc.

From what I could see, many people like to return an error 400 code. From my experience Sijax silently fails when something goes wrong. It'd be great if at least that could change in the near future, considering that the full range of changes is probably something for the far future. With a view, a flask.flash message could be an option. Or you add some kind of error method to the ResponseObject which can then be checked/handled as the developer wishes.

coreybrett commented 9 years ago

What about adding a Sijax.request_include_in_data method in the sijax.js file. Then I could put something like... <script type="text/javascript">Sijax.request_include_in_data('{{sijax_token}}') into one of my base templates.