-
-
Notifications
You must be signed in to change notification settings - Fork 420
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 commentThe 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 |
||
Secure: !config.InsecureCookie, | ||
Expires: expires, | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -278,6 +278,13 @@ func TestAuthMakeCookie(t *testing.T) { | |
c = MakeCookie(r, "[email protected]") | ||
assert.Equal("testname", c.Name) | ||
assert.False(c.Secure) | ||
|
||
config.CookieName = "testname" | ||
config.InsecureCookie = true | ||
config.SameSiteCookie = 3 | ||
c = MakeCookie(r, "[email protected]") | ||
assert.Equal("testname", c.Name) | ||
assert.Equal(http.SameSiteStrictMode, c.SameSite) | ||
} | ||
|
||
func TestAuthMakeCSRFCookie(t *testing.T) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 commentThe 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 |
||
CookieName string `long:"cookie-name" env:"COOKIE_NAME" default:"_forward_auth" description:"Cookie Name"` | ||
CSRFCookieName string `long:"csrf-cookie-name" env:"CSRF_COOKIE_NAME" default:"_forward_auth_csrf" description:"CSRF Cookie Name"` | ||
DefaultAction string `long:"default-action" env:"DEFAULT_ACTION" default:"auth" choice:"auth" choice:"allow" description:"Default action"` | ||
|
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?