fix: parse multiple cookies with spaces (#943)

* fix: parse multiple cookies with spaces

---------

Signed-off-by: Felipe Zipitria <[email protected]>

Author: fzipi

Diff for: go.sum

@@ -6,8 +6,6 @@ github.com/foxcpp/go-mockdns v1.0.0 h1:7jBqxd3WDWwi/6WhDvacvH1XsN3rOLXyHM1uhvIx6
 github.com/foxcpp/go-mockdns v1.0.0/go.mod h1:lgRN6+KxQBawyIghpnl5CezHFGS9VLzvtVlwxvzXTQ4=
 github.com/magefile/mage v1.15.0 h1:BvGheCMAsG3bWUDbZ8AyXXpCNwU9u5CB6sM+HNb9HYg=
 github.com/magefile/mage v1.15.0/go.mod h1:z5UZb/iS3GoOSn0JgWuiw7dxlurVYTu+/jHXqQg881A=
-github.com/mccutchen/go-httpbin/v2 v2.12.0 h1:MPrFw/Avug0E83SN/j5SYDuD9By0GDAJ9hNTR4RwjyU=
-github.com/mccutchen/go-httpbin/v2 v2.12.0/go.mod h1:f4DUXYlU6yH0V81O4lJIwqpmYdTXXmYwzxMnYEimFPk=
 github.com/mccutchen/go-httpbin/v2 v2.13.1 h1:mDTz2RTD3tugs1BKZM7o6YJsXODYWNvjKZko30B/aWk=
 github.com/mccutchen/go-httpbin/v2 v2.13.1/go.mod h1:f4DUXYlU6yH0V81O4lJIwqpmYdTXXmYwzxMnYEimFPk=
 github.com/miekg/dns v1.1.25/go.mod h1:bPDLeHnStXmXAq1m/Ch/hvfNHr14JKNPMBo3VZKjuso=

Diff for: internal/cookies/cookies.go

@@ -0,0 +1,36 @@
+package cookies
+
+import (
+	"net/textproto"
+	"strings"
+)
+
+// ParseCookies parses cookies and splits in name, value pairs. Won't check for valid names nor values.
+// If there are multiple cookies with the same name, it will append to the list with the same name key.
+// Loosely based in the stdlib src/net/http/cookie.go
+func ParseCookies(rawCookies string) map[string][]string {
+	cookies := make(map[string][]string)
+
+	rawCookies = textproto.TrimString(rawCookies)
+
+	if rawCookies == "" {
+		return cookies
+	}
+
+	var part string
+	for len(rawCookies) > 0 { // continue since we have rest
+		part, rawCookies, _ = strings.Cut(rawCookies, ";")
+		part = textproto.TrimString(part)
+		if part == "" {
+			continue
+		}
+		name, val, _ := strings.Cut(part, "=")
+		name = textproto.TrimString(name)
+		// if name is empty (eg: "Cookie:   =foo;") skip it
+		if name == "" {
+			continue
+		}
+		cookies[name] = append(cookies[name], val)
+	}
+	return cookies
+}

Diff for: internal/cookies/cookies_test.go

@@ -0,0 +1,97 @@
+package cookies
+
+import (
+	"testing"
+)
+
+func equalMaps(map1 map[string][]string, map2 map[string][]string) bool {
+	if len(map1) != len(map2) {
+		return false
+	}
+
+	// Iterate through the key-value pairs of the first map
+	for key, slice1 := range map1 {
+		// Check if the key exists in the second map
+		slice2, ok := map2[key]
+		if !ok {
+			return false
+		}
+
+		// Compare the values of the corresponding keys
+		for i, val1 := range slice1 {
+			val2 := slice2[i]
+
+			// Compare the elements
+			if val1 != val2 {
+				return false
+			}
+		}
+	}
+
+	return true
+}
+
+func TestParseCookies(t *testing.T) {
+	type args struct {
+		rawCookies string
+	}
+	tests := []struct {
+		name string
+		args args
+		want map[string][]string
+	}{
+		{
+			name: "EmptyString",
+			args: args{rawCookies: "  "},
+			want: map[string][]string{},
+		},
+		{
+			name: "SimpleCookie",
+			args: args{rawCookies: "test=test_value"},
+			want: map[string][]string{"test": {"test_value"}},
+		},
+		{
+			name: "MultipleCookies",
+			args: args{rawCookies: "test1=test_value1; test2=test_value2"},
+			want: map[string][]string{"test1": {"test_value1"}, "test2": {"test_value2"}},
+		},
+		{
+			name: "SpacesInCookieName",
+			args: args{rawCookies: " test1  =test_value1; test2  =test_value2"},
+			want: map[string][]string{"test1": {"test_value1"}, "test2": {"test_value2"}},
+		},
+		{
+			name: "SpacesInCookieValue",
+			args: args{rawCookies: "test1=test   _value1; test2  =test_value2"},
+			want: map[string][]string{"test1": {"test   _value1"}, "test2": {"test_value2"}},
+		},
+		{
+			name: "EmptyCookie",
+			args: args{rawCookies: ";;foo=bar"},
+			want: map[string][]string{"foo": {"bar"}},
+		},
+		{
+			name: "EmptyName",
+			args: args{rawCookies: "=bar;"},
+			want: map[string][]string{},
+		},
+		{
+			name: "MultipleEqualsInValues",
+			args: args{rawCookies: "test1=val==ue1;test2=value2"},
+			want: map[string][]string{"test1": {"val==ue1"}, "test2": {"value2"}},
+		},
+		{
+			name: "RepeatedCookieNameShouldGiveList",
+			args: args{rawCookies: "test1=value1;test1=value2"},
+			want: map[string][]string{"test1": {"value1", "value2"}},
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got := ParseCookies(tt.args.rawCookies)
+			if !equalMaps(got, tt.want) {
+				t.Errorf("ParseCookies() = %v, want %v", got, tt.want)
+			}
+		})
+	}
+}

Diff for: internal/corazawaf/transaction.go

@@ -22,6 +22,7 @@ import (
 	"github.com/corazawaf/coraza/v3/internal/auditlog"
 	"github.com/corazawaf/coraza/v3/internal/bodyprocessors"
 	"github.com/corazawaf/coraza/v3/internal/collections"
+	"github.com/corazawaf/coraza/v3/internal/cookies"
 	"github.com/corazawaf/coraza/v3/internal/corazarules"
 	"github.com/corazawaf/coraza/v3/internal/corazatypes"
 	stringsutil "github.com/corazawaf/coraza/v3/internal/strings"
@@ -320,9 +321,21 @@ func (tx *Transaction) AddRequestHeader(key string, value string) {
 			tx.variables.reqbodyProcessor.Set("MULTIPART")
 		}
 	case "cookie":
-		// Cookies use the same syntax as GET params but with semicolon (;) separator
-		// WithoutUnescape is used to avoid implicitly performing an URL decode on the cookies
-		values := urlutil.ParseQueryWithoutUnescape(value, ';')
+		// 4.2.  Cookie
+		//
+		// 4.2.1.  Syntax
+		//
+		//   The user agent sends stored cookies to the origin server in the
+		//   Cookie header.  If the server conforms to the requirements in
+		//   Section 4.1 (and the user agent conforms to the requirements in
+		//   Section 5), the user agent will send a Cookie header that conforms to
+		//   the following grammar:
+		//
+		//   cookie-header = "Cookie:" OWS cookie-string OWS
+		//   cookie-string = cookie-pair *( ";" SP cookie-pair )
+		//
+		// There is no URL Decode performed no the cookies
+		values := cookies.ParseCookies(value)
 		for k, vr := range values {
 			for _, v := range vr {
 				tx.variables.requestCookies.Add(k, v)

Diff for: internal/corazawaf/transaction_test.go

@@ -835,6 +835,28 @@ func TestCookiesNotUrldecoded(t *testing.T) {
 	}
 }
 
+func TestMultipleCookiesWithSpaceBetweenThem(t *testing.T) {
+	waf := NewWAF()
+	tx := waf.NewTransaction()
+	multipleCookies := "cookie1=value1; cookie2=value2;    cookie1=value2"
+	tx.AddRequestHeader("cookie", multipleCookies)
+	v11 := tx.variables.requestCookies.Get("cookie1")[0]
+	if v11 != "value1" {
+		t.Errorf("failed to set cookie, got %q", v11)
+	}
+	v12 := tx.variables.requestCookies.Get("cookie1")[1]
+	if v12 != "value2" {
+		t.Errorf("failed to set cookie, got %q", v12)
+	}
+	v2 := tx.variables.requestCookies.Get("cookie2")[0]
+	if v2 != "value2" {
+		t.Errorf("failed to set cookie, got %q", v2)
+	}
+	if err := tx.Close(); err != nil {
+		t.Error(err)
+	}
+}
+
 func collectionValues(t *testing.T, col collection.Collection) []string {
 	t.Helper()
 	var values []string

Diff for: internal/url/url.go

@@ -13,11 +13,6 @@ func ParseQuery(query string, separator byte) map[string][]string {
 	return doParseQuery(query, separator, true)
 }
 
-// ParseQueryWithoutUnescape is a sibling of ParseQuery, but without performing URL unescape of keys and values.
-func ParseQueryWithoutUnescape(query string, separator byte) map[string][]string {
-	return doParseQuery(query, separator, false)
-}
-
 func doParseQuery(query string, separator byte, urlUnescape bool) map[string][]string {
 	m := make(map[string][]string)
 	for query != "" {