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

Merged
merged 3 commits into from
Dec 13, 2024

Conversation

lopezvoliver
Copy link
Contributor

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?

@lopezvoliver
Copy link
Contributor 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.

lopezvoliver and others added 2 commits December 13, 2024 11:03
@maartenbreddels maartenbreddels merged commit e2ad53d into widgetti:master Dec 13, 2024
1 check passed
@maartenbreddels
Copy link
Contributor

Thanks for your contribution @lopezvoliver !

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