Split cookies more precisely at 4096 bytes

CHANGELOG.md

@@ -8,6 +8,7 @@
 
 ## Changes since v6.0.0
 
+- [#656](https://github.com/oauth2-proxy/oauth2-proxy/pull/656) Split long session cookies more precisely (@NickMeves)
 - [#619](https://github.com/oauth2-proxy/oauth2-proxy/pull/619) Improve Redirect to HTTPs behaviour (@JoelSpeed)
 - [#654](https://github.com/oauth2-proxy/oauth2-proxy/pull/654) Close client connections after each redis test (@JoelSpeed)
 - [#542](https://github.com/oauth2-proxy/oauth2-proxy/pull/542) Move SessionStore tests to independent package (@JoelSpeed)

pkg/sessions/cookie/session_store.go

@@ -10,17 +10,11 @@ import (
 
 	"github.com/oauth2-proxy/oauth2-proxy/pkg/apis/options"
 	"github.com/oauth2-proxy/oauth2-proxy/pkg/apis/sessions"
-	"github.com/oauth2-proxy/oauth2-proxy/pkg/cookies"
+	pkgcookies "github.com/oauth2-proxy/oauth2-proxy/pkg/cookies"
 	"github.com/oauth2-proxy/oauth2-proxy/pkg/encryption"
 	"github.com/oauth2-proxy/oauth2-proxy/pkg/logger"
 )
 
-const (
-	// Cookies are limited to 4kb including the length of the cookie name,
-	// the cookie name can be up to 256 bytes
-	maxCookieLength = 3840
-)
-
 // Ensure CookieSessionStore implements the interface
 var _ sessions.SessionStore = &SessionStore{}
 
@@ -107,6 +101,9 @@ func (s *SessionStore) makeSessionCookie(req *http.Request, value string, now ti
 		value = encryption.SignedValue(s.CookieOptions.Secret, s.CookieOptions.Name, []byte(value), now)
 	}
 	c := s.makeCookie(req, s.CookieOptions.Name, value, s.CookieOptions.Expire, now)
+
+	// Cookies are limited to 4kb including the length of the cookie name,
+	// the cookie name can be up to 256 bytes
 	if len(c.Value) > 4096-len(s.CookieOptions.Name) {
 		return splitCookie(c)
 	}
@@ -114,7 +111,7 @@ func (s *SessionStore) makeSessionCookie(req *http.Request, value string, now ti
 }
 
 func (s *SessionStore) makeCookie(req *http.Request, name string, value string, expiration time.Duration, now time.Time) *http.Cookie {
-	return cookies.MakeCookieFromOptions(
+	return pkgcookies.MakeCookieFromOptions(
 		req,
 		name,
 		value,
@@ -142,17 +139,21 @@ func NewCookieSessionStore(opts *options.SessionOptions, cookieOpts *options.Coo
 // it into a slice of cookies which fit within the 4kb cookie limit indexing
 // the cookies from 0
 func splitCookie(c *http.Cookie) []*http.Cookie {
-	logger.Printf("WARNING: Multiple cookies are required for this session as it exceeds the 4kb cookie limit. Please use server side session storage (eg. Redis) instead.")
+	if len(c.Value) < 4096-len(c.Name) {
-	if len(c.Value) < maxCookieLength {
 		return []*http.Cookie{c}
 	}
+
+	logger.Printf("WARNING: Multiple cookies are required for this session as it exceeds the 4kb cookie limit. Please use server side session storage (eg. Redis) instead.")
+
 	cookies := []*http.Cookie{}
 	valueBytes := []byte(c.Value)
 	count := 0
 	for len(valueBytes) > 0 {
 		newCookie := copyCookie(c)
-		newCookie.Name = fmt.Sprintf("%s_%d", c.Name, count)
+		newCookie.Name = splitCookieName(c.Name, count)
 		count++
+
+		maxCookieLength := 4096 - len(newCookie.Name)
 		if len(valueBytes) < maxCookieLength {
 			newCookie.Value = string(valueBytes)
 			valueBytes = []byte{}
@@ -166,6 +167,15 @@ func splitCookie(c *http.Cookie) []*http.Cookie {
 	return cookies
 }
 
+func splitCookieName(name string, count int) string {
+	splitName := fmt.Sprintf("%s_%d", name, count)
+	overflow := len(splitName) - 256
+	if overflow > 0 {
+		splitName = fmt.Sprintf("%s_%d", name[:len(name)-overflow], count)
+	}
+	return splitName
+}
+
 // loadCookie retreieves the sessions state cookie from the http request.
 // If a single cookie is present this will be returned, otherwise it attempts
 // to reconstruct a cookie split up by splitCookie
@@ -179,7 +189,7 @@ func loadCookie(req *http.Request, cookieName string) (*http.Cookie, error) {
 	count := 0
 	for err == nil {
 		var c *http.Cookie
-		c, err = req.Cookie(fmt.Sprintf("%s_%d", cookieName, count))
+		c, err = req.Cookie(splitCookieName(cookieName, count))
 		if err == nil {
 			cookies = append(cookies, c)
 			count++

pkg/sessions/cookie/session_store_test.go

@@ -1,7 +1,10 @@
 package cookie
 
 import (
+	"fmt"
+	mathrand "math/rand"
 	"net/http"
+	"strings"
 	"testing"
 	"time"
 
@@ -49,3 +52,115 @@ func Test_copyCookie(t *testing.T) {
 	got := copyCookie(c)
 	assert.Equal(t, c, got)
 }
+
+func Test_splitCookie(t *testing.T) {
+	testCases := map[string]struct {
+		Cookie        *http.Cookie
+		ExpectedSizes []int
+	}{
+		"Short cookie name": {
+			Cookie: &http.Cookie{
+				Name:  "short",
+				Value: strings.Repeat("v", 10000),
+			},
+			ExpectedSizes: []int{4089, 4089, 1822},
+		},
+		"Long cookie name": {
+			Cookie: &http.Cookie{
+				Name:  strings.Repeat("n", 251),
+				Value: strings.Repeat("a", 10000),
+			},
+			ExpectedSizes: []int{3843, 3843, 2314},
+		},
+		"Max cookie name": {
+			Cookie: &http.Cookie{
+				Name:  strings.Repeat("n", 256),
+				Value: strings.Repeat("a", 10000),
+			},
+			ExpectedSizes: []int{3840, 3840, 2320},
+		},
+		"Suffix overflow cookie name": {
+			Cookie: &http.Cookie{
+				Name:  strings.Repeat("n", 255),
+				Value: strings.Repeat("a", 10000),
+			},
+			ExpectedSizes: []int{3840, 3840, 2320},
+		},
+		"Double digit suffix cookie name results in different sizes": {
+			Cookie: &http.Cookie{
+				Name:  strings.Repeat("n", 253),
+				Value: strings.Repeat("a", 50000),
+			},
+			ExpectedSizes: []int{3841, 3841, 3841, 3841, 3841, 3841, 3841, 3841, 3841, 3841,
+				3840, 3840, 3840, 70},
+		},
+		"Double digit suffix overflow cookie name results in same sizes": {
+			Cookie: &http.Cookie{
+				Name:  strings.Repeat("n", 254),
+				Value: strings.Repeat("a", 50000),
+			},
+			ExpectedSizes: []int{3840, 3840, 3840, 3840, 3840, 3840, 3840, 3840, 3840, 3840,
+				3840, 3840, 3840, 80},
+		},
+	}
+	for testName, tc := range testCases {
+		t.Run(testName, func(t *testing.T) {
+			for i, cookie := range splitCookie(tc.Cookie) {
+				assert.Equal(t, tc.ExpectedSizes[i], len(cookie.Value))
+			}
+		})
+	}
+}
+
+func Test_splitCookieName(t *testing.T) {
+	testCases := map[string]struct {
+		Name   string
+		Count  int
+		Output string
+	}{
+		"Standard length": {
+			Name:   "IAmSoNormal",
+			Count:  2,
+			Output: "IAmSoNormal_2",
+		},
+		"Max length": {
+			Name:   strings.Repeat("n", 256),
+			Count:  1,
+			Output: fmt.Sprintf("%s_%d", strings.Repeat("n", 254), 1),
+		},
+		"Large count overflow": {
+			Name:   strings.Repeat("n", 253),
+			Count:  1000,
+			Output: fmt.Sprintf("%s_%d", strings.Repeat("n", 251), 1000),
+		},
+	}
+	for testName, tc := range testCases {
+		t.Run(testName, func(t *testing.T) {
+			splitName := splitCookieName(tc.Name, tc.Count)
+			assert.Equal(t, tc.Output, splitName)
+		})
+	}
+}
+
+func Test_splitCookie_joinCookies(t *testing.T) {
+	const charset = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789"
+
+	v := make([]byte, 251)
+	for i := range v {
+		v[i] = charset[mathrand.Intn(len(charset))]
+	}
+	value := strings.Repeat(string(v), 1000)
+
+	for _, nameSize := range []int{1, 10, 50, 100, 200, 254} {
+		t.Run(fmt.Sprintf("%d length cookie name", nameSize), func(t *testing.T) {
+			cookie := &http.Cookie{
+				Name:  strings.Repeat("n", nameSize),
+				Value: value,
+			}
+			splitCookies := splitCookie(cookie)
+			joinedCookie, err := joinCookies(splitCookies)
+			assert.NoError(t, err)
+			assert.Equal(t, *cookie, *joinedCookie)
+		})
+	}
+}