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

CSRF should be a middleware #2082

Closed
julianlam opened this issue Sep 14, 2014 · 8 comments
Closed

CSRF should be a middleware #2082

julianlam opened this issue Sep 14, 2014 · 8 comments

Comments

@julianlam
Copy link
Member

This has the possibility of breaking a number of plugins, but given that I don't know a single example of a plugin that adds POST routes that isn't written by me, I think we can just squeeze this into 0.5.1 as well.

Anyway -- we require CSRF for all POST/PUT/DELETE requests, which precludes the possibility of building a write-enabled API that utilises other methods of authentication (e.g. API key or oauth2 access token over HTTPS).

Also, the CSRF system is due for a rewrite anyway.

Notes

  • CSRF will be an optional middleware instead of included by default on all requests
@julianlam julianlam self-assigned this Sep 14, 2014
@julianlam julianlam added this to the 0.5.1 milestone Sep 14, 2014
julianlam added a commit that referenced this issue Sep 14, 2014
@julianlam
Copy link
Member Author

@psychobunny
Copy link
Contributor

but given that I don't know a single example of a plugin that adds POST routes that isn't written by me

dbsearch + RSS plugin by @barisusakli and my blog comments plugin, just off the top of my head

@julianlam
Copy link
Member Author

Eh, either me or one of us, anyway.

@akhoury
Copy link
Member

akhoury commented Sep 16, 2014

@xCRNSx, this concerns https://github.com/xCRNSx/nodebb-widget-gameservers.
I couldn't create an issue in the fork. You should create your own repo instead of a fork

@julianlam
Copy link
Member Author

I believe you can re-enable issues in forks... though I'm not entirely sure on that.

julianlam added a commit to NodeBB/nodebb-theme-lavender that referenced this issue Sep 17, 2014
julianlam added a commit to NodeBB/nodebb-theme-vanilla that referenced this issue Sep 17, 2014
@julianlam
Copy link
Member Author

closed via a061079

@julianlam
Copy link
Member Author

The behaviour of the csrf token has changed!

Instead of being omni-present on the header, they are added to certain templates that require it.

For example, we want to secure the uploading of favicons in the admin panel, which is a POST upload request:
* We add the middleware.applyCSRF middleware to the uploadfavicon route
* We then generate a csrf token in the controller and pass it through to the template
* We add it to the template (or if you're using a form, you can put it into the form as a hidden input as well)

@akhoury
Copy link
Member

akhoury commented Sep 18, 2014

it's too complicated for me if it's more than one LOC
I'll stalk around for an example. (db-search, rss or blog)

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

No branches or pull requests

3 participants