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

Added a httponly option for HTMLResponse.set_cookie #876

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

Conversation

lopezvoliver
Copy link

Following the discussion in #799, I added an option configurable through an environment variable SOLARA_SESSION_HTTP_ONLY, and passing this to the httponly option in HTMLResponse.

As discussed, this change is backwards compatible because it defaults to False.

Here we can verify the httpOnly attribute of the solara-session-id cookie using Developer tools:

image

and here's another screenshot showing the behavior when setting the environment variable SOLARA_SESSION_HTTP_ONLY=True:

image

Note that the cookie no longer appears in document.cookie.

Thus, this PR makes it possible for a solara-based application to comply with cwe 402

Copy link
Collaborator

@iisakkirotko iisakkirotko left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks a lot for the PR @lopezvoliver!

Copy link
Contributor

@maartenbreddels maartenbreddels left a comment

Choose a reason for hiding this comment

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

Awesome, I think we can make it a bit simpler even, what do you think?

solara/server/settings.py Outdated Show resolved Hide resolved
Co-authored-by: Maarten Breddels <[email protected]>
@lopezvoliver
Copy link
Author

Awesome, I think we can make it a bit simpler even, what do you think?

Yes, and this is also consistent with the default False value in starlette.

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