sessions pkg: omit domain attribute in session cookie (#329)

* pkg: cookie_store: omit domain value unless explicitly set * pkg: cookie_store: small adjustments for the sake of (mostly) logging * update documentation further
buzzfeed · Dec 17, 2021 · 8f2d3ac · 8f2d3ac
1 parent 7018b6f
commit 8f2d3ac
Show file tree

Hide file tree

Showing 4 changed files with 23 additions and 11 deletions.
diff --git a/docs/sso_authenticator_config.md b/docs/sso_authenticator_config.md
@@ -11,7 +11,9 @@ Defaults for the below settings can be found here: https://github.com/buzzfeed/s
 ```
 SESSION_COOKIE_NAME     - string - name associated with the session cookie
 SESSION_COOKIE_SECRET   - string - seed string for secure cookies
-SESSION_COOKIE_DOMAIN   - string - cookie domain to force cookies to (ie: .yourcompany.com)*
+SESSION_COOKIE_DOMAIN   - string - cookie domain (and subdomains) to force cookies to (ie: .yourcompany.com). If this is set,
+cookies will be valid on subdomains too. To restrict to only the request's domain, leave this unset.
+Reference: https://developer.mozilla.org/en-US/docs/Web/HTTP/Cookies#define_where_cookies_are_sent
 SESSION_KEY             - string - seed string for secure auth codes
 SESSION_COOKIE_SECURE   - bool - set secure (HTTPS) cookie flag
 SESSION_COOKIE_HTTPONLY - bool - set 'httponly' cookie flag

diff --git a/docs/sso_proxy_config.md b/docs/sso_proxy_config.md
@@ -11,7 +11,9 @@ Defaults for the below settings can be found here: https://github.com/buzzfeed/s
 ```
 SESSION_COOKIE_NAME     - string - name associated with the session cookie
 SESSION_COOKIE_SECRET   - string - seed string for secure cookies
-SESSION_COOKIE_DOMAIN   - string - cookie domain to force cookies to (ie: .yourcompany.com)*
+SESSION_COOKIE_DOMAIN   - string - cookie domain (and subdomains) to force cookies to (ie: .yourcompany.com). If this is set,
+cookies will be valid on subdomains too. To restrict to only the request's domain, leave this unset.
+Reference: https://developer.mozilla.org/en-US/docs/Web/HTTP/Cookies#define_where_cookies_are_sent
 SESSION_COOKIE_SECURE   - bool - set secure (HTTPS) cookie flag
 SESSION_COOKIE_HTTPONLY - bool - set 'httponly' cookie flag
 SESSION_TTL_LIFETIME    - time.Duration - 'time-to-live' of a session lifetime

diff --git a/internal/pkg/sessions/cookie_store.go b/internal/pkg/sessions/cookie_store.go
@@ -81,22 +81,22 @@ func NewCookieStore(cookieName string, optFuncs ...func(*CookieStore) error) (*C
 
 func (s *CookieStore) makeCookie(req *http.Request, name string, value string, expiration time.Duration, now time.Time) *http.Cookie {
 	logger := log.NewLogEntry()
-	domain := req.Host
-	if h, _, err := net.SplitHostPort(domain); err == nil {
-		domain = h
-	}
+
 	if s.CookieDomain != "" {
+		domain := req.Host
+		if h, _, err := net.SplitHostPort(domain); err == nil {
+			domain = h
+		}
 		if !strings.HasSuffix(domain, s.CookieDomain) {
-			logger.WithRequestHost(domain).WithCookieDomain(s.CookieDomain).Warn("Warning: Using configured cookie domain.")
+			logger.WithRequestHost(domain).WithCookieDomain(s.CookieDomain).Warn("Warning: Using explicitly configured cookie domain.")
 		}
-		domain = s.CookieDomain
 	}
 
 	return &http.Cookie{
 		Name:     name,
 		Value:    value,
 		Path:     "/",
-		Domain:   domain,
+		Domain:   s.CookieDomain,
 		HttpOnly: s.CookieHTTPOnly,
 		Secure:   s.CookieSecure,
 		Expires:  now.Add(expiration),

diff --git a/internal/pkg/sessions/cookie_store_test.go b/internal/pkg/sessions/cookie_store_test.go
@@ -104,12 +104,16 @@ func TestMakeSessionCookie(t *testing.T) {
 		expectedCookie *http.Cookie
 	}{
 		{
+			// In order to prevent subdomains being included, the Domain
+			// value is left empty by default.
+			// While empty, it defaults to the URL domain with subdomains _excluded_.
+			// https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Set-Cookie
 			name: "default cookie domain",
 			expectedCookie: &http.Cookie{
 				Name:     cookieName,
 				Value:    cookieValue,
 				Path:     "/",
-				Domain:   "www.example.com",
+				Domain:   "",
 				HttpOnly: true,
 				Secure:   true,
 				Expires:  now.Add(expiration),
@@ -159,12 +163,16 @@ func TestMakeSessionCSRFCookie(t *testing.T) {
 		expectedCookie *http.Cookie
 	}{
 		{
+			// In order to prevent subdomains being included, the Domain
+			// value is left empty by default.
+			// While empty, it defaults to the URL domain with subdomains _excluded_.
+			// https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Set-Cookie
 			name: "default cookie domain",
 			expectedCookie: &http.Cookie{
 				Name:     csrfName,
 				Value:    cookieValue,
 				Path:     "/",
-				Domain:   "www.example.com",
+				Domain:   "",
 				HttpOnly: true,
 				Secure:   true,
 				Expires:  now.Add(expiration),