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

auth: use samesite=lax for oauth cookies #3158

Merged
merged 1 commit into from
Jul 11, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 13 additions & 6 deletions auth/cookies.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,12 +10,12 @@ import (
)

// SetCookie will set a cookie value for all API prefixes, respecting the current config parameters.
func SetCookie(w http.ResponseWriter, req *http.Request, name, value string) {
SetCookieAge(w, req, name, value, 0)
func SetCookie(w http.ResponseWriter, req *http.Request, name, value string, isSession bool) {
SetCookieAge(w, req, name, value, 0, isSession)
}

// SetCookieAge behaves like SetCookie but also sets the MaxAge.
func SetCookieAge(w http.ResponseWriter, req *http.Request, name, value string, age time.Duration) {
func SetCookieAge(w http.ResponseWriter, req *http.Request, name, value string, age time.Duration, isSession bool) {
cfg := config.FromContext(req.Context())
u, err := url.Parse(cfg.PublicURL())
if err != nil {
Expand All @@ -29,6 +29,13 @@ func SetCookieAge(w http.ResponseWriter, req *http.Request, name, value string,
secure = u.Scheme == "https"
}

// Use Lax mode for non-session cookies, this allows the cookie to be sent when
// navigating to the login page from a different domain (e.g., OAuth redirect).
sameSite := http.SameSiteLaxMode
if isSession {
sameSite = http.SameSiteStrictMode
}

http.SetCookie(w, &http.Cookie{
HttpOnly: true,
Secure: secure,
Expand All @@ -38,11 +45,11 @@ func SetCookieAge(w http.ResponseWriter, req *http.Request, name, value string,
Value: value,
MaxAge: int(age.Seconds()),

SameSite: http.SameSiteStrictMode,
SameSite: sameSite,
})
}

// ClearCookie will clear and expire the cookie with the given name, for all API prefixes.
func ClearCookie(w http.ResponseWriter, req *http.Request, name string) {
SetCookieAge(w, req, name, "", -time.Second)
func ClearCookie(w http.ResponseWriter, req *http.Request, name string, isSession bool) {
SetCookieAge(w, req, name, "", -time.Second, isSession)
}
4 changes: 2 additions & 2 deletions auth/github/identityprovider.go
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ func (p *Provider) ExtractIdentity(route *auth.RouteInfo, w http.ResponseWriter,
return nil, auth.Error("Failed to generate state token.")
}

auth.SetCookie(w, req, stateCookieName, tok)
auth.SetCookie(w, req, stateCookieName, tok, false)
u := authConfig(ctx).AuthCodeURL(tok, oauth2.ApprovalForce)

return nil, auth.RedirectURL(u)
Expand All @@ -110,7 +110,7 @@ func (p *Provider) ExtractIdentity(route *auth.RouteInfo, w http.ResponseWriter,
if err != nil || stateCookie.Value != tokStr {
return nil, auth.Error("Invalid state token.")
}
auth.ClearCookie(w, req, stateCookieName)
auth.ClearCookie(w, req, stateCookieName, false)

valid, err := p.validateStateToken(req.Context(), tokStr)
if err != nil {
Expand Down
6 changes: 3 additions & 3 deletions auth/handler.go
Original file line number Diff line number Diff line change
Expand Up @@ -212,7 +212,7 @@ func (h *Handler) FindAllUserSessions(ctx context.Context, userID string) ([]Use

// ServeLogout will clear the current session cookie and end the session(s) (if any).
func (h *Handler) ServeLogout(w http.ResponseWriter, req *http.Request) {
ClearCookie(w, req, CookieName)
ClearCookie(w, req, CookieName, true)
var sessionIDs []string
for _, c := range req.Cookies() {
switch c.Name {
Expand Down Expand Up @@ -539,7 +539,7 @@ func (h *Handler) CreateSession(ctx context.Context, userAgent, userID string) (
}

func (h *Handler) setSessionCookie(w http.ResponseWriter, req *http.Request, val string) {
SetCookieAge(w, req, CookieName, val, 30*24*time.Hour)
SetCookieAge(w, req, CookieName, val, 30*24*time.Hour, true)
}

func (h *Handler) authWithToken(w http.ResponseWriter, req *http.Request, next http.Handler) bool {
Expand Down Expand Up @@ -717,7 +717,7 @@ func (h *Handler) refererURL(w http.ResponseWriter, req *http.Request) (*url.URL
}

func (h *Handler) serveProviderPost(id string, p IdentityProvider, refU *url.URL, w http.ResponseWriter, req *http.Request) {
SetCookie(w, req, "login_redir", refU.String())
SetCookie(w, req, "login_redir", refU.String(), false)

h.handleProvider(id, p, refU, w, req)
}
Expand Down
4 changes: 2 additions & 2 deletions auth/oidc/identityprovider.go
Original file line number Diff line number Diff line change
Expand Up @@ -194,7 +194,7 @@ func (p *Provider) ExtractIdentity(route *auth.RouteInfo, w http.ResponseWriter,
return nil, auth.Error("Failed to generate state token.")
}
nonceStr := b64enc.EncodeToString(nonce[:])
auth.SetCookie(w, req, nonceCookieName, nonceStr)
auth.SetCookie(w, req, nonceCookieName, nonceStr, false)

oaCfg, _, err := p.oaConfig(ctx)
if err != nil {
Expand All @@ -215,7 +215,7 @@ func (p *Provider) ExtractIdentity(route *auth.RouteInfo, w http.ResponseWriter,
if err != nil {
return nil, auth.Error("There was a problem recognizing this browser. You can try again")
}
auth.ClearCookie(w, req, nonceCookieName)
auth.ClearCookie(w, req, nonceCookieName, false)

nonce, err := b64enc.DecodeString(nonceC.Value)
if err != nil || len(nonce) != 16 {
Expand Down