-
-
Notifications
You must be signed in to change notification settings - Fork 413
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 SameSite option #264
base: master
Are you sure you want to change the base?
Add SameSite option #264
Conversation
Hi, I'd really appreciate if this could be merged: It would solve the issues we have when trying to embedd a page authenticated over these means as an iframe into e.g. confluence or sharepoint! |
@@ -28,6 +28,7 @@ type Config struct { | |||
Config func(s string) error `long:"config" env:"CONFIG" description:"Path to config file" json:"-"` | |||
CookieDomains []CookieDomain `long:"cookie-domain" env:"COOKIE_DOMAIN" env-delim:"," description:"Domain to set auth cookie on, can be set multiple times"` | |||
InsecureCookie bool `long:"insecure-cookie" env:"INSECURE_COOKIE" description:"Use insecure cookies"` | |||
SameSiteCookie int `long:"same-site-cookie" env:"SAMESITE_COOKIE" default:"0" description:"Set cookies SameSite value (0: Default (1), 1: Lax, 2: Strict, 3: None)"` |
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 think that those numbers are wrong. For me I needed to set 4 for None for example
@@ -173,6 +173,7 @@ func MakeCookie(r *http.Request, email string) *http.Cookie { | |||
Path: "/", | |||
Domain: cookieDomain(r), | |||
HttpOnly: true, | |||
SameSite: http.SameSite(config.SameSiteCookie), |
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.
shouldn't we have this line in the 3 other similar blocks in this package? So both for the "Clear" and for the CSRF-Cookie further down? I needed to do this in order to get everything work on my end
@@ -154,6 +161,7 @@ Application Options: | |||
--config= Path to config file [$CONFIG] | |||
--cookie-domain= Domain to set auth cookie on, can be set multiple times [$COOKIE_DOMAIN] | |||
--insecure-cookie Use insecure cookies [$INSECURE_COOKIE] | |||
--same-site-cookie Set SameSite cookie property (0: Default (1), 1: Lax, 2: Strict, 3: None) |
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.
The Env-Name available to control this sould be documented here, [$SAMESITE_COOKIE]
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.
same issue here with the numbers as well, I think also the (1) is missleading?
Not sure if this is related: even with the fix from the PR it is only working to embedd a page protected over this tool as an IFrame if at least once before the user has accessed the page standalone and has a current session with the IDP established. If not the flow tries to redirect to login.microsoftonline and that page denies to be shown in an iframe.
|
Related to #95
Add the possibility to set le SameSite cookie flag.