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

Add CSRF protection #27

Closed
josephmancuso opened this issue Feb 28, 2018 · 8 comments
Closed

Add CSRF protection #27

josephmancuso opened this issue Feb 28, 2018 · 8 comments
Assignees
Labels
enhancement New feature or request
Milestone

Comments

@josephmancuso
Copy link
Member

josephmancuso commented Feb 28, 2018

So this will require a few moving parts here.

So how CSRF works is that there is a random key generated (like 8yr8738b8b87b8br48gf84b7bv) at the start of each request. This changes on every request so it needs to be random.

This token can be used inside forms so there needs to be a {{ csrf_field() }} function which just points to the request.get_csrf() method or something. Check the HelpersProvider for how helper functions work. Especially in templates (it uses the View.share) method.

Then when the request is submitted via POST, the CSRF token outputted by the csrf_field() function is checked against the one that was at the start of the request. If it's the same then the request was made by the current. If it wasn't then there is something wrong and it should throw an exception like InvalidCSRFToken

Now there is something to be aware of and that is that not all routes should have to be CSRF protected. It should be up to the developer to choose the ones that are not protected (all routes should be protected by default).

What I'm thinking of is that only new CSRF tokens should be generated if it is a GET request. This allows you to have a POST request and check if the CSRF token is valid

Please let me know if you have any questions

@josephmancuso josephmancuso added this to the Roadmap milestone Feb 28, 2018
@josephmancuso josephmancuso added the enhancement New feature or request label Feb 28, 2018
@josephmancuso josephmancuso mentioned this issue Feb 28, 2018
1 task
@mapeveri
Copy link
Contributor

mapeveri commented Mar 1, 2018

I think it's pretty clear, what I think is that all routes must have csrf, now if a route does not need that protection, you should use some method or function, csrf_except or something similar.

@josephmancuso
Copy link
Member Author

josephmancuso commented Mar 1, 2018

Should we do it in the routes.py file?

Post().route('url', 'controller').name('url').no_csrf()

or something like that?

This MAY create some complications when checking if its valid. but may not. We'd have to test

@mapeveri
Copy link
Contributor

mapeveri commented Mar 1, 2018

Yes, exactly.

@josephmancuso
Copy link
Member Author

josephmancuso commented Mar 1, 2018

or option 2 would be to create a list of routes in a configuration file but meh. I like the speed involved in the routes file

@josephmancuso
Copy link
Member Author

ok good

@josephmancuso
Copy link
Member Author

I say a good route here is that unless I'm thinking wrong, Get() should not have a no_csrf() method on it. So we can do like option 1 from the contracts issue where the Base class (check the masonite/routes.py) file should have a NotImplemented and only the Post class should override it and the Get will throw an exception. Just in case people don't realize it's only for POST requests.

CSRF is not needed for GET requests correct?

@josephmancuso
Copy link
Member Author

josephmancuso commented Mar 1, 2018

also remember that the Request object is instantiated before the server starts. So every time the request is made, its the same request object as the last request. Use that to your advantage. Think of it as a singleton

@mapeveri
Copy link
Contributor

mapeveri commented Mar 1, 2018

It's true for GET requests not is necessary the csrf protection, is more, for GET request It doesn't add protection, but the opposite, makes a possible attack in the site.

mapeveri added a commit to mapeveri/core that referenced this issue Mar 4, 2018
mapeveri added a commit to mapeveri/core that referenced this issue Mar 4, 2018
@josephmancuso josephmancuso modified the milestones: Roadmap, Masonite 1.4 Mar 8, 2018
josephmancuso added a commit that referenced this issue Mar 8, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants