Use secure and httponly csrf cookie#284
Conversation
dantownsend
left a comment
dantownsend
left a comment
There was a problem hiding this comment.
Thanks for the PR.
I've left some comments about why the values are currently set as they - happy to discuss further.
piccolo_api/csrf/middleware.py
Outdated
| self.cookie_name, | ||
| token, | ||
| max_age=self.max_age, | ||
| httponly=True, |
There was a problem hiding this comment.
With CSRF cookies, when the front end code makes AJAX requests, it has to read the value from the CSRF cookie and submit it as a HTTP header. So we can't use httponly unfortunately.
https://piccolo-api.readthedocs.io/en/latest/csrf/usage.html#reading-from-the-cookie
piccolo_api/csrf/middleware.py
Outdated
| token, | ||
| max_age=self.max_age, | ||
| httponly=True, | ||
| secure=True, |
There was a problem hiding this comment.
The only reason this was currently set to False is because when using Piccolo Admin locally, most people wouldn't run it under HTTPS so it would stop working. I think a good compromise would be to have this configurable. For example, we have a production argument with Session auth:
There was a problem hiding this comment.
Hmm, this needs to be updated in piccolo admin as well, right?
There was a problem hiding this comment.
Yeah, that's right. We would pass production in here: https://github.com/piccolo-orm/piccolo_admin/blob/5e8e96d56656cbf7283b63f8729d0748eb895867/piccolo_admin/endpoints.py#L439
|
@dantownsend Is this alright now? |
3 participants
No description provided.