Repository: coreruleset/go-ftw
Pull request #103 (2022-11-11 05:02:16)

fix: only escape key/values for data in POST requests.
When trying to update FTW, I noticed CRS tests failing and AFAICT, it is because of the escaping behavior introduced in #96. Now, there are never key/value pairs since equals and ampersands are always escaped. Ideally, the test format itself was a yaml map instead of a single string or the single string was always escaped, but as is now, I think we have to assume that `=` and `&` in the `data` field are the form parameter delimiters and only escape the actual key/values.


Patch:
diff --git a/ftwhttp/request.go b/ftwhttp/request.go
index 23a80d7..5259735 100644
--- a/ftwhttp/request.go
+++ b/ftwhttp/request.go
@@ -185,15 +185,52 @@ func buildRequest(r *Request) ([]byte, error) {
 
 // encodeDataParameters url encode parameters in data
 func encodeDataParameters(h Header, data []byte) ([]byte, error) {
+	if len(data) == 0 {
+		return nil, nil
+	}
+
 	var err error
 
 	if h.Get(ContentTypeHeader) == "application/x-www-form-urlencoded" {
+		// Best effort attempt to determine if the data is already escaped by seeing if unescaping has any effect.
 		if escapedData, err := url.QueryUnescape(string(data)); escapedData == string(data) {
 			if err != nil {
 				return nil, errors.New("Failed")
 			}
-			queryString := url.QueryEscape(string(data))
-			return []byte(queryString), nil
+
+			// CRS tests include form parameters as key=value pairs with unencoded key/values, so we encode them.
+			// If we were to encode the entire string, the equals sign would be encoded as well
+			escaped := bytes.Buffer{}
+			remaining := data
+			for len(remaining) > 0 {
+				ampIndex := bytes.IndexByte(remaining, '&')
+				var token []byte
+				if ampIndex == -1 {
+					token = remaining
+					remaining = nil
+				} else {
+					token = remaining[:ampIndex]
+					remaining = remaining[ampIndex+1:]
+				}
+
+				eqIndex := bytes.IndexByte(token, '=')
+				if eqIndex == -1 {
+					escaped.WriteString(url.QueryEscape(string(token)))
+					escaped.WriteByte('&')
+					continue
+				}
+
+				key := token[:eqIndex]
+				value := token[eqIndex+1:]
+				escaped.WriteString(url.QueryEscape(string(key)))
+				escaped.WriteByte('=')
+				escaped.WriteString(url.QueryEscape(string(value)))
+				escaped.WriteByte('&')
+			}
+
+			// Strip trailing &
+			queryString := escaped.Bytes()[:escaped.Len()-1]
+			return queryString, nil
 		}
 	}
 	return data, err


Test patch:
diff --git a/ftwhttp/request_test.go b/ftwhttp/request_test.go
index 2a15966..435b5f2 100644
--- a/ftwhttp/request_test.go
+++ b/ftwhttp/request_test.go
@@ -228,23 +228,59 @@ func TestRequestURLParseFail(t *testing.T) {
 }
 
 func TestRequestEncodesPostData(t *testing.T) {
-	req := generateBaseRequestForTesting()
-
-	h := req.Headers()
-	h.Add(ContentTypeHeader, "application/x-www-form-urlencoded")
-	// Test adding semicolons to test parse
-	err := req.SetData([]byte(`c4= ;c3=t;c2=a;c1=c;a1=/;a2=e;a3=t;a4=c;a5=/;a6=p;a7=a;a8=s;a9=s;a10=w;a11=d;$c1$c2$c3$c4$a1$a2$a3$a4$a5$a6$a7$a8$a9$a10$a11`))
-	if err != nil {
-		t.Errorf("Failed !")
-	}
-	result, err := encodeDataParameters(h, req.Data())
-	if err != nil {
-		t.Errorf("Failed to encode %s", req.Data())
+	tests := []struct {
+		raw     string
+		encoded string
+	}{
+		{
+			raw:     "",
+			encoded: "",
+		},
+		{
+			raw:     "hello=world",
+			encoded: "hello=world",
+		},
+		{
+			raw:     "foo bar",
+			encoded: "foo+bar",
+		},
+		{
+			raw:     "name=panda&food=bamboo",
+			encoded: "name=panda&food=bamboo",
+		},
+		{
+			// Test adding semicolons to test parse
+			raw:     `c4= ;c3=t;c2=a;c1=c;a1=/;a2=e;a3=t;a4=c;a5=/;a6=p;a7=a;a8=s;a9=s;a10=w;a11=d;$c1$c2$c3$c4$a1$a2$a3$a4$a5$a6$a7$a8$a9$a10$a11`,
+			encoded: "c4=+%3Bc3%3Dt%3Bc2%3Da%3Bc1%3Dc%3Ba1%3D%2F%3Ba2%3De%3Ba3%3Dt%3Ba4%3Dc%3Ba5%3D%2F%3Ba6%3Dp%3Ba7%3Da%3Ba8%3Ds%3Ba9%3Ds%3Ba10%3Dw%3Ba11%3Dd%3B%24c1%24c2%24c3%24c4%24a1%24a2%24a3%24a4%24a5%24a6%24a7%24a8%24a9%24a10%24a11",
+		},
+		{
+			// Already encoded
+			raw:     "foo+bar",
+			encoded: "foo+bar",
+		},
 	}
 
-	expected := "c4%3D+%3Bc3%3Dt%3Bc2%3Da%3Bc1%3Dc%3Ba1%3D%2F%3Ba2%3De%3Ba3%3Dt%3Ba4%3Dc%3Ba5%3D%2F%3Ba6%3Dp%3Ba7%3Da%3Ba8%3Ds%3Ba9%3Ds%3Ba10%3Dw%3Ba11%3Dd%3B%24c1%24c2%24c3%24c4%24a1%24a2%24a3%24a4%24a5%24a6%24a7%24a8%24a9%24a10%24a11"
-	actual := string(result)
-	if actual != expected {
-		t.Error("Unexpected URL encoded payload")
+	for _, tc := range tests {
+		tt := tc
+		t.Run(tt.raw, func(t *testing.T) {
+			req := generateBaseRequestForTesting()
+
+			h := req.Headers()
+			h.Add(ContentTypeHeader, "application/x-www-form-urlencoded")
+			err := req.SetData([]byte(tt.raw))
+			if err != nil {
+				t.Errorf("Failed !")
+			}
+			result, err := encodeDataParameters(h, req.Data())
+			if err != nil {
+				t.Errorf("Failed to encode %s", req.Data())
+			}
+
+			expected := tt.encoded
+			actual := string(result)
+			if actual != expected {
+				t.Errorf("Unexpected URL encoded payload, expected %s, got %s", expected, actual)
+			}
+		})
 	}
 }