Repository: coreruleset/go-ftw
Pull request #135 (2023-02-26 12:57:56)

fix: send Content-Length header if there is payload
Fixed: https://github.com/coreruleset/go-ftw/issues/134

CRS 942101-* tests received "411 Length Required" data. go-ftw sends Content-Length header if request data is not 0 byte for now.
https://github.com/coreruleset/go-ftw/blob/main/ftwhttp/header.go#L78-L80

I think the Content-Length header needed if request header's method is "POST".

camelmasa: @theseion Got it. Thanks for reviewing. I'll update them then.
camelmasa: @theseion Okay, I updated them. How is this?
theseion: Thanks! Almost perfect.
jcchavezs: > That's great, thanks. Would you mind adding 6 tests for that change? One for each of the methods with body, one for GET, and one for GET + non-empty payload (this should set the Content-Length header regardless of the method)?

Does it make a difference actually? I would just go for GET+e.g.PUT with/without body and move the regex verb check into a meaningful private method which could be tests if we really want to get 100% coverage.

Side note: What about `OPTIONS` verb?
theseion: > Does it make a difference actually (to add 6 tests)? I would just go for GET+e.g.PUT with/without body and move the regex verb check into a meaningful private method which could be tests if we really want to get 100% coverage. I am coming from the fact that we should test in isolation as much as possible and testing all 4 verbs does not seem to add more value.

I'm not a Go expert, so I'm not very familiar with idioms and best practices. My reasoning for testing each verb is that I want to know when one of them disappears. There may also come a point where we have to extend the list (e.g. for WebDAV verbs) and then I'd want an additional test for each verb.
I'm not looking for 100% coverage, that doesn't add much value, IMO :).

> Side note: What about OPTIONS verb?

`OPTIONS` has no body, so the `Content-Length` header is unnecessary.
camelmasa: @theseion I added tests https://github.com/coreruleset/go-ftw/pull/135/commits/e7b9771cb4ff9cafcdf8d9692fa5c60cf9cae580.

Also, I'm not Go expert too 😓 

camelmasa: @theseion Okay, added tests https://github.com/coreruleset/go-ftw/pull/135/commits/f496432ca7878c3250b9525ddf2ed9eb96a06b8c

Patch:
diff --git a/ftwhttp/header.go b/ftwhttp/header.go
index e34a539..ccd7ea3 100644
--- a/ftwhttp/header.go
+++ b/ftwhttp/header.go
@@ -4,7 +4,6 @@ import (
 	"bytes"
 	"io"
 	"sort"
-	"strconv"
 )
 
 const (
@@ -70,16 +69,6 @@ func (h Header) Del(key string) {
 	delete(h, key)
 }
 
-// AddStandard adds standard headers
-func (h Header) AddStandard(dataSize int) {
-	// For better performance, we always close the connection (unless otherwise)
-	h.Add("Connection", "close")
-	// If there is data, we add the length also
-	if dataSize > 0 {
-		h.Add("Content-Length", strconv.Itoa(dataSize))
-	}
-}
-
 // Write writes a header in wire format.
 func (h Header) Write(w io.Writer) error {
 	ws, ok := w.(io.StringWriter)
diff --git a/ftwhttp/request.go b/ftwhttp/request.go
index 5259735..9e3d9bf 100644
--- a/ftwhttp/request.go
+++ b/ftwhttp/request.go
@@ -5,6 +5,8 @@ import (
 	"errors"
 	"fmt"
 	"net/url"
+	"regexp"
+	"strconv"
 	"strings"
 
 	"github.com/rs/zerolog/log"
@@ -12,6 +14,8 @@ import (
 	"github.com/coreruleset/go-ftw/utils"
 )
 
+var methodsWithBodyRegex = regexp.MustCompile(`^POST|PUT|PATCH|DELETE$`)
+
 // ToString converts the request line to string for sending it in the wire
 func (rl RequestLine) ToString() string {
 	return fmt.Sprintf("%s %s %s\r\n", rl.Method, rl.URI, rl.Version)
@@ -102,9 +106,19 @@ func (r *Request) AddHeader(name string, value string) {
 
 // AddStandardHeaders adds standard headers to the request, if they don't exist
 //
-// This will add Content-Length and the proper Content-Type
-func (r *Request) AddStandardHeaders(size int) {
-	r.headers.AddStandard(size)
+// AddStandardHeaders does the following:
+//   - adds `Connection` header with `close` value (if not set) to improve performance
+//   - adds `Content-Length` header if payload size > 0 or the request method
+//     permits a body (the spec says that the client SHOULD send `Content-Length`
+//     in that case)
+func (r *Request) AddStandardHeaders() {
+	if r.headers.Get("Connection") == "" {
+		r.headers.Add("Connection", "close")
+	}
+
+	if len(r.data) > 0 || methodsWithBodyRegex.MatchString(r.requestLine.Method) {
+		r.headers.Add("Content-Length", strconv.Itoa(len(r.data)))
+	}
 }
 
 // isRaw is a helper that returns true if raw or encoded data
@@ -153,7 +167,7 @@ func buildRequest(r *Request) ([]byte, error) {
 		}
 
 		if r.WithAutoCompleteHeaders() {
-			r.AddStandardHeaders(len(r.data))
+			r.AddStandardHeaders()
 		}
 
 		err = r.Headers().WriteBytes(&b)


Test patch:
diff --git a/ftwhttp/request_test.go b/ftwhttp/request_test.go
index 435b5f2..85f2245 100644
--- a/ftwhttp/request_test.go
+++ b/ftwhttp/request_test.go
@@ -23,6 +23,70 @@ func generateBaseRequestForTesting() *Request {
 	return req
 }
 
+func TestAddStandardHeadersWhenConnectionHeaderIsPresent(t *testing.T) {
+	req := NewRequest(&RequestLine{}, Header{"Connection": "Not-Closed"}, []byte("Data"), true)
+
+	req.AddStandardHeaders()
+
+	assert.Equal(t, req.headers.Get("Connection"), "Not-Closed")
+}
+
+func TestAddStandardHeadersWhenConnectionHeaderIsEmpty(t *testing.T) {
+	req := NewRequest(&RequestLine{}, Header{}, []byte("Data"), true)
+
+	req.AddStandardHeaders()
+
+	assert.Equal(t, req.headers.Get("Connection"), "close")
+}
+
+func TestAddStandardHeadersWhenNoData(t *testing.T) {
+	req := NewRequest(&RequestLine{Method: "GET"}, Header{}, []byte(""), true)
+
+	req.AddStandardHeaders()
+
+	assert.Equal(t, req.headers.Get("Content-Length"), "")
+}
+
+func TestAddStandardHeadersWhenGetMethod(t *testing.T) {
+	req := NewRequest(&RequestLine{Method: "GET"}, Header{}, []byte("Data"), true)
+
+	req.AddStandardHeaders()
+
+	assert.Equal(t, req.headers.Get("Content-Length"), "4")
+}
+
+func TestAddStandardHeadersWhenPostMethod(t *testing.T) {
+	req := NewRequest(&RequestLine{Method: "POST"}, Header{}, []byte("Data"), true)
+
+	req.AddStandardHeaders()
+
+	assert.Equal(t, req.headers.Get("Content-Length"), "4")
+}
+
+func TestAddStandardHeadersWhenPutMethod(t *testing.T) {
+	req := NewRequest(&RequestLine{Method: "PUT"}, Header{}, []byte("Data"), true)
+
+	req.AddStandardHeaders()
+
+	assert.Equal(t, req.headers.Get("Content-Length"), "4")
+}
+
+func TestAddStandardHeadersWhenPatchMethod(t *testing.T) {
+	req := NewRequest(&RequestLine{Method: "PATCH"}, Header{}, []byte("Data"), true)
+
+	req.AddStandardHeaders()
+
+	assert.Equal(t, req.headers.Get("Content-Length"), "4")
+}
+
+func TestAddStandardHeadersWhenDeleteMethod(t *testing.T) {
+	req := NewRequest(&RequestLine{Method: "DELETE"}, Header{}, []byte("Data"), true)
+
+	req.AddStandardHeaders()
+
+	assert.Equal(t, req.headers.Get("Content-Length"), "4")
+}
+
 func TestMultipartFormDataRequest(t *testing.T) {
 	var req *Request
 
@@ -158,8 +222,6 @@ func TestRequestHeadersSet(t *testing.T) {
 	req.AddHeader("X-New-Header2", "Value")
 	head := req.Headers()
 	assert.Equal(t, "Value", head.Get("X-New-Header2"))
-
-	req.AddStandardHeaders(5)
 }
 
 func TestRequestAutoCompleteHeaders(t *testing.T) {