Skip to content

Conversation

@Astra-X
Copy link

@Astra-X Astra-X commented May 16, 2024

No description provided.

Copy link
Member

@dantownsend dantownsend left a comment

Choose a reason for hiding this comment

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

Thanks for the PR.

I've left some comments about why the values are currently set as they - happy to discuss further.

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

token,
max_age=self.max_age,
httponly=True,
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:

production: bool = False,

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.

@amazingakai
Copy link

@dantownsend Is this alright now?

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants