-
Notifications
You must be signed in to change notification settings - Fork 5
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
Add additional builder for cookie settings #6
Add additional builder for cookie settings #6
Conversation
I'm not fond of exposing knobs to weaken the security posture - looking at |
Thanks for the feedback. Fixed. Now only path and domain are settable. |
Thanks for checking that. How about this? Domain should now remain unset if it is not set by the user. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good in terms of functionality! Left a style comment.
Thanks! Yes, much better this way! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice improvements to the comments :)
Hi,
I'm proposing to add builder options for the rest of the cookie settings in CookieMessageStore. It's useful to be able to set these, for example when testing without https, etc. Please let me know, if there is anything I can do to improve this.
All best,
Jeremy