-
Notifications
You must be signed in to change notification settings - Fork 968
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
feat: allow configuring same-site for session cookies #303
Conversation
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.
Looks good!
@@ -370,3 +373,15 @@ func (p *ViperProvider) SelfServiceVerificationReturnTo() *url.URL { | |||
func (p *ViperProvider) SelfServicePrivilegedSessionMaxAge() time.Duration { | |||
return viperx.GetDuration(p.l, ViperKeySelfServicePrivilegedAuthenticationAfter, time.Hour) | |||
} | |||
|
|||
func (p *ViperProvider) SessionSameSiteMode() http.SameSite { | |||
switch viperx.GetString(p.l, ViperKeySessionSameSite, "Lax") { |
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.
Let's make this case-insensitive!
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.
I thought we might respect the standard https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Set-Cookie#SameSite
but is not really necessary
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.
Yeah I guess you're right, I mean it is being validated in the JSON Schema so it would be easily detectable. I think both ways are ok. What do you think?
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.
Let's just leave it this way, validation will anyway only allow the capitalized version.
Related issue
closes #257
Proposed changes
It is now possible to set SameSite for the session cookie via the key
security.session.cookie.same_site
.