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.

Sign up for GitHub

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

Use secure and httponly csrf cookie #284

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Use secure and httponly csrf cookie #284

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ab10d3c
use secure and httponly csrf cookie
Astra-X May 16, 2024
6cdf3a5
add procution param for secure csrf cookie
May 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions piccolo_api/csrf/middleware.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,8 @@ async def dispatch(
self.cookie_name,
token,
max_age=self.max_age,
httponly=True,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

secure=True,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

piccolo_api/piccolo_api/session_auth/endpoints.py

Line 330 in f3e17a6

production: bool = False,

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, this needs to be updated in piccolo admin as well, right?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that's right. We would pass production in here: https://github.com/piccolo-orm/piccolo_admin/blob/5e8e96d56656cbf7283b63f8729d0748eb895867/piccolo_admin/endpoints.py#L439

)
return response
else:
Expand Down
Loading